[FFmpeg-devel] [PATCH 4/6 v2] avutil/mastering_display_metadata: add a new allocator function that returns a size

[James Almer]

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 at 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.