[FFmpeg-devel] [PATCH] QCELP decoder

[Vitor Sessak]

On Sat, Oct 4, 2008 at 12:48 AM, Kenan Gillet <kenan.gillet at gmail.com> wrote:
> Hi,
>
> here is a round 2 of the patch based on feedback from Vitor and Diego.
> It includes:
> - some spelling/grammar fixes,
> - some cosmetics,
> - changes to output float instead of int,
> - bug fixes uncovered from the change of output,
> - improvements to the pitch pre/synthesis filters.

Thanks, some more comments:

>+typedef struct {
>+    uint16_t x;
>+    uint16_t y;
>+} qcelp_vector;
>+
>+/**
>+ * LSP Vector quantization tables in x*10000 form
>+ *
>+ * TIA/EIA/IS-733 tables 2.4.3.2.6.3-1 through 2.4.3.2.6.3-5
>+ */
>+
>+static const qcelp_vector qcelp_lspvq1[64] = {
>+{ 327, 118},{ 919, 111},{ 427, 440},{1327, 185},

It think it would be better to just have

static const int16_t qcelp_lspvq1[64][2] = {

This make it easier for someone who is not familiar with the code to
understand it (one less struct to learn) without making it more
complex.

+static const double qcelp_rnd_fir_coefs[22] = {
>+  0          , -1.344519e-1, 1.735384e-2, -6.905826e-2,
>+  2.434368e-2, -8.210701e-2, 3.041388e-2, -9.251384e-2,
>+  3.501983e-2, -9.918777e-2, 3.749518e-2,  8.985137e-1,
>+  3.749518e-2, -9.918777e-2, 3.501983e-2, -9.251384e-2,
>+  3.041388e-2, -8.210701e-2, 2.434368e-2, -6.905826e-2,
>+  1.735384e-2, -1.344519e-1
>+}; /*!< Start reading from [1]. */

Can this be just a vector of floats or is the double preision really needed?

>+static int apply_pitch_filters(QCELPContext *q, float *cdn_vector, float *ppf_vector) {
>+    int     i;
>+    uint8_t *pgain, *plag, *pfrac;
>+    float   gain[4];
>+    int     lag[4];
>+
>+    if (q->rate == RATE_FULL ||
>+        q->rate == RATE_HALF ||
>+       (q->rate == I_F_Q && (q->prev_frame_rate == RATE_FULL ||
>+                             q->prev_frame_rate == RATE_HALF))) {
>+
>+        pfrac = q->data + QCELP_PFRAC0_POS;
>+
>+        if (q->rate == RATE_FULL ||
>+            q->rate == RATE_HALF) {
>+            pgain = q->data + QCELP_PGAIN0_POS;
>+            plag  = q->data + QCELP_PLAG0_POS;
>+
>+            // Compute Gain & Lag for the whole frame
>+            for (i = 0; i < 4; i++) {
>+                gain[i] = plag[i] ? (pgain[i] + 1) / 4.0 : 0.0;
>+
>+                lag[i] = plag[i] + 16;
>+
>+                if (pfrac[i] && lag[i] >= 140) return -1;
>+            }
>+            memcpy(q->prev_pitch_lag,  lag,  sizeof(q->prev_pitch_lag));

The lag[] buffer could be just a pointer, it is always the copy of
something. Also this function does a lot of memcpy'ing, I wonder if
they are all really needed.

-Vitor