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

[wm4]

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.

> Also, it would be great if someone could write a new generic and well
> defined side data API already.

I thought about it, but I don't know how I'd make old and new API
compatible. The API user is completely free to manipulate the current
side data management structures manually, so old and new side data
could get out of sync without a way to know which is the correct
version. And obviously we can't just break the old API now.

Maybe if the new side data API used the old fields to store the data,
but that'd be a pain too.

I'm open for suggestions.

> PS: frame side data still adds new elements at the end without question
> at the request of Ronald.

That sounds weird. What was the reason?