[FFmpeg-devel] [PATCH] Parsing ALS object type in MPEG-4

[Alex Converse]

On Tue, Nov 10, 2009 at 5:36 PM, Baptiste Coudurier
<baptiste.coudurier at gmail.com> wrote:
> On 11/10/2009 02:06 PM, Alex Converse wrote:
>>
>> On Tue, Nov 10, 2009 at 4:29 PM, Baptiste Coudurier
>> <baptiste.coudurier at gmail.com> ?wrote:
>>>
>>> On 11/10/2009 12:10 PM, Thilo Borgmann wrote:
>>>>
>>>> Baptiste Coudurier schrieb:
>>>>>
>>>>> On 10/11/2009 03:04 AM, Thilo Borgmann wrote:
>>>>>>
>>>>>> Baptiste Coudurier schrieb:
>>>>>>>
>>>>>>> On 10/9/09 1:48 PM, Alex Converse wrote:
>>>>>>>>
>>>>>>>> On Sun, Aug 23, 2009 at 3:51 PM, Thilo Borgmann
>>>>>>>> <thilo.borgmann at googlemail.com> ? ? ?wrote:
>>>>>>>>>
>> [...]
>>>>>>
>>>>>> Index: libavformat/mov.c
>>>>>> ===================================================================
>>>>>> --- libavformat/mov.c ? ?(revision 20011)
>>>>>> +++ libavformat/mov.c ? ?(working copy)
>>>>>> @@ -434,9 +434,13 @@
>>>>>> ? ? ? ? ? ? ? ? ? MPEG4AudioConfig cfg;
>>>>>> ? ? ? ? ? ? ? ? ? ff_mpeg4audio_get_config(&cfg, st->codec->extradata,
>>>>>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?st->codec->extradata_size);
>>>>>> + ? ? ? ? ? ? ? ?if (cfg.chan_config) {
>>>>>> ? ? ? ? ? ? ? ? ? if (cfg.chan_config> ? ? 7)
>>>>>> ? ? ? ? ? ? ? ? ? ? ? return -1;
>>>>>> ? ? ? ? ? ? ? ? ? st->codec->channels =
>>>>>> ff_mpeg4audio_channels[cfg.chan_config];
>>>>>> + ? ? ? ? ? ? ? ?} else {
>>>>>> + ? ? ? ? ? ? ? ? ? ?st->codec->channels = cfg.channels;
>>>>>> + ? ? ? ? ? ? ? ?}
>>>>>
>>>>> By always setting cfg.channels I wanted to avoid the
>>>>> if (cfg.chan_config) ...
>>>>>
>>>>> That's ok though.
>>>>>
>>>>
>>>> Hm I tried to but cannot really follow the arguing about setting
>>>> cfg.channels. Do you mean to always set
>>>>
>>>> st->codec->channels = cfg.channels;
>>>>
>>>> ? This would set it to 0 (for PCE streams == !ALS) but don't we loose
>>>> something from ff_mpeg4audio_channes[] in that case? And are PCE streams
>>>> always given if ALS is not given?
>>>
>>> What do we loose ? It seems that chan_config value is still set, and it
>>> avoids duplicating this if everywhere.
>>
>> Has it been confirmed that this doesn't break any of the ways that AAC
>> or MP4A can be muxed into mov-realted formats that we support?
>
> Why would it be ?
>
>> Particularly:
>> MP4 objectTypeIndication 0x66/0x67
>
> 0x68 as well ?
> Currently, CODEC_ID_AAC will always use 0x40.
> MPEG2 AAC might needs a different CODEC_ID value.
>

There used to be a different CODEC_ID for mpeg-2 aac and they were
combined. 0x67 only supports mpeg-2 aac but the real difference is
that it uses a different style header than 0x40. You can have an
mpeg-4 aac file with an ADIF header if it's standalone, you can also
have a standalone ADTS file with either mpeg-2 or mpeg-4 aac.

>> MP4 objectTypeIndication 0x40 AOT_LC channel_config = 0
>> MP4 objectTypeIndication 0x40 AOT_LC channel_config> ?0
>> MP4 objectTypeIndication 0x40 AOT_L1/L2/L3
>> MP4 objectTypeIndication 0x40 AOT_PS (Old mp3on4)
>> AAC in mov?
>
> I don't understand what you mean.
> Do you mean that chan_config is used when muxing ?
> chan_config is located in extradata is supplied by the encoder or is kept
> when stream copying. The muxer only set mp4 object type id based on codec,
> and uses 0x40 for CODEC_ID_AAC.
>
> Currently channels are set according to chan_config using
> ff_mpeg4audio_channels[], if chan_config is 0 channels are set to 0, and
> decoders will init channels parsing pce or anything else.

What worries me is that the AAC decoder seems somewhat brittle in this
regard. There are places in aac.c where we check "if
(avccontext->channels > 0)". I think they are all in the
extradata_size == 0 path. There are many ways to wrap AAC. We support
more than one and should be careful not to break the less common
cases. Unfortunately we don't have any regression testing on AAC
demuxing/decoding.