[gs-code-review] Fw: A patch for bug 687984
Igor V. Melichev
igor.melichev at artifex.com
Mon Jun 9 00:27:00 PDT 2008
----- Original Message -----
From: "Igor V. Melichev" <igor.melichev at artifex.com>
To: <code-review at ghostscript.com>; <tech at artifex.com>
Sent: Sunday, June 08, 2008 10:29 PM
Subject: A patch for bug 687984
> Here is a patch for Bug 687974 "pdf file has banding with ghostscript
> but
> not with acrobat".
>
> I'm unhappy of so heavy thing to appear for a too narrow goal,
> but I don't see a better solution. Suggestions are welcome.
>
> Leo.
>
-------------- next part --------------
[Log message beg]
Fix (stroking) : Prevent unpainted gaps between neighbour strokes that could appear due to stroke adjustment.
DETAILS :
Bug 687974 "pdf file has banding with ghostscript but not with acrobat".
The test case includes a gradient, which is represented
as a set of paralel strokes. Due to stroke widths equals to fractional pixels,
stroke adjustment shifts some of them so that
an unpainted gap appears between strokes and the gradient looks striped.
Note the test document is created with Quark Express 4.11 .
This patch implements a hewristic recognizer for parallel contacting strokes
and suppresses stroke adjustment for them. See comments in code
for more details.
Note that the recognizer data are associated with a device but they
are not a device property. We store them in a device structure because
we haven't got an associated structure in the graphic state,
) d we don't want to introduce one now against extra complexity
with memory management. Also the recognizer code
Appears to be distributed along multiple functions and we would like
to apply a better incapsulation. Nevertheless the encapsulation
would cause CPU time expense for multiple small function calls
unless we create new macros. So commonly we're unhappy of
this code but we don't see a better solution.
Also note that the recognizer data is copied from/to clipper
device when the stroking algorithm installs and deinstalls a clipper.
So the data actually is not a property of a device.
Also they're not a part of graphjic state because they must live
across gsave/grestore (the test case requires so).
EXPECTED DIFFERENCES :
Minor raster difference (a 1 pixel shift of some strokes) with Altona*.pdf at 72 dpi.
[Log message end]
*** F:\SVN-GS\HEAD\gs\src\gdevbit.c Thu May 22 14:08:10 2008
--- files\gs\src\gdevbit.c Sun Jun 8 12:30:25 2008
***************
*** 268,271 ****
--- 268,272 ----
0,
0,
+ {false},
{
gx_default_install,
*** F:\SVN-GS\HEAD\gs\src\gsimage.c Thu May 22 14:08:00 2008
--- files\gs\src\gsimage.c Sun Jun 8 21:05:54 2008
***************
*** 398,401 ****
--- 398,402 ----
const gs_data_image_t * pim, gs_state *pgs)
{
+ pgs->device->sgr.stroke_stored = false;
return gs_image_common_init(penum, pie, pim,
(pgs->in_charpath ? NULL :
*** F:\SVN-GS\HEAD\gs\src\gspaint.c Mon Nov 12 23:00:48 2007
--- files\gs\src\gspaint.c Sun Jun 8 21:03:30 2008
***************
*** 343,346 ****
--- 343,347 ----
gs_fill(gs_state * pgs)
{
+ pgs->device->sgr.stroke_stored = false;
return fill_with_rule(pgs, gx_rule_winding_number);
}
***************
*** 349,352 ****
--- 350,354 ----
gs_eofill(gs_state * pgs)
{
+ pgs->device->sgr.stroke_stored = false;
return fill_with_rule(pgs, gx_rule_even_odd);
}
***************
*** 495,498 ****
--- 497,501 ----
return code;
}
+ pgs->device->sgr.stroke_stored = false;
code = gx_path_assign_free(pgs->path, &spath);
if (code < 0)
*** F:\SVN-GS\HEAD\gs\src\gstext.c Thu May 22 14:08:23 2008
--- files\gs\src\gstext.c Sun Jun 8 21:03:42 2008
***************
*** 264,267 ****
--- 264,268 ----
if (code < 0)
return code;
+ pgs->device->sgr.stroke_stored = false;
return gx_device_text_begin(pgs->device, (gs_imager_state *) pgs,
text, pgs->font, pgs->path, pgs->dev_color,
*** F:\SVN-GS\HEAD\gs\src\gxclip.c Mon Nov 12 23:00:43 2007
--- files\gs\src\gxclip.c Sun Jun 8 13:24:35 2008
***************
*** 117,120 ****
--- 117,121 ----
dev->HWResolution[0] = target->HWResolution[0];
dev->HWResolution[1] = target->HWResolution[1];
+ dev->sgr = target->sgr;
dev->target = target;
(*dev_proc(dev, open_device)) ((gx_device *)dev);
***************
*** 130,133 ****
--- 131,135 ----
dev->HWResolution[0] = target->HWResolution[0];
dev->HWResolution[1] = target->HWResolution[1];
+ dev->sgr = target->sgr;
gx_device_set_target((gx_device_forward *)dev, target);
gx_device_retain((gx_device *)dev, true); /* will free explicitly */
*** F:\SVN-GS\HEAD\gs\src\gxdevcli.h Thu May 22 14:08:24 2008
--- files\gs\src\gxdevcli.h Sun Jun 8 13:30:26 2008
***************
*** 639,642 ****
--- 639,657 ----
dev_page_proc_end_page(gx_default_end_page);
+ /* ----------- A stroked gradient recognizer data ----------*/
+
+ /* This structure is associated with a device for
+ internal needs of the graphics library.
+ The main purpose is to suppress stroke adjustment
+ when painting a gradient as a set of parallel strokes.
+ Such gradients still come from some obsolete 3d party software.
+ See bug 687974,
+ */
+
+ typedef struct gx_stroked_gradient_recognizer_s {
+ bool stroke_stored;
+ gs_fixed_point orig[4], adjusted[4]; /* from, to, width, vector. */
+ } gx_stroked_gradient_recognizer_t;
+
/* ---------------- Device structure ---------------- */
***************
*** 713,716 ****
--- 728,732 ----
long band_offset_x; /* offsets of clist band base to (mem device) buffer */\
long band_offset_y; /* for rendering that is phase sensitive (wtsimdi) */\
+ gx_stroked_gradient_recognizer_t sgr;\
gx_page_device_procs page_procs; /* must be last */\
/* end of std_device_body */\
*** F:\SVN-GS\HEAD\gs\src\gxdevice.h Mon Nov 12 22:58:56 2007
--- files\gs\src\gxdevice.h Sun Jun 8 12:30:54 2008
***************
*** 103,107 ****
0/*PageCount*/, 0/*ShowpageCount*/, 1/*NumCopies*/, 0/*NumCopies_set*/,\
0/*IgnoreNumCopies*/, 0/*UseCIEColor*/, 0/*LockSafetyParams*/,\
! 0/*band_offset_x*/, 0/*band_offset_y*/,\
{ gx_default_install, gx_default_begin_page, gx_default_end_page }
/*
--- 103,107 ----
0/*PageCount*/, 0/*ShowpageCount*/, 1/*NumCopies*/, 0/*NumCopies_set*/,\
0/*IgnoreNumCopies*/, 0/*UseCIEColor*/, 0/*LockSafetyParams*/,\
! 0/*band_offset_x*/, 0/*band_offset_y*/, {false}/* sgr */,\
{ gx_default_install, gx_default_begin_page, gx_default_end_page }
/*
*** F:\SVN-GS\HEAD\gs\src\gxstroke.c Thu May 22 14:08:20 2008
--- files\gs\src\gxstroke.c Sun Jun 8 21:47:11 2008
***************
*** 210,214 ****
/* Other forward declarations */
static bool width_is_thin(pl_ptr);
! static void adjust_stroke(pl_ptr, const gs_imager_state *, bool, bool);
static int line_join_points(const gx_line_params * pgs_lp,
pl_ptr plp, pl_ptr nplp,
--- 210,214 ----
/* Other forward declarations */
static bool width_is_thin(pl_ptr);
! static void adjust_stroke(gx_device *, pl_ptr, const gs_imager_state *, bool, bool);
static int line_join_points(const gx_line_params * pgs_lp,
pl_ptr plp, pl_ptr nplp,
***************
*** 739,743 ****
}
if (!pl.thin) {
! adjust_stroke(&pl, pis, false,
(pseg->prev == 0 || pseg->prev->type == s_start) &&
(pseg->next == 0 || pseg->next->type == s_start) &&
--- 739,745 ----
}
if (!pl.thin) {
! if (index)
! dev->sgr.stroke_stored = false;
! adjust_stroke(dev, &pl, pis, false,
(pseg->prev == 0 || pseg->prev->type == s_start) &&
(pseg->next == 0 || pseg->next->type == s_start) &&
***************
*** 801,804 ****
--- 803,808 ----
}
exit:
+ if (dev == (gx_device *)&cdev)
+ cdev.target->sgr = cdev.sgr;
if (to_path == &stroke_path_body)
gx_path_free(&stroke_path_body, "gx_stroke_path_only error"); /* (only needed if error) */
***************
*** 827,830 ****
--- 831,836 ----
}
}
+ if (vd_enabled)
+ vd_setcolor(pdevc->colors.pure);
code = gx_stroke_path_only_aux(ppath, to_path, pdev, pis, params, pdevc, pcpath);
if (vd_allowed('S'))
***************
*** 944,947 ****
--- 950,959 ----
: STROKE_ADJUSTMENT(thin, pis, y)) << 1;
+ /* fixme :
+ The best value for adjust_longitude is whether
+ the dash is isolated and doesn't cover entire segment.
+ The current data structure can't pass this info.
+ Therefore we restrict adjust_stroke_longitude with 1 pixel length.
+ */
if (length > fixed_1) /* comparefiles/file2.pdf */
return;
***************
*** 976,995 ****
/* Only o.p, e.p, e.cdelta, and width have been set. */
static void
! adjust_stroke(pl_ptr plp, const gs_imager_state * pis, bool thin, bool adjust_longitude)
{
! bool horiz;
! if (!pis->stroke_adjust && plp->width.x != 0 && plp->width.y != 0)
return; /* don't adjust */
horiz = (any_abs(plp->width.x) <= any_abs(plp->width.y));
adjust_stroke_transversal(plp, pis, thin, horiz);
if (adjust_longitude)
adjust_stroke_longitude(plp, pis, thin, horiz);
! /* fixme :
! The best value for adjust_longitude is whether
! the dash is isolated and doesn't cover entire segment.
! The current data structure can't pass this info.
! Therefore we restrict adjust_stroke_longitude with 1 pixel length.
! */
}
--- 988,1087 ----
/* Only o.p, e.p, e.cdelta, and width have been set. */
static void
! adjust_stroke(gx_device *dev, pl_ptr plp, const gs_imager_state * pis, bool thin, bool adjust_longitude)
{
! bool horiz, adjust = true;
! if (!pis->stroke_adjust && (plp->width.x != 0 && plp->width.y != 0)) {
! dev->sgr.stroke_stored = false;
return; /* don't adjust */
+ }
+ /* Recognizing gradients, which some obsolete software
+ represent as a set of parallel strokes.
+ Such strokes must not be adjusted - bug 687974. */
+ if (dev->sgr.stroke_stored && pis->line_params.cap == gs_cap_butt &&
+ dev->sgr.orig[3].x == plp->vector.x && dev->sgr.orig[3].y == plp->vector.y) {
+ /* Parallel. */
+ if ((int64_t)(plp->o.p.x - dev->sgr.orig[0].x) * plp->vector.x ==
+ (int64_t)(plp->o.p.y - dev->sgr.orig[0].y) * plp->vector.y &&
+ (int64_t)(plp->e.p.x - dev->sgr.orig[1].x) * plp->vector.x ==
+ (int64_t)(plp->e.p.y - dev->sgr.orig[1].y) * plp->vector.y) {
+ /* Transversal shift. */
+ if (any_abs(plp->o.p.x - dev->sgr.orig[0].x) <= any_abs(plp->width.x + dev->sgr.orig[2].x) &&
+ any_abs(plp->o.p.y - dev->sgr.orig[0].y) <= any_abs(plp->width.y + dev->sgr.orig[2].y) &&
+ any_abs(plp->e.p.x - dev->sgr.orig[1].x) <= any_abs(plp->width.x + dev->sgr.orig[2].x) &&
+ any_abs(plp->e.p.y - dev->sgr.orig[1].y) <= any_abs(plp->width.y + dev->sgr.orig[2].y)) {
+ /* The strokes were contacting or overlapping. */
+ if (any_abs(plp->o.p.x - dev->sgr.orig[0].x) >= any_abs(plp->width.x + dev->sgr.orig[2].x) / 2 &&
+ any_abs(plp->o.p.y - dev->sgr.orig[0].y) >= any_abs(plp->width.y + dev->sgr.orig[2].y) / 2 &&
+ any_abs(plp->e.p.x - dev->sgr.orig[1].x) >= any_abs(plp->width.x + dev->sgr.orig[2].x) / 2 &&
+ any_abs(plp->e.p.y - dev->sgr.orig[1].y) >= any_abs(plp->width.y + dev->sgr.orig[2].y) / 2) {
+ /* The strokes were not much overlapping. */
+ if (!(any_abs(plp->o.p.x - dev->sgr.adjusted[0].x) <= any_abs(plp->width.x + dev->sgr.adjusted[2].x) &&
+ any_abs(plp->o.p.y - dev->sgr.adjusted[0].y) <= any_abs(plp->width.y + dev->sgr.adjusted[2].y) &&
+ any_abs(plp->e.p.x - dev->sgr.adjusted[1].x) <= any_abs(plp->width.x + dev->sgr.adjusted[2].x) &&
+ any_abs(plp->e.p.y - dev->sgr.adjusted[1].y) <= any_abs(plp->width.y + dev->sgr.adjusted[2].y))) {
+ /* they became not contacting.
+ We should not have adjusted the last stroke. Since if we did,
+ lets change the current one to restore the contact,
+ so that we don't leave gaps when rasterising. See bug 687974.
+ */
+ fixed delta_w_x = (dev->sgr.adjusted[2].x - dev->sgr.orig[2].x);
+ fixed delta_w_y = (dev->sgr.adjusted[2].y - dev->sgr.orig[2].y);
+ fixed shift_o_x = (dev->sgr.adjusted[0].x - dev->sgr.orig[0].x);
+ fixed shift_o_y = (dev->sgr.adjusted[0].y - dev->sgr.orig[0].y);
+ fixed shift_e_x = (dev->sgr.adjusted[1].x - dev->sgr.orig[1].x); /* Must be same, but we prefer clarity. */
+ fixed shift_e_y = (dev->sgr.adjusted[1].y - dev->sgr.orig[1].y);
+
+ if (plp->o.p.x < dev->sgr.orig[0].x ||
+ (plp->o.p.x == dev->sgr.orig[0].x && plp->o.p.y < dev->sgr.orig[0].y)) {
+ /* Left contact, adjust to keep the contact. */
+ if_debug4('O', "[O]don't adjust {{%f,%f},{%f,%f}}\n",
+ fixed2float(plp->o.p.x), fixed2float(plp->o.p.y),
+ fixed2float(plp->e.p.x), fixed2float(plp->e.p.y));
+ plp->width.x += (shift_o_x - delta_w_x) / 2;
+ plp->width.y += (shift_o_y - delta_w_y) / 2;
+ plp->o.p.x += (shift_o_x - delta_w_x) / 2;
+ plp->o.p.y += (shift_o_y - delta_w_y) / 2;
+ plp->e.p.x += (shift_e_x - delta_w_x) / 2;
+ plp->e.p.y += (shift_e_y - delta_w_y) / 2;
+ adjust = false;
+ } else {
+ /* Right contact, adjust to keep the contact. */
+ if_debug4('O', "[O]don't adjust {{%f,%f},{%f,%f}}\n",
+ fixed2float(plp->o.p.x), fixed2float(plp->o.p.y),
+ fixed2float(plp->e.p.x), fixed2float(plp->e.p.y));
+ plp->width.x -= (shift_o_x + delta_w_x) / 2;
+ plp->width.y -= (shift_o_y + delta_w_y) / 2;
+ plp->o.p.x += (shift_o_x + delta_w_x) / 2;
+ plp->o.p.y += (shift_o_y + delta_w_y) / 2;
+ plp->e.p.x += (shift_e_x + delta_w_x) / 2;
+ plp->e.p.y += (shift_e_y + delta_w_y) / 2;
+ adjust = false;
+ }
+ }
+ }
+ }
+ }
+ }
+ if (pis->line_params.cap == gs_cap_butt) {
+ dev->sgr.stroke_stored = true;
+ dev->sgr.orig[0] = plp->o.p;
+ dev->sgr.orig[1] = plp->e.p;
+ dev->sgr.orig[2] = plp->width;
+ dev->sgr.orig[3] = plp->vector;
+ } else
+ dev->sgr.stroke_stored = false;
+ if (adjust) {
horiz = (any_abs(plp->width.x) <= any_abs(plp->width.y));
adjust_stroke_transversal(plp, pis, thin, horiz);
if (adjust_longitude)
adjust_stroke_longitude(plp, pis, thin, horiz);
! }
! if (pis->line_params.cap == gs_cap_butt) {
! dev->sgr.adjusted[0] = plp->o.p;
! dev->sgr.adjusted[1] = plp->e.p;
! dev->sgr.adjusted[2] = plp->width;
! dev->sgr.adjusted[3] = plp->vector;
! }
}
***************
*** 1180,1184 ****
/* because the line was thin. Do it now. */
set_thin_widths(plp);
! adjust_stroke(plp, pis, true, first == 0 && nplp == 0);
compute_caps(plp);
}
--- 1272,1276 ----
/* because the line was thin. Do it now. */
set_thin_widths(plp);
! adjust_stroke(dev, plp, pis, true, first == 0 && nplp == 0);
compute_caps(plp);
}
***************
*** 1252,1256 ****
/* because the line was thin. Do it now. */
set_thin_widths(plp);
! adjust_stroke(plp, pis, true, first == 0 && nplp == 0);
compute_caps(plp);
}
--- 1344,1348 ----
/* because the line was thin. Do it now. */
set_thin_widths(plp);
! adjust_stroke(dev, plp, pis, true, first == 0 && nplp == 0);
compute_caps(plp);
}
***************
*** 1348,1352 ****
/* because the line was thin. Do it now. */
set_thin_widths(plp);
! adjust_stroke(plp, pis, true, adlust_longitude);
compute_caps(plp);
}
--- 1440,1444 ----
/* because the line was thin. Do it now. */
set_thin_widths(plp);
! adjust_stroke(dev, plp, pis, true, adlust_longitude);
compute_caps(plp);
}
***************
*** 1381,1385 ****
int code;
! vd_setcolor(0);
vd_setlinewidth(0);
if (moveto_first) {
--- 1473,1477 ----
int code;
! /* vd_setcolor(0); */
vd_setlinewidth(0);
if (moveto_first) {
***************
*** 1467,1471 ****
np = &np1;
}
! if_debug1('O', "[o]use %s\n", (ccw ? "co (ccw)" : "ce (cw)"));
/* Handle triangular joins now. */
--- 1559,1563 ----
np = &np1;
}
! if_debug1('O', "[O]use %s\n", (ccw ? "co (ccw)" : "ce (cw)"));
/* Handle triangular joins now. */
More information about the gs-code-review
mailing list