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

[Baptiste Coudurier]

On 10/9/09 1:27 PM, Alex Converse wrote:
> On Fri, Oct 9, 2009 at 4:14 PM, Baptiste Coudurier
> <baptiste.coudurier at gmail.com>  wrote:
>> On 10/9/09 1:00 PM, Alex Converse wrote:
>>>
>>> On Fri, Oct 9, 2009 at 3:55 PM, Baptiste Coudurier
>>> <baptiste.coudurier at gmail.com>    wrote:
>>>>
>>>> Hi guys,
>>>>
>>>> On 10/9/09 11:34 AM, Alex Converse wrote:
>>>>>
>>>>> On Fri, Oct 9, 2009 at 2:12 PM, Thilo Borgmann
>>>>> <thilo.borgmann at googlemail.com>      wrote:
>>>>>>
>>>>>> Alex Converse schrieb:
>>>>>>>
>>>>>>> On Fri, Oct 9, 2009 at 10:30 AM, Thilo Borgmann
>>>>>>> <thilo.borgmann at googlemail.com>      wrote:
>>>>>>>>
>>>>>>>> Thilo Borgmann schrieb:
>>>>>>>>>
>>>>>>>>> Alex Converse schrieb:
>>>>>>>>>>
>>>>>>>>>> On Fri, Sep 18, 2009 at 6:42 PM, Baptiste Coudurier
>>>>>>>>>> <baptiste.coudurier at gmail.com>      wrote:
>>>>>>>>>>>
>>>>>>>>>>> Hi,
>>>>>>>>>>>
>>>>>>>>>>> On 09/18/2009 03:35 PM, Thilo Borgmann wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> Alex Converse schrieb:
>>>>>>>>>>>>>
>>>>>>>>>>>>> On Sun, Aug 23, 2009 at 3:51 PM, Thilo
>>>>>>>>>>>>> Borgmann<thilo.borgmann at googlemail.com>        wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Revision 6 attached (rev. 5 skipped...)
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> [...]
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Index: libavcodec/mpeg4audio.h
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> ===================================================================
>>>>>>>>>>>>>> --- libavcodec/mpeg4audio.h     (revision 19689)
>>>>>>>>>>>>>> +++ libavcodec/mpeg4audio.h     (working copy)
>>>>>>>>>>>>>> @@ -36,6 +36,7 @@
>>>>>>>>>>>>>>      int ext_sampling_index;
>>>>>>>>>>>>>>      int ext_sample_rate;
>>>>>>>>>>>>>>      int ext_chan_config;
>>>>>>>>>>>>>> +    int channels;
>>>>>>>>>>>>>>   } MPEG4AudioConfig;
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>   extern const int ff_mpeg4audio_sample_rates[16];
>>>>>>>>>>>>>> Index: libavformat/mov.c
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> ===================================================================
>>>>>>>>>>>>>> --- libavformat/mov.c   (revision 19689)
>>>>>>>>>>>>>> +++ 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;
>>>>>>>>>>>>>> +                }
>>>>>>>>>>>>>>                  if (cfg.object_type == 29&&
>>>>>>>>>>>>>>   cfg.sampling_index<        3) //
>>>>>>>>>>>>>> old mp3on4
>>>>>>>>>>>>>>                      st->codec->sample_rate =
>>>>>>>>>>>>>> ff_mpa_freq_tab[cfg.sampling_index];
>>>>>>>>>>>>>>                  else
>>>>>>>>>>>>>
>>>>>>>>>>>>> The rest of this seems OK but Rob and Baptiste are the
>>>>>>>>>>>>> maintainers
>>>>>>>>>>>>> here.
>>>>>>>>>>>
>>>>>>>>>>> Maybe we should always set ->channels in mpeg4audio_get_config,
>>>>>>>>>>> that
>>>>>>>>>>> would
>>>>>>>>>>> simplify the code everywhere else. What do you think ?
>>>>>>>>>>
>>>>>>>>>> In principle that seems fine as long as it doesn't break muxing or
>>>>>>>>>> decoding files with (those awful) PCEs. In practice this probably
>>>>>>>>>> means adding yet more PCE code to mpeg4audio.[ch] since it doesn't
>>>>>>>>>> look like there is any way to adapt ff_copy_pce_data (mpeg4audio.c)
>>>>>>>>>> or
>>>>>>>>>> decode_pce (aac.c) to this purpose.
>>>>>>>>>>
>>>>>>>>>> I would also accept having this as an inevitable goal and leaving
>>>>>>>>>> it
>>>>>>>>>> on a todo list for a while. This whole multichannel business in
>>>>>>>>>> MPEG
>>>>>>>>>> 4
>>>>>>>>>> audio was dreadfully thought out but there is nothing we can do
>>>>>>>>>> about
>>>>>>>>>> it now.
>>>>>>>>>
>>>>>>>>> So........ I should move these "st->codec->channels = ..." into
>>>>>>>>> ff_mpeg4audio_get_config() ?
>>>>>>>>
>>>>>>>> ping
>>>>>>>
>>>>>>> However you want to do it is fine as far as I'm concerned. I don't
>>>>>>> want to make refactoring stuff in AAC a prereq for landing this.
>>>>>>
>>>>>> Thus if altering aac and may be others is a prereq for moving set
>>>>>> ->channels into ff_mpeg4audio_get_config() I might not be able to do
>>>>>> this.
>>>>>>
>>>>>> So I would suggest to apply the patch as is, if the rest is ok.
>>>>>
>>>>> If Rob and Baptiste are ok with it then go ahead and apply.
>>>>
>>>> I was more thinking of always setting cfg.channels, to simplify, maybe
>>>> make
>>>> mpeg4audio_get_config return -1 in case of error.
>>>>
>>>
>>> As I said earlier always setting cfg.channels is going to require some
>>> major changes in aac.
>>
>> Hummm, really ? Even if chan_config is still set as it is ?
>
> Yes, unless we are ok with setting cfg.channels to zero for PCE based
> configurations.

Currently, aac decoder does not seem to use cfg.channels, can setting it 
(and keeping chan_config set as it was) have effect on decoder ?

If cfg.chan_config is set to 0, PCE should always be present, right ?
I think the chan_config == 0 (and therefore cfg.channels == 0) case can 
be kept as is for now with no incidence.

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