[FFmpeg-devel] [PATCH] G.729 and G.729D decoders

[Michael Niedermayer]

On Sun, Apr 20, 2008 at 12:41:26AM +0700, Vladimir Voroshilov wrote:
> Hi, All again
> 
> Here is current version of G.729 decoder.
> 
> 1  G.729A (reduced complexity decoder) was replaced with full G.729
> (G.729 @8kHz)
> 2. G.729D (G.729 @6.4kHz) is implemented (is dynamic switching between
> 8k and 6.4k mode required?)
> 3. Code was splitted into three parts: header, main body and lookup tables.
> 4. All unnecessary clippings were removed (the rest are required for
> passing ITU's tests).
> 5. Decoder passes ITU tests (both G.729 and G.729D)
> 
> I tried also to fix all issues mentioned by Michael.
> 
> g729h_15.diff - header
> g729dec_15.diff.gz - decoder
> g729tab_15.diff.gz - lookup tables
> g729_build_15.diff - build part
> 
> P.S. Files are gzipped due to overall size >80k

quick review of the non gziped parts is below
i will review the rest as soon as i see a non compressed patch <100k
which is selfcontained, that is it is usefull as it is, a header alone
is not a selfcontanied patch it is useless as it is.


[...]
> +#define Q15_MAX 0x3fffffff
> +#define Q15_MIN (-Q15_MAX-1)
> +#define Q15_Q0_ROUND(a) (((a)+0x4000) >> 15)
> +
> +#define Q14_MAX 0x1fffffff
> +#define Q14_MIN (-Q14_MAX-1)
> +#define Q14_Q0_ROUND(a) (((a)+0x2000) >> 14)
> +
> +#define Q13_MAX 0xfffffff
> +#define Q13_MIN (-Q13_MAX-1)
> +
> +#define Q12_MAX 0x7ffffff
> +#define Q12_MIN (-Q12_MAX-1)
> +#define Q12_Q0_ROUND(a) (((a)+0x800) >> 12)

I would just write the constants in the code, i mean
0x3fffffff is clear Q15_MAX is something one has to look up
the same is true for MIN and ROUND
if you dont like 0x3fffffff, you could always write (1<<30)-1


[...]
> +#define MIN_GPLT    21845      /* LT gain minimum (Q15)                     */
> +
> +/**
> + * Fractional delay resolution
> + */
> +#define F_UP_PST    8

these 2 could have more descriptive names


> +
> +/**
> + * Short interpolation filter length
> + */
> +#define SHORT_INT_FILT_LEN       4
> +
> +/**
> + * Long interpolation filter length
> + */
> +#define LONG_INT_FILT_LEN       16
> +
> +/**
> + * Maximum size of one subframe over supported formats
> + */
> +#define MAX_SUBFRAME_SIZE 44

> +#define MEM_RES2 (PITCH_LAG_MAX+9)

and that is what? Please add a comment or improve the name


> +
> +#define MA_PREDICTOR_BITS   1  ///< switched MA predictor of LSP quantizer (size in bits)
> +#define VQ_1ST_BITS         7  ///< first stage vector of quantizer (size in bits)
> +#define VQ_2ND_BITS         5  ///< second stage vector of quantizer (size in bits)
> +

> +#define AC_IND_1ST_BITS_8K  8  ///< adaptive codebook index for first subframe, 8k mode (size in bits)
> +#define AC_IND_2ND_BITS_8K  5  ///< adaptive codebook index for second subframe, 8k mode (size in bits)

index is generally abbreviated as idx or ndx


> +#define GC_1ST_IND_BITS_8K  3  ///< gain codebook (first stage) index, 8k mode (size in bits)
> +#define GC_2ND_IND_BITS_8K  4  ///< gain codebook (second stage) index, 8k mode (size in bits)
> +#define FC_SIGNS_BITS_8K    4  ///< fixed codebook signs, 8k mode (size in bits)
> +#define FC_INDEXES_BITS_8K 13  ///< fixed codebook indexes, 8k mode (size in bits)
> +

> +#define AC_IND_1ST_BITS_64  8  ///< adaptive codebook index for first subframe, 6.4k mode (size in bits)

i would abbreviate 6.4k as 6K4 not 64


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

I have often repented speaking, but never of holding my tongue.
-- Xenocrates
-------------- 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/20080420/4ca84d30/attachment.pgp>