[FFmpeg-devel] [PATCH] QCELP decoder

[Diego Biurrun]

On Tue, Oct 28, 2008 at 04:47:02PM -0700, Kenan Gillet wrote:
> On Tue, Oct 28, 2008 at 4:43 PM, Kenan Gillet <kenan.gillet at gmail.com> wrote:
> > thanks Diego and Michael for your reviewing.
> >
> > And here is an updated set of patches with a few changes too.
> >
> > round 7 summary:
> > - qcelp-round7-doc-glue.patch.txt has already been oked
> > - grammar / spelling / cosmetics
> > - generalize convolve function used in qcelp_lspf2lpc
> > - remove some multiplication and division in compute_svector
> > - frame unpacking rework
> > - most of the previous comments
> >
> 
> forgot one patch in previous mail :(
> 
> --- libavcodec/qcelpdata.h	(revision 0)
> +++ libavcodec/qcelpdata.h	(revision 0)
> @@ -0,0 +1,541 @@
> +/**
> + * pre-calculated table for hammsinc function
> + * Only half of the tables is needed because of symetry.

table or tables?

syMMetry

> +    uint8_t bitpos; /*!< position of the lowet bit in the value's byte */

loweSt

> +/**
> + * the upper boundary of the clipping, depends on
> + */
> +#define QCELP_CLIP_UPPER_BOUND (8191.75/8192.)

Depends on what?

> --- libavcodec/qcelpdec.c	(revision 0)
> +++ libavcodec/qcelpdec.c	(revision 0)
> @@ -0,0 +1,723 @@
> +/**
> + * Decodes the 10 quantized LSP frequencies from the LSPV/LSP
> + * transmission codes of any framerate and check for badly received packets.

checks

> +/*
> + * Determine the framerate from the frame size and/or the first byte of the frame.
> + *
> + * @return 0 on success, negative error number otherwise.
> + */
> +static int determine_framerate(AVCodecContext *avctx,
> +                               QCELPContext *q,
> +                               const int buf_size,
> +                               uint8_t **buf) {

What rule do you follow when it comes to documenting parameters or not?

> +        warn_insufficient_frame_quality(avctx, "Sanity check of the codebook gain failed.");

Codebook gain sanity check failed.

> --- Changelog	(revision 15692)
> +++ Changelog	(working copy)
> @@ -138,6 +138,7 @@
>  - liba52 wrapper removed
> +- QCELP / PureVoice decoder

This has since changed.

> --- libavcodec/Makefile	(revision 15692)
> +++ libavcodec/Makefile	(working copy)
> @@ -152,6 +152,7 @@
>  OBJS-$(CONFIG_PPM_ENCODER)             += pnmenc.o pnm.o
>  OBJS-$(CONFIG_PTX_DECODER)             += ptx.o
> +OBJS-$(CONFIG_QCELP_DECODER)           += qcelpdec.o qcelp_lsp.o celp_math.o celp_filters.o
>  OBJS-$(CONFIG_QDM2_DECODER)            += qdm2.o mdct.o mpegaudiodec.o mpegaudiodecheader.o mpegaudio.o mpegaudiodata.o

ditto

> --- libavcodec/celp_filters.c	(revision 15692)
> +++ libavcodec/celp_filters.c	(working copy)
> @@ -84,3 +84,24 @@
>  
> +    for(n=0; n<buffer_length; n++)
> +    {

You put the opening brace on the same line in other places.  And the for
expression could use some whitespace.

> +        out[n] = in[n];
> +        for(i=1; i<filter_length; i++)

ditto

> --- libavcodec/celp_math.c	(revision 15692)
> +++ libavcodec/celp_math.c	(working copy)
> @@ -195,3 +195,14 @@
>  
> +    for(i=0; i<length; i++)

Some more whitespace will earn you extra good karma here.

Diego