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

[Vladimir Voroshilov]

Hi, Michael.
Thanks for quick answer.

On Sun, Apr 20, 2008 at 5:03 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
> On Sun, Apr 20, 2008 at 12:41:26AM +0700, Vladimir Voroshilov wrote:
>  > Hi, All again
>  >

[...]

>  > 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.

I hope, lookup tables and build api can be separated.
Otherwise i can't create uncompressed <100k patch.

>  [...]
>  > +#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

Inlined

>  > +#define MIN_GPLT    21845      /* LT gain minimum (Q15)                     */
>  > +
>  > +/**
>  > + * Fractional delay resolutionb
>  > + */
>  > +#define F_UP_PST    8
>
>  these 2 could have more descriptive names

Renamed and clarified.

>  > +
>  > +/**
>  > + * 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

Renamed and clarified

>  > +
>  > +#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

Changed to IDX

>  > +#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

Renamed

I hope attached files are those which you want to see.

-- 
Regards,
Vladimir Voroshilov     mailto:voroshil at gmail.com
JID: voroshil at gmail.com, voroshil at jabber.ru
ICQ: 95587719
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: g729_build_16.diff
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080420/563d2cb9/attachment.asc>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: g729tab_16.diff.gz
Type: application/x-gzip
Size: 9783 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080420/563d2cb9/attachment.bin>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: g729dec_16.diff
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080420/563d2cb9/attachment-0001.asc>