[Ffmpeg-devel] [PATCH] updated LGPL AC-3 decoder
Michael Niedermayer
michaelni
Thu Mar 8 03:57:01 CET 2007
Hi
On Sat, Feb 24, 2007 at 03:11:17AM -0500, Justin Ruggles wrote:
> Hello,
>
> Here is a much-modified patch of the AC-3 decoder. I basically rewrote
> about half the code. The 5.1 channel decoding still needs speeding-up,
> but everything is working properly now.
>
> This patch is of all changes against current SVN, with ac3.c being a
> copy of ac3enc.c. As noted before, this is meant to be applied in a
> logical sequence of commits, not all at once.
[...]
> +
> + /* audio info */
> + int sample_rate; ///< sample rate in Hz
> + int bit_rate; ///< bit rate in kbps
> + int frame_size; ///< frame size in bytes
> + int nfchans; ///< number of full-bandwidth channels
> + int nchans; ///< number of total channels, including LFE
> + int lfeon; ///< indicates if LFE channel is present
> + int lfe_channel; ///< index of LFE channel
> + int cpl_channel; ///< index of coupling channel
> + int blkoutput; ///< output configuration for block
> + int out_channels; ///< number of output channels
> + float downmix_scale_factors[AC3_MAX_CHANNELS]; ///< downmix gain for each channel
the "audio info" comment is not doxygen compaible, it should rather be
/** audio info */
//@{
int balh
int foo
int bar
//@}
IIRC the syntax correctly
[...]
> +static int16_t dither_int16(AVRandomState *state)
> +{
> + uint32_t y = av_random(state);
> + return (int16_t)(y ^ (y >> 16));
a simple
av_random(state) & 0xFFFF;
instead of dither_int16(state) will do too, its not optimal but the code above
is even slower
> +}
> +
> +/**
> + * Generates a Kaiser-Bessel Derived Window.
> + */
> +static void ac3_window_init(float *window)
> +{
> + int i, j;
> + double sum = 0.0, bessel, tmp;
> + double local_window[256];
> + double alpha2 = (5.0 * M_PI / 256.0) * (5.0 * M_PI / 256.0);
> +
> + for (i = 0; i < 256; i++) {
> + tmp = i * (256 - i) * alpha2;
> + bessel = 1.0;
> + for (j = 100; j > 0; j--) /* default to 100 iterations */
> + bessel = bessel * tmp / (j * j) + 1;
> + sum += bessel;
> + local_window[i] = sum;
> + }
> +
> + sum++;
> + for (i = 0; i < 256; i++)
> + window[i] = sqrt(local_window[i] / sum);
> +}
hmm this looks like a little redundant with respect to ac3_window[]
later could be inited by this too ...
[...]
> + /* generate dynamic range table */
> + for(i=0; i<256; i++) {
> + int v = i >> 5;
> + if(v > 3) v -= 7;
> + else v++;
> + dynrng_tbl[i] = powf(2.0f, v);
> + v = (i & 0x1F) | 0x20;
> + dynrng_tbl[i] *= v / 64.0f;
maybe the following gives the same value ...
int v= ((int8_t)i >> 5) + 1 - 6;
dynrng_tbl[i] = powf(2.0f, v);
dynrng_tbl[i] *= (i & 0x1F) | 0x20;
[...]
> +/**
> + * Parses the 'sync info' and 'bit stream info' from the AC-3 bitstream.
> + * GetBitContext within AC3DecodeContext must point to start of the
> + * synchronized AC-3 bitstream.
> + */
> +static int parse_header(AC3DecodeContext *ctx)
> +{
> + AC3HeaderInfo hdr;
> + GetBitContext *gb = &ctx->gb;
> + int err, i;
> +
> + err = ac3_parse_header(gb->buffer, &hdr);
> + if(err)
> + return err;
> +
> + /* get decoding parameters from header info */
> + ctx->crc1 = hdr.crc1;
> + ctx->fscod = hdr.fscod;
> + ctx->bit_alloc_params.fscod = hdr.fscod;
> + ctx->acmod = hdr.acmod;
> + ctx->cmixlev = hdr.cmixlev;
> + ctx->surmixlev = hdr.surmixlev;
> + ctx->lfeon = hdr.lfeon;
> + ctx->halfratecod = hdr.halfratecod;
> + ctx->bit_alloc_params.halfratecod = hdr.halfratecod;
> + ctx->sample_rate = hdr.sample_rate;
> + ctx->bit_rate = hdr.bit_rate / 1000;
> + ctx->nchans = hdr.channels;
> + ctx->nfchans = ctx->nchans - ctx->lfeon;
> + ctx->lfe_channel = ctx->nfchans;
> + ctx->cpl_channel = ctx->lfe_channel+1;
> + ctx->frame_size = hdr.frame_size;
do you see an easy way to avoid this copying between 2 structs? i mean
would adding AC3HeaderInfo into AC3DecodeContext be an option or would
that add too many ctx->foobar.blah ? if later then ive no objections
to the current code above, the copying might very well be the smaller
evil
[...]
> +/**
> + * Generates transform coefficients for each coupled channel in the coupling
> + * range using the coupling coefficients and coupling coordinates.
> + */
> +static void uncouple_channels(AC3DecodeContext *ctx)
> +{
> + int i, ch, end, bnd, subbnd;
> +
> + subbnd = 0;
> + for(i=ctx->cplstrtmant,bnd=0; i<ctx->cplendmant; bnd++) {
> + /* get last bin in coupling band using coupling band structure */
> + end = i + 12;
> + while (ctx->cplbndstrc[subbnd]) {
> + end += 12;
> + subbnd++;
> + }
> + subbnd++;
> +
> + /* calculate transform coeffs */
> + while(i < end) {
> + for(ch=0; ch<ctx->nfchans; ch++) {
> + if(ctx->chincpl[ch]) {
> + ctx->transform_coeffs[ch][i] = ctx->cpl_coeffs[i] *
> + ctx->cplco[ch][bnd];
> + }
> + }
> + i++;
> + }
> + }
> +}
subbnd = 0;
for(i=ctx->cplstrtmant,bnd=0; i<ctx->cplendmant; bnd++) {
do{
for(j=0; j<12; j++){
for(ch=0; ch<ctx->nfchans; ch++) {
if(ctx->chincpl[ch]) {
ctx->transform_coeffs[ch][i] = ctx->cpl_coeffs[i] *
ctx->cplco[ch][bnd];
}
}
i++;
}
}while(ctx->cplbndstrc[subbnd++]);
}
> +
> +/* ungrouped mantissas */
> +typedef struct {
comment not doxygen compatible
[...]
> + do {
> + ctx->transform_coeffs[ch][end] = 0;
> + } while(++end < 256);
memset()
[...]
> +/**
> + * Downmixes the output.
> + * This function downmixes the output when the number of input
> + * channels is not equal to the number of output channels requested.
> + */
> +static void do_downmix(AC3DecodeContext *ctx)
could you split the whole downmix out into its own patch and file and make it
indenpandant of AC3? so it could be used by other similar codecs?
[...]
> + /* phase flags */
> + if (acmod == AC3_CHANNEL_MODE_STEREO && ctx->phsflginu &&
> + (ctx->cplcoe & 1 || ctx->cplcoe & 2)) {
ctx->cplcoe & 3
[...]
> + /* coupling and channel exponent strategies */
> + got_cplch = !ctx->cplinu;
> + for(ch=0; ch<nfchans; ch++) {
> + if(!got_cplch) {
> + ch = ctx->cpl_channel;
> + got_cplch = 1;
> + }
> + ctx->expstr[ch] = get_bits(gb, 2);
> + if(ctx->expstr[ch] != EXP_REUSE) {
> + ctx->bit_alloc_stages[ch] = 3;
> + }
> + if(ch == ctx->cpl_channel)
> + ch = -1;
> + }
> + /* lfe exponent strategy */
> + if(ctx->lfeon) {
> + ctx->expstr[ctx->lfe_channel] = get_bits1(gb);
> + if(ctx->expstr[ctx->lfe_channel] != EXP_REUSE) {
> + ctx->bit_alloc_stages[ctx->lfe_channel] = 3;
> + }
> + }
if(ctx->cplinu){
ctx->expstr[ctx->cpl_channel] = get_bits(gb, 2);
}
for(ch=0; ch<nfchans; ch++) {
ctx->expstr[ch] = get_bits(gb, 2);
}
if(ctx->lfeon) {
ctx->expstr[ctx->lfe_channel] = get_bits1(gb);
}
for(ch=0; ch<=AC3_MAX_CHANNELS; ch++){
if(ctx->expstr[ch] != EXP_REUSE)
ctx->bit_alloc_stages[ch] = 3;
}
[...]
> + /* fbw & lfe exponents */
> + for(ch=0; ch<ctx->nchans; ch++) {
> + if (ctx->expstr[ch] != EXP_REUSE) {
> + int ngrps = 2;
> + if(ch != ctx->lfe_channel) {
isnt ctx->lfe_channel == ctx->nchans ? if so ch will never be
== ctx->lfe_channel here i think
[...]
> + /* fine snr offsets and fast gain codes */
> + got_cplch = !ctx->cplinu;
> + for(ch=0; ch<ctx->nchans; ch++) {
> + if(!got_cplch) {
> + ch = ctx->cpl_channel;
> + got_cplch = 1;
> + }
> + ctx->fsnroffst[ch] = get_bits(gb, 4);
> + ctx->fgaincod[ch] = get_bits(gb, 3);
> + if(ch == ctx->cpl_channel)
> + ch = -1;
> + }
if(ctx->cplinu){
ctx->fsnroffst[ctx->cpl_channel] = get_bits(gb, 4);
ctx->fgaincod [ctx->cpl_channel] = get_bits(gb, 3);
}
for(ch=0; ch<ctx->nchans; ch++) {
ctx->fsnroffst[ch] = get_bits(gb, 4);
ctx->fgaincod [ch] = get_bits(gb, 3);
}
and actually if the channels where ordered so that cpl_channel is 0
then code like this could be simplified to
for(ch=ctx->cplinu; ch<=ctx->nchans; ch++) {
ctx->fsnroffst[ch] = get_bits(gb, 4);
ctx->fgaincod [ch] = get_bits(gb, 3);
}
and similar code seems common ...
[...]
> + /* interleave output */
> + for (k = 0; k < AC3_BLOCK_SIZE; k++) {
> + for (j = 0; j < avctx->channels; j++) {
> + *(out_samples++) = ctx->int_output[j][k];
> + }
can that extra copy be avoided? i mean directly convert the floats into
the final int16 array?
[...]
> -/* possible frequencies */
> +/** possible frequencies */
> -static const uint16_t ac3_freqs[3] = { 48000, 44100, 32000 };
> +const uint16_t ff_ac3_freqs[3] = { 48000, 44100, 32000 };
>
> -/* possible bitrates */
> +/** possible bitrates */
great, doxygenization is welcome but not as
part of a already >100kb patch which is about adding a ac3 decoder not
about cleaning up comments so feel free to apply such seperate changes
anytime :)
> -static const uint16_t ac3_bitratetab[19] = {
> +const uint16_t ff_ac3_bitratetab[19] = {
> 32, 40, 48, 56, 64, 80, 96, 112, 128,
> 160, 192, 224, 256, 320, 384, 448, 512, 576, 640
> };
>
> +/* acmod to number of channels */
> +const uint8_t ff_ac3_channels[8] = {
> + 2, 1, 2, 3, 3, 4, 4, 5
> +};
> +
> +/** possible frame sizes */
> +const uint16_t ff_ac3_frame_sizes[64][3] = {
> + { 64, 69, 96 },
> + { 64, 70, 96 },
> + { 80, 87, 120 },
> + { 80, 88, 120 },
> + { 96, 104, 144 },
> + { 96, 105, 144 },
> + { 112, 121, 168 },
> + { 112, 122, 168 },
> + { 128, 139, 192 },
> + { 128, 140, 192 },
> + { 160, 174, 240 },
> + { 160, 175, 240 },
> + { 192, 208, 288 },
> + { 192, 209, 288 },
> + { 224, 243, 336 },
> + { 224, 244, 336 },
> + { 256, 278, 384 },
> + { 256, 279, 384 },
> + { 320, 348, 480 },
> + { 320, 349, 480 },
> + { 384, 417, 576 },
> + { 384, 418, 576 },
> + { 448, 487, 672 },
> + { 448, 488, 672 },
> + { 512, 557, 768 },
> + { 512, 558, 768 },
> + { 640, 696, 960 },
> + { 640, 697, 960 },
> + { 768, 835, 1152 },
> + { 768, 836, 1152 },
> + { 896, 975, 1344 },
> + { 896, 976, 1344 },
> + { 1024, 1114, 1536 },
> + { 1024, 1115, 1536 },
> + { 1152, 1253, 1728 },
> + { 1152, 1254, 1728 },
> + { 1280, 1393, 1920 },
> + { 1280, 1394, 1920 },
> +};
moving these tables from parser.c to ac3tab.h is ok and can be applied
anytime as a seperate change (reduces the amount of code i have to review
in the next iteration of the ac3 decoder review cycle)
[...]
> -static const uint16_t fgaintab[8]= {
> +const uint16_t ff_fgaintab[8]= {
all static tab -> ff_tab changes are ok, they can be applied anytime
> 0x080, 0x100, 0x180, 0x200, 0x280, 0x300, 0x380, 0x400,
> };
>
> -static const uint8_t bndsz[50]={
> +static const uint8_t bndsz[50]= {
cosmetic
[...]
> +static inline void mix_mono_to_stereo(float output[AC3_MAX_CHANNELS][AC3_BLOCK_SIZE])
> +{
> + int i;
> + for(i=0; i<AC3_BLOCK_SIZE; i++) {
> + output[1][i] = output[0][i];
> + }
memcpy()
[...]
> /* exponent mapping to PSD */
> - for(bin=start;bin<end;bin++) {
> - psd[bin]=(3072 - (exp[bin] << 7));
> + for(i=start; i<end; i++) {
> + psd[i] = 3072 - (exp[i] << 7);
> }
while i like the new code above better it is a cosmetic change and must be
commited seperately if you want to use i instead of bin (and yes you can
commit this anytime)
[...]
> +void ac3_bit_allocation_calc_mask(AC3BitAllocParameters *s, int16_t *mask,
> + int16_t *bndpsd, int start, int end,
> + int fgain, int is_lfe,
> + int deltbae, int deltnseg,
> + uint8_t *deltoffst, uint8_t *deltlen,
> + uint8_t *deltba)
static or ff_ prefix, the same applies to all new functions which are non
static and dont have a av_ or ff_ prefix
[...]
> - lowcomp = calc_lowcomp1(lowcomp, bndpsd[0], bndpsd[1]) ;
> - excite[0] = bndpsd[0] - fgain - lowcomp ;
> - lowcomp = calc_lowcomp1(lowcomp, bndpsd[1], bndpsd[2]) ;
> - excite[1] = bndpsd[1] - fgain - lowcomp ;
> - begin = 7 ;
> + lowcomp = calc_lowcomp1(lowcomp, bndpsd[0], bndpsd[1], 384);
> + excite[0] = bndpsd[0] - fgain - lowcomp;
> + lowcomp = calc_lowcomp1(lowcomp, bndpsd[1], bndpsd[2], 384);
> + excite[1] = bndpsd[1] - fgain - lowcomp;
> + begin = 7;
while iam happy with X ; -> X; it is a cosmetic change and should be done
seperately, that is feel free to commit such cleanup anytime just not
as part of a ac3 decoder patch or commit
[...]
> @@ -803,755 +226,58 @@
> for(i=0;i<50;i++) {
> bndtab[i] = l;
> v = bndsz[i];
> - for(j=0;j<v;j++) masktab[k++]=i;
> + for(j=0; j<v; j++) masktab[k++]=i;
cosmetic
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
There will always be a question for which you do not know the correct awnser.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070308/e2cc6b39/attachment.pgp>
More information about the ffmpeg-devel
mailing list