[FFmpeg-devel] [PATCH v16 01/16] global: Prepare AVFrame for subtitle handling

[Soft Works]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of Anton
> Khirnov
> Sent: Saturday, November 27, 2021 9:52 AM
> To: ffmpeg-devel at ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v16 01/16] global: Prepare AVFrame for
> subtitle handling
> 
> Quoting Soft Works (2021-11-25 18:53:24)
> > @@ -759,9 +802,39 @@ void av_frame_move_ref(AVFrame *dst, AVFrame *src);
> >   *              recommended to pass 0 here unless you know what you are
> doing.
> >   *
> >   * @return 0 on success, a negative AVERROR on error.
> > + *
> > + * @deprecated Use @ref av_frame_get_buffer2 instead and set @ref
> AVFrame.type
> > + * before calling.
> >   */
> > +attribute_deprecated
> >  int av_frame_get_buffer(AVFrame *frame, int align);
> >
> > +/**
> > + * Allocate new buffer(s) for audio, video or subtitle data.
> > + *
> > + * The following fields must be set on frame before calling this function:
> > + * - format (pixel format for video, sample format for audio)
> > + * - width and height for video
> > + * - nb_samples and channel_layout for audio
> > + * - type (AVMediaType)
> > + *
> > + * This function will fill AVFrame.data and AVFrame.buf arrays and, if
> > + * necessary, allocate and fill AVFrame.extended_data and
> AVFrame.extended_buf.
> > + * For planar formats, one buffer will be allocated for each plane.
> > + *
> > + * @warning: if frame already has been allocated, calling this function
> will
> > + *           leak memory. In addition, undefined behavior can occur in
> certain
> > + *           cases.
> > + *
> > + * @param frame frame in which to store the new buffers.
> > + * @param align Required buffer size alignment. If equal to 0, alignment
> will be
> > + *              chosen automatically for the current CPU. It is highly
> > + *              recommended to pass 0 here unless you know what you are
> doing.
> > + *
> > + * @return 0 on success, a negative AVERROR on error.
> > + */
> > +int av_frame_get_buffer2(AVFrame *frame, int align);
> 
> Not sure whether this was asked already - why do we need this new
> function? Seems to me you can accomplish the same thing by just adding
> the type field to AVFrame. Then
> - if type is AVMEDIA_TYPE_SUBTITLE -> allocate a subtitle
> - if type is AVMEDIA_TYPE_{VIDEO,AUDIO} -> allocate video/audio
> - otherwise detect video/audio as we do now`


I can't find it on mail archive, so here are some quotes from the e-mails about it:

(originally I had av_frame_get_buffer2 with a AVMediaType parameter)

softworkz:


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?


Andreas:

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).


Paul: 


why? AVFrame is supposed to have media type in itself.



Andreas:


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.


softworkz:

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


Andreas:

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.