[FFmpeg-devel] [PATCH] Add AMR-NB decoder, next try

[Vitor Sessak]

Ronald S. Bultje wrote:
> Hi,
>
> On Sun, Jan 3, 2010 at 12:01 PM, Vitor Sessak <vitor1001 at gmail.com> wrote:
>   
>> Hi, the SoC AMR decoder is in a pretty nice shape and have already gone
>> through a few review rounds, so I'll give my try in getting it committed.
>>     
>
> +/**
> + * @file libavcodec/amrnbdata.h
> + * AMR narrowband data and definitions
> + */
> [..]
>
> Any defines that can be in the C should be in the C (as Diego said).
>   

ok, will do in the next patch.

> +/**
> + * Bit-order table type
> + */
> +typedef struct AMROrder {
> +    uint8_t index;                ///< index in (uint16_t *)AMRNBFrame
> +    uint8_t bit;                  ///< bit index, LSB=0, MSB=15
> +} AMROrder;
>
> OK? So let's look at what we have below:
>
> +static const AMROrder order_MODE_4k75[95] = {
>
> ??? Is this defining a scripting language for the buildup of the frame
> here? No wonder the patch is huge. Is there a particular reason we're
> doing it? Certainly doesn't seem faster to me...
>   

The problem is that the bitstream format is nonsensical. The individual 
bits of each bitstream field are spread across the input stream, so this 
table gives the offset and the bit-index of where every bit in the input 
bitstream belong. Collin wrote me to say he has improved it (at least 
making the tables smaller, which is non-trivial) and he'll commit it to 
soc soon, so I'll wait for it before sending a new patch.

> +    uint16_t sid_vector;       ///< index of SID LSF reference vector
> +    uint16_t sid_energy;       ///< SID frame energy
>
> What's an SID?
>   

Anyway, these two fields are only used for the DTX mode, which is 
unsupported by this code ATM, so if nobody objects I'll remove they from 
the patch (but leave them in the soc tree).

> +// The following order* tables are used to convert AMR frame parameters to and
> +// from a bitstream. See 3GPP TS 26.101 for more information.
>
> /**Doxy*/
>   

Do you know a decent way of saying to Doxygen that a comment applies to 
a series of declarations? When I tested their "block" syntax, I was 
disappointed with the generated output.

> +#define AMR_BIT(field, bit)                  {offsetof(AMRNBFrame,
> field) >> 1, bit}
> [..]
> + AMR_SENERGY(   2), AMR_SENERGY(   1), AMR_SENERGY(   0)
> [..]
>
> (for all this stuff, see the comment above, I'm not sure this is the
> most optimal solution for reading a bitstream.)
>
> +static const AMROrder * const amr_unpacking_bitmaps_per_mode[9] = {
>
> 9 -> N_MODES?  

Indeed, agree.

> +// LSF tables
>
> /** Doxy */  

Same as before.

> +static const int16_t lsf_3_3_MODE_5k15[128][4] = {
> +{  419,  163,  -30, -262},{ -455, -789,-1430, -721},{ 1006,  664,  269,   25},
> [..]
>
> <space><space><space><space>{ .. },<space> { .. },<space><etc>
>   

"<space><space><space><space>{ .. },<space> { .. },<space><etc>" generate too long lines. I'll use intead "{ .. },<space> { .. },<space><etc>"


> +#define LSF_R_FAC          (8000.0/32768.0) ///< LSF residual tables to Hertz
> +#define MIN_LSF_SPACING             50.0488 ///< Ensures stability of
> LPC filter
>
> I suppose this means that all tables are in this virtually random
> scale as well? Is that so that the tables above (lsf_mean etc.) fit in
> int16_t instead of float, or is there another reason for it? If it's
> not for int16_t <> float, maybe we want to convert this to the final
> scale used for LSF/P -> LPC conversion?  

I think the spec gives the tables as integers, so this scale was chosen 
when AMR was designed and values were truncated...

> +/** Double version of ff_weighted_vector_sumf() */
> +void weighted_vector_sumd(double *out, const double *in_a, const double *in_b,
> +                          double weight_coeff_a, double weight_coeff_b,
> +                          int length)
>
> Should be shared between others, so not here but in lsp.c or so.  

Even if no other file use it?

> +static av_cold int amrnb_decode_init(AVCodecContext *avctx)
> [..]
> +    for (i = 0; i < LP_FILTER_ORDER; i++) {
> +        p->prev_lsp_sub4[i] = lsp_sub4_init[i] * 1000 / (float)(1 << 15);
> +        p->lsf_avg[i]       =
> +        p->lsf_q[3][i]      = lsp_avg_init[i]         / (float)(1 << 15);
>
> I'm not a fan of this, easily confused for a coding bug.
>   

I agree, will change in the next patch.

> +static void lsf2lsp(const float *lsf, double *lsp)
> [..]
> +{
> +    int i;
> +
> +    for (i = 0; i < LP_FILTER_ORDER; i++)
> +        lsp[i] = cos(2.0 * M_PI / 8000.0 * lsf[i]);
> +}
>
> So this is sort of what I though, the scale of the LSFs is random and
> is converted only just before LSP->LPC conversion. I'm OK with it if
> that really makes the tables smaller (i.e. int16_t vs. float), but
> only then.
>
> Also, I'm wondering if we shouldn't do lsfs in double also, since
> we're artificially adding precision here (= noise).

The reason lsp are calculated using doubles is because they will be 
passed to ff_acelp_lspd2lpc(), that takes doubles as input, but I've 
tried to keep it use as minimal as possible (to avoid polluting the cache).

-Vitor