[3/3] libvo-aacenc: Only produce extradata if the global header flag is set
diff mbox

Message ID 1302594268-81662-3-git-send-email-martin@martin.st
State Committed
Commit 2d3267936a40c0a8db6ab76aeb0017e1959ae2fa
Headers show

Commit Message

Martin Storsjö April 12, 2011, 7:44 a.m. 
---
 libavcodec/libvo-aacenc.c |   18 ++++++++++--------
 1 files changed, 10 insertions(+), 8 deletions(-)

Comments

Ronald Bultje April 13, 2011, 12:01 a.m. | #1 
Hi,

On Tue, Apr 12, 2011 at 3:44 AM, Martin Storsjö <martin@martin.st> 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
Martin Storsjö April 13, 2011, 6:49 a.m. | #2 
On Tue, 12 Apr 2011, Ronald S. Bultje wrote:

> On Tue, Apr 12, 2011 at 3:44 AM, Martin Storsjö <martin@martin.st> 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
Ronald Bultje April 13, 2011, 11:36 a.m. | #3 
Hi,

On Wed, Apr 13, 2011 at 2:49 AM, Martin Storsjö <martin@martin.st> wrote:
> On Tue, 12 Apr 2011, Ronald S. Bultje wrote:
>> On Tue, Apr 12, 2011 at 3:44 AM, Martin Storsjö <martin@martin.st> 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
Martin Storsjö April 13, 2011, 12:44 p.m. | #4 
On Wed, 13 Apr 2011, Ronald S. Bultje wrote:

> On Wed, Apr 13, 2011 at 2:49 AM, Martin Storsjö <martin@martin.st> wrote:
> > On Tue, 12 Apr 2011, Ronald S. Bultje wrote:
> >> On Tue, Apr 12, 2011 at 3:44 AM, Martin Storsjö <martin@martin.st> 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

Patch
diff mbox 

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;
 }