[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
> Andreas Rheinhardt
> Sent: Sunday, 12 September 2021 00:17
> To: ffmpeg-devel at ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v3 01/18] avutil/frame: Subtitle
> Filtering - Add AVMediaType property to AVFrame
> 
> 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.

The type property is new, so existing code doesn't set it and users
of the API don't know that it needs to be set.

Adding av_frame_get_buffer2 with the same set of parameters but only
a different convention regarding the way it's expected to be used
might be confusing, but if that's the way you prefer, it's fine 
for me and I'll do it like that :-)

> Of course av_frame_alloc (or rather get_frame_defaults) should
> initialize the media type to AVMEDIA_TYPE_UNKNOWN. 

OK, let's see whether it won't break things in cases when none of
the av_frame_get_buffer, _copy or _clone functions are being used.

softworkz