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

[James Almer]

On 4/30/2021 10:36 AM, 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.
> 
> I don't like this function's name, and I don't like the way it operates.
> If we do allow multiple side-data entries with the same type (my opinion is if you can't read them
> without workarounds they're just not allowed), I'd much rather have a linked list, which would
> allow us to keep the same style of _next(prev) iteration functions we've used everywhere else.
> Plus, it would mean you don't have to do a worst possible case iteration for each lookup when you
> have a million side data entries. Users have wanted to do this.
> 
> I think we should just have a av_frame_get_side_data_next(prev), where prev will be
> NULL for the first call. Users can filter by data type themselves.

av_frame_get_side_data_iterate(void **opaque), if anything, following 
existing API like those iterating through AVCodecs. There's nothing in a 
returned AVFrameSideData that points to the next entry.

In any case, this would require a warning about calling 
av_frame_remove_side_data() invalidating the iteration state.

> That way av_frame_get_side_data_nb can be removed.
> 
> I'd also like an APIchanges entry we're allowing multiple side data entries with the same type.
> This is not a small change in behavior that we're making official.

This would not be a change in behavior. We would only be adding an 
official way to fetch every entry beyond the first by introducing an 
iterator function, something already possible if you manually iterate 
through frame->side_data entries, something we afaik never really forbid.

> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>