| Message ID | 20230125010744.255455-1-stefasab@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] doc/ffmpeg: extend -dts_delta_threshold option description | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
On 2023-01-25 06:37 am, Stefano Sabatini wrote: > --- > doc/ffmpeg.texi | 17 +++++++++++++++-- > 1 file changed, 15 insertions(+), 2 deletions(-) > > diff --git a/doc/ffmpeg.texi b/doc/ffmpeg.texi > index 67b3294256..122f7e3387 100644 > --- a/doc/ffmpeg.texi > +++ b/doc/ffmpeg.texi > @@ -1823,8 +1823,21 @@ results, but increase memory use and latency. > > The default value is 10 seconds. > > -@item -dts_delta_threshold > -Timestamp discontinuity delta threshold. > +@item -dts_delta_threshold @var{threshold} > +Timestamp discontinuity delta threshold, expressed as a floating point > +number of @var{AV_TIME_BASE} units. This is a CLI option and those users don't deal with AV_TIME_BASE . More useful to say it's in seconds. > + > +If a timestamp discontinuity is detected whose absolute value is > +greater than @var{threshold} * @var{AV_TIME_BASE}, ffmpeg will remove the > +discontinuity by decreasing/increasing the current DTS and PTS by the > +corresponding delta value. Might want to mention that this only applies to AV_FMT_DISCONT demuxers, or rather give a few examples, like MPEG-TS, HLS..etc. For all other formats that users normally work with, clarify that only dts_error_threshold is relevant. > + > +Timestamp discontinuity correction can be inhibited by setting a big value for > +@var{threshold}, and is automatically disabled when employing the > +@code{-copy_ts} option. For copy_ts, it is still applied for all negative deltas except the smallest. Not blocking, but I'm reworking this code at present. Shouldn't really affect this patch. See https://ffmpeg.org/pipermail/ffmpeg-devel/2023-January/305539.html Regards, Gyan
On date Wednesday 2023-01-25 10:47:20 +0530, Gyan Doshi wrote: > > > On 2023-01-25 06:37 am, Stefano Sabatini wrote: > > --- > > doc/ffmpeg.texi | 17 +++++++++++++++-- > > 1 file changed, 15 insertions(+), 2 deletions(-) > > > > diff --git a/doc/ffmpeg.texi b/doc/ffmpeg.texi > > index 67b3294256..122f7e3387 100644 > > --- a/doc/ffmpeg.texi > > +++ b/doc/ffmpeg.texi > > @@ -1823,8 +1823,21 @@ results, but increase memory use and latency. > > The default value is 10 seconds. > > -@item -dts_delta_threshold > > -Timestamp discontinuity delta threshold. > > +@item -dts_delta_threshold @var{threshold} > > +Timestamp discontinuity delta threshold, expressed as a floating point > > +number of @var{AV_TIME_BASE} units. > This is a CLI option and those users don't deal with AV_TIME_BASE . More > useful to say it's in seconds. > > > + > > +If a timestamp discontinuity is detected whose absolute value is > > +greater than @var{threshold} * @var{AV_TIME_BASE}, ffmpeg will remove the > > +discontinuity by decreasing/increasing the current DTS and PTS by the > > +corresponding delta value. > > Might want to mention that this only applies to AV_FMT_DISCONT demuxers, or > rather give a few examples, like MPEG-TS, HLS..etc. > For all other formats that users normally work with, clarify that only > dts_error_threshold is relevant. > > + > > +Timestamp discontinuity correction can be inhibited by setting a big value for > > +@var{threshold}, and is automatically disabled when employing the > > +@code{-copy_ts} option. > > For copy_ts, it is still applied for all negative deltas except the > smallest. Which is a bit arbitrary. > > Not blocking, but I'm reworking this code at present. Shouldn't really > affect this patch. See > https://ffmpeg.org/pipermail/ffmpeg-devel/2023-January/305539.html Updated (had to review the code to get some clarity on the current behavior). Thanks.
On 2023-02-06 06:55 am, Stefano Sabatini wrote: > On date Wednesday 2023-01-25 10:47:20 +0530, Gyan Doshi wrote: >> For copy_ts, it is still applied for all negative deltas except the >> smallest. > Which is a bit arbitrary. Possibly, but I imagine there was some trac ticket which prompted that handling. Does git blame shed any light? Regards, Gyan
On Mon, Feb 06, 2023 at 02:25:23AM +0100, Stefano Sabatini wrote: > On date Wednesday 2023-01-25 10:47:20 +0530, Gyan Doshi wrote: > > > > > > On 2023-01-25 06:37 am, Stefano Sabatini wrote: > > > --- > > > doc/ffmpeg.texi | 17 +++++++++++++++-- > > > 1 file changed, 15 insertions(+), 2 deletions(-) > > > > > > diff --git a/doc/ffmpeg.texi b/doc/ffmpeg.texi > > > index 67b3294256..122f7e3387 100644 > > > --- a/doc/ffmpeg.texi > > > +++ b/doc/ffmpeg.texi > > > @@ -1823,8 +1823,21 @@ results, but increase memory use and latency. > > > The default value is 10 seconds. > > > -@item -dts_delta_threshold > > > -Timestamp discontinuity delta threshold. > > > +@item -dts_delta_threshold @var{threshold} > > > +Timestamp discontinuity delta threshold, expressed as a floating point > > > +number of @var{AV_TIME_BASE} units. > > This is a CLI option and those users don't deal with AV_TIME_BASE . More > > useful to say it's in seconds. > > > > > + > > > +If a timestamp discontinuity is detected whose absolute value is > > > +greater than @var{threshold} * @var{AV_TIME_BASE}, ffmpeg will remove the > > > +discontinuity by decreasing/increasing the current DTS and PTS by the > > > +corresponding delta value. > > > > > Might want to mention that this only applies to AV_FMT_DISCONT demuxers, or > > rather give a few examples, like MPEG-TS, HLS..etc. > > For all other formats that users normally work with, clarify that only > > dts_error_threshold is relevant. > > > > > + > > > +Timestamp discontinuity correction can be inhibited by setting a big value for > > > +@var{threshold}, and is automatically disabled when employing the > > > +@code{-copy_ts} option. > > > > For copy_ts, it is still applied for all negative deltas except the > > smallest. > > Which is a bit arbitrary. > > > > > Not blocking, but I'm reworking this code at present. Shouldn't really > > affect this patch. See > > https://ffmpeg.org/pipermail/ffmpeg-devel/2023-January/305539.html > > Updated (had to review the code to get some clarity on the current > behavior). > > Thanks. > doc/ffmpeg.texi | 36 ++++++++++++++++++---- > fftools/ffmpeg.c | 72 +++++++++++++++++++++++--------------------- > fftools/ffmpeg.h | 2 + > fftools/ffmpeg_demux.c | 3 + > tests/fate/filter-audio.mak | 2 - > tests/fate/mpeg4.mak | 2 - > 6 files changed, 77 insertions(+), 40 deletions(-) > 55e91624524749274a62bdc43cacdf8e47dd3598 0002-ffmpeg-review-dts_delta_threshold-and-dts_delta_erro.patch > From 30ac56a14d66eb25428b71c9211f401f818bd057 Mon Sep 17 00:00:00 2001 > From: Stefano Sabatini <stefasab@gmail.com> > Date: Fri, 1 Apr 2011 01:13:35 +0200 > Subject: [PATCH 2/2] ffmpeg: review -dts_delta_threshold and -dts_delta_error > options > > Review handling of -dts_delta_threshold and -dts_delta_error options, > specify them as floating point expressed in seconds. > > Also, review and simplify logic. Adjust values for tests, since in > some cases the new values does not trigger the correction logic. > > PR: https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=8252 > --- > doc/ffmpeg.texi | 36 ++++++++++++++++--- > fftools/ffmpeg.c | 72 ++++++++++++++++++++----------------- > fftools/ffmpeg.h | 2 ++ > fftools/ffmpeg_demux.c | 3 ++ > tests/fate/filter-audio.mak | 2 +- > tests/fate/mpeg4.mak | 2 +- > 6 files changed, 77 insertions(+), 40 deletions(-) This seems to break a case with concat and vsync ./ffmpeg -y -i 'concat:///home/michael/videos/angels.mpg|/home/michael/videos/angels.mpg' -vsync 0 -an file.avi ... cpb: bitrate max/min/avg: 0/0/200000 buffer size: 0 vbv_delay: N/A [mpeg4 @ 0x55e051b8d4c0] Invalid pts (0) <= last (11)00.00 bitrate=N/A speed= 0x [vost#0:0/mpeg4 @ 0x55e051b9d700] Error submitting video frame to the encoder Conversion failed! Ill mail you the angels.mpg, i think its online somewhere but i cant find it thx [...]
On date Thursday 2023-02-09 00:41:18 +0100, Michael Niedermayer wrote: > On Mon, Feb 06, 2023 at 02:25:23AM +0100, Stefano Sabatini wrote: [...] > > Subject: [PATCH 2/2] ffmpeg: review -dts_delta_threshold and -dts_delta_error > > options > > > > Review handling of -dts_delta_threshold and -dts_delta_error options, > > specify them as floating point expressed in seconds. > > > > Also, review and simplify logic. Adjust values for tests, since in > > some cases the new values does not trigger the correction logic. > > > > PR: https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=8252 > > --- > > doc/ffmpeg.texi | 36 ++++++++++++++++--- > > fftools/ffmpeg.c | 72 ++++++++++++++++++++----------------- > > fftools/ffmpeg.h | 2 ++ > > fftools/ffmpeg_demux.c | 3 ++ > > tests/fate/filter-audio.mak | 2 +- > > tests/fate/mpeg4.mak | 2 +- > > 6 files changed, 77 insertions(+), 40 deletions(-) > > This seems to break a case with concat and vsync > ./ffmpeg -y -i 'concat:///home/michael/videos/angels.mpg|/home/michael/videos/angels.mpg' -vsync 0 -an file.avi > > ... > cpb: bitrate max/min/avg: 0/0/200000 buffer size: 0 vbv_delay: N/A > [mpeg4 @ 0x55e051b8d4c0] Invalid pts (0) <= last (11)00.00 bitrate=N/A speed= 0x > [vost#0:0/mpeg4 @ 0x55e051b9d700] Error submitting video frame to the encoder > Conversion failed! > > > Ill mail you the angels.mpg, i think its online somewhere but i cant > find it Fixed, now the code should be equivalent to the previous implementation. What happened in this case (and apparently in the other fate tests failing), is that some sort of limit correction is applied: detected dts:-0.041711 < dts_limit:0.358789 ts delta 0.5005 applied => ts_offset_discont:0.5005 dts:0.458789 preventing the invalid pts error. The limit correction, hardcoded in the ffmpeg.c code, is completely unrelated to the dts_delta_threshold value, no idea if it would make sense to make this parametric (but at least now it should be a bit more clear from the code/logs).
On date Saturday 2023-02-11 03:30:00 +0100, Stefano Sabatini wrote: > On date Thursday 2023-02-09 00:41:18 +0100, Michael Niedermayer wrote: > > On Mon, Feb 06, 2023 at 02:25:23AM +0100, Stefano Sabatini wrote: > [...] > > > Subject: [PATCH 2/2] ffmpeg: review -dts_delta_threshold and -dts_delta_error > > > options > > > > > > Review handling of -dts_delta_threshold and -dts_delta_error options, > > > specify them as floating point expressed in seconds. > > > > > > Also, review and simplify logic. Adjust values for tests, since in > > > some cases the new values does not trigger the correction logic. > > > > > > PR: https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=8252 > > > --- > > > doc/ffmpeg.texi | 36 ++++++++++++++++--- > > > fftools/ffmpeg.c | 72 ++++++++++++++++++++----------------- > > > fftools/ffmpeg.h | 2 ++ > > > fftools/ffmpeg_demux.c | 3 ++ > > > tests/fate/filter-audio.mak | 2 +- > > > tests/fate/mpeg4.mak | 2 +- > > > 6 files changed, 77 insertions(+), 40 deletions(-) > > > > This seems to break a case with concat and vsync > > ./ffmpeg -y -i 'concat:///home/michael/videos/angels.mpg|/home/michael/videos/angels.mpg' -vsync 0 -an file.avi > > > > ... > > cpb: bitrate max/min/avg: 0/0/200000 buffer size: 0 vbv_delay: N/A > > [mpeg4 @ 0x55e051b8d4c0] Invalid pts (0) <= last (11)00.00 bitrate=N/A speed= 0x > > [vost#0:0/mpeg4 @ 0x55e051b9d700] Error submitting video frame to the encoder > > Conversion failed! > > > > > > Ill mail you the angels.mpg, i think its online somewhere but i cant > > find it > > Fixed, now the code should be equivalent to the previous > implementation. > > What happened in this case (and apparently in the other fate tests > failing), is that some sort of limit correction is applied: > > detected dts:-0.041711 < dts_limit:0.358789 > ts delta 0.5005 applied => ts_offset_discont:0.5005 dts:0.458789 > > preventing the invalid pts error. > > The limit correction, hardcoded in the ffmpeg.c code, is completely > unrelated to the dts_delta_threshold value, no idea if it would make > sense to make this parametric (but at least now it should be a bit > more clear from the code/logs). Moving the refactoring changes to a dedicated thread. Updating the doc extensions.
On 2023-02-11 09:58 pm, Stefano Sabatini wrote: > On date Saturday 2023-02-11 03:30:00 +0100, Stefano Sabatini wrote: >> On date Thursday 2023-02-09 00:41:18 +0100, Michael Niedermayer wrote: >>> On Mon, Feb 06, 2023 at 02:25:23AM +0100, Stefano Sabatini wrote: >> [...] >>>> Subject: [PATCH 2/2] ffmpeg: review -dts_delta_threshold and -dts_delta_error >>>> options >>>> >>>> Review handling of -dts_delta_threshold and -dts_delta_error options, >>>> specify them as floating point expressed in seconds. >>>> >>>> Also, review and simplify logic. Adjust values for tests, since in >>>> some cases the new values does not trigger the correction logic. >>>> >>>> PR: https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=8252 >>>> --- >>>> doc/ffmpeg.texi | 36 ++++++++++++++++--- >>>> fftools/ffmpeg.c | 72 ++++++++++++++++++++----------------- >>>> fftools/ffmpeg.h | 2 ++ >>>> fftools/ffmpeg_demux.c | 3 ++ >>>> tests/fate/filter-audio.mak | 2 +- >>>> tests/fate/mpeg4.mak | 2 +- >>>> 6 files changed, 77 insertions(+), 40 deletions(-) >>> This seems to break a case with concat and vsync >>> ./ffmpeg -y -i 'concat:///home/michael/videos/angels.mpg|/home/michael/videos/angels.mpg' -vsync 0 -an file.avi >>> >>> ... >>> cpb: bitrate max/min/avg: 0/0/200000 buffer size: 0 vbv_delay: N/A >>> [mpeg4 @ 0x55e051b8d4c0] Invalid pts (0) <= last (11)00.00 bitrate=N/A speed= 0x >>> [vost#0:0/mpeg4 @ 0x55e051b9d700] Error submitting video frame to the encoder >>> Conversion failed! >>> >>> >>> Ill mail you the angels.mpg, i think its online somewhere but i cant >>> find it >> Fixed, now the code should be equivalent to the previous >> implementation. >> >> What happened in this case (and apparently in the other fate tests >> failing), is that some sort of limit correction is applied: >> >> detected dts:-0.041711 < dts_limit:0.358789 >> ts delta 0.5005 applied => ts_offset_discont:0.5005 dts:0.458789 >> >> preventing the invalid pts error. >> >> The limit correction, hardcoded in the ffmpeg.c code, is completely >> unrelated to the dts_delta_threshold value, no idea if it would make >> sense to make this parametric (but at least now it should be a bit >> more clear from the code/logs). > Moving the refactoring changes to a dedicated thread. > > Updating the doc extensions. LGTM. Regards, Gyan
Quoting Stefano Sabatini (2023-02-11 03:30:00) > On date Thursday 2023-02-09 00:41:18 +0100, Michael Niedermayer wrote: > > On Mon, Feb 06, 2023 at 02:25:23AM +0100, Stefano Sabatini wrote: > [...] > > > Subject: [PATCH 2/2] ffmpeg: review -dts_delta_threshold and -dts_delta_error > > > options > > > > > > Review handling of -dts_delta_threshold and -dts_delta_error options, > > > specify them as floating point expressed in seconds. Maybe it's a bit nitpicky, but these are decimal numbers, not floating point.
On date Monday 2023-02-20 18:57:44 +0100, Anton Khirnov wrote: > Quoting Stefano Sabatini (2023-02-11 03:30:00) > > On date Thursday 2023-02-09 00:41:18 +0100, Michael Niedermayer wrote: > > > On Mon, Feb 06, 2023 at 02:25:23AM +0100, Stefano Sabatini wrote: > > [...] > > > > Subject: [PATCH 2/2] ffmpeg: review -dts_delta_threshold and -dts_delta_error > > > > options > > > > > > > > Review handling of -dts_delta_threshold and -dts_delta_error options, > > > > specify them as floating point expressed in seconds. > > Maybe it's a bit nitpicky, but these are decimal numbers, not floating > point. Fixed and pushed, thanks.
diff --git a/doc/ffmpeg.texi b/doc/ffmpeg.texi index 67b3294256..122f7e3387 100644 --- a/doc/ffmpeg.texi +++ b/doc/ffmpeg.texi @@ -1823,8 +1823,21 @@ results, but increase memory use and latency. The default value is 10 seconds. -@item -dts_delta_threshold -Timestamp discontinuity delta threshold. +@item -dts_delta_threshold @var{threshold} +Timestamp discontinuity delta threshold, expressed as a floating point +number of @var{AV_TIME_BASE} units. + +If a timestamp discontinuity is detected whose absolute value is +greater than @var{threshold} * @var{AV_TIME_BASE}, ffmpeg will remove the +discontinuity by decreasing/increasing the current DTS and PTS by the +corresponding delta value. + +Timestamp discontinuity correction can be inhibited by setting a big value for +@var{threshold}, and is automatically disabled when employing the +@code{-copy_ts} option. + +Default value is 10. + @item -dts_error_threshold @var{seconds} Timestamp error delta threshold. This threshold use to discard crazy/damaged timestamps and the default is 30 hours which is arbitrarily picked and quite