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

It only seems to produce duplicate frames.
---
 fftools/ffmpeg_filter.c               | 3 ---
 tests/ref/fate/sub2video_basic        | 3 ---
 tests/ref/fate/sub2video_time_limited | 1 -
 3 files changed, 7 deletions(-)

Anton Khirnov (12023-11-04):
> It only seems to produce duplicate frames.

Yes it does, that is the point of it. Without the duplicated frame, a
filter with synchronized inputs (like overlay) will accumulate video
frames while waiting for the next sub2video frame.

It is actually super easy to test: any file with both video and bitmap
subtitles will cause a huge memory consumption if there is a long enough
time without subtitles.

Since I am feeling super helpful, here is how I just tested:

./ffmpeg_g -lavfi testsrc2=s=720x480:d=150 -preset ultrafast -y /tmp/dummy.mkv
mkvmerge -o /tmp/dummy_with_sub.mkv /tmp/dummy.mkv $ffmpeg_fate/sub/vobsub.idx
limit addressspace 2G
./ffmpeg_g -xerror -i /tmp/dummy_with_sub.mkv -preset ultrafast -lavfi '[0:v][0:s]overlay' -y /tmp/dummy_with_hardsub.mkv
mplayer /tmp/dummy_with_hardsub.mkv -ss 2:05

The limit addressspace 2G is there so that the huge memory consumption
will hit something. Another way is to log the number of queued frames:

--- a/libavfilter/framequeue.c
+++ b/libavfilter/framequeue.c
@@ -90,2 +90,3 @@ int ff_framequeue_add(FFFrameQueue *fq, AVFrame *frame)
     fq->queued++;
+    av_log(0, 16, "queued = %zd\n", fq->queued);
     fq->total_frames_head++;

ffmpeg version N-112710-g86e0dea620 Copyright (c) 2000-2023 the FFmpeg developers
  built with gcc 13 (Debian 13.2.0-5)
  configuration: --enable-shared --disable-static --enable-gpl --enable-libx264 --enable-libopus --enable-libass --enable-libfreetype --enable-opengl --assert-level=2
  libavutil      58. 31.100 / 58. 31.100
  libavcodec     60. 32.102 / 60. 32.102
  libavformat    60. 17.100 / 60. 17.100
  libavdevice    60.  4.100 / 60.  4.100
  libavfilter     9. 13.100 /  9. 13.100
  libswscale      7.  6.100 /  7.  6.100
  libswresample   4. 13.100 /  4. 13.100
  libpostproc    57.  4.100 / 57.  4.100
Input #0, matroska,webm, from '/tmp/dummy_with_sub.mkv':
[…]
queued = 2210
queued = 2211
queued = 2212
queued = 2213
[h264 @ 0x562874ac4f40] get_buffer() failed
[h264 @ 0x562874ac4f40] thread_get_buffer() failed
[h264 @ 0x562874ac4f40] decode_slice_header error
[h264 @ 0x562874ac4f40] no frame!
[…]

Quoting Nicolas George (2023-11-04 15:19:00)
> Anton Khirnov (12023-11-04):
> > It only seems to produce duplicate frames.
> 
> Yes it does, that is the point of it. Without the duplicated frame, a
> filter with synchronized inputs (like overlay) will accumulate video
> frames while waiting for the next sub2video frame.

The problem with that code (and the reason this patch exists), is that
it depends on the order in which the frames arrive on filtergraph
inputs, which is not meaningful (and becomes non-deterministic with
threading).

Anton Khirnov (12023-11-09):
> The problem with that code (and the reason this patch exists), is that
> it depends on the order in which the frames arrive on filtergraph
> inputs, which is not meaningful (and becomes non-deterministic with
> threading).

I can understand that. We will have to find a solution, won't we? (I
hope you do not consider removing a feature that people have been using
for years an option.)

I can suggest this: have demuxer code emit virtual subtitles packets to
trigger the sending of the heartbeat frames.

Regards,

Quoting Nicolas George (2023-11-09 11:47:23)
> Anton Khirnov (12023-11-09):
> > The problem with that code (and the reason this patch exists), is that
> > it depends on the order in which the frames arrive on filtergraph
> > inputs, which is not meaningful (and becomes non-deterministic with
> > threading).
> 
> I can understand that. We will have to find a solution, won't we? (I
> hope you do not consider removing a feature that people have been using
> for years an option.)

I am obviously not proposing that, given the amount of patches I sent so
far to keep sub2video working.

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

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);
 }


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.

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.

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.

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

> 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?

Regards,

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.