[FFmpeg-devel] [PATCH] Indeo5 decoder

[Reimar Döffinger]

On Fri, Mar 27, 2009 at 04:22:40PM +0100, Maxim wrote:
> >> +            switch (get_bits(&ctx->gb,2)) {
> >> +                case 0:
> >> +                    mb_size = 16;
> >> +                    blk_size = 8;
> >> +                    break;
> >> +                case 1:
> >> +                    mb_size = 8;
> >> +                    blk_size = 8;
> >> +                    break;
> >> +                case 2:
> >> +                    mb_size = 8;
> >> +                    blk_size = 4;
> >> +                    break;
> >> +                case 3:
> >> +                    mb_size = 4;
> >> +                    blk_size = 4;
> >> +                    break;
> >> +            }
> >>     
> >
> > mb_size = blk_size =  4;
> > if (!get_bits1(...)) {
> >     mb_size  <<= 1;
> >     blk_size <<= 1;
> > }
> > if (!get_bits1(...))
> >     mb_size  <<= 1;
> >
> > Maybe?
> >   
> 
> this make the code less readable IMHO. Therefore leaved as is...

How about e.g.
blk_size = get_bits1(...) ? 4 : 8;
mb_size  = get_bits1(...) ? blk_size : 2*blksize;
or
blk_size = get_bits1(...) ? 4 : 8;
mb_size  = blk_size << !get_bits1(...);
or
blk_size = 4        << !get_bits1(...);
mb_size  = blk_size << !get_bits1(...);

The switch IMO simply makes it look too much like "magic numbers" when
it could be much more nicely interpreted as two independent flags - not
to mention 2 vs. 18 lines of code.