[FFmpeg-devel] [PATCH] ffmpeg: prevent premature EOF in sub2video with nullptr AVSubtitles

[Jan Ekström]

On Thu, Mar 29, 2018 at 5:25 PM, Jan Ekström <jeebjp at gmail.com> wrote:
> On Thu, Mar 29, 2018 at 1:14 PM, Michael Niedermayer
> <michael at niedermayer.cc> wrote:
>>
>> this breaks fate-sub2video
>>
>> TEST    sub2video
>> --- ./tests/ref/fate/sub2video  2018-03-29 02:30:48.095578219 +0200
>> +++ tests/data/fate/sub2video   2018-03-29 12:13:25.191428538 +0200
>> @@ -68,7 +68,8 @@
>>  0,        258,        258,        1,   518400, 0x34cdddee
>>  0,        269,        269,        1,   518400, 0xbab197ea
>>  1,   53910000,   53910000,  2696000,     2095, 0x61bb15ed, F=0x0
>> -0,        270,        270,        1,   518400, 0x4db4ce51
>> +0,        270,        270,        1,   518400, 0xbab197ea
>> +0,        271,        271,        1,   518400, 0x4db4ce51
>>  0,        283,        283,        1,   518400, 0xbab197ea
>>  1,   56663000,   56663000,  1262000,     1013, 0xc9ae89b7, F=0x0
>>  0,        284,        284,        1,   518400, 0xe6bc0ea9
>> @@ -137,7 +138,7 @@
>>  1,  168049000,  168049000,  1900000,     1312, 0x0bf20e8d, F=0x0
>>  0,        850,        850,        1,   518400, 0xbab197ea
>>  1,  170035000,  170035000,  1524000,     1279, 0xb6c2dafe, F=0x0
>> -0,        851,        851,        1,   518400, 0x8780239e
>> +0,        851,        851,        1,   518400, 0xbab197ea
>>  0,        858,        858,        1,   518400, 0xbab197ea
>>  0,        861,        861,        1,   518400, 0x6eb72347
>>  1,  172203000,  172203000,  1695000,     1826, 0x9a1ac769, F=0x0
>> @@ -161,7 +162,8 @@
>>  0,        976,        976,        1,   518400, 0x923d1ce7
>>  0,        981,        981,        1,   518400, 0xbab197ea
>>  1,  196361000,  196361000,  1524000,     1715, 0x695ca41e, F=0x0
>> -0,        982,        982,        1,   518400, 0x6e652cd2
>> +0,        982,        982,        1,   518400, 0xbab197ea
>> +0,        983,        983,        1,   518400, 0x6e652cd2
>>  0,        989,        989,        1,   518400, 0xbab197ea
>>  1,  197946000,  197946000,  1160000,      789, 0xc63a189e, F=0x0
>>  0,        990,        990,        1,   518400, 0x25113966
>> Test sub2video failed. Look at tests/data/fate/sub2video.err for details.
>> make: *** [fate-sub2video] Error 1
>
> Thanks. I tried running this last night but it required some of the
> samples in FATE so I decided to re-run it today. Will check if the
> change is correct. For the reference, this change has now been running
> in a testing setup for at least 24h with the subtitles still being
> overlayed correctly, so the change seems alright by the general
> metrics (that I can gather).
>
> Jan

OK, I have run this test at 25fps and 30/1.001fps in addition to the
5fps it runs at by default. I find it very interesting at how the
output of the filter gets all VFR in some cases, while you'd think
that taking in a rawvideo clip with a static fps set would lead to
constant output of frames?

In any case, the diff with 5fps is what has been posted and the three diffs are:
1. subtitle with timestamp 53910000 (53.91 -> 56.606)
  * Frame that ends up being shown 54.0 -> 56.6 (although original
duration is just 1/5 seconds) gets replaced with one frame that is
shown 54.0 -> 54.2 (no subtitle shown) and another that is 54.2 ->
56.6 (subtitle shown, this as well seems to originally only have a
duration of 1/5s)
2. subtitle with timestamp 170035000 (170.035 -> 171.559)
  * Frame that ends up being shown 170.2 -> 171.6 (although original
duration is just 1/5 seconds) gets replaced with a frame without a
subtitle.
3. subtitle with timestamp 196361000 (196.361 -> 197.885)
  * Similar to nr1, one frame in a VFR spot replaced with two of which
the first one has no subtitle.

With higher frame rates the output of the filter chain stayed VFR, but
you only had frames added in some spots but none replaced. I recall
all of those frames not being directly relevant to subtitles in that
case, but I will double-check as I was getting tired at that point.

Another alternative would of course to be not to send the nullptr
AVSubtitle with what would be INT64_MAX further into the filter chain
at all (this change doesn't change that at all, the "heartbeat" thing
gets pushed into the filter chain). I'm just not 100% clear which is
the correct place to control this.
a) in `sub2video_update`, in the else case you could add `if
(ist->sub2video.end_pts == INT64_MAX) return;` to make sure that value
wouldn't get propagated into an EOF.
b)  in `sub2video_heartbeat`, changing the check from just checking
that data[0] is falsie to also checking if `sub2video.end_pts !=
INT64_MAX`, since I think this is the spot that sends out those
nullptr AVSubtitles to `sub2video_update`.
c) somewhere else which causes this to happen when the filter chain
gets re-initialized/configured because of a seemingly unrelated
stream?

Additionally, I just noticed as an unrelated issue that the subtitles'
scaling seems to get forgotten with the re-init/configuration as well.
So you can end up with SD dvb subtitles being no longer scaled on top
of a HD video frame correctly afterwards. I just seem to have been
working around this by always making sure to re-scale the subtitle
overlay to the (known) video resolution. But this is a separate issue,
for now getting the premature EOF fixed for the overlay input is the
theme of this thread :) .

Best regards,
Jan