[FFmpeg-devel] [PATCH 2/7 v4] avutil/frame: add helper for adding side data w/ AVBufferRef to array

[James Almer]

On 3/28/2024 8:27 AM, Anton Khirnov wrote:
> Quoting James Almer (2024-03-28 04:12:05)
>> Signed-off-by: James Almer <jamrial at gmail.com>
>> ---
>>   libavutil/frame.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++
>>   libavutil/frame.h | 34 ++++++++++++++++++++++++++++++
>>   2 files changed, 87 insertions(+)
>>
>> diff --git a/libavutil/frame.c b/libavutil/frame.c
>> index d9bd19b2aa..a165e56a64 100644
>> --- a/libavutil/frame.c
>> +++ b/libavutil/frame.c
>> @@ -834,6 +834,59 @@ AVFrameSideData *av_frame_side_data_new(AVFrameSideData ***sd, int *nb_sd,
>>       return ret;
>>   }
>>   
>> +AVFrameSideData *av_frame_side_data_add(AVFrameSideData ***sd, int *nb_sd,
>> +                                        enum AVFrameSideDataType type,
>> +                                        AVBufferRef **pbuf, unsigned int flags)
>> +{
>> +    const AVSideDataDescriptor *desc = av_frame_side_data_desc(type);
>> +    AVFrameSideData *sd_dst  = NULL;
>> +    AVBufferRef *buf;
>> +
>> +    if (!sd || !pbuf || !*pbuf || !nb_sd || (*nb_sd && !*sd))
> 
> Overzealous checks like these tend to hide bugs. Any of these conditions
> being false means the caller is insane and we should crash.

I'll remove some, but others simplify the code below (like knowing 
beforehand that *pbuf is not NULL).

> 
>> +        return 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)) {
>> +        for (int i = 0; i < *nb_sd; i++) {
>> +            AVFrameSideData *entry = ((*sd)[i]);
>> +
>> +            if (entry->type != type)
>> +                continue;
>> +            if (!(flags & AV_FRAME_SIDE_DATA_FLAG_REPLACE))
>> +                return NULL;
>> +
>> +            buf = *pbuf;
>> +            if (flags & AV_FRAME_SIDE_DATA_FLAG_NEW_REF) {
>> +                int ret = av_buffer_replace(&entry->buf, buf);
>> +                if (ret < 0)
>> +                    return NULL;
>> +            } else
>> +                *pbuf = NULL;
>> +
>> +            av_dict_free(&entry->metadata);
>> +            entry->data = buf->data;
>> +            entry->size = buf->size;
>> +            return entry;
> 
> This again looks like a minor variation of the  block you've added twice
> already.
> 
>> +        }
>> +    }
>> +
>> +    buf = (flags & AV_FRAME_SIDE_DATA_FLAG_NEW_REF) ?
>> +           av_buffer_ref(*pbuf) : *pbuf;
>> +
>> +    sd_dst = add_side_data_from_buf(sd, nb_sd, type, buf);
>> +    if (!sd_dst) {
>> +        if (flags & AV_FRAME_SIDE_DATA_FLAG_NEW_REF)
>> +            av_buffer_unref(&buf);
>> +        return NULL;
>> +    }
>> +
>> +    if (!(flags & AV_FRAME_SIDE_DATA_FLAG_NEW_REF))
>> +        *pbuf = NULL;
>> +
>> +    return sd_dst;
>> +}
>> +
>>   int av_frame_side_data_clone(AVFrameSideData ***sd, int *nb_sd,
>>                                const AVFrameSideData *src, unsigned int flags)
>>   {
>> diff --git a/libavutil/frame.h b/libavutil/frame.h
>> index 2ea129888e..3e5d170a5b 100644
>> --- a/libavutil/frame.h
>> +++ b/libavutil/frame.h
>> @@ -1048,6 +1048,10 @@ void av_frame_side_data_free(AVFrameSideData ***sd, int *nb_sd);
>>    * Don't add a new entry if another of the same type exists.
>>    */
>>   #define AV_FRAME_SIDE_DATA_FLAG_REPLACE (1 << 1)
>> +/**
>> + * Create a new reference instead of taking ownership of the passed in one.
>> + */
>> +#define AV_FRAME_SIDE_DATA_FLAG_NEW_REF (1 << 2)
> 
> Who needs this?

Someone who wants to keep the reference around, like when attaching a 
buffer to several outputs (global to individual output frames).