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

[Thilo Borgmann]

Baptiste Coudurier schrieb:
> On 10/11/2009 03:04 AM, Thilo Borgmann wrote:
>> Baptiste Coudurier schrieb:
>>> On 10/9/09 1:48 PM, Alex Converse wrote:
>>>> On Sun, Aug 23, 2009 at 3:51 PM, Thilo Borgmann
>>>> <thilo.borgmann at googlemail.com>   wrote:
>>>>> 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;
>>>>
>>>> Can we put "channels" directly below "chan_config" without API/ABI
>>>> breakage? If so I'd like to try to keep related thing together.
>>>
>>> We could since this struct is not part of the public API, though since
>>> libavformat uses libavcodec, it is not safe.
>>>
>>
>> Patch updated.
>>
>>
>>> This reminds me, I think avcodec_register_all and av_register_all should
>>> put requirements for libavcodec and libavutil versions respectively, to
>>> avoid shared libraries problems, because soname only contains major
>>> version and API changes only update minor version.
>>>
>>
>> [...]
>>
>> Index: libavcodec/mpeg4audio.h
>> ===================================================================
>> --- libavcodec/mpeg4audio.h    (revision 20011)
>> +++ libavcodec/mpeg4audio.h    (working copy)
>> @@ -31,6 +31,7 @@
>>       int sampling_index;
>>       int sample_rate;
>>       int chan_config;
>> +    int channels;
>>       int sbr; //<  -1 implicit, 1 presence
>>       int ext_object_type;
>>       int ext_sampling_index;
> 
> Forgot to change that ?

Hm, no... looking good locally as well as in the patch I had sent (rev7)

> 
>> Index: libavformat/mov.c
>> ===================================================================
>> --- libavformat/mov.c    (revision 20011)
>> +++ 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;
>> +                }
> 
> By always setting cfg.channels I wanted to avoid the
> if (cfg.chan_config) ...
> 
> That's ok though.
> 

Hm I tried to but cannot really follow the arguing about setting
cfg.channels. Do you mean to always set

st->codec->channels = cfg.channels;

? This would set it to 0 (for PCE streams == !ALS) but don't we loose
something from ff_mpeg4audio_channes[] in that case? And are PCE streams
always given if ALS is not given?

-Thilo