[FFmpeg-devel] [PATCH 1/7 v4] avutil/frame: add a flag to allow overwritting existing entries

[James Almer]

On 3/28/2024 8:23 AM, Anton Khirnov wrote:
> Quoting James Almer (2024-03-28 04:12:04)
>> Enable it only for side data types that don't allow more than one entry.
>>
>> Signed-off-by: James Almer <jamrial at gmail.com>
>> ---
>>   libavutil/frame.c                 | 59 ++++++++++++++++++++++++++++---
>>   libavutil/frame.h                 | 27 +++++++++-----
>>   libavutil/tests/side_data_array.c | 52 +++++++++++++++------------
>>   tests/ref/fate/side_data_array    | 22 ++++++------
>>   4 files changed, 115 insertions(+), 45 deletions(-)
>>
>> diff --git a/libavutil/frame.c b/libavutil/frame.c
>> index ef1613c344..d9bd19b2aa 100644
>> --- a/libavutil/frame.c
>> +++ b/libavutil/frame.c
>> @@ -799,12 +799,34 @@ AVFrameSideData *av_frame_side_data_new(AVFrameSideData ***sd, int *nb_sd,
>>                                           enum AVFrameSideDataType type,
>>                                           size_t size, unsigned int flags)
>>   {
>> -    AVBufferRef     *buf = av_buffer_alloc(size);
>> +    const AVSideDataDescriptor *desc = av_frame_side_data_desc(type);
>> +    AVBufferRef     *buf;
>>       AVFrameSideData *ret = NULL;
>>   
>>       if (flags & AV_FRAME_SIDE_DATA_FLAG_UNIQUE)
>>           remove_side_data(sd, nb_sd, type);
>> +    if (!desc || !(desc->props & AV_SIDE_DATA_PROP_MULTI)) {
> 
> desc == NULL means type is invalid

This being a generic alloc function, it should IMO not be limited to 
currently defined types. And I chose to treat them as non multi, as 
that's the most common scenario.

> 
>> +        for (int i = 0; i < *nb_sd; i++) {
>> +            AVFrameSideData *entry = ((*sd)[i]);
>> +            if (entry->type != type)
>> +                continue;
> 
> Why are you not calling av_frame_side_data_get()?

An oversight i guess :p

> 
>> @@ -822,13 +845,41 @@ int av_frame_side_data_clone(AVFrameSideData ***sd, int *nb_sd,
>>       if (!sd || !src || !nb_sd || (*nb_sd && !*sd))
>>           return AVERROR(EINVAL);
>>   
>> +    desc = av_frame_side_data_desc(src->type);
>> +    if (flags & AV_FRAME_SIDE_DATA_FLAG_UNIQUE)
>> +        remove_side_data(sd, nb_sd, src->type);
>> +    if (!desc || !(desc->props & AV_SIDE_DATA_PROP_MULTI)) {
>> +        for (int i = 0; i < *nb_sd; i++) {
>> +            AVFrameSideData *entry = ((*sd)[i]);
>> +            AVDictionary *dict = NULL;
>> +
>> +            if (entry->type != src->type)
>> +                continue;
>> +            if (!(flags & AV_FRAME_SIDE_DATA_FLAG_REPLACE))
>> +                return AVERROR(EEXIST);
>> +
>> +            ret = av_dict_copy(&dict, src->metadata, 0);
>> +            if (ret < 0)
>> +                return ret;
>> +
>> +            ret = av_buffer_replace(&entry->buf, src->buf);
>> +            if (ret < 0) {
>> +                av_dict_free(&dict);
>> +                return ret;
>> +            }
>> +
>> +            av_dict_free(&entry->metadata);
>> +            entry->metadata = dict;
>> +            entry->data     = src->data;
>> +            entry->size     = src->size;
>> +            return 0;
>> +        }
> 
> This whole block looks very similar to the one you're adding to
> av_frame_side_data_new().

I can try to factorize it.

> 
>> +    }
>> +
>>       buf = av_buffer_ref(src->buf);
>>       if (!buf)
>>           return AVERROR(ENOMEM);
>>   
>> -    if (flags & AV_FRAME_SIDE_DATA_FLAG_UNIQUE)
>> -        remove_side_data(sd, nb_sd, src->type);
>> -
>>       sd_dst = add_side_data_from_buf(sd, nb_sd, src->type, buf);
>>       if (!sd_dst) {
>>           av_buffer_unref(&buf);
>> diff --git a/libavutil/frame.h b/libavutil/frame.h
>> index 3b6d746a16..2ea129888e 100644
>> --- a/libavutil/frame.h
>> +++ b/libavutil/frame.h
>> @@ -1040,7 +1040,14 @@ const AVSideDataDescriptor *av_frame_side_data_desc(enum AVFrameSideDataType typ
>>    */
>>   void av_frame_side_data_free(AVFrameSideData ***sd, int *nb_sd);
>>   
>> +/**
>> + * Remove existing entries before adding new ones.
>> + */
>>   #define AV_FRAME_SIDE_DATA_FLAG_UNIQUE (1 << 0)
>> +/**
>> + * Don't add a new entry if another of the same type exists.
> 
> This should mention that it only applies to MULTI side data types.

Ok.