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

Submitted by James Almer on Nov. 1, 2016, 2:32 a.m.

Details

Message ID 20161101023247.892-1-jamrial@gmail.com
State New
Headers show

Commit Message

James Almer Nov. 1, 2016, 2:32 a.m.
Fixes remuxing apng streams coming from the apng demuxer.
This is a regression since 97792e85c338d129342f5812e2a52048373e57d6.

Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavformat/apngenc.c | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

James Almer Nov. 1, 2016, 2:57 a.m.
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.

Should be 940b8908b94404a65f9f55e33efb4ccc6c81383c, sorry.

> 
> Signed-off-by: James Almer <jamrial@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);
> +
>      return 0;
>  }
>  
>
James Almer Nov. 1, 2016, 4:09 a.m.
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@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?
Andreas Cadhalpun Nov. 1, 2016, 3:54 p.m.
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@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
James Almer Nov. 1, 2016, 4:06 p.m.
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@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.

Is it worth doing that? This patch is pretty simple and solves the issue for
both encoder and demuxer.
With it the muxer will use either the original extradata or any new one sent
within a packet. In both cases, the muxer uses its own buffer to keep it.

I don't think changing the demuxer to append the same extradata it already
sent during init() again with a packet makes sense. It's extra complexity
for no gain.
Andreas Cadhalpun Nov. 1, 2016, 4:17 p.m.
On 01.11.2016 17:06, James Almer wrote:
> On 11/1/2016 12:54 PM, Andreas Cadhalpun wrote:
>> 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.
> 
> Is it worth doing that? This patch is pretty simple and solves the issue for
> both encoder and demuxer.
> With it the muxer will use either the original extradata or any new one sent
> within a packet. In both cases, the muxer uses its own buffer to keep it.
> 
> I don't think changing the demuxer to append the same extradata it already
> sent during init() again with a packet makes sense. It's extra complexity
> for no gain.

Not again, but instead, as the extradata is then only transferred as side data.
That way it is again consistent between demuxer/decoder and encoder/muxer.

Best regards,
Andreas
Hendrik Leppkes Nov. 1, 2016, 10:10 p.m.
Am 01.11.2016 17:17 schrieb "Andreas Cadhalpun" <
andreas.cadhalpun@googlemail.com>:
>
> On 01.11.2016 17:06, James Almer wrote:
> > On 11/1/2016 12:54 PM, Andreas Cadhalpun wrote:
> >> 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.
> >
> > Is it worth doing that? This patch is pretty simple and solves the
issue for
> > both encoder and demuxer.
> > With it the muxer will use either the original extradata or any new one
sent
> > within a packet. In both cases, the muxer uses its own buffer to keep
it.
> >
> > I don't think changing the demuxer to append the same extradata it
already
> > sent during init() again with a packet makes sense. It's extra
complexity
> > for no gain.
>
> Not again, but instead, as the extradata is then only transferred as side
data.
> That way it is again consistent between demuxer/decoder and encoder/muxer.
>

I don't think thats a good idea. Demuxers should fill the extradata field
if any is present and required for decoding - some other decoder might want
extradata for init or something, and that way you can accommodate it
without having to wait for the first packet.

That's basically how all demuxers work, will extradata and if it can and
does change just send an update notice.

- Hendrik
James Almer Nov. 2, 2016, 4:41 p.m.
On 11/1/2016 7:10 PM, Hendrik Leppkes wrote:
> Am 01.11.2016 17:17 schrieb "Andreas Cadhalpun" <
> andreas.cadhalpun@googlemail.com>:
>>
>> On 01.11.2016 17:06, James Almer wrote:
>>> On 11/1/2016 12:54 PM, Andreas Cadhalpun wrote:
>>>> 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.
>>>
>>> Is it worth doing that? This patch is pretty simple and solves the
> issue for
>>> both encoder and demuxer.
>>> With it the muxer will use either the original extradata or any new one
> sent
>>> within a packet. In both cases, the muxer uses its own buffer to keep
> it.
>>>
>>> I don't think changing the demuxer to append the same extradata it
> already
>>> sent during init() again with a packet makes sense. It's extra
> complexity
>>> for no gain.
>>
>> Not again, but instead, as the extradata is then only transferred as side
> data.
>> That way it is again consistent between demuxer/decoder and encoder/muxer.
>>
> 
> I don't think thats a good idea. Demuxers should fill the extradata field
> if any is present and required for decoding - some other decoder might want
> extradata for init or something, and that way you can accommodate it
> without having to wait for the first packet.
> 
> That's basically how all demuxers work, will extradata and if it can and
> does change just send an update notice.

Agree, that's how it's done for flac for example.

Should i revert this and apply my patch instead? Or something else?

> 
> - Hendrik
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Andreas Cadhalpun Nov. 2, 2016, 7:13 p.m.
On 01.11.2016 23:10, Hendrik Leppkes wrote:
> Am 01.11.2016 17:17 schrieb "Andreas Cadhalpun" <
> andreas.cadhalpun@googlemail.com>:
>> Not again, but instead, as the extradata is then only transferred as side data.
>> That way it is again consistent between demuxer/decoder and encoder/muxer.
>>
> 
> I don't think thats a good idea. Demuxers should fill the extradata field
> if any is present and required for decoding - some other decoder might want
> extradata for init or something, and that way you can accommodate it
> without having to wait for the first packet.

Is this documented somewhere?

> That's basically how all demuxers work, will extradata and if it can and
> does change just send an update notice.

Requiring to use codecpar->extradata for passing this from demuxer to decoder,
while at the same time requiring to use AV_PKT_DATA_NEW_EXTRADATA side data
to pass this from muxer to encoder seems quite strange to me.

Best regards,
Andreas
Hendrik Leppkes Nov. 2, 2016, 8:10 p.m.
On Wed, Nov 2, 2016 at 8:13 PM, Andreas Cadhalpun
<andreas.cadhalpun@googlemail.com> wrote:
> On 01.11.2016 23:10, Hendrik Leppkes wrote:
>> Am 01.11.2016 17:17 schrieb "Andreas Cadhalpun" <
>> andreas.cadhalpun@googlemail.com>:
>>> Not again, but instead, as the extradata is then only transferred as side data.
>>> That way it is again consistent between demuxer/decoder and encoder/muxer.
>>>
>>
>> I don't think thats a good idea. Demuxers should fill the extradata field
>> if any is present and required for decoding - some other decoder might want
>> extradata for init or something, and that way you can accommodate it
>> without having to wait for the first packet.
>
> Is this documented somewhere?

What documented? This seems quite logical. If you have extradata, set
extradata in codecpar

>
>> That's basically how all demuxers work, will extradata and if it can and
>> does change just send an update notice.
>
> Requiring to use codecpar->extradata for passing this from demuxer to decoder,
> while at the same time requiring to use AV_PKT_DATA_NEW_EXTRADATA side data
> to pass this from muxer to encoder seems quite strange to me.
>

encoders should also be able to just set extradata and the muxer read
that, this is how many other encoders work.
I don't know how the apng muxer works, but maybe it also just pretends
to be special.

- Hendrik
Andreas Cadhalpun Nov. 2, 2016, 8:20 p.m.
On 02.11.2016 21:10, Hendrik Leppkes wrote:
> On Wed, Nov 2, 2016 at 8:13 PM, Andreas Cadhalpun
> <andreas.cadhalpun@googlemail.com> wrote:
>> On 01.11.2016 23:10, Hendrik Leppkes wrote:
>>> Am 01.11.2016 17:17 schrieb "Andreas Cadhalpun" <
>>> andreas.cadhalpun@googlemail.com>:
>>>> Not again, but instead, as the extradata is then only transferred as side data.
>>>> That way it is again consistent between demuxer/decoder and encoder/muxer.
>>>>
>>>
>>> I don't think thats a good idea. Demuxers should fill the extradata field
>>> if any is present and required for decoding - some other decoder might want
>>> extradata for init or something, and that way you can accommodate it
>>> without having to wait for the first packet.
>>
>> Is this documented somewhere?
> 
> What documented?

You're claim that 'Demuxers should fill the extradata field'.

> This seems quite logical. If you have extradata, set extradata in codecpar

It's not logical to do this, if it's not necessary, e.g. for the apng demuxer.
It is only necessary if the decoder actually needs this information during
init, but the apng decoder does not.

>>> That's basically how all demuxers work, will extradata and if it can and
>>> does change just send an update notice.
>>
>> Requiring to use codecpar->extradata for passing this from demuxer to decoder,
>> while at the same time requiring to use AV_PKT_DATA_NEW_EXTRADATA side data
>> to pass this from muxer to encoder seems quite strange to me.
>>
> 
> encoders should also be able to just set extradata and the muxer read
> that, this is how many other encoders work.

That only works if it is set during write_header, setting it during
write_packet was broken by commit 5ef1959.

> I don't know how the apng muxer works, but maybe it also just pretends
> to be special.

It's special in that it doesn't set the extradata during write_header.

Best regards,
Andreas
Hendrik Leppkes Nov. 3, 2016, 8:52 a.m.
On Wed, Nov 2, 2016 at 9:20 PM, Andreas Cadhalpun
<andreas.cadhalpun@googlemail.com> wrote:
> On 02.11.2016 21:10, Hendrik Leppkes wrote:
>> On Wed, Nov 2, 2016 at 8:13 PM, Andreas Cadhalpun
>> <andreas.cadhalpun@googlemail.com> wrote:
>>> On 01.11.2016 23:10, Hendrik Leppkes wrote:
>>>> Am 01.11.2016 17:17 schrieb "Andreas Cadhalpun" <
>>>> andreas.cadhalpun@googlemail.com>:
>>>>> Not again, but instead, as the extradata is then only transferred as side data.
>>>>> That way it is again consistent between demuxer/decoder and encoder/muxer.
>>>>>
>>>>
>>>> I don't think thats a good idea. Demuxers should fill the extradata field
>>>> if any is present and required for decoding - some other decoder might want
>>>> extradata for init or something, and that way you can accommodate it
>>>> without having to wait for the first packet.
>>>
>>> Is this documented somewhere?
>>
>> What documented?
>
> You're claim that 'Demuxers should fill the extradata field'.
>
>> This seems quite logical. If you have extradata, set extradata in codecpar
>
> It's not logical to do this, if it's not necessary, e.g. for the apng demuxer.
> It is only necessary if the decoder actually needs this information during
> init, but the apng decoder does not.

Hence my point about trying to stick to a common behavior scheme
without looking at what *OUR* decoder needs - there could be others,
after all.
Its common to write extradata into codecpar if its present. You can
try to dispute that as long as you wish, but that doesn't make it any
less true.

avcodec is not the only thing people use avformat with, and
vice-versa, so sticking to common patterns makes the life easier for
those.

Do we really need to write this down somewhere to convince you, what
do we have the ML for then if we can't inform contributors about
common practice?

- Hendrik
Andreas Cadhalpun Nov. 3, 2016, 6:39 p.m.
On 03.11.2016 09:52, Hendrik Leppkes wrote:
> Hence my point about trying to stick to a common behavior scheme
> without looking at what *OUR* decoder needs - there could be others,
> after all.
> Its common to write extradata into codecpar if its present. You can
> try to dispute that as long as you wish, but that doesn't make it any
> less true.
> 
> avcodec is not the only thing people use avformat with, and
> vice-versa, so sticking to common patterns makes the life easier for
> those.
> 
> Do we really need to write this down somewhere to convince you, what
> do we have the ML for then if we can't inform contributors about
> common practice?

What you need to convince me are good arguments.
The reason why I asked about documentation is that if it's not documented,
your argument about other decoders, that you repeated above, is void,
because others shouldn't rely on undocumented behavior, as our
documentation states clearly [1]:
"Behavior in undocumented situations may change slightly (and be documented)."

So if you want to be able to rely on the behavior you describe, you'll
have to get it documented.
As you seem to care about this a lot, I don't understand why you appear
to be opposed to the idea of documenting it.

Best regards,
Andreas

1: https://ffmpeg.org/doxygen/3.2/index.html
Hendrik Leppkes Nov. 3, 2016, 6:53 p.m.
On Thu, Nov 3, 2016 at 7:39 PM, Andreas Cadhalpun
<andreas.cadhalpun@googlemail.com> wrote:
> On 03.11.2016 09:52, Hendrik Leppkes wrote:
>> Hence my point about trying to stick to a common behavior scheme
>> without looking at what *OUR* decoder needs - there could be others,
>> after all.
>> Its common to write extradata into codecpar if its present. You can
>> try to dispute that as long as you wish, but that doesn't make it any
>> less true.
>>
>> avcodec is not the only thing people use avformat with, and
>> vice-versa, so sticking to common patterns makes the life easier for
>> those.
>>
>> Do we really need to write this down somewhere to convince you, what
>> do we have the ML for then if we can't inform contributors about
>> common practice?
>
> What you need to convince me are good arguments.
> The reason why I asked about documentation is that if it's not documented,
> your argument about other decoders, that you repeated above, is void,
> because others shouldn't rely on undocumented behavior, as our
> documentation states clearly [1]:
> "Behavior in undocumented situations may change slightly (and be documented)."
>
> So if you want to be able to rely on the behavior you describe, you'll
> have to get it documented.
> As you seem to care about this a lot, I don't understand why you appear
> to be opposed to the idea of documenting it.
>
>

Since you want docs, I even found one extremely specific to this
particular case:
https://ffmpeg.org/ffmpeg-formats.html#apng

"All headers, but the PNG signature, up to (but not including) the
first fcTL chunk are transmitted as extradata."

I'm sure you will argue that side-data is still extradata of some
sort, but fact is the apng demuxer changed behavior recently, and the
documentation suggests a certain content of extradata is to be
expected.

- Hendrik
Andreas Cadhalpun Nov. 3, 2016, 7 p.m.
On 03.11.2016 19:53, Hendrik Leppkes wrote:
> Since you want docs, I even found one extremely specific to this
> particular case:
> https://ffmpeg.org/ffmpeg-formats.html#apng
> 
> "All headers, but the PNG signature, up to (but not including) the
> first fcTL chunk are transmitted as extradata."
> 
> I'm sure you will argue that side-data is still extradata of some
> sort,

Then why do you continue this pointless discussion instead of writing
some nice general documentation about the behavior you actually
expect?

Best regards,
Andreas
James Almer Nov. 3, 2016, 7:03 p.m.
On 11/3/2016 4:00 PM, Andreas Cadhalpun wrote:
> On 03.11.2016 19:53, Hendrik Leppkes wrote:
>> Since you want docs, I even found one extremely specific to this
>> particular case:
>> https://ffmpeg.org/ffmpeg-formats.html#apng
>>
>> "All headers, but the PNG signature, up to (but not including) the
>> first fcTL chunk are transmitted as extradata."
>>
>> I'm sure you will argue that side-data is still extradata of some
>> sort,
> 
> Then why do you continue this pointless discussion instead of writing
> some nice general documentation about the behavior you actually
> expect?
> 
> Best regards,
> Andreas

You wanted arguments an email ago. He just gave them to you.
Andreas Cadhalpun Nov. 3, 2016, 7:06 p.m.
On 03.11.2016 20:03, James Almer wrote:
> On 11/3/2016 4:00 PM, Andreas Cadhalpun wrote:
>> On 03.11.2016 19:53, Hendrik Leppkes wrote:
>>> Since you want docs, I even found one extremely specific to this
>>> particular case:
>>> https://ffmpeg.org/ffmpeg-formats.html#apng
>>>
>>> "All headers, but the PNG signature, up to (but not including) the
>>> first fcTL chunk are transmitted as extradata."
>>>
>>> I'm sure you will argue that side-data is still extradata of some
>>> sort,
>>
>> Then why do you continue this pointless discussion instead of writing
>> some nice general documentation about the behavior you actually
>> expect?
>>
>> Best regards,
>> Andreas
> 
> You wanted arguments an email ago. He just gave them to you.

Together with the counter-arguments...

Best regards,
Andreas
James Almer Nov. 17, 2016, 11:48 p.m.
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@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.
James Almer Nov. 18, 2016, 4:08 p.m.
On 11/17/2016 8:48 PM, James Almer wrote:
> 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@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.

Applied.
Andreas Cadhalpun Nov. 18, 2016, 9:33 p.m.
On 18.11.2016 00:48, James Almer wrote:
> On 11/1/2016 12:54 PM, Andreas Cadhalpun wrote:
> 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.

I still don't think that's a valid argument unless the "proper behavior" is
documented. Will you write documentation for that?

Best regards,
Andreas

Patch hide | download patch | download mbox

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);
+
     return 0;
 }