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

Michael Niedermayer michaelni
Sat Feb 10 16:30:41 CET 2007


Hi

On Sat, Feb 10, 2007 at 01:02:20AM -0500, Justin Ruggles wrote:
> Hello,
> 
> Here is what I have so far in my attempts to merge the AC-3 decoder
> written by Kartikey Mahendra BHATT for the Google Summer of Code.  There
> are some things which still need work, and I have some enhancements
> planned for after it's included.  Right now, this gives generally
> working results, but has some issues.
> 
> The 256-point MDCT is not working correctly.  I'm still trying to get
> that fixed.  Also, the downmixing seems to not always work like it
> should.  The code could use some clean-up as well.
> 
> The patch I've attached is against current SVN.  The ac3.c and
> ac3_decoder.c files go into libavcodec.  When it's committed, ac3.c will
> be created by doing an svn copy of ac3enc.c and then patching it to what
> I've included here.  I think I got the build system stuff right, but I'm
> not completely sure.

you should have diffed ac3.c against ac3enc.c then, also keep in mind
that code duplication (i dont know if theres any after the copy and patch) is
unacceptable


> 
> Here is a summary of the changes to the decoder:
>  - used the new av_random for dithering
>  - used the common bit allocation code from ac3enc.c (via ac3.c)
>  - replaced the exponent decoding, which was from liba52 (GPL), with an
>    implementation I wrote for my own AC-3 parser, which is LGPL.
>  - reused some of the macros from ac3.h and got rid of ac3_decoder.h
> 
> I really want to fix the short block MDCT before merging this, but I
> thought I would go ahead and post what I have so it could be reviewed.


[...]
> #include "avcodec.h"
> #include "ac3.h"
> #include "ac3tab.h"
> 
> static inline int calc_lowcomp1(int a, int b0, int b1)
> {
>     if ((b0 + 256) == b1) {
>         a = 384 ;
>     } else if (b0 > b1) {
>         a = a - 64;
>         if (a < 0) a=0;
>     }
>     return a;
> }

calc_lowcomp1(int a, int b0, int b1, int C){
    if ((b0 + 256) == b1) {
        a= C;
    } else if (b0 > b1) {
        a= FFMAX(a - 64, 0);
    }
    return a;


> 
> static inline int calc_lowcomp(int a, int b0, int b1, int bin)
> {
>     if (bin < 7) {
>         if ((b0 + 256) == b1) {
>             a = 384 ;
>         } else if (b0 > b1) {
>             a = a - 64;
>             if (a < 0) a=0;
>         }
>     } else if (bin < 20) {
>         if ((b0 + 256) == b1) {
>             a = 320 ;
>         } else if (b0 > b1) {
>             a= a - 64;
>             if (a < 0) a=0;
>         }
>     } else {
>         a = a - 128;
>         if (a < 0) a=0;
>     }
>     return a;
> }

    if (bin < 7) {
        return calc_lowcomp1(a, b0, b1, 384);
    } else if (bin < 20) {
        return calc_lowcomp1(a, b0, b1, 320);
    } else {
        return FFMAX(a - 128, 0);
    }


> 
> /* AC3 bit allocation. The algorithm is the one described in the AC3
>    spec. */
> void ac3_parametric_bit_allocation(AC3BitAllocParameters *s, uint8_t *bap,

not doxygen compatible comment


>                                    int8_t *exp, int start, int end,
>                                    int snroffset, int fgain, int is_lfe,
>                                    int deltbae,int deltnseg,
>                                    uint8_t *deltoffst, uint8_t *deltlen, uint8_t *deltba)
> {
>     int bin,i,j,k,end1,v,v1,bndstrt,bndend,lowcomp,begin;
>     int fastleak,slowleak,address,tmp;
>     int16_t psd[256]; /* scaled exponents */
>     int16_t bndpsd[50]; /* interpolated exponents */
>     int16_t excite[50]; /* excitation */
>     int16_t mask[50];   /* masking value */
> 
>     /* exponent mapping to PSD */
>     for(bin=start;bin<end;bin++) {
>         psd[bin]=(3072 - (exp[bin] << 7));
>     }
> 
>     /* PSD integration */
>     j=start;
>     k=masktab[start];
>     do {
>         v=psd[j];
>         j++;
>         end1=bndtab[k+1];
>         if (end1 > end) end1=end;

end1= FFMIN(end1, end);


>         for(i=j;i<end1;i++) {
>             int c,adr;
>             /* logadd */
>             v1=psd[j];
>             c=v-v1;
>             if (c >= 0) {
>                 adr=c >> 1;
>                 if (adr > 255) adr=255;
>                 v=v + latab[adr];
>             } else {
>                 adr=(-c) >> 1;
>                 if (adr > 255) adr=255;
>                 v=v1 + latab[adr];
>             }

adr= FFMIN(FFABS(v-v1)>>1, 255)
v= FFMAX(v,v1) + latab[adr];



>             j++;
>         }
>         bndpsd[k]=v;
>         k++;
>     } while (end > bndtab[k]);
> 
>     /* excitation function */
>     bndstrt = masktab[start];
>     bndend = masktab[end-1] + 1;
> 
>     if (bndstrt == 0) {
>         lowcomp = 0;
>         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 ;
>         for (bin = 2; bin < 7; bin++) {
>             if (!(is_lfe && bin == 6))
>                 lowcomp = calc_lowcomp1(lowcomp, bndpsd[bin], bndpsd[bin+1]) ;
>             fastleak = bndpsd[bin] - fgain ;
>             slowleak = bndpsd[bin] - s->sgain ;
>             excite[bin] = fastleak - lowcomp ;
>             if (!(is_lfe && bin == 6)) {
>                 if (bndpsd[bin] <= bndpsd[bin+1]) {
>                     begin = bin + 1 ;
>                     break ;
>                 }
>             }
>         }
> 
>         end1=bndend;
>         if (end1 > 22) end1=22;
> 
>         for (bin = begin; bin < end1; bin++) {
>             if (!(is_lfe && bin == 6))
>                 lowcomp = calc_lowcomp(lowcomp, bndpsd[bin], bndpsd[bin+1], bin) ;
> 
>             fastleak -= s->fdecay ;
>             v = bndpsd[bin] - fgain;
>             if (fastleak < v) fastleak = v;

FFMAX


> 
>             slowleak -= s->sdecay ;
>             v = bndpsd[bin] - s->sgain;
>             if (slowleak < v) slowleak = v;

FFMAX


> 
>             v=fastleak - lowcomp;
>             if (slowleak > v) v=slowleak;

v= FFMAX(fastleak - lowcomp, slowleak)



[...]
>         for (k = i; k < end1; k++) {
>             address = (psd[i] - v) >> 5 ;
>             if (address < 0) address=0;
>             else if (address > 63) address=63;

clip()

[...]

also improvements to existing files (ac3enc.c) must be commited seperately as
they are seperate from the actual ac3 decoder


[...]

> #define ALT_BITSTREAM_READER

why?


> 
> #include "avcodec.h"
> #include "ac3.h"
> #include "bitstream.h"
> #include "dsputil.h"
> #include "random.h"
> 
> static const int nfchans_tbl[8] = { 2, 1, 2, 3, 3, 4, 4, 5 };

uint8_t


[...]
> /* Adjustments in dB gain */
> #define LEVEL_MINUS_3DB         0.7071067811865476

add "// sqrt(2)/2"


> #define LEVEL_MINUS_4POINT5DB   0.5946035575013605
> #define LEVEL_MINUS_6DB         0.5000000000000000
> #define LEVEL_PLUS_3DB          1.4142135623730951

add "// sqrt(2)"


[...]
> #define AC3_INPUT_DUALMONO      0x00
> #define AC3_INPUT_MONO          0x01
> #define AC3_INPUT_STEREO        0x02
> #define AC3_INPUT_3F            0x03
> #define AC3_INPUT_2F_1R         0x04
> #define AC3_INPUT_3F_1R         0x05
> #define AC3_INPUT_2F_2R         0x06
> #define AC3_INPUT_3F_2R         0x07

this could be changed to a enum


> 
> typedef struct {
>     uint16_t crc1;
>     uint8_t  fscod;
> 
>     uint8_t  acmod;         

trailing whitespace


>     uint8_t  cmixlev;
>     uint8_t  surmixlev;
>     uint8_t  dsurmod;
> 
>     uint8_t  blksw;
>     uint8_t  dithflag;
>     uint8_t  cplinu;
>     uint8_t  chincpl;
>     uint8_t  phsflginu;
>     uint8_t  cplbegf;
>     uint8_t  cplendf;
>     uint8_t  cplcoe;
>     uint32_t cplbndstrc;
>     uint8_t  rematstr;
>     uint8_t  rematflg;
>     uint8_t  cplexpstr;
>     uint8_t  lfeexpstr;
>     uint8_t  chexpstr[FBW_CHANNELS];
>     uint8_t  halfratecod;
>     uint8_t  sdcycod;
>     uint8_t  fdcycod;
>     uint8_t  sgaincod;
>     uint8_t  dbpbcod;
>     uint8_t  floorcod;
>     uint8_t  csnroffst;
>     uint8_t  cplfsnroffst;
>     uint8_t  cplfgaincod;
>     uint8_t  fsnroffst[FBW_CHANNELS];
>     uint8_t  fgaincod[FBW_CHANNELS];
>     uint8_t  lfefsnroffst;
>     uint8_t  lfefgaincod;
>     uint8_t  cplfleak;
>     uint8_t  cplsleak;
>     uint8_t  cpldeltbae;
>     uint8_t  deltbae[FBW_CHANNELS];
>     uint8_t  cpldeltnseg;
>     uint8_t  cpldeltoffst[DBA_SEG_MAX];
>     uint8_t  cpldeltlen[DBA_SEG_MAX];
>     uint8_t  cpldeltba[DBA_SEG_MAX];
>     uint8_t  deltnseg[FBW_CHANNELS];
>     uint8_t  deltoffst[FBW_CHANNELS][DBA_SEG_MAX];
>     uint8_t  deltlen[FBW_CHANNELS][DBA_SEG_MAX];
>     uint8_t  deltba[FBW_CHANNELS][DBA_SEG_MAX];
> 
>     /* Derived Attributes. */
>     int      sampling_rate;
>     int      bit_rate;
>     int      frame_size;
> 
>     int      nfchans;   // number of channels
>     int      lfeon;     // lfe channel in use
> 
>     float    dynrng;                        // dynamic range gain
>     float    dynrng2;                       // dynamic range gain for 1+1 mode
>     float    chcoeffs[AC3_MAX_CHANNELS];    // normalized channel coefficients
>     float    cplco[FBW_CHANNELS][18];       // coupling coordinates
>     int      ncplbnd;                       // number of coupling bands
>     int      ncplsubnd;                     // number of coupling sub bands
>     int      cplstrtmant;                   // coupling start mantissa
>     int      cplendmant;                    // coupling end mantissa
>     int      endmant[FBW_CHANNELS];         // channel end mantissas

are all these wierd names from the ac3 spec? if no then id strongly vote
for more readable names cplstrtmant -> coupling_start_mantisse for example

if they are in the ac3 spec then iam undecided if these names should be keept


>     
>     uint8_t  dcplexps[AC3_MAX_COEFS];            // decoded coupling exponents
>     uint8_t  dexps[FBW_CHANNELS][AC3_MAX_COEFS]; // decoded fbw channel exponents
>     uint8_t  dlfeexps[AC3_MAX_COEFS];            // decoded lfe channel exponents
>     uint8_t  cplbap[AC3_MAX_COEFS];              // coupling bit allocation pointers
>     uint8_t  bap[FBW_CHANNELS][AC3_MAX_COEFS];   // fbw channel bit allocation pointers
>     uint8_t  lfebap[AC3_MAX_COEFS];              // lfe channel bit allocation pointers

the comments are all not doxygen compatible (///<)


[...]
> static int16_t dither_int16(AVRandomState *state) 
> {
>     uint32_t y = av_random(state);
>     return (int16_t)((y << 16) >> 16);
> }

both high and low 16bit should be used due to speed reasons ...


[...]

> 
> /* 
>  * Generate quantizer tables.
>  */
> static void generate_quantizers_table(int16_t quantizers[], int level, int length)
> {
>     int i;
> 
>     for (i = 0; i < length; i++)
>         quantizers[i] = ((2 * i - level + 1) << 15) / level;
> }
> 
> static void generate_quantizers_table_1(int16_t quantizers[], int level, int length1, int length2, int size)
> {
>     int i, j;
>     int16_t v;
> 
>     for (i = 0; i < length1; i++) {
>         v = ((2 * i - level + 1) << 15) / level;
>         for (j = 0; j < length2; j++) 
>             quantizers[i * length2 + j] = v;
>     }
> 
>     for (i = length1 * length2; i < size; i++)
>         quantizers[i] = 0;
> }
> 
> static void generate_quantizers_table_2(int16_t quantizers[], int level, int length1, int length2, int size)
> {
>     int i, j;
>     int16_t v;
> 
>     for (i = 0; i < length1; i++) {
>         v = ((2 * (i % level) - level + 1) << 15) / level;
>         for (j = 0; j < length2; j++) 
>             quantizers[i * length2 + j] = v;
>     }
> 
>     for (i = length1 * length2; i < size; i++)
>         quantizers[i] = 0;
> 
> }
> 
> static void generate_quantizers_table_3(int16_t quantizers[], int level, int length1, int length2, int size)
> {
>     int i, j;
> 
>     for (i = 0; i < length1; i++)
>         for (j = 0; j < length2; j++)
>             quantizers[i * length2 + j] = ((2 * (j % level) - level + 1) << 15) / level;
> 
>     for (i = length1 * length2; i < size; i++)
>         quantizers[i] = 0;
> }

replace the quadruplicated shit above by

generate_quantizers_table(int16_t quantizers[], int level, int length1, int length2, int size, int im, int jm){
    int i, j;

    for (i = 0; i < length1; i++)
        for (j = 0; j < length2; j++)
        quantizers[i * length2 + j] = ((2 * (j % jm + i % im) - level + 1) << 15) / level;
}

also note the = 0 is useless as static&global arrays are initalized to 0


[...]
> /* Parse the 'sync_info' from the ac3 bitstream.
>  * This function extracts the sync_info from ac3 bitstream.
>  * GetBitContext within AC3DecodeContext must point to 
>  * start of the synchronized ac3 bitstream.
>  *
>  * @param ctx  AC3DecodeContext
>  * @return Returns framesize, returns 0 if fscod, frmsizecod or bsid is not valid 

almost all code in lav* returns negative values on failure id suggest that
ac3 does too

doxygen comments start with /** or various other forms but not "/* "


> static int ac3_parse_sync_info(AC3DecodeContext *ctx)
> {
>     GetBitContext *gb = &ctx->gb;
>     int frmsizecod, bsid;
> 
>     skip_bits(gb, 16); //skip the sync_word, sync_info->sync_word = get_bits(gb, 16);
>     ctx->crc1 = get_bits(gb, 16);
>     ctx->fscod = get_bits(gb, 2);
>     if (ctx->fscod == 0x03)
>         return 0;
>     frmsizecod = get_bits(gb, 6);
>     if (frmsizecod >= 38)
>         return 0;
> 
>     /* we include it here in order to determine validity of ac3 frame */
>     bsid = get_bits(gb, 5);
>     if (bsid > 10)
>         return 0;
>     skip_bits(gb, 3); //skip the bsmod, bsi->bsmod = get_bits(gb, 3);
> 
>     ctx->halfratecod = FFMAX(0, bsid - 8);
>     ctx->sampling_rate = ac3_freqs[ctx->fscod] >> ctx->halfratecod;
>     ctx->bit_rate = ac3_bitratetab[frmsizecod >> 1];
> 
>     ctx->frame_size = 0;
>     switch (ctx->fscod) {
>         case 0x00:
>             ctx->frame_size = 4 * ctx->bit_rate;
>             break;
>         case 0x01:
>             ctx->frame_size = 2 * (320 * ctx->bit_rate / 147 + (frmsizecod & 1));
>             break;
>         case 0x02:
>             ctx->frame_size =  6 * ctx->bit_rate;
>             break;
>     }
> 
>     ctx->bit_rate >>= ctx->halfratecod;
> 
>     return ctx->frame_size;
> }

this looks a little bit similar to what is in the avparser, if so it is
code duplication which is not ok (put the common code in a common function)


[...]
> /* Decodes the grouped exponents.
>  * This function decodes the coded exponents according to exponent strategy
>  * and stores them in the decoded exponents buffer.
>  *
>  * @param gb GetBitContext which points to start of coded exponents
>  * @param expstr Exponent coding strategy
>  * @param ngrps Number of grouped exponetns
>  * @param absexp Absolute exponent
>  * @param dexps Decoded exponents are stored in dexps
>  */
> static void decode_exponents(GetBitContext *gb, int expstr, int ngrps,
>                              uint8_t absexp, uint8_t *dexps)
> {
>     int i, j, grp, grpsize;
>     int dexp[256];
>     int aexp[256];
>     int expacc, prevexp;
> 
>     // unpack groups
>     grpsize = expstr;
>     if(expstr == EXP_D45) grpsize = 4;
>     for(grp=0; grp<ngrps; grp++) {
>         expacc = get_bits(gb, 7);
>         dexp[grp*3] = expacc / 25;
>         expacc -= 25 * dexp[grp*3];
>         dexp[(grp*3)+1] = expacc / 5;
>         expacc -= 5 * dexp[(grp*3)+1];
>         dexp[(grp*3)+2] = expacc;

what about using a table which store the /25 (%25)/5 %5 values? should be
faster ...


>     }
> 
>     // convert to absolute exps
>     prevexp = absexp;
>     for(i=0; i<ngrps*3; i++) {
>         aexp[i] = prevexp + (dexp[i]-2);
>         aexp[i] = FFMIN(FFMAX(aexp[i], 0), 24);
clip()

>         prevexp = aexp[i];

also id

prevexp += dexp[i]-2;
prevexp= clip(prevexp, ...)
aexp[i]= prevexp;

somehow that looks cleaner to me ...


[...]

> #define TRANSFORM_COEFF(tc, m, e, f) (tc) = (m) * (f)[(e)]

i hate macros which hide trivial arithmetic operations, please replace
all TRANSFORM_COEFF by what they acctually do, and feel free to add a comment
to all like // transforms coeff


> 
> /* Get the transform coefficients for coupling channel and uncouple channels.
>  * The coupling transform coefficients starts at the the cplstrtmant, which is
>  * equal to endmant[ch] for fbw channels. Hence we can uncouple channels before
>  * getting transform coefficients for the channel.
>  */
> static int get_transform_coeffs_cpling(AC3DecodeContext *ctx, mant_groups *m)
> {
>     GetBitContext *gb = &ctx->gb;
>     int ch, start, end, cplbndstrc, bnd, gcode, tbap;
>     float cplcos[5], cplcoeff;
>     uint8_t *exps = ctx->dcplexps;
>     uint8_t *bap = ctx->cplbap;
> 
>     cplbndstrc = ctx->cplbndstrc;
>     start = ctx->cplstrtmant;
>     bnd = 0;
> 
>     while (start < ctx->cplendmant) {
>         end = start + 12;
>         while (cplbndstrc & 1) {
>             end += 12;
>             cplbndstrc >>= 1;
>         }
>         cplbndstrc >>= 1;
>         for (ch = 0; ch < ctx->nfchans; ch++) 
>             cplcos[ch] = ctx->chcoeffs[ch] * ctx->cplco[ch][bnd];
>         bnd++;
> 
>         while (start < end) {
>             tbap = bap[start];
>             switch(tbap) {
>                 case 0:
>                     for (ch = 0; ch < ctx->nfchans; ch++)
>                         if (((ctx->chincpl) >> ch) & 1) {
>                             if ((ctx->dithflag >> ch) & 1) {
>                                 TRANSFORM_COEFF(cplcoeff, dither_int16(&ctx->dith_state), exps[start], scale_factors);
>                                 ctx->transform_coeffs[ch + 1][start] = cplcoeff * cplcos[ch] * LEVEL_MINUS_3DB;
>                             } else 
>                                 ctx->transform_coeffs[ch + 1][start] = 0;
>                         }
>                     start++;
>                     continue;
>                 case 1:
>                     if (m->l3ptr > 2) {
>                         gcode = get_bits(gb, 5);
>                         m->l3_quantizers[0] = l3_quantizers_1[gcode];
>                         m->l3_quantizers[1] = l3_quantizers_2[gcode];
>                         m->l3_quantizers[2] = l3_quantizers_3[gcode];
>                         m->l3ptr = 0;
>                     }
>                     TRANSFORM_COEFF(cplcoeff, m->l3_quantizers[m->l3ptr++], exps[start], scale_factors);
>                     break;
> 
>                 case 2:
>                     if (m->l5ptr > 2) {
>                         gcode = get_bits(gb, 7);
>                         m->l5_quantizers[0] = l5_quantizers_1[gcode];
>                         m->l5_quantizers[1] = l5_quantizers_2[gcode];
>                         m->l5_quantizers[2] = l5_quantizers_3[gcode];
>                         m->l5ptr = 0;
>                     }
>                     TRANSFORM_COEFF(cplcoeff, m->l5_quantizers[m->l5ptr++], exps[start], scale_factors);
>                     break;
> 
>                 case 3:
>                     TRANSFORM_COEFF(cplcoeff, l7_quantizers[get_bits(gb, 3)], exps[start], scale_factors);
>                     break;
> 
>                 case 4:
>                     if (m->l11ptr > 1) {
>                         gcode = get_bits(gb, 7);
>                         m->l11_quantizers[0] = l11_quantizers_1[gcode];
>                         m->l11_quantizers[1] = l11_quantizers_2[gcode];
>                         m->l11ptr = 0;
>                     }
>                     TRANSFORM_COEFF(cplcoeff, m->l11_quantizers[m->l11ptr++], exps[start], scale_factors);
>                     break;
> 
>                 case 5:
>                     TRANSFORM_COEFF(cplcoeff, l15_quantizers[get_bits(gb, 4)], exps[start], scale_factors);
>                     break;
> 
>                 default:
>                     TRANSFORM_COEFF(cplcoeff, get_sbits(gb, qntztab[tbap]) << (16 - qntztab[tbap]),
>                             exps[start], scale_factors);
>             }

all the TRANSFORM_COEFF are equal excpet the second argument so this could be simplified like

...
case 3:
    coeff= l7_quantizers[get_bits(gb, 3)];
    break;

case 4:
    if (m->l11ptr > 1) {
        gcode = get_bits(gb, 7);
        m->l11_quantizers[0] = l11_quantizers_1[gcode];
        m->l11_quantizers[1] = l11_quantizers_2[gcode];
        m->l11ptr = 0;
    }
    coeff= m->l11_quantizers[m->l11ptr++];
    break;

case 5:
    coeff= l15_quantizers[get_bits(gb, 4)];
    break;
}
TRANSFORM_COEFF(cplcoeff, coeff, exps[start], scale_factors);


>             for (ch = 0; ch < ctx->nfchans; ch++)
>                 if ((ctx->chincpl >> ch) & 1) 

maybe chincpl and dithflag should be changed to normal arrays? or maybe
not? dunno


[...]
>     if (ch_index != -1) { /* fbw channels */
>         dithflag = (ctx->dithflag >> ch_index) & 1;
>         exps = ctx->dexps[ch_index];
>         bap = ctx->bap[ch_index];
>         coeffs = ctx->transform_coeffs[ch_index + 1];
>         end = ctx->endmant[ch_index];
>     } else if (ch_index == -1) {

that else if is silly it must be true if the first is false


>         dithflag = 0;
>         exps = ctx->dlfeexps;
>         bap = ctx->lfebap;
>         coeffs = ctx->transform_coeffs[0];
>         end = 7;
>     }
> 
> 
>     for (i = 0; i < end; i++) {
>         tbap = bap[i];
>         switch (tbap) {
>             case 0:
>                 if (!dithflag) {
>                     coeffs[i] = 0;
>                     continue;
>                 }
>                 else {
>                     TRANSFORM_COEFF(coeffs[i], dither_int16(&ctx->dith_state), exps[i], factors);
>                     coeffs[i] *= LEVEL_MINUS_3DB;
>                     continue;
>                 }
> 
>             case 1:
>                 if (m->l3ptr > 2) {
>                     gcode = get_bits(gb, 5);
>                     m->l3_quantizers[0] = l3_quantizers_1[gcode];
>                     m->l3_quantizers[1] = l3_quantizers_2[gcode];
>                     m->l3_quantizers[2] = l3_quantizers_3[gcode];
>                     m->l3ptr = 0;
>                 }
>                 TRANSFORM_COEFF(coeffs[i], m->l3_quantizers[m->l3ptr++], exps[i], factors);
>                 continue;
> 
>             case 2:
>                 if (m->l5ptr > 2) {
>                     gcode = get_bits(gb, 7);
>                     m->l5_quantizers[0] = l5_quantizers_1[gcode];
>                     m->l5_quantizers[1] = l5_quantizers_2[gcode];
>                     m->l5_quantizers[2] = l5_quantizers_3[gcode];
>                     m->l5ptr = 0;
>                 }
>                 TRANSFORM_COEFF(coeffs[i], m->l5_quantizers[m->l5ptr++], exps[i], factors);
>                 continue;
> 
>             case 3:
>                 TRANSFORM_COEFF(coeffs[i], l7_quantizers[get_bits(gb, 3)], exps[i], factors);
>                 continue;
> 
>             case 4:
>                 if (m->l11ptr > 1) {
>                     gcode = get_bits(gb, 7);
>                     m->l11_quantizers[0] = l11_quantizers_1[gcode];
>                     m->l11_quantizers[1] = l11_quantizers_2[gcode];
>                     m->l11ptr = 0;
>                 }
>                 TRANSFORM_COEFF(coeffs[i], m->l11_quantizers[m->l11ptr++], exps[i], factors);
>                 continue;
> 
>             case 5:
>                 TRANSFORM_COEFF(coeffs[i], l15_quantizers[get_bits(gb, 4)], exps[i], factors);
>                 continue;
> 
>             default:
>                 TRANSFORM_COEFF(coeffs[i], get_sbits(gb, qntztab[tbap]) << (16 - qntztab[tbap]), exps[i], factors);
>                 continue;
>         }
>     }
> 
>     return 0;
> }

duplicate code (see get_transform_coeffs_cpling)


[...]
>         if (get_transform_coeffs_ch(ctx, -1, &m))
>                 return -1;

indention looks slightly wrong


[...]
>         case AC3_INPUT_3F:
>             switch (to) {
>                 case AC3_OUTPUT_MONO:
>                     nf = LEVEL_MINUS_3DB / (1.0 + clev);
>                     ctx->chcoeffs[0] *= (nf * LEVEL_MINUS_3DB);
>                     ctx->chcoeffs[2] *= (nf * LEVEL_MINUS_3DB);
>                     ctx->chcoeffs[1] *= ((nf * clev * LEVEL_MINUS_3DB) / 2.0);

nf = 0.5 / (1.0 + clev);
ctx->chcoeffs[0] *= nf;
ctx->chcoeffs[2] *= nf;
ctx->chcoeffs[1] *= nf * clev * 0.5;


[...]
>         case AC3_INPUT_2F_1R:
>             switch (to) {
>                 case AC3_OUTPUT_MONO:
>                     nf = 2.0 * LEVEL_MINUS_3DB / (2.0 + slev);
>                     ctx->chcoeffs[0] *= (nf * LEVEL_MINUS_3DB);
>                     ctx->chcoeffs[1] *= (nf * LEVEL_MINUS_3DB);
>                     ctx->chcoeffs[2] *= (nf * slev * LEVEL_MINUS_3DB);

nf = 1.0 / (2.0 + slev);
ctx->chcoeffs[0] *= nf;
ctx->chcoeffs[1] *= nf;
ctx->chcoeffs[2] *= nf * slev;

(LEVEL_MINUS_3DB * LEVEL_MINUS_3DB = 0.5)



[...]
> /*********** BEGIN DOWNMIX FUNCTIONS ***********/

id put these in their own file


> static inline void mix_dualmono_to_mono(AC3DecodeContext *ctx)
> {
>     int i;
>     float (*output)[AC3_BLOCK_SIZE] = ctx->output;

all these functions seem to just use ctx->output so that should be passed
as argument instead of AC3DecodeContext


[...]
> static inline void upmix_mono_to_stereo(AC3DecodeContext *ctx)
> {
>     int i;
>     float (*output)[AC3_BLOCK_SIZE] = ctx->output;
> 
>     for (i = 0; i < 256; i++)
>         output[2][i] = output[1][i];

memcpy()


> }
> 
> static inline void mix_stereo_to_mono(AC3DecodeContext *ctx)
> {
>     int i;
>     float (*output)[AC3_BLOCK_SIZE] = ctx->output;
> 
>     for (i = 0; i < 256; i++)
>         output[1][i] += output[2][i];
>     memset(output[2], 0, sizeof(output[2]));
> }

duplicate of mix_dualmono_to_mono()


[...]
> static inline void mix_2f_1r_to_mono(AC3DecodeContext *ctx)
> {
>     int i;
>     float (*output)[AC3_BLOCK_SIZE] = ctx->output;
> 
>     for (i = 0; i < 256; i++)
>         output[1][i] += (output[2][i] + output[3][i]);
>     memset(output[2], 0, sizeof(output[2]));
>     memset(output[3], 0, sizeof(output[3]));

duplicate of mix_3f_to_mono

> 
> }
> 
> static inline void mix_2f_1r_to_stereo(AC3DecodeContext *ctx)
> {
>     int i;
>     float (*output)[AC3_BLOCK_SIZE] = ctx->output;
> 
>     for (i = 0; i < 256; i++) {
>         output[1][i] += output[2][i];
>         output[2][i] += output[3][i];
>     }
>     memset(output[3], 0, sizeof(output[3]));

duplicate of mix_3f_to_stereo

[...]
> static inline void mix_2f_2r_to_mono(AC3DecodeContext *ctx)
> {
>     int i;
>     float (*output)[AC3_BLOCK_SIZE] = ctx->output;
> 
>     for (i = 0; i < 256; i++) 
>         output[1][i] = (output[2][i] + output[3][i] + output[4][i]);
>     memset(output[2], 0, sizeof(output[2]));
>     memset(output[3], 0, sizeof(output[3]));
>     memset(output[4], 0, sizeof(output[4]));

duplictae of mix_3f_1r_to_mono

[...]
>     for (k = 0; k < N / 8; k++)
>     {
>         o_ptr[2 * k] = -ptr1[k].im * w[2 * k] + d_ptr[2 * k] + 384.0;
>         o_ptr[2 * k + 1] = ptr1[N / 8 - k - 1].re * w[2 * k + 1] + 384.0;
>         o_ptr[N / 4 + 2 * k] = -ptr1[k].re * w[N / 4 + 2 * k] + d_ptr[N / 4 + 2 * k] + 384.0;
>         o_ptr[N / 4 + 2 * k + 1] = ptr1[N / 8 - k - 1].im * w[N / 4 + 2 * k + 1] + d_ptr[N / 4 + 2 * k + 1] + 384.0;
>         d_ptr[2 * k] = ptr2[k].re * w[k / 2 - 2 * k - 1];
>         d_ptr[2 * k + 1] = -ptr2[N / 8 - k - 1].im * w[N / 2 - 2 * k - 2];
>         d_ptr[N / 4 + 2 * k] = ptr2[k].im * w[N / 4 - 2 * k - 1];
>         d_ptr[N / 4 + 2 * k + 1] = -ptr2[N / 8 - k - 1].re * w[N / 4 - 2 * k - 2];
>     }

what a mess ...


[...]
>     if (get_bits1(gb)) { /* dynamic range */
>         dynrng = get_sbits(gb, 8);
>         ctx->dynrng = ((((dynrng & 0x1f) | 0x20) << 13) * scale_factors[3 - (dynrng >> 5)]);
>     }
> 
>     if (acmod == 0x00 && get_bits1(gb)) { /* dynamic range 1+1 mode */ 
>         dynrng = get_sbits(gb, 8);
>         ctx->dynrng2 = ((((dynrng & 0x1f) | 0x20) << 13) * scale_factors[3 - (dynrng >> 5)]);
>     }

dynrng/dynrng2 -> dynrng[] and

for(i=0; i<= !acmod; i++){
    if(get_bits1(gb)){
        a= get_sbits(gb, 3);
        b=  get_bits(gb, 5);
        ctx->dynrng[i]= ((b | 0x20) << 13) * scale_factors[3 - a];
    }
}


[...]
>             if (!(ctx->cplinu) || ctx->cplbegf > 2)
>                 for (rbnd = 0; rbnd < 4; rbnd++) 
>                     ctx->rematflg |= get_bits1(gb) << rbnd;
>             if (ctx->cplbegf > 0 && ctx->cplbegf <= 2 && ctx->cplinu) 
>                 for (rbnd = 0; rbnd < 3; rbnd++) 
>                     ctx->rematflg |= get_bits1(gb) << rbnd;
>             if (ctx->cplbegf == 0 && ctx->cplinu) 
>                 for (rbnd = 0; rbnd < 2; rbnd++)
>                     ctx->rematflg |= get_bits1(gb) << rbnd;

rbnd_len= 4;
if(ctx->cplinu)
    rbnd_len-= (ctx->cplbegf==0) + (ctx->cplbegf<=2);
for (rbnd = 0; rbnd < rbnd_len; rbnd++) 
    ctx->rematflg |= get_bits1(gb) << rbnd;


[...]
>                 av_log(NULL, AV_LOG_ERROR, "coupling delta bit allocation strategy reserved\n");

av_log(NULL -> av_log(avctx
yes all av_log not just this one

[...]
>         if (ctx->cplinu)
>             if (ctx->cpldeltbae == DBA_NEW) { /*coupling delta offset, len and bit allocation */
>                 ctx->cpldeltnseg = get_bits(gb, 3);
>                 for (seg = 0; seg <= ctx->cpldeltnseg; seg++) {
>                     ctx->cpldeltoffst[seg] = get_bits(gb, 5);
>                     ctx->cpldeltlen[seg] = get_bits(gb, 4);
>                     ctx->cpldeltba[seg] = get_bits(gb, 3);
>                 }
>             }
> 
>         for (i = 0; i < nfchans; i++)
>             if (ctx->deltbae[i] == DBA_NEW) {/*channel delta offset, len and bit allocation */
>                 ctx->deltnseg[i] = get_bits(gb, 3);
>                 for (seg = 0; seg <= ctx->deltnseg[i]; seg++) {
>                     ctx->deltoffst[i][seg] = get_bits(gb, 5);
>                     ctx->deltlen[i][seg] = get_bits(gb, 4);
>                     ctx->deltba[i][seg] = get_bits(gb, 3);
>                 }
>             }

hmm the 2 loops above could be merged if cpldelt* where in the same arrays as
delt*[]

[...]
> static inline int16_t convert(int32_t i)
> {
>     if (i > 0x43c07fff)
>         return 32767;
>     else if (i <= 0x43bf8000)
>         return -32768;
>     else
>         return (i - 0x43c00000);
> }

duplicate see a52dec/ac3dec.c


> 
> //static int frame_count = 0;

hmm, forgotten debug code?


> 
> /* Decode ac3 frame.
>  *
>  * @param avctx Pointer to AVCodecContext
>  * @param data Pointer to pcm smaples
>  * @param data_size Set to number of pcm samples produced by decoding
>  * @param buf Data to be decoded
>  * @param buf_size Size of the buffer
>  */
> static int ac3_decode_frame(AVCodecContext * avctx, void *data, int *data_size, uint8_t *buf, int buf_size)
> {
>     AC3DecodeContext *ctx = (AC3DecodeContext *)avctx->priv_data;
>     int frame_start;
>     int16_t *out_samples = (int16_t *)data;
>     int i, j, k, start;
>     int32_t *int_ptr[6];
> 
>     for (i = 0; i < 6; i++)
>         int_ptr[i] = (int32_t *)(&ctx->output[i]);
> 
>     //av_log(NULL, AV_LOG_INFO, "decoding frame %d buf_size = %d\n", frame_count++, buf_size);
> 
>     //Synchronize the frame.
>     frame_start = ac3_synchronize(buf, buf_size);

as the ac3 bitstream, should have been sent through a parser there should be
no need to search for any synccode, either its there or theres an error

[...]

> Index: libavcodec/ac3tab.h
> ===================================================================
> --- libavcodec/ac3tab.h	(revision 7906)
> +++ libavcodec/ac3tab.h	(working copy)
> @@ -25,10 +25,10 @@
>   */
>  
>  /* possible frequencies */
> -static const uint16_t ac3_freqs[3] = { 48000, 44100, 32000 };
> +const uint16_t ac3_freqs[3] = { 48000, 44100, 32000 };
>  
>  /* possible bitrates */
> -static const uint16_t ac3_bitratetab[19] = {
> +const uint16_t ac3_bitratetab[19] = {
>      32, 40, 48, 56, 64, 80, 96, 112, 128,
>      160, 192, 224, 256, 320, 384, 448, 512, 576, 640
>  };
> @@ -36,7 +36,7 @@
>  /* AC3 MDCT window */
>  
>  /* MDCT window */
> -static const int16_t ac3_window[256] = {
> +const int16_t ac3_window[256] = {
>      4,    7,   12,   16,   21,   28,   34,   42,
>     51,   61,   72,   84,   97,  111,  127,  145,
>    164,  184,  207,  231,  257,  285,  315,  347,
> @@ -165,41 +165,34 @@
>      15, 15, 15, 15,
>  };

global tables need a ff_ prefix to avoid name clashed


[...]
> +/* delta bit allocation strategy */
> +#define DBA_NEW      0x01
> +#define DBA_NONE     0x02
> +#define DBA_RESERVED 0x03
> +#define DBA_REUSE    0x00

could be changed to an enum


[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Many that live deserve death. And some that die deserve life. Can you give
it to them? Then do not be too eager to deal out death in judgement. For
even the very wise cannot see all ends. -- Gandalf
-------------- 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/20070210/36df6333/attachment.pgp>



More information about the ffmpeg-devel mailing list