[FFmpeg-soc] EAC3 review

[Michael Niedermayer]

Hi

On Mon, Sep 10, 2007 at 10:51:29PM -0400, Justin Ruggles wrote:
> 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. :)  

yes, every time i look at it i guess
ive already considered to write a script to rename these so i can properly
review the code ...


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

yes, but would english words and common abbreviations really be harder
to understand for you?

s->full_bw_channels vs. s->nfchans
s->channels vs. s->ntchans

ive no problem if a codec has maybe 10 odd abbreviations or so but in AC3
every damn variable has a meaningless name made of concatenating words while
droping random letters


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

yes, why not datae ? why did they drop the last letter of data and add the
first letter of exists? iam wondering what the native language of the people
was who designed EAC-3 ...


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

yes


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

well then at least the multiply could be moved, and the addition could
be kept at the current position ...

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I count him braver who overcomes his desires than him who conquers his
enemies for the hardest victory is over self. -- 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/20070911/cab73e3a/attachment.pgp>