[FFmpeg-devel] [PATCH] QCELP decoder

[Kenan Gillet]

On Oct 30, 2008, at 2:59 AM, Diego Biurrun wrote:

> On Tue, Oct 28, 2008 at 04:47:02PM -0700, Kenan Gillet wrote:
>> On Tue, Oct 28, 2008 at 4:43 PM, Kenan Gillet  
>> <kenan.gillet at gmail.com> wrote:
>>> thanks Diego and Michael for your reviewing.
>>>
>>> And here is an updated set of patches with a few changes too.
>>>
>>> round 7 summary:
>>> - qcelp-round7-doc-glue.patch.txt has already been oked
>>> - grammar / spelling / cosmetics
>>> - generalize convolve function used in qcelp_lspf2lpc
>>> - remove some multiplication and division in compute_svector
>>> - frame unpacking rework
>>> - most of the previous comments
>>>
>>
>> forgot one patch in previous mail :(
>>
>> --- libavcodec/qcelpdata.h	(revision 0)
>> +++ libavcodec/qcelpdata.h	(revision 0)
>> @@ -0,0 +1,541 @@
>> +/**
>> + * pre-calculated table for hammsinc function
>> + * Only half of the tables is needed because of symetry.
>
> table or tables?
>
> syMMetry

done


>> +    uint8_t bitpos; /*!< position of the lowet bit in the value's  
>> byte */
>
> loweSt

done


>> +/**
>> + * the upper boundary of the clipping, depends on
>> + */
>> +#define QCELP_CLIP_UPPER_BOUND (8191.75/8192.)
>
> Depends on what?

done,
depends on QCELP_SCALE


>> --- libavcodec/qcelpdec.c	(revision 0)
>> +++ libavcodec/qcelpdec.c	(revision 0)
>> @@ -0,0 +1,723 @@
>> +/**
>> + * Decodes the 10 quantized LSP frequencies from the LSPV/LSP
>> + * transmission codes of any framerate and check for badly  
>> received packets.
>
> checks

done


>> +/*
>> + * Determine the framerate from the frame size and/or the first  
>> byte of the frame.
>> + *
>> + * @return 0 on success, negative error number otherwise.
>> + */
>> +static int determine_framerate(AVCodecContext *avctx,
>> +                               QCELPContext *q,
>> +                               const int buf_size,
>> +                               uint8_t **buf) {
>
> What rule do you follow when it comes to documenting parameters or  
> not?

No firm rule, for now on if I document one parameter for a function,  
I'll document the others.
Or, o you have a better suggestion?


>> +        warn_insufficient_frame_quality(avctx, "Sanity check of  
>> the codebook gain failed.");
>
> Codebook gain sanity check failed.

done


>> --- Changelog	(revision 15692)
>> +++ Changelog	(working copy)
>> @@ -138,6 +138,7 @@
>> - liba52 wrapper removed
>> +- QCELP / PureVoice decoder
>
> This has since changed.

done

>> --- libavcodec/Makefile	(revision 15692)
>> +++ libavcodec/Makefile	(working copy)
>> @@ -152,6 +152,7 @@
>> OBJS-$(CONFIG_PPM_ENCODER)             += pnmenc.o pnm.o
>> OBJS-$(CONFIG_PTX_DECODER)             += ptx.o
>> +OBJS-$(CONFIG_QCELP_DECODER)           += qcelpdec.o qcelp_lsp.o  
>> celp_math.o celp_filters.o
>> OBJS-$(CONFIG_QDM2_DECODER)            += qdm2.o mdct.o  
>> mpegaudiodec.o mpegaudiodecheader.o mpegaudio.o mpegaudiodata.o
>
> ditto

done


>> --- libavcodec/celp_filters.c	(revision 15692)
>> +++ libavcodec/celp_filters.c	(working copy)
>> @@ -84,3 +84,24 @@
>>
>> +    for(n=0; n<buffer_length; n++)
>> +    {
>
> You put the opening brace on the same line in other places.  And the  
> for
> expression could use some whitespace.

done

>> +        out[n] = in[n];
>> +        for(i=1; i<filter_length; i++)
>
> ditto

done


>> --- libavcodec/celp_math.c	(revision 15692)
>> +++ libavcodec/celp_math.c	(working copy)
>> @@ -195,3 +195,14 @@
>>
>> +    for(i=0; i<length; i++)
>
> Some more whitespace will earn you extra good karma here.

done

Kenan