diff mbox series

[FFmpeg-devel] avformat/libamqp: add option vhost

Message ID 20201217203422.43071-1-levis.florian@gmail.com
State Withdrawn
Headers show
Series [FFmpeg-devel] avformat/libamqp: add option vhost
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate warning Make fate failed

Commit Message

Florian Levis Dec. 17, 2020, 8:34 p.m. UTC
From: Florian Levis <flevis@hubee.tv>

Add option vhost to allow publishing on other
vhost than default '/'

Signed-off-by: Florian Levis <levis.florian@gmail.com>
---
 doc/protocols.texi    | 3 +++
 libavformat/libamqp.c | 4 +++-
 2 files changed, 6 insertions(+), 1 deletion(-)

Comments

Andriy Gelman Dec. 18, 2020, 9:06 p.m. UTC | #1
On Thu, 17. Dec 21:34, Florian Levis wrote:
> From: Florian Levis <flevis@hubee.tv>
> 
> Add option vhost to allow publishing on other
> vhost than default '/'
> 
> Signed-off-by: Florian Levis <levis.florian@gmail.com>
> ---
>  doc/protocols.texi    | 3 +++
>  libavformat/libamqp.c | 4 +++-
>  2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/doc/protocols.texi b/doc/protocols.texi
> index b4efa14509..8c7e4c7c52 100644
> --- a/doc/protocols.texi
> +++ b/doc/protocols.texi
> @@ -84,6 +84,9 @@ The following options are supported:
>  
>  @table @option
>  
> +@item vhost
> +Sets the vhost to use on the broker. Default is "/".
> +
>  @item exchange
>  Sets the exchange to use on the broker. RabbitMQ has several predefined
>  exchanges: "amq.direct" is the default exchange, where the publisher and
> diff --git a/libavformat/libamqp.c b/libavformat/libamqp.c
> index 81df724a6d..1465a4a133 100644
> --- a/libavformat/libamqp.c
> +++ b/libavformat/libamqp.c
> @@ -34,6 +34,7 @@ typedef struct AMQPContext {
>      const AVClass *class;
>      amqp_connection_state_t conn;
>      amqp_socket_t *socket;
> +    const char *vhost;
>      const char *exchange;
>      const char *routing_key;
>      int pkt_size;
> @@ -50,6 +51,7 @@ typedef struct AMQPContext {
>  #define E AV_OPT_FLAG_ENCODING_PARAM
>  static const AVOption options[] = {
>      { "pkt_size", "Maximum send/read packet size", OFFSET(pkt_size), AV_OPT_TYPE_INT, { .i64 = 131072 }, 4096, INT_MAX, .flags = D | E },

> +    { "vhost", "vhost to send/read packets", OFFSET(vhost), AV_OPT_TYPE_STRING, { .str = "/" }, 0, 0, .flags = D | E },

I'll change the description to
"Name of virtual host on broker"

>      { "exchange", "Exchange to send/read packets", OFFSET(exchange), AV_OPT_TYPE_STRING, { .str = "amq.direct" }, 0, 0, .flags = D | E },
>      { "routing_key", "Key to filter streams", OFFSET(routing_key), AV_OPT_TYPE_STRING, { .str = "amqp" }, 0, 0, .flags = D | E },
>      { "connection_timeout", "Initial connection timeout", OFFSET(connection_timeout), AV_OPT_TYPE_DURATION, { .i64 = -1 }, -1, INT64_MAX, .flags = D | E},
> @@ -136,7 +138,7 @@ static int amqp_proto_open(URLContext *h, const char *uri, int flags)
>          goto destroy_connection;
>      }
>  
> -    broker_reply = amqp_login(s->conn, "/", 0, s->pkt_size, 0,
> +    broker_reply = amqp_login(s->conn, s->vhost, 0, s->pkt_size, 0,
>                                AMQP_SASL_METHOD_PLAIN, user_decoded, password_decoded);
>  
>      if (broker_reply.reply_type != AMQP_RESPONSE_NORMAL) {

Will apply in the next few days.

Thanks,
--
Andriy
Florian Levis Dec. 19, 2020, 9:44 a.m. UTC | #2
Thanks

Le ven. 18 déc. 2020 à 22:08, Andriy Gelman <andriy.gelman@gmail.com> a
écrit :

> On Thu, 17. Dec 21:34, Florian Levis wrote:
> > From: Florian Levis <flevis@hubee.tv>
> >
> > Add option vhost to allow publishing on other
> > vhost than default '/'
> >
> > Signed-off-by: Florian Levis <levis.florian@gmail.com>
> > ---
> >  doc/protocols.texi    | 3 +++
> >  libavformat/libamqp.c | 4 +++-
> >  2 files changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/doc/protocols.texi b/doc/protocols.texi
> > index b4efa14509..8c7e4c7c52 100644
> > --- a/doc/protocols.texi
> > +++ b/doc/protocols.texi
> > @@ -84,6 +84,9 @@ The following options are supported:
> >
> >  @table @option
> >
> > +@item vhost
> > +Sets the vhost to use on the broker. Default is "/".
> > +
> >  @item exchange
> >  Sets the exchange to use on the broker. RabbitMQ has several predefined
> >  exchanges: "amq.direct" is the default exchange, where the publisher and
> > diff --git a/libavformat/libamqp.c b/libavformat/libamqp.c
> > index 81df724a6d..1465a4a133 100644
> > --- a/libavformat/libamqp.c
> > +++ b/libavformat/libamqp.c
> > @@ -34,6 +34,7 @@ typedef struct AMQPContext {
> >      const AVClass *class;
> >      amqp_connection_state_t conn;
> >      amqp_socket_t *socket;
> > +    const char *vhost;
> >      const char *exchange;
> >      const char *routing_key;
> >      int pkt_size;
> > @@ -50,6 +51,7 @@ typedef struct AMQPContext {
> >  #define E AV_OPT_FLAG_ENCODING_PARAM
> >  static const AVOption options[] = {
> >      { "pkt_size", "Maximum send/read packet size", OFFSET(pkt_size),
> AV_OPT_TYPE_INT, { .i64 = 131072 }, 4096, INT_MAX, .flags = D | E },
>
> > +    { "vhost", "vhost to send/read packets", OFFSET(vhost),
> AV_OPT_TYPE_STRING, { .str = "/" }, 0, 0, .flags = D | E },
>
> I'll change the description to
> "Name of virtual host on broker"
>
> >      { "exchange", "Exchange to send/read packets", OFFSET(exchange),
> AV_OPT_TYPE_STRING, { .str = "amq.direct" }, 0, 0, .flags = D | E },
> >      { "routing_key", "Key to filter streams", OFFSET(routing_key),
> AV_OPT_TYPE_STRING, { .str = "amqp" }, 0, 0, .flags = D | E },
> >      { "connection_timeout", "Initial connection timeout",
> OFFSET(connection_timeout), AV_OPT_TYPE_DURATION, { .i64 = -1 }, -1,
> INT64_MAX, .flags = D | E},
> > @@ -136,7 +138,7 @@ static int amqp_proto_open(URLContext *h, const char
> *uri, int flags)
> >          goto destroy_connection;
> >      }
> >
> > -    broker_reply = amqp_login(s->conn, "/", 0, s->pkt_size, 0,
> > +    broker_reply = amqp_login(s->conn, s->vhost, 0, s->pkt_size, 0,
> >                                AMQP_SASL_METHOD_PLAIN, user_decoded,
> password_decoded);
> >
> >      if (broker_reply.reply_type != AMQP_RESPONSE_NORMAL) {
>
> Will apply in the next few days.
>
> Thanks,
> --
> Andriy
>
Marton Balint Dec. 19, 2020, 10:54 a.m. UTC | #3
On Fri, 18 Dec 2020, Andriy Gelman wrote:

> On Thu, 17. Dec 21:34, Florian Levis wrote:
>> From: Florian Levis <flevis@hubee.tv>
>> 
>> Add option vhost to allow publishing on other
>> vhost than default '/'
>> 
>> Signed-off-by: Florian Levis <levis.florian@gmail.com>
>> ---
>>  doc/protocols.texi    | 3 +++
>>  libavformat/libamqp.c | 4 +++-
>>  2 files changed, 6 insertions(+), 1 deletion(-)
>> 
>> diff --git a/doc/protocols.texi b/doc/protocols.texi
>> index b4efa14509..8c7e4c7c52 100644
>> --- a/doc/protocols.texi
>> +++ b/doc/protocols.texi
>> @@ -84,6 +84,9 @@ The following options are supported:
>>
>>  @table @option
>> 
>> +@item vhost
>> +Sets the vhost to use on the broker. Default is "/".
>> +
>>  @item exchange
>>  Sets the exchange to use on the broker. RabbitMQ has several predefined
>>  exchanges: "amq.direct" is the default exchange, where the publisher and
>> diff --git a/libavformat/libamqp.c b/libavformat/libamqp.c
>> index 81df724a6d..1465a4a133 100644
>> --- a/libavformat/libamqp.c
>> +++ b/libavformat/libamqp.c
>> @@ -34,6 +34,7 @@ typedef struct AMQPContext {
>>      const AVClass *class;
>>      amqp_connection_state_t conn;
>>      amqp_socket_t *socket;
>> +    const char *vhost;
>>      const char *exchange;
>>      const char *routing_key;
>>      int pkt_size;
>> @@ -50,6 +51,7 @@ typedef struct AMQPContext {
>>  #define E AV_OPT_FLAG_ENCODING_PARAM
>>  static const AVOption options[] = {
>>      { "pkt_size", "Maximum send/read packet size", OFFSET(pkt_size), AV_OPT_TYPE_INT, { .i64 = 131072 }, 4096, INT_MAX, .flags = D | E },
>
>> +    { "vhost", "vhost to send/read packets", OFFSET(vhost), AV_OPT_TYPE_STRING, { .str = "/" }, 0, 0, .flags = D | E },
>
> I'll change the description to
> "Name of virtual host on broker"
>
>>      { "exchange", "Exchange to send/read packets", OFFSET(exchange), AV_OPT_TYPE_STRING, { .str = "amq.direct" }, 0, 0, .flags = D | E },
>>      { "routing_key", "Key to filter streams", OFFSET(routing_key), AV_OPT_TYPE_STRING, { .str = "amqp" }, 0, 0, .flags = D | E },
>>      { "connection_timeout", "Initial connection timeout", OFFSET(connection_timeout), AV_OPT_TYPE_DURATION, { .i64 = -1 }, -1, INT64_MAX, .flags = D | E},
>> @@ -136,7 +138,7 @@ static int amqp_proto_open(URLContext *h, const char *uri, int flags)
>>          goto destroy_connection;
>>      }
>> 
>> -    broker_reply = amqp_login(s->conn, "/", 0, s->pkt_size, 0,
>> +    broker_reply = amqp_login(s->conn, s->vhost, 0, s->pkt_size, 0,
>>                                AMQP_SASL_METHOD_PLAIN, user_decoded, password_decoded);
>>
>>      if (broker_reply.reply_type != AMQP_RESPONSE_NORMAL) {
>
> Will apply in the next few days.

I think it is much better approach to parse the amqp URL to get the vhost 
instead of adding a new option.

https://www.rabbitmq.com/uri-spec.html

amqp_URI       = "amqp://" amqp_authority [ "/" vhost ] [ "?" query ]

Regards,
Marton
Andriy Gelman Dec. 19, 2020, 3:43 p.m. UTC | #4
On Sat, 19. Dec 11:54, Marton Balint wrote:
> 
> 
> On Fri, 18 Dec 2020, Andriy Gelman wrote:
> 
> > On Thu, 17. Dec 21:34, Florian Levis wrote:
> > > From: Florian Levis <flevis@hubee.tv>
> > > 
> > > Add option vhost to allow publishing on other
> > > vhost than default '/'
> > > 
> > > Signed-off-by: Florian Levis <levis.florian@gmail.com>
> > > ---
> > >  doc/protocols.texi    | 3 +++
> > >  libavformat/libamqp.c | 4 +++-
> > >  2 files changed, 6 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/doc/protocols.texi b/doc/protocols.texi
> > > index b4efa14509..8c7e4c7c52 100644
> > > --- a/doc/protocols.texi
> > > +++ b/doc/protocols.texi
> > > @@ -84,6 +84,9 @@ The following options are supported:
> > > 
> > >  @table @option
> > > 
> > > +@item vhost
> > > +Sets the vhost to use on the broker. Default is "/".
> > > +
> > >  @item exchange
> > >  Sets the exchange to use on the broker. RabbitMQ has several predefined
> > >  exchanges: "amq.direct" is the default exchange, where the publisher and
> > > diff --git a/libavformat/libamqp.c b/libavformat/libamqp.c
> > > index 81df724a6d..1465a4a133 100644
> > > --- a/libavformat/libamqp.c
> > > +++ b/libavformat/libamqp.c
> > > @@ -34,6 +34,7 @@ typedef struct AMQPContext {
> > >      const AVClass *class;
> > >      amqp_connection_state_t conn;
> > >      amqp_socket_t *socket;
> > > +    const char *vhost;
> > >      const char *exchange;
> > >      const char *routing_key;
> > >      int pkt_size;
> > > @@ -50,6 +51,7 @@ typedef struct AMQPContext {
> > >  #define E AV_OPT_FLAG_ENCODING_PARAM
> > >  static const AVOption options[] = {
> > >      { "pkt_size", "Maximum send/read packet size", OFFSET(pkt_size), AV_OPT_TYPE_INT, { .i64 = 131072 }, 4096, INT_MAX, .flags = D | E },
> > 
> > > +    { "vhost", "vhost to send/read packets", OFFSET(vhost), AV_OPT_TYPE_STRING, { .str = "/" }, 0, 0, .flags = D | E },
> > 
> > I'll change the description to
> > "Name of virtual host on broker"
> > 
> > >      { "exchange", "Exchange to send/read packets", OFFSET(exchange), AV_OPT_TYPE_STRING, { .str = "amq.direct" }, 0, 0, .flags = D | E },
> > >      { "routing_key", "Key to filter streams", OFFSET(routing_key), AV_OPT_TYPE_STRING, { .str = "amqp" }, 0, 0, .flags = D | E },
> > >      { "connection_timeout", "Initial connection timeout", OFFSET(connection_timeout), AV_OPT_TYPE_DURATION, { .i64 = -1 }, -1, INT64_MAX, .flags = D | E},
> > > @@ -136,7 +138,7 @@ static int amqp_proto_open(URLContext *h, const char *uri, int flags)
> > >          goto destroy_connection;
> > >      }
> > > 
> > > -    broker_reply = amqp_login(s->conn, "/", 0, s->pkt_size, 0,
> > > +    broker_reply = amqp_login(s->conn, s->vhost, 0, s->pkt_size, 0,
> > >                                AMQP_SASL_METHOD_PLAIN, user_decoded, password_decoded);
> > > 
> > >      if (broker_reply.reply_type != AMQP_RESPONSE_NORMAL) {
> > 
> > Will apply in the next few days.
> 
> I think it is much better approach to parse the amqp URL to get the vhost
> instead of adding a new option.
> 
> https://www.rabbitmq.com/uri-spec.html
> 
> amqp_URI       = "amqp://" amqp_authority [ "/" vhost ] [ "?" query ]

I agree, it does look a better approach. 

--
Andriy
Florian Levis Dec. 19, 2020, 5:41 p.m. UTC | #5
I have no idea how to do it.
I can take a look ; but I'm really not sure how to do it.

If one of you can handle it it might be safer ; if you can't, I will try.
Just let me know.

Thanks.

--
Florian LEVIS


Le sam. 19 déc. 2020 à 16:45, Andriy Gelman <andriy.gelman@gmail.com> a
écrit :

> On Sat, 19. Dec 11:54, Marton Balint wrote:
> >
> >
> > On Fri, 18 Dec 2020, Andriy Gelman wrote:
> >
> > > On Thu, 17. Dec 21:34, Florian Levis wrote:
> > > > From: Florian Levis <flevis@hubee.tv>
> > > >
> > > > Add option vhost to allow publishing on other
> > > > vhost than default '/'
> > > >
> > > > Signed-off-by: Florian Levis <levis.florian@gmail.com>
> > > > ---
> > > >  doc/protocols.texi    | 3 +++
> > > >  libavformat/libamqp.c | 4 +++-
> > > >  2 files changed, 6 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/doc/protocols.texi b/doc/protocols.texi
> > > > index b4efa14509..8c7e4c7c52 100644
> > > > --- a/doc/protocols.texi
> > > > +++ b/doc/protocols.texi
> > > > @@ -84,6 +84,9 @@ The following options are supported:
> > > >
> > > >  @table @option
> > > >
> > > > +@item vhost
> > > > +Sets the vhost to use on the broker. Default is "/".
> > > > +
> > > >  @item exchange
> > > >  Sets the exchange to use on the broker. RabbitMQ has several
> predefined
> > > >  exchanges: "amq.direct" is the default exchange, where the
> publisher and
> > > > diff --git a/libavformat/libamqp.c b/libavformat/libamqp.c
> > > > index 81df724a6d..1465a4a133 100644
> > > > --- a/libavformat/libamqp.c
> > > > +++ b/libavformat/libamqp.c
> > > > @@ -34,6 +34,7 @@ typedef struct AMQPContext {
> > > >      const AVClass *class;
> > > >      amqp_connection_state_t conn;
> > > >      amqp_socket_t *socket;
> > > > +    const char *vhost;
> > > >      const char *exchange;
> > > >      const char *routing_key;
> > > >      int pkt_size;
> > > > @@ -50,6 +51,7 @@ typedef struct AMQPContext {
> > > >  #define E AV_OPT_FLAG_ENCODING_PARAM
> > > >  static const AVOption options[] = {
> > > >      { "pkt_size", "Maximum send/read packet size",
> OFFSET(pkt_size), AV_OPT_TYPE_INT, { .i64 = 131072 }, 4096, INT_MAX, .flags
> = D | E },
> > >
> > > > +    { "vhost", "vhost to send/read packets", OFFSET(vhost),
> AV_OPT_TYPE_STRING, { .str = "/" }, 0, 0, .flags = D | E },
> > >
> > > I'll change the description to
> > > "Name of virtual host on broker"
> > >
> > > >      { "exchange", "Exchange to send/read packets",
> OFFSET(exchange), AV_OPT_TYPE_STRING, { .str = "amq.direct" }, 0, 0, .flags
> = D | E },
> > > >      { "routing_key", "Key to filter streams", OFFSET(routing_key),
> AV_OPT_TYPE_STRING, { .str = "amqp" }, 0, 0, .flags = D | E },
> > > >      { "connection_timeout", "Initial connection timeout",
> OFFSET(connection_timeout), AV_OPT_TYPE_DURATION, { .i64 = -1 }, -1,
> INT64_MAX, .flags = D | E},
> > > > @@ -136,7 +138,7 @@ static int amqp_proto_open(URLContext *h, const
> char *uri, int flags)
> > > >          goto destroy_connection;
> > > >      }
> > > >
> > > > -    broker_reply = amqp_login(s->conn, "/", 0, s->pkt_size, 0,
> > > > +    broker_reply = amqp_login(s->conn, s->vhost, 0, s->pkt_size, 0,
> > > >                                AMQP_SASL_METHOD_PLAIN, user_decoded,
> password_decoded);
> > > >
> > > >      if (broker_reply.reply_type != AMQP_RESPONSE_NORMAL) {
> > >
> > > Will apply in the next few days.
> >
> > I think it is much better approach to parse the amqp URL to get the vhost
> > instead of adding a new option.
> >
> > https://www.rabbitmq.com/uri-spec.html
> >
> > amqp_URI       = "amqp://" amqp_authority [ "/" vhost ] [ "?" query ]
>
> I agree, it does look a better approach.
>
> --
> Andriy
>
Florian Levis Dec. 19, 2020, 6:53 p.m. UTC | #6
I use AMQP in PHP, .NET and also Java/Kotlin.
All clients have their way to handle the "options" ; usually they handle it
via the URI
It seems to be simplifier to handle all options related to a "connexion"
via an URI ; I see the same for e-mail & database connections strings (in
Symfony or Laravel for example).

So, should all the other already supported options (pkt_size, exchange,
routing_key, connection_timeout, delivery_mode) be passed in the URI?

--
Florian LEVIS


Le sam. 19 déc. 2020 à 18:41, Florian Levis <levis.florian@gmail.com> a
écrit :

> I have no idea how to do it.
> I can take a look ; but I'm really not sure how to do it.
>
> If one of you can handle it it might be safer ; if you can't, I will try.
> Just let me know.
>
> Thanks.
>
> --
> Florian LEVIS
>
>
> Le sam. 19 déc. 2020 à 16:45, Andriy Gelman <andriy.gelman@gmail.com> a
> écrit :
>
>> On Sat, 19. Dec 11:54, Marton Balint wrote:
>> >
>> >
>> > On Fri, 18 Dec 2020, Andriy Gelman wrote:
>> >
>> > > On Thu, 17. Dec 21:34, Florian Levis wrote:
>> > > > From: Florian Levis <flevis@hubee.tv>
>> > > >
>> > > > Add option vhost to allow publishing on other
>> > > > vhost than default '/'
>> > > >
>> > > > Signed-off-by: Florian Levis <levis.florian@gmail.com>
>> > > > ---
>> > > >  doc/protocols.texi    | 3 +++
>> > > >  libavformat/libamqp.c | 4 +++-
>> > > >  2 files changed, 6 insertions(+), 1 deletion(-)
>> > > >
>> > > > diff --git a/doc/protocols.texi b/doc/protocols.texi
>> > > > index b4efa14509..8c7e4c7c52 100644
>> > > > --- a/doc/protocols.texi
>> > > > +++ b/doc/protocols.texi
>> > > > @@ -84,6 +84,9 @@ The following options are supported:
>> > > >
>> > > >  @table @option
>> > > >
>> > > > +@item vhost
>> > > > +Sets the vhost to use on the broker. Default is "/".
>> > > > +
>> > > >  @item exchange
>> > > >  Sets the exchange to use on the broker. RabbitMQ has several
>> predefined
>> > > >  exchanges: "amq.direct" is the default exchange, where the
>> publisher and
>> > > > diff --git a/libavformat/libamqp.c b/libavformat/libamqp.c
>> > > > index 81df724a6d..1465a4a133 100644
>> > > > --- a/libavformat/libamqp.c
>> > > > +++ b/libavformat/libamqp.c
>> > > > @@ -34,6 +34,7 @@ typedef struct AMQPContext {
>> > > >      const AVClass *class;
>> > > >      amqp_connection_state_t conn;
>> > > >      amqp_socket_t *socket;
>> > > > +    const char *vhost;
>> > > >      const char *exchange;
>> > > >      const char *routing_key;
>> > > >      int pkt_size;
>> > > > @@ -50,6 +51,7 @@ typedef struct AMQPContext {
>> > > >  #define E AV_OPT_FLAG_ENCODING_PARAM
>> > > >  static const AVOption options[] = {
>> > > >      { "pkt_size", "Maximum send/read packet size",
>> OFFSET(pkt_size), AV_OPT_TYPE_INT, { .i64 = 131072 }, 4096, INT_MAX, .flags
>> = D | E },
>> > >
>> > > > +    { "vhost", "vhost to send/read packets", OFFSET(vhost),
>> AV_OPT_TYPE_STRING, { .str = "/" }, 0, 0, .flags = D | E },
>> > >
>> > > I'll change the description to
>> > > "Name of virtual host on broker"
>> > >
>> > > >      { "exchange", "Exchange to send/read packets",
>> OFFSET(exchange), AV_OPT_TYPE_STRING, { .str = "amq.direct" }, 0, 0, .flags
>> = D | E },
>> > > >      { "routing_key", "Key to filter streams", OFFSET(routing_key),
>> AV_OPT_TYPE_STRING, { .str = "amqp" }, 0, 0, .flags = D | E },
>> > > >      { "connection_timeout", "Initial connection timeout",
>> OFFSET(connection_timeout), AV_OPT_TYPE_DURATION, { .i64 = -1 }, -1,
>> INT64_MAX, .flags = D | E},
>> > > > @@ -136,7 +138,7 @@ static int amqp_proto_open(URLContext *h, const
>> char *uri, int flags)
>> > > >          goto destroy_connection;
>> > > >      }
>> > > >
>> > > > -    broker_reply = amqp_login(s->conn, "/", 0, s->pkt_size, 0,
>> > > > +    broker_reply = amqp_login(s->conn, s->vhost, 0, s->pkt_size, 0,
>> > > >                                AMQP_SASL_METHOD_PLAIN,
>> user_decoded, password_decoded);
>> > > >
>> > > >      if (broker_reply.reply_type != AMQP_RESPONSE_NORMAL) {
>> > >
>> > > Will apply in the next few days.
>> >
>> > I think it is much better approach to parse the amqp URL to get the
>> vhost
>> > instead of adding a new option.
>> >
>> > https://www.rabbitmq.com/uri-spec.html
>> >
>> > amqp_URI       = "amqp://" amqp_authority [ "/" vhost ] [ "?" query ]
>>
>> I agree, it does look a better approach.
>>
>> --
>> Andriy
>>
>
Marton Balint Dec. 19, 2020, 11:54 p.m. UTC | #7
On Sat, 19 Dec 2020, Florian Levis wrote:

> I use AMQP in PHP, .NET and also Java/Kotlin.
> All clients have their way to handle the "options" ; usually they handle it
> via the URI
> It seems to be simplifier to handle all options related to a "connexion"
> via an URI ; I see the same for e-mail & database connections strings (in
> Symfony or Laravel for example).
>
> So, should all the other already supported options (pkt_size, exchange,
> routing_key, connection_timeout, delivery_mode) be passed in the URI?

Personally I don't really like this approach, but the main reason for that 
is that - unlike regular AVOptions which are parsed by the 
framework automatically - parsing them is a huge pain and extra code.

It might make sense to add support for the "official" query parameters:
https://www.rabbitmq.com/uri-query-parameters.html

But I'd rather not clutter existing code with adding URL option parsing 
for all the normal options we have.

Keep in mind that others may have other opinion about this, the ffmpeg 
codebase is not very consistent about which options can appear as URL 
parameters and which can not.

Regards,
Marton

>
> --
> Florian LEVIS
>
>
> Le sam. 19 déc. 2020 à 18:41, Florian Levis <levis.florian@gmail.com> a
> écrit :
>
>> I have no idea how to do it.
>> I can take a look ; but I'm really not sure how to do it.
>>
>> If one of you can handle it it might be safer ; if you can't, I will try.
>> Just let me know.
>>
>> Thanks.
>>
>> --
>> Florian LEVIS
>>
>>
>> Le sam. 19 déc. 2020 à 16:45, Andriy Gelman <andriy.gelman@gmail.com> a
>> écrit :
>>
>>> On Sat, 19. Dec 11:54, Marton Balint wrote:
>>> >
>>> >
>>> > On Fri, 18 Dec 2020, Andriy Gelman wrote:
>>> >
>>> > > On Thu, 17. Dec 21:34, Florian Levis wrote:
>>> > > > From: Florian Levis <flevis@hubee.tv>
>>> > > >
>>> > > > Add option vhost to allow publishing on other
>>> > > > vhost than default '/'
>>> > > >
>>> > > > Signed-off-by: Florian Levis <levis.florian@gmail.com>
>>> > > > ---
>>> > > >  doc/protocols.texi    | 3 +++
>>> > > >  libavformat/libamqp.c | 4 +++-
>>> > > >  2 files changed, 6 insertions(+), 1 deletion(-)
>>> > > >
>>> > > > diff --git a/doc/protocols.texi b/doc/protocols.texi
>>> > > > index b4efa14509..8c7e4c7c52 100644
>>> > > > --- a/doc/protocols.texi
>>> > > > +++ b/doc/protocols.texi
>>> > > > @@ -84,6 +84,9 @@ The following options are supported:
>>> > > >
>>> > > >  @table @option
>>> > > >
>>> > > > +@item vhost
>>> > > > +Sets the vhost to use on the broker. Default is "/".
>>> > > > +
>>> > > >  @item exchange
>>> > > >  Sets the exchange to use on the broker. RabbitMQ has several
>>> predefined
>>> > > >  exchanges: "amq.direct" is the default exchange, where the
>>> publisher and
>>> > > > diff --git a/libavformat/libamqp.c b/libavformat/libamqp.c
>>> > > > index 81df724a6d..1465a4a133 100644
>>> > > > --- a/libavformat/libamqp.c
>>> > > > +++ b/libavformat/libamqp.c
>>> > > > @@ -34,6 +34,7 @@ typedef struct AMQPContext {
>>> > > >      const AVClass *class;
>>> > > >      amqp_connection_state_t conn;
>>> > > >      amqp_socket_t *socket;
>>> > > > +    const char *vhost;
>>> > > >      const char *exchange;
>>> > > >      const char *routing_key;
>>> > > >      int pkt_size;
>>> > > > @@ -50,6 +51,7 @@ typedef struct AMQPContext {
>>> > > >  #define E AV_OPT_FLAG_ENCODING_PARAM
>>> > > >  static const AVOption options[] = {
>>> > > >      { "pkt_size", "Maximum send/read packet size",
>>> OFFSET(pkt_size), AV_OPT_TYPE_INT, { .i64 = 131072 }, 4096, INT_MAX, .flags
>>> = D | E },
>>> > >
>>> > > > +    { "vhost", "vhost to send/read packets", OFFSET(vhost),
>>> AV_OPT_TYPE_STRING, { .str = "/" }, 0, 0, .flags = D | E },
>>> > >
>>> > > I'll change the description to
>>> > > "Name of virtual host on broker"
>>> > >
>>> > > >      { "exchange", "Exchange to send/read packets",
>>> OFFSET(exchange), AV_OPT_TYPE_STRING, { .str = "amq.direct" }, 0, 0, .flags
>>> = D | E },
>>> > > >      { "routing_key", "Key to filter streams", OFFSET(routing_key),
>>> AV_OPT_TYPE_STRING, { .str = "amqp" }, 0, 0, .flags = D | E },
>>> > > >      { "connection_timeout", "Initial connection timeout",
>>> OFFSET(connection_timeout), AV_OPT_TYPE_DURATION, { .i64 = -1 }, -1,
>>> INT64_MAX, .flags = D | E},
>>> > > > @@ -136,7 +138,7 @@ static int amqp_proto_open(URLContext *h, const
>>> char *uri, int flags)
>>> > > >          goto destroy_connection;
>>> > > >      }
>>> > > >
>>> > > > -    broker_reply = amqp_login(s->conn, "/", 0, s->pkt_size, 0,
>>> > > > +    broker_reply = amqp_login(s->conn, s->vhost, 0, s->pkt_size, 0,
>>> > > >                                AMQP_SASL_METHOD_PLAIN,
>>> user_decoded, password_decoded);
>>> > > >
>>> > > >      if (broker_reply.reply_type != AMQP_RESPONSE_NORMAL) {
>>> > >
>>> > > Will apply in the next few days.
>>> >
>>> > I think it is much better approach to parse the amqp URL to get the
>>> vhost
>>> > instead of adding a new option.
>>> >
>>> > https://www.rabbitmq.com/uri-spec.html
>>> >
>>> > amqp_URI       = "amqp://" amqp_authority [ "/" vhost ] [ "?" query ]
>>>
>>> I agree, it does look a better approach.
>>>
>>> --
>>> Andriy
>>>
>>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Andriy Gelman Dec. 20, 2020, 1:28 a.m. UTC | #8
On Sat, 19. Dec 18:41, Florian Levis wrote:
> I have no idea how to do it.
> I can take a look ; but I'm really not sure how to do it.
> 
> If one of you can handle it it might be safer ; if you can't, I will try.
> Just let me know.

I can send in the patch.

--
Andriy
Florian Levis Dec. 20, 2020, 12:56 p.m. UTC | #9
Thanks Marton for the clarification
Thanks Andriy

--
Florian LEVIS


Le dim. 20 déc. 2020 à 02:30, Andriy Gelman <andriy.gelman@gmail.com> a
écrit :

> On Sat, 19. Dec 18:41, Florian Levis wrote:
> > I have no idea how to do it.
> > I can take a look ; but I'm really not sure how to do it.
> >
> > If one of you can handle it it might be safer ; if you can't, I will try.
> > Just let me know.
>
> I can send in the patch.
>
> --
> Andriy
>
diff mbox series

Patch

diff --git a/doc/protocols.texi b/doc/protocols.texi
index b4efa14509..8c7e4c7c52 100644
--- a/doc/protocols.texi
+++ b/doc/protocols.texi
@@ -84,6 +84,9 @@  The following options are supported:
 
 @table @option
 
+@item vhost
+Sets the vhost to use on the broker. Default is "/".
+
 @item exchange
 Sets the exchange to use on the broker. RabbitMQ has several predefined
 exchanges: "amq.direct" is the default exchange, where the publisher and
diff --git a/libavformat/libamqp.c b/libavformat/libamqp.c
index 81df724a6d..1465a4a133 100644
--- a/libavformat/libamqp.c
+++ b/libavformat/libamqp.c
@@ -34,6 +34,7 @@  typedef struct AMQPContext {
     const AVClass *class;
     amqp_connection_state_t conn;
     amqp_socket_t *socket;
+    const char *vhost;
     const char *exchange;
     const char *routing_key;
     int pkt_size;
@@ -50,6 +51,7 @@  typedef struct AMQPContext {
 #define E AV_OPT_FLAG_ENCODING_PARAM
 static const AVOption options[] = {
     { "pkt_size", "Maximum send/read packet size", OFFSET(pkt_size), AV_OPT_TYPE_INT, { .i64 = 131072 }, 4096, INT_MAX, .flags = D | E },
+    { "vhost", "vhost to send/read packets", OFFSET(vhost), AV_OPT_TYPE_STRING, { .str = "/" }, 0, 0, .flags = D | E },
     { "exchange", "Exchange to send/read packets", OFFSET(exchange), AV_OPT_TYPE_STRING, { .str = "amq.direct" }, 0, 0, .flags = D | E },
     { "routing_key", "Key to filter streams", OFFSET(routing_key), AV_OPT_TYPE_STRING, { .str = "amqp" }, 0, 0, .flags = D | E },
     { "connection_timeout", "Initial connection timeout", OFFSET(connection_timeout), AV_OPT_TYPE_DURATION, { .i64 = -1 }, -1, INT64_MAX, .flags = D | E},
@@ -136,7 +138,7 @@  static int amqp_proto_open(URLContext *h, const char *uri, int flags)
         goto destroy_connection;
     }
 
-    broker_reply = amqp_login(s->conn, "/", 0, s->pkt_size, 0,
+    broker_reply = amqp_login(s->conn, s->vhost, 0, s->pkt_size, 0,
                               AMQP_SASL_METHOD_PLAIN, user_decoded, password_decoded);
 
     if (broker_reply.reply_type != AMQP_RESPONSE_NORMAL) {