On Wed, 14 Feb 2018 19:59:32 -0300 James Almer <jamr...@gmail.com> wrote:
> On 2/14/2018 7:54 PM, wm4 wrote: > > On Wed, 14 Feb 2018 18:57:37 -0300 > > James Almer <jamr...@gmail.com> wrote: > > > >> On 2/14/2018 4:21 PM, wm4 wrote: > >>> On Wed, 14 Feb 2018 13:14:19 -0300 > >>> James Almer <jamr...@gmail.com> wrote: > >>> > >>>> On 2/14/2018 2:25 AM, wm4 wrote: > >>>>> On Wed, 14 Feb 2018 00:11:32 -0300 > >>>>> James Almer <jamr...@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). _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel