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

wm4 nfxjfg at googlemail.com
Sun Mar 11 16:29:49 EET 2018


On Sun, 11 Mar 2018 14:49:37 +0100
Michael Niedermayer <michael at 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 at 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 at 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.


More information about the ffmpeg-devel mailing list