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

[James Almer]

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.