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

Submitted by James Almer on Dec. 11, 2016, 3:33 a.m.

Details

Message ID 20161211033308.8592-1-jamrial@gmail.com
State New
Headers show

Commit Message

James Almer Dec. 11, 2016, 3:33 a.m.
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@gmail.com>
---
 libavutil/mastering_display_metadata.c | 12 ++++++++++++
 libavutil/mastering_display_metadata.h | 17 +++++++++++++++++
 2 files changed, 29 insertions(+)

Comments

wm4 Dec. 11, 2016, 1 p.m.
On Sun, 11 Dec 2016 00:33:03 -0300
James Almer <jamrial@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@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.
- 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)
- 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
- provide functions that return the struct size, and let the user
  use av_mallocz() (instead of both allocating and returning the size)
James Almer Dec. 11, 2016, 4:38 p.m.
On 12/11/2016 10:00 AM, wm4 wrote:
> On Sun, 11 Dec 2016 00:33:03 -0300
> James Almer <jamrial@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@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.
Michael Niedermayer Dec. 11, 2016, 9:20 p.m.
On Sun, Dec 11, 2016 at 02:00:04PM +0100, wm4 wrote:
> On Sun, 11 Dec 2016 00:33:03 -0300
[...]
> Here are some brainstormed alternative ideas to adding those ...2()
> functions:
> - add functions to add side data by type to AVFrames etc.
> - 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)

> - unify the side data enums into one (i.e. merge packet and frame side
>   data),

I was wondering previously already why there are multiple enums
I think a single enum list would be simpler independant of other
things


>   and provide a central function to allocate or retrieve size by
>   side data ID
> - provide functions that return the struct size, and let the user
>   use av_mallocz() (instead of both allocating and returning the size)
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
wm4 Dec. 11, 2016, 9:21 p.m.
On Sun, 11 Dec 2016 13:38:00 -0300
James Almer <jamrial@gmail.com> wrote:

> On 12/11/2016 10:00 AM, wm4 wrote:
> > On Sun, 11 Dec 2016 00:33:03 -0300
> > James Almer <jamrial@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@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.
wm4 Dec. 12, 2016, 9:06 a.m.
On Sun, 11 Dec 2016 22:20:39 +0100
Michael Niedermayer <michael@niedermayer.cc> wrote:

> On Sun, Dec 11, 2016 at 02:00:04PM +0100, wm4 wrote:
> > On Sun, 11 Dec 2016 00:33:03 -0300  
> [...]
> > Here are some brainstormed alternative ideas to adding those ...2()
> > functions:
> > - add functions to add side data by type to AVFrames etc.
> > - 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)  
> 
> > - unify the side data enums into one (i.e. merge packet and frame side
> >   data),  
> 
> I was wondering previously already why there are multiple enums
> I think a single enum list would be simpler independant of other
> things

Unfortunately Libav doesn't seem to have much interest, since it would
break the API (or at least ABI). Also they don't have many side data
types that require allocators.
Michael Niedermayer Dec. 12, 2016, 4:31 p.m.
On Mon, Dec 12, 2016 at 10:06:31AM +0100, wm4 wrote:
> On Sun, 11 Dec 2016 22:20:39 +0100
> Michael Niedermayer <michael@niedermayer.cc> wrote:
> 
> > On Sun, Dec 11, 2016 at 02:00:04PM +0100, wm4 wrote:
> > > On Sun, 11 Dec 2016 00:33:03 -0300  
> > [...]
> > > Here are some brainstormed alternative ideas to adding those ...2()
> > > functions:
> > > - add functions to add side data by type to AVFrames etc.
> > > - 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)  
> > 
> > > - unify the side data enums into one (i.e. merge packet and frame side
> > >   data),  
> > 
> > I was wondering previously already why there are multiple enums
> > I think a single enum list would be simpler independant of other
> > things
> 
> Unfortunately Libav doesn't seem to have much interest, since it would
> break the API (or at least ABI). Also they don't have many side data
> types that require allocators.

While it makes sense for us to listen and consider what Libav does,
and we may very well choose to do the same. We could choose to
do something different, if people prefer that ...

[...]
wm4 Dec. 12, 2016, 5 p.m.
On Mon, 12 Dec 2016 17:31:51 +0100
Michael Niedermayer <michael@niedermayer.cc> wrote:

> On Mon, Dec 12, 2016 at 10:06:31AM +0100, wm4 wrote:
> > On Sun, 11 Dec 2016 22:20:39 +0100
> > Michael Niedermayer <michael@niedermayer.cc> wrote:
> >   
> > > On Sun, Dec 11, 2016 at 02:00:04PM +0100, wm4 wrote:  
> > > > On Sun, 11 Dec 2016 00:33:03 -0300    
> > > [...]  
> > > > Here are some brainstormed alternative ideas to adding those ...2()
> > > > functions:
> > > > - add functions to add side data by type to AVFrames etc.
> > > > - 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)    
> > >   
> > > > - unify the side data enums into one (i.e. merge packet and frame side
> > > >   data),    
> > > 
> > > I was wondering previously already why there are multiple enums
> > > I think a single enum list would be simpler independant of other
> > > things  
> > 
> > Unfortunately Libav doesn't seem to have much interest, since it would
> > break the API (or at least ABI). Also they don't have many side data
> > types that require allocators.  
> 
> While it makes sense for us to listen and consider what Libav does,
> and we may very well choose to do the same. We could choose to
> do something different, if people prefer that ...

Well, they have a point: it would require changing ABI at least (and
maybe semi-weird hacks to keep the API compatible for a while).

We could still do av_allocate_frame_side_data(int id, ...) etc.

Patch hide | download patch | download mbox

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.