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

wm4 nfxjfg at googlemail.com
Thu Feb 15 00:54:46 EET 2018


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?

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

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.


More information about the ffmpeg-devel mailing list