[FFmpeg-devel] [PATCH] QCELP decoder

[Diego Biurrun]

On Sun, Oct 05, 2008 at 09:49:59AM -0700, Kenan Gillet wrote:
> thank you for your reviewing. Here is a round3 of he decoder.

Build system part is OK, documentation and changelog updates are
missing.

> --- libavcodec/qcelpdata.h	(revision 0)
> +++ libavcodec/qcelpdata.h	(revision 0)
> @@ -0,0 +1,398 @@
> + * lspv[10]  60    61 62 63 64 65 66 67 68 69 (LSP for RATE_OCTAVE, LSPV for other rate)

I think you need to say 'rateS' here.

> + * What follows are the reference frame slices. Each tuple will be mapped
> + * to a QCELPBitmap showing the location of each bit in the input with respect
> + * to a transmission code in the 'universal frame'.

nit: needlessly long line

> +/**
> + * LSP Vector quantization tables in x*10000 form

vector

> --- libavcodec/qcelpdec.c	(revision 0)
> +++ libavcodec/qcelpdec.c	(revision 0)
> @@ -0,0 +1,812 @@
> +    uint8_t           lspv[10];               /*!< LSP for RATE_OCTAVE, LSPV for other rate */

rateS, see above

> +/**
> +* Verify the samplerate and the number of channels.
> +* Initialize the speech codec to the specs.

spec or specs?

And I think you mean "according to" instead of "to".

> +static int qcelp_decode_init(AVCodecContext *avctx) {
> +    QCELPContext *q = avctx->priv_data;
> +    int          i;

I think there is no need to align the variable names here.

> +    for (i = 0; i < 10; i++) {
> +        q->prev_lspf[i] = (i + 1) / 11.;
> +    }

useless {}

> +/**
> + * Decodes the 10 quantized LSP frequencies from the LSPV/LSP
> + * transmission codes of any framerate and check for badly
> + * received packets.

nit: This can be shortened to two lines.

> +    } else if (q->framerate == I_F_Q) {
> +        predictor = QCELP_LSP_OCTAVE_PREDICTOR * -QCELP_LSP_SPREAD_FACTOR;
> +        if (q->erasure_count > 1) {
> +            predictor *= (q->erasure_count < 4 ? 0.9 : 0.7);
> +        }
> +        for (i = 0; i < 10; i++) {
> +            lspf[i] = (i + 1) / 11. + predictor;
> +        }

more pointless {}

There are more cases below.  In FFmpeg generally the form without {} is
preferred.

> +static int decode_gain_and_index(QCELPContext  *q, float *gain) {
> +    int           i, g1[16];
> +    float         ga[16], gain_memory, smooth_coef;

strange alignment

> +        for (i = 0; i < 16; i++) {
> +            if (q->framerate == RATE_HALF && i>=4) break;

I dislike putting the statement right after the if instead of on a new
line, it makes the code harder to read.

> + * Computes energy of the subframeno-ith subvector. These values are

the energy

> + * @param vector vector from which to measure the subframe's energy

vector to measure the subframe energy of

There should be better ways to say this.

> + * @param q if not null, apply harcoded coefficient infinite impulse response filter

harDcoded

> + * @param in vector to control gain off

of

> + * @param out gain-controled output vector

controlled

> + * @param v_in the input vector of the filter
> + * @param v_out the output vector of the filter
> + */
> +static void do_pitchfilter(float *memory,
> +                           const float gain[4], const uint8_t *lag, const uint8_t pfrac[4],
> +                           float v_out[160], const float v_in[160]) {

The order of the last two parameters is swapped wrt the function signature.

> +    float hamm_tmp;

hamming?

> + * @param q the context
> + * @param cdn_vector the scaled codebook vector
> + * @param ppf_vector array where the result of the filter will be stored
> + *
> + * @return 0 if everything goes well, -1 if frame should be erased
> + */
> +static int apply_pitch_filters(QCELPContext *q, float *cdn_vector) {

ppf_vector?

> + * @param cos

?

> +void interpolate_lpc(QCELPContext *q, const float *curr_lspf, float *lpc, const int subframe_num) {

Sometimes you break long function declarations, sometimes you do not.

> +/**
> + * formant synthesis filter
> + *
> + * TIA/EIA/IS-733 2.4.3.1 (NOOOOT)
> + *
> + * @param vector in/out vector of the formant filter
> + */
> +static void do_formant(float *vector, const float *lpc, float *memory) {

missing parameter documentation

> +        av_log(avctx, AV_LOG_WARNING,
> +               "Framerate byte is missing, guessing the framerate from packet size.\n");

from the

> + * @param cnd_vector array where to put the generated scaled codebook vector

array for the generated scaled codebook vector

> + * @return 0  on successful, -1 if the frame must be erased

on success

> +        warn_insufficient_frame_quality(avctx, "cannot initiliaze pitch filter.");

initialize

Quickly running your files through a spellchecker will likely find more
of these..

Diego