[FFmpeg-soc] EAC3 review
Michael Niedermayer
michaelni at gmx.at
Mon Sep 10 21:40:00 CEST 2007
Hi
heres some quick review of the EAC-3 code, ive not really reviewed the
highlevel structure as i dont know AC3 that well and the variable names
make it quite impossible to understand the code without looking everything
up in the spec
note, diego, theres a question for you somewhere below
ac3dec.c:
[...]
> /**
> * table for exponent to scale_factor mapping
> - * scale_factors[i] = 2 ^ -i
> + * ff_ac3_scale_factors[i] = 2 ^ -i
> */
> -static float scale_factors[25];
> +float ff_ac3_scale_factors[25];
>
> /** table for grouping exponents */
> -static uint8_t exp_ungroup_tbl[128][3];
> +uint8_t ff_ac3_exp_ungroup_tbl[128][3];
>
>
all these renamings will need to be commited seperately!
[...]
>
> /* reverse the post-rotation & reordering from standard IMDCT */
> for(k=0; k<32; k++) {
> - z[i][32+k].re = -o_ptr[128+2*k];
> - z[i][32+k].im = -o_ptr[2*k];
> - z[i][31-k].re = o_ptr[2*k+1];
> - z[i][31-k].im = o_ptr[128+2*k+1];
> + z[i][32+k].re = -tmp_output[128+2*k];
> + z[i][32+k].im = -tmp_output[2*k];
> + z[i][31-k].re = tmp_output[2*k+1];
> + z[i][31-k].im = tmp_output[128+2*k+1];
> }
> }
>
> /* apply AC-3 post-rotation & reordering */
> for(k=0; k<64; k++) {
> - o_ptr[ 2*k ] = -z[0][ k].im;
> - o_ptr[ 2*k+1] = z[0][63-k].re;
> - o_ptr[128+2*k ] = -z[0][ k].re;
> - o_ptr[128+2*k+1] = z[0][63-k].im;
> - o_ptr[256+2*k ] = -z[1][ k].re;
> - o_ptr[256+2*k+1] = z[1][63-k].im;
> - o_ptr[384+2*k ] = z[1][ k].im;
> - o_ptr[384+2*k+1] = -z[1][63-k].re;
> + tmp_output[ 2*k ] = -z[0][ k].im;
> + tmp_output[ 2*k+1] = z[0][63-k].re;
> + tmp_output[128+2*k ] = -z[0][ k].re;
> + tmp_output[128+2*k+1] = z[0][63-k].im;
> + tmp_output[256+2*k ] = -z[1][ k].re;
> + tmp_output[256+2*k+1] = z[1][63-k].im;
> + tmp_output[384+2*k ] = z[1][ k].im;
> + tmp_output[384+2*k+1] = -z[1][63-k].re;
> }
this renamings can be avoided
[...]
eac3dec.c:
[...]
> #ifdef DEBUG
> #define GET_BITS(a, gbc, n) {a = get_bits(gbc, n); av_log(NULL, AV_LOG_INFO, "%s: %i\n", __STRING(a), a);}
> #define GET_SBITS(a, gbc, n) {a = get_sbits(gbc, n); av_log(NULL, AV_LOG_INFO, "%s: %i\n", __STRING(a), a);}
> #else
> #define GET_BITS(a, gbc, n) a = get_bits(gbc, n)
> #define GET_SBITS(a, gbc, n) a = get_sbits(gbc, n)
> #endif //DEBUG
i think it has been said already but these are ugly, and i wont
accept that in ffmpeg svn
>
> static void spectral_extension(EAC3Context *s);
> static void get_transform_coeffs_aht_ch(GetBitContext *gbc, EAC3Context *s, int ch);
> static void idct_transform_coeffs_ch(EAC3Context *s, int ch, int blk);
> static void get_eac3_transform_coeffs_ch(GetBitContext *gbc, EAC3Context *s, int blk,
> int ch, mant_groups *m);
> static void uncouple_channels(EAC3Context *s);
> static void log_missing_feature(AVCodecContext *avctx, const char *log);
please order functions so that you dont need these
[...]
> };
>
> static int parse_bsi(GetBitContext *gbc, EAC3Context *s){
please add a doxygen comment saying that bsi == bit stream identification
or even better rename the function so this isnt needed
[...]
> // calculate number of channels
> s->nfchans = ff_ac3_channels[s->acmod];
> s->ntchans = s->nfchans;
rename this to a meaningfull variable name
if meaningless variable names occur in the spec ill accept them though
i do not think its a good idea, personally, if i would be writing the
decoder i would rename every variable and not use a single name from the
spec
and then just add the names from the spec in comments in the struct ...
but this one does not occur in the spec so theres no excuse for such
a meaningless name, channel_count or num_channels would be more
related to what the thing is
btw, would you be very opposed to rename all "ac3 variables" to some
variables made of english words?
[...]
> if(s->bsid < 11 || s->bsid > 16){
> av_log(s->avctx, AV_LOG_ERROR, "bsid should be between 11 and 16\n");
the error is "bsid is not within ..."
> return -1;
> }
>
> for(i = 0; i < (s->acmod?1:2); i++){
> s->dialnorm[i] = ff_ac3_dialnorm_tbl[get_bits(gbc, 5)];
you mix tab and tbl as abbreviations for table, choose one and use that
consistently
> if(get_bits1(gbc)){
> skip_bits(gbc, 8); //skip Compression gain word
> }
> }
> if(s->strmtyp == 0x1){
> /* if dependent stream */
> if(get_bits1(gbc)){
> GET_BITS(s->chanmap, gbc, 16);
> }else{
> //TODO default channel map based on acmod and lfeon
> }
shouldnt this be log_missing_feature() or something?
> }
>
> /* set stereo downmixing coefficients
> reference: Section 7.8.2 Downmixing Into Two Channels */
> for(i=0; i<s->nfchans; i++) {
you mix for(i=0; and for(i = 0;
> s->downmix_coeffs[i][0] = mixlevels[eac3_default_coeffs[s->acmod][i][0]];
> s->downmix_coeffs[i][1] = mixlevels[eac3_default_coeffs[s->acmod][i][1]];
> }
>
> GET_BITS(s->mixmdate, gbc, 1);
> if(s->mixmdate){
> /* Mixing metadata */
now if only the variables had names which made sense then this comment
wouldnt have been needed ...
i really dont understand why they called it mixmdate not mixmdata ...
> if(s->acmod > 0x2){
> /* if more than 2 channels */
> GET_BITS(s->dmixmod, gbc, 2);
> }
> if((s->acmod & 0x1) && (s->acmod > 0x2)){
the 0x prefix is unneeded
[...]
> if(s->frmmixcfginfoe){
> /* mixing configuration information */
beautifull abbreviation, still id use frame_mix_cfg its shorter and makes
sense, alsoitsmorereadableasthewordsareseperated
[...]
> static int parse_audfrm(GetBitContext *gbc, EAC3Context *s){
parse_audio_frame ?
[...]
> memset(s->cplinu, 0, sizeof(int) * ff_eac3_blocks[s->numblkscod]);
sizeof(*s->cplinu)
[...]
> /* AHT data */
> if(s->ahte){
> /* AHT is only available in 6 block mode (numblkscod ==0x3) */
> /* coupling can use AHT only when coupling in use for all blocks */
> /* ncplregs derived from cplstre and cplexpstr - see Section E3.3.2 */
> int nchregs;
> s->chahtinu[CPL_CH]=0;
> for(ch = (s->ncplblks!=6); ch <= s->ntchans; ch++){
> nchregs = 0;
> for(blk = 0; blk < 6; blk++)
> nchregs += (s->chexpstr[blk][ch] != EXP_REUSE);
> s->chahtinu[ch] = (nchregs == 1) && get_bits1(gbc);
> }
> }else{
> for(ch=0; ch<=s->ntchans; ch++)
> s->chahtinu[ch] = 0;
isnt this a memset(0, sizeof(s->chahtinu)) ?
> }
> /* Audio frame SNR offset data */
> if(s->snroffststr == 0x0){
> int csnroffst = (get_bits(gbc, 6) - 15) << 4;
> int snroffst = (csnroffst + get_bits(gbc, 4)) << 2;
> for(ch=0; ch<= s->ntchans; ch++)
> s->snroffst[ch] = snroffst;
> }
> /* Audio frame transient pre-noise processing data */
> if(s->transproce){
> av_log(s->avctx, AV_LOG_ERROR, "transient pre-noise processing NOT IMPLEMENTED\n");
why not log_missing_feature() ?
[...]
> /* Block start information */
> if (s->numblkscod != 0x0){
the != 0 is superflous
> GET_BITS(s->blkstrtinfoe, gbc, 1);
> }else{
> s->blkstrtinfoe = 0;
> }
> if(s->blkstrtinfoe){
if(s->numblkscod && get_bits1()){
[...]
> /* enhanced coupling in use */
> log_missing_feature(s->avctx, "Enhanced coupling");
> return -1;
> #if 0
> GET_BITS(s->ecplbegf, gbc, 4);
> if(s->ecplbegf < 3){
why is all the untested code commented out? that way it wont be compiled and
could easily become broken with noone noticing
[...]
> s->bit_alloc_params.sdecay = ff_sdecaytab[0x2]; /* Table 7.6 */
> s->bit_alloc_params.fdecay = ff_fdecaytab[0x1]; /* Table 7.7 */
> s->bit_alloc_params.sgain = ff_sgaintab[0x1]; /* Table 7.8 */
> s->bit_alloc_params.dbknee = ff_dbkneetab[0x2]; /* Table 7.9 */
> s->bit_alloc_params.floor = ff_floortab[0x7]; /* Table 7.10 */
the 0x is superflous
> }
>
> if(s->snroffststr != 0x0){
> av_log(s->avctx, AV_LOG_INFO, "NOT TESTED\n");
inconsistent relative to the other not tested things
> if(!blk || get_bits1(gbc) ){
> int csnroffst = (get_bits(gbc, 6) - 15) << 4;
> if(s->snroffststr == 0x1){
> int snroffst = (csnroffst + get_bits(gbc, 4)) << 2;
> for(ch=!s->cplinu[blk]; ch<= s->ntchans; ch++)
> s->snroffst[ch] = snroffst;
> }else if(s->snroffststr == 0x2){
> for(ch=!s->cplinu[blk]; ch<= s->ntchans; ch++)
> s->snroffst[ch] = (csnroffst + get_bits(gbc, 4)) << 2;
> }
if(s->snroffststr < 3){
for(ch=!s->cplinu[blk]; ch<=s->ntchans; ch++){
if(ch==!s->cplinu[blk] || s->snroffststr == 2)
snroffst = (csnroffst + get_bits(gbc, 4)) << 2;
s->snroffst[ch] = snroffst;
}
}
also this init code looks quite similar to the snroffststr == 0
case so maybe it could be merged somehow?
[...]
> if(s->strmtyp == 0x0){
if(!s->strmtyp){
[...]
> int start=0, end=0;
> start = s->strtmant[ch];
> end = s->endmant[ch];
?
>
> ff_ac3_bit_alloc_calc_psd((int8_t *)s->dexps[ch], start, end,
> s->psd[ch], s->bndpsd[ch]);
why the int8_t cast?
[...]
> // TODO only for debug
> for(ch=0; ch<=s->ntchans; ch++)
> memset(s->transform_coeffs[ch], 0, 256*sizeof(float));
then remove it or put it under #ifdef DEBUG_WHATEVER
[...]
> if (nratio < 0.0)
> nratio = 0.0;
> else if (nratio > 1.0)
> nratio = 1.0;
FFMIN/FFMAX
[...]
> filtbin = filtbin - 5;
-=5
[...]
> if (s->chgaqmod[ch] < 2){
> endbap = 12;
> }else{
> endbap = 17;
> }
>
> s->chactivegaqbins[ch] = 0;
> for(bin = 0; bin < s->endmant[ch]; bin++){
> if(s->hebap[ch][bin] > 7 && s->hebap[ch][bin] < endbap){
> s->chgaqbin[ch][bin] = 1; /* Gain word is present */
> s->chactivegaqbins[ch]++;
> }else if (s->hebap[ch][bin] >= endbap){
> s->chgaqbin[ch][bin] = -1;/* Gain word not present */
> }else{
> s->chgaqbin[ch][bin] = 0;
> }
> }
this is copy and pasted 1:1 from the pseudo code in the a52b spec
even the comments, and i suspect that applies to other parts as well
in principle that should be ok but well IANAL, diego whats your
oppinion about it? is it ok to copy and paste code from the spec?
or would it be better if we didnt do that, and what about the variable
names, they are practically all copy and pasted from the spec
[...]
> /**
> * Performs Inverse MDCT transform
> */
> static void do_imdct(EAC3Context *ctx)
> {
> int ch;
>
> for(ch=1; ch<=ctx->nfchans+ctx->lfeon; ch++) {
> if(ctx->blksw[ch]) {
> /* 256-point IMDCT */
> ff_ac3_do_imdct_256(ctx->tmp_output, ctx->transform_coeffs[ch],
> &ctx->imdct_256, ctx->tmp_imdct);
> } else {
> /* 512-point IMDCT */
> ctx->imdct_512.fft.imdct_calc(&ctx->imdct_512, ctx->tmp_output,
> ctx->transform_coeffs[ch],
> ctx->tmp_imdct);
> }
> /* apply window function, overlap/add output, save delay */
> ctx->dsp.vector_fmul_add_add(ctx->output[ch-1], ctx->tmp_output,
> ctx->window, ctx->delay[ch-1], 0,
> AC3_BLOCK_SIZE, 1);
> ctx->dsp.vector_fmul_reverse(ctx->delay[ch-1], ctx->tmp_output+256,
> ctx->window, AC3_BLOCK_SIZE);
> }
> }
>
> static int eac3_decode_frame(AVCodecContext *avctx, void *data, int *data_size,
> uint8_t *buf, int buf_size){
the { placement is inconsistant
[...]
> avctx->bit_rate = (c->frmsiz * (avctx->sample_rate) * 16 / ( ff_eac3_blocks[c->numblkscod] * 256)) / 1000;
superflous ()
[...]
> // convert float to 16-bit integer
> for(ch = 0; ch<avctx->channels; ch++) {
> for(i=0; i<AC3_BLOCK_SIZE; i++) {
> c->output[ch][i] = c->output[ch][i] * c->mul_bias +
> c->add_bias;
> }
> c->dsp.float_to_int16(c->int_output[ch], c->output[ch],
> AC3_BLOCK_SIZE);
inefficient
you shouldnt add 0 and mutiply by 1 but rather use 2 cases and
you should try to apply that bias at a more efficint point like on some
dequantization coeff or such if possible
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
The worst form of inequality is to try to make unequal things equal.
-- Aristotle
-------------- 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-soc/attachments/20070910/2b3684d9/attachment.pgp>
More information about the FFmpeg-soc
mailing list