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

[Alex Converse]

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.

> If cfg.chan_config is set to 0, PCE should always be present, right ?

Yes

> I think the chan_config == 0 (and therefore cfg.channels == 0) case can be
> kept as is for now with no incidence.
>