[FFmpeg-devel] [PATCH 08/24] fftools/ffmpeg_filter: remove an unnecessary sub2video_push_ref() call

[Anton Khirnov]

Quoting Nicolas George (2023-11-17 10:44:22)
> Anton Khirnov (12023-11-09):
> > I am obviously not proposing that, given the amount of patches I sent so
> > far to keep sub2video working.
> 
> Ok, sorry.
> 
> > 
> > > I can suggest this: have demuxer code emit virtual subtitles packets to
> > > trigger the sending of the heartbeat frames.
> > 
> > That's what already happens, unless I misunderstand what you mean.
> 
> Right now, the heartbeat code path is:
> 
> demuxer → heartbeat → filter
> 
> But the normal frame code path is:
> 
> demuxer → decode frame → filter
> 
> And it causes a problem because decode frame is in a different thread,
> so there is a race condition between the heartbeat frames and the normal
> frames.

There is no race in current master, because the decoding thread only
runs while the main thread is waiting, and vice versa. That changes
after this patchet of course, but...

> Instead, if you make the demuxer generate a dummy packet instead of a
> heartbeat frame, you get these code paths:
> 
> demuxer → real sub packet → decode frame → filter
> demuxer → dummy hb packet → decode frame → filter
> 
> There is no race condition between the real sub packet and the dummy hb
> packet because they both happen in the demuxer thread, and after that
> they follow the same code path so there is no race condition either.

...that is exactly what I'm doing in 19-20/24.

> > 
> > Another possibility is to make the call independently of the state of
> > the graph, like this
> > --- a/fftools/ffmpeg_filter.c
> > +++ b/fftools/ffmpeg_filter.c
> > @@ -2274,8 +2274,7 @@ void ifilter_sub2video_heartbeat(InputFilter *ifilter, int64_t pts, AVRational t
> >             or if we need to initialize the system, update the
> >             overlayed subpicture and its start/end times */
> >          sub2video_update(ifp, pts2 + 1, NULL);
> > -
> > -    if (av_buffersrc_get_nb_failed_requests(ifp->filter))
> > +    else
> >          sub2video_push_ref(ifp, pts2);
> >  }
> 
> Sending frames when they are not wanted can have bad consequences. I do
> not have the time to look for a test case right now, but I have already
> delayed replying to this mail too much.
> 
> I think a situation where subtitles are delayed with the setpts filter
> would trigger a problem with this change: the heartbeat frames will
> accumulate in the input of the overlay filter while without this patch
> only the real subtitles frames accumulate. For a medium delay, normal
> frames would be in the dozens while heartbeat frames would be in the
> thousands.

I do not hink there should be any excessive buffering, because the
scheduling code will simply stop reading from that input and only read
from the lagging one until it catches up.

> > This actually seems to improve filter-overlay-dvdsub-2397, where
> > the first subtitle appears two frames later, which more closely matches
> > the subtitles timestamp stored in the file.
> 
> I would need to look into it. Does it improve it with your changes or
> also with the precedent non-threaded code?

The improvement happens immediately when the change is made, i.e.
without threading. With threading it remains the same.

-- 
Anton Khirnov