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 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.
> 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.
Note the discrepancy between AVPacket/AVStream and AVFrame side data.
AVPacket and AVStream side data DOES NOT allow the same type multiple
times (the helper functions creating a new side data entry ovewrite the
old of the same type of it exists). AVFrame does allow it.
But the fact that we have API like av_frame_get_side_data() which returns
a single (the first) entry implies that - at least for some side data
types - only a single entry makes sense. Therefore - as James mentioned -
for side data types when multiple entries make sense the users typically
iterate over the side data entries themselves using a simple for() loop.
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.
Regards,
Marton