On Sun, 11 Mar 2018 14:49:37 +0100 Michael Niedermayer <mich...@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 <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! 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. _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel