On 2/14/2018 12:22 AM, Yusuke Nakamura wrote: > 2018-02-14 12:11 GMT+09:00 James Almer <jamr...@gmail.com>: > >> On 2/13/2018 11:43 PM, Yusuke Nakamura wrote: >>> This makes you need not call av_init_packet before av_packet_copy_props >> like the following. >>> >>> AVPacket dst; >>> av_packet_copy_props(&dst, &src); >> >> In this scenario, dst->side_data is uninitialized, and bad things can >> happen when av_packet_copy_props calls av_packet_new_side_data. >> >> For something like this, it's best to instead just zero initialize dst. >> Also, av_packet_init should *always* be called before a packet stored on >> stack is used, regardless of it being zero initialized or not. >> > > Ooops! Sorry, originally it is for the case using av_packet_ref not > av_packet_copy_props. > av_packet_ref expects the dst packet is inited by av_packet_init?
It needs to be in a "clean" state, be it because it was just allocated by av_packet_alloc, or right after having zero initialized it or called av_packet_init or av_packet_unref, because otherwise dst->buf will be either uninitialized or wrongly pointing to something (Apparently only an issue if the source packet is not ref counted, though). Similarly, what i said above regarding dst->side_data. > If so, should be documented on > av_packet_ref docs I think. I'm trapped that. I don't know if this is the intended behavior, but that's how it currently works. Maybe that should be fixed rather than the documentation, after it's properly defined. > > >>> --- >>> 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. >> _______________________________________________ >> ffmpeg-devel mailing list >> ffmpeg-devel@ffmpeg.org >> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >> > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel