[FFmpeg-devel] [PATCH] QCELP decoder

[Diego Biurrun]

On Tue, Oct 14, 2008 at 06:59:34PM -0700, Kenan Gillet wrote:
> 
> Here is a summary of the round 4 patch:

some more comments, mostly nits...

> --- libavcodec/qcelpdata.h	(revision 0)
> +++ libavcodec/qcelpdata.h	(revision 0)
> +
> +#ifndef AVCODEC_QCELPDATA_H
> +#define AVCODEC_QCELPDATA_H
> +
> +static const uint16_t qcelp_bits_per_rate[6] = {

You will need to #include at least stdint.h for this file to pass 'make
checkheaders'.  Please test your code with 'make checkheaders'.

> + * YOU WILL NOT SEE ANY mention of a REFERENCE nor an UNIVERSAL frame
> + * in the specs, this is just some internal way of handling the

Split this sentence in two at the comma.

> + * each bit in the input with respect  to a transmission code in the 

nit: two spaces

> +    {19,3},{19,2},{19,1},{19,0}
> +};
> +static const QCELPBitmap qcelp_rate_4thr_bitmap[] = {
> +static const QCELPBitmap qcelp_rate_4thr_bitmap[] = {

4thr?

nit: You left a blank line between the other tables, do it here as well.

> --- libavcodec/qcelpdec.c	(revision 0)
> +++ libavcodec/qcelpdec.c	(revision 0)
> +        // Low-pass filter the LSP frequencies

.

> +        // Check for badly received packets

.

> +        gain[0] =     ga[0];
> +        gain[1] = 0.6*ga[0]+0.4*ga[1];
> +        gain[2] =     ga[1];
> +        gain[3] = 0.2*ga[1]+0.8*ga[2];
> +        gain[4] = 0.8*ga[2]+0.2*ga[3];
> +        gain[5] =     ga[3];
> +        gain[6] = 0.4*ga[3]+0.6*ga[4];
> +        gain[7] =     ga[4];

A few spaces between operators would make this look less cramped.

> + * The reason for this mistake seems to be the fact they forget to mention you

forgot

> + * @param cnd_vector array for the generated scaled codebook vector
> + */
> +static void compute_svector(const QCELPContext *q, const float *gain, float *cdn_vector) {

cdn_vector or cnd_vector?

> + * @param gain array of gain per subframe, each element is between 0.0 and 2.0

per-subframe gain array

> + * @param lag array of lag per subframe, each element is

per-subframe lag array

> + * @param pfrac array of boolean per subframe, 1 if the lag is fractional, 0 otherwise

per-subframe boolean array

> + * @param v_out the output vector of the filter

filter output vector

> + * @param v_in the input vector of the filter

filter input vector

> +/**
> + * Apply pitch synthesis filter and pitch pre-filter to the scaled codebook vector.

I would say 'prefilter', same in other places.

> +            // Compute Gain & Lag for the whole frame

.

gain, lag

> +            memset(q->pfrac, 0, 4 *sizeof(*q->pfrac));
> +            memcpy(q->plag, q->prev_pitch_lag, 4 * sizeof(*q->plag));

memset(q->pfrac,                0, 4 * sizeof(*q->pfrac));
memcpy(q->plag, q->prev_pitch_lag, 4 * sizeof(*q->plag));

> + * @param memory the previous state of the filter

s/the//

> +        av_log(avctx, AV_LOG_ERROR, "frame #%d: unknown framerate, unsupported size: %d\n",
> +        warn_insufficient_frame_quality(avctx, "framerate is 1/8 and first 16 bits are on.");
> +            warn_insufficient_frame_quality(avctx, "wrong data in reserved frame area.");
> +        warn_insufficient_frame_quality(avctx, "sanity check of the codebook gain failed.");
> +        warn_insufficient_frame_quality(avctx, "badly received packets in frame.");
> +        warn_insufficient_frame_quality(avctx, "cannot initialize pitch filter.");

Please capitalize all these messages.

> +        // WIP Adaptive postfilter should be here

adaptive

> +    .long_name = NULL_IF_CONFIG_SMALL("QCLEP / PureVoice"),

QCELP

> --- doc/general.texi	(revision 15618)
> +++ doc/general.texi	(working copy)
> @@ -388,6 +388,7 @@
>  @item Nellymoser ASAO        @tab  X  @tab  X
> + at item QCELP / PureVoice      @tab  X  @tab  X

Ummm, I don't think you have implemented an encoder as well, so just put
an X in the decoder column.

Diego