[FFmpeg-devel] [PATCH 1/5] libavutil: add convenience accessors for frame side data

[James Almer]

On 5/4/2021 12:44 PM, Lynne wrote:
> Apr 30, 2021, 22:29 by cus at passwd.hu:
> 
>>
>>
>> On Fri, 30 Apr 2021, Lynne wrote:
>>
>>> Apr 30, 2021, 13:34 by bradh at frogmouth.net:
>>>
>>>> Signed-off-by: Brad Hards <bradh at frogmouth.net>
>>>> ---
>>>>   libavutil/frame.c | 31 +++++++++++++++++++++++++++++++
>>>>   libavutil/frame.h | 23 +++++++++++++++++++++++
>>>>   2 files changed, 54 insertions(+)
>>>>
>>>> diff --git a/libavutil/frame.c b/libavutil/frame.c
>>>> index 2ec59b44b1..9f9953c2b4 100644
>>>> --- a/libavutil/frame.c
>>>> +++ b/libavutil/frame.c
>>>> @@ -625,6 +625,37 @@ AVFrameSideData *av_frame_get_side_data(const AVFrame *frame,
>>>>   return NULL;
>>>>   }
>>>>
>>>> +AVFrameSideData *av_frame_get_side_data_n(const AVFrame *frame,
>>>> +                                          enum AVFrameSideDataType type,
>>>> +                                          const int side_data_instance)
>>>> +{
>>>> +    int i;
>>>> +    int n = 0;
>>>> +
>>>> +    for (i = 0; i < frame->nb_side_data; i++) {
>>>> +        if (frame->side_data[i]->type == type) {
>>>> +            if (n == side_data_instance) {
>>>> +                return frame->side_data[i];
>>>> +            } else {
>>>> +                n++;
>>>> +            }
>>>> +        }
>>>> +    }
>>>> +    return NULL;
>>>> +}
>>>>
>>>
>>> _follow_the_code_style_
>>> We have a document! We have thousands of lines of code already written with it!
>>> We do not add brackets on one-line statements, and we allow for (int loops.
>>>
>>
>> The developer docs also says that FFmpeg does not force an indentation style on developers. So strictly speaking patch authors are not obligated to follow the general practice if they don't want to.
>>
> 
> I already said what I had to say about this, so I'm just going to say "no" and
> end it at that.
> Unless you too agree that asterisk symbols look so much like insects we should
> definitely have a MUL() macro, then we can talk.
> 
> 
>> If we want to provide an API for getting multiple frame side data of the same type, then I think the more natural thing to do is extending the existing function, e.g.: "av_frame_get_side_data2(frame, type, prev)"
>> This way the user can use the extended version (iterator-style) if having multiple side data makes sense, and the original version if it does not.
>>
> 
> I could agree with that, though I'd still prefer a type-less av_frame_get_side_data_next(frame, prev).

Again, AVFrameSideData has no fields pointing to another entry. It's all 
stored as flat array of pointers in AVFrame.side_data. A next() 
implementation would have to iterate through the entire array from the 
first element until it finds prev, and then return the next one, on 
every call.

An iterate() implementation using a caller owned state variable is much 
more efficient. Something like (assuming type-less)

> AVFrameSideData *av_frame_side_data_iterate(const AVFrame *frame, void **opaque)
> {
>     uintptr_t i = (uintptr_t)*opaque;
> 
>     if (i >= frame->nb_side_data)
>         return NULL;
> 
>     *opaque = (void*)(i + 1);
> 
>     return frame->side_data[i];
> }

Which is how av_codec_iterate() and many others handle this.

I would also need a warning that calling av_frame_remove_side_data() 
will invalidate the iterator state.