[FFmpeg-devel] [PATCH v2 1/4] frame: handle add side data with the same type

[Marton Balint]

On Sat, 2 Nov 2019, Zhao Zhili wrote:

>
>
>> On Nov 2, 2019, at 1:16 AM, Marton Balint <cus at passwd.hu> wrote:
>> 
>> 
>> 
>> On Fri, 1 Nov 2019, "zhilizhao(赵志立)" wrote:
>> 
>>> 
>>> 
>>>> On Nov 1, 2019, at 8:13 PM, Hendrik Leppkes <h.leppkes at gmail.com> wrote:
>>>> On Fri, Nov 1, 2019 at 1:03 PM <quinkblack at foxmail.com> wrote:
>>>>> From: Zhao Zhili <zhilizhao at tencent.com>
>>>>> ---
>>>>> libavutil/frame.c | 13 +++++++++++++
>>>>> libavutil/frame.h |  4 ++++
>>>>> 2 files changed, 17 insertions(+)
>>>> I believe there have been some use-cases, especially around
>>>> closed-captions, where multiple blocks of the same type have been
>>>> used, somehow. Since this is really an API change, not sure its such a
>>>> good idea.
>>> 
>>> I guess it may be too late to change the behavior.
>> 
>> I am not sure, all our API around side data (get/remove) is based on the assumption that a single entry of a type exists. Also for packet/stream side data it is already assumed as far as I see. So at least for the sake of consistency it should be the same way. Maybe we should print a deprecation warning if something adds multiple side data of the same type. And later sometime it can be changed to actually replace the old side data.
>> 
>>> Whether keep the current
>>> behavior or not, remove_side_data is broken. It can remove multiple side data
>>> and miss a few.
>> 
>> Yes.
>> 
>>> 
>>> It can be fixed in two different ways:
>>> 
>>> 1. Remove the first side data of the supplied type, like get_side_data().
>>> 2. Remove all side data of the supplied type.
>>> 
>>> The first solution match current documentation.
>> 
>> I don't see why the 2nd would contradict docs:
>> 
>> /**
>> * If side data of the supplied type exists in the frame, free it and remove it
>> * from the frame.
>> */
>> 
>> So I'd vote for 2), which removes all side data of the specified type.
>
> APIchanges says (if it counts as part of the documentation):
>
> 2014-02-24 - 3e1f241 / d161ae0 - lavu 52.69.100 / 53.8.0 - frame.h
>  Add av_frame_remove_side_data() for removing a single side data
>  instance from a frame.

I guess that can be iterpreted that way, still I'd go for 2).

Regards,
Marton