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

[wm4]

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).