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

[James Almer]

On 2/14/2018 7:54 PM, wm4 wrote:
> On Wed, 14 Feb 2018 18:57:37 -0300
> James Almer <jamrial at gmail.com> wrote:
> 
>> On 2/14/2018 4:21 PM, wm4 wrote:
>>> On Wed, 14 Feb 2018 13:14:19 -0300
>>> James Almer <jamrial at gmail.com> wrote:
>>>   
>>>> 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().  
>>>
>>> Yeah, I switched src/dst at some point, resulting in confusing text.
>>> Anyway, you could argue it should work both ways, and considering the
>>> past confusion, I don't think it'd be a problem to always strictly
>>> overwrite dst side data like the patch suggests. It would have the
>>> advantage of having clearer semantics. (If side data gets "merged", you
>>> could still argue it should merge the contents in a clever way instead
>>> of just overwriting side data types that in both src and dst. Making a
>>> strict copy of the metadata would have more predictable semantics.
>>>   
>>
>> Ok, will apply a slightly modified version of Yusuke's patch then, by
>> also setting dst->side_data to NULL to avoid issues in
>> av_packet_add_side_data's av_realloc call if the field was
>> uninitialized. Is that ok?
> 
> What is our goal here: changing the semantics, or enabling this
> function to be called on uninitialized packets?

In a way, both. Make it actually copy side data rather than merging it,
which by extension lets you use it on an uninitialized packet.

> 
> If it's the latter, it should probably call av_init_packet() (and also
> set data/size to 0).

av_init_packet() sets buf to NULL, and av_packet_copy_props() is meant
to copy properties and touch nothing else. It's stated in the doxy.

> 
> To be honest I'm not sure if that change won't cause other problems,
> but all these packet functions are so messy, so it's hard to tell how
> it should work.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>