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

[James Almer]

On 3/28/2024 9:19 AM, Anton Khirnov wrote:
> Quoting James Almer (2024-03-28 12:49:08)
>> 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).
> 
> You can just assume them all to be true. Or use av_assert0().
> 
>>>> 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).
> 
> Is that a common enough use case to warrant this flag? It complicates
> the code quite substantially.
> 
> And if you're making some side data instance global, what is the point
> of also attaching it to frames?

To pass it to API that does not handle global entries, like currently 
happens with filtergraphs.

I can remove the flag for now in any case. It can be added later trivially.