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

[James Almer]

On 2/14/2018 12:22 AM, Yusuke Nakamura wrote:
> 2018-02-14 12:11 GMT+09:00 James Almer <jamrial at 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 at ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>