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

[Andreas Rheinhardt]

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

- Andreas