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
