Message ID | 20180331170904.21797-1-jeebjp@gmail.com |
---|---|
State | Accepted |
Commit | e760c12aeef608aa8b416664687b9aca3a2c6f68 |
Headers | show |
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
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);