On Sun, 11 Dec 2016 13:38:00 -0300
James Almer <jamr...@gmail.com> wrote:
> On 12/11/2016 10:00 AM, wm4 wrote:
> > On Sun, 11 Dec 2016 00:33:03 -0300
> > James Almer <jamr...@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 <jamr...@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.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
