[FFmpeg-devel] [PATCH v8 12/13] avfilter/split_cc: Add split_cc filter for closed caption handling

[Soft Works]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of Andreas
> Rheinhardt
> Sent: Wednesday, 22 September 2021 06:55
> To: ffmpeg-devel at ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v8 12/13] avfilter/split_cc: Add split_cc
> filter for closed caption handling
> 
> Soft Works:
> >
> >
> >> -----Original Message-----
> >> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of Andreas
> >> Rheinhardt
> >> Sent: Wednesday, 22 September 2021 05:50
> >> To: ffmpeg-devel at ffmpeg.org
> >> Subject: Re: [FFmpeg-devel] [PATCH v8 12/13] avfilter/split_cc: Add
> split_cc
> >> filter for closed caption handling
> >>
> >> Soft Works:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
> Andreas
> >>>> Rheinhardt
> >>>> Sent: Wednesday, 22 September 2021 04:43
> >>>> To: ffmpeg-devel at ffmpeg.org
> >>>> Subject: Re: [FFmpeg-devel] [PATCH v8 12/13] avfilter/split_cc: Add
> >> split_cc
> >>>> filter for closed caption handling
> >>>>
> >>>> Soft Works:
> >>>>>
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
> >> Andreas
> >>>>>> Rheinhardt
> >>>>>> Sent: Wednesday, 22 September 2021 04:18
> >>>>>> To: ffmpeg-devel at ffmpeg.org
> >>>>>> Subject: Re: [FFmpeg-devel] [PATCH v8 12/13] avfilter/split_cc: Add
> >>>> split_cc
> >>>>>> filter for closed caption handling
> >>>>>>
> >>>>>>> This is a good question, as to whether it is safe to assume that the
> >>>>>>> filter's lifetime doesn't end before the last frame has been
> processed.
> >>>>>>> I thought it would be?
> >>>>>>
> >>>>>> So the frames are supposed to be never forwarded to the user? Or is
> the
> >>>>>> user supposed to stop using these frames after the filter's lifetime
> >>>> ended?
> >>>>>
> >>>>>
> >>>>> The subtitle_header is global and is constant across all frames.
> >>>>> A resulting media file has only a (= the) single "subtitle header".
> >>>>> (it's not like a "header for each subtitle line or event").
> >>>>>
> >>>>
> >>>> Who ensures that it is constant across all frames? What happens if one
> >>>> mixes frames from two different sources? And who owns the subtitle
> header?
> >>>
> >>> In cases when the subtitle stream originates from a decoder and frames
> >>> are submitted into the filtergraph, ffmpeg.c makes a copy of it (field
> >>> of AVCodecContext) and stores it in InputStream->subtitle_heartbeat-
> >>> subtitle header
> >>> and stores the pointer to this in each subtitle frame.
> >>> It gets freed during ffmpeg uninit.
> >>>
> >>> In cases when the subtitle stream originates from a filter like split_cc
> >>> or graphicsubs2text, the filter creates the header and owns it, and
> stores
> >>> the reference in each subtitle frame it produces.
> >>> It gets freed during filter uninit.
> >>>
> >>> Relevant is the subtitle_header of the first frame that arrives at a
> >> target.
> >>> A target can be an encoder at the end of the filtergraph or other filters
> >>> like overlay_textsubs.
> >>>
> >>> Any filters in-between may use the subtitle_header for reference when
> >>> performing certain manipulations, but are not allowed (meant to) modify
> it.
> >>>
> >>
> >> If a frame or packet is refcounted, the lifetime of the data contained
> >> (or rather referenced) by it is indefinite; it survives closing a
> >> demuxer/encoder (for packets) or a decoder (for frames).
> >
> > That's why it's strduped to InputStream->subtitle_hearbeat which is
> > freed only on ffmpeg shutdown.
> >
> >> This is not so with this field; it is supposed to be not modified by the
> >> owner of an AVFrame, yet it is not even a const char*. Worse, it's
> >> lifetime is tied in a completely unclear way to something else. This is
> >> just not going to work.
> >
> > I was unsure how this would be considered. I'm sometimes having a hard
> > time to guess what's considered good or bad. At some places, code is
> > strongly relying on assumptions and conventions and could easily fail
> > if any of those is not fulfilled, and at other places/situations
> > there are rather strict constraints imposed even when it's unlikely
> > that these won't be missed.
> 
> Ownership needs to be absolutely clear or everything will crash (or leak).
> 
> >
> > Anyway - focusing on that issue - I'd change the subtitle header
> > string to be carried in a refcounted buffer instead,
> > would that be ok?
> >
> 
> It would solve the ownership issues; unfortunately at the cost of an
> allocation more for av_frame_ref, av_frame_copy_props etc. But that is
> not your fault. But it does not solve the problem of ensuring that the
> header that is supposed not to change does in fact not change.

Every language has its upsides and downsides, and in the case of C, 
options are limited for enforcing things like that. 
But everybody knows that. There are thousands of similar things 
which a C API can't prevent a user from doing and in order to know 
what is allowed to be done and what not, it is required to read docs, 
source code or example code.

So I wonder why there would be a reason to be concerned about 
specifically this subtitle_header string. The worst case is that
an API consumer doesn't read docs or source annotations, starts to
modify that field and then: he will notice that it doesn't have any 
effect and stop modifying it.

On the other hand it's clear that it's neither nice to copy over this
from frame-to-frame nor does it make sense from a logical perspective
for this to be carried as a field of a frame.

> The other route would be to treat it like an AVStream's extradata, i.e.
> like something that is by design not part of a single AVPacket/AVFrame,
> but belongs to something else that is associated with these
> packets/frames. How to know where to find this other thing is up to each
> piece of code.

That makes more sense of course, to have a single global "object"
containing per-stream data and have each frame carry a reference to 
that instead.

If such thing would be available, I would use it of course...

softworkz