diff mbox series

[FFmpeg-devel,v2] ffmpeg: allow full range of dts_delta_threshold

Message ID 20200414091847.596-1-ffmpeg@gyani.pro
State Superseded
Headers show
Series [FFmpeg-devel,v2] ffmpeg: allow full range of dts_delta_threshold | expand

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Gyan Doshi April 14, 2020, 9:18 a.m. UTC
For inputs from demuxers with AVFMT_TS_DISCONT flag,
the existing condition,

  delta < -1LL*dts_delta_threshold*AV_TIME_BASE

is rendered superflous due to the fixed threshold in

pkt_dts + AV_TIME_BASE/10 < FFMAX(ist->pts, ist->dts)

This prevents users from setting a high threshold to
avoid discontinuity correction due to errant timestamps.

Now, the maximum of the two thresholds is used.

fate-mpeg4-resolution-change call changed to preserve existing
timestamp correction by ffmpeg.c
---
Tested with multiple satellite MPEG-TS inputs.

 fftools/ffmpeg.c     | 2 +-
 tests/fate/mpeg4.mak | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Comments

Michael Niedermayer April 14, 2020, 8:03 p.m. UTC | #1
On Tue, Apr 14, 2020 at 02:48:47PM +0530, Gyan Doshi wrote:
> For inputs from demuxers with AVFMT_TS_DISCONT flag,
> the existing condition,
> 
>   delta < -1LL*dts_delta_threshold*AV_TIME_BASE
> 
> is rendered superflous due to the fixed threshold in
> 
> pkt_dts + AV_TIME_BASE/10 < FFMAX(ist->pts, ist->dts)
> 
> This prevents users from setting a high threshold to
> avoid discontinuity correction due to errant timestamps.
> 
> Now, the maximum of the two thresholds is used.
> 
> fate-mpeg4-resolution-change call changed to preserve existing
> timestamp correction by ffmpeg.c
> ---
> Tested with multiple satellite MPEG-TS inputs.
> 
>  fftools/ffmpeg.c     | 2 +-
>  tests/fate/mpeg4.mak | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)

breaks:
./ffmpeg -i 'concat:angels.mpg|angels.mpg'  -vsync 0  file.avi

...
frame=   24 fps=0.0 q=12.5 Lsize=     215kB time=00:00:07.50 bitrate= 234.7kbits/s speed=75.5x    
video:84kB audio:110kB subtitle:0kB other streams:0kB global headers:0kB muxing overhead: 10.697786%

vs.

...
[avi @ 0x5601caccc180] Non-monotonous DTS in output stream 0:1; previous: 219, current: 86; changing to 220. This may result in incorrect timestamps in the output file.
[avi @ 0x5601caccc180] Non-monotonous DTS in output stream 0:1; previous: 220, current: 87; changing to 221. This may result in incorrect timestamps in the output file.
[avi @ 0x5601caccc180] Non-monotonous DTS in output stream 0:1; previous: 221, current: 88; changing to 222. This may result in incorrect timestamps in the output file.
[avi @ 0x5601caccc180] Non-monotonous DTS in output stream 0:1; previous: 222, current: 89; changing to 223. This may result in incorrect timestamps in the output file.
[avi @ 0x5601caccc180] Non-monotonous DTS in output stream 0:1; previous: 223, current: 90; changing to 224. This may result in incorrect timestamps in the output file.
[avi @ 0x5601caccc180] Non-monotonous DTS in output stream 0:1; previous: 224, current: 91; changing to 225. This may result in incorrect timestamps in the output file.
[mpeg4 @ 0x5601cac87540] Invalid pts (72) <= last (83)
Video encoding failed

[...]
Gyan Doshi April 15, 2020, 5:25 a.m. UTC | #2
On 15-04-2020 01:33 am, Michael Niedermayer wrote:
> On Tue, Apr 14, 2020 at 02:48:47PM +0530, Gyan Doshi wrote:
>> For inputs from demuxers with AVFMT_TS_DISCONT flag,
>> the existing condition,
>>
>>    delta < -1LL*dts_delta_threshold*AV_TIME_BASE
>>
>> is rendered superflous due to the fixed threshold in
>>
>> pkt_dts + AV_TIME_BASE/10 < FFMAX(ist->pts, ist->dts)
>>
>> This prevents users from setting a high threshold to
>> avoid discontinuity correction due to errant timestamps.
>>
>> Now, the maximum of the two thresholds is used.
>>
>> fate-mpeg4-resolution-change call changed to preserve existing
>> timestamp correction by ffmpeg.c
>> ---
>> Tested with multiple satellite MPEG-TS inputs.
>>
>>   fftools/ffmpeg.c     | 2 +-
>>   tests/fate/mpeg4.mak | 2 +-
>>   2 files changed, 2 insertions(+), 2 deletions(-)
> breaks:
> ./ffmpeg -i 'concat:angels.mpg|angels.mpg'  -vsync 0  file.avi
>
> ...
> frame=   24 fps=0.0 q=12.5 Lsize=     215kB time=00:00:07.50 bitrate= 234.7kbits/s speed=75.5x
> video:84kB audio:110kB subtitle:0kB other streams:0kB global headers:0kB muxing overhead: 10.697786%
>
> vs.
>
> ...
> [avi @ 0x5601caccc180] Non-monotonous DTS in output stream 0:1; previous: 219, current: 86; changing to 220. This may result in incorrect timestamps in the output file.
> [avi @ 0x5601caccc180] Non-monotonous DTS in output stream 0:1; previous: 220, current: 87; changing to 221. This may result in incorrect timestamps in the output file.
> [avi @ 0x5601caccc180] Non-monotonous DTS in output stream 0:1; previous: 221, current: 88; changing to 222. This may result in incorrect timestamps in the output file.
> [avi @ 0x5601caccc180] Non-monotonous DTS in output stream 0:1; previous: 222, current: 89; changing to 223. This may result in incorrect timestamps in the output file.
> [avi @ 0x5601caccc180] Non-monotonous DTS in output stream 0:1; previous: 223, current: 90; changing to 224. This may result in incorrect timestamps in the output file.
> [avi @ 0x5601caccc180] Non-monotonous DTS in output stream 0:1; previous: 224, current: 91; changing to 225. This may result in incorrect timestamps in the output file.
> [mpeg4 @ 0x5601cac87540] Invalid pts (72) <= last (83)
> Video encoding failed

Yes, if the input includes a mid-stream dts reset of gap less than the 
default value of dts_delta_threshold then timestamps won't be made 
continuous. Default value is 10 seconds but till now it wasn't enforced 
for negative jumps, so your command worked. With this change, user will 
have to set it manually if they expect smaller jumps.

What can be done is to set the default value to 0, and then set separate 
values for positive and negative jumps inside the block to keep current 
behaviour. If user-set to non-zero, then the user's value is honoured.

Gyan
Michael Niedermayer April 15, 2020, 11:33 a.m. UTC | #3
On Wed, Apr 15, 2020 at 10:55:47AM +0530, Gyan Doshi wrote:
> 
> 
> On 15-04-2020 01:33 am, Michael Niedermayer wrote:
> >On Tue, Apr 14, 2020 at 02:48:47PM +0530, Gyan Doshi wrote:
> >>For inputs from demuxers with AVFMT_TS_DISCONT flag,
> >>the existing condition,
> >>
> >>   delta < -1LL*dts_delta_threshold*AV_TIME_BASE
> >>
> >>is rendered superflous due to the fixed threshold in
> >>
> >>pkt_dts + AV_TIME_BASE/10 < FFMAX(ist->pts, ist->dts)
> >>
> >>This prevents users from setting a high threshold to
> >>avoid discontinuity correction due to errant timestamps.
> >>
> >>Now, the maximum of the two thresholds is used.
> >>
> >>fate-mpeg4-resolution-change call changed to preserve existing
> >>timestamp correction by ffmpeg.c
> >>---
> >>Tested with multiple satellite MPEG-TS inputs.
> >>
> >>  fftools/ffmpeg.c     | 2 +-
> >>  tests/fate/mpeg4.mak | 2 +-
> >>  2 files changed, 2 insertions(+), 2 deletions(-)
> >breaks:
> >./ffmpeg -i 'concat:angels.mpg|angels.mpg'  -vsync 0  file.avi
> >
> >...
> >frame=   24 fps=0.0 q=12.5 Lsize=     215kB time=00:00:07.50 bitrate= 234.7kbits/s speed=75.5x
> >video:84kB audio:110kB subtitle:0kB other streams:0kB global headers:0kB muxing overhead: 10.697786%
> >
> >vs.
> >
> >...
> >[avi @ 0x5601caccc180] Non-monotonous DTS in output stream 0:1; previous: 219, current: 86; changing to 220. This may result in incorrect timestamps in the output file.
> >[avi @ 0x5601caccc180] Non-monotonous DTS in output stream 0:1; previous: 220, current: 87; changing to 221. This may result in incorrect timestamps in the output file.
> >[avi @ 0x5601caccc180] Non-monotonous DTS in output stream 0:1; previous: 221, current: 88; changing to 222. This may result in incorrect timestamps in the output file.
> >[avi @ 0x5601caccc180] Non-monotonous DTS in output stream 0:1; previous: 222, current: 89; changing to 223. This may result in incorrect timestamps in the output file.
> >[avi @ 0x5601caccc180] Non-monotonous DTS in output stream 0:1; previous: 223, current: 90; changing to 224. This may result in incorrect timestamps in the output file.
> >[avi @ 0x5601caccc180] Non-monotonous DTS in output stream 0:1; previous: 224, current: 91; changing to 225. This may result in incorrect timestamps in the output file.
> >[mpeg4 @ 0x5601cac87540] Invalid pts (72) <= last (83)
> >Video encoding failed
> 
> Yes, if the input includes a mid-stream dts reset of gap less than the
> default value of dts_delta_threshold then timestamps won't be made
> continuous. Default value is 10 seconds but till now it wasn't enforced for
> negative jumps, so your command worked. With this change, user will have to
> set it manually if they expect smaller jumps.

Negative jumps should always be a discontinuity more or less.
What are you trying to fix exactly ?

is this about bitstream errors in the timestamps ?

thx

[...]
Gyan Doshi April 15, 2020, 11:59 a.m. UTC | #4
On 15-04-2020 05:03 pm, Michael Niedermayer wrote:
> On Wed, Apr 15, 2020 at 10:55:47AM +0530, Gyan Doshi wrote:
>>
>> On 15-04-2020 01:33 am, Michael Niedermayer wrote:
>>> On Tue, Apr 14, 2020 at 02:48:47PM +0530, Gyan Doshi wrote:
>>>> For inputs from demuxers with AVFMT_TS_DISCONT flag,
>>>> the existing condition,
>>>>
>>>>    delta < -1LL*dts_delta_threshold*AV_TIME_BASE
>>>>
>>>> is rendered superflous due to the fixed threshold in
>>>>
>>>> pkt_dts + AV_TIME_BASE/10 < FFMAX(ist->pts, ist->dts)
>>>>
>>>> This prevents users from setting a high threshold to
>>>> avoid discontinuity correction due to errant timestamps.
>>>>
>>>> Now, the maximum of the two thresholds is used.
>>>>
>>>> fate-mpeg4-resolution-change call changed to preserve existing
>>>> timestamp correction by ffmpeg.c
>>>> ---
>>>> Tested with multiple satellite MPEG-TS inputs.
>>>>
>>>>   fftools/ffmpeg.c     | 2 +-
>>>>   tests/fate/mpeg4.mak | 2 +-
>>>>   2 files changed, 2 insertions(+), 2 deletions(-)
>>> breaks:
>>> ./ffmpeg -i 'concat:angels.mpg|angels.mpg'  -vsync 0  file.avi
>>>
>>> ...
>>> frame=   24 fps=0.0 q=12.5 Lsize=     215kB time=00:00:07.50 bitrate= 234.7kbits/s speed=75.5x
>>> video:84kB audio:110kB subtitle:0kB other streams:0kB global headers:0kB muxing overhead: 10.697786%
>>>
>>> vs.
>>>
>>> ...
>>> [avi @ 0x5601caccc180] Non-monotonous DTS in output stream 0:1; previous: 219, current: 86; changing to 220. This may result in incorrect timestamps in the output file.
>>> [avi @ 0x5601caccc180] Non-monotonous DTS in output stream 0:1; previous: 220, current: 87; changing to 221. This may result in incorrect timestamps in the output file.
>>> [avi @ 0x5601caccc180] Non-monotonous DTS in output stream 0:1; previous: 221, current: 88; changing to 222. This may result in incorrect timestamps in the output file.
>>> [avi @ 0x5601caccc180] Non-monotonous DTS in output stream 0:1; previous: 222, current: 89; changing to 223. This may result in incorrect timestamps in the output file.
>>> [avi @ 0x5601caccc180] Non-monotonous DTS in output stream 0:1; previous: 223, current: 90; changing to 224. This may result in incorrect timestamps in the output file.
>>> [avi @ 0x5601caccc180] Non-monotonous DTS in output stream 0:1; previous: 224, current: 91; changing to 225. This may result in incorrect timestamps in the output file.
>>> [mpeg4 @ 0x5601cac87540] Invalid pts (72) <= last (83)
>>> Video encoding failed
>> Yes, if the input includes a mid-stream dts reset of gap less than the
>> default value of dts_delta_threshold then timestamps won't be made
>> continuous. Default value is 10 seconds but till now it wasn't enforced for
>> negative jumps, so your command worked. With this change, user will have to
>> set it manually if they expect smaller jumps.
> Negative jumps should always be a discontinuity more or less.
> What are you trying to fix exactly ?
>
> is this about bitstream errors in the timestamps ?

Yes. This is when there's a backward jump in one of the streams due to 
corruption in the input and that leads to a new ts_offset. This new 
offset when applied to other streams with error-free timestamps leads to 
async as well as muxing errors due to (now) increased interval in the 
muxing queue. dts_delta_threshold (in combination with filtering) should 
allow to prevent these offset adjustments at the cost of filtering out 
or adjusting a few packets, but the existing check prevents that by 
making    'delta < -1LL*dts_delta_threshold*AV_TIME_BASE'    irrelevant  
. With this patch, the user can set a threshold and avoid unwanted 
offset adjustments due to negative jumps.

Gyan
Michael Niedermayer April 15, 2020, 5:09 p.m. UTC | #5
On Wed, Apr 15, 2020 at 05:29:06PM +0530, Gyan Doshi wrote:
> 
> 
> On 15-04-2020 05:03 pm, Michael Niedermayer wrote:
> >On Wed, Apr 15, 2020 at 10:55:47AM +0530, Gyan Doshi wrote:
> >>
> >>On 15-04-2020 01:33 am, Michael Niedermayer wrote:
> >>>On Tue, Apr 14, 2020 at 02:48:47PM +0530, Gyan Doshi wrote:
> >>>>For inputs from demuxers with AVFMT_TS_DISCONT flag,
> >>>>the existing condition,
> >>>>
> >>>>   delta < -1LL*dts_delta_threshold*AV_TIME_BASE
> >>>>
> >>>>is rendered superflous due to the fixed threshold in
> >>>>
> >>>>pkt_dts + AV_TIME_BASE/10 < FFMAX(ist->pts, ist->dts)
> >>>>
> >>>>This prevents users from setting a high threshold to
> >>>>avoid discontinuity correction due to errant timestamps.
> >>>>
> >>>>Now, the maximum of the two thresholds is used.
> >>>>
> >>>>fate-mpeg4-resolution-change call changed to preserve existing
> >>>>timestamp correction by ffmpeg.c
> >>>>---
> >>>>Tested with multiple satellite MPEG-TS inputs.
> >>>>
> >>>>  fftools/ffmpeg.c     | 2 +-
> >>>>  tests/fate/mpeg4.mak | 2 +-
> >>>>  2 files changed, 2 insertions(+), 2 deletions(-)
> >>>breaks:
> >>>./ffmpeg -i 'concat:angels.mpg|angels.mpg'  -vsync 0  file.avi
> >>>
> >>>...
> >>>frame=   24 fps=0.0 q=12.5 Lsize=     215kB time=00:00:07.50 bitrate= 234.7kbits/s speed=75.5x
> >>>video:84kB audio:110kB subtitle:0kB other streams:0kB global headers:0kB muxing overhead: 10.697786%
> >>>
> >>>vs.
> >>>
> >>>...
> >>>[avi @ 0x5601caccc180] Non-monotonous DTS in output stream 0:1; previous: 219, current: 86; changing to 220. This may result in incorrect timestamps in the output file.
> >>>[avi @ 0x5601caccc180] Non-monotonous DTS in output stream 0:1; previous: 220, current: 87; changing to 221. This may result in incorrect timestamps in the output file.
> >>>[avi @ 0x5601caccc180] Non-monotonous DTS in output stream 0:1; previous: 221, current: 88; changing to 222. This may result in incorrect timestamps in the output file.
> >>>[avi @ 0x5601caccc180] Non-monotonous DTS in output stream 0:1; previous: 222, current: 89; changing to 223. This may result in incorrect timestamps in the output file.
> >>>[avi @ 0x5601caccc180] Non-monotonous DTS in output stream 0:1; previous: 223, current: 90; changing to 224. This may result in incorrect timestamps in the output file.
> >>>[avi @ 0x5601caccc180] Non-monotonous DTS in output stream 0:1; previous: 224, current: 91; changing to 225. This may result in incorrect timestamps in the output file.
> >>>[mpeg4 @ 0x5601cac87540] Invalid pts (72) <= last (83)
> >>>Video encoding failed
> >>Yes, if the input includes a mid-stream dts reset of gap less than the
> >>default value of dts_delta_threshold then timestamps won't be made
> >>continuous. Default value is 10 seconds but till now it wasn't enforced for
> >>negative jumps, so your command worked. With this change, user will have to
> >>set it manually if they expect smaller jumps.
> >Negative jumps should always be a discontinuity more or less.
> >What are you trying to fix exactly ?
> >
> >is this about bitstream errors in the timestamps ?
> 
> Yes. This is when there's a backward jump in one of the streams due to
> corruption in the input and that leads to a new ts_offset. This new offset
> when applied to other streams with error-free timestamps leads to async as
> well as muxing errors due to (now) increased interval in the muxing queue.
> dts_delta_threshold (in combination with filtering) should allow to prevent
> these offset adjustments at the cost of filtering out or adjusting a few
> packets, but the existing check prevents that by making    'delta <
> -1LL*dts_delta_threshold*AV_TIME_BASE'    irrelevant  . With this patch, the
> user can set a threshold and avoid unwanted offset adjustments due to
> negative jumps.

iam still not sure we talk about the same thing
if there is corruption in the timestamp field then on average
the timestamp delta will be large and a threshold will not work
in seperating that reliably from discontinuities.

to detect / filter such issues looking at more than just the next
timestamp would be needed
as in
5,6,7,79861 you dont know if this is
5,6,7,79861,79862,79863
or
5,6,7,79861,9,10,
without seeing the next values

but maybe you are talking about a different problem


[...]
Gyan Doshi April 15, 2020, 5:37 p.m. UTC | #6
On 15-04-2020 10:39 pm, Michael Niedermayer wrote:
> On Wed, Apr 15, 2020 at 05:29:06PM +0530, Gyan Doshi wrote:
>>
>> On 15-04-2020 05:03 pm, Michael Niedermayer wrote:
>>> On Wed, Apr 15, 2020 at 10:55:47AM +0530, Gyan Doshi wrote:
>>>> On 15-04-2020 01:33 am, Michael Niedermayer wrote:
>>>>> On Tue, Apr 14, 2020 at 02:48:47PM +0530, Gyan Doshi wrote:
>>>>>> For inputs from demuxers with AVFMT_TS_DISCONT flag,
>>>>>> the existing condition,
>>>>>>
>>>>>>    delta < -1LL*dts_delta_threshold*AV_TIME_BASE
>>>>>>
>>>>>> is rendered superflous due to the fixed threshold in
>>>>>>
>>>>>> pkt_dts + AV_TIME_BASE/10 < FFMAX(ist->pts, ist->dts)
>>>>>>
>>>>>> This prevents users from setting a high threshold to
>>>>>> avoid discontinuity correction due to errant timestamps.
>>>>>>
>>>>>> Now, the maximum of the two thresholds is used.
>>>>>>
>>>>>> fate-mpeg4-resolution-change call changed to preserve existing
>>>>>> timestamp correction by ffmpeg.c
>>>>>> ---
>>>>>> Tested with multiple satellite MPEG-TS inputs.
>>>>>>
>>>>>>   fftools/ffmpeg.c     | 2 +-
>>>>>>   tests/fate/mpeg4.mak | 2 +-
>>>>>>   2 files changed, 2 insertions(+), 2 deletions(-)
>>>>> breaks:
>>>>> ./ffmpeg -i 'concat:angels.mpg|angels.mpg'  -vsync 0  file.avi
>>>>>
>>>>> ...
>>>>> frame=   24 fps=0.0 q=12.5 Lsize=     215kB time=00:00:07.50 bitrate= 234.7kbits/s speed=75.5x
>>>>> video:84kB audio:110kB subtitle:0kB other streams:0kB global headers:0kB muxing overhead: 10.697786%
>>>>>
>>>>> vs.
>>>>>
>>>>> ...
>>>>> [avi @ 0x5601caccc180] Non-monotonous DTS in output stream 0:1; previous: 219, current: 86; changing to 220. This may result in incorrect timestamps in the output file.
>>>>> [avi @ 0x5601caccc180] Non-monotonous DTS in output stream 0:1; previous: 220, current: 87; changing to 221. This may result in incorrect timestamps in the output file.
>>>>> [avi @ 0x5601caccc180] Non-monotonous DTS in output stream 0:1; previous: 221, current: 88; changing to 222. This may result in incorrect timestamps in the output file.
>>>>> [avi @ 0x5601caccc180] Non-monotonous DTS in output stream 0:1; previous: 222, current: 89; changing to 223. This may result in incorrect timestamps in the output file.
>>>>> [avi @ 0x5601caccc180] Non-monotonous DTS in output stream 0:1; previous: 223, current: 90; changing to 224. This may result in incorrect timestamps in the output file.
>>>>> [avi @ 0x5601caccc180] Non-monotonous DTS in output stream 0:1; previous: 224, current: 91; changing to 225. This may result in incorrect timestamps in the output file.
>>>>> [mpeg4 @ 0x5601cac87540] Invalid pts (72) <= last (83)
>>>>> Video encoding failed
>>>> Yes, if the input includes a mid-stream dts reset of gap less than the
>>>> default value of dts_delta_threshold then timestamps won't be made
>>>> continuous. Default value is 10 seconds but till now it wasn't enforced for
>>>> negative jumps, so your command worked. With this change, user will have to
>>>> set it manually if they expect smaller jumps.
>>> Negative jumps should always be a discontinuity more or less.
>>> What are you trying to fix exactly ?
>>>
>>> is this about bitstream errors in the timestamps ?
>> Yes. This is when there's a backward jump in one of the streams due to
>> corruption in the input and that leads to a new ts_offset. This new offset
>> when applied to other streams with error-free timestamps leads to async as
>> well as muxing errors due to (now) increased interval in the muxing queue.
>> dts_delta_threshold (in combination with filtering) should allow to prevent
>> these offset adjustments at the cost of filtering out or adjusting a few
>> packets, but the existing check prevents that by making    'delta <
>> -1LL*dts_delta_threshold*AV_TIME_BASE'    irrelevant  . With this patch, the
>> user can set a threshold and avoid unwanted offset adjustments due to
>> negative jumps.
> iam still not sure we talk about the same thing
> if there is corruption in the timestamp field then on average
> the timestamp delta will be large and a threshold will not work
> in seperating that reliably from discontinuities.
>
> to detect / filter such issues looking at more than just the next
> timestamp would be needed
> as in
> 5,6,7,79861 you dont know if this is
> 5,6,7,79861,79862,79863
> or
> 5,6,7,79861,9,10,
> without seeing the next values

Right. So I'm talking about scenarios where the user *knows* that the 
input is supposed to be

85006,85007,85008,85009,...95443,0,1,2,3,4,5...

but is received as

85006,85007,24104,85009...95443,0,1,2,14131,4,5...

Without this patch, user can set a threshold to ignore 2,14131 but not 85007,24104.

Gyan
Michael Niedermayer April 16, 2020, 6:56 p.m. UTC | #7
On Wed, Apr 15, 2020 at 11:07:05PM +0530, Gyan Doshi wrote:
> 
> 
> On 15-04-2020 10:39 pm, Michael Niedermayer wrote:
> >On Wed, Apr 15, 2020 at 05:29:06PM +0530, Gyan Doshi wrote:
> >>
> >>On 15-04-2020 05:03 pm, Michael Niedermayer wrote:
> >>>On Wed, Apr 15, 2020 at 10:55:47AM +0530, Gyan Doshi wrote:
> >>>>On 15-04-2020 01:33 am, Michael Niedermayer wrote:
> >>>>>On Tue, Apr 14, 2020 at 02:48:47PM +0530, Gyan Doshi wrote:
> >>>>>>For inputs from demuxers with AVFMT_TS_DISCONT flag,
> >>>>>>the existing condition,
> >>>>>>
> >>>>>>   delta < -1LL*dts_delta_threshold*AV_TIME_BASE
> >>>>>>
> >>>>>>is rendered superflous due to the fixed threshold in
> >>>>>>
> >>>>>>pkt_dts + AV_TIME_BASE/10 < FFMAX(ist->pts, ist->dts)
> >>>>>>
> >>>>>>This prevents users from setting a high threshold to
> >>>>>>avoid discontinuity correction due to errant timestamps.
> >>>>>>
> >>>>>>Now, the maximum of the two thresholds is used.
> >>>>>>
> >>>>>>fate-mpeg4-resolution-change call changed to preserve existing
> >>>>>>timestamp correction by ffmpeg.c
> >>>>>>---
> >>>>>>Tested with multiple satellite MPEG-TS inputs.
> >>>>>>
> >>>>>>  fftools/ffmpeg.c     | 2 +-
> >>>>>>  tests/fate/mpeg4.mak | 2 +-
> >>>>>>  2 files changed, 2 insertions(+), 2 deletions(-)
> >>>>>breaks:
> >>>>>./ffmpeg -i 'concat:angels.mpg|angels.mpg'  -vsync 0  file.avi
> >>>>>
> >>>>>...
> >>>>>frame=   24 fps=0.0 q=12.5 Lsize=     215kB time=00:00:07.50 bitrate= 234.7kbits/s speed=75.5x
> >>>>>video:84kB audio:110kB subtitle:0kB other streams:0kB global headers:0kB muxing overhead: 10.697786%
> >>>>>
> >>>>>vs.
> >>>>>
> >>>>>...
> >>>>>[avi @ 0x5601caccc180] Non-monotonous DTS in output stream 0:1; previous: 219, current: 86; changing to 220. This may result in incorrect timestamps in the output file.
> >>>>>[avi @ 0x5601caccc180] Non-monotonous DTS in output stream 0:1; previous: 220, current: 87; changing to 221. This may result in incorrect timestamps in the output file.
> >>>>>[avi @ 0x5601caccc180] Non-monotonous DTS in output stream 0:1; previous: 221, current: 88; changing to 222. This may result in incorrect timestamps in the output file.
> >>>>>[avi @ 0x5601caccc180] Non-monotonous DTS in output stream 0:1; previous: 222, current: 89; changing to 223. This may result in incorrect timestamps in the output file.
> >>>>>[avi @ 0x5601caccc180] Non-monotonous DTS in output stream 0:1; previous: 223, current: 90; changing to 224. This may result in incorrect timestamps in the output file.
> >>>>>[avi @ 0x5601caccc180] Non-monotonous DTS in output stream 0:1; previous: 224, current: 91; changing to 225. This may result in incorrect timestamps in the output file.
> >>>>>[mpeg4 @ 0x5601cac87540] Invalid pts (72) <= last (83)
> >>>>>Video encoding failed
> >>>>Yes, if the input includes a mid-stream dts reset of gap less than the
> >>>>default value of dts_delta_threshold then timestamps won't be made
> >>>>continuous. Default value is 10 seconds but till now it wasn't enforced for
> >>>>negative jumps, so your command worked. With this change, user will have to
> >>>>set it manually if they expect smaller jumps.
> >>>Negative jumps should always be a discontinuity more or less.
> >>>What are you trying to fix exactly ?
> >>>
> >>>is this about bitstream errors in the timestamps ?
> >>Yes. This is when there's a backward jump in one of the streams due to
> >>corruption in the input and that leads to a new ts_offset. This new offset
> >>when applied to other streams with error-free timestamps leads to async as
> >>well as muxing errors due to (now) increased interval in the muxing queue.
> >>dts_delta_threshold (in combination with filtering) should allow to prevent
> >>these offset adjustments at the cost of filtering out or adjusting a few
> >>packets, but the existing check prevents that by making    'delta <
> >>-1LL*dts_delta_threshold*AV_TIME_BASE'    irrelevant  . With this patch, the
> >>user can set a threshold and avoid unwanted offset adjustments due to
> >>negative jumps.
> >iam still not sure we talk about the same thing
> >if there is corruption in the timestamp field then on average
> >the timestamp delta will be large and a threshold will not work
> >in seperating that reliably from discontinuities.
> >
> >to detect / filter such issues looking at more than just the next
> >timestamp would be needed
> >as in
> >5,6,7,79861 you dont know if this is
> >5,6,7,79861,79862,79863
> >or
> >5,6,7,79861,9,10,
> >without seeing the next values
> 
> Right. So I'm talking about scenarios where the user *knows* that the input
> is supposed to be
> 
> 85006,85007,85008,85009,...95443,0,1,2,3,4,5...
> 
> but is received as
> 
> 85006,85007,24104,85009...95443,0,1,2,14131,4,5...
> 
> Without this patch, user can set a threshold to ignore 2,14131 but not 85007,24104.

dts_delta_threshold is not there for detecting bitstream errors in timestamps
also its not able to be used for that.
This just works in the one sample you show here.
A bitstream error would generally be bigger than a discontinuity so it would
be the other way around

If you want to filter out such individual timestamps that are bad that should
be done by a filter maybe a bitstream filter that has access to more future
timestamps.

Also theres another issue, lets just assume there is a error and it does 
get detected as a timestamp discontinuity. The next frame afterwards too
should be detected as a discontinuity and these 2 discontinuity when being
handled should remove the error with no ill effects
That IIUC does not happen in your case, finding out why not is something
thats probably worth investigating

thx


[...]
diff mbox series

Patch

diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
index 0578265c1e..505fef5bdc 100644
--- a/fftools/ffmpeg.c
+++ b/fftools/ffmpeg.c
@@ -4480,7 +4480,7 @@  static int process_input(int file_index)
         if (is->iformat->flags & AVFMT_TS_DISCONT) {
             if (delta < -1LL*dts_delta_threshold*AV_TIME_BASE ||
                 delta >  1LL*dts_delta_threshold*AV_TIME_BASE ||
-                pkt_dts + AV_TIME_BASE/10 < FFMAX(ist->pts, ist->dts)) {
+                pkt_dts + FFMAX(AV_TIME_BASE/10, dts_delta_threshold*AV_TIME_BASE) < FFMAX(ist->pts, ist->dts)) {
                 ifile->ts_offset -= delta;
                 av_log(NULL, AV_LOG_DEBUG,
                        "timestamp discontinuity for stream #%d:%d "
diff --git a/tests/fate/mpeg4.mak b/tests/fate/mpeg4.mak
index ed6a2fac20..d7a9c62d7e 100644
--- a/tests/fate/mpeg4.mak
+++ b/tests/fate/mpeg4.mak
@@ -1,7 +1,7 @@ 
 
 MPEG4_RESOLUTION_CHANGE = down-down down-up up-down up-up
 
-fate-mpeg4-resolution-change-%: CMD = framemd5 -flags +bitexact -idct simple -i $(TARGET_SAMPLES)/mpeg4/resize_$(@:fate-mpeg4-resolution-change-%=%).h263 -sws_flags +bitexact
+fate-mpeg4-resolution-change-%: CMD = framemd5 -dts_delta_threshold 1.99 -flags +bitexact -idct simple -i $(TARGET_SAMPLES)/mpeg4/resize_$(@:fate-mpeg4-resolution-change-%=%).h263 -sws_flags +bitexact
 
 FATE_MPEG4-$(call DEMDEC, H263, H263) := $(addprefix fate-mpeg4-resolution-change-, $(MPEG4_RESOLUTION_CHANGE))