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

[Alex Converse]

On Tue, Nov 10, 2009 at 7:13 PM, Baptiste Coudurier
<baptiste.coudurier at gmail.com> wrote:
> On 11/10/2009 03:55 PM, Alex Converse wrote:
>>
>> 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.
>
> I thought we were talking about mp4/mov container here.
>
>>>> 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.
>
> I don't see how the decoding behaviour would be changed, avctx->channels is
> still set the same way (based on chan_config), but through an easier
> mechanism.
>
> IMHO the decoder should behave according to what is in the _bitstream_, ie
> MPEG4AudioConfig specifically, considering extradata is part of the bistream
> of course.
>

The problem is figuring out what type of the seven AAC bitstream
formats is being passed to the decoder, and within those 7 types where
the channel information lies.

In particular the AAC decoder tries to set channel information at init
but sometimes that's impossible.