[FFmpeg-devel] [PATCH 1/6] avutil/mastering_display_metadata: add av_mastering_display_metadata_alloc2()

[wm4]

On Sun, 11 Dec 2016 13:38:00 -0300
James Almer <jamrial at gmail.com> wrote:

> On 12/11/2016 10:00 AM, wm4 wrote:
> > On Sun, 11 Dec 2016 00:33:03 -0300
> > James Almer <jamrial at gmail.com> wrote:
> >   
> >> Also deprecate av_mastering_display_metadata_alloc().
> >>
> >> This new alloc function optionally returns the size of the AVMasteringDisplayMetadata
> >> struct.
> >>
> >> Signed-off-by: James Almer <jamrial at gmail.com>
> >> ---
> >>  libavutil/mastering_display_metadata.c | 12 ++++++++++++
> >>  libavutil/mastering_display_metadata.h | 17 +++++++++++++++++
> >>  2 files changed, 29 insertions(+)
> >>
> >> diff --git a/libavutil/mastering_display_metadata.c b/libavutil/mastering_display_metadata.c
> >> index e1683e5..872f14b 100644
> >> --- a/libavutil/mastering_display_metadata.c
> >> +++ b/libavutil/mastering_display_metadata.c
> >> @@ -29,6 +29,18 @@ AVMasteringDisplayMetadata *av_mastering_display_metadata_alloc(void)
> >>      return av_mallocz(sizeof(AVMasteringDisplayMetadata));
> >>  }
> >>  
> >> +AVMasteringDisplayMetadata *av_mastering_display_metadata_alloc2(size_t *size)
> >> +{
> >> +    AVMasteringDisplayMetadata *mastering = av_mallocz(sizeof(AVMasteringDisplayMetadata));
> >> +    if (!mastering)
> >> +        return NULL;
> >> +
> >> +    if (size)
> >> +        *size = sizeof(*mastering);
> >> +
> >> +    return mastering;
> >> +}
> >> +
> >>  AVMasteringDisplayMetadata *av_mastering_display_metadata_create_side_data(AVFrame *frame)
> >>  {
> >>      AVFrameSideData *side_data = av_frame_new_side_data(frame,
> >> diff --git a/libavutil/mastering_display_metadata.h b/libavutil/mastering_display_metadata.h
> >> index 936533f..32357b3 100644
> >> --- a/libavutil/mastering_display_metadata.h
> >> +++ b/libavutil/mastering_display_metadata.h
> >> @@ -21,6 +21,10 @@
> >>  #ifndef AVUTIL_MASTERING_DISPLAY_METADATA_H
> >>  #define AVUTIL_MASTERING_DISPLAY_METADATA_H
> >>  
> >> +#include <stddef.h>
> >> +#include <stdint.h>
> >> +
> >> +#include "attributes.h"
> >>  #include "frame.h"
> >>  #include "rational.h"
> >>  
> >> @@ -74,10 +78,23 @@ typedef struct AVMasteringDisplayMetadata {
> >>   *
> >>   * @return An AVMasteringDisplayMetadata filled with default values or NULL
> >>   *         on failure.
> >> + *
> >> + * @deprecated Use av_mastering_display_metadata_alloc2().
> >>   */
> >> +attribute_deprecated
> >>  AVMasteringDisplayMetadata *av_mastering_display_metadata_alloc(void);
> >>  
> >>  /**
> >> + * Allocate an AVMasteringDisplayMetadata structure and set its fields to
> >> + * default values. The resulting struct can be freed using av_freep().
> >> + *
> >> + * @param size pointer for AVMasteringDisplayMetadata structure size to store (optional)
> >> + * @return An AVMasteringDisplayMetadata filled with default values or NULL
> >> + *         on failure.
> >> + */
> >> +AVMasteringDisplayMetadata *av_mastering_display_metadata_alloc2(size_t *size);
> >> +
> >> +/**
> >>   * Allocate a complete AVMasteringDisplayMetadata and add it to the frame.
> >>   *
> >>   * @param frame The frame which side data is added to.  
> > 
> > I guess it's ok to deprecate the old function, since it couldn't be
> > used correctly.
> > 
> > Here are some brainstormed alternative ideas to adding those ...2()
> > functions:
> > - add functions to add side data by type to AVFrames etc.  
> 
> These already have a function for that.
> 
> av_stereo3d_create_side_data(frame)
> av_mastering_display_metadata_create_side_data(frame)
> 
> Display Matrix and Spherical Video Mapping don't, however, but that
> may be because there's no use for them just yet.
> 
> > - provide a generic function that allocates side data by passing the
> >   side data ID to it (would need separate ones for packet and stream
> >   side data)  
> 
> av_{stream,packet,frame}_{add,new}_side_data() exist, but they expect
> the size value as an argument. Adding new ones that take the size
> internally from within lavu would be more or less the same as adding
> the new alloc functions from this patchset.
> 
> > - unify the side data enums into one (i.e. merge packet and frame side
> >   data), and provide a central function to allocate or retrieve size by
> >   side data ID  
> 
> This sounds like a big API break, so I'm not going to try tackling it.
> Someone else is welcome to try.
> 
> > - provide functions that return the struct size, and let the user
> >   use av_mallocz() (instead of both allocating and returning the size)  
> 
> This was my first idea, but then i noticed the doxy for the alloc()
> functions mention they fill the fields with default values, and are
> thus listed as the only correct way to alloc the structs.
> They all just zero the whole thing, which right now is indeed the
> default value for their fields, but that may change in the future
> and using av_malloc* would not be a valid alternative anymore.
> 
> It is in any case a good idea to add this alongside the new alloc()
> functions (or whatever alternative we use) since we're using the
> sizeof these structs in libavformat/dump.c and the relevant muxers
> to validate side data sent by demuxers.

Yeah, seems like going with your patches is the simplest solution for
now. Though this way of returning two values is a bit inelegant (which
is due to C not having good support for returning multiple values). So
it's a bit sad that a whole bunch of such "ugly" functions is needed.