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

[Jimmy Christensen]

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.