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

[Andreas Rheinhardt]

Paul B Mahol:
> On Sun, Sep 12, 2021 at 12:02 AM Andreas Rheinhardt <
> andreas.rheinhardt at outlook.com> wrote:
> 
>> 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).
>>
> 
> why? AVFrame is supposed to have media type in itself.
> 

Yes, and therefore the media type should be read from the AVFrame, not
from an additional parameter to av_frame_get_buffer2. As I said.

- Andreas