| Summary: | Rounding errors in xcursorgen premultiplication | ||||||
|---|---|---|---|---|---|---|---|
| Product: | xorg | Reporter: | Jonathan Lennox <lennox> | ||||
| Component: | App/other | Assignee: | Keith Packard <keithp> | ||||
| Status: | CLOSED FIXED | QA Contact: | |||||
| Severity: | normal | ||||||
| Priority: | high | CC: | keithp | ||||
| Version: | unspecified | ||||||
| Hardware: | x86 (IA32) | ||||||
| OS: | FreeBSD | ||||||
| Whiteboard: | |||||||
| i915 platform: | i915 features: | ||||||
| Bug Depends on: | |||||||
| Bug Blocks: | 5041 | ||||||
| Attachments: |
|
||||||
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.
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.