[FFmpeg-devel] [PATCH] code to parse mpeg4audio extradata

[Baptiste Coudurier]

Hi,

Michael Niedermayer wrote:
> Hi
> 
> On Mon, Mar 31, 2008 at 11:00:31AM +0200, Baptiste Coudurier wrote:
>> Hi,
>>
>> Michael Niedermayer wrote:
>>> On Mon, Mar 31, 2008 at 12:41:08AM +0200, Baptiste Coudurier wrote:
>>>> Hi,
>>>>
>>>> Michael Niedermayer wrote:
>>>>> On Wed, Mar 19, 2008 at 04:54:03PM +0100, Baptiste Coudurier wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Here is a patch that adds parsing of mpeg4audio extradata, and retrieve
>>>>>> needed infos for decoders (only mp3on4 atm, but will be needed for aac),
>>>>>> and will be used by mov demuxer ideally to correctly compute sample rate
>>>>>> and channels.
>>>>>> Also extract common mpeg4audio code from aac_parser.c.
>>>>> [...]
>>>>>
>>>>>> Index: libavcodec/mpegaudiodec.c
>>>>>> ===================================================================
>>>>>> --- libavcodec/mpegaudiodec.c	(revision 12503)
>>>>>> +++ libavcodec/mpegaudiodec.c	(working copy)
>>>>>> @@ -2486,9 +2486,11 @@
>>>>>>  #endif /* CONFIG_MP3ADU_DECODER */
>>>>>>  
>>>>>>  #ifdef CONFIG_MP3ON4_DECODER
>>>>>> +
>>>>>> +#include "mpeg4audio.h"
>>>>>> +
>>>>>>  /* Next 3 arrays are indexed by channel config number (passed via codecdata) */
>>>>>> -static int mp3Frames[16] = {0,1,1,2,3,3,4,5,2};   /* number of mp3 decoder instances */
>>>>>> -static int mp3Channels[16] = {0,1,2,3,4,5,6,8,4}; /* total output channels */
>>>>>> +static const uint8_t mp3_frames[8] = {0,1,1,2,3,3,4,5}; /* number of mp3 decoder instances */
>>>>> seperate commit and why does the 2 at the end disapear?
>>>>>
>>>> Yes, of course. I'll post an updated patch for mp3on4 decoder anyway.
>>>>
>>>> It disappears because this last configuration is not mentioned anymore
>>>> in published specs. It was in old draft, and I expect/hope nobody ever
>>>> used it.
>>>>
>>>>> [...]
>>>>>
>>>>  > This either needs a function ptr as a argument for parsing codec
>>>> specific stuff
>>>>> or return some ptr / bitindex to where that is stored. Later is more flexible
>>>>> for example with a binary decoder which expects a pointer to this stuff.
>>>>>
>>>> I like the idea of bitindex. Updated patch attached.
>>> [...]
>>>> +    if (c->ext_object_type != 5) {
>>>> +        int bits_left = buf_size*8 - specific_config_bitindex;
>>>> +        for (; bits_left > 15; bits_left--) {
>>>> +            if (show_bits(&gb, 11) == 0x2b7) { // sync extension
>>>> +                get_bits(&gb, 11);
>>>> +                if ((c->ext_object_type = get_bits(&gb, 5)) == 31)
>>>> +                    c->ext_object_type = 32 + get_bits(&gb, 6);
>>> That code occurs 3 times, maybe it would be more readable in a fuction.
>>> The same is true for the sample_rate code below.
>>>
>>>> +                if (c->ext_object_type == 5)
>>>> +                    if ((c->sbr = get_bits1(&gb)) == 1) {
>>>> +                        if ((c->ext_sampling_index = get_bits(&gb, 4)) == 0x0f)
>>>> +                            c->ext_sample_rate = get_bits(&gb, 24);
>>>> +                        else
>>>> +                            c->ext_sample_rate = ff_mpeg4audio_sample_rates[c->ext_sampling_index];
>>>> +                    }
>>> [...]
>>>> +typedef struct {
>>>> +    int object_type;
>>>> +    int sampling_index;
>>> Is the *sampling_index really needed? Iam asking because it would complicate
>>> a factored out function to read the sample_rate.
>>>
>> You're right, I factored out, updated patch attached.
> 
> looks ok
> 

Great, applied.

-- 
Baptiste COUDURIER                              GnuPG Key Id: 0x5C1ABAAA
SMARTJOG SAS                                     http://www.smartjog.com
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
Phone: +33 1 49966312