[FFmpeg-devel] [PATCH] avformat/apngenc: use the stream parameters extradata if no updated one is made available

[James Almer]

On 11/1/2016 12:54 PM, Andreas Cadhalpun wrote:
> On 01.11.2016 05:09, James Almer wrote:
>> On 10/31/2016 11:32 PM, James Almer wrote:
>>> Fixes remuxing apng streams coming from the apng demuxer.
>>> This is a regression since 97792e85c338d129342f5812e2a52048373e57d6.
>>>
>>> Signed-off-by: James Almer <jamrial at gmail.com>
>>> ---
>>>  libavformat/apngenc.c | 7 +++++++
>>>  1 file changed, 7 insertions(+)
>>>
>>> diff --git a/libavformat/apngenc.c b/libavformat/apngenc.c
>>> index e5e8aa9..2b88dcc 100644
>>> --- a/libavformat/apngenc.c
>>> +++ b/libavformat/apngenc.c
>>> @@ -101,6 +101,13 @@ static int apng_write_header(AVFormatContext *format_context)
>>>      avio_wb64(format_context->pb, PNGSIG);
>>>      // Remaining headers are written when they are copied from the encoder
>>>  
>>> +    apng->extra_data = av_mallocz(format_context->streams[0]->codecpar->extradata_size + AV_INPUT_BUFFER_PADDING_SIZE);
>>> +    if (!apng->extra_data)
>>> +        return AVERROR(ENOMEM);
>>> +    apng->extra_data_size =  format_context->streams[0]->codecpar->extradata_size;
>>> +    memcpy(apng->extra_data, format_context->streams[0]->codecpar->extradata,
>>> +           format_context->streams[0]->codecpar->extradata_size);
>>> +
>>
>> Alternatively we could just go back to the first version of Andreas' patch,
>> where the codecpar extradata was overwritten by the updated one from the
>> packet side data, and get rid of the private context buffer.
>>
>> I assumed keeping the codecpar extradata intact was the correct behavior,
>> based on the avcodec.h documentation and since AVCodecParameters is an
>> intermediary struct meant to pass parameters between de/muxers and
>> de/encoders, but it seems the mov and flv muxers just overwrite it. I'm not
>> sure if it makes a difference at all.
>>
>> Any opinions?
> 
> What I like about the approach of using the private extra_data context buffer is
> that is is quite obvious where it is set. On the other hand the codecpar
> extradata can be set anywhere, which makes it difficult to understand/debug.
> The very bug this patch would fix is an excellent example of that.
> 
> So I'd rather convert the apngdec demuxer to pass the extradata as side data.
> I'll send a patch doing that.
> 
> Best regards,
> Andreas

I'll apply this patch later tonight or tomorrow morning and revert yours to
restore the original proper behavior of making extradata available at init
when possible, and signaling new one as needed on packets.

Will also backport it to 3.2 seeing you already backported yours. apng in
3.2 must not behave in a different way than 3.1.

Sorry for the delay.