Re: [libav-devel] [PATCH 3/3] libvo-aacenc: Only produce extradata if the global header flag is set
Hi, On Tue, Apr 12, 2011 at 3:44 AM, Martin Storsjö wrote: > --- > libavcodec/libvo-aacenc.c | 18 ++ > 1 files changed, 10 insertions(+), 8 deletions(-) > > diff --git a/libavcodec/libvo-aacenc.c b/libavcodec/libvo-aacenc.c > index 3c7dde7..7c1738d 100644 > --- a/libavcodec/libvo-aacenc.c > +++ b/libavcodec/libvo-aacenc.c > @@ -62,12 +62,6 @@ static av_cold int aac_encode_init(AVCodecContext *avctx) > return AVERROR_UNKNOWN; > } > > - avctx->extradata_size = 2; > - avctx->extradata = av_mallocz(avctx->extradata_size + > - FF_INPUT_BUFFER_PADDING_SIZE); > - if (!avctx->extradata) > - return AVERROR(ENOMEM); > - > for (index = 0; index < 16; index++) > if (avctx->sample_rate == ff_mpeg4audio_sample_rates[index]) > break; > @@ -76,8 +70,16 @@ static av_cold int aac_encode_init(AVCodecContext *avctx) > avctx->sample_rate); > return AVERROR_NOTSUPP; > } > - avctx->extradata[0] = 0x02 << 3 | index >> 1; > - avctx->extradata[1] = (index & 0x01) << 7 | avctx->channels << 3; > + if (avctx->flags & CODEC_FLAG_GLOBAL_HEADER) { > + avctx->extradata_size = 2; > + avctx->extradata = av_mallocz(avctx->extradata_size + > + FF_INPUT_BUFFER_PADDING_SIZE); > + if (!avctx->extradata) > + return AVERROR(ENOMEM); > + > + avctx->extradata[0] = 0x02 << 3 | index >> 1; > + avctx->extradata[1] = (index & 0x01) << 7 | avctx->channels << 3; > + } > return 0; > } This isn't really what the flag means. If it's data that can be discarded at choice, then the flag can be ignored. Formats that don't like it, simply don't set extradata. The idea of the flag is that it creates extradata if set, and then not interleave that same data in the bitstream, and if the flag is not set, it interleaves that extradata in the bitstream instead. This patch doesn't appear to do that, so then it's not necessary... Ronald ___ libav-devel mailing list libav-devel@libav.org https://lists.libav.org/mailman/listinfo/libav-devel
Re: [libav-devel] [PATCH 3/3] libvo-aacenc: Only produce extradata if the global header flag is set
On Tue, 12 Apr 2011, Ronald S. Bultje wrote: > On Tue, Apr 12, 2011 at 3:44 AM, Martin Storsjö wrote: > > --- > > libavcodec/libvo-aacenc.c | 18 ++ > > 1 files changed, 10 insertions(+), 8 deletions(-) > > > > diff --git a/libavcodec/libvo-aacenc.c b/libavcodec/libvo-aacenc.c > > index 3c7dde7..7c1738d 100644 > > --- a/libavcodec/libvo-aacenc.c > > +++ b/libavcodec/libvo-aacenc.c > > @@ -62,12 +62,6 @@ static av_cold int aac_encode_init(AVCodecContext *avctx) > > return AVERROR_UNKNOWN; > > } > > > > - avctx->extradata_size = 2; > > - avctx->extradata = av_mallocz(avctx->extradata_size + > > - FF_INPUT_BUFFER_PADDING_SIZE); > > - if (!avctx->extradata) > > - return AVERROR(ENOMEM); > > - > > for (index = 0; index < 16; index++) > > if (avctx->sample_rate == ff_mpeg4audio_sample_rates[index]) > > break; > > @@ -76,8 +70,16 @@ static av_cold int aac_encode_init(AVCodecContext *avctx) > > avctx->sample_rate); > > return AVERROR_NOTSUPP; > > } > > - avctx->extradata[0] = 0x02 << 3 | index >> 1; > > - avctx->extradata[1] = (index & 0x01) << 7 | avctx->channels << 3; > > + if (avctx->flags & CODEC_FLAG_GLOBAL_HEADER) { > > + avctx->extradata_size = 2; > > + avctx->extradata = av_mallocz(avctx->extradata_size + > > + FF_INPUT_BUFFER_PADDING_SIZE); > > + if (!avctx->extradata) > > + return AVERROR(ENOMEM); > > + > > + avctx->extradata[0] = 0x02 << 3 | index >> 1; > > + avctx->extradata[1] = (index & 0x01) << 7 | avctx->channels << 3; > > + } > > return 0; > > } > > This isn't really what the flag means. If it's data that can be > discarded at choice, then the flag can be ignored. Formats that don't > like it, simply don't set extradata. > > The idea of the flag is that it creates extradata if set, and then not > interleave that same data in the bitstream, and if the flag is not > set, it interleaves that extradata in the bitstream instead. This > patch doesn't appear to do that, so then it's not necessary... Yes, we do that already, by enabling ADTS if the global header flag isn't set - the ADTS header contains the same data (but is produced by the encoder library when the adtsUsed flag is set). But if the global header flag isn't set, we perhaps shouldn't produce any extradata at all. Does anyone else have an opinion, or even better, insight into whether it actually is ok to produce extradata even if the global header flag isn't set? The data set in the extradata is MPEG4 Audio Specific Config - I don't think it makes sense to output such extradata while outputting ADTS data. // Martin___ libav-devel mailing list libav-devel@libav.org https://lists.libav.org/mailman/listinfo/libav-devel
Re: [libav-devel] [PATCH 3/3] libvo-aacenc: Only produce extradata if the global header flag is set
Hi, On Wed, Apr 13, 2011 at 2:49 AM, Martin Storsjö wrote: > On Tue, 12 Apr 2011, Ronald S. Bultje wrote: >> On Tue, Apr 12, 2011 at 3:44 AM, Martin Storsjö wrote: >> > --- >> > libavcodec/libvo-aacenc.c | 18 ++ >> > 1 files changed, 10 insertions(+), 8 deletions(-) >> > >> > diff --git a/libavcodec/libvo-aacenc.c b/libavcodec/libvo-aacenc.c >> > index 3c7dde7..7c1738d 100644 >> > --- a/libavcodec/libvo-aacenc.c >> > +++ b/libavcodec/libvo-aacenc.c >> > @@ -62,12 +62,6 @@ static av_cold int aac_encode_init(AVCodecContext >> > *avctx) >> > return AVERROR_UNKNOWN; >> > } >> > >> > - avctx->extradata_size = 2; >> > - avctx->extradata = av_mallocz(avctx->extradata_size + >> > - FF_INPUT_BUFFER_PADDING_SIZE); >> > - if (!avctx->extradata) >> > - return AVERROR(ENOMEM); >> > - >> > for (index = 0; index < 16; index++) >> > if (avctx->sample_rate == ff_mpeg4audio_sample_rates[index]) >> > break; >> > @@ -76,8 +70,16 @@ static av_cold int aac_encode_init(AVCodecContext >> > *avctx) >> > avctx->sample_rate); >> > return AVERROR_NOTSUPP; >> > } >> > - avctx->extradata[0] = 0x02 << 3 | index >> 1; >> > - avctx->extradata[1] = (index & 0x01) << 7 | avctx->channels << 3; >> > + if (avctx->flags & CODEC_FLAG_GLOBAL_HEADER) { >> > + avctx->extradata_size = 2; >> > + avctx->extradata = av_mallocz(avctx->extradata_size + >> > + FF_INPUT_BUFFER_PADDING_SIZE); >> > + if (!avctx->extradata) >> > + return AVERROR(ENOMEM); >> > + >> > + avctx->extradata[0] = 0x02 << 3 | index >> 1; >> > + avctx->extradata[1] = (index & 0x01) << 7 | avctx->channels << 3; >> > + } >> > return 0; >> > } >> >> This isn't really what the flag means. If it's data that can be >> discarded at choice, then the flag can be ignored. Formats that don't >> like it, simply don't set extradata. >> >> The idea of the flag is that it creates extradata if set, and then not >> interleave that same data in the bitstream, and if the flag is not >> set, it interleaves that extradata in the bitstream instead. This >> patch doesn't appear to do that, so then it's not necessary... > > Yes, we do that already, by enabling ADTS if the global header flag isn't > set - the ADTS header contains the same data (but is produced by the > encoder library when the adtsUsed flag is set). But if the global header > flag isn't set, we perhaps shouldn't produce any extradata at all. > > Does anyone else have an opinion, or even better, insight into whether it > actually is ok to produce extradata even if the global header flag isn't > set? The data set in the extradata is MPEG4 Audio Specific Config - I > don't think it makes sense to output such extradata while outputting ADTS > data. I didn't see that you used the same flag elsewhere in this file already. Just checked, turns out it's true, so then patch is fine. Sorry I didn't see that before. Ronald ___ libav-devel mailing list libav-devel@libav.org https://lists.libav.org/mailman/listinfo/libav-devel
Re: [libav-devel] [PATCH 3/3] libvo-aacenc: Only produce extradata if the global header flag is set
On Wed, 13 Apr 2011, Ronald S. Bultje wrote: > On Wed, Apr 13, 2011 at 2:49 AM, Martin Storsjö wrote: > > On Tue, 12 Apr 2011, Ronald S. Bultje wrote: > >> On Tue, Apr 12, 2011 at 3:44 AM, Martin Storsjö wrote: > >> > --- > >> > libavcodec/libvo-aacenc.c | 18 ++ > >> > 1 files changed, 10 insertions(+), 8 deletions(-) > >> > > >> > diff --git a/libavcodec/libvo-aacenc.c b/libavcodec/libvo-aacenc.c > >> > index 3c7dde7..7c1738d 100644 > >> > --- a/libavcodec/libvo-aacenc.c > >> > +++ b/libavcodec/libvo-aacenc.c > >> > @@ -62,12 +62,6 @@ static av_cold int aac_encode_init(AVCodecContext > >> > *avctx) > >> > return AVERROR_UNKNOWN; > >> > } > >> > > >> > - avctx->extradata_size = 2; > >> > - avctx->extradata = av_mallocz(avctx->extradata_size + > >> > - FF_INPUT_BUFFER_PADDING_SIZE); > >> > - if (!avctx->extradata) > >> > - return AVERROR(ENOMEM); > >> > - > >> > for (index = 0; index < 16; index++) > >> > if (avctx->sample_rate == ff_mpeg4audio_sample_rates[index]) > >> > break; > >> > @@ -76,8 +70,16 @@ static av_cold int aac_encode_init(AVCodecContext > >> > *avctx) > >> > avctx->sample_rate); > >> > return AVERROR_NOTSUPP; > >> > } > >> > - avctx->extradata[0] = 0x02 << 3 | index >> 1; > >> > - avctx->extradata[1] = (index & 0x01) << 7 | avctx->channels << 3; > >> > + if (avctx->flags & CODEC_FLAG_GLOBAL_HEADER) { > >> > + avctx->extradata_size = 2; > >> > + avctx->extradata = av_mallocz(avctx->extradata_size + > >> > + > >> > FF_INPUT_BUFFER_PADDING_SIZE); > >> > + if (!avctx->extradata) > >> > + return AVERROR(ENOMEM); > >> > + > >> > + avctx->extradata[0] = 0x02 << 3 | index >> 1; > >> > + avctx->extradata[1] = (index & 0x01) << 7 | avctx->channels << > >> > 3; > >> > + } > >> > return 0; > >> > } > >> > >> This isn't really what the flag means. If it's data that can be > >> discarded at choice, then the flag can be ignored. Formats that don't > >> like it, simply don't set extradata. > >> > >> The idea of the flag is that it creates extradata if set, and then not > >> interleave that same data in the bitstream, and if the flag is not > >> set, it interleaves that extradata in the bitstream instead. This > >> patch doesn't appear to do that, so then it's not necessary... > > > > Yes, we do that already, by enabling ADTS if the global header flag isn't > > set - the ADTS header contains the same data (but is produced by the > > encoder library when the adtsUsed flag is set). But if the global header > > flag isn't set, we perhaps shouldn't produce any extradata at all. > > > > Does anyone else have an opinion, or even better, insight into whether it > > actually is ok to produce extradata even if the global header flag isn't > > set? The data set in the extradata is MPEG4 Audio Specific Config - I > > don't think it makes sense to output such extradata while outputting ADTS > > data. > > I didn't see that you used the same flag elsewhere in this file > already. Just checked, turns out it's true, so then patch is fine. > Sorry I didn't see that before. Thanks - pushed. // Martin___ libav-devel mailing list libav-devel@libav.org https://lists.libav.org/mailman/listinfo/libav-devel