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

[Alex Converse]

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.