[FFmpeg-devel] [PATCH v3 01/18] avutil/frame: Subtitle Filtering - Add AVMediaType property to AVFrame

[Andreas Rheinhardt]

Soft Works:
> 
> 
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
>> Paul B Mahol
>> Sent: Saturday, 11 September 2021 23:45
>> To: FFmpeg development discussions and patches <ffmpeg-
>> devel at ffmpeg.org>
>> Subject: Re: [FFmpeg-devel] [PATCH v3 01/18] avutil/frame: Subtitle
>> Filtering - Add AVMediaType property to AVFrame
>>
>> On Sat, Sep 11, 2021 at 11:43 PM Soft Works <softworkz at hotmail.com>
>> wrote:
>>
>>>
>>>
>>>> -----Original Message-----
>>>> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
>>>> Hendrik Leppkes
>>>> Sent: Saturday, 11 September 2021 23:42
>>>> To: FFmpeg development discussions and patches <ffmpeg-
>>>> devel at ffmpeg.org>
>>>> Subject: Re: [FFmpeg-devel] [PATCH v3 01/18] avutil/frame:
>> Subtitle
>>>> Filtering - Add AVMediaType property to AVFrame
>>>>
>>>> On Sat, Sep 11, 2021 at 8:31 PM Soft Works
>> <softworkz at hotmail.com>
>>>> wrote:
>>>>>
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On
>> Behalf Of
>>>>>> Paul B Mahol
>>>>>> Sent: Saturday, 11 September 2021 12:51
>>>>>> To: FFmpeg development discussions and patches <ffmpeg-
>>>>>> devel at ffmpeg.org>
>>>>>> Subject: Re: [FFmpeg-devel] [PATCH v3 01/18] avutil/frame:
>>>> Subtitle
>>>>>> Filtering - Add AVMediaType property to AVFrame
>>>>>>
>>>>>>> @@ -721,6 +728,8 @@ void av_frame_move_ref(AVFrame *dst,
>>>> AVFrame
>>>>>> *src);
>>>>>>>  /**
>>>>>>>   * Allocate new buffer(s) for audio or video data.
>>>>>>>   *
>>>>>>> + * Note: For subtitle data, use av_frame_get_buffer2
>>>>>>>
>>>>>>
>>>>>> I dislike this approach. It should be consistent with audio &
>>>> video.
>>>>>
>>>>> I agree, it's not nice. I will just change av_frame_get_buffer
>>>> instead.
>>>>>
>>>>
>>>> We don't change existing functions like that. We add new API, or
>>>> deprecate old ones, we don't change it in such an incompatible
>>>> manner.
>>>> But before these functions are even needed, lets just see if its
>> even
>>>> needed to change anything, depending on how subtitles will
>> ultimately
>>>> end up packaged.
>>>
>>> When you say, "We add new API", then my initial approach with
>>> av_frame_get_buffer2 was correct?
>>>
>>> No. That is bad design.
>>
>> You do not need another argument for function.
>>
>> Use same function, same number of arguments and type.
>> Just change internal stuff.
> 
> The problem is that the current implementation is inferring the media 
> type by checking width and height (=> video, otherwise audio).
> 
> Subtitle frames can have w+h or not, so it can't continue to work
> this way.
> 
> The idea of av_frame_get_buffer2 is to have an explicit AVMediaType 
> parameter to specify.
> 
> An alternative would be to require the (new) type property to be 
> set explicitly in case of subtitles and continue the current way
> of inference when it's not set.
> That would work but it wouldn't be a clean API design either.
> 
> What would you suggest?
> 

Add an av_frame_get_buffer2 that requires the media type of the frame to
be set (on the frame, not as an additional parameter).

- Andreas