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

[James Almer]

On 3/28/2024 9:21 AM, Anton Khirnov wrote:
> Quoting James Almer (2024-03-28 12:41:40)
>> 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.
> 
> Andreas argued in the thread adding descriptors that side data type
> without a descriptor is invalid and should explode. I tend to agree.

I agree with that when you're handling side data that comes from 
something like a filter or similar modules within our libraries, as 
that'd be a bug. But this is the public core API for side data, and i 
don't see a reason why we should fail if the caller does 
av_frame_side_data_new(&sd, &nd_sd, 1000, 1, 0);. We never did before, 
and doing it now feels wrong.