<div dir="ltr">OK thanks for clearing it up Max. The way I came across it is that in implementing 16-bit support on the PSP, I used a colormasking feature of the hardware, which takes a 32 bit ARGB value as its parameter. Converting the way that we do doesn't give the proper ARGB value causing the PSP not to mask it, so I had to look into what the problem was. <div>
<br></div><div>I guess since ScummVm works with at most 16 bits, this isn't really a big issue in general, since any conversion of a 16-bit format to RGB and back to another 16bit format should result in minimal loss. As such, I don't see a reason to rewrite the pixelFormat code unless someone else can think of one. </div>
<div><br></div><div>Yotam</div><div><br><br><div class="gmail_quote">On Thu, Mar 18, 2010 at 3:36 AM, Max Horn <span dir="ltr"><<a href="mailto:max@quendi.de">max@quendi.de</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
Hi Yotam,<br>
<br>
Our implementation of pixelFormat.colorToRGB is not incorrect. It converts a given color to an RGB triple (r,g,b) with the property that converting that RGB triple back to a color gives the same color as we started with. As such, it conforms to its specs to the letter. And it does computations very, very fast.<br>
<br>
However, you may have extra requirements on the RGB triple which make this particular conversion undesirable; in the end, it's a trade-off between speed and accuracy that has been made here. Whether this trade off is a good one depends on context, and of course we can re-evaluate it. This is why the ColorMask documentation says this:<br>
<br>
"<br>
The k*Bits and k*Shift values can be used to extract R,G,B. I.e. to get<br>
the red color component of a pixel, as a 8-bit value, you would write<br>
<br>
R = ((color & kRedMask) >> kRedShift) << (8-kRedBits)<br>
<br>
Actually, instead of the simple left shift, one might want to use somewhat<br>
more sophisticated code (which fills up the least significant bits with<br>
appropriate data).<br>
"<br>
<br>
This comment refers precisely to the "trick" Marcus already explained.<br>
<br>
So, it might be "nicer" if a fully saturated color in 565 format is converted to the RGB triple (255,255,255) instead of (248,252,248). But then again, what "nice" means really depends on the context; and colorToRGB resp. the ColorMask class never claimed that fully saturated colors are mapped to (255,255,255). Maybe it is desirable to do so, in that case, we can document this as a mandatory requirement (in the "spec", i.e. source comments for the class/functions) and modify the code accordingly.<br>
<br>
Note that so far it seems that the difference between those two values does not really matter :-). Now, everything having been said, I don't mind if you want to rewrite the code to use a different scheme, e.g. based on what Marcus wrote.<br>
<br>
<br>
Cheers,<br>
Max<br>
------------------------------------------------------------------------------<br>
Download Intel® Parallel Studio Eval<br>
Try the new software tools for yourself. Speed compiling, find bugs<br>
proactively, and fine-tune applications for parallel performance.<br>
See why Intel Parallel Studio got high marks during beta.<br>
<a href="http://p.sf.net/sfu/intel-sw-dev" target="_blank">http://p.sf.net/sfu/intel-sw-dev</a><br>
_______________________________________________<br>
Scummvm-devel mailing list<br>
<a href="mailto:Scummvm-devel@lists.sourceforge.net">Scummvm-devel@lists.sourceforge.net</a><br>
<a href="https://lists.sourceforge.net/lists/listinfo/scummvm-devel" target="_blank">https://lists.sourceforge.net/lists/listinfo/scummvm-devel</a><br>
</blockquote></div><br></div></div>