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

wm4 nfxjfg at googlemail.com
Sun Mar 11 04:03:06 EET 2018


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.

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.


More information about the ffmpeg-devel mailing list