[FFmpeg-devel] [PATCH] QCELP decoder

[Kenan Gillet]

On Oct 3, 2008, at 4:38 PM, Diego Biurrun wrote:

> On Fri, Oct 03, 2008 at 03:48:52PM -0700, Kenan Gillet wrote:
>>
>> here is a round 2 of the patch based on feedback from Vitor and  
>> Diego.
>>
>> --- libavcodec/qcelpdata.h	(revision 0)
>> +++ libavcodec/qcelpdata.h	(revision 0)
>> @@ -0,0 +1,397 @@
>> +/**
>> + * position of the bitmapping data for each pkt type in
>> + * the big REFERENCE FRAME array
>
> If 'pkt' means packet, then please write just packet.
>
done

>> --- libavcodec/qcelpdec.c	(revision 0)
>> +++ libavcodec/qcelpdec.c	(revision 0)
>> @@ -0,0 +1,867 @@
>> +
>> +    uint8_t           data[77];               /*!< Data from a  
>> _parsed_ frame */
>
> lowercase
done


>> +    float             prev_iirf_scalefactor;  /*!< Last gain scale  
>> factor from the previous
>> +                                                   frame (TIA/EIA/ 
>> IS-733 2.4.8.6-6) */
>
> ditto
done



>> +static void interpolate_lspf(float *interpolated_lspf, const float  
>> curr_weight,
>> +                             const float *curr_lspf, const float  
>> *prev_lspf) {
>> +    int   i;
>> +
>> +    for (i=0;i<10;i++)
>
> That for expression is in dire need of whitespace.
done, for all for loop


>> +* Verify the sample rate and the number of channels.
>
> samplerate
done


>> +                   "Unofficial samplerate %d, but supported by  
>> Quicktime.", avctx->sample_rate);
>
> QuickTime
done


>> +    switch (q->rate) {
>> +        case RATE_FULL:
>
> I think the preferred indentation style is to keep case on the same
> level as switch.
done


>> +            // Provide smoothing of the energy of the unvoiced  
>> excitation
>
> Provide smoothing of the unvoiced excitation energy
done


>> +/**
>> + * Computes the scaled codebook vector Cdn From INDEX and GAIN
>> + * For all rates.
>> + *
>> + * The specification misses some information here.
>
> is missing / lacks
done


>> + * @param subframeno Size 40 subframe number that should be measured
>
> lowercase
done


>> + * @param q if not null, apply harcoded coef infinite impulse  
>> response filter
>
> coefficient
done


>> +/**
>> + * Apply pitch synthetis filter and pitch pre-filter to the scaled  
>> book vector.
>
> book vector?
codebook vector


>> + * @param ppf_vector an array where the result of the filter will  
>> be stored in.
>
> s/an array/array/, s/stored in./stored/
done


>> + * Reconstructs LPC coefficients from the line spectral pairs  
>> frequencies.
>
> I think this should be 'pair'.
done



>> +/**
>> + * Interpolates LSP frequencies and computes LPC coefficients for  
>> a given rate & pitch subframe.
>
> Please break unnecessarily long lines like this one.
done, in other places too


>> + * @param QCELPContext q the context
>
> stray q?
would qctx be better ?


>> +/**
>> + * Formant synthesis filter
>> + *
>> + * @param vector in/out vector of the formant filter
>
> Settle for the capitalized or uncapitalized version of Formant.
done, uncapitalized


>> +/*
>> + * Figure out and set it in QCELPContext frame rate by its buffer  
>> size and/or by looking at
>> + * the first byte of its buffer if applicable.
>
> What does the first 'it' refer to?  I find this explanation very
> confusing.
what about

Determine the framerate from the frame size and/or the first byte of  
the frame.

otherwise, we can just rename the function determine_framerate and  
drop the comment.


>
>
> Also, settle for a consistent spelling of framerate.
done, settle for framerate

>
>
>> + * @return 0 if success, negative error number otherwise.
>
> on success
done



>> +/**
>> + * Decodes the scaled codebook vector Cdn.
>> + *
>> + * @param cnd_vector Array where to put the generated scaled  
>> codebook vector
>
> array
done


>> + * @return 0  if successful, -1 if the frame must be erased
>
> on success
done

thanks for the feedback.

Kenan