[FFmpeg-devel] [RFC] AAC Encoder

[Michael Niedermayer]

On Mon, Aug 18, 2008 at 01:03:18PM +0200, Michael Niedermayer wrote:
> On Mon, Aug 18, 2008 at 09:57:50AM +0300, Kostya wrote:
> > 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:
[...]
> >  
> > [...]
> > > > +    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.
> 
> the code was
> for(i = 1; i < info->num_windows; i++)
>     put_bits(&s->pb, 1, info->group_len[i]);
> 
> why can it not be
> for(i = 1; i < info->num_windows; i++)
>     put_bits(&s->pb, 1, !!info->group_len[i]);
> 
> ?
> 
> group_len here can be the actual lens

ping, this is currently blocking quite a few hunks in your patch.
I wont forget about it, nor will i skip going over all previous reviews
at the end to ensure nothing has been forgotten.

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

No great genius has ever existed without some touch of madness. -- 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-devel/attachments/20080823/9d078ba4/attachment.pgp>