Bug 690466 - Bits of image missing
Summary: Bits of image missing
Status: CONFIRMED
Alias: None
Product: Ghostscript
Classification: Unclassified
Component: Graphics Library (show other bugs)
Version: master
Hardware: All All
: P3 normal
Assignee: Robin Watts
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-05-04 17:10 UTC by Marcos H. Woehrmann
Modified: 2021-01-03 17:22 UTC (History)
3 users (show)

See Also:
Customer:
Word Size: ---


Attachments
dopout-fix.patch (1.22 KB, patch)
2009-05-13 16:12 UTC, Ray Johnston
Details | Diff
D.pdf (14.33 KB, application/pdf)
2019-10-29 15:06 UTC, Ray Johnston
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Marcos H. Woehrmann 2009-05-04 17:10:25 UTC
The customer reports and I've verified that the attached PDF file is rendered by Ghostscript with some bits 
missing.  The customer reported this with 8.64 but head (r9720) behaves the same way.

The command line I'm using for testing:

  bin/gs -sDEVICE=tiffpack -r600 -o test.tif ./PR_010_e.pdf

Note that his is a small portion of a large file.
Comment 1 Marcos H. Woehrmann 2009-05-04 17:10:49 UTC
Created attachment 4992 [details]
PR_010_e.pdf
Comment 2 Alex Cherepanov 2009-05-04 20:57:40 UTC
Created attachment 4995 [details]
simplified sample file
Comment 3 Alex Cherepanov 2009-05-04 21:05:20 UTC
This PDF file uses strange way to stroke a 0-width line.
The file draws a path through a few points, then draws the path
back through the same points and fills the result.

This is a PostScript program similar to the sample PDF file.
Ghostscript drops one of the segments at 600 dpi.

%!
<</PageSize [20 20]>> setpagedevice

/m /moveto load def
/l /lineto load def
/h /closepath load def
/f /fill load def

[1 0 0 1 10.9199982 10.1600037] concat

0 0 m
0 0.36 l
0.36 1.26 l
0.72 1.8 l
1.26 2.16 l
2.7 2.16 l
3.24 1.8 l
3.6 1.26 l
3.96 0.36 l
3.96 -0.36 l
3.6 -1.26 l
2.88 -2.52 l
-0.18 -6.84 l
4.32 -6.84 l
-0.18 -6.84 l
2.88 -2.52 l
3.6 -1.26 l
3.96 -0.36 l
3.96 0.36 l
3.6 1.26 l
3.24 1.8 l
2.7 2.16 l
1.26 2.16 l
0.72 1.8 l
0.36 1.26 l
0 0.36 l
h
f
showpage
Comment 4 Ray Johnston 2009-05-09 20:27:31 UTC
I am testing a change on the cluster now. I am pretty convinced that there was
a duplicate allowance for the 'fixed_epsilon' both in setting the 'adjust_left'
and 'adjust_below' and then later in the fill code using a test using the
'fixed2int_pixround' that includes the epsilon to check a half-open interval.

Local testing the customer file results in no dropout (missing segments) and
doesn't seem to introduce any extra 'bold' segments. Comparison to Adobe
Acrobat Pro 'Save To ...' TIFF looks the same as Ghostscript with this change.
Comment 5 Ray Johnston 2009-05-13 16:12:44 UTC
Created attachment 5016 [details]
dopout-fix.patch

This patch does fix the dropout and the raster EXACTLY matches the result from
AR 7 PRo "Save As TIFF" at 600dpi. Furthermore this patch reduces the number of

single pixel differences w.r.t. Adobe output on the few files I have tested.

The problem, of course, is that on the cluster regreesion run (gs 600 dpi
ppmraw) there were 1900 / 4000+ files that differed!

Also since the nightly regressions still have quite a few pending differences,
we probably don't want to do a wholesale update on the baselines.

This patch is posted for review and comment.

Note that the simple method of removing the 'fixed_epsilon' fudge in lines
402 and 407 of base/gxfill.c also eliminated the dopout, but produced even
more differences on the cluster (50% more) and did not match Adobe as closely.
in the areas that changed.
Comment 6 Henry Stiles 2009-05-14 07:12:41 UTC
> This patch is posted for review and comment.

I haven't looked carefully but why isn't the epsilon accounted for in gxfill.c
near line 400?  It looks to me like fill adjust used to be initialized to
fixed_half in gssttate.c, then gx_general_fill_path would give it the
appropriate epsilon adjustment.  Subsequently somebody changed the fill_adjust
initialization to .25 (a separate discussion) and never changed the epsilon
adjustment which was only set if the value was 1/2.  Is that the correct history?

Generally, fill adjust for touched rendering in gs needs some review it would be
nice if "a lot of eyes" looked at this one.
Comment 7 Henry Stiles 2009-05-14 20:05:52 UTC
After Alex's example "sunk in" I was able to create a simpler example:

%!PS-Adobe

% simple test to show 0-width vertical fills do not work.

72 72 moveto 150 150 lineto 72 72 lineto fill % works
72 72 moveto 72 150 lineto 72 72 lineto fill showpage % fails

It fails at screen resolutions also using 72 dpi resolution so the problem can
be studied with integral device space coordinates.


Comment 8 Henry Stiles 2009-05-15 10:51:54 UTC
The following untested patch seems to fix the example in comment #7.  It won't
work with Alex's complex path example, that would require something more
sophisticated for gx_adjust_if_empty().  We should be able to fix this problem
in gx_adjust_if_empty() with few regressions as the code only affects graphics
of 0 extant.  Thoughts, flames?

Index: gxfill.c
===================================================================
--- gxfill.c	(revision 9732)
+++ gxfill.c	(working copy)
@@ -273,11 +273,11 @@
     const fixed
 	  dx = pbox->q.x - pbox->p.x, dy = pbox->q.y - pbox->p.y;
 
-    if (dx < fixed_half && dx > 0 && (dy >= int2fixed(2) || dy < 0)) {
+    if (dx <= fixed_half && dx >= 0 && (dy >= int2fixed(2) || dy <= 0)) {
 	adjust->x = arith_rshift_1(fixed_1 + fixed_epsilon - dx);
 	if_debug1('f', "[f]thin adjust_x=%g\n",
 		  fixed2float(adjust->x));
-    } else if (dy < fixed_half && dy > 0 && (dx >= int2fixed(2) || dx < 0)) {
+    } else if (dy <= fixed_half && dy >= 0 && (dx >= int2fixed(2) || dx <= 0)) {
 	adjust->y = arith_rshift_1(fixed_1 + fixed_epsilon - dy);
 	if_debug1('f', "[f]thin adjust_y=%g\n",
 		  fixed2float(adjust->y));
Comment 9 Ray Johnston 2009-05-20 15:03:35 UTC
The patch in commment #8 still results in dropout on the attachment #1 [details] at 900
dpi (-r900).

The following change cures the droput, but needs to be analyzed for regressions
(in process on peeves).
--- base/gxfill.c       (revision 9743)
+++ base/gxfill.c       (working copy)
@@ -388,26 +388,20 @@
        }
     }
     /*
-     * Compute the proper adjustment values.
-     * To get the effect of the any-part-of-pixel rule,
-     * we may have to tweak them slightly.
      * NOTE: We changed the adjust_right/above value from 0.5+epsilon
      * to 0.5 in release 5.01; even though this does the right thing
      * in every case we could imagine, we aren't confident that it's
      * correct.  (The old values were definitely incorrect, since they
      * caused 1-pixel-wide/high objects to color 2 pixels even if
      * they fell exactly on pixel boundaries.)
+     * The most recent change gets rid of the lower adjust reduction
+     * by 'fixed_epsilon' since the intent of a 'fixed_half' adjust
+     * value is to color any part of a pixel, so the range should be
+     * fixed_1 not (fixed_1 - fixed_epsilon). E.g. 256, not 255.
+     * For a case where this is needed, see bug 690466.
      */
-    if (adjust.x == fixed_half)
-       fo.adjust_left = fixed_half - fixed_epsilon,
-           fo.adjust_right = fixed_half /* + fixed_epsilon */ ;        /* see
above */
-    else
-       fo.adjust_left = fo.adjust_right = adjust.x;
-    if (adjust.y == fixed_half)
-       fo.adjust_below = fixed_half - fixed_epsilon,
-           fo.adjust_above = fixed_half /* + fixed_epsilon */ ;        /* see
above */
-    else
-       fo.adjust_below = fo.adjust_above = adjust.y;
+    fo.adjust_left = fo.adjust_right = adjust.x;
+    fo.adjust_below = fo.adjust_above = adjust.y;
     /* Initialize the active line list. */
     init_line_list(&lst, ppath->memory);
     sbox.p.x = ibox.p.x - adjust.x;
Comment 10 Henry Stiles 2009-05-20 16:04:39 UTC
The patch in comment #8 should not fix either Alex's attachment or the original
customer file at any resolution, it should have no effect. 
gx_fill_adjust_if_empty() only increases adjust for very thin paths - meaning
the bounding box for the entire path is thin.  For alex's attachment and the
customers original file there is zero width filling needed but the bounding box
for the entire path is large so the adjustment in gx_fill_adjust_if_empy() is
never triggered.  

As it stands now we have an obvious inconsistency (with or without your patch),
a simple thin rectangle will be adjusted, but if that same rectangle is part of
a larger path like in Alex's or the customer's file it won't.

I do agree adjust should be fixed_half without fixed_epsilon.  I suspect
gs_fill_adjust_if_empty() should be removed.

Comment 11 Henry Stiles 2009-05-20 22:03:08 UTC
A simple counterexample running the code with the patch in #9, a 1 pixel wide
fill renders 2 pixels wide:

.10 .10 scale 720 720 moveto 720 7200 lineto 721 7200 lineto 721 720 lineto
closepath fill showpage % AR 1 pixel gs 2 pixels

I used gs -sDEVICE=tiffpack -r720 -o foo.tif ...

For acrobat I distilled and saved as a tiff at 720 dpi.

We can't simply change the adjustment.  The widths of interval [0, 1] closed 
render 1 pixel on adobe AR, 0 is a special case.  Width intervals after this are
half open (1, 2] renders 2 pixels (2, 3] renders 3 ... (n, n+1] renders n+1
pixels, where the parenthesis '(' is used to exclude the endpoint and the square
bracket '[' includes the endpoint.  So the adjustment for 0 width must be
different than all other widths or the code has to handle 0 width as a special case.

Note Apple Preview does not render 0 fill widths at all and by my read of the
postscript documentation it is not necessary to do so.  I guess we should check
CPSI as well.

Another blemish I notice in the gs code is the scanline algorithm drops 0 height
horizontal fills while the trapezoid algorithm is dropping 0 width vertical
fills for the customer test case.  

Comment 12 Ray Johnston 2009-06-15 09:24:45 UTC
Changing priority and removing the customer ID since the customer has responded
that the rev 9783 change, compiled with FILL_ZERO_WIDTH solves their issue.

Leaving this bug open until we can decide whether or not to make FILL_ZERO_WIDTH
the default, but this will take a while given the huge number of 1-bit
regression differences that result.

Note that some of the PCL differences I examined look like progressions.
Comment 13 Ray Johnston 2019-10-29 15:06:04 UTC
Created attachment 18395 [details]
D.pdf

Even more simplified file -- a single "D" of the file.
Comment 14 Ray Johnston 2019-10-29 15:07:58 UTC
Assigning to Robin for comment. Note that mupdf draws a blank page and at
some resolutions, Ghostscript draws all the lines as Adobe does.
Comment 15 Peter Cherepanov 2021-01-03 17:22:23 UTC
The bug is still in the current master branch.