[FFmpeg-soc] [soc]: r5570 - in amr: amrnbdata.h amrnbdec.c

[Vitor Sessak]

Michael Niedermayer wrote:
> On Sat, Jan 09, 2010 at 09:29:47AM -0500, Vitor Sessak wrote:
>> Michael Niedermayer wrote:
>>> On Fri, Jan 08, 2010 at 10:19:54PM -0500, Vitor Sessak wrote:
>>>> cmcq wrote:
>>>>> Author: cmcq
>>>>> Date: Fri Jan  8 18:16:25 2010
>>>>> New Revision: 5570
>>>>> Log:
>>>>> Save about 1KB of space using a field-to-bit lookup instead of 
>>>>> bit-to-field.
>>>>> Modified:
>>>>>    amr/amrnbdata.h
>>>>>    amr/amrnbdec.c
>>>>> Modified: amr/amrnbdata.h
>>>>> ==============================================================================
>>>>> --- amr/amrnbdata.h	Fri Jan  8 18:13:05 2010	(r5569)
>>>>> +++ amr/amrnbdata.h	Fri Jan  8 18:16:25 2010	(r5570)
>>>>> @@ -68,15 +68,6 @@ enum Mode {
>>>>>  #define LP_FILTER_ORDER 10        ///< linear predictive coding filter 
>>>>> order
>>>> [...]
>>>>
>>>>> +// Each field in AMRNBFrame is stored as:
>>>>> +//   one byte of 16 * index in AMRNBFrame relative to previous field
>>>>> +//               + number of bits in the field
>>>>> +//   then, one byte for each bit of the field (from most-significant to 
>>>>> least)
>>>>> +//         of the position of that bit in the AMR frame.
>>>>> +static const uint8_t order_MODE_4k75[] = {
>>>>> +    0x28,   7,   6,   5,   4,   3,   2,   1,   0,
>>>>> +    0x18,  15,  14,  13,  12,  11,  10,   9,   8,
>>>>> +    0x17,  51,  35,  34,  50,  33,  49,  32,
>>>>> +    0x38,  23,  22,  21,  20,  19,  18,  43,  42,
>>>> [...]
>>>>
>>>>>      if (mode <= MODE_DTX) {
>>>>>          uint16_t *data = (uint16_t *)&p->frame;
>>>>> -        const AMROrder *order = amr_unpacking_bitmaps_per_mode[mode];
>>>>> -        int i;
>>>>> +        const uint8_t *order = amr_unpacking_bitmaps_per_mode[mode];
>>>>> +        int field_header; // 16 * relative field index + number of 
>>>>> field bits
>>>>>           memset(&p->frame, 0, sizeof(AMRNBFrame));
>>>>> -        for (i = 0; i < mode_bits[mode]; i++)
>>>>> -            data[order[i].index] += get_bits1(&gb) << order[i].bit;
>>>>> +        buf++;
>>>>> +        while ((field_header = *order++)) {
>>>>> +            int field = 0;
>>>>> +            data += field_header >> 4;
>>>>> +            field_header &= 0xf;
>>>>> +            while (field_header--) {
>>>>> +               int bit = *order++;
>>>>> +               field <<= 1;
>>>>> +               field |= buf[bit >> 3] >> (bit & 7) & 1;
>>>>> +            }
>>>>> +            *data = field;
>>>>> +        }
>>>> I think that this is only guaranteed to work if you declare the 
>>>> AMRNBFrame struct as packed, since the spec allows the compiler to add 
>>>> some padding space inside structs otherwise.
>>> the struct contains only uint16_t fields, so this would seem odd to me if
>>> a compiler did it but iam not saying no compiler does ...
>> Is there any disadvantage in using "packed"?
> 
> dunno, benchmark

Unsurprisingly, the .o is unchanged if I add "__attribute__((packed))" 
to the struct. I'm not really fan neither of adding gnu-extensions to 
the code or of having a code with undefined behavior according to the 
C99 standard...

-Vitor