[FFmpeg-devel] [PATCH] QCELP decoder

[Kenan Gillet]

On Oct 4, 2008, at 4:38 AM, Vitor Sessak wrote:

> 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.
>

done

> +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?

I think the double precision is needed to hold several values like  
-9.251384E-2
coming from the specs, but it is borderline.
I tested the changes to float with tiny_psnr: it does not seem to  
change the
ouput, because
- it is a rare case (frame is 1/4)
- the output is clipped

>
>
>> +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
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at mplayerhq.hu
> https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel