[FFmpeg-devel] [PATCH] QCELP decoder
Kenan Gillet
kenan.gillet
Fri Oct 10 02:02:23 CEST 2008
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
More information about the ffmpeg-devel
mailing list