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

[Stefano Sabatini]

On date Sunday 2012-10-14 01:46:39 +0200, Clément Bœsch encoded:
> 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)

The !! is tricking gcc. Anyway changed globally to:
(buf[j]>>(7-k))&1;

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

Changed.

I'll push the patch in a day if I read no more comments.
-- 
FFmpeg = Funny Frenzy Mastering Portable Enhancing Guru
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-lavc-add-xface-image-decoder-and-encoder.patch
Type: text/x-diff
Size: 36323 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20121014/a80db7d7/attachment.bin>