Menu
▾
▴
#2948 Inconsistent color scaling
The Tk_GetColor() man page states: "When fewer than 16 bits are provided for each color, they represent the most significant bits of the color. For example, #3a7 is the same as #3000a0007000." The [photo] man page references the Tk_GetColor() man page regarding how color names are interpreted.
I just tested this in MS-Windows XP by putting the color #3a7 into a photo image and into a widget background. I then took a screenshot using The GIMP and read back the color values. The photo contained #33aa77, but the widget background contained #30a070. The colors are visibly different when compared side-by-side. I'm not sure how this inconsistency is possible, unless not everything is calling Tk_GetColor().
I prefer the bit replication technique used by photo, even though it's contrary to the man page. It makes #fff map to white, not light gray.
This looks like a simplification in the
function XParseColor (xlib/xcolors.c),
which assumes that the color string is
either 7 or 13 characters (i=2 or i=4),
because for other values the used
formulas don't do as expected:
colorPtr->red = (((unsigned short) red) << (4 * (4 - i)))
| ((unsigned short) red);
colorPtr->green = (((unsigned short) green) << (4 * (4 - i)))
| ((unsigned short) green);
colorPtr->blue = (((unsigned short) blue) << (4 * (4 - i)))
| ((unsigned short) blue);
Suggested fix attached
In case you get some strange javascript when trying to get to the
attachment (this is a sourceforge bug!!!). Here is the patch inline
*** xlib/xcolors.c.orig Fri Feb 10 10:21:51 2012
--- xlib/xcolors.c Fri Feb 10 10:59:10 2012
***************
*** 891,902 ****
if (sscanf(spec+1, fmt, &red, &green, &blue) != 3) {
return 0;
}
! colorPtr->red = (((unsigned short) red) << (4 * (4 - i)))
! | ((unsigned short) red);
! colorPtr->green = (((unsigned short) green) << (4 * (4 - i)))
! | ((unsigned short) green);
! colorPtr->blue = (((unsigned short) blue) << (4 * (4 - i)))
! | ((unsigned short) blue);
} else {
if (!FindColor(spec, colorPtr)) {
return 0;
--- 891,914 ----
if (sscanf(spec+1, fmt, &red, &green, &blue) != 3) {
return 0;
}
! switch (i) {
! case 1:
! colorPtr->red = 0x1111 * (unsigned short) red;
! colorPtr->green = 0x1111 * (unsigned short) green;
! colorPtr->blue = 0x1111 * (unsigned short) blue;
! case 2:
! colorPtr->red = (((unsigned short) red) << 8) | (unsigned short) red;
! colorPtr->green = (((unsigned short) green) << 8) | (unsigned short) green;
! colorPtr->blue = (((unsigned short) blue) << 8) | (unsigned short) blue;
! case 3:
! colorPtr->red = (((unsigned short) red) << 4) | (((unsigned short) red) >> 8);
! colorPtr->green = (((unsigned short) green) << 4) | (((unsigned short) green) >> 8);
! colorPtr->blue = (((unsigned short) blue) << 4) | (((unsigned short) blue) >> 8);
! case 4:
! colorPtr->red = (unsigned short) red;
! colorPtr->green = (unsigned short) green;
! colorPtr->blue = (unsigned short) blue;
! }
} else {
if (!FindColor(spec, colorPtr)) {
return 0;
Oops, forgot the breaks in the switch statement, but you get the idea ;-(
Guess what. Tried the same on UNIX and the same problem there. Looking
in the XParseColor manual:
The R, G, and B represent single hexadecimal digits. When
fewer than 16 bits each are specified, they represent the
most-significant bits of the value (unlike the ``rgb:'' syn-
tax, in which values are scaled). For example, the string
``#3a7'' is the same as ``#3000a0007000''.
This means that Tk_GetColor() behaves as documented,
the photo handler doesn't use Tk_GetColor for the
color parsing. However, as the whole world is using
the #hhh syntax as suggested here, I still recommend
to fix this. As a proof, here the relevant part of css-3:
The format of an RGB value in hexadecimal notation is a
‘#’ immediately followed by either three or six hexadecimal
characters. The three-digit RGB notation (#rgb) is converted
into six-digit form (#rrggbb) by replicating digits, not by adding
zeros. For example, #fb0 expands to #ffbb00. This ensures that
white (#ffffff) can be specified with the short notation (#fff) and
removes any dependencies on the color depth of the display.
The best Tk can do is implement this replication before
transferring the string to XParseColor.
The photo code used to use Tk_GetColor exclusively (or was it the objectified version?) but I changed it because that's still a brutally slow piece of code when you're dealing with a very large number of colors to parse (i.e., in an image!) When doing that, I maximized the color spread in the local color parser as I thought that most likely to give a pleasing effect in practice.
I'm still not keen on using Tk_GetColor in the photo code as I'd like to avoid the memory management overhead where possible (and the color _won't_ be kept around for long).
>When doing that, I maximized the color spread in
>the local color parser as I thought that most likely to give a pleasing
>effect in practice.
I agree with that change! But I think that color handling in
other places should do the same, and the change should
be documented.
Anyway, I will come up with a proposal for you to
evaluate. (please forget the patch I gave earlier!)
Thank you very much for the quick response! I've got a few bug reports, some with patches attached, that have been languishing in this tracker for years with no attention, so I was afraid to submit a report here. Instead I composed an email to tcl-core, then at the last second decided to dump it here anyway. Apparently I did the right thing!
proposed fix committed in branch bug-3486474
Solution:
- For Windows, rewrote XParseColor to
do the right thing in the fastest way: just
do the byte replication in a small buffer, and
use strtol in stead of sscanf.
- For other platforms, put a little wrapper
TkParseColor around XParseColor. This
function does the byte replication if
necessary and the calls the original
XParseColor.
Please, evaluate!
Little addendum committed, in fact only meant for Tk 8.6:
The function Tk_NameOfColor always outsput a 13-byte
color name, but if the color is exactly equivalent to a
converted 7-byte color-name then the shorter name
should be output. Not all external implementations
of colors understand the 13-byte variant, so this
is generally safer.
With that, we have a kind of 'canonical' color name,
in which the 7-byte variant is prefered, and the
13-byte variant is used otherwise.
Please evaluate!. Do others find that a good idea as well?
Regards,
Jan Nijtmans
Committed improved (speeded up) implementations of
XParseColor and TkParseColor:
- XParseColor now uses _strtoi64() function to parse
the hex-string in a single step, so the only thing left
is a little bit-magic getting the colors right. Should be
much faster than the original implementation.
- TkParseColor now loops through the characters
forwards first, then backwards, which can be
optimized much better by the C-compiler
I consider this ready to be committed, but
evaluation by others is still highly appreciated.
Fix committed to core-8-4-branch, core-8-5-branch and trunk.