[FFmpeg-devel] [PATCH] updated LGPL AC-3 decoder

Michael Niedermayer michaelni
Fri May 4 00:16:06 CEST 2007


Hi

On Thu, May 03, 2007 at 12:42:09AM -0400, Justin Ruggles wrote:
> Hi,
> 
> First of all, I don't have the original emails for this thread, so I'm
> starting a new thread with the same name...sorry.
> 
> Here is an update of the AC-3 decoder patch.  I've included most of
> Michael's suggestions.  Also, the downmixing is much simpler now.
> 
> I think this should be ready for merging after another review.  A
> codec-independent channel reordering and downmixing patch can always be
> added later for more advanced mixing.  For now, the downmixing
> functionality mirrors that of the liba52 and dca decoders.
[...]

> +/** adjustments in dB gain */
> +#define LEVEL_ZERO              (0.0000000000000000f)
> +#define LEVEL_ONE               (1.0000000000000000f)
> +#define LEVEL_MINUS_3DB         (0.7071067811865476f) /* sqrt(2)/2 */
> +#define LEVEL_MINUS_4POINT5DB   (0.5946035575013605f)
> +#define LEVEL_MINUS_6DB         (0.5000000000000000f)
> +#define LEVEL_MINUS_9DB         (0.3548133892335755f)

are you sure these are correct?

LEVEL_MINUS_xDB 2^(-x/6)
matches all except LEVEL_MINUS_9DB


[...]

> +static inline float
> +symmetric_dequant(int code, int levels)
> +{
> +    float m = (code - (levels >> 1)) << 1;
> +    return (m / levels);
> +}

as level is always a constant this could be simplified to

return (code - (levels >> 1)) * (2.0/levels);

gcc should here be able to change levels >> 1 and 2.0/levels to
constants


> +
> +static void ac3_decoder_tables_init(void)
> +{
> +    int i;
> +
> +    /* generate mantissa tables */
> +    for(i=0; i<32; i++) {
> +        b1_mantissas[i][0] = symmetric_dequant( i / 9     , 3);
> +        b1_mantissas[i][1] = symmetric_dequant((i % 9) / 3, 3);
> +        b1_mantissas[i][2] = symmetric_dequant((i % 9) % 3, 3);
> +    }
> +    for(i=0; i<128; i++) {
> +        b2_mantissas[i][0] = symmetric_dequant( i / 25     , 5);
> +        b2_mantissas[i][1] = symmetric_dequant((i % 25) / 5, 5);
> +        b2_mantissas[i][2] = symmetric_dequant((i % 25) % 5, 5);
> +    }
> +    for(i=0; i<128; i++) {
> +        b4_mantissas[i][0] = symmetric_dequant(i / 11, 11);
> +        b4_mantissas[i][1] = symmetric_dequant(i % 11, 11);
> +    }
> +
> +    /* 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;
> +    }
> +
> +    /* generate scale factors */
> +    for(i=0; i<25; i++)
> +        scale_factors[i] = pow(2.0, -i);
> +
> +    /* generate exponent tables */
> +    for(i=0; i<128; i++) {
> +        exp_ungroup_tbl[i][0] =  i / 25;
> +        exp_ungroup_tbl[i][1] = (i % 25) / 5;
> +        exp_ungroup_tbl[i][2] = (i % 25) % 5;
> +    }

all the loops with 128 iteratons could be merged though iam unsure if this
would be a good idea, maybe its clearer if its left as it is


[...]
> +/**
> + * 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;
> +    float cmix, smix, smix1r;
> +
> +    err = ff_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;
> +    ctx->output_mode = nfchans_tbl[ctx->acmod];

this could be vertically aligned like:

ctx->crc1                   = hdr.crc1;
ctx->fscod                  = hdr.fscod;
ctx->bit_alloc_params.fscod = hdr.fscod;
ctx->acmod                  = hdr.acmod;


[...]
> +static void do_bit_allocation(AC3DecodeContext *ctx, int ch)
> +{
> +    int start=0, end=0;
> +
> +    if(ch == ctx->cpl_channel) {
> +        start = ctx->cplstrtmant;
> +        end = ctx->cplendmant;
> +    } else {
> +        end = ctx->endmant[ch];
> +    }

iam tempted to suggest to set
ctx->endmant[ ctx->cpl_channel ] = ctx->cplendmant;
and then simplify the related code
whats your oppinion about that?


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

the if() is idented wrongly


[...]
> +        v0 = 0.0f;
> +        for(j=0; j<nfchans; j++) {
> +            v0 += samples[j][i] * coef[j][0];
> +        }
> +        v1 = 0.0f;
> +        for(j=0; j<nfchans; j++) {
> +            v1 += samples[j][i] * coef[j][1];
> +        }

the 2 loops can be merged


[...]
> +    /*block switch flag */

nitpick, missing space between * and b :)


> +    memset(ctx->blksw, 0, sizeof(ctx->blksw));
> +    for(ch=0; ch<nfchans; ch++)
> +        ctx->blksw[ch] = get_bits1(gb);
> +
> +    /* dithering flag */
> +    memset(ctx->dithflag, 0, sizeof(ctx->dithflag));
> +    for(ch=0; ch<nfchans; ch++)
> +        ctx->dithflag[ch] = get_bits1(gb);

are all these memsets really needed?


[...]
> +                    av_log(NULL, AV_LOG_ERROR, "chbwcod = %d > 60", chbwcod);

please use some context, NULL is bad if there are several decoders


[...]
> +    /* coupling exponents */
> +    if(ctx->cplinu && ctx->expstr[ctx->cpl_channel] != EXP_REUSE) {
> +        int cplabsexp = get_bits(gb, 4) << 1;
> +        int grpsize = 3 << (ctx->expstr[ctx->cpl_channel] - 1);
> +        int ngrps = (ctx->cplendmant - ctx->cplstrtmant) / grpsize;
> +        dexps = ctx->dexps[ctx->cpl_channel];
> +        decode_exponents(gb, ctx->expstr[ctx->cpl_channel], ngrps, cplabsexp,
> +                         dexps + ctx->cplstrtmant);
> +    }
> +    /* fbw & lfe exponents */
> +    for(ch=0; ch<ctx->nchans; ch++) {
> +        if(ctx->expstr[ch] != EXP_REUSE) {
> +            int ngrps = 2;
> +            if(ch != ctx->lfe_channel) {
> +                int grpsize = 3 << (ctx->expstr[ch] - 1);
> +                ngrps = (ctx->endmant[ch] + grpsize - 4) / grpsize;
> +            }
> +            dexps = ctx->dexps[ch];
> +            dexps[0] = get_bits(gb, 4);
> +            decode_exponents(gb, ctx->expstr[ch], ngrps, dexps[0], dexps + 1);
> +            if(ch != ctx->lfe_channel)
> +                skip_bits(gb, 2); // skip gainrng
> +        }
> +    }

maybe these can be merged?


[...]
> +    /* snroffset */
> +    if(get_bits1(gb)) {
> +        /* coarse snr offset */
> +        ctx->csnroffst = get_bits(gb, 6);
> +
> +        /* fine snr offsets and fast gain codes */
> +        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);
> +        }
> +        memset(ctx->bit_alloc_stages, 3, sizeof(ctx->bit_alloc_stages));
> +    }
> +
> +    /* coupling leak information */
> +    if(ctx->cplinu && get_bits1(gb)) {
> +        ctx->bit_alloc_params.cplfleak = get_bits(gb, 3);
> +        ctx->bit_alloc_params.cplsleak = get_bits(gb, 3);
> +        ctx->bit_alloc_stages[ctx->cpl_channel] = FFMAX(ctx->bit_alloc_stages[ctx->cpl_channel], 2);
> +    }
> +
> +    /* delta bit allocation information */
> +    if(get_bits1(gb)) {
> +        /* bit allocation exists (new/reuse/none) */
> +        if(ctx->cplinu) {
> +            ctx->deltbae[ctx->cpl_channel] = get_bits(gb, 2);
> +        }
> +        for(ch=0; ch<nfchans; ch++) {
> +            ctx->deltbae[ch] = get_bits(gb, 2);
> +        }
> +
> +        /* delta offset, len and bit allocation */
> +        if(ctx->cplinu) {
> +            ch = ctx->cpl_channel;
> +            if(ctx->deltbae[ch] == DBA_NEW) {
> +                ctx->deltnseg[ch] = get_bits(gb, 3);
> +                for(seg=0; seg<=ctx->deltnseg[ch]; seg++) {
> +                    ctx->deltoffst[ch][seg] = get_bits(gb, 5);
> +                    ctx->deltlen[ch][seg] = get_bits(gb, 4);
> +                    ctx->deltba[ch][seg] = get_bits(gb, 3);
> +                }
> +            }
> +            ctx->bit_alloc_stages[ch] = FFMAX(ctx->bit_alloc_stages[ch], 2);
> +        }
> +        for(ch=0; ch<nfchans; ch++) {
> +            if(ctx->deltbae[ch] == DBA_NEW) {
> +                ctx->deltnseg[ch] = get_bits(gb, 3);
> +                for(seg=0; seg<=ctx->deltnseg[ch]; seg++) {
> +                    ctx->deltoffst[ch][seg] = get_bits(gb, 5);
> +                    ctx->deltlen[ch][seg] = get_bits(gb, 4);
> +                    ctx->deltba[ch][seg] = get_bits(gb, 3);
> +                }
> +            }
> +            ctx->bit_alloc_stages[ch] = FFMAX(ctx->bit_alloc_stages[ch], 2);
> +        }

why not put the cpl_channel at ch=-1 position? maybe ive already asked that
i dont remember exactly ...


[...]
> +    /* apply dynamic range scaling */
> +    for(ch=0; ch<ctx->nchans; ch++) {
> +        float gain = 1.0f;
> +        if(ctx->acmod == AC3_ACMOD_DUALMONO) {
> +            gain = 2.0f * ctx->dynrng[ch];
> +        } else {
> +            gain = 2.0f * ctx->dynrng[0];
> +        }

hmmmm, setting gain to 1.0 and then setting it to another value, the
initial set is useless


[...]
> +    /* convert float to 16-bit integer */
> +    for(ch=0; ch<ctx->out_channels; ch++) {
> +        for(i=0; i<256; i++) {
> +            ctx->output[ch][i] = ctx->output[ch][i] * ctx->mul_bias +
> +                                 ctx->add_bias;
> +        }
> +        ctx->dsp.float_to_int16(ctx->int_output[ch], ctx->output[ch], 256);

this isnt exactly ideal
the bias should be applied in some more efficient way like multiplying some
scaling coefficients, allthough i dunno if thats possible in AC3 ...

[...]
-- 
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: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070504/0c040796/attachment.pgp>



More information about the ffmpeg-devel mailing list