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

Michael Niedermayer michaelni
Sat Mar 1 00:12:33 CET 2008


On Fri, Feb 29, 2008 at 01:36:01AM +0600, Vladimir Voroshilov wrote:
> Hi, Michael
> 
> Let's start second roundup of patch cleanup procedure.
> I want to collect issues for fixing at weekend :)
> 
[...]


> +///Switched MA predictor of LSP quantizer (size in bits)
> +#define L0_BITS 1
> +///First stage vector of quantizer (size in bits)
> +#define L1_BITS 7
> +///First stage lowervector of quantizer (size in bits)
> +#define L2_BITS 5
> +///First stage hihjer vector of quantizer (size in bits)
> +#define L3_BITS 5
> +///Adaptive codebook index for first subframe (size in bits)
> +#define P1_BITS 8
> +///Adaptive codebook index for second subframe (size in bits)
> +#define P2_BITS 5
> +///Parity bit for pitch delay (size in bits)
> +#define P0_BITS 1
> +/// GA codebook index (size in bits)
> +#define GA_BITS 3
> +/// GB codebook index (size in bits)
> +#define GB_BITS 4
> +/// Number of pulses in fixed-codebook vector
> +#define FC_PULSE_COUNT 4
> +
> +///Size of parameters vector
> +#define VECTOR_SIZE 15
> +
> +static const struct
> +{
> +    int sample_rate;
> +    ///Size (in bytes) of input frame
> +    uint8_t input_frame_size;
> +    ///Size (in bytes) of output frame
> +    uint8_t output_frame_size;
> +    ///Size (in bits) of fixed codebook index
> +    uint8_t fc_index_bits;
> +} formats[] =

please put the doxygen comments to the right of variables


[...]
> +static const float interp_filter[31] =
> +{
> +   0.898517,
> +   0.769271,   0.448635,   0.095915,
> +  -0.134333,  -0.178528,  -0.084919,
> +   0.036952,   0.095533,   0.068936,
> +  -0.000000,  -0.050404,  -0.050835,
> +  -0.014169,   0.023083,   0.033543,
> +   0.016774,  -0.007466,  -0.019340,
> +  -0.013755,   0.000000,   0.009400,
> +   0.009029,   0.002381,  -0.003658,
> +  -0.005027,  -0.002405,   0.001050,
> +   0.002780,   0.002145,   0.000000
> +};

do we loose any PSNR if this is replaced by the integer one?
also it could be a [3][10] array for simpler access


[...]
> +/**
> + * Initial LSP values
> + */
> +static const float lsp_init[10] =
> +{
> +  0.9595, 0.8413, 0.6549, 0.4154, 0.1423, -0.1423, -0.4154, -0.6549, -0.8413, -0.9595,
> +};

fits in an int16_t


[...]
> +static float sum_of_squares(float *speech, int length)
> +{
> +    int n;
> +    float sum;
> +
> +    if(!length)
> +        return 0;
> +
> +    sum=speech[0]*speech[0];
> +    for(n=1; n<length; n++)
> +       sum+=speech[n]*speech[n];
> +
> +    return sum;
> +}

static float sum_of_squares(float *speech, int length)
{
    int n;
    float sum=0;

    for(n=0; n<length; n++)
       sum+=speech[n]*speech[n];

    return sum;
}



> +
> +/**
> + * \brief pseudo random number generator
> + */
> +static inline uint16_t g729_random(G729A_Context* ctx)
> +{
> +    return ctx->rand_value = 31821 * (uint32_t)ctx->rand_value + 13849;
> +}

remove the cast


[...]
> +/**
> + * \brief Decoding of the adaptive-codebook vector delay for second subframe (4.1.3)
> + * \param ctx private data structure
> + * \param ac_index Adaptive codebook index for second subframe
> + * \param intT1 first subframe's pitch delay integer part
> + *
> + * \return 3*intT+frac+1, where
> + *   intT integer part of delay
> + *   frac fractional part of delay [-1, 0, 1]
> + */
> +static int g729_decode_ac_delay_subframe2(G729A_Context* ctx, int ac_index, int intT1)
> +{
> +    if(ctx->data_error)
> +        return 3*intT1+1;

s/intT1/whatever sane name it has elsewhere/


> +
> +    return ac_index + 3*FFMIN(FFMAX(intT1-5, PITCH_MIN), PITCH_MAX-9) - 1;
> +}
> +

> +/**
> + * \brief Decoding of the adaptive-codebook vector (4.1.3)
> + * \param ctx private data structure
> + * \param pitch_delay_int pitch delay, integer part
> + * \param pitch_delay_frac pitch delay, fraction part [-1, 0, 1]
> + * \param ac_v buffer to store decoded vector into
> + */
> +static void g729_decode_ac_vector(G729A_Context* ctx, int pitch_delay_int, int pitch_delay_frac, float* ac_v)
> +{
> +    int n, i;
> +    float v;
> +
> +    //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]

t ? k? there are no t and k


> +    for(n=0; n<ctx->subframe_size; n++)
> +    {
> +        /* 3.7.1, Equation 40 */
> +        v=0;
> +        for(i=0; i<10; i++)
> +        {

> +            /*  R(x):=ac_v[-k+x] */
> +            v+=ac_v[n-pitch_delay_int-i]*interp_filter[pitch_delay_frac+3*i];     //R(n-i)*b30(t+3i)
> +            v+=ac_v[n-pitch_delay_int+i+1]*interp_filter[3-pitch_delay_frac+3*i]; //R(n+i+1)*b30(3-t+3i)

vertical align


[...]
> +/**
> + * \brief fixed codebook vector modification if delay is less than 40 (4.1.4 and 3.8)
> + * \param pitch_delay integer part of pitch delay
> + * \param fc_v [in/out] fixed codebook vector to change
> + *
> + * \remark if pitch_delay>=subframe_size no changes to vector are made
> + */
> +static void g729_fix_fc_vector(G729A_Context *ctx, int pitch_delay, float* fc_v)
> +{
> +    int i;
> +
> +    if(pitch_delay>=ctx->subframe_size)
> +        return;
> +
> +    for(i=pitch_delay; i<ctx->subframe_size;i++)
> +        fc_v[i] += fc_v[i-pitch_delay]*ctx->gain_pitch;
> +}

the if() is useless
gain_pitch and subframe_size could be passed as arguments intead of ctx


> +
> +/**
> + * \brief Decoding of the adaptive and fixed codebook gains from previous subframe (4.4.2)
> + * \param ctx private data structure
> + * \param gp pointer to variable receiving quantized fixed-codebook gain (gain pitch)
> + * \param gc pointer to variable receiving quantized adaptive-codebook gain (gain code)
> + */
> +static void g729_get_gain_from_previous(G729A_Context *ctx, float* gp, float* gc)
> +{
> +    /* 4.4.2, Equation 93 */
> +    *gc = 0.98*ctx->gain_code;
> +    ctx->gain_code = *gc;
> +
> +    /* 4.4.2, Equation 94 */
> +    *gp = FFMIN(0.9*ctx->gain_pitch, 0.9);
> +    ctx->gain_pitch = *gp;
> +}

could you please avoid writing into random things of the context in functions.
in this case the 2 independant equations should just be inlined.


> +
> +/**
> + * \brief Attenuation of the memory of the gain predictor (4.4.3)
> + * \param ctx private data structure
> + */
> +static void g729_update_gain(G729A_Context *ctx)

the pointer to the modified array should be passed as argument instead of ctx.


> +{
> +    float avg_gain=0;
> +    int i;
> +
> +    /* 4.4.3. Equation 95 */
> +    for(i=0; i<4; i++)
> +        avg_gain+=ctx->pred_vect_q[i];
> +
> +    avg_gain = FFMAX(avg_gain * 0.25 - 4.0, -14);
> +
> +    for(i=3; i>0; i--)
> +        ctx->pred_vect_q[i]=ctx->pred_vect_q[i-1];
> +    ctx->pred_vect_q[0]=avg_gain;


float avg_gain= pred_vect_q[3];

for(i=3; i>0; i--){
    avg_gain     += pred_vect_q[i-1];
    pred_vect_q[i]= pred_vect_q[i-1];
}
pred_vect_q[0]= FFMAX(avg_gain * 0.25 - 4.0, -14);



> +}
> +

> +/**
> + * \brief Decoding of the adaptive and fixed codebook gains (4.1.5 and 3.9.1)
> + * \param ctx private data structure
> + * \param GA Gain codebook (stage 2)
> + * \param GB Gain codebook (stage 2)
> + * \param fc_v fixed-codebook vector
> + * \param gp pointer to variable receiving quantized fixed-codebook gain (gain pitch)
> + * \param gc pointer to variable receiving quantized adaptive-codebook gain (gain code)
> + */
> +static void g729_get_gain(G729A_Context *ctx, int nGA, int nGB, float* fc_v, float* gp, float* gc)

doxygen and parameters dont match


[...]
> +    /* 3.9.1, Equation 74 */
> +    *gc = energy*(cb1_sum);  //quantized fixed-codebook gain (gain pitch)

useless ()


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

const float* Az

> +{

> +    float gamma_pow;
> +    int n;
> +
> +    gamma_pow=gamma;

declaration and initialization can be marged


[...]

> +/**
> + * \brief long-term postfilter (4.2.1)
> + * \param ctx private data structure
> + * \param intT1 integer part of the pitch delay T1 in the first subframe
> + * \param residual_filt speech signal with applied A(z/GAMMA_N) filter
> + */
> +static void g729a_long_term_filter(G729A_Context *ctx, int intT1, float *residual_filt)
> +{
> +    int k, n, intT0;
> +    float gl;      ///< gain coefficient for long-term postfilter
> +    float corr_t0; ///< correlation of residual signal with delay intT0
> +    float corr_0;  ///< correlation of residual signal with delay 0
> +    float correlation, corr_max;
> +    float inv_glgp;///< 1.0/(1+gl*GAMMA_P)
> +    float glgp_inv_glgp; ///< gl*GAMMA_P/(1+gl*GAMMA_P);

I already said that doxygen comments are not for local variables!


> +
> +    /* A.4.2.1 */
> +    int minT0=FFMIN(intT1, PITCH_MAX-3)-3;
> +    int maxT0=FFMIN(intT1, PITCH_MAX-3)+3;
> +    /* Long-term postfilter start */
> +

> +    /*
> +       First pass: searching the best T0 (pitch delay)
> +       Second pass is not used in G.729A: fractional part is always zero
> +    */
> +    intT0=minT0;
> +    corr_max=INT_MIN;
> +    for(k=minT0; k<=maxT0; k++)
> +    {
> +        correlation=0;
> +        /* 4.2.1, Equation 80 */
> +        for(n=0; n<ctx->subframe_size; n++)
> +            correlation+=ctx->residual[n+PITCH_MAX]*ctx->residual[n+PITCH_MAX-k];
> +        if(correlation>corr_max)
> +        {
> +            corr_max=correlation;
> +            intT0=k;
> +        }
> +    }
> +
> +    corr_t0=sum_of_squares(ctx->residual+PITCH_MAX-intT0, ctx->subframe_size);
> +    corr_0=sum_of_squares(ctx->residual+PITCH_MAX, ctx->subframe_size);

vertical align, also if you add a int offset as parameter then this could be
used in the loop above as well


> +
> +    /* 4.2.1, Equation 82. checking if filter should be disabled */
> +    if(corr_max*corr_max < 0.5*corr_0*corr_t0)
> +        gl=0;
> +    else if(!corr_t0)
> +        gl=1;
> +    else
> +        gl=FFMIN(corr_max/corr_t0, 1);
> +

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

g1 *= GAMMA_P;
inv_glgp      = 1.0 / (1 + gl);
glgp_inv_glgp = g1  / (1 + gl);


> +
> +    /* 4.2.1, Equation 78, reconstructing delayed signal */
> +    for(n=0; n<ctx->subframe_size; n++)
> +        residual_filt[n]=ctx->residual[n+PITCH_MAX]*inv_glgp+ctx->residual[n+PITCH_MAX-intT0]*glgp_inv_glgp;

use spaces, please, and format things in a slightly more readable way and
not onle here ...

residual_filt[n]=  ctx->residual[n+PITCH_MAX      ]*inv_glgp
                 + ctx->residual[n+PITCH_MAX-intT0]*glgp_inv_glgp;


> +
> +    //Shift residual for using in next subframe
> +    memmove(ctx->residual, ctx->residual+ctx->subframe_size, PITCH_MAX*sizeof(float));
> +}
> +

> +/**
> + * \brief compensates the tilt in the short-term postfilter (4.2.3)
> + * \param ctx private data structure
> + * \param lp_gn coefficients of A(z/GAMMA_N) filter
> + * \param lp_gd coefficients of A(z/GAMMA_D) filter
> + * \param res_pst residual signal (partially filtered)
> +*/
> +static void g729a_tilt_compensation(G729A_Context *ctx,float *lp_gn, float *lp_gd, float* res_pst)
> +{
> +    float tmp;
> +    float gt,k,rh1,rh0;
> +    float hf[22]; // A(Z/GAMMA_N)/A(z/GAMMA_D) filter impulse response
> +    float tmp_buf[11+22];
> +    float sum;
> +    int i, n;
> +
> +    hf[0]=1;
> +    for(i=0; i<10; i++)
> +        hf[i+1]=lp_gn[i];
> +
> +    for(i=11; i<22;i++)
> +        hf[i]=0;
> +

> +    /* Applying 1/A(z/GAMMA_D) to hf */
> +    for(i=0; i<10; i++)
> +        tmp_buf[i]=hf[i+11];

hf[i+11] == 0 here


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

tmp_buf and hf are redundant please remove one


> +
> +    /* Now hf contains impulse response of A(z/GAMMA_N)/A(z/GAMMA_D) filter */
> +
> +    /* A.4.2.3, Equation A.14, calcuating rh(0)  */
> +    rh0=0;
> +    for(i=0; i<22; i++)
> +        rh0+=hf[i]*hf[i];
> +
> +    /* A.4.2.3, Equation A.14, calcuating rh(1)  */
> +    rh1=0;
> +    for(i=0; i<22-1; i++)
> +        rh1+=hf[i]*hf[i+1];
> +

> +    /* A.4.2.3, Equation A.14 */
> +    k=-rh1/rh0;
> +
> +    if(k>=0)
> +        gt=0;
> +    else
> +        gt=GAMMA_T*k;

gt= -GAMMA_T * FFMAX(rh1 / rh0, 0);


[...]
> +/**
> + * \brief Signal postfiltering (4.2, with A.4.2 simplification)
> + * \param ctx private data structure
> + * \param lp LP filter coefficients

> + * \param intT1 integer part of the pitch delay T1 of the first subframe

why not call it pitch_delay_int?


[...]
> +/**
> + * \brief high-pass filtering and upscaling (4.2.5)
> + * \param ctx private data structure
> + * \param speech reconstructed speech signal for applying filter to
> + *
> + * Filter has cut-off frequency 100Hz
> + */
> +static void g729_high_pass_filter(G729A_Context* ctx, float* speech)
> +{
> +    float z_2=0;
> +    float f_0=0;
> +    int i;
> +
> +    for(i=0; i<ctx->subframe_size; i++)
> +    {
> +        z_2=ctx->hpf_z1;
> +        ctx->hpf_z1=ctx->hpf_z0;
> +        ctx->hpf_z0=speech[i];
> +

> +        f_0 = 1.9330735 * ctx->hpf_f1 - 0.93589199 * ctx->hpf_f2 + 
> +              0.93980581 * ctx->hpf_z0 - 1.8795834 * ctx->hpf_z1 + 0.93980581 * z_2;

f_0 =   1.9330735  * ctx->hpf_f1
      - 0.93589199 * ctx->hpf_f2
      + 0.93980581 * (ctx->hpf_z0 + z_2) 
      - 1.8795834  * ctx->hpf_z1;


[...]
> +    for(i=0; i<ctx->subframe_size; i++)
> +    {
> +        tmp_speech[i] = FFMIN(tmp_speech[i],  32767.0);
> +        tmp_speech[i] = FFMAX(tmp_speech[i], -32768.0);
> +        speech[i]=lrintf(tmp_speech[i]);

the MAX/MIN should be after the lrintf()


> +    }
> +}
> +

> +/**
> + * \brief Convert LSF to LSP
> + * \param ctx private data structure
> + * \param lsf LSF coefficients
> + * \param lsp LSP coefficients
> + *
> + * \remark It is safe to pass the same array in lsf and lsp parameters
> + */
> +static void g729_lsf2lsp(G729A_Context *ctx, float *lsf, float *lsp)

const float *lsf;

> +{
> +    int i;
> +
> +    /* Convert LSF to LSP */
> +    for(i=0;i<10; i++)
> +        lsp[i]=cos(lsf[i]);
> +}

cos() can be very slow (depending on hardware)
please use the LUT based solution from the integer implementation


[...]
> +/**
> + * \brief Decode LP coefficients from L0-L3 (3.2.4)

LP or LSP ?


> + * \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
> + * \param lsfq Decoded LSP coefficients
> + */
> +static void g729_lsp_decode(G729A_Context* ctx, int16_t L0, int16_t L1, int16_t L2, int16_t L3, float* lsfq)

[...]

> +    /* 3.2.4, Equation 20 */
> +    for(i=0; i<10; i++)
> +    {
> +        lsfq[i]=lq[i] * ma_predictor_sum[L0][i] / Q13_BASE;
> +        for(k=0; k<MA_NP; k++)
> +            lsfq[i] += (ctx->lq_prev[k][i] * ma_predictor[L0][k][i]); //Q15
> +	lsfq[i] /= Q15_BASE;
> +        //Saving LSF for using when error occured in next frames
> +        ctx->lsf_prev[i]=lsfq[i];
> +    }

tabs are forbidden in svn
the Q13_BASE downscale is done at the wrong point, see the integer
implementation.
lq_prev and lsf_prev can be changed to integer as well.


[...]

> +
> +static void get_lsp_coefficients(float* q, float* f)

const float *q or even better const float *lsp


> +{
> +    int i, j;
> +    int qidx=2;
> +    float b;
> +
> +    f[0]=1.0;
> +    f[1]=-2*q[0];
> +
> +    for(i=2; i<=5; i++)
> +    {
> +        b=-2*q[qidx];

> +        f[i] = b*f[i-1] + 2*f[i-2];
> +
> +        for(j=i-1; j>1; j--)
> +        {
> +            f[j] += b*f[j-1] + f[j-2];
> +        }

f[i] = f[i-2];

for(j=i; j>1; j--)
    f[j] += b*f[j-1] + f[j-2];


> +        f[1]+=b;
> +        qidx+=2;
> +    }

as qidx=2*i-2 theres no need for a qidx variable


> +}

> +/**
> + * \brief LSP to LP conversion (3.2.6)
> + * \param ctx private data structure

> + * \param lsp LSP coefficients
> + * \param lp decoded LP coefficients

indicate what is input and output please


> + */
> +static void g729_lsp2lp(G729A_Context* ctx, float* lsp, float* lp)

const float *lsp


[...]
> +/**
> + * \brief interpolate LSP end decode LP for both first and second subframes (3.2.5 and 3.2.6)

Interpolate LSP for the first subframe and convert LSP -> LP for both subframes.


> + * \param ctx private data structure

> + * \param lsp_curr current LSP coefficients

@param lsp_2nd LSP coefficients of the 2nd subframe


> + * \param lp [out] decoded LP coefficients
> + */
> +static void g729_lp_decode(G729A_Context* ctx, float* lsp_curr, float* lp)
> +{
> +    float lsp[10];

lsp_1st[10];


[...]
> +/**
> + * \brief G.729A decoder initialization
> + * \param ctx private data structure
> + * \return 0 if success, non-zero otherwise
> + */
> +static int ff_g729a_decoder_init(AVCodecContext * avctx)

param ctx ?


> +{
> +    G729A_Context* ctx=avctx->priv_data;
> +    int i,k;
> +
> +    if(avctx->sample_rate==8000)
> +        ctx->format=0;
> +#ifdef G729_SUPPORT_4400
> +    else if (avctx->sample_rate==4400)
> +        ctx->format=1;
> +#endif
> +    else
> +    {
> +        av_log(avctx, AV_LOG_ERROR, "Sample rate %d is not supported\n", avctx->sample_rate);
> +        return AVERROR_NOFMT;
> +    }

> +    ctx->subframe_size=formats[ctx->format].output_frame_size>>2; // cnumber of 2-byte long samples in one subframe
                                                                        ^^^^^^^
whats a cnumber?


[...]
> + * \param serial array if bits (0x81 - 1, 0x7F -0)
> + * \param serial_size number of items in array
> + * \param out_frame array for output PCM samples
> + * \param out_frame_size maximum number of elements in output array
> + */
> +static int  g729a_decode_frame_internal(void* context, int16_t* out_frame, int out_frame_size, int *parm)

The parameters documented are not arguments to this function (serial, ...)
and the context should have a proper type.


> +{
> +    G729A_Context* ctx=context;
> +    float lp[20];
> +    float lsp[10];
> +    int pitch_delay;                      ///< pitch delay
> +    float fc[MAX_SUBFRAME_SIZE];          ///< fixed codebooc vector
> +    float gp, gc;
> +    int intT1;
> +

> +    ctx->data_error=0;

> +    ctx->bad_pitch=0;
> +
> +    if(!g729_parity_check(parm[4], parm[5]))
> +        ctx->bad_pitch=1;

ctx->bad_pitch = !g729_parity_check(parm[4], parm[5]);


> +
> +    if(ctx->data_error)
> +        g729_lsp_restore_from_previous(ctx, lsp);
> +    else
> +        g729_lsp_decode(ctx, parm[0], parm[1], parm[2], parm[3], lsp);

data_error cannot be != 0 here in your code.


> +
> +    g729_lp_decode(ctx, lsp, lp);
> +
> +    /* first subframe */
> +    pitch_delay=g729_decode_ac_delay_subframe1(ctx, parm[4], ctx->intT2_prev);

> +    intT1=pitch_delay/3;    //Used in long-term postfilter    

There are several occurances of pitch_delay/3 below which could be replaced by
intT1 (and please rename it to something more meaningfull).


> +
> +    g729_decode_ac_vector(ctx, pitch_delay/3, (pitch_delay%3)-1, ctx->exc);
> +
> +    if(ctx->data_error)
> +    {
> +        parm[6] = g729_random(ctx) & 0x1fff;
> +        parm[7] = g729_random(ctx) & 0x000f;
> +    }
> +
> +    g729_decode_fc_vector(ctx, parm[6], parm[7], fc);
> +    g729_fix_fc_vector(ctx, pitch_delay/3, fc);
> +
> +    if(ctx->data_error)
> +    {
> +        g729_get_gain_from_previous(ctx, &gp, &gc);
> +        g729_update_gain(ctx);
> +    }
> +    else
> +    {
> +        g729_get_gain(ctx, parm[8], parm[9], fc, &gp, &gc);
> +    }
> +    g729_mem_update(ctx, fc, gp, gc, ctx->exc);
> +    g729_reconstruct_speech(ctx, lp, intT1, ctx->exc, out_frame);
> +    ctx->subframe_idx++;
> +
> +    /* second subframe */
> +    pitch_delay=g729_decode_ac_delay_subframe2(ctx, parm[10], pitch_delay/3);
> +
> +    ctx->intT2_prev=pitch_delay/3;
> +    if(ctx->data_error)
> +        ctx->intT2_prev=FFMIN(ctx->intT2_prev+1, PITCH_MAX);
> +
> +    g729_decode_ac_vector(ctx, pitch_delay/3, (pitch_delay%3)-1, ctx->exc+ctx->subframe_size);
> +
> +    if(ctx->data_error)
> +    {
> +        parm[11] = g729_random(ctx) & 0x1fff;
> +        parm[12] = g729_random(ctx) & 0x000f;
> +    }
> +
> +    g729_decode_fc_vector(ctx, parm[11], parm[12], fc);
> +    g729_fix_fc_vector(ctx, pitch_delay/3, fc);
> +
> +    if(ctx->data_error)
> +    {
> +        g729_get_gain_from_previous(ctx, &gp, &gc);
> +        g729_update_gain(ctx);
> +    }
> +    else
> +    {
> +        g729_get_gain(ctx, parm[13], parm[14], fc, &gp, &gc);
> +    }
> +    g729_mem_update(ctx, fc, gp, gc, ctx->exc+ctx->subframe_size);
> +    g729_reconstruct_speech(ctx, lp+10, intT1, ctx->exc+ctx->subframe_size, out_frame+ctx->subframe_size);
> +    ctx->subframe_idx++;

This code looks duplicated/unrolled, make a loop out of it please.


> +
> +    //Save signal for using in next frame
> +    memmove(ctx->exc_base, ctx->exc_base+2*ctx->subframe_size, (PITCH_MAX+INTERPOL_LEN)*sizeof(float));
> +
> +    return 2*ctx->subframe_size;
> +}
> +
> +/**
> + * \brief decodes one G.729 frame (10 bytes long) into parameters vector
> + * \param ctx private data structure

s/private data structure/context/


> + * \param buf 10 bytes of decoder parameters
> + * \param buf_size size of input buffer

> + * \param int parm output vector of decoded parameters
             ^^^^^^^^
?

> + *
> + * \return 0 if success, nonzero - otherwise
> + */
> +static int g729_bytes2parm(G729A_Context *ctx, const uint8_t *buf, int buf_size, int *parm)
> +{
> +    GetBitContext gb;
> +    int l_frame=formats[ctx->format].input_frame_size;
> +
> +    if(buf_size<l_frame)
> +        return AVERROR(EIO);
> +
> +    init_get_bits(&gb, buf, buf_size);
> +
> +    parm[0]=get_bits(&gb, L0_BITS); //L0
> +    parm[1]=get_bits(&gb, L1_BITS); //L1
> +    parm[2]=get_bits(&gb, L2_BITS); //L2
> +    parm[3]=get_bits(&gb, L3_BITS); //L3
> +    parm[4]=get_bits(&gb, P1_BITS); //P1
> +    parm[5]=get_bits(&gb, P0_BITS); //Parity
> +    parm[6]=get_bits(&gb, formats[ctx->format].fc_index_bits*FC_PULSE_COUNT+1); //C1
> +    parm[7]=get_bits(&gb, FC_PULSE_COUNT); //S1

> +    parm[8]=get_bits(&gb, GA_BITS); //GA1
> +    parm[9]=get_bits(&gb, GB_BITS); //GB1
> +    parm[10]=get_bits(&gb, P2_BITS); //P2

parm[ 8]=get_bits(&gb, GA_BITS); //GA1
parm[ 9]=get_bits(&gb, GB_BITS); //GB1
parm[10]=get_bits(&gb, P2_BITS); //P2

also parm should be a struct and this should look like
parm->cb_index[0]= get_bits(&gb, GA_CB_INDEX_BITS); //GA1
parm->cb_index[1]= get_bits(&gb, GB_CB_INDEX_BITS); //GB1
...

Or some other appropriate names, 2 letter names are not very understanable.


[...]
> +    int in_frame_size=formats[ctx->format].input_frame_size;
> +    int out_frame_size=formats[ctx->format].output_frame_size;

vertical align


> +    int i, ret;
> +    uint8_t *dst=data;
> +    const uint8_t *src=buf;
> +
> +    if (buf_size<in_frame_size)
> +        return AVERROR(EIO);
> +
> +    *data_size=0;

You are now checking the input buffer size, but what is really important is
the output buffer size, its in data_size and you just overwrite this, this is
not ok. You must check that te buffer is large enough for what you write in
there.


> +    for(i=0; i<buf_size; i+=in_frame_size)

Each packet the decoder receives should be 1 frame.


[...]

> +    *data_size=(dst-(uint8_t*)data);
> +    return (src-buf);

superflous ()

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Dictatorship naturally arises out of democracy, and the most aggravated
form of tyranny and slavery out of the most extreme liberty. -- 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/20080301/8316ad24/attachment.pgp>



More information about the ffmpeg-devel mailing list