[FFmpeg-devel] [PATCH] G.729A (now fixed-point) decoder

Michael Niedermayer michaelni
Sun Mar 23 20:00:32 CET 2008


On Thu, Mar 20, 2008 at 10:12:54PM +0600, Vladimir Voroshilov wrote:
> Hi, Michael.
> 
> On Thu, Mar 20, 2008 at 8:29 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
> > On Sun, Mar 16, 2008 at 12:24:58AM +0600, Vladimir Voroshilov wrote:
> >
> >  > +Per-vector results (PASS means no hearable differences, FAILED means hearable artefacts):
> >  > +algthm  : PASS
> >  > +erasure : PASS
> >  > +fixed   : PASS
> >  > +lsp     : PASS
> >
> >  > +overflow: FAIL
> >
> >  fix it!
> 
> "FAIL" here means 39 PSNR vs 65-88 in other tests.
> Searching for error still in progress, though.

just keep in mind this has to be fixed before it will hit svn!
Also i like to have binary identical output or an explanation why precissely
it is not.


[...]
> >  [...]
> >  > +    int16_t pred_energ_q[4];    ///< (Q10) past quantized energies
> >
> >  energY
> 
> Was undestood and fixed as "(Q10) past quantized energy"

s/pred_energ_q/quant_energy/


[...]
> +#define PITCH_MIN 20
> +#define PITCH_MAX 143

s/PITCH_/PITCH_LAG_/


[...]
> +#define P1_BITS 8             ///< zdaptive codebook index for first subframe (size in bits)
                                      ^^^^^^^^
no en. word


[...]
> +    int subframe_idx;           ///< subframe index (for debugging)

unused remove it!


[...]
> +/* 4.2.2 */
> +#define GAMMA_N 18022 //0.55 in Q15
> +#define GAMMA_D 22938 //0.70 in Q15

s/N/NUM/
s/D/DEN/
s/GAMMA/FORMANT_PP_FACTOR/


> +#define GAMMA_T 26214 //0.80 in Q15

TILT_FACTOR


[...]
> +#define Q12_BASE 4096.0
> +#define Q13_BASE 8192.0
> +#define Q15_BASE 32768.0

unused (and remove all other unused things as well


[...]
> +/**
> + * GA codebook (3.9.2)
> + */

Do you know what GA stands for? hint: gain
Write it in the comment!


[...]

> +/**
> + * MA predictor (3.2.4)
> + */

do you know what "MA" stands for?
I dont, but you should write it in the comment! Its more usefull
than to repeat the abbreviation of the variable name.


[...]
> +/**
> + * \brief divide two fixed point numbers
> + * \param num numenator
> + * \param denom denumenator
> + * \param base base to scale result to
> + *
> + * \return result of division scaled to given base
> + */
> +int l_div(int num, int denom, uint8_t base)
> +{
> +    int diff, sig;
> +
> +    if(!num)
> +        return 0;
> +
> +    sig = (num ^ denom) >> 31;
> +
> +    num = FFABS(num);
> +    denom = FFABS(denom);
> +
> +    diff = 26 - av_log2(num);
> +    assert(diff >= 0 && base >= 0);
> +
> +    num   <<= FFMIN(base, diff);
> +    denom >>= FFMAX(base, diff) - diff;
> +
> +    assert(denom); //division by zero or base too large (overflow)
> +

> +    if(sig)
> +        return -num/denom;
> +    else
> +        return num/denom;

if(sig)
    num= -num;
return num/denom;

you can also get rid of the >> 31
and use if(sig<0)

and the assert(denom) is useless, it will kill the prog with a div by
zero anyway, this isnt float stuff with infinity.


[...]
> +/**
> + * \brief Decoding of the adaptive-codebook vector (4.1.3)
> + * \param pitch_delay_int pitch delay, integer part
> + * \param pitch_delay_frac pitch delay, fraction part [-1, 0, 1]
> + * \param ac_v [out] (Q0) buffer to store decoded vector into
> + * \param subframe_size length of subframe
> + */
> +static void g729_decode_ac_vector(int pitch_delay_int, int pitch_delay_frac, int16_t* ac_v, int subframe_size)
> +{

Maybe this should rather be caled interpolate_excitation() with the
meaningless spec name just in a comment?


> +    int n, i;
> +    int v, tmp;
> +
> +    //Make sure that pitch_delay_frac will be always positive
> +    pitch_delay_frac =- pitch_delay_frac;
> +    if(pitch_delay_frac < 0)
> +    {
> +        pitch_delay_frac += 3;
> +        pitch_delay_int++;
> +    }
> +
> +    //t [0, 1, 2]
> +    //k [PITCH_MIN-1; PITCH_MAX]
> +    for(n=0; n<subframe_size; n++)
> +    {
> +        /* 3.7.1, Equation 40 */
> +        v=0;
> +        for(i=0; i<10; i++)
> +        {
> +            /*  R(x):=ac_v[-k+x] */

> +            tmp = ac_v[n - pitch_delay_int - i    ] * interp_filter[i][    pitch_delay_frac];
> +            v = av_clip(v + tmp, INT_MIN >> 1, INT_MAX >> 1); //v += R(n-i)*interp_filter(t+3i)

v+= ac_v[n - pitch_delay_int - i    ] * interp_filter[i][    pitch_delay_frac];
v=  av_clip(v, INT_MIN >> 1, INT_MAX >> 1); //v += R(n-i)*interp_filter(t+3i)


[...]
> +/**
> + * \brief fixed codebook vector modification if delay is less than 40 (4.1.4 and 3.8)

really? it seems you apply it always?


[...]
> +
> +/**
> + * \brief Decoding of the adaptive codebook gain (4.1.5 and 3.9.1)
> + * \param ctx private data structure
> + * \param ga_cb_index GA gain codebook index (stage 2)
> + * \param gb_cb_index GB gain codebook (stage 2)
> + * \param fc_v (Q13) fixed-codebook vector
> + * \param pred_energ_q [in/out] (Q10) past quantized energies
> + * \param subframe_size length of subframe
> + *
> + * \return (Q1) quantized adaptive-codebook gain (gain code)
> + */
> +static int16_t g729_get_gain_code(int ga_cb_index, int gb_cb_index, const int16_t* fc_v, int16_t* pred_energ_q, int subframe_size)
> +{

This predicts the gain and then "adds" the correction from the bitstream.
maybe that should be split in 2 functions



> +    int i;
> +    int cb1_sum; // Q12
> +    int energy;
> +    int exp;
> +
> +    /* 3.9.1, Equation 66 */
> +    energy = sum_of_squares(fc_v, subframe_size, 0, 0);
> +
> +    /*
> +      energy=mean_energy-E
> +      mean_energy=30dB
> +      E is calculated in 3.9.1 Equation 66
> +
> +      (energy is in Q26)
> +      energy = 30 - 10 * log10(energy / (2^26 * subframe_size))
> +      =30 - 10 * log2(energy / (2^26 * subframe_size)) / log2(10)
> +      =30 - 10*log2(energy/2^26)/log2(10) + 10*log2(subframe_size)/log2(10)
> +      =30 - [10/log2(10)] * log2(energy/2^26) + [10/log2(10)] * log2(subframe_size)
> +      = -24660 * log2(energy) + 24660 * log2(subframe_size) + 24660 * 26 + 30<<13

> +      

trailing whitespace


[...]
> +    /* 3.9.1, Equation 71 */
> +    /*
> +      energy = 10^(energy / 20) = 2^(3.3219 * energy / 20) = 2^ (0.166 * energy)
> +      5439 = 0.166 in Q15
> +    */
> +    energy = (5439 * (energy >> 15)) >> 8; // Q23->Q8, Q23 -> Q15
> +
> +    /* 
> +      Following code will calculate energy*2^14 instead of energy*2^exp
> +      due to recent change of energy_int's integer part.
> +      This is done to avoid overflow. Result fits into 16-bit.
> +    */
> +    exp = (energy >> 15);             // integer part (exponent)
> +    energy = l_pow2(energy & 0x7fff) & 0x7fff; // Only fraction part of Q15

> +
> +    // shift prediction error vector
> +    for(i=3; i>0; i--)
> +        pred_energ_q[i] = pred_energ_q[i-1];

s/error/energy/


[...]
> +/**
> + * \brief Memory update (3.10)
> + * \param fc_v (Q13) fixed-codebook vector
> + * \param gp (Q14) quantized fixed-codebook gain (gain pitch)
> + * \param gc (Q1) quantized adaptive-codebook gain (gain code)
> + * \param exc [in/out] (Q0) last excitation signal buffer for current subframe
> + * \param subframe_size length of subframe
> + */
> +static void g729_mem_update(const int16_t *fc_v, int16_t gp, int16_t gc, int16_t* exc, int subframe_size)

update_excitation()

[...]



> +
> +/**
> + * \brief LP synthesis filter
> + * \param lp (Q12) filter coefficients

if so, rename lp to filter_coeffs


> + * \param in (Q0) input signal
> + * \param out [out] (Q0) output (filtered) signal
> + * \param filter_data [in/out] (Q0) filter data array (previous synthesis data)
> + * \param subframe_size length of subframe
> + * \param exit_on_overflow 1 - If overflow occured routine updates neither out nor
> + *                         filter data arrays, 0 - always update
> + *
> + * \return 1 if overflow occured, 0 - otherwise
> + *
> + * Routine applies 1/A(z) filter to given speech data
> + */
> +static int g729_lp_synthesis_filter(const int16_t* lp, const int16_t *in, int16_t *out, int16_t *filter_data, int subframe_size, int exit_on_overflow)
> +{
> +    int16_t tmp_buf[MAX_SUBFRAME_SIZE+10];
> +    int16_t* tmp=tmp_buf+10;
> +    int i,n;
> +    int sum;
> +
> +    memcpy(tmp_buf, filter_data, 10*sizeof(int16_t));
> +
> +    for(n=0; n<subframe_size; n++)
> +    {
> +        sum = in[n] << 12;
> +        for(i=0; i<10; i++)
> +            sum -= lp[i] * tmp[n-i-1];
> +        sum >>= 12;
> +        if(sum > SHRT_MAX || sum < SHRT_MIN)
> +        {
> +            if(exit_on_overflow)
> +                return 1;
> +            sum = av_clip_int16(sum);
> +        }
> +        tmp[n] = sum;
> +    }
> +
> +    memcpy(filter_data, tmp + subframe_size - 10, 10*sizeof(int16_t));
> +    memcpy(out, tmp, subframe_size*sizeof(int16_t));

int i,n;

for(n=0; n<subframe_size; n++){
    int sum = in[n] << 12;
    for(i=0; i<10; i++)
        sum -= filter_coeffs[i] * filter_data[9+n-i];
    sum >>= 12;
    if(sum > SHRT_MAX || sum < SHRT_MIN){
        if(exit_on_overflow)
            return 1;
        sum = av_clip_int16(sum);
    }
    out[n] =
    filter_data[10+n] = sum;
}

memcpy(filter_data, filter_data + subframe_size - 10, 10*sizeof(int16_t));



> +
> +    return 0;
> +}

> +
> +/**
> + * \brief Adaptive gain control (4.2.4)
> + * \param gain_before (Q0) gain of speech before applying postfilters
> + * \param gain_after  (Q0) gain of speech after applying postfilters
> + * \param speech [in/out] (Q0) signal buffer
> + * \param subframe_size length of subframe
> + * \param gain_prev previous value of gain coefficient
> + *
> + * \return last value of gain coefficient
> + */
> +static int16_t g729a_adaptive_gain_control(int gain_before, int gain_after, int16_t *speech, int subframe_size, int16_t gain_prev)
> +{
> +    int gain; // Q12
> +    int n;
> +
> +    if(!gain_after)
> +        return gain_prev;
> +
> +    if(gain_before)
> +    {
> +        gain = l_div(gain_after, gain_before, 12); // Q12
> +        gain = l_inv_sqrt(gain) >> 11; // Q23 -> Q12
> +    }
> +    else
> +        gain = 0;
> +
> +    for(n=0; n<subframe_size; n++)
> +    {
> +        // 0.9 * ctx->gain_coeff + 0.1 * gain

> +        gain_prev = (29491 * gain_prev + 3276 * gain) >> 15;

the >>15 are done at the wrong spot



> +        speech[n] = (speech[n] * gain_prev) >> 12;
> +    }
> +    return gain_prev;
> +}
> +

> +/**
> + * \brief Calculates coefficients of weighted A(z/GAMMA) filter
> + * \param Az (Q12) source filter
> + * \param gamma (Q15) weight coefficients
> + * \param Azg [out] (Q12) resulted weighted A(z/GAMMA) filter
> + *
> + * Azg[i]=GAMMA^i*Az[i] , i=0..subframe_size
> + */
> +static void g729a_weighted_filter(const int16_t* Az, int16_t gamma, int16_t *Azg)

What is gamma? This is not a greek book, gamma identifies a letter. But this
clearly is not about that letter.


[...]
> +/**
> + * \brief long-term postfilter (4.2.1)
> + * \param intT1 integer part of the pitch delay T1 in the first subframe
> + * \param residual (Q0) filtering input data
> + * \param residual_filt [out] (Q0) speech signal with applied A(z/GAMMA_N) filter
> + * \param subframe_size size of subframe
> + */
> +static void g729a_long_term_filter(int intT1, const int16_t* residual, int16_t *residual_filt, int subframe_size)
> +{
> +    int k, n, intT0;
> +    int gl;      // gain coefficient for long-term postfilter
> +    int corr_t0; // correlation of residual signal with delay intT0
> +    int corr_0;  // correlation of residual signal with delay 0
> +    int correlation, corr_max;


> +    int inv_glgp;      // Q15 1.0/(1+gl*GAMMA_P)
> +    int glgp_inv_glgp; // Q15 gl*GAMMA_P/(1+gl*GAMMA_P);

int coeff_a; // Q15          1 / (1 + gl*GAMMA_P)
int coeff_b; // Q15 gl*GAMMA_P / (1 + gl*GAMMA_P);

these names are just confusing i think


[...]
> +    tmp = av_log2(FFMAX(corr_0, FFMAX(corr_t0, corr_max)));

add a FFMAX3(a,b,c) function
also send patch replacing `grep 'FFMAX(.*FFMAX' *` by it


> +    if(tmp > 14)
> +    {
> +        corr_t0  >>= tmp - 14;
> +        corr_0   >>= tmp - 14;
> +        corr_max >>= tmp - 14;
> +    }
> +
> +    /* 4.2.1, Equation 82. check if filter should be disabled */
> +    if(corr_max * corr_max < (corr_0 * corr_t0) >> 1)
> +        gl = 0;
> +    else if(!corr_t0 || corr_max > corr_t0)
> +        gl = 32768; // 1.0 in Q15
> +    else
> +        gl=l_div(corr_max, corr_t0, 15);
> +
> +    gl = (gl * GAMMA_P) >> 15;
> +
> +    if (gl < -32768) // -1.0 in Q15
> +        inv_glgp = 0;
> +    else
> +        inv_glgp = l_div(32768, 32768 + gl, 15); // 1/(1+gl) in Q15
> +

> +    glgp_inv_glgp = 32768 - inv_glgp; // 1.0 in Q15

shouldnt this be 32767?



> +
> +    /* 4.2.1, Equation 78, reconstruct delayed signal */
> +    for(n=0; n<subframe_size; n++)

> +        residual_filt[n] = (residual[n + PITCH_MAX        ] * inv_glgp +
> +                            residual[n + PITCH_MAX - intT0] * glgp_inv_glgp) >> 15;

The position where the >> 15 is done is wrong i think


> +}
> +

> +/**
> + * \brief compensates the tilt in the short-term postfilter (4.2.3)
> + * \param ctx private data structure
> + * \param lp_gn (Q12) coefficients of A(z/GAMMA_N) filter
> + * \param lp_gd (Q12) coefficients of A(z/GAMMA_D) filter
> + * \param res_pst [in/out] (Q0) residual signal (partially filtered)
> +*/
> +static void g729a_tilt_compensation(G729A_Context *ctx, const int16_t *lp_gn, const int16_t *lp_gd, int16_t* res_pst)
> +{
> +    int tmp;
> +    int gt;      // Q12
> +    int rh1,rh0; // Q12
> +    int16_t hf_buf[11+22]; // Q12 A(Z/GAMMA_N)/A(z/GAMMA_D) filter impulse response
> +    int sum;
> +    int i, n;
> +    
> +    memset(hf_buf, 0, 33 * sizeof(int16_t));
> +

> +    hf_buf[10] = 4096; //1.0 in Q12
> +    for(i=0; i<10; i++)
> +        hf_buf[i+11] = lp_gn[i];
> +
> +    /* Apply 1/A(z/GAMMA_D) filter to hf */
> +    for(n=0; n<22; n++)
> +    {
> +        sum=hf_buf[n+10];
> +        for(i=0; i<10; i++)
> +            sum -= (lp_gd[i] * hf_buf[n+10-i-1]) >> 12;
> +        hf_buf[n+10] = sum;
> +    }

for(n=0; n<22; n++){
    sum= lp_gn[n];
    for(i=0; i<10; i++)
        sum -= (lp_gd[i] * hf_buf[n+10-i-1]) >> 12;
    hf_buf[n+10] = sum;
}


> +
> +    /* Now hf_buf (starting with 10) contains impulse response of A(z/GAMMA_N)/A(z/GAMMA_D) filter */
> +
> +    /* A.4.2.3, Equation A.14, calcuate rh(0)  */
> +    rh0 = sum_of_squares(hf_buf+10, 22, 0, 0) >> 12;   // Q24 -> Q12
> +
> +    /* A.4.2.3, Equation A.14, calcuate rh(1)  */
> +    rh1 = sum_of_squares(hf_buf+10, 22-1, 1, 0) >> 12; // Q24 -> Q12
> +
> +    rh1 = rh1 * GAMMA_T >> 15; // Q12 * Q15 = Q27 -> Q12

> +
> +    /* A.4.2.3, Equation A.14 */
> +    if(rh1>0)
> +        gt = -l_div(rh1, rh0, 12); // l_div accepts only positive parameters

The comment is wrong


> +    else
> +        gt = 0;
> +

and a FFMAX(0, ) can achive this cleaner


[...]
> +/**
> + * \brief Residual signal calculation (4.2.1)

> + * \param lp (Q12) A(z/GAMMA_N) filter coefficients

if lp are filter coeffs name the variable appropriately


> + * \param speech (Q0) input speech data
> + * \param residual [out] (Q0) output data filtered through A(z/GAMMA_N)
> + * \param subframe_size size of one subframe
> + * \param pos_filter_data [in/out] (Q0) speech data of previous subframe
> + */
> +static void g729_residual(int16_t* lp, const int16_t* speech, int16_t* residual, int subframe_size, int16_t* pos_filter_data)
> +{
> +    int i, n, sum;
> +    int16_t tmp_speech_buf[MAX_SUBFRAME_SIZE+10];
> +    int16_t *tmp_speech=tmp_speech_buf+10;
> +
> +    // Copy data from previous frame
> +    for(i=0; i<10; i++)
> +        tmp_speech[-10+i] = pos_filter_data[i];
> +
> +    // Copy the rest of speech data
> +    for(i=0; i<subframe_size; i++)
> +        tmp_speech[i] = speech[i];
> +    /*
> +      4.2.1, Equation 79 Residual signal calculation
> +      ( filtering through A(z/GAMMA_N) , one half of short-term filter)
> +    */
> +    for(n=0; n<subframe_size; n++)
> +    {
> +        sum = tmp_speech[n] << 12;
> +        for(i=0; i<10; i++)
> +            sum += lp[i] * tmp_speech[n-i-1];
> +        sum = av_clip(sum, SHRT_MIN << 12, SHRT_MAX << 12);
> +        residual[n+PITCH_MAX] = g729_round(sum << 4);
> +    }
> +
> +    // Save data to use it in the next subframe
> +    for(i=0; i<10; i++)
> +        pos_filter_data[i] = speech[subframe_size-10+i];

there are 3 memcpy() in there please get rid of 2 of them
the same applies to other routines which similarly are full of memcpys
get rid of ALL unneeded mempcpy()


[...]
> +/**
> + * \brief high-pass filtering and upscaling (4.2.5)
> + * \param ctx private data structure
> + * \param speech [in/out] reconstructed speech signal for applying filter to
> + * \param length size of input data
> + *
> + * Filter has cut-off frequency 100Hz
> + */
> +static void g729_high_pass_filter(G729A_Context* ctx, int16_t* speech, int length)
> +{
> +    int i;
> +
> +    for(i=0; i<length; i++)
> +    {
> +        memmove(ctx->hpf_z+1, ctx->hpf_z, 2*sizeof(ctx->hpf_z[0]));
> +        ctx->hpf_z[0]=speech[i];
> +

> +        ctx->hpf_f[0] = mul_24_15(ctx->hpf_f[1], 15836)
> +            + mul_24_15(ctx->hpf_f[2], -7667)

vertical align


[...]
> +/**
> + * \brief save quantized LSF coefficients for using in next frame
> + * \param ctx context structure
> + * \param lq quantized LSF coefficients
> + */
> +static void g729_lq_rotate(G729A_Context *ctx, int* lq)
> +{
> +    int i, k;
> +

> +    /* Rotate lq_prev */
> +    for(i=0; i<10; i++)
> +    {
> +        for(k=MA_NP-1; k>0; k--)
> +            ctx->lq_prev[k][i] = ctx->lq_prev[k-1][i];
> +
> +        ctx->lq_prev[0][i] = lq[i];
> +    }

reorder and use memcpy()
also pass the arrays not the context as parameters, this is cleaner IMHO
and makes it more clear what is changed



[...]

> +/**
> + * \brief Decode LSP coefficients from L0-L3 (3.2.4)
> + * \param ctx private data structure

> + * \param L0 Switched MA predictor of LSP quantizer
> + * \param L1 First stage vector of quantizer
> + * \param L2 Second stage lower vector of LSP quantizer
> + * \param L3 Second stage higher vector of LSP quantizer

I do not like the variable names at all, these are indexes into VQ tables
nothing in their name nor comment gives a hint to that.


> + * \param lsfq [out] (Q13) Decoded LSP coefficients
> + */
> +static void g729_lsf_decode(G729A_Context* ctx, int16_t L0, int16_t L1, int16_t L2, int16_t L3, int16_t* lsfq)
> +{
> +    int i,j,k;

> +    int16_t J[2]={10, 5}; //Q13

find a better name, also this should be static const uint8_t


[...]
> +    /*
> +    Pitch gain of previous subframe.
> +
> +    (EE) This does not comply with specification, but reference
> +         and Intel decoder uses here minimum sharpen value instead of maximum. */
> +    ctx->pitch_sharp = SHARP_MIN;

The comment should be somehow changed so it is more seperate from the code
that is * before the lines or indented.


[...]
> +    /* LSP coefficients */
> +    for(i=0; i<10; i++)
> +        ctx->lq_prev[0][i]=lq_init[i];
[...]
> +    for(k=1; k<MA_NP; k++)
> +        for(i=0; i<10; i++)
> +            ctx->lq_prev[k][i]=ctx->lq_prev[0][i];

This can be done simpler and cleaner.


[...]
> +/**
> + * \brief decode one G.729 frame into PCM samples
> + * \param ctx private data structure
> + * \param out_frame array for output PCM samples
> + * \param out_frame_size maximum number of elements in output array
> + * \param parm decoded parameters of the codec
> + * \param frame_erasure frame erasure flag
> + *
> + * \return 2 * subframe_size
> + */
> +static int  g729a_decode_frame_internal(G729A_Context* ctx, int16_t* out_frame, int out_frame_size, G729_parameters *parm, int frame_erasure)
> +{
> +    int16_t lp[20];              // Q12
> +    int16_t lsp[10];             // Q15
> +    int16_t lsf[10];             // Q13
> +    int pitch_delay_3x;          // pitch delay, multiplied by 3
> +    int pitch_delay_int;         // pitch delay, integer part
> +    int16_t fc[MAX_SUBFRAME_SIZE]; // fixed codebooc vector
> +    int i, j;
> +

> +    ctx->data_error = frame_erasure;

whichever it is erasure (missing/droped frame) != error
figure it out and name everything consistently


[...]
> +        if(!i)
> +        {
> +            // Decoding of the adaptive-codebook vector delay for first subframe (4.1.3)
> +            if(ctx->bad_pitch || ctx->data_error)
> +                pitch_delay_3x = 3 * ctx->pitch_delay_int_prev + 1;
> +            else
> +            {

> +                if(parm->ac_index[i] >= 197)
> +                    pitch_delay_3x = 3 * parm->ac_index[i] - 335;
> +                else
> +                    pitch_delay_3x = parm->ac_index[i] + 59;

not sure but somehow the follow looks nicer to me

pitch_delay_3x = parm->ac_index[i] + 59;
if(pitch_delay_3x > 255)
    pitch_delay_3x = 3 * pitch_delay_3x - 512;



> +            }
> +        }
> +        else
> +        {
> +            // Decoding of the adaptive-codebook vector delay for second subframe (4.1.3)
> +            if(ctx->data_error)
> +                pitch_delay_3x = 3 * ctx->pitch_delay_int_prev + 1;
> +            else
> +                pitch_delay_3x = parm->ac_index[i] +
> +                        3 * av_clip(ctx->pitch_delay_int_prev-5, PITCH_MIN, PITCH_MAX-9) - 1;
> +        }

the error case is the same and can be factored out


> +        pitch_delay_int = pitch_delay_3x / 3;
> +
> +        g729_decode_ac_vector(pitch_delay_int, (pitch_delay_3x%3)-1,
> +                ctx->exc + i*ctx->subframe_size, ctx->subframe_size);

spliting it in int & frac just to then fix the resulting mess in an if()
in the function is not good, pass pitch_delay_3x and correctly split it


[...]
> +            g729_update_gain_erasure(ctx->pred_energ_q);
> +        }
> +        else
> +        {
> +            // Decoding of the fixed codebook gain (4.1.5 and 3.9.1)
> +            ctx->gain_pitch = cb_GA[parm->ga_cb_index[i]][0] + cb_GB[parm->gb_cb_index[i]][0];
> +
> +            ctx->gain_code = g729_get_gain_code(parm->ga_cb_index[i],
> +                    parm->gb_cb_index[i],

besides the very ugly misalignment of the parameters
it woudl be MUCH cleaer to look up all values outside, not look up the first 2
outside and the next 2 inside the function


[...]
> +    parm->ma_predictor     = get_bits(&gb, L0_BITS);        //L0
> +    parm->quantizer_1st    = get_bits(&gb, L1_BITS);        //L1
> +    parm->quantizer_2nd_lo = get_bits(&gb, L2_BITS);        //L2
> +    parm->quantizer_2nd_hi = get_bits(&gb, L3_BITS);        //L3
> +
> +    parm->ac_index[0]      = get_bits(&gb, P1_BITS);        //P1
> +    parm->parity           = get_bits(&gb, P0_BITS);        //P0 (parity)
> +    parm->fc_indexes[0]    = get_bits(&gb, FC_BITS(ctx));   //C1
> +    parm->pulses_signs[0]  = get_bits(&gb, FC_PULSE_COUNT); //S1
> +    parm->ga_cb_index[0]   = get_bits(&gb, GA_BITS);        //GA1
> +    parm->gb_cb_index[0]   = get_bits(&gb, GB_BITS);        //GB1
> +
> +    parm->ac_index[1]      = get_bits(&gb, P2_BITS);        //P2
> +    parm->fc_indexes[1]    = get_bits(&gb, FC_BITS(ctx));   //C2
> +    parm->pulses_signs[1]  = get_bits(&gb, FC_PULSE_COUNT); //S2
> +    parm->ga_cb_index[1]   = get_bits(&gb, GA_BITS);        //GA2
> +    parm->gb_cb_index[1]   = get_bits(&gb, GB_BITS);        //GB2

reaname all the LPFG_1234_BITS to names describing their content!


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Those who are too smart to engage in politics are punished by being
governed by those who are dumber. -- Plato 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080323/4eb171b7/attachment.pgp>



More information about the ffmpeg-devel mailing list