| Message ID | 20161211033308.8592-1-jamrial@gmail.com |
|---|---|
| State | New |
| Headers | show |
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)
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.
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
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.
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.
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 ... [...]
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.
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.
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(+)