On Sun, Mar 11, 2018 at 03:03:06AM +0100, wm4 wrote:
> On Sat, 10 Mar 2018 20:37:20 +0100
> Nicolas George <geo...@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 <geo...@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
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
I have never wished to cater to the crowd; for what I know they do not
approve, and what they approve I do not know. -- Epicurus
signature.asc
Description: PGP signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
