[FFmpeg-soc] EAC3 review

[Justin Ruggles]

Michael Niedermayer wrote:
> 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:
> [...]
>>     // 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?

This has been brought up before. :)  Personally, I like using the names 
from the spec, but I can also see the ugliness of it.  I like it because 
it makes the code easier *for me* to understand, but I do agree that 
it's not ideal if you're trying to figure out what the code is doing. 
We could maybe strike a balance.  But I just realized that I should 
probably not be the one who makes the decisions on the balancing. :) 
Pretty much all of the AC3 spec variable names are embedded in my brain 
from using them almost daily for the past couple years.

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

Part of this would be my fault.  The AC3 decoder mixes those names too. 
  I agree that it should be fixed.  Using "tab" seems the better choice 
since most of the tables in ac3tab.c use that.

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

"mixing metadata exists"  yeah...ugly abbreviation

In several cases, these variables can be avoided altogether.  I can see 
how it was useful to parse each field for debugging purposes while it 
was being developed, but now it makes more sense to just do things such as:
/* Mixing Metadata */
if(get_bits1(gbc)) {
     [parse stuff here]
}

rather than store the 1 bit in a variable that's just used one time, 
right after it's read.

>>         ff_ac3_bit_alloc_calc_psd((int8_t *)s->dexps[ch], start, end,
>>                 s->psd[ch], s->bndpsd[ch]);
> 
> why the int8_t cast?

That's leftover from Fabrice's original AC3 encoder, which uses int8_t 
for exponents.  That function is shared between the encoder and decoder. 
  I see no reason why it can't be changed, but it would have to be 
across-the-board.

> [...]
>>         // 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
> 
> [...]

Something similar is done in the current AC3 decoder.  I agree that it 
should be applied at a more efficient point, but I don't think it's 
possible with downmixing done as it is currently.  Well, the 
multiplication could be done earlier, but not the addition.  An 
alternative could be to (if needed) undo the addition before mixing the 
channels, then add the bias back into the combined channel.  Any other 
suggestions would be great though.  I don't much like that this has to 
be done in order to use the optimized conversion, but in the end it's 
still faster than the plain C conversion.

The ideal would be to implement downmixing before the IMDCT.  This can 
be done, but it's not really straight-forward if the channels being 
combined use different transform lengths.  It can be done though.

-Justin