[FFmpeg-devel] [PATCH] avpacket: Set dst->side_data_elems to 0 within av_packet_copy_props.

[James Almer]

On 2/14/2018 2:25 AM, wm4 wrote:
> On Wed, 14 Feb 2018 00:11:32 -0300
> James Almer <jamrial at gmail.com> wrote:
> 
>>> ---
>>>  libavcodec/avpacket.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
>>> index 90b8215928..1a9be60e20 100644
>>> --- a/libavcodec/avpacket.c
>>> +++ b/libavcodec/avpacket.c
>>> @@ -571,6 +571,7 @@ FF_ENABLE_DEPRECATION_WARNINGS
>>>      dst->flags                = src->flags;
>>>      dst->stream_index         = src->stream_index;
>>>  
>>> +    dst->side_data_elems = 0;
>>>      for (i = 0; i < src->side_data_elems; i++) {
>>>           enum AVPacketSideDataType type = src->side_data[i].type;
>>>           int size          = src->side_data[i].size;
>>>   
>>
>> Afaik, the intended behavior of this function was to merge the side data
>> in dst with that of src, and this patch would break that.
>> It's admittedly not really defined and can get confusing, especially
>> when the old deprecated API (av_copy_packet, av_copy_packet_side_data,
>> av_dup_packet) do seem to just completely overwrite rather than merge.
>>
>> IMO, we should first define what should happen with side data in this
>> function before we make any further changes to it.
> 
> If you ask me, merging the side data is under-defined at best. What
> happens if there are side data elements of the same type in src and
> dst? It looks like dst currently overwrites src. Does this even make
> sense? You could as well argue that src should be preserved (because it
> could mean that dst is supposed to provide fallbacks for missing info
> in src).

av_packet_add_side_data() used to add whatever new element you feed it
at the end of the array without question. This meant that
av_packet_get_side_data() would never actually get to them if another of
the same types existed beforehand, as it returns the first element of
the requested type it finds while looping through the array.
I changed this in 28f60eeabb to instead replace the existing side data,
so only the last one to be added is actually present in the packet. This
is further enforced by making sure side_data_elems <= AV_PKT_DATA_NB
when adding new elements.
In the case of av_packet_copy_props(), the resulting merge prioritizes
the elements from src over those in dst. Before, the elements from src
would be added at the end of dst and potentially never be returned by
av_packet_get_side_data().

So what do you suggest to do in av_packet_copy_props()? Call
av_packet_free_side_data() on dst before copying elements from src so
effectively generates an actual mirror of properties from src?
Also, it would be great if someone could write a new generic and well
defined side data API already.

PS: frame side data still adds new elements at the end without question
at the request of Ronald.

> So in my opinion, code which requires "merging" needs to be
> conscious about the required semantics and is best off doing it
> manually.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>