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

[Alex Converse]

On Fri, Oct 9, 2009 at 5:00 PM, Baptiste Coudurier
<baptiste.coudurier at gmail.com> wrote:
> 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.
>

Setting it to zero for PCE based configs shouldn't break anything. It
might break some things if we set it to the actual channel count for
PCE based streams. Zero should be ok.