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

softworkz