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

[wm4]

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). So in my opinion, code which requires "merging" needs to be
conscious about the required semantics and is best off doing it
manually.