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

[Baptiste Coudurier]

On 10/9/09 1:45 PM, Alex Converse wrote:
> On Fri, Oct 9, 2009 at 4:39 PM, Baptiste Coudurier
> <baptiste.coudurier at gmail.com>  wrote:
>> 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 ?
>>
>
> cfg.channels does not exist without this patch.
>

That's what I thought, so can you please explain in details how adding 
it without modifying aac decoder can have effect on it ? I don't get it.

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