[FFmpeg-devel] [PATCH 1/2] avcodec/avpacket: Use av_copy_packet_side_data() in av_packet_copy_props()

[James Almer]

On 5/3/2017 11:00 PM, Michael Niedermayer wrote:
> On Wed, May 03, 2017 at 03:28:53PM +0200, Hendrik Leppkes wrote:
>> On Wed, May 3, 2017 at 3:23 PM, Michael Niedermayer
>> <michael at niedermayer.cc> wrote:
>>> On Wed, May 03, 2017 at 12:54:53PM +0200, Hendrik Leppkes wrote:
>>>> On Wed, May 3, 2017 at 11:50 AM, Michael Niedermayer
>>>> <michael at niedermayer.cc> wrote:
>>>>> On Wed, May 03, 2017 at 11:37:35AM +0200, wm4 wrote:
>>>>>> On Wed, 3 May 2017 11:29:04 +0200
>>>>>> Michael Niedermayer <michael at niedermayer.cc> wrote:
>>>>>>
>>>>>>> On Wed, May 03, 2017 at 05:29:07AM +0200, wm4 wrote:
>>>>>>>> On Wed,  3 May 2017 05:21:50 +0200
>>>>>>>> Michael Niedermayer <michael at niedermayer.cc> wrote:
>>>>>>>>
>>>>>>>>> Fixes timeout
>>>>>>>>> Fixes: 1293/clusterfuzz-testcase-minimized-6054752074858496
>>>>>>>>>
>>>>>>>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg
>>>>>>>>> Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
>>>>>>>>> ---
>>>>>>>>>  libavcodec/avpacket.c | 3 +++
>>>>>>>>>  1 file changed, 3 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
>>>>>>>>> index 4bf830bb8a..ff7ee730a4 100644
>>>>>>>>> --- a/libavcodec/avpacket.c
>>>>>>>>> +++ b/libavcodec/avpacket.c
>>>>>>>>> @@ -557,6 +557,9 @@ FF_ENABLE_DEPRECATION_WARNINGS
>>>>>>>>>      dst->flags                = src->flags;
>>>>>>>>>      dst->stream_index         = src->stream_index;
>>>>>>>>>
>>>>>>>>> +    if (!dst->side_data_elems);
>>>>>>>>> +        return av_copy_packet_side_data(dst, src);
>>>>>>>>> +
>>>>>>>>>      for (i = 0; i < src->side_data_elems; i++) {
>>>>>>>>>           enum AVPacketSideDataType type = src->side_data[i].type;
>>>>>>>>>           int size          = src->side_data[i].size;
>>>>>>>>
>>>>>>>> This doesn't look right...
>>>>>>>
>>>>>>> already fixed the ; locally
>>>>>>>
>>>>>>>
>>>>>>> [...]
>>>>>>
>>>>>> I didn't see that, I was referring to the fact that you call
>>>>>> av_copy_packet_side_data(), and only sometimes (at least by intention).
>>>>>> That requires at least an explanation in the commit message.
>>>>>
>>>>> av_packet_copy_props() would add side data to the destination packet
>>>>> it doesnt replace previously existing side data except in case of
>>>>> error.
>>>>> I dont know if that is intended but i didnt want to change it as that
>>>>> would be unrelated to this patch
>>>>>
>>>>
>>>> This behavior seems odd at best, so maybe we should just change that
>>>> and make the behavior more logical, and fix your issue at the same
>>>> time?
>>>
>>> That can be done and makes alot of sense after the patch.
>>>
>>> we need to fix this issue in our releases too
>>> a simple bugfix and a seperate behavior change afterwards allows us
>>> to simply backport the bugfix from master.
>>>
>>
>> If anything your "bugfix" here is a performance improvement, and I
>> don't think that warrants backporting either way.
> 
> Before the patch the sample file takes
> 51seconds to decode, after it, it takes 203 milliseconds.
> The difference is caused by only 2 calls to the copy code
> 
> blocking a machine for 51 seconds for something that decodes in 203ms
> otherwise, is IMO worth backporting a fix for.
> 
> We can add a bound to the number of side data elements if theres
> consens about doing that and doing it in releases.
> still, no code should call realloc() in a loop when it can do one
> (re)allocation call at the top.

First we need to figure out if these side data copy functions are meant
to append the source packet's side data to the dest's, or if they should
replace it. That is, we need to see if these functions are meant to
assume the dest packet is empty or not. Because judging by every other
field copied, i guess it's implied the dest packet is expected to be empty.
It's worth nothing that av_stream_add_side_data() overwrites existing
side data if one of the same type already exists, unlike
av_packet_add_side_data().

After that we can adapt the code to do a single realloc, since depending
on the above decision the code will be different.