| Message ID | 20180310193720.12708-1-george@nsup.org |
|---|---|
| State | New |
| Headers | show |
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
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.
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 [...]
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.
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.
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(-)