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

[James Almer]

On 2/14/2018 8:13 PM, wm4 wrote:
> On Wed, 14 Feb 2018 19:59:32 -0300
> James Almer <jamrial at gmail.com> wrote:
> 
>> 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.
> 
> OK then. (What a mess.) Possibly it would still make sense to trivially
> factor the code to reset the fields into a separate function, just so
> it's not forgotten should new AVPacket fields be added (but feel free
> to ignore this).

That would result in a reset then copy in this function, so kinda
redundant IMO.
In any case, in the unlikely case a new field is added to AVPacket (its
size is part of the ABI, so that can only happen after a major bump), i
doubt anyone would forget to add it to both init() and copy_props().

Patch amended and pushed.