[FFmpeg-devel] [PATCH] QCELP decoder

[Diego Biurrun]

On Fri, Oct 17, 2008 at 09:46:31AM -0700, Kenan Gillet wrote:
> 
> round 6 summary:
> - no code change from round 5
> - replace svn cp by svn add to make the patch smaller
> - rename qcelp_math.[ch] into celp_math.[ch]

Maybe we can rename some of the acelp_*.[ch] files to celp_*.[ch]
already if it helps you.

> - the patch is now broken into:
> 
>     - qcelp-round6-doc-glue.patch.txt
>     doc and glue code

This patch is OK.

> Let me know if I can do anything to ease you reviewing effort.

The small patches are helping, some of these can be committed before the
others, that should speed things up.

> --- libavcodec/qcelpdata.h	(revision 0)
> +++ libavcodec/qcelpdata.h	(revision 0)
> @@ -0,0 +1,500 @@
> +/**
> + * Unpacking bitmap tables for RATE_FULL

Are these the unpacked tables or the tables used to unpack?

> --- libavcodec/qcelpdec.c	(revision 0)
> +++ libavcodec/qcelpdec.c	(revision 0)
> @@ -0,0 +1,662 @@
> + * @param memory buffer for the previous state of the filter
> + *        - must be able to contain 303 elements
> + *        - the 143 fist elements are from the previous state

fiRst

> +    v_out = memory + 143; // output vector start at memory[143]

startS

> +                if (pfrac[i]) { // If is a fractional lag...

If it is

> +static int decode_scaled_codebook_vector(QCELPContext *q, float cdn_vector[160]) {
> +    float gain[16];

Align the function parameters.


> --- libavcodec/qcelp_lsp.c	(revision 0)
> +++ libavcodec/qcelp_lsp.c	(revision 0)
> @@ -0,0 +1,98 @@
> +/**
> + * Linear convolution of two vectors v1 and [1, cos, 1]
> + *
> + * @param v_out the result of the convolution,
> + *              needs to be able to hold v_in_len + 2 elements
> + * @param v_in the input vector
> + * @param cos

Yes?

> + * @param v_in_len the dimension of v_in assumed to be in  {2,4,6,8,10}

nit: two spaces

> +/**
> + * Reconstructs LPC coefficients from the line spectral pair frequencies.
> + *
> + * TIA/EIA/IS-733 2.4.3.3.5
> + */
> +void qcelp_lspf2lpc(const float *lspf, float *lpc) {
> +    float pa[5],qa[5];
> +    float v1[12], v2[12];
> +    float beta = 0.9883;
> +    int   i;
> +
> +    v1[0] = 0.5;
> +    v1[1] = 0.5;
> +    lsp2poly(pa, lspf, v1, v2, 0);
> +
> +    v1[0] =  0.5;
> +    v1[1] = -0.5;
> +    lsp2poly(qa, lspf, v1, v2, 1);
> +
> +    for (i = 0; i < 5; i++) {
> +        lpc[i] = -(pa[i] + qa[i]) * beta;
> +        beta *= 0.9883;
> +    }
> +    for (i = 5; i < 10; i++) {
> +        lpc[i] = -(pa[9-i] - qa[9-i]) * beta;
> +        beta *= 0.9883;
> +    }
> +}

> Index: libavcodec/celp_math.c
> ===================================================================
> --- libavcodec/celp_math.c	(revision 0)
> +++ libavcodec/celp_math.c	(revision 0)
> @@ -0,0 +1,40 @@
> +
> +/**
> + * returns the dot product.
> + * @param a input data array
> + * @param b input data array
> + * @param length number of elements
> + *
> + * @return dot product = sum of elementwise products
> + */
> +float ff_dot_product(const float* a, const float* b, int length)

I think the Doxygen documentation in the header file is enough.

> --- libavcodec/celp_math.h	(revision 0)
> +++ libavcodec/celp_math.h	(revision 0)
> @@ -0,0 +1,36 @@
> +
> +#ifndef AVCODEC_QCELP_MATH_H
> +#define AVCODEC_QCELP_MATH_H

Use the filename as multiple inclusion guard.

Diego