| Message ID | 20240325200602.63020-4-jamrial@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/6,v2] avutil/frame: add a flag to not create duplicate entries in a side data array | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/commit_msg_loongarch64 | warning | The first line of the commit message must start with a context terminated by a colon and a space, for example "lavu/opt: " or "doc: ". |
| andriy/commit_msg_x86 | warning | The first line of the commit message must start with a context terminated by a colon and a space, for example "lavu/opt: " or "doc: ". |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
James Almer: > av_mastering_display_metadata_alloc() is not useful in scenarios where you need to > know the runtime size of AVMasteringDisplayMetadata. > > Signed-off-by: James Almer <jamrial@gmail.com> > --- > libavutil/mastering_display_metadata.c | 13 +++++++++++++ > libavutil/mastering_display_metadata.h | 9 +++++++++ > 2 files changed, 22 insertions(+) > > diff --git a/libavutil/mastering_display_metadata.c b/libavutil/mastering_display_metadata.c > index 6069347617..ea41f13f9d 100644 > --- a/libavutil/mastering_display_metadata.c > +++ b/libavutil/mastering_display_metadata.c > @@ -18,6 +18,7 @@ > * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA > */ > > +#include <stddef.h> > #include <stdint.h> > #include <string.h> > > @@ -29,6 +30,18 @@ AVMasteringDisplayMetadata *av_mastering_display_metadata_alloc(void) > return av_mallocz(sizeof(AVMasteringDisplayMetadata)); > } > > +AVMasteringDisplayMetadata *av_mastering_display_metadata_alloc_size(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 c23b07c3cd..52fcef9e37 100644 > --- a/libavutil/mastering_display_metadata.h > +++ b/libavutil/mastering_display_metadata.h > @@ -77,6 +77,15 @@ typedef struct AVMasteringDisplayMetadata { > */ > 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(). > + * > + * @return An AVMasteringDisplayMetadata filled with default values or NULL > + * on failure. > + */ > +AVMasteringDisplayMetadata *av_mastering_display_metadata_alloc_size(size_t *size); > + > /** > * Allocate a complete AVMasteringDisplayMetadata and add it to the frame. > * Instead of this we should have a generic allocator like void *av_frame_side_data_allocate(enum AVFrameSideDataType, size_t *size, size_t elem_count), with the latter being used for the allocators that allocate arrays (like AVRegionOfInterest); it has to be set to zero for the others. This will also avoid creating new av_*_create_side_data() functions. - Andreas
On 3/25/2024 5:40 PM, Andreas Rheinhardt wrote: > James Almer: >> av_mastering_display_metadata_alloc() is not useful in scenarios where you need to >> know the runtime size of AVMasteringDisplayMetadata. >> >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> libavutil/mastering_display_metadata.c | 13 +++++++++++++ >> libavutil/mastering_display_metadata.h | 9 +++++++++ >> 2 files changed, 22 insertions(+) >> >> diff --git a/libavutil/mastering_display_metadata.c b/libavutil/mastering_display_metadata.c >> index 6069347617..ea41f13f9d 100644 >> --- a/libavutil/mastering_display_metadata.c >> +++ b/libavutil/mastering_display_metadata.c >> @@ -18,6 +18,7 @@ >> * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA >> */ >> >> +#include <stddef.h> >> #include <stdint.h> >> #include <string.h> >> >> @@ -29,6 +30,18 @@ AVMasteringDisplayMetadata *av_mastering_display_metadata_alloc(void) >> return av_mallocz(sizeof(AVMasteringDisplayMetadata)); >> } >> >> +AVMasteringDisplayMetadata *av_mastering_display_metadata_alloc_size(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 c23b07c3cd..52fcef9e37 100644 >> --- a/libavutil/mastering_display_metadata.h >> +++ b/libavutil/mastering_display_metadata.h >> @@ -77,6 +77,15 @@ typedef struct AVMasteringDisplayMetadata { >> */ >> 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(). >> + * >> + * @return An AVMasteringDisplayMetadata filled with default values or NULL >> + * on failure. >> + */ >> +AVMasteringDisplayMetadata *av_mastering_display_metadata_alloc_size(size_t *size); >> + >> /** >> * Allocate a complete AVMasteringDisplayMetadata and add it to the frame. >> * > > Instead of this we should have a generic allocator like > void *av_frame_side_data_allocate(enum AVFrameSideDataType, size_t > *size, size_t elem_count), with the latter being used for the allocators > that allocate arrays (like AVRegionOfInterest); it has to be set to zero > for the others. This will also avoid creating new > av_*_create_side_data() functions. I don't mind a function like that being added to simplify future additions, but this API is orthogonal to the frame side data one. It's also used in packets, for example, and right now lavf is using sizeof(AVMasteringDisplayMetadata) because av_mastering_display_metadata_alloc() is not useful. > > - Andreas > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
James Almer: > On 3/25/2024 5:40 PM, Andreas Rheinhardt wrote: >> James Almer: >>> av_mastering_display_metadata_alloc() is not useful in scenarios >>> where you need to >>> know the runtime size of AVMasteringDisplayMetadata. >>> >>> Signed-off-by: James Almer <jamrial@gmail.com> >>> --- >>> libavutil/mastering_display_metadata.c | 13 +++++++++++++ >>> libavutil/mastering_display_metadata.h | 9 +++++++++ >>> 2 files changed, 22 insertions(+) >>> >>> diff --git a/libavutil/mastering_display_metadata.c >>> b/libavutil/mastering_display_metadata.c >>> index 6069347617..ea41f13f9d 100644 >>> --- a/libavutil/mastering_display_metadata.c >>> +++ b/libavutil/mastering_display_metadata.c >>> @@ -18,6 +18,7 @@ >>> * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA >>> 02110-1301 USA >>> */ >>> +#include <stddef.h> >>> #include <stdint.h> >>> #include <string.h> >>> @@ -29,6 +30,18 @@ AVMasteringDisplayMetadata >>> *av_mastering_display_metadata_alloc(void) >>> return av_mallocz(sizeof(AVMasteringDisplayMetadata)); >>> } >>> +AVMasteringDisplayMetadata >>> *av_mastering_display_metadata_alloc_size(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 c23b07c3cd..52fcef9e37 100644 >>> --- a/libavutil/mastering_display_metadata.h >>> +++ b/libavutil/mastering_display_metadata.h >>> @@ -77,6 +77,15 @@ typedef struct AVMasteringDisplayMetadata { >>> */ >>> 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(). >>> + * >>> + * @return An AVMasteringDisplayMetadata filled with default values >>> or NULL >>> + * on failure. >>> + */ >>> +AVMasteringDisplayMetadata >>> *av_mastering_display_metadata_alloc_size(size_t *size); >>> + >>> /** >>> * Allocate a complete AVMasteringDisplayMetadata and add it to the >>> frame. >>> * >> >> Instead of this we should have a generic allocator like >> void *av_frame_side_data_allocate(enum AVFrameSideDataType, size_t >> *size, size_t elem_count), with the latter being used for the allocators >> that allocate arrays (like AVRegionOfInterest); it has to be set to zero >> for the others. This will also avoid creating new >> av_*_create_side_data() functions. > > I don't mind a function like that being added to simplify future > additions, but this API is orthogonal to the frame side data one. It's > also used in packets, for example, and right now lavf is using > sizeof(AVMasteringDisplayMetadata) because > av_mastering_display_metadata_alloc() is not useful. > The API proposed by me is supposed to make API like av_mastering_display_metadata_alloc_size() redundant and therefore these two additions are not orthogonal. - Andreas
On 3/25/2024 6:02 PM, Andreas Rheinhardt wrote: > James Almer: >> On 3/25/2024 5:40 PM, Andreas Rheinhardt wrote: >>> James Almer: >>>> av_mastering_display_metadata_alloc() is not useful in scenarios >>>> where you need to >>>> know the runtime size of AVMasteringDisplayMetadata. >>>> >>>> Signed-off-by: James Almer <jamrial@gmail.com> >>>> --- >>>> libavutil/mastering_display_metadata.c | 13 +++++++++++++ >>>> libavutil/mastering_display_metadata.h | 9 +++++++++ >>>> 2 files changed, 22 insertions(+) >>>> >>>> diff --git a/libavutil/mastering_display_metadata.c >>>> b/libavutil/mastering_display_metadata.c >>>> index 6069347617..ea41f13f9d 100644 >>>> --- a/libavutil/mastering_display_metadata.c >>>> +++ b/libavutil/mastering_display_metadata.c >>>> @@ -18,6 +18,7 @@ >>>> * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA >>>> 02110-1301 USA >>>> */ >>>> +#include <stddef.h> >>>> #include <stdint.h> >>>> #include <string.h> >>>> @@ -29,6 +30,18 @@ AVMasteringDisplayMetadata >>>> *av_mastering_display_metadata_alloc(void) >>>> return av_mallocz(sizeof(AVMasteringDisplayMetadata)); >>>> } >>>> +AVMasteringDisplayMetadata >>>> *av_mastering_display_metadata_alloc_size(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 c23b07c3cd..52fcef9e37 100644 >>>> --- a/libavutil/mastering_display_metadata.h >>>> +++ b/libavutil/mastering_display_metadata.h >>>> @@ -77,6 +77,15 @@ typedef struct AVMasteringDisplayMetadata { >>>> */ >>>> 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(). >>>> + * >>>> + * @return An AVMasteringDisplayMetadata filled with default values >>>> or NULL >>>> + * on failure. >>>> + */ >>>> +AVMasteringDisplayMetadata >>>> *av_mastering_display_metadata_alloc_size(size_t *size); >>>> + >>>> /** >>>> * Allocate a complete AVMasteringDisplayMetadata and add it to the >>>> frame. >>>> * >>> >>> Instead of this we should have a generic allocator like >>> void *av_frame_side_data_allocate(enum AVFrameSideDataType, size_t >>> *size, size_t elem_count), with the latter being used for the allocators >>> that allocate arrays (like AVRegionOfInterest); it has to be set to zero >>> for the others. This will also avoid creating new >>> av_*_create_side_data() functions. >> >> I don't mind a function like that being added to simplify future >> additions, but this API is orthogonal to the frame side data one. It's >> also used in packets, for example, and right now lavf is using >> sizeof(AVMasteringDisplayMetadata) because >> av_mastering_display_metadata_alloc() is not useful. >> > > The API proposed by me is supposed to make API like > av_mastering_display_metadata_alloc_size() redundant and therefore these > two additions are not orthogonal. Just because there's a frame side data type for MDM does not make the new alloc function redundant. The frame side data API is one API wrapping others. You don't see code using the AVHash API when they only care about AVMD5. MDM is and should be standalone. av_mastering_display_metadata_alloc() should be deprecated and removed after this, too. That will ensure we still have a single symbol for this. Regarding the av_*_create_side_data() functions, they were never needed. They are a convenient wrapper/shortcut, and your av_frame_side_data_allocate() would not really replace them as you'd still need to use av_frame_add_side_data() afterwards.
Quoting James Almer (2024-03-25 22:13:25) > On 3/25/2024 6:02 PM, Andreas Rheinhardt wrote: > > James Almer: > >> I don't mind a function like that being added to simplify future > >> additions, but this API is orthogonal to the frame side data one. It's > >> also used in packets, for example, and right now lavf is using > >> sizeof(AVMasteringDisplayMetadata) because > >> av_mastering_display_metadata_alloc() is not useful. > >> > > > > The API proposed by me is supposed to make API like > > av_mastering_display_metadata_alloc_size() redundant and therefore these > > two additions are not orthogonal. > > Just because there's a frame side data type for MDM does not make the > new alloc function redundant. The commit message says: > av_mastering_display_metadata_alloc() is not useful in scenarios where you need to > know the runtime size of AVMasteringDisplayMetadata. In what scenarios besides side data do you need to know the size of this struct?
On 3/27/2024 4:41 AM, Anton Khirnov wrote: > Quoting James Almer (2024-03-25 22:13:25) >> On 3/25/2024 6:02 PM, Andreas Rheinhardt wrote: >>> James Almer: >>>> I don't mind a function like that being added to simplify future >>>> additions, but this API is orthogonal to the frame side data one. It's >>>> also used in packets, for example, and right now lavf is using >>>> sizeof(AVMasteringDisplayMetadata) because >>>> av_mastering_display_metadata_alloc() is not useful. >>>> >>> >>> The API proposed by me is supposed to make API like >>> av_mastering_display_metadata_alloc_size() redundant and therefore these >>> two additions are not orthogonal. >> >> Just because there's a frame side data type for MDM does not make the >> new alloc function redundant. > > The commit message says: > >> av_mastering_display_metadata_alloc() is not useful in scenarios where you need to >> know the runtime size of AVMasteringDisplayMetadata. > > In what scenarios besides side data do you need to know the size of this > struct? None within our libraries that i can think of, but library users can have scenarios we need to provide for. MDM is a standalone API, so lets not try to make its usability depend on a separate one. I'm replacing a helper with a better one, it should not be so controversial.
Quoting James Almer (2024-03-27 13:35:35) > On 3/27/2024 4:41 AM, Anton Khirnov wrote: > > Quoting James Almer (2024-03-25 22:13:25) > >> On 3/25/2024 6:02 PM, Andreas Rheinhardt wrote: > >>> James Almer: > >>>> I don't mind a function like that being added to simplify future > >>>> additions, but this API is orthogonal to the frame side data one. It's > >>>> also used in packets, for example, and right now lavf is using > >>>> sizeof(AVMasteringDisplayMetadata) because > >>>> av_mastering_display_metadata_alloc() is not useful. > >>>> > >>> > >>> The API proposed by me is supposed to make API like > >>> av_mastering_display_metadata_alloc_size() redundant and therefore these > >>> two additions are not orthogonal. > >> > >> Just because there's a frame side data type for MDM does not make the > >> new alloc function redundant. > > > > The commit message says: > > > >> av_mastering_display_metadata_alloc() is not useful in scenarios where you need to > >> know the runtime size of AVMasteringDisplayMetadata. > > > > In what scenarios besides side data do you need to know the size of this > > struct? > > None within our libraries that i can think of, but library users can > have scenarios we need to provide for. MDM is a standalone API, so lets > not try to make its usability depend on a separate one. > I'm replacing a helper with a better one, it should not be so controversial. Breaking API to benefit hypothetical use cases IS a controversial change in my view.
On 3/27/2024 9:40 AM, Anton Khirnov wrote: > Quoting James Almer (2024-03-27 13:35:35) >> On 3/27/2024 4:41 AM, Anton Khirnov wrote: >>> Quoting James Almer (2024-03-25 22:13:25) >>>> On 3/25/2024 6:02 PM, Andreas Rheinhardt wrote: >>>>> James Almer: >>>>>> I don't mind a function like that being added to simplify future >>>>>> additions, but this API is orthogonal to the frame side data one. It's >>>>>> also used in packets, for example, and right now lavf is using >>>>>> sizeof(AVMasteringDisplayMetadata) because >>>>>> av_mastering_display_metadata_alloc() is not useful. >>>>>> >>>>> >>>>> The API proposed by me is supposed to make API like >>>>> av_mastering_display_metadata_alloc_size() redundant and therefore these >>>>> two additions are not orthogonal. >>>> >>>> Just because there's a frame side data type for MDM does not make the >>>> new alloc function redundant. >>> >>> The commit message says: >>> >>>> av_mastering_display_metadata_alloc() is not useful in scenarios where you need to >>>> know the runtime size of AVMasteringDisplayMetadata. >>> >>> In what scenarios besides side data do you need to know the size of this >>> struct? >> >> None within our libraries that i can think of, but library users can >> have scenarios we need to provide for. MDM is a standalone API, so lets >> not try to make its usability depend on a separate one. >> I'm replacing a helper with a better one, it should not be so controversial. > > Breaking API to benefit hypothetical use cases IS a controversial change > in my view. Then I can leave the old on in place. But don't insist on telling people to use a frame side data specific alloc function if they want a MDM struct, because said function will not be usable for all side data types anyway (see video enc params, iamf params), so it's just making things more complex for the user for no reason.
diff --git a/libavutil/mastering_display_metadata.c b/libavutil/mastering_display_metadata.c index 6069347617..ea41f13f9d 100644 --- a/libavutil/mastering_display_metadata.c +++ b/libavutil/mastering_display_metadata.c @@ -18,6 +18,7 @@ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA */ +#include <stddef.h> #include <stdint.h> #include <string.h> @@ -29,6 +30,18 @@ AVMasteringDisplayMetadata *av_mastering_display_metadata_alloc(void) return av_mallocz(sizeof(AVMasteringDisplayMetadata)); } +AVMasteringDisplayMetadata *av_mastering_display_metadata_alloc_size(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 c23b07c3cd..52fcef9e37 100644 --- a/libavutil/mastering_display_metadata.h +++ b/libavutil/mastering_display_metadata.h @@ -77,6 +77,15 @@ typedef struct AVMasteringDisplayMetadata { */ 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(). + * + * @return An AVMasteringDisplayMetadata filled with default values or NULL + * on failure. + */ +AVMasteringDisplayMetadata *av_mastering_display_metadata_alloc_size(size_t *size); + /** * Allocate a complete AVMasteringDisplayMetadata and add it to the frame. *
av_mastering_display_metadata_alloc() is not useful in scenarios where you need to know the runtime size of AVMasteringDisplayMetadata. Signed-off-by: James Almer <jamrial@gmail.com> --- libavutil/mastering_display_metadata.c | 13 +++++++++++++ libavutil/mastering_display_metadata.h | 9 +++++++++ 2 files changed, 22 insertions(+)