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

[Baptiste Coudurier]

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.

Do you think PCE should be parsed instead of setting chan_config to 0 ?
I think we are ok with setting cfg.channels to 0 if chan_config is set to 0.

Besides, I'm not sure if a decoder internal variable wouldn't be needed 
because avctx->channels could change (adts parser can update 
avctx->channels value, asynchronous decoding would break).

-- 
Baptiste COUDURIER
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
FFmpeg maintainer                                  http://www.ffmpeg.org