[Scummvm-devel] colorToRGB() implementation

yotam barnoy yotambarnoy at gmail.com
Wed Mar 17 09:05:27 CET 2010


If it's efficiency we're after, then the multiplication is a small part of
the cost - division is much more expensive (about 4 times more on MIPS).

The problem is that we need to multiply by more correct ratios than our
shifting simplifications allow. For example to convert a 5 bit channel to 8
bits, we need to multiply by 255/31 = 8.225 instead of 8, which is what we
currently do.

Since the number of cases we're using is limited (reduced a,r,g,b range from
1 bit to 7 bits, with some cases being trivial) , we could provide
specialized code (perhaps in templates?) to do the conversion. 8.23 means
roughly an extra unit every 5 jumps, so code like
                if (inputbits > 5) outputbits++;
                if (inputbits > 10) outputbits++;
                if (inputbits > 15) outputbits++;
                ...
                outputbits += inputbits<<3;

should do the trick (obviously paying attention to the exact point where
addition of a unit is needed). This should also be better than a lookup
table.

Yotam



On Wed, Mar 17, 2010 at 9:35 AM, Vladimir Menshakov <whoozle at yandex.ru>wrote:

> Multiplication could be harmful on portable platforms in a place which
> could be called for every pixel of the screen. Shifting is much cheaper. We
> need to check that gcc optimizes multiplication by 255 as '(m << 8) - m', or
> consider adding 7 there ;)
> 17.03.10, 08:35, "yotam barnoy" <yotambarnoy at gmail.com>:
>
> Hey guys
>
> I came across our implementation of pixelFormat.colorToRGB() and
> colorToRGBA() implementation and it seems incorrect to me. Currently it's
>
> inline void colorToRGB(uint32 color, uint8 &r, uint8 &g, uint8 &b) const {
>  r = ((color >> rShift) << rLoss) & 0xFF;
> g = ((color >> gShift) << gLoss) & 0xFF;
> b = ((color >> bShift) << bLoss) & 0xFF;
>
> The problem is, you can't ever restore the intended r,g,b values from the
> original 16-bit color. For example in R5G5B5  mode, 0x7FFF, which is
> supposed to be the brightest white, will only become 248,248,248 instead of
> 255,255,255. What we really need to do is
>
> r = (((color >> rShift) & rMask) * 255) / rMax();
>
> in order to translate the values correctly. rMax() should probably be saved
> in the struct for speed.
>
> The same criticism applies to the conversion in colormasks.h.
>
> Feedback?
>
> Yotam
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.scummvm.org/pipermail/scummvm-devel/attachments/20100317/e423362f/attachment.html>


More information about the Scummvm-devel mailing list