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

[Baptiste Coudurier]

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.

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