[FFmpeg-devel] [PATCH] QCELP decoder

Kenan Gillet kenan.gillet
Thu Oct 9 21:54:17 CEST 2008


Hi,
On Oct 8, 2008, at 5:46 PM, Michael Niedermayer wrote:

> On Sun, Oct 05, 2008 at 09:49:59AM -0700, Kenan Gillet wrote:
>> Hi,
>> thank you for your reviewing. Here is a round3 of he decoder.
>> - the parser has been dropped.
>> - the unpacking has been revamped.
>> - some code has been simplify & factorize.
>> - some useless memcpy has been removed.
>
> [...]
>
>> +#define QCELP_FULLRATE_CODEBOOK(i) qcelp_fullrate_ccodebook[i] /  
>> 100.0
>
> * .001 is faster than / 100.0

done

> [...]
>> +/**
>> + * Decodes the 10 quantized LSP frequencies from the LSPV/LSP
>> + * transmission codes of any framerate and check for badly
>> + * received packets.
>> + *
>> + * TIA/EIA/IS-733 2.4.3.2.6.2-2, 2.4.8.7.3
>> + */
>> +static int decode_lspf(QCELPContext *q, float *lspf) {
>
> the return value should be documented

done


>> +    int i;
>> +    float predictor, tmp_lspf;
>> +
>> +    if (q->framerate == RATE_OCTAVE) {
>> +        q->octave_count++;
>> +
>> +        for (i = 0; i < 10; i++) {
>> +            lspf[i] = (i + 1) / 11.
>> +                    + (q->lspv[i] ?  QCELP_LSP_SPREAD_FACTOR *  
>> QCELP_LSP_OCTAVE_PREDICTOR
>> +                                  : -QCELP_LSP_SPREAD_FACTOR *  
>> QCELP_LSP_OCTAVE_PREDICTOR);
>> +        }
>> +
>> +        // Check the stability of the LSP frequencies.
>> +        if (lspf[0] < QCELP_LSP_SPREAD_FACTOR)
>> +            lspf[0] = QCELP_LSP_SPREAD_FACTOR;
>> +        for (i = 1; i < 10; i++) {
>> +            if (lspf[i] < lspf[i-1] + QCELP_LSP_SPREAD_FACTOR)
>> +                lspf[i] = lspf[i-1] + QCELP_LSP_SPREAD_FACTOR;
>> +        }
>> +        if (lspf[9] > 1.0 - QCELP_LSP_SPREAD_FACTOR)
>> +            lspf[9] = 1.0 - QCELP_LSP_SPREAD_FACTOR;
>> +        for (i = 9; i > 0; i--) {
>> +            if (lspf[i-1] > lspf[i] - QCELP_LSP_SPREAD_FACTOR)
>> +                lspf[i-1] = lspf[i] - QCELP_LSP_SPREAD_FACTOR;
>> +        }
>
> this stuff has to be fixed before the patch can be commited, and yes
> i would give you a hint how if i knew what is intended.

I am working on it. Unfortunately, I do not have any background in audio
filtering so I am trying to make sense of the specs:
http://www.3gpp2.org/Public_html/Specs/C.S0020-0with3Gcover.pdf

This particular part is on page 48, in section
'2.4.3.3.1 Converting the LSP Transmission Codes to LSP Frequencies'

any help would be appreciated :)



>> +
>> +        // Low-pass filter the LSP frequencies
>> +        if (q->octave_count < 10) {
>> +            interpolate_lspf(lspf, 1 - 0.125, lspf, q->prev_lspf);
>> +        } else {
>> +            interpolate_lspf(lspf, 1 - 0.9, lspf, q->prev_lspf);
>> +        }
>> +
>> +    } 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;
>> +        }
>> +
>> +        // Low-pass filter the LSP frequencies
>> +        interpolate_lspf(lspf, 1 - 0.875, lspf, q->prev_lspf);
>> +    } else {
>> +        q->octave_count = 0;
>> +
>> +        tmp_lspf = 0.;
>
>> +        for (i = 0; i < 10 ; i++) {
>> +            lspf[i]   =
>> +            tmp_lspf += qcelp_lspvq[i / 2][q->lspv[i / 2]][i &  
>> 1] / 10000.0;
>> +        }
>
> i would write:
> for (i = 0; i < 5 ; i++) {
>    lspf[2*i+0] = tmp_lspf += qcelp_lspvq[i][q->lspv[i]][0] * 0.0001;
>    lspf[2*i+1] = tmp_lspf += qcelp_lspvq[i][q->lspv[i]][1] * 0.0001;
> }
> but its a little nitpicking ...

done


> [...]
>> +/**
>> + * Converts codebook transmission codes to GAIN and INDEX.
>> + *
>> + * TIA/EIA/IS-733 2.4.6.2
>> + */
>> +static int decode_gain_and_index(QCELPContext  *q, float *gain) {
>> +    int           i, g1[16];
>> +    float         ga[16], gain_memory, smooth_coef;
>> +
>> +    switch (q->framerate) {
>> +    case RATE_FULL:
>> +    case RATE_HALF:
>> +        for (i = 0; i < 16; i++) {
>> +            if (q->framerate == RATE_HALF && i>=4) break;
>> +
>> +            g1[i] = 4 * q->cbgain[i];
>
>> +            if (q->framerate == RATE_FULL && i > 0 && !((i+1) &  
>> 3)) {
>
> the i>0 check is redundant

done


> [...]
>> +        // Provide smoothing of the unvoiced excitation energy.
>> +        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[7] = 0.4*ga[3]+0.6*ga[4];
>> +        gain[7] =     ga[4];
>
> you didnt mean 7 twice, did you?

no,definitely not :)
changed


> btw, is there some reference sw or reference bitstreams?
> if so patch acceptance requires the decoder to decode the bitstreams
> sufficiently correct (that is if some hard numbers are required for
> conformance then at least that well) ...
> errors like the above would be caught by such tests ...

I don't know of any but I'll do some more research.


> [...]
>> +        cbseed = q->first16bits;
>> +        for (i = 0; i < 160; i++) {
>> +            cbseed = 521 * cbseed + 259;
>> +            cdn_vector[i] = gain[i/20]
>> +                          * QCELP_SQRT1887 / 32768.0 *  
>> (int16_t)cbseed;
>> +        }
>
> division is a rather expensive operation ...
> 2 loops would avoid the /20 and (QCELP_SQRT1887 / 32768.0) would  
> ensure that
> the constants are combined at compile time

done


> [...]
>> +/**
>> + * Apply generic gain control.
>> + *
>> + * @param q if not null, apply harcoded coefficient infinite  
>> impulse response filter
>> + * @param in vector to control gain off
>> + * @param out gain-controled output vector
>> + *
>> + * TIA/EIA/IS-733 2.4.8.3-4/5, 2.4.8.6
>> + */
>> +static void apply_gain_ctrl(const float *in, float *out) {
>
> param q?
> i see no q ...

done


> [...]
>> +/**
>> + * Apply filter in pitch-subframe steps.
>> + *
>> + * @param memory a buffer for the previous state of the filter
>> + * @param gain array of gain per subframe, each element is between  
>> 0.0 and 2.0
>> + * @param lag array of lag per subframe, each element is
>> + *            between 16 and 143 if its corresponding pfrac is 0,
>> + *            between 16 and 139 otherwise
>> + * @param pfrac array of boolean per subframe, 1 if the lag is  
>> fractional, 0 otherwise
>> + * @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]) {
>> +    int   i, j, k, index;
>> +    float hamm_tmp;
>> +
>> +    memory += 143; // Lookup must be done starting at the end of  
>> the buffer.
>> +
>> +    for (i = 0; i < 4; i++) {
>> +        if (gain[i] != 0.0) {
>> +            index = 40 * i - lag[i];
>> +            for (k = 0; k < 40; k++) {
>> +                if (pfrac[i]) { // If is a fractional lag...
>> +                    hamm_tmp = 0.0;
>
>> +                    for (j = -4; j < 4; j++) {
>> +                        hamm_tmp += qcelp_hammsinc_table[j + 4]
>> +                                  * (index + j < 0 ? memory[index  
>> + j] : v_out [index + j]);
>> +                    }
>
> having a check in the innermost loop to switch between
> 2 arrays is not a good idea. the code should be changed so the
> data is in the same array if possible

what about:
                     for (j = -4; j < 4; j++) {
                         if (index + j >= 0)
                             break;
                         hamm_tmp += qcelp_hammsinc_table[j + 4] *  
memory[index + j];
                     }
                     for (; j < 4; j++) {
                         hamm_tmp += qcelp_hammsinc_table[j + 4] *  
v_out [index + j];
                     }


also qcelp_hammsinc_table is symetric and could be reduce to 4 elements,
but i am not sure if it worth the extra complexity.


>> +                } else {
>> +                    hamm_tmp = index < 0 ? memory[index] :  
>> v_out[index];
>> +                }
>> +                v_out[40 * i + k] = v_in[40 * i + k] + gain[i] *  
>> hamm_tmp;
>> +                index++;
>
>> +            }
>> +        }
>> +        else {
>> +          memcpy(v_out + 40 * i, v_in + 40 * i, 40 * sizeof(float));
>> +        }
>
> funny indention

done




>> +    }
>> +    memcpy(memory - 143, v_out + 17, 143 * sizeof(float));
>> +}
>> +
>
>> +/**
>> + * Apply pitch synthetis filter and pitch pre-filter to the scaled  
>> codebook vector.
>> + * TIA/EIA/IS-733 2.4.5.2
>> + *
>> + * @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) {
>
> this function has 2 params, 3 are documented ...

removed


> [...]
>> +/**
>> + * formant synthesis filter
>> + *
>> + * TIA/EIA/IS-733 2.4.3.1 (NOOOOT)
>
> NOOOOT ?
>

don't know either, removed.





More information about the ffmpeg-devel mailing list