[FFmpeg-devel] lavf/rtsp: fix the type of the timeout option.

Submitted by Nicolas George on March 10, 2018, 7:37 p.m.

Details

Message ID 20180310193720.12708-1-george@nsup.org
State New
Headers show

Commit Message

Nicolas George March 10, 2018, 7:37 p.m.
A timeout is a duration of time, therefore the correct type for it
is AV_OPT_TYPE_DURATION. It has the benefit of offering a better
user interface, with units specification.
Unfortunately, ff46124b0df17a1d35249e09ae8eae9a61f16e04 was pushed
before that mistake could be corrected.

Signed-off-by: Nicolas George <george@nsup.org>
---
 libavformat/rtsp.c | 5 +++--
 libavformat/rtsp.h | 4 ++++
 2 files changed, 7 insertions(+), 2 deletions(-)

Comments

wm4 March 11, 2018, 1:26 a.m.
On Sat, 10 Mar 2018 20:37:20 +0100
Nicolas George <george@nsup.org> wrote:

> A timeout is a duration of time, therefore the correct type for it
> is AV_OPT_TYPE_DURATION. It has the benefit of offering a better
> user interface, with units specification.
> Unfortunately, ff46124b0df17a1d35249e09ae8eae9a61f16e04 was pushed
> before that mistake could be corrected.
> 
> Signed-off-by: Nicolas George <george@nsup.org>
> ---
>  libavformat/rtsp.c | 5 +++--
>  libavformat/rtsp.h | 4 ++++
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/libavformat/rtsp.c b/libavformat/rtsp.c
> index ceb770a3a4..1fbdcfcedd 100644
> --- a/libavformat/rtsp.c
> +++ b/libavformat/rtsp.c
> @@ -98,7 +98,7 @@ const AVOption ff_rtsp_options[] = {
>      { "timeout", "set maximum timeout (in seconds) to wait for incoming connections (-1 is infinite, imply flag listen) (deprecated, use listen_timeout)", OFFSET(initial_timeout), AV_OPT_TYPE_INT, {.i64 = -1}, INT_MIN, INT_MAX, DEC },
>      { "stimeout", "set timeout (in microseconds) of socket TCP I/O operations", OFFSET(stimeout), AV_OPT_TYPE_INT, {.i64 = 0}, INT_MIN, INT_MAX, DEC },
>  #else
> -    { "timeout", "set timeout (in microseconds) of socket TCP I/O operations", OFFSET(stimeout), AV_OPT_TYPE_INT, {.i64 = 0}, INT_MIN, INT_MAX, DEC },
> +    { "timeout", "set timeout of socket TCP I/O operations", OFFSET(stimeout), AV_OPT_TYPE_DURATION, {.i64 = 0}, 0, INT64_MAX, DEC },
>  #endif
>      COMMON_OPTS(),
>      { "user_agent", "override User-Agent header", OFFSET(user_agent), AV_OPT_TYPE_STRING, {.str = LIBAVFORMAT_IDENT}, 0, 0, DEC },
> @@ -1818,7 +1818,8 @@ redirect:
>          /* open the tcp connection */
>          ff_url_join(tcpname, sizeof(tcpname), lower_rtsp_proto, NULL,
>                      host, port,
> -                    "?timeout=%d", rt->stimeout);
> +                    /* cast necessary until FF_API_OLD_RTSP_OPTIONS removed */
> +                    "?timeout=%"PRId64, (int64_t)rt->stimeout);
>          if ((ret = ffurl_open_whitelist(&rt->rtsp_hd, tcpname, AVIO_FLAG_READ_WRITE,
>                         &s->interrupt_callback, NULL, s->protocol_whitelist, s->protocol_blacklist, NULL)) < 0) {
>              err = ret;
> diff --git a/libavformat/rtsp.h b/libavformat/rtsp.h
> index 9a7f366b39..1524962e1b 100644
> --- a/libavformat/rtsp.h
> +++ b/libavformat/rtsp.h
> @@ -395,7 +395,11 @@ typedef struct RTSPState {
>      /**
>       * timeout of socket i/o operations.
>       */
> +#if FF_API_OLD_RTSP_OPTIONS
>      int stimeout;
> +#else
> +    int64_t stimeout;
> +#endif
>  
>      /**
>       * Size of RTP packet reordering queue.

NACK
wm4 March 11, 2018, 2:03 a.m.
On Sat, 10 Mar 2018 20:37:20 +0100
Nicolas George <george@nsup.org> wrote:

> A timeout is a duration of time, therefore the correct type for it
> is AV_OPT_TYPE_DURATION. It has the benefit of offering a better
> user interface, with units specification.
> Unfortunately, ff46124b0df17a1d35249e09ae8eae9a61f16e04 was pushed
> before that mistake could be corrected.
> 
> Signed-off-by: Nicolas George <george@nsup.org>
> ---
>  libavformat/rtsp.c | 5 +++--
>  libavformat/rtsp.h | 4 ++++
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/libavformat/rtsp.c b/libavformat/rtsp.c
> index ceb770a3a4..1fbdcfcedd 100644
> --- a/libavformat/rtsp.c
> +++ b/libavformat/rtsp.c
> @@ -98,7 +98,7 @@ const AVOption ff_rtsp_options[] = {
>      { "timeout", "set maximum timeout (in seconds) to wait for incoming connections (-1 is infinite, imply flag listen) (deprecated, use listen_timeout)", OFFSET(initial_timeout), AV_OPT_TYPE_INT, {.i64 = -1}, INT_MIN, INT_MAX, DEC },
>      { "stimeout", "set timeout (in microseconds) of socket TCP I/O operations", OFFSET(stimeout), AV_OPT_TYPE_INT, {.i64 = 0}, INT_MIN, INT_MAX, DEC },
>  #else
> -    { "timeout", "set timeout (in microseconds) of socket TCP I/O operations", OFFSET(stimeout), AV_OPT_TYPE_INT, {.i64 = 0}, INT_MIN, INT_MAX, DEC },
> +    { "timeout", "set timeout of socket TCP I/O operations", OFFSET(stimeout), AV_OPT_TYPE_DURATION, {.i64 = 0}, 0, INT64_MAX, DEC },
>  #endif
>      COMMON_OPTS(),
>      { "user_agent", "override User-Agent header", OFFSET(user_agent), AV_OPT_TYPE_STRING, {.str = LIBAVFORMAT_IDENT}, 0, 0, DEC },
> @@ -1818,7 +1818,8 @@ redirect:
>          /* open the tcp connection */
>          ff_url_join(tcpname, sizeof(tcpname), lower_rtsp_proto, NULL,
>                      host, port,
> -                    "?timeout=%d", rt->stimeout);
> +                    /* cast necessary until FF_API_OLD_RTSP_OPTIONS removed */
> +                    "?timeout=%"PRId64, (int64_t)rt->stimeout);
>          if ((ret = ffurl_open_whitelist(&rt->rtsp_hd, tcpname, AVIO_FLAG_READ_WRITE,
>                         &s->interrupt_callback, NULL, s->protocol_whitelist, s->protocol_blacklist, NULL)) < 0) {
>              err = ret;
> diff --git a/libavformat/rtsp.h b/libavformat/rtsp.h
> index 9a7f366b39..1524962e1b 100644
> --- a/libavformat/rtsp.h
> +++ b/libavformat/rtsp.h
> @@ -395,7 +395,11 @@ typedef struct RTSPState {
>      /**
>       * timeout of socket i/o operations.
>       */
> +#if FF_API_OLD_RTSP_OPTIONS
>      int stimeout;
> +#else
> +    int64_t stimeout;
> +#endif
>  
>      /**
>       * Size of RTP packet reordering queue.

On IRC I was asked to explain my NACK:

The whole point of ff46124b0df17a1d35249e09ae8eae9a61f16e04 was to
harmonize the timeout options with that of other protocols. In
particular, http/tcp (the most used network protocol of them all) uses
an AV_OPT_TYPE_INT "timeout" option, using microseconds.

AV_OPT_TYPE_DURATION parses the time in seconds, so this is
incompatible. It's incompatible on both CLI and API, because the API
usually requires you to pass all options as string in AVDictionary
entries (this is how dispatching general options to underlying
protocols work, and it's impossible to access the options for sub
protocols directly).

Thus your change negates the whole point of the previous change, which
is why I'm rejecting it. Reverting others patches after the discussion
of it was done and everything finalized isn't exactly how team work
works either.

You have 2 choices:
- change all timeout options to AV_OPT_TYPE_DURATION (after a
  deprecation period)
- drop the patch

Also, your patch message implies I pushed the previous patch too early.
This is not true - I waited long enough, and because there was a
flamewar, certainly it can't be claimed that you didn't notice it, or
didn't have enough time to do whatever. Especially considering you
replied to the thread on the same day as I posted the patch, and I
pushed the patch over a week later. So I'm politely asking you to stop
making such implications.
Michael Niedermayer March 11, 2018, 1:49 p.m.
On Sun, Mar 11, 2018 at 03:03:06AM +0100, wm4 wrote:
> On Sat, 10 Mar 2018 20:37:20 +0100
> Nicolas George <george@nsup.org> wrote:
> 
> > A timeout is a duration of time, therefore the correct type for it
> > is AV_OPT_TYPE_DURATION. It has the benefit of offering a better
> > user interface, with units specification.
> > Unfortunately, ff46124b0df17a1d35249e09ae8eae9a61f16e04 was pushed
> > before that mistake could be corrected.
> > 
> > Signed-off-by: Nicolas George <george@nsup.org>
> > ---
> >  libavformat/rtsp.c | 5 +++--
> >  libavformat/rtsp.h | 4 ++++
> >  2 files changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/libavformat/rtsp.c b/libavformat/rtsp.c
> > index ceb770a3a4..1fbdcfcedd 100644
> > --- a/libavformat/rtsp.c
> > +++ b/libavformat/rtsp.c
> > @@ -98,7 +98,7 @@ const AVOption ff_rtsp_options[] = {
> >      { "timeout", "set maximum timeout (in seconds) to wait for incoming connections (-1 is infinite, imply flag listen) (deprecated, use listen_timeout)", OFFSET(initial_timeout), AV_OPT_TYPE_INT, {.i64 = -1}, INT_MIN, INT_MAX, DEC },
> >      { "stimeout", "set timeout (in microseconds) of socket TCP I/O operations", OFFSET(stimeout), AV_OPT_TYPE_INT, {.i64 = 0}, INT_MIN, INT_MAX, DEC },
> >  #else
> > -    { "timeout", "set timeout (in microseconds) of socket TCP I/O operations", OFFSET(stimeout), AV_OPT_TYPE_INT, {.i64 = 0}, INT_MIN, INT_MAX, DEC },
> > +    { "timeout", "set timeout of socket TCP I/O operations", OFFSET(stimeout), AV_OPT_TYPE_DURATION, {.i64 = 0}, 0, INT64_MAX, DEC },
> >  #endif
> >      COMMON_OPTS(),
> >      { "user_agent", "override User-Agent header", OFFSET(user_agent), AV_OPT_TYPE_STRING, {.str = LIBAVFORMAT_IDENT}, 0, 0, DEC },
> > @@ -1818,7 +1818,8 @@ redirect:
> >          /* open the tcp connection */
> >          ff_url_join(tcpname, sizeof(tcpname), lower_rtsp_proto, NULL,
> >                      host, port,
> > -                    "?timeout=%d", rt->stimeout);
> > +                    /* cast necessary until FF_API_OLD_RTSP_OPTIONS removed */
> > +                    "?timeout=%"PRId64, (int64_t)rt->stimeout);
> >          if ((ret = ffurl_open_whitelist(&rt->rtsp_hd, tcpname, AVIO_FLAG_READ_WRITE,
> >                         &s->interrupt_callback, NULL, s->protocol_whitelist, s->protocol_blacklist, NULL)) < 0) {
> >              err = ret;
> > diff --git a/libavformat/rtsp.h b/libavformat/rtsp.h
> > index 9a7f366b39..1524962e1b 100644
> > --- a/libavformat/rtsp.h
> > +++ b/libavformat/rtsp.h
> > @@ -395,7 +395,11 @@ typedef struct RTSPState {
> >      /**
> >       * timeout of socket i/o operations.
> >       */
> > +#if FF_API_OLD_RTSP_OPTIONS
> >      int stimeout;
> > +#else
> > +    int64_t stimeout;
> > +#endif
> >  
> >      /**
> >       * Size of RTP packet reordering queue.

I have been asked to comment on this patch


> 
> On IRC I was asked to explain my NACK:
> 
> The whole point of ff46124b0df17a1d35249e09ae8eae9a61f16e04 was to
> harmonize the timeout options with that of other protocols. In
> particular, http/tcp (the most used network protocol of them all) uses
> an AV_OPT_TYPE_INT "timeout" option, using microseconds.
> 

> AV_OPT_TYPE_DURATION parses the time in seconds, so this is
> incompatible. It's incompatible on both CLI and API, because the API
> usually requires you to pass all options as string in AVDictionary
> entries (this is how dispatching general options to underlying
> protocols work, and it's impossible to access the options for sub
> protocols directly).

If we look at the last release:
git grep '"timeout"' release/3.4 libavformat/rtsp.c
release/3.4:libavformat/rtsp.c:    { "timeout", "set maximum timeout (in seconds) to wait for incoming connections (-1 is infinite, imply flag listen)", OFFSET(initial_timeout), AV_OPT_TYPE_INT, {.i64 = -1}, INT_MIN, INT_MAX, DEC },
                                                                      ^^^^^^^^^^
That in fact the timeout parameter of rtsp.c was in seconds already


> 
> Thus your change negates the whole point of the previous change, which
> is why I'm rejecting it. Reverting others patches after the discussion
> of it was done and everything finalized isn't exactly how team work
> works either.
> 
> You have 2 choices:
> - change all timeout options to AV_OPT_TYPE_DURATION (after a
>   deprecation period)
> - drop the patch

Let me summarize it
IIRC nicolas was against your patch
you are against nicolas patch

neither of these patches move us toward a consistent and compatible
AV_OPT_TYPE_DURATION based timeout interface.

"timeout" as it is currently, is not consistent, it is not in seconds
in many cases and we cant just change it everywhere as it would break 
compatibility.
What we need is a new parameter with a new name that then will be
consistently be in AV_OPT_TYPE_DURATION (seconds). And then deprecate 
all the old parameters
I wanted to work on this but didnt had time yet.

I think we should not fight over this patch here or yours.
The timeout parameter they modify will be deprecated. And all variants
that can be done to it before suck in some form
yours breaks compatibility, nicolas is inconsistent with other code
...

So please lets work towards implementing consistent AV_OPT_TYPE_DURATION
timeouts that doesnt break anything (that is with new parameters).
And lets all work together!

thanks

[...]
wm4 March 11, 2018, 2:29 p.m.
On Sun, 11 Mar 2018 14:49:37 +0100
Michael Niedermayer <michael@niedermayer.cc> wrote:

> On Sun, Mar 11, 2018 at 03:03:06AM +0100, wm4 wrote:
> > On Sat, 10 Mar 2018 20:37:20 +0100
> > Nicolas George <george@nsup.org> wrote:
> >   
> > > A timeout is a duration of time, therefore the correct type for it
> > > is AV_OPT_TYPE_DURATION. It has the benefit of offering a better
> > > user interface, with units specification.
> > > Unfortunately, ff46124b0df17a1d35249e09ae8eae9a61f16e04 was pushed
> > > before that mistake could be corrected.
> > > 
> > > Signed-off-by: Nicolas George <george@nsup.org>
> > > ---
> > >  libavformat/rtsp.c | 5 +++--
> > >  libavformat/rtsp.h | 4 ++++
> > >  2 files changed, 7 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/libavformat/rtsp.c b/libavformat/rtsp.c
> > > index ceb770a3a4..1fbdcfcedd 100644
> > > --- a/libavformat/rtsp.c
> > > +++ b/libavformat/rtsp.c
> > > @@ -98,7 +98,7 @@ const AVOption ff_rtsp_options[] = {
> > >      { "timeout", "set maximum timeout (in seconds) to wait for incoming connections (-1 is infinite, imply flag listen) (deprecated, use listen_timeout)", OFFSET(initial_timeout), AV_OPT_TYPE_INT, {.i64 = -1}, INT_MIN, INT_MAX, DEC },
> > >      { "stimeout", "set timeout (in microseconds) of socket TCP I/O operations", OFFSET(stimeout), AV_OPT_TYPE_INT, {.i64 = 0}, INT_MIN, INT_MAX, DEC },
> > >  #else
> > > -    { "timeout", "set timeout (in microseconds) of socket TCP I/O operations", OFFSET(stimeout), AV_OPT_TYPE_INT, {.i64 = 0}, INT_MIN, INT_MAX, DEC },
> > > +    { "timeout", "set timeout of socket TCP I/O operations", OFFSET(stimeout), AV_OPT_TYPE_DURATION, {.i64 = 0}, 0, INT64_MAX, DEC },
> > >  #endif
> > >      COMMON_OPTS(),
> > >      { "user_agent", "override User-Agent header", OFFSET(user_agent), AV_OPT_TYPE_STRING, {.str = LIBAVFORMAT_IDENT}, 0, 0, DEC },
> > > @@ -1818,7 +1818,8 @@ redirect:
> > >          /* open the tcp connection */
> > >          ff_url_join(tcpname, sizeof(tcpname), lower_rtsp_proto, NULL,
> > >                      host, port,
> > > -                    "?timeout=%d", rt->stimeout);
> > > +                    /* cast necessary until FF_API_OLD_RTSP_OPTIONS removed */
> > > +                    "?timeout=%"PRId64, (int64_t)rt->stimeout);
> > >          if ((ret = ffurl_open_whitelist(&rt->rtsp_hd, tcpname, AVIO_FLAG_READ_WRITE,
> > >                         &s->interrupt_callback, NULL, s->protocol_whitelist, s->protocol_blacklist, NULL)) < 0) {
> > >              err = ret;
> > > diff --git a/libavformat/rtsp.h b/libavformat/rtsp.h
> > > index 9a7f366b39..1524962e1b 100644
> > > --- a/libavformat/rtsp.h
> > > +++ b/libavformat/rtsp.h
> > > @@ -395,7 +395,11 @@ typedef struct RTSPState {
> > >      /**
> > >       * timeout of socket i/o operations.
> > >       */
> > > +#if FF_API_OLD_RTSP_OPTIONS
> > >      int stimeout;
> > > +#else
> > > +    int64_t stimeout;
> > > +#endif
> > >  
> > >      /**
> > >       * Size of RTP packet reordering queue.  
> 
> I have been asked to comment on this patch
> 
> 
> > 
> > On IRC I was asked to explain my NACK:
> > 
> > The whole point of ff46124b0df17a1d35249e09ae8eae9a61f16e04 was to
> > harmonize the timeout options with that of other protocols. In
> > particular, http/tcp (the most used network protocol of them all) uses
> > an AV_OPT_TYPE_INT "timeout" option, using microseconds.
> >   
> 
> > AV_OPT_TYPE_DURATION parses the time in seconds, so this is
> > incompatible. It's incompatible on both CLI and API, because the API
> > usually requires you to pass all options as string in AVDictionary
> > entries (this is how dispatching general options to underlying
> > protocols work, and it's impossible to access the options for sub
> > protocols directly).  
> 
> If we look at the last release:
> git grep '"timeout"' release/3.4 libavformat/rtsp.c
> release/3.4:libavformat/rtsp.c:    { "timeout", "set maximum timeout (in seconds) to wait for incoming connections (-1 is infinite, imply flag listen)", OFFSET(initial_timeout), AV_OPT_TYPE_INT, {.i64 = -1}, INT_MIN, INT_MAX, DEC },
>                                                                       ^^^^^^^^^^
> That in fact the timeout parameter of rtsp.c was in seconds already
> 
> 
> > 
> > Thus your change negates the whole point of the previous change, which
> > is why I'm rejecting it. Reverting others patches after the discussion
> > of it was done and everything finalized isn't exactly how team work
> > works either.
> > 
> > You have 2 choices:
> > - change all timeout options to AV_OPT_TYPE_DURATION (after a
> >   deprecation period)
> > - drop the patch  
> 
> Let me summarize it
> IIRC nicolas was against your patch
> you are against nicolas patch
> 
> neither of these patches move us toward a consistent and compatible
> AV_OPT_TYPE_DURATION based timeout interface.
> 
> "timeout" as it is currently, is not consistent, it is not in seconds
> in many cases and we cant just change it everywhere as it would break 
> compatibility.
> What we need is a new parameter with a new name that then will be
> consistently be in AV_OPT_TYPE_DURATION (seconds). And then deprecate 
> all the old parameters
> I wanted to work on this but didnt had time yet.
> 
> I think we should not fight over this patch here or yours.
> The timeout parameter they modify will be deprecated. And all variants
> that can be done to it before suck in some form
> yours breaks compatibility, nicolas is inconsistent with other code
> ...
> 
> So please lets work towards implementing consistent AV_OPT_TYPE_DURATION
> timeouts that doesnt break anything (that is with new parameters).
> And lets all work together!

Sure, but don't just send a patch that reverts an earlier patch of mine
just because it uses the shiny (completely undocumented and also
questionable in design) AV_OPT_TYPE_DURATION. If anyone sends a patch
that introduces a new option name (or deprecated the current timeout
one) for all protocols, I won't say anything against it.

Patch hide | download patch | download mbox

diff --git a/libavformat/rtsp.c b/libavformat/rtsp.c
index ceb770a3a4..1fbdcfcedd 100644
--- a/libavformat/rtsp.c
+++ b/libavformat/rtsp.c
@@ -98,7 +98,7 @@  const AVOption ff_rtsp_options[] = {
     { "timeout", "set maximum timeout (in seconds) to wait for incoming connections (-1 is infinite, imply flag listen) (deprecated, use listen_timeout)", OFFSET(initial_timeout), AV_OPT_TYPE_INT, {.i64 = -1}, INT_MIN, INT_MAX, DEC },
     { "stimeout", "set timeout (in microseconds) of socket TCP I/O operations", OFFSET(stimeout), AV_OPT_TYPE_INT, {.i64 = 0}, INT_MIN, INT_MAX, DEC },
 #else
-    { "timeout", "set timeout (in microseconds) of socket TCP I/O operations", OFFSET(stimeout), AV_OPT_TYPE_INT, {.i64 = 0}, INT_MIN, INT_MAX, DEC },
+    { "timeout", "set timeout of socket TCP I/O operations", OFFSET(stimeout), AV_OPT_TYPE_DURATION, {.i64 = 0}, 0, INT64_MAX, DEC },
 #endif
     COMMON_OPTS(),
     { "user_agent", "override User-Agent header", OFFSET(user_agent), AV_OPT_TYPE_STRING, {.str = LIBAVFORMAT_IDENT}, 0, 0, DEC },
@@ -1818,7 +1818,8 @@  redirect:
         /* open the tcp connection */
         ff_url_join(tcpname, sizeof(tcpname), lower_rtsp_proto, NULL,
                     host, port,
-                    "?timeout=%d", rt->stimeout);
+                    /* cast necessary until FF_API_OLD_RTSP_OPTIONS removed */
+                    "?timeout=%"PRId64, (int64_t)rt->stimeout);
         if ((ret = ffurl_open_whitelist(&rt->rtsp_hd, tcpname, AVIO_FLAG_READ_WRITE,
                        &s->interrupt_callback, NULL, s->protocol_whitelist, s->protocol_blacklist, NULL)) < 0) {
             err = ret;
diff --git a/libavformat/rtsp.h b/libavformat/rtsp.h
index 9a7f366b39..1524962e1b 100644
--- a/libavformat/rtsp.h
+++ b/libavformat/rtsp.h
@@ -395,7 +395,11 @@  typedef struct RTSPState {
     /**
      * timeout of socket i/o operations.
      */
+#if FF_API_OLD_RTSP_OPTIONS
     int stimeout;
+#else
+    int64_t stimeout;
+#endif
 
     /**
      * Size of RTP packet reordering queue.