When xcursorgen translates PNG files to X cursors, it performs premultiplication on the alpha channels. However, this premultiplication is done with integer arithmetic, and so loses precision. For example, one pixel in redglass/xterm-24.png (the fourth pixel in line 2) has alpha=62 and red=164. In integer arithmetic, (164 * 62) / 255 = 39. Mathematically, however, 164 * 62/255 = 39.8745, so the premultiplied pixel would be better expressed as 40. To fix this, the line in the function premultiply_data() red = (unsigned) red * (unsigned) alpha / 255; should be rewritten as red = (unsigned) round(red * (alpha / 255.0)); and similarly for green and blue. The file will need "#include <math.h>" added to the top. xcursorgen already links with libm, so the use of round(3) is not a problem. I can construct a proper patch if desired.
please do send a patch, so we can commit it for 6.9/7.0. cheers.
reporter: ping. do you have a patch for this?
I can construct a patch, but on reflection I believe that using round() is inadvisable, since I think it's new in C99. Is it acceptable to use rint() in X sources? That seems to be ancient (FreeBSD's man page claims it was in Version 6 Unix), but its specification has some language I don't understand about "raising an inexact exception".
Created attachment 3100 [details] [review] Patch to use rint() in xcursorgen Here's the patch, as mentioned, to use rint() in xcursorgen.
keith, can you review this please?
why not just use the div_255 macro from fb? #define div_255(x) (((x) + 0x80 + (((x) + 0x80) >> 8)) >> 8)
so like this instead: diff -u -d -p -r1.3 xcursorgen.c --- programs/xcursorgen/xcursorgen.c 3 Dec 2004 22:21:22 -0000 1.3 +++ programs/xcursorgen/xcursorgen.c 18 Nov 2005 18:26:34 -0000 @@ -149,6 +149,8 @@ read_config_file (char *config, struct f return count; } +#define div_255(x) (((x) + 0x80 + (((x) + 0x80) >> 8)) >> 8) + static void premultiply_data (png_structp png, png_row_infop row_info, png_bytep data) { @@ -163,9 +165,9 @@ premultiply_data (png_structp png, png_r unsigned char alpha = base[3]; XcursorPixel p; - red = (unsigned) red * (unsigned) alpha / 255; - green = (unsigned) green * (unsigned) alpha / 255; - blue = (unsigned) blue * (unsigned) alpha / 255; + red = div_255(red * alpha); + green = div_255(red * alpha); + blue = div_255(red * alpha); p = (alpha << 24) | (red << 16) | (green << 8) | (blue << 0); memcpy (base, &p, sizeof (XcursorPixel)); }
uh, more like this: diff -u -d -p -r1.3 xcursorgen.c --- xcursorgen.c 3 Dec 2004 22:21:22 -0000 1.3 +++ xcursorgen.c 2 Dec 2005 17:08:03 -0000 @@ -149,6 +149,8 @@ read_config_file (char *config, struct f return count; } +#define div_255(x) (((x) + 0x80 + (((x) + 0x80) >> 8)) >> 8) + static void premultiply_data (png_structp png, png_row_infop row_info, png_bytep data) { @@ -163,9 +165,9 @@ premultiply_data (png_structp png, png_r unsigned char alpha = base[3]; XcursorPixel p; - red = (unsigned) red * (unsigned) alpha / 255; - green = (unsigned) green * (unsigned) alpha / 255; - blue = (unsigned) blue * (unsigned) alpha / 255; + red = div_255((unsigned)red * (unsigned)alpha); + green = div_255((unsigned)green * (unsigned)alpha); + blue = div_255((unsigned)blue * (unsigned)alpha); p = (alpha << 24) | (red << 16) | (green << 8) | (blue << 0); memcpy (base, &p, sizeof (XcursorPixel)); }
not a blocker.
fixed in head.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.