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

With certain types of input and the filter chain getting re-initialized
or re-configured, multiple nullptr AVSubtitles can get pushed into
sub2video_update() in a row from sub2video_heartbeat.

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, check that the sub2video.end_pts is smaller than INT64_MAX
in a similar fashion to sub2video_flush before sending out the
nullptr AVSubtitle. This keeps premature EOFs from happening in
framesync and the subtitle overlay is kept past the filter chain
re-initializations/configurations.
---
Alternative patch to the first one, not really a v2 so not marking
it as such.

The initial patch tried to still pass on the updates but keeping the
end_pts from becoming INT64_MAX (as it would get utilized as the pts
for the next packet). This changed the sub2video FATE test's
results as well as made the theoretical secondary stream not being
possible to EOF with nullptr AVSubtitles. That said, I am not sure
if the test's changes or the change in capability to do such
EOF'ification  were explicitly incorrect, but clearly my knowledge in
this area was not perfect and the change in FATE test probably kept
everyone away from the initial patch. For the record, the initial patch
worked for way more than 48h without seeming problems with a CFR stream,
so the previous patch was not as if not tested.

Thus, this alternative patch instead opts to never send out new nullptr
AVSubtitle updates if end_pts is already at INT64_MAX. The FATE test
also is unchanged.
---
 fftools/ffmpeg.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

On 3/31/18, Jan Ekstroem <jeebjp@gmail.com> wrote:
> With certain types of input and the filter chain getting re-initialized
> or re-configured, multiple nullptr AVSubtitles can get pushed into
> sub2video_update() in a row from sub2video_heartbeat.
>
> 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, check that the sub2video.end_pts is smaller than INT64_MAX
> in a similar fashion to sub2video_flush before sending out the
> nullptr AVSubtitle. This keeps premature EOFs from happening in
> framesync and the subtitle overlay is kept past the filter chain
> re-initializations/configurations.
> ---
> Alternative patch to the first one, not really a v2 so not marking
> it as such.
>
> The initial patch tried to still pass on the updates but keeping the
> end_pts from becoming INT64_MAX (as it would get utilized as the pts
> for the next packet). This changed the sub2video FATE test's
> results as well as made the theoretical secondary stream not being
> possible to EOF with nullptr AVSubtitles. That said, I am not sure
> if the test's changes or the change in capability to do such
> EOF'ification  were explicitly incorrect, but clearly my knowledge in
> this area was not perfect and the change in FATE test probably kept
> everyone away from the initial patch. For the record, the initial patch
> worked for way more than 48h without seeming problems with a CFR stream,
> so the previous patch was not as if not tested.
>
> Thus, this alternative patch instead opts to never send out new nullptr
> AVSubtitle updates if end_pts is already at INT64_MAX. The FATE test
> also is unchanged.
> ---
>  fftools/ffmpeg.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>

lgtm

Jan Ekström (2018-03-31):
> With certain types of input and the filter chain getting re-initialized
> or re-configured, multiple nullptr AVSubtitles can get pushed into
> sub2video_update() in a row from sub2video_heartbeat.
> 
> 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, check that the sub2video.end_pts is smaller than INT64_MAX
> in a similar fashion to sub2video_flush before sending out the
> nullptr AVSubtitle. This keeps premature EOFs from happening in
> framesync and the subtitle overlay is kept past the filter chain
> re-initializations/configurations.
> ---
> Alternative patch to the first one, not really a v2 so not marking
> it as such.
> 
> The initial patch tried to still pass on the updates but keeping the
> end_pts from becoming INT64_MAX (as it would get utilized as the pts
> for the next packet). This changed the sub2video FATE test's
> results as well as made the theoretical secondary stream not being
> possible to EOF with nullptr AVSubtitles. That said, I am not sure
> if the test's changes or the change in capability to do such
> EOF'ification  were explicitly incorrect, but clearly my knowledge in
> this area was not perfect and the change in FATE test probably kept
> everyone away from the initial patch. For the record, the initial patch
> worked for way more than 48h without seeming problems with a CFR stream,
> so the previous patch was not as if not tested.
> 
> Thus, this alternative patch instead opts to never send out new nullptr
> AVSubtitle updates if end_pts is already at INT64_MAX. The FATE test
> also is unchanged.
> ---
>  fftools/ffmpeg.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> index 4724f62fff..d3bc382dec 100644
> --- a/fftools/ffmpeg.c
> +++ b/fftools/ffmpeg.c
> @@ -285,7 +285,8 @@ static void sub2video_heartbeat(InputStream *ist, int64_t pts)
>          /* do not send the heartbeat frame if the subtitle is already ahead */
>          if (pts2 <= ist2->sub2video.last_pts)
>              continue;
> -        if (pts2 >= ist2->sub2video.end_pts || !ist2->sub2video.frame->data[0])
> +        if (pts2 >= ist2->sub2video.end_pts ||
> +            (!ist2->sub2video.frame->data[0] && ist2->sub2video.end_pts < INT64_MAX))
>              sub2video_update(ist2, NULL);
>          for (j = 0, nb_reqs = 0; j < ist2->nb_filters; j++)
>              nb_reqs += av_buffersrc_get_nb_failed_requests(ist2->filters[j]->filter);

Looks reasonable, thanks.

Regards,

On Sun, Apr 1, 2018 at 1:43 PM, Nicolas George <george@nsup.org> wrote:
>
> Looks reasonable, thanks.
>
> Regards,
>
> --
>   Nicolas George
>

Thanks for the review, pushed.

Jan