| Message ID | 20161101023247.892-1-jamrial@gmail.com |
|---|---|
| State | New |
| Headers | show |
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; > } > >
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?
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
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.
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
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
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 >
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
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
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
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
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
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
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
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.
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
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.
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.
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
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; }
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(+)