[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: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..?

softworkz