[FFmpeg-devel] [PATCH] QCELP decoder

[Kenan Gillet]

Hi,
On Oct 16, 2008, at 8:34 AM, Diego Biurrun wrote:

> On Tue, Oct 14, 2008 at 06:59:34PM -0700, Kenan Gillet wrote:
>>
>> Here is a summary of the round 4 patch:
>
> some more comments, mostly nits...

thanks for your review.


>> --- libavcodec/qcelpdata.h	(revision 0)
>> +++ libavcodec/qcelpdata.h	(revision 0)
>> +
>> +#ifndef AVCODEC_QCELPDATA_H
>> +#define AVCODEC_QCELPDATA_H
>> +
>> +static const uint16_t qcelp_bits_per_rate[6] = {
>
> You will need to #include at least stdint.h for this file to pass  
> 'make
> checkheaders'.  Please test your code with 'make checkheaders'.

done


>> + * YOU WILL NOT SEE ANY mention of a REFERENCE nor an UNIVERSAL  
>> frame
>> + * in the specs, this is just some internal way of handling the
>
> Split this sentence in two at the comma.

this whole comment has been removed after cleanup asked
by Michael Niedermayer.
>




>> + * each bit in the input with respect  to a transmission code in the
>
> nit: two spaces

same as above.


>> +    {19,3},{19,2},{19,1},{19,0}
>> +};
>> +static const QCELPBitmap qcelp_rate_4thr_bitmap[] = {
>> +static const QCELPBitmap qcelp_rate_4thr_bitmap[] = {
>
> 4thr?

renamed qcelp_rate_4thr_bitmap to qcelp_rate_quarter_bitmap
renamed qcelp_rate_8thr_bitmap to qcelp_rate_octave_bitmap


> nit: You left a blank line between the other tables, do it here as  
> well.

done



>> --- libavcodec/qcelpdec.c	(revision 0)
>> +++ libavcodec/qcelpdec.c	(revision 0)
>> +        // Low-pass filter the LSP frequencies
>
> .

done


>> +        // Check for badly received packets
>
> .

done


>> +        gain[0] =     ga[0];
>> +        gain[1] = 0.6*ga[0]+0.4*ga[1];
>> +        gain[2] =     ga[1];
>> +        gain[3] = 0.2*ga[1]+0.8*ga[2];
>> +        gain[4] = 0.8*ga[2]+0.2*ga[3];
>> +        gain[5] =     ga[3];
>> +        gain[6] = 0.4*ga[3]+0.6*ga[4];
>> +        gain[7] =     ga[4];
>
> A few spaces between operators would make this look less cramped.

done space around +
>
>
>> + * The reason for this mistake seems to be the fact they forget to  
>> mention you
>
> forgot

done


>> + * @param cnd_vector array for the generated scaled codebook vector
>> + */
>> +static void compute_svector(const QCELPContext *q, const float  
>> *gain, float *cdn_vector) {
>
> cdn_vector or cnd_vector?

done


>> + * @param gain array of gain per subframe, each element is between  
>> 0.0 and 2.0
>
> per-subframe gain array

done


>> + * @param lag array of lag per subframe, each element is
>
> per-subframe lag array

done


>> + * @param pfrac array of boolean per subframe, 1 if the lag is  
>> fractional, 0 otherwise
>
> per-subframe boolean array

done


>> + * @param v_out the output vector of the filter
>
> filter output vector

done


>> + * @param v_in the input vector of the filter
>
> filter input vector

done


>> +/**
>> + * Apply pitch synthesis filter and pitch pre-filter to the scaled  
>> codebook vector.
>
> I would say 'prefilter', same in other places.

done


>> +            // Compute Gain & Lag for the whole frame
>
> .

done


> gain, lag

done


>> +            memset(q->pfrac, 0, 4 *sizeof(*q->pfrac));
>> +            memcpy(q->plag, q->prev_pitch_lag, 4 * sizeof(*q- 
>> >plag));
>
> memset(q->pfrac,                0, 4 * sizeof(*q->pfrac));
> memcpy(q->plag, q->prev_pitch_lag, 4 * sizeof(*q->plag));
>
>> + * @param memory the previous state of the filter
>
> s/the//

done


>> +        av_log(avctx, AV_LOG_ERROR, "frame #%d: unknown framerate,  
>> unsupported size: %d\n",
>> +        warn_insufficient_frame_quality(avctx, "framerate is 1/8  
>> and first 16 bits are on.");
>> +            warn_insufficient_frame_quality(avctx, "wrong data in  
>> reserved frame area.");
>> +        warn_insufficient_frame_quality(avctx, "sanity check of  
>> the codebook gain failed.");
>> +        warn_insufficient_frame_quality(avctx, "badly received  
>> packets in frame.");
>> +        warn_insufficient_frame_quality(avctx, "cannot initialize  
>> pitch filter.");
>
> Please capitalize all these messages.

done

>
>
>> +        // WIP Adaptive postfilter should be here
>
> adaptive

done


>> +    .long_name = NULL_IF_CONFIG_SMALL("QCLEP / PureVoice"),
>
> QCELP

done


>> --- doc/general.texi	(revision 15618)
>> +++ doc/general.texi	(working copy)
>> @@ -388,6 +388,7 @@
>> @item Nellymoser ASAO        @tab  X  @tab  X
>> + at item QCELP / PureVoice      @tab  X  @tab  X
>
> Ummm, I don't think you have implemented an encoder as well, so just  
> put
> an X in the decoder column.

done

Kenan