[FFmpeg-devel] [PATCH] X-Face image encoder and decoder

[Clément Bœsch]

On Sun, Oct 14, 2012 at 01:30:33AM +0200, Stefano Sabatini wrote:
[...]
> > > +            return AVERROR(EINVAL);
> > > +        }
> > > +    }
> > > +    avctx->width  = XFACE_WIDTH;
> > > +    avctx->height = XFACE_HEIGHT;
> > > +
> > > +    /* convert image from MONOWHITE to 1=black 0=white bitmap */
> > > +    buf = frame->data[0];
> > > +    for (i = 0, j = 0; i < XFACE_PIXELS; ) {
> > > +        for (k = 0; k < 8; k++)
> > > +            xface->bitmap[i++] = !!((1<<(7-k)) & buf[j]);
> > 
> > Wouldn't it be faster/simpler to do something like (untested):
> > 
> >   xface->bitmap[i++] = buf[j]>>(7-k) & 1;
> > 
> > ?
> 
> changed to:
>     xface->bitmap[i++] = !!(buf[j] & (0x80>>k));
> 
> which I consider a bit more intelligible (and consistent with what I
> did in xface.c).
> 

FYI, it seems your original code and my proposal outputs the same assembler
with gcc (-O2), but your proposition adds a test:

uint8_t f1(uint8_t b, int k) { return !!((1<<(7-k)) & b); }
uint8_t f2(uint8_t b, int k) { return b>>(7-k) & 1;       }
uint8_t f3(uint8_t b, int k) { return !!(b & 0x80>>k);    }


0000000000000000 <f1>:
   0:   b9 07 00 00 00          mov    ecx,0x7
   5:   40 0f b6 c7             movzx  eax,dil
   9:   29 f1                   sub    ecx,esi
   b:   d3 f8                   sar    eax,cl
   d:   83 e0 01                and    eax,0x1
  10:   c3                      ret    

0000000000000020 <f2>:
  20:   b9 07 00 00 00          mov    ecx,0x7
  25:   40 0f b6 c7             movzx  eax,dil
  29:   29 f1                   sub    ecx,esi
  2b:   d3 f8                   sar    eax,cl
  2d:   83 e0 01                and    eax,0x1
  30:   c3                      ret    

0000000000000040 <f3>:
  40:   89 f1                   mov    ecx,esi
  42:   b8 80 00 00 00          mov    eax,0x80
  47:   40 0f b6 ff             movzx  edi,dil
  4b:   d3 f8                   sar    eax,cl
  4d:   85 f8                   test   eax,edi
  4f:   0f 95 c0                setne  al
  52:   c3                      ret    

(I'm not trying to argue about any solution, just found that funny)

Ah and BTW, I think you can make buf const.

[...]

-- 
Clément B.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 490 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20121014/dcb8d60d/attachment.asc>