[FFmpeg-devel] [PATCH] QCELP decoder

[Kenan Gillet]

On Nov 20, 2008, at 4:03 PM, Reynaldo H. Verdejo Pinochet wrote:

> Hello Kenan
>
>>> qcelp_bits_per_rate does not seem correct here nor does its name  
>>> seem
>>> to match what it contains
>>
>> yes changed back to buf_size.
>>
>> what about changing qcelp_bits_per_rate  to  
>> qcelp_unpacking_bitmaps_per_rate_len
>> because it really is the len of the unpacking bitmaps, or do you have
>> a better suggestion ?
>
> I don't like the name, It doesn't make sence. This is what I had:
>
> typedef enum
> {
>     RATE_UNKNOWN = -2,
>     I_F_Q,             /*!< insufficient frame quality */
>     BLANK,
>     RATE_OCTAVE,
>     RATE_QUARTER,
>     RATE_HALF,
>     RATE_FULL
> } qcelp_packet_rate;
>
> static const uint16_t qcelp_bits_per_rate[]={0, 20, 54, 124, 266};
>
> and then qcelp_bits_per_rate was exactly what its name would made
> you think it was. Can you remind me why did you change that enum's
> order? If that change is avoided this one would not be needed.
>

My original intent for reordering the enum was
so that the value of the rate byte(1st one)
in the frame would correspond to the enum value
0 => BLANK / SILENCE = 0
1 => RATE_OCTAVE = 1
...
4 => RATE_FULL = 4

which allowed to simplify the code to determine the rate.


For info, from the SoC r2373 te enum was:
typedef enum
{
     RATE_FULL   = 0,
     RATE_HALF   = 1,
     RATE_QUARTER= 2,
     RATE_OCTAVE = 3,
     I_F_Q,          /*!< insufficient frame quality */
     BLANK,
     RATE_UNKNOWN
} qcelp_packet_rate;






Concerning the renaming issue, the unpacking used to be done
1bit at a time in the SoC code, and qcelp_bits_per_rate was
used more like the size of the unpacking tables.
But during the review, Michael remarked that the unpacking
could be done by block of bits instead, which would lead to smaller
unpacking table and even maybe some faster code.
see QCELPBitmap and the different associated tables in ffmpeg trunk

During this refactoring, the length of the unpacking tables shrunk and
I should have renamed the table qcelp_bits_per_rate  at that time.

hope it makes my intent clearer.

My next planned patch against SoC is about the unpacking :)

Kenan