[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.
 
Anyway - focusing on that issue - I'd change the subtitle header 
string to be carried in a refcounted buffer instead,
would that be ok?

softworkz