[FFmpeg-devel] [RFC] AAC Encoder

[Kostya]

On Sun, Aug 17, 2008 at 02:59:01PM +0200, Michael Niedermayer wrote:
> On Sun, Aug 17, 2008 at 02:40:19PM +0300, Kostya wrote:
[...]
> > > anyway iam not arguing for the encoder to do exactly what the decoder does ATM
> > > but it must be clean, compact, optimal and decoder and encoder should match
> > > each other whenever possible.
> > > 
> > > Could you maybe point to exactly what code would become uglier with the
> > > style used by the decoder? ideally with a diff showing the uglification?
> > 
> > they are almost the same to the extent decoder knows all limits and can
> > exploit it and my encoder have data for all windows and just marks window
> > group starts.
> 
> can the decoder be changed to use the system of the encoder?
> I really would prefer if both where using the same system ...
> also [w*16 + i] can be written as [w][i] which is a little cleaner

we have either up to 51 scalefactor bands in one window or
up to 15 scalefactor bands in 8 windows, so flat array is 4 times smaller
than of two-dimensional one
 
[...]
> > > [...]
> > > > +/**
> > > > + * Encode scalefactors.
> > > > + */
> > > > +static void encode_scale_factors(AVCodecContext *avctx, AACEncContext *s, ChannelElement *cpe, int channel, int global_gain)
> > > > +{
> > > > +    int off = global_gain, diff;
> > > > +    int i, w, wg;
> > > > +
> > > > +    w = 0;
> > > > +    for(wg = 0; wg < cpe->ch[channel].ics.num_window_groups; wg++){
> > > > +        for(i = 0; i < cpe->ch[channel].ics.max_sfb; i++){
> > > > +            if(!cpe->ch[channel].zeroes[w*16 + i]){
> > > > +                /* if we have encountered scale=256 it means empty band
> > > > +                 * which was decided to be coded by encoder, so assign it
> > > > +                 * last scalefactor value for compression efficiency
> > > > +                 */
> > > > +                if(cpe->ch[channel].sf_idx[w*16 + i] == 256)
> > > > +                    cpe->ch[channel].sf_idx[w*16 + i] = off;
> > > 
> > > why is th code that selects scalefactors not simply setting it to the last
> > > scale factor?
> > 
> > it may occur before first band with scale too 
> 
> for(i=end; i>=0; i--)
>     if(sf[i] == 256) sf[i]=last;
>     else             last= sf[i];
> 
> as a sideeffect s[0] will contain the "global_gain" which ive seen some
> complicated code to find because the firsts may be 256
 
what about the last bands then? They are usually empty and encoder
probably can choose to encode one in the run too.
 
[...]
> > +    int wg;
> >  
> >      put_bits(&s->pb, 1, 0);                // ics_reserved bit
> >      put_bits(&s->pb, 2, info->window_sequence[0]);
> > @@ -248,15 +334,307 @@
> >          put_bits(&s->pb, 1, 0);            // no prediction
> >      }else{
> >          put_bits(&s->pb, 4, info->max_sfb);
> > -        for(i = 1; i < info->num_windows; i++)
> > -            put_bits(&s->pb, 1, info->group_len[i]);
> > +        for(wg = 0; wg < info->num_window_groups; wg++){
> > +            if(wg)
> > +                put_bits(&s->pb, 1, 0);
> > +            if(info->group_len[wg] > 1)
> > +                put_sbits(&s->pb, info->group_len[wg] - 1, 0xFF);
> > +        }
> > +    }
> > +}
> 
> is the change in group_len content leading to an overall simplifiation?
> Because it makes the code distinctly worse here
> Also the way i understood you was that you didnt want arrays compacted
> like that
 
there are two approaches - store bit mask that indicates window belonging
to the group or starting new group, and storing group lengths explicitly.

First approach gives a lot of if(group_len[wg]) continue and simplier put_ics_info(),
second one makes put_ics_info() uglier but better handle group lengths.
 
[...]
> > +        for(cb = 0; cb < 12; cb++){
> > +            int sum = 0;
> > +            for(j = 1; j <= max_sfb - i; j++){
> 
> > +                if(band_bits[i+j-1][cb] == INT_MAX)
> > +                    break;
> > +                sum += band_bits[i+j-1][cb];
> 
> if(sum<0) break;

bad condition, consider the case:
sum = 0, band_bits[] = INT_MAX, bits < 0 => wrong path

[...]

Since I'll be computerless for a week you can rest from me and
we'll continue then. Some changes are in SoC SVN already.

> -- 
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> 
> While the State exists there can be no freedom; when there is freedom there
> will be no State. -- Vladimir Lenin