[FFmpeg-devel] [PATCH] QCELP decoder

[Kenan Gillet]

On Oct 9, 2008, at 2:59 PM, Diego Biurrun wrote:

> On Sun, Oct 05, 2008 at 09:49:59AM -0700, Kenan Gillet wrote:
>> thank you for your reviewing. Here is a round3 of he decoder.
>
> Build system part is OK, documentation and changelog updates are
> missing.

added in doc/general.texi and Changelog


>> --- libavcodec/qcelpdata.h	(revision 0)
>> +++ libavcodec/qcelpdata.h	(revision 0)
>> @@ -0,0 +1,398 @@
>> + * lspv[10]  60    61 62 63 64 65 66 67 68 69 (LSP for  
>> RATE_OCTAVE, LSPV for other rate)
>
> I think you need to say 'rateS' here.

done

>
>
>> + * What follows are the reference frame slices. Each tuple will be  
>> mapped
>> + * to a QCELPBitmap showing the location of each bit in the input  
>> with respect
>> + * to a transmission code in the 'universal frame'.
>
> nit: needlessly long line

done


>> +/**
>> + * LSP Vector quantization tables in x*10000 form
>
> vector

done


>> --- libavcodec/qcelpdec.c	(revision 0)
>> +++ libavcodec/qcelpdec.c	(revision 0)
>> @@ -0,0 +1,812 @@
>> +    uint8_t           lspv[10];               /*!< LSP for  
>> RATE_OCTAVE, LSPV for other rate */
>
> rateS, see above

done


>> +/**
>> +* Verify the samplerate and the number of channels.
>> +* Initialize the speech codec to the specs.
>
> spec or specs?

replaced by specification




> And I think you mean "according to" instead of "to".

yes



>> +static int qcelp_decode_init(AVCodecContext *avctx) {
>> +    QCELPContext *q = avctx->priv_data;
>> +    int          i;
>
> I think there is no need to align the variable names here.

done


>> +    for (i = 0; i < 10; i++) {
>> +        q->prev_lspf[i] = (i + 1) / 11.;
>> +    }
>
> useless {}
>
>> +/**
>> + * Decodes the 10 quantized LSP frequencies from the LSPV/LSP
>> + * transmission codes of any framerate and check for badly
>> + * received packets.
>
> nit: This can be shortened to two lines.
>
>> +    } else if (q->framerate == I_F_Q) {
>> +        predictor = QCELP_LSP_OCTAVE_PREDICTOR * - 
>> QCELP_LSP_SPREAD_FACTOR;
>> +        if (q->erasure_count > 1) {
>> +            predictor *= (q->erasure_count < 4 ? 0.9 : 0.7);
>> +        }
>> +        for (i = 0; i < 10; i++) {
>> +            lspf[i] = (i + 1) / 11. + predictor;
>> +        }
>
> more pointless {}
>
> There are more cases below.  In FFmpeg generally the form without {}  
> is
> preferred.

done, removed
>
>
>> +static int decode_gain_and_index(QCELPContext  *q, float *gain) {
>> +    int           i, g1[16];
>> +    float         ga[16], gain_memory, smooth_coef;
>
> strange alignment

done


>> +        for (i = 0; i < 16; i++) {
>> +            if (q->framerate == RATE_HALF && i>=4) break;
>
> I dislike putting the statement right after the if instead of on a new
> line, it makes the code harder to read.

done



>> + * Computes energy of the subframeno-ith subvector. These values are
>
> the energy

done


>> + * @param vector vector from which to measure the subframe's energy
>
> vector to measure the subframe energy of

done

> There should be better ways to say this.



>
>
>> + * @param q if not null, apply harcoded coefficient infinite  
>> impulse response filter
>
> harDcoded
doxygen comments removed because  q is not passed anymore.

>
>
>> + * @param in vector to control gain off
>
> of

done


>> + * @param out gain-controled output vector
>
> controlled

done


>> + * @param v_in the input vector of the filter
>> + * @param v_out the output 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]) {
>
> The order of the last two parameters is swapped wrt the function  
> signature.

done


>> +    float hamm_tmp;
>
> hamming?
>
>> + * @param q the context
>> + * @param cdn_vector the scaled codebook vector
>> + * @param ppf_vector array where the result of the filter will be  
>> stored
>> + *
>> + * @return 0 if everything goes well, -1 if frame should be erased
>> + */
>> +static int apply_pitch_filters(QCELPContext *q, float *cdn_vector) {
>
> ppf_vector?

removed


>> + * @param cos
>
> ?



>
>
>> +void interpolate_lpc(QCELPContext *q, const float *curr_lspf,  
>> float *lpc, const int subframe_num) {
>
> Sometimes you break long function declarations, sometimes you do not.
>
>> +/**
>> + * formant synthesis filter
>> + *
>> + * TIA/EIA/IS-733 2.4.3.1 (NOOOOT)
>> + *
>> + * @param vector in/out vector of the formant filter
>> + */
>> +static void do_formant(float *vector, const float *lpc, float  
>> *memory) {
>
> missing parameter documentation

done


>> +        av_log(avctx, AV_LOG_WARNING,
>> +               "Framerate byte is missing, guessing the framerate  
>> from packet size.\n");
>
> from the
>
>> + * @param cnd_vector array where to put the generated scaled  
>> codebook vector
>
> array for the generated scaled codebook vector

done


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

done


>> +        warn_insufficient_frame_quality(avctx, "cannot initiliaze  
>> pitch filter.");
>
> initialize

done


> Quickly running your files through a spellchecker will likely find  
> more
> of these..
>

done

thanks