[FFmpeg-devel] [PATCH] Add DPX decoder rev-16

[Reimar Döffinger]

On Wed, Jun 03, 2009 at 07:19:17PM +0200, Jimmy Christensen wrote:
> On 2009-06-03 18:25, Reimar D?ffinger wrote:
> > On Wed, Jun 03, 2009 at 06:07:59PM +0200, Jimmy Christensen wrote:
> >> +            *dst++ = (((rgbBuffer&  0xFFC00000)>>  16) + ((rgbBuffer&  0xFFC00000)>>  26));
> >
> > The second&-masking is useless.
> 
> You're right. Can change that.
> 
> >
> >> +            *dst++ = (((rgbBuffer&  0x003FF000)>>  6) + ((rgbBuffer&  0x003FF000)>>  16));
> >> +            *dst++ = (((rgbBuffer&  0x00000FFC)<<  4) + ((rgbBuffer&  0x00000FFC)>>  6));
> >
> > having the&  0x... twice is ugly.
> >
> > Also I think it would be more readable as e.g.
> > dst[0] = rgbBuffer>>  16;
> > dst[1] = rgbBuffer>>   6;
> > dst[2] = rgbBuffer<<   4;
> > for (i = 0; i<  3; i++) {
> >      // mask away invalid bits
> >      dst[i]&= 0xFFC0;
> >      // correctly expand to 16 bits
> >      dst[i] += dst[i]>>  10;
> > }
> >
> > Note that if you are pedantic, in that loop
> > dst[i] = (dst[i]&  0xFFC0) + (dst[i]>>  10);
> > might be faster since it removes a data dependency.
> 
> The double masking might be ugly, but the code you presented made the 
> total encode time about 9% slower (including encoding to x264 Quicktime 
> from 46 fps to 42 fps). For readability I would say that the macros I 
> had in the first place were a lot better, and only impacted performance 
> about 0.9% of total encode time.

Hm. I really wonder which compiler you use.
Anyway I guess Michael would accept a version using macros if it was
done without adding useless calculation.
Except that it's rather silly to use macros in cases like this when
there is absolutely no reason to.
Example:
static inline unsigned make_16bit(unsigned value)
{
    // mask away invalid bits
    value &= 0xFFC0;
    // correctly expand to 16 bits
    return value + (value >> 10);
}

            *dst++ = make_16bit(rgbBuffer >> 16);
            *dst++ = make_16bit(rgbBuffer >>  6);
            *dst++ = make_16bit(rgbBuffer <<  4);