[FFmpeg-devel] [PATCH] QCELP decoder

Kenan Gillet kenan.gillet
Fri Oct 17 00:38:03 CEST 2008


On Oct 15, 2008, at 3:09 PM, Michael Niedermayer wrote:

> cOn Tue, Oct 14, 2008 at 06:59:34PM -0700, Kenan Gillet wrote:
>> Thank you for all your feedback.
>>
>> Here is a summary of the round 4 patch:
>> - add documentation
>> - grammar / spelling fixes
>> - cosmetics
>> - split lsp2lpc code into its own file qcelp_lsp.c
>> - small optimizations
>> - bug fixes
>>
>> For info, postfilter and final gain control are still missing.
> [...]
>> Index: libavcodec/qcelp.h
>> ===================================================================
>> --- libavcodec/qcelp.h	(revision 0)
>> +++ libavcodec/qcelp.h	(revision 0)
> [...]
>> +#ifndef AVCODEC_QCELP_H
>> +#define AVCODEC_QCELP_H
>> +
>> +/**
>> + * @file qcelpdata.h
>> + * QCELP decoder
>
> hmm

changed.


>
>> + * @author Reynaldo H. Verdejo Pinochet
>> + */
>> +
>> +typedef enum
>> +{
>> +    SILENCE = 0,
>> +    RATE_OCTAVE,
>> +    RATE_QUARTER,
>> +    RATE_HALF,
>> +    RATE_FULL,
>> +    I_F_Q,          /*!< insufficient frame quality */
>> +    RATE_UNKNOWN
>> +} qcelp_packet_rate;
>> +
>
>> +/**
>> + * Reconstructs LPC coefficients from the line spectral pair  
>> frequencies.
>> + *
>> + * TIA/EIA/IS-733 2.4.3.3.5
>> + */
>> +void qcelp_lspf2lpc(const float *lspf, float *lpc);
>
> where is the file that contains this? it seems not in the patch ...
>

Oups, will include it.


>
> [...]
>> +static const QCELPBitmap qcelp_rate_full_bitmap[] = {
>> +    {62,2},{62,1},{62,0},{61,6},{61,5},{61,4},{61,3},{61,2},
>> +    {61,1},{61,0},{60,5},{60,4},{60,3},{60,2},{60,1},{60,0},
>> +    {64,5},{64,4},{64,3},{64,2},{64,1},{64,0},{63,5},{63,4},
>> +    {63,3},{63,2},{63,1},{63,0},{62,6},{62,5},{62,4},{62,3},
>> +    { 0,0},{16,3},{16,2},{16,1},{16,0},{52,0},{48,6},{48,5},
>> +    {48,4},{48,3},{48,2},{48,1},{48,0},{56,2},{56,1},{56,0},
>> +    {33,3},{33,2},{33,1},{33,0},{ 1,0},{17,3},{17,2},{17,1},
>> +    {17,0},{32,6},{32,5},{32,4},{32,3},{32,2},{32,1},{32,0},
>> +    {19,0},{34,6},{34,5},{34,4},{34,3},{34,2},{34,1},{34,0},
>> +    { 2,0},{18,3},{18,2},{18,1},{18,0},{33,6},{33,5},{33,4},
>> +    {49,2},{49,1},{49,0},{57,2},{57,1},{57,0},{35,6},{35,5},
>> +    {35,4},{35,3},{35,2},{35,1},{35,0},{ 3,0},{19,2},{19,1},
>> +    {36,5},{36,4},{36,3},{36,2},{36,1},{36,0},{ 4,0},{20,3},
>> +    {20,2},{20,1},{20,0},{53,0},{49,6},{49,5},{49,4},{49,3},
>> +    {22,2},{22,1},{22,0},{37,6},{37,5},{37,4},{37,3},{37,2},
>> +    {37,1},{37,0},{ 5,0},{21,3},{21,2},{21,1},{21,0},{36,6},
>> +    {39,2},{39,1},{39,0},{ 7,0},{23,2},{23,1},{23,0},{38,6},
>> +    {38,5},{38,4},{38,3},{38,2},{38,1},{38,0},{ 6,0},{22,3},
>> +    {24,0},{54,0},{50,6},{50,5},{50,4},{50,3},{50,2},{50,1},
>> +    {50,0},{58,2},{58,1},{58,0},{39,6},{39,5},{39,4},{39,3},
>> +    { 9,0},{25,3},{25,2},{25,1},{25,0},{40,6},{40,5},{40,4},
>> +    {40,3},{40,2},{40,1},{40,0},{ 8,0},{24,3},{24,2},{24,1},
>> +    {42,3},{42,2},{42,1},{42,0},{10,0},{26,3},{26,2},{26,1},
>> +    {26,0},{41,6},{41,5},{41,4},{41,3},{41,2},{41,1},{41,0},
>> +    {59,1},{59,0},{43,6},{43,5},{43,4},{43,3},{43,2},{43,1},
>> +    {43,0},{11,0},{27,2},{27,1},{27,0},{42,6},{42,5},{42,4},
>> +    {44,1},{44,0},{12,0},{28,3},{28,2},{28,1},{28,0},{55,0},
>> +    {51,6},{51,5},{51,4},{51,3},{51,2},{51,1},{51,0},{59,2},
>> +    {45,5},{45,4},{45,3},{45,2},{45,1},{45,0},{13,0},{29,3},
>> +    {29,2},{29,1},{29,0},{44,6},{44,5},{44,4},{44,3},{44,2},
>> +    {31,2},{31,1},{31,0},{46,6},{46,5},{46,4},{46,3},{46,2},
>> +    {46,1},{46,0},{14,0},{30,3},{30,2},{30,1},{30,0},{45,6},
>> +    {70,1},{70,0},{47,6},{47,5},{47,4},{47,3},{47,2},{47,1},
>> +    {47,0},{15,0}
>> +};
>> +
>> +static const QCELPBitmap qcelp_rate_half_bitmap[] = {
>> +    {62,2},{62,1},{62,0},{61,6},{61,5},{61,4},{61,3},{61,2},
>> +    {61,1},{61,0},{60,5},{60,4},{60,3},{60,2},{60,1},{60,0},
>> +    {64,5},{64,4},{64,3},{64,2},{64,1},{64,0},{63,5},{63,4},
>> +    {63,3},{63,2},{63,1},{63,0},{62,6},{62,5},{62,4},{62,3},
>> +    { 0,0},{16,3},{16,2},{16,1},{16,0},{52,0},{48,6},{48,5},
>> +    {48,4},{48,3},{48,2},{48,1},{48,0},{56,2},{56,1},{56,0},
>> +    {49,5},{49,4},{49,3},{49,2},{49,1},{49,0},{57,2},{57,1},
>> +    {57,0},{32,6},{32,5},{32,4},{32,3},{32,2},{32,1},{32,0},
>> +    {58,1},{58,0},{33,6},{33,5},{33,4},{33,3},{33,2},{33,1},
>> +    {33,0},{ 1,0},{17,3},{17,2},{17,1},{17,0},{53,0},{49,6},
>> +    {34,1},{34,0},{ 2,0},{18,3},{18,2},{18,1},{18,0},{54,0},
>> +    {50,6},{50,5},{50,4},{50,3},{50,2},{50,1},{50,0},{58,2},
>> +    {55,0},{51,6},{51,5},{51,4},{51,3},{51,2},{51,1},{51,0},
>> +    {59,2},{59,1},{59,0},{34,6},{34,5},{34,4},{34,3},{34,2},
>> +    {35,6},{35,5},{35,4},{35,3},{35,2},{35,1},{35,0},{ 3,0},
>> +    {19,3},{19,2},{19,1},{19,0}
>> +};
>> +static const QCELPBitmap qcelp_rate_4thr_bitmap[] = {
>> +    {62,2},{62,1},{62,0},{61,6},{61,5},{61,4},{61,3},{61,2},
>> +    {61,1},{61,0},{60,5},{60,4},{60,3},{60,2},{60,1},{60,0},
>> +    {64,5},{64,4},{64,3},{64,2},{64,1},{64,0},{63,5},{63,4},
>> +    {63,3},{63,2},{63,1},{63,0},{62,6},{62,5},{62,4},{62,3},
>> +    {19,3},{19,2},{19,1},{19,0},{18,3},{18,2},{18,1},{18,0},
>> +    {17,3},{17,2},{17,1},{17,0},{16,3},{16,2},{16,1},{16,0},
>> +    {70,1},{70,0},{20,3},{20,2},{20,1},{20,0}
>> +};
>> +
>> +static const QCELPBitmap qcelp_rate_8thr_bitmap[] = {
>> +    {71,3},{60,0},{61,0},{62,0},{71,2},{63,0},{64,0},{65,0},
>> +    {71,1},{66,0},{67,0},{68,0},{71,0},{69,0},{16,1},{16,0},
>> +    {70,3},{70,2},{70,1},{70,0}
>> +};
>
> i think these would be more readable with offsetof() or some macro  
> using
> that

done, using offset


> [...]
>> +/**
>> + * circular codebook for rate 1 frames
>> + * TIA/EIA/IS-733 2.4.6.1-2
>> + */
>> +static const int16_t qcelp_fullrate_ccodebook[128] = {
>> +     10,  -65,  - 59,  12,  110,   34, -134,  157,
>> +    104,  -84,  -34, -115,   23, -101,    3,   45,
>> +   -101,  -16,  -59,   28,  -45,  134,  -67,   22,
>> +     61,  -29,  226,  -26,  -55, -179,  157,  -51,
>> +   -220,  -93,  -37,   60,  118,   74,  -48,  -95,
>> +   -181,  111,   36,  -52, -215,   78, -112,   39,
>> +    -17,  -47, -223,   19,   12,  -98, -142,  130,
>> +     54, -127,   21,  -12,   39,  -48,   12,  128,
>> +      6, -167,   82, -102,  -79,   55,  -44,   48,
>> +    -20,  -53,    8,  -61,   11,  -70, -157, -168,
>> +     20,  -56,  -74,   78,   33,  -63, -173,   -2,
>> +    -75,  -53, -146,   77,   66,  -29,    9,  -75,
>> +     65,  119,  -43,   76,  233,   98,  125, -156,
>> +    -27,   78,   -9,  170,  176,  143, -148,   -7,
>> +     27, -136,    5,   27,   18,  139,  204,    7,
>> +   -184, -197,   52,   -3,   78, -189,    8,  -65
>> +};
>
>> +#define QCELP_FULLRATE_CODEBOOK(i) qcelp_fullrate_ccodebook[i] * .01
>
> i think a constant is clearer than a opaque macro(i)

done


>
> [...]
>> +typedef struct {
>> +    GetBitContext     gb;
>> +    qcelp_packet_rate framerate;
>> +
>> +// beginning of unpacked data
>> +    uint8_t           cbsign[16];
>> +    uint8_t           cbgain[16];
>> +    uint8_t           cindex[16];
>> +    uint8_t           plag[4];
>> +    uint8_t           pfrac[4];
>> +    uint8_t           pgain[4];
>> +    uint8_t           lspv[10];               /*!< LSP for  
>> RATE_OCTAVE, LSPV for other rates */
>> +    uint8_t           reserved;               /*!< on all but rate  
>> 1/2 packets */
>
>> +    uint8_t           cbseed;                 /*!< only in rate  
>> 1/8 packets */
>
> unused
>

removed

>
>> +// end of unpacked data
>> +
>> +    uint8_t           erasure_count;
>> +    uint8_t           octave_count;           /*!< count the  
>> consecutive RATE_OCTAVE frames */
>> +    float             prev_lspf[10];
>> +    float             predictor_lspf[10];     /*!< LSP predictor,
>> +                                                  only use for  
>> RATE_OCTAVE and I_F_Q */
>> +    float             pitch_synthesis_filter_mem[143];
>> +    float             pitch_pre_filter_mem[143];
>> +    float             formant_mem[10];
>> +    float             last_codebook_gain;
>> +    int               prev_g1[2];
>> +    int               prev_framerate;
>> +    float             prev_pitch_gain[4];
>> +    uint8_t           prev_pitch_lag[4];
>> +    uint16_t          first16bits;
>
>> +    int               frame_num;
>
> unused

replaced by frame_number in AVCodecContext

>
>
>
> [...]
>> +/**
>> + * Decodes the 10 quantized LSP frequencies from the LSPV/LSP
>> + * transmission codes of any framerate and check for badly  
>> received packets.
>> + *
>> + * @return 0 on success, -1 if the packet is badly received
>> + *
>> + * TIA/EIA/IS-733 2.4.3.2.6.2-2, 2.4.8.7.3
>> + */
>> +static int decode_lspf(QCELPContext *q, float *lspf) {
>> +    int i;
>> +    float tmp_lspf;
>> +
>> +    if (q->framerate == RATE_OCTAVE ||
>> +        q->framerate == I_F_Q) {
>> +        float smooth;
>> +        float erasure_coeff = 1.0;
>> +        const float *predictors = (q->prev_framerate !=  
>> RATE_OCTAVE ||
>> +                                   q->prev_framerate != I_F_Q ? q- 
>> >prev_lspf
>> +                                                              : q- 
>> >predictor_lspf);
>> +
>> +        if (q->framerate == RATE_OCTAVE) {
>> +            q->octave_count++;
>> +            smooth = (q->octave_count < 10 ? .875 : 0.1);
>> +        } else {
>> +            if (q->erasure_count > 1)
>> +                erasure_coeff = (q->erasure_count < 4 ? 0.9 : 0.7);
>> +            smooth = 0.125;
>> +        }
>> +
>> +        for (i = 0; i < 10; i++) {
>> +            lspf[i] = (i + 1) / 11.;
>> +            if (q->framerate == RATE_OCTAVE) {
>> +                q->predictor_lspf[i]  =
>> +                             lspf[i] += (q->lspv[i] ?   
>> QCELP_LSP_SPREAD_FACTOR
>> +                                                    : - 
>> QCELP_LSP_SPREAD_FACTOR)
>> +                                      + (predictors[i] - lspf[i])  
>> * QCELP_LSP_OCTAVE_PREDICTOR;
>> +            } else {
>> +                q->predictor_lspf[i] = (predictors[i] - lspf[i]) *  
>> erasure_coeff;
>> +                lspf[i] += QCELP_LSP_OCTAVE_PREDICTOR * q- 
>> >predictor_lspf[i];
>> +            }
>> +        }
>> +
>
>> +        // 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;
>
> i think these can be simplified with FFMIN/FFMAX

done


>> +
>> +        // Low-pass filter the LSP frequencies
>> +        interpolate_lspf(lspf, smooth, lspf, q->prev_lspf);
>> +    } else {
>> +        q->octave_count = 0;
>> +
>> +        tmp_lspf = 0.;
>> +        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;
>> +        }
>> +
>
>> +        // Check for badly received packets
>> +        if (q->framerate != RATE_QUARTER) {
>> +            if (lspf[9] <= .66 || lspf[9] >= .985)
>> +                return -1;
>> +            for (i = 4; i < 10; i++)
>> +                if (FFABS(lspf[i] - lspf[i-4]) < .0931)
>> +                    return -1;
>> +        } else {
>> +            if (lspf[9] <= .70 || lspf[9] >=  .97)
>> +                return -1;
>> +            for (i = 3; i < 10; i++)
>> +                if (FFABS(lspf[i] - lspf[i-2]) < .08)
>> +                    return -1;
>> +        }
>
> i think this would be slightly more readable as
> if(q->framerate == RATE_QUARTER)
> else

done


>> +    }
>> +    return 0;
>> +}
>> +
>> +/**
>> + * 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;
>
> this may be more readable as
> int something=
> for(i=0; i<something; i++

done


>> +
>> +            g1[i] = 4 * q->cbgain[i];
>> +            if (q->framerate == RATE_FULL && !((i+1) & 3)) {
>> +                g1[i] += av_clip((g1[i-1] + g1[i-2] + g1[i-3]) /  
>> 3, 6, 38) - 6;
>> +                if (g1[i] > 60)
>> +                    g1[i] = 60;
>> +            }
>> +
>> +            gain[i] = qcelp_g12ga[g1[i]];
>> +
>> +            if (q->cbsign[i]) {
>> +                gain[i] = -gain[i];
>> +                q->cindex[i] = (q->cindex[i]-89) & 127;
>> +            }
>> +        }
>> +
>> +        q->prev_g1[0] = g1[i-2];
>> +        q->prev_g1[1] = g1[i-1];
>> +        q->last_codebook_gain = gain[i-1];
>> +
>> +        break;
>
>> +    case RATE_QUARTER:
>> +        for (i = 0; i < 5; i++) {
>> +            g1[i] = 4 * q->cbgain[i];
>> +
>> +            if (i>0 && FFABS(g1[i] - g1[i-1]) > 40)
>> +                return -1;
>> +            if (i >= 2 && FFABS(g1[i] - 2*g1[i-1] + g1[i-2]) > 48)
>> +                return -1;
>> +            ga[i] = qcelp_g12ga[g1[i]];
>> +        }
>> +
>> +        q->prev_g1[0] = g1[3];
>> +        q->prev_g1[1] = g1[4];
>> +        q->last_codebook_gain = ga[4];
>
> this code looks like it can be factorized with the other 2cases

done


> [...]
>> +static float compute_subframe_energy(const float *vector, const  
>> int subframeno) {
>> +    int   i;
>> +    float energy = 0;
>> +
>> +    vector += 40 * subframeno;
>> +
>> +    for (i = 0; i < 40; i++)
>> +        energy += vector[i] * vector[i];
>> +
>> +    return energy;
>> +}
>
> IMHO its cleaner not to add subframeno inside the function.
> a simple dot_product(vector, len) should be more readable
> also such a function has a much better chance to be shareable
> between codec, and especially when things like that are optimized its
> better if it isnt needed to be redone for each len and other obscure  
> variant

done, I svn cp libavcodec/acelp_math.c into libavcodec/qcelp_math.c
and made it ff_dot_product.
or is there a better place for that ?
Should it be in a different patch ?

>
>> +
>> +/**
>> + * Apply generic gain control.
>> + *
>> + * @param in vector to control gain of
>> + * @param out gain-controlled 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) {
>> +    int   i;
>> +    float scalefactors[4];
>> +
>> +    for (i = 0; i < 4; i++)
>> +        scalefactors[i] = sqrt(compute_subframe_energy(in , i) /
>> +                               compute_subframe_energy(out, i));
>
> is there something that gurantees "energy" wont be 0 ?

unfortunately, no
changed

>> +
>> +    for (i = 0; i < 160; i++)
>> +        out[i] = scalefactors[i/40] * out[i];
>
> the /40 is avoidable
> the scalefactors array unneeded

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_out the output vector of the filter
>> + * @param v_in the input 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) {
>
> the != 0.0 is superflous

done


>> +            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]);
>> +                    }
>> +                } else
>> +                    hamm_tmp = index < 0 ? memory[index] :  
>> v_out[index];
>
> ive looked a little more at the code and i see no problem with  
> removing
> the checks, also i dont see why yet another memcpy() would be needed.
> we need to carefully review if the existing memcpies can be  
> avoided ...

done


> [...]
>> +/**
>> + * Apply pitch synthesis 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
>> + *
>> + * @return 0 if everything goes well, -1 if frame should be erased
>> + */
>> +static int apply_pitch_filters(QCELPContext *q, float *cdn_vector) {
>> +    int     i;
>> +    float   gain[4];
>> +    float v_buf[160];
>> +
>> +    if (q->framerate == RATE_FULL ||
>> +        q->framerate == RATE_HALF ||
>> +       (q->framerate == I_F_Q && (q->prev_framerate == RATE_FULL ||
>> +                                  q->prev_framerate ==  
>> RATE_HALF))) {
>> +
>> +        if (q->framerate == RATE_FULL ||
>> +            q->framerate == RATE_HALF) {
>> +
>> +            // Compute Gain & Lag for the whole frame
>> +            for (i = 0; i < 4; i++) {
>> +                gain[i] = q->plag[i] ? (q->pgain[i] + 1) / 4.0 :  
>> 0.0;
>> +
>> +                q->plag[i] += 16;
>> +
>> +                if (q->pfrac[i] && q->plag[i] >= 140) return -1;
>> +            }
>
>> +            memcpy(q->prev_pitch_lag,  q->plag,   4 * sizeof(*q- 
>> >plag));
>
> are the many spaces intended?

no, removed


>> +        } else {
>> +            gain[3] = q->erasure_count < 3 ? 0.9 - 0.3 * (q- 
>> >erasure_count - 1)
>> +                                       : 0.0;
>> +            for (i = 0; i < 4; i++)
>> +                gain[i] = FFMIN(q->prev_pitch_gain[i], gain[3]);
>> +
>
>> +            memset(q->pfrac, 0, 4 *sizeof(*q->pfrac));
>
> ive not checked but shouldnt this be zero alraedy?

no, IFQ can happen after for instance, a RATE_FULL frame has been  
unpacked,
the code does not zero the unpacked data when an IFQ occurs, and then  
pfrac
might be != 0



> [...]
>> +/**
>> + * formant synthesis filter
>> + *
>> + * TIA/EIA/IS-733 2.4.3.1 (NOOOOT)
>> + *
>> + * @param vector in/out vector of the formant filter
>> + * @param lpc linear predictive coding coefficients
>> + * @param memory the previous state of the filter
>> + */
>> +static void do_formant(float *vector, const float *lpc, float  
>> *memory) {
>> +    int i,j;
>> +
>> +    for (i = 0; i < 40;i++)
>> +        for (j = 0; j < 10; j++)
>> +            vector[i] += lpc[j] * (i > j ? vector[     i-j-1]
>> +                                         : memory[10 + i-j-1]);
>
> please get rid of this check as well, you are alraedy doing a memcpy  
> of the
> array.
> also IMHO do_formant() is a very poor name
> see ff_acelp_lp_synthesis_filter()
>
> also this belongs in a file seperate from the qcelp decoder so it  
> can be
> shared with other decoders.

done
I svn cp libavcodec/acelp_filtes.[ch] into libavcodec/celp_filters.[ch]
and made it ff_dot_product

Should it be in a different patch ?


> [...]
>> +    case SILENCE:
>> +        av_log_missing_feature(avctx, "Blank frame", 1);
>> +    default:
>> +        q->framerate = I_F_Q;
>
> is it intended that SILENCE sets IFQ ?

The SILENCE handling  is not yet implemented.
I have no sample to test it on, and was planning to implement it later  
on.
Do you want me me to work on it for the next round ?



>
> [...]
>
>> +static int qcelp_decode_frame(AVCodecContext *avctx,
>
> qcelp_ prefixes shouldnt be needed for static functions unless you/ 
> Reynaldo
> want them

changed



>
>> +                              void *data, int *data_size,
>> +                              uint8_t *buf, const int buf_size) {
>> +    QCELPContext      *q = avctx->priv_data;
>> +    const QCELPBitmap *order = NULL;
>> +    float             *outbuffer = data;
>> +    int               i, bits;
>> +    float             qtzd_lspf[10], lpc[10];
>> +    float             *formant_mem;
>> +
>> +    if (determine_framerate(avctx, q, buf_size, &buf) < 0) {
>> +        av_log(avctx, AV_LOG_ERROR, "frame #%d: unknown framerate,  
>> unsupported size: %d\n",
>> +               q->frame_num++, buf_size);
>> +        return -1;
>> +    }
>> +
>> +    if (q->framerate == RATE_OCTAVE &&
>> +       (q->first16bits = (buf[0] << 8) + buf[1]) == 0xFFFF) {
>> +        warn_insufficient_frame_quality(avctx, "framerate is 1/8  
>> and first 16 bits are on.");
>> +        goto erasure;
>> +    }
>> +
>> +    order = qcelp_unpacking_bitmpap_per_rate[q->framerate];
>> +    if (order) {
>> +        bits = qcelp_bits_per_rate[q->framerate];
>> +
>> +        init_get_bits(&q->gb, buf, 8 * buf_size);
>> +
>
>> +        memset(q->cbsign, 0, 72);
>
> iam not sure if C gurantees that these variables will be packed in  
> 72 byte

done, now uses sizeof(uint8_t)

> [...]
>
>> +    if (q->framerate == I_F_Q) {
>> +erasure:
>> +        q->framerate = I_F_Q;
>> +        q->erasure_count++;
>> +        decode_scaled_codebook_vector(q, outbuffer);
>> +        decode_lspf(q, qtzd_lspf);
>> +        apply_pitch_filters(q, outbuffer);
>> +    }
>> +
>> +    formant_mem = q->formant_mem;
>> +    for (i = 0; i < 4; i++) {
>> +        interpolate_lpc(q, qtzd_lspf, lpc, i);
>> +
>> +        do_formant(outbuffer + i * 40, lpc, formant_mem);
>> +
>> +        // WIP Adaptive postfilter should be here
>> +
>> +        formant_mem = outbuffer + i * 40 + 30;
>> +    }
>> +    memcpy(q->formant_mem, outbuffer + 150, 10 * sizeof(float));
>> +
>
>> +    if (q->framerate != I_F_Q)
>> +        q->erasure_count = 0;
>
> cant this be a else above?

it can. done


>> +
>> +    for (i = 0; i < 160; i++)
>> +        outbuffer[i] = av_clipf(outbuffer[i], -8192., 8191.75) /  
>> 8192.;
>
> cant this scale factor be merged into some tables?

by merging it in qcelp_g12ga and testing on the samples
from samples.mplayerhq.hu and others:

I use 'ffmpeg --i sample_file sample_file.wav' to create the output
I use md5sum to check if the output had changed
  and run 'tiny_psnr old_file new_file' if it had.

here are the results:

101.mov: audio same as previous build

V6_text.mov: audio mismatchs from previous build
     stddev:    1.26 PSNR: 46.10 bytes: 11371244/ 11371244

blue_earth.mov: audio same as previous build

codeblue-interrupt01.mov: audio same as previous build

codeblue-interrupt06.mov: audio same as previous build

codeblue-plot08motor.mov: audio same as previous build

h263.mov: audio same as previous build

h263.trashed.mov: audio same as previous build

installfect_export.mov: audio mismatchs from previous build
     stddev:    0.07 PSNR: 70.67 bytes: 10448044/ 10448044

oldepisode.mov: audio mismatchs from previous build
     stddev:    0.21 PSNR: 61.41 bytes:  4588844/  4588844

schlangenmannliten.mov: audio same as previous build

surge-1-16-B-Qclp.mov: audio mismatchs from previous build
     stddev:    0.72 PSNR: 50.92 bytes:  1156524/  1156524

8fe3b9dcd6d7e6db3375521c5c1b_Video_041606_002.3g2: audio mismatchs  
from previous build
     stddev:    1.95 PSNR: 42.30 bytes:   721004/   721004

Test3gpp-mp4a.3g2: audio same as previous build

c8wwz2m0.3g2: audio same as previous build

qtaudio-qcelp-problem.3g2: audio mismatchs from previous build
     stddev:   11.93 PSNR: 26.59 bytes:   208044/   208044

test.3g2: audio same as previous build

tube.3g2: audio same as previous build

zg3dx2d6.3g2: audio mismatchs from previous build
     stddev:   17.77 PSNR: 23.12 bytes:   486444/   486444

vidoo_MP4_audio_Qcelp13k.k3g: audio mismatchs from previous build
     stddev:   10.94 PSNR: 27.34 bytes:   743724/   743724


does it look reasonable ? is at acceptable?


thanks for your review

Kenan






More information about the ffmpeg-devel mailing list