[FFmpeg-devel] [PATCH] AAC Decoder - Round 2.

[Kostya]

On Fri, Jun 20, 2008 at 01:09:29PM +0200, Michael Niedermayer wrote:
> On Thu, Jun 19, 2008 at 04:22:57PM +0100, Robert Swain wrote:
[...]
> > +
> > +/**
> > + * Individual Channel Stream
> > + */
> > +typedef struct {
> 
> > +    int intensity_present;
> 
> This seems to be used to prevent intensity_tool() from being run if its not
> used anywhere, I do not think this is efficient.
> instead of
> 
> if(a.present)
>     for(i){
>         if(tab[i] == a)
>             do something
>     }
> 
> if(b.present)
>     for(i){
>         if(tab[i] == b)
>             do something
>     }
> 
> the following looks simpler
> 
>     for(i){
>         if(tab[i] == a)
>             do something
>         else if((tab[i] == b)
>             do something
>     }
> 
> or switch/case instead of if/else
> 
> Of course above is just a suggestion, i want the fastest and simplest code!
> But i think quite a bit of these specific "tools" could be merged into a
> single loop. the do_something itself could of course be a sperate function
> for clarity ...
 
I'm all for leaving it. It's quite useful in encoder.
 
> > +    int max_sfb;
> 
> whats a sfb?

Scalefactor band. A group of coefficients with a single quantizer.

[...]
> 
> > +    int start;
> > +    int offset[4];
> 
> start and offset[0] also are redundant, offset[0] is enough

They have different meaning - start is for indicating scalefactor band,
offsets go from start of it. It is useful for encoder. 
 
[...]
> > +    float * saved = sce->saved;
> 
> > +    const float * lwindow      = ics->window_shape      ? kbd_long_1024 : sine_long_1024;
> > +    const float * swindow      = ics->window_shape      ? kbd_short_128 : sine_short_128;
> > +    const float * lwindow_prev = ics->window_shape_prev ? kbd_long_1024 : sine_long_1024;
> > +    const float * swindow_prev = ics->window_shape_prev ? kbd_short_128 : sine_short_128;
> 
> duplicate 
 
no, just two possible cases - 128 and 1024 point windows,
each can be in one of two possible shapes

[...]

Also, maybe it would be better to submit decoder without SSR stuff,
it's currently #ifdef'ed out anyway. We can always add it later.
 
> [...]
> -- 
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> 
> In a rich man's house there is no place to spit but his face.
> -- Diogenes of Sinope