Bug 690620 - low resolution fill adjust is wrong
Summary: low resolution fill adjust is wrong
Status: RESOLVED FIXED
Alias: None
Product: Ghostscript
Classification: Unclassified
Component: Graphics Library (show other bugs)
Version: master
Hardware: Macintosh MacOS X
: P4 normal
Assignee: Robin Watts
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-07-10 13:35 UTC by Henry Stiles
Modified: 2009-12-17 12:30 UTC (History)
1 user (show)

See Also:
Customer:
Word Size: ---


Attachments
rectfill.ps (373 bytes, application/postscript)
2009-07-10 13:44 UTC, Henry Stiles
Details
rectfill.tiff (2.48 KB, image/tiff)
2009-07-10 13:45 UTC, Henry Stiles
Details
foo.tiff (2.47 KB, image/tiff)
2009-07-10 13:51 UTC, Henry Stiles
Details
rectfill_frac_pixel.ps (557 bytes, application/postscript)
2009-07-14 09:02 UTC, Henry Stiles
Details
clipbug.ps (50 bytes, application/postscript)
2009-12-17 05:09 UTC, Robin Watts
Details
clipbug.ps (707 bytes, application/postscript)
2009-12-17 07:11 UTC, Robin Watts
Details
patch (3.67 KB, patch)
2009-12-17 07:16 UTC, Robin Watts
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Henry Stiles 2009-07-10 13:35:40 UTC
The default state of fill adjust in the graphics library for resolutions less
than 150 dpi is .25 and should be .50.  Unfortunately changing this will break
most of the tests in the regression suite.
Comment 1 Henry Stiles 2009-07-10 13:44:05 UTC
Created attachment 5207 [details]
rectfill.ps

postscript program to print out sample rectangular fills, the number on the
right is the height of the rectangle.  The actual pixel height of the
rectangles should be 1 if the height == 0 and ceil(height) otherwise, like the
adobe output.  Our output is "less than" any part of pixel due to only
adjusting fills by .25.
Comment 2 Henry Stiles 2009-07-10 13:45:37 UTC
Created attachment 5208 [details]
rectfill.tiff

96 dpi acrobat rendering.
Comment 3 Henry Stiles 2009-07-10 13:51:27 UTC
Created attachment 5209 [details]
foo.tiff

ghostscript's 96 dpi output.
Comment 4 Henry Stiles 2009-07-11 08:46:50 UTC
The attached tiff files may be opened in Adobe Acrobat (8 and 9), set the
resolution to 96 dpi and turn off smoothing options.  A magnifier like "pixie"
(mac) could be used to look at the pixel width of the lines.  Imagemagick's
display program has a built in magnifier and shows device coordinates.
Comment 5 Henry Stiles 2009-07-14 09:02:34 UTC
Created attachment 5211 [details]
rectfill_frac_pixel.ps

Attached is a more comprehensive filling test, this is a 31 page test the first
page tests a range of rectangle fill heights and subsequent pages translate the
test by a small amount in an effort to see how fractional pixel moves affect
the filling.  The current gs rectangle fill algorithm does not match adobe
acrobat 8.
Comment 6 Henry Stiles 2009-07-15 08:27:38 UTC
The following code change prints the comprehensive test (see the attachment in
comment #5) reasonably and the regression fixing several problems in the ps
cets.  It also is a reasonable interpretation of the mathematical description of
the filling algorithm from the postscript and pdf manual (6.5.3 scan conversion
rules), contrasting markedly with the code it replaces.  I am posting the patch
here for feedback and counterexamples.

At 72 dpi the patch does give a pixel for pixel match of the rectangles in the
comprehensive test (the current code has many discrepancies), with a few
exceptional cases where the adobe renderer appears incorrect.

NOTE: the code should not be checked in yet since it has not been tested with
languages other than postscript and pdf.

svn diff
Index: gsdps1.c
===================================================================
--- gsdps1.c	(revision 9859)
+++ gsdps1.c	(working copy)
@@ -243,19 +243,21 @@
 		}
 	    } else {
 		int x, y, w, h;
-
-		draw_rect.p.x -= max(pgs->fill_adjust.x - fixed_epsilon, 0);
-		draw_rect.p.y -= max(pgs->fill_adjust.y - fixed_epsilon, 0);
-		draw_rect.q.x += pgs->fill_adjust.x;
-		draw_rect.q.y += pgs->fill_adjust.y;
 		rect_intersect(draw_rect, clip_rect);
-		x = fixed2int_pixround(draw_rect.p.x);
-		y = fixed2int_pixround(draw_rect.p.y);
-		w = fixed2int_pixround(draw_rect.q.x) - x;
-		h = fixed2int_pixround(draw_rect.q.y) - y;
-		if (w > 0 && h > 0)
-    		    if (gx_fill_rectangle(x, y, w, h, pdc, pgs) < 0)
-			goto slow;
+		draw_rect.p.x = fixed_floor(draw_rect.p.x);
+		draw_rect.p.y = fixed_floor(draw_rect.p.y);
+                draw_rect.q.x = fixed_ceiling(draw_rect.q.x);
+                draw_rect.q.y = fixed_ceiling(draw_rect.q.y);
+		x = fixed2int(draw_rect.p.x);
+		y = fixed2int(draw_rect.p.y);
+		w = fixed2int(draw_rect.q.x - draw_rect.p.x);
+		h = fixed2int(draw_rect.q.y - draw_rect.p.y);
+                if (w == 0)
+                    w = 1;
+                if (h == 0)
+                    h = 1;
+                if (gx_fill_rectangle(x, y, w, h, pdc, pgs) < 0)
+                    goto slow;
 	    }
 	}
 	return 0;
Comment 7 Henry Stiles 2009-07-16 21:15:26 UTC
The patch in #6 is improved to support center of pixel clients, pass through 0
area rectangles to high level devices, and emulate the peculiarities of Adobe
Acrobate 8 0 fills.  With the latter change the rectangles in
rectfill_frac_pixel.ps match adobe pixel for pixel at 72 dpi.  The patch has now
been regression tested against all the languages without issues.

svn diff
Index: gsdps1.c
===================================================================
--- gsdps1.c	(revision 9862)
+++ gsdps1.c	(working copy)
@@ -196,7 +196,9 @@
     bool hl_color = (hl_color_available && 
 		dev_proc(pdev, fill_rectangle_hl_color)(pdev, 
 		    	    &empty, pis, pdc, NULL) == 0);
+    bool center_of_pixel = (pgs->fill_adjust.x == 0 && pgs->fill_adjust.y == 0);
 
+    
     /* Processing a fill object operation */
     gs_set_object_tag(pgs, GS_PATH_TAG);
 
@@ -234,8 +236,14 @@
 	    draw_rect.q.y = max(p.y, q.y);
 	    if (hl_color) {
 		rect_intersect(draw_rect, clip_rect);
-		if (draw_rect.p.x < draw_rect.q.x &&
-		    draw_rect.p.y < draw_rect.q.y) {
+                /* We do pass on 0 extant rectangles to high level
+                   devices.  It isn't clear how a client and an output
+                   device should interact if one uses a center of
+                   pixel algorithm and the other uses any part of
+                   pixel.  For now we punt and just pass the high
+                   level rectangle on without adjustment. */
+		if (draw_rect.p.x <= draw_rect.q.x &&
+		    draw_rect.p.y <= draw_rect.q.y) {
 		    code = dev_proc(pdev, fill_rectangle_hl_color)(pdev,
 			     &draw_rect, pis, pdc, pcpath);
 		    if (code < 0)
@@ -243,19 +251,37 @@
 		}
 	    } else {
 		int x, y, w, h;
-
-		draw_rect.p.x -= max(pgs->fill_adjust.x - fixed_epsilon, 0);
-		draw_rect.p.y -= max(pgs->fill_adjust.y - fixed_epsilon, 0);
-		draw_rect.q.x += pgs->fill_adjust.x;
-		draw_rect.q.y += pgs->fill_adjust.y;
 		rect_intersect(draw_rect, clip_rect);
-		x = fixed2int_pixround(draw_rect.p.x);
-		y = fixed2int_pixround(draw_rect.p.y);
-		w = fixed2int_pixround(draw_rect.q.x) - x;
-		h = fixed2int_pixround(draw_rect.q.y) - y;
-		if (w > 0 && h > 0)
-    		    if (gx_fill_rectangle(x, y, w, h, pdc, pgs) < 0)
-			goto slow;
+                if (center_of_pixel) {
+                    draw_rect.p.x = fixed_rounded(draw_rect.p.x);
+                    draw_rect.p.y = fixed_rounded(draw_rect.p.y);
+                    draw_rect.q.x = fixed_rounded(draw_rect.q.x);
+                    draw_rect.q.y = fixed_rounded(draw_rect.q.y);
+                } else { /* any part of pixel rule - touched */
+                    draw_rect.p.x = fixed_floor(draw_rect.p.x);
+                    draw_rect.p.y = fixed_floor(draw_rect.p.y);
+                    draw_rect.q.x = fixed_ceiling(draw_rect.q.x);
+                    draw_rect.q.y = fixed_ceiling(draw_rect.q.y);
+                }
+		x = fixed2int(draw_rect.p.x);
+		y = fixed2int(draw_rect.p.y);
+		w = fixed2int(draw_rect.q.x - draw_rect.p.x);
+		h = fixed2int(draw_rect.q.y - draw_rect.p.y);
+                /* clients that use the "any part of pixel" rule also
+                   fill 0 areas.  This is true of current graphics
+                   library clients but not a general rule.  */
+                if (!center_of_pixel) {
+                    if (w == 0)
+                        w = 1;
+                    /* yes Adobe Acrobat 8, seems to back up the y
+                       coordinate when the width is 0, sigh. */
+                    if (h == 0) {
+                        y--;
+                        h = 1;
+                    }
+                }
+                if (gx_fill_rectangle(x, y, w, h, pdc, pgs) < 0)
+                    goto slow;
 	    }
 	}
 	return 0;
Comment 8 Henry Stiles 2009-11-19 07:50:30 UTC
Reassigning path and fill problems to Robin Watts.
Comment 9 Robin Watts 2009-12-13 10:36:42 UTC
Henry - is there are reason why the patch in comment #7 can't just be
tested/committed? I can't see any outstanding issues...
Comment 10 Henry Stiles 2009-12-13 11:04:51 UTC
Not that I can think of, but I'd rather you go forward with it, I've shifted my
attention elsewhere and it would take me a while to wrap my head around this
again.  A separate but related problem I do remember is the pxl client has
changed on the HP Color Laserjet 4600, rendering the same as postscript using an
any-part-of-pixel (not centered) model, but we can deal with that by changing
the XL client.  This code should be okay.
Comment 11 Robin Watts 2009-12-17 05:09:39 UTC
Created attachment 5788 [details]
clipbug.ps

Testing with local regressions/htmldiff shows problems in various of the ps3cet
test files, whereby a single dot appears where it shouldn't (09-01.ps for
example, page 2). I've done some investigation and boiled the problem down to
the attached file.

The problem is that the new code doesn't cope well with a zero size clipping
rectangle.
Comment 12 Robin Watts 2009-12-17 07:11:21 UTC
Created attachment 5789 [details]
clipbug.ps

A more complex version of clipbug.ps that demonstrates rectfilling an area
constrained by 6 different clipping paths. The clipping path in each case is:

 1) A moveto
 2) A moveto, close
 3) A moveto, lineto (same position), close
 4) A moteto, lineto (horizontal change), close
 5) A moteto, lineto (vertical change), close
 6) A moteto, lineto (diagonal change), close

Acrobat only marks the page for 4,5,6. Unpatched ghostscript for 4 and 6.
Patched ghostscript marks the page for all but 5.
Comment 13 Robin Watts 2009-12-17 07:16:21 UTC
Created attachment 5790 [details]
patch

Updated version of henrys patch that returns patched ghostscript to only
getting rendering as wrong as unpatched ghostscript currently does.

Basically, we add an additional few lines to ensure that we don't mark the page
when the clip is truly empty.

@@ -219,6 +220,10 @@
	gs_fixed_rect clip_rect;

	gx_cpath_inner_box(pcpath, &clip_rect);
+	/* We should never plot anything for an empty clip rectangle */
+	if ((clip_rect.p.x >= clip_rect.q.x) &&
+	    (clip_rect.p.y >= clip_rect.q.y))
+	    return 0;
	for (i = 0; i < count; ++i) {
	    gs_fixed_point p, q;
	    gs_fixed_rect draw_rect;
Comment 14 Ken Sharp 2009-12-17 07:23:28 UTC
FWIW Jaws marks the page the same as Acrobat; 4, 5 and 6.
Comment 15 Robin Watts 2009-12-17 12:30:44 UTC
The patch has has been committed as revision 10513.
The clipping bug seen with clipbug.ps has been opened as bug 691008.
Therefore I'm closing this bug.