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

[Baptiste Coudurier]

On 11/10/2009 12:10 PM, Thilo Borgmann wrote:
> 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)

I think it's safer to put it at the end of the struct.

>>
>>> 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?

What do we loose ? It seems that chan_config value is still set, and it 
avoids duplicating this if everywhere.

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