diff mbox

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

Message ID 20180329101439.GD20131@michaelspb
State Not Applicable
Headers show

Commit Message

Michael Niedermayer March 29, 2018, 10:14 a.m. UTC 
On Thu, Mar 29, 2018 at 12:53:18AM +0300, Jan Ekström wrote:
> From: Jan Ekström <jan.ekstrom@aminocom.com>
> 
> With some streams multiple nullptr AVSubtitles can get pushed
> into sub2video_update() in a row.
> 
> This causes end_pts, and on the next round pts, to become
> INT64_MAX, latter of which signals EOF in framesync, leading to
> complete loss of subtitles from that point on.
> 
> Thus, utilize the previous sub2video.end_pts as both the pts and
> end_pts in case a nullptr AVSubtitle is received. This lets further
> incoming subtitle packets be properly processed, as EOF is not hit
> in framesync.
> 
> Signed-off-by: Jan Ekström <jan.ekstrom@aminocom.com>
> ---
>  fftools/ffmpeg.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

this breaks fate-sub2video

TEST    sub2video
Test sub2video failed. Look at tests/data/fate/sub2video.err for details.
make: *** [fate-sub2video] Error 1

[...]

Comments

Jan Ekström March 29, 2018, 2:25 p.m. UTC | #1 
On Thu, Mar 29, 2018 at 1:14 PM, Michael Niedermayer
<michael@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
Jan Ekström March 30, 2018, 3:44 p.m. UTC | #2 
On Thu, Mar 29, 2018 at 5:25 PM, Jan Ekström <jeebjp@gmail.com> wrote:
> On Thu, Mar 29, 2018 at 1:14 PM, Michael Niedermayer
> <michael@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
diff mbox

Patch

--- ./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