[FFmpeg-devel] [PATCH] Common ACELP code & G.729 [4/7] - G.729 core

[Michael Niedermayer]

On Sun, Aug 17, 2008 at 05:42:33PM +0700, Vladimir Voroshilov wrote:
[...]

> +/**
> + * maximum gain pitch value (3.8, Equation 47)
> + * 0.7945 in Q14
> + * (EE) This does not comply with the specification.
> + */
> +#define SHARP_MAX                  13017

what does not comply with the spec? This should be clearly documented


[...]

> +typedef struct
> +{
> +    int sample_rate;

> +    uint8_t input_frame_size; ///< input frame size(in bytes)
> +    uint8_t output_frame_size;///< output frame size (in bytes)

input/output are unwisely choosen names once a encoder exists ...


> +    uint8_t ac_index_1st_bits;///< adaptive codebook index for first subframe (size in bits)
> +    uint8_t ac_index_2nd_bits;///< adaptive codebook index for second subframe (size in bits)
> +    uint8_t parity_bits;      ///< parity bit for pitch delay (size in bits)
> +    uint8_t gc_1st_index_bits;///< gain codebook (first stage) index (size in bits)
> +    uint8_t gc_2nd_index_bits;///< gain codebook (second stage) index (size in bits)
> +    uint8_t fc_signs_bits;    ///< number of pulses in fixed-codebook vector
> +    uint8_t fc_indexes_bits;  ///< size (in bits) of fixed-codebook index entry
> +    /// mr_energy = mean_energy + 10 * log10(2^26  * subframe_size) in (7.13)
> +    int mr_energy;
> +} G729_format_description;

as this is just used in the .c file it does not need to be in the header
Also please move ALL other things from g729 headers into the corresponding
.c files when they are ot used outside of a single c file.


> +

> +enum {
> +    FORMAT_G729_8K = 0,
> +    FORMAT_G729_4K4,
> +};

the enum should have a name and that should be used instead of int.


[...]
> +static const G729_format_description formats[] =
> +{
> +  {8000, 10, 160, 8, 5, 1, GC_1ST_IDX_BITS_8K,  GC_2ND_IDX_BITS_8K,  4, 13, 0xf892c,},
> +// Note: may not work
> +  {4400, 11, 176, 8, 5, 1, GC_1ST_IDX_BITS_8K,  GC_2ND_IDX_BITS_8K,  4, 17, 0xf966b,},
> +};

The values that are the same between both do not need to be stored here


[...]
> +    if(!formats[ctx->format].parity_bits)
> +        ctx->bad_pitch = 0;
> +    else
> +        ctx->bad_pitch = g729_get_parity(parm->ac_index[0]) == parm->parity;

if(x)
else

is preferable over

if(!x)
else

as its simpler and IMHO more readable


[...]

> +
> +        if(ctx->frame_erasure)
> +        {
> +            /*
> +                Decoding of the adaptive and fixed-codebook gains
> +                from previous subframe (4.4.2)
> +            */
> +
> +            /* 4.4.2, equation 94 */
> +            ctx->gain_pitch = FFMIN((29491 * ctx->gain_pitch) >> 15, 29491); // 0.9 (0.15)
> +
> +            /* 4.4.2, equation 93 */
> +            ctx->gain_code  = (8028 * ctx->gain_code) >> 13; // 0.98 in (2.13)

I think ive said it many times already could you PLEASE REMOVE these
useless comments
This IS a requirement for accepting the patch! i would like _ZERO_
references to the spec inside functions, you can add a SINGLE reference
to the spec before each function.

This should look like:

if(ctx->frame_erasure){
    ctx->gain_pitch = FFMIN((29491 * ctx->gain_pitch) >> 15, 29491);
    ctx->gain_code  =       ( 2007 * ctx->gain_code ) >> 11;
}

and this is not
"Decoding of the adaptive and fixed-codebook gains from previous subframe"
it is REUSING attenuated values from the previous subframe

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

While the State exists there can be no freedom; when there is freedom there
will be no State. -- Vladimir Lenin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080821/98572184/attachment.pgp>