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

[Baptiste Coudurier]

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 ?

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