[FFmpeg-devel] [PATCH] Create ff_amr_bit_reorder() in a shared amr.h file

[Vitor Sessak]

On 09/08/2010 06:51 PM, Marcelo Galv?o P?voa wrote:
> On 8 September 2010 09:07, Vitor Sessak<vitor1001 at gmail.com>  wrote:
>> On 09/07/2010 12:02 AM, Marcelo Galv?o P?voa wrote:
>>>
>>> Hello,
>>>
>>> I am not sure if it is the best way to do it, maybe it's better to
>>> move the memset outside the function. Unfortunately, I had to convert
>>> several tables in amrnbdata.h from uint8_t to uint16_t because
>>> amrwbdata.h uses the latter.
>>
>> If you split the order tables in two (one for the pulses and one for the
>> rest) can you replace in AMRWB
>>
>> ff_amr_bit_reorder(ctx, size, data, order_mode);
>>
>> by
>>
>> ff_amr_bit_reorder(ctx, size, data, order_mode);
>> ff_amr_bit_reorder(ctx, size2, data + 8, order_mode_pulses);
>>
>> in a way that you can keep uint8_t for the order tables?
>>
>
> I don't know if I understand that correctly but the pulse bits aren't
> all contained after the first byte. For example, for this pulse:
>
> 11, AMR_OF(1, pul_ih[2]), 106, 134, 125, 154, 205, 267, 306, 220,
>                            185, 330, 297,
>
> Some bits would have to be in one table and the rest in another, which
> would break the reordering function.

For this example, you could send data+24 to ff_amr_reorder(), so you 
would subtract all the terms by 24*8 == 96, and it would fit in 8 bits.

But I just found another example:

     11, AMR_OF(1, pul_il[0]), 266, 186, 377, 439, 279, 227, 256, 376,
                               401, 417, 450,

since 450-186 == 264, this field could never fit in 8 bits, no matter 
what one do.

So we have the following options:

1) Convert amrnbdata.h order mode tables to 16-bit like in your patch 
(wastes space)
2) duplicate this code in amrwbdec.c and amrwbdec.c (ugly)
3) Use a preprocessor trick to create both functions (messy).

For the last option, one would move all the code to amr.h (*not* amr.c) 
and put something like in it

#ifdef AMR_USE_16BIT_TABLES
#define TABLE_TYPE uint16_t
#else
#define TABLE_TYPE uint8_t
#endif

static inline void ff_amr_bit_reorder(uint16_t *out, int size,
                                       const uint8_t *data,
                                       const TABLE_TYPE *ord_table)
{
...
}

So the different files might do either

#include "amr.h"

or

#define AMR_USE_16BIT_TABLES
#include "amr.h"
.

I prefer slightly option (3), but if others have a strong opinion for 
another option I will not oppose. Michael, Rob, Ronald?

-Vitor