[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
>> Andreas Rheinhardt
>> Sent: Sunday, 12 September 2021 00:03
>> To: ffmpeg-devel at ffmpeg.org
>> Subject: Re: [FFmpeg-devel] [PATCH v3 01/18] avutil/frame: Subtitle
>> Filtering - Add AVMediaType property to AVFrame
>>
>> 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).
> 
> Are you sure? The default after av_frame_alloc is 0 (AVMEDIA_TYPE_VIDEO),
> so it's not possible to check whether the value 0 was set intentionally
> or not..?
> 

Of course av_frame_alloc (or rather get_frame_defaults) should
initialize the media type to AVMEDIA_TYPE_UNKNOWN. And adding
av_frame_get_buffer2 is done precisely because of this: Any user of it
is by definition aware of the existence of the new AVFrame mediatype
field and therefore has to make sure that it is really set in accordance
with his intentions. On the other hand, if one reused
av_frame_get_buffer and used the media type if it is set and the current
rules if it is AVMEDIA_TYPE_UNKNOWN, then there is a risk that a user
not knowing the new field gets surprised.

- Andreas