Bug 690585

Assign to Henry (per his request) at least until Marcos returns from vacation.

If the visual discrepancies hasn't gone away with my recent changes in the
pxlcolor device, this could be re-assigned to me. (although it looks like it is
pcl6 driving pxlcolor rather than ghostscript).

I ran at -r72 quickly through the 184 files in tests_private/pcl/pcl5cfts/ -
these 46 files shows significant differences (plus some showing small
width/positioning differences). 43 of those were in Ray's list and the 3
additions are fts.0070 fts.0080 fts.0800. Out of the 46 files, about 14 of them
are similiar to '690949 - colorred pattern fill appeared with portion painted
with black' . Suggest revisit this after fixing 690949. 

-----------
fts.0060
fts.0070
fts.0080
fts.0110
fts.0210
fts.0220
fts.0230
fts.0740
fts.0750
fts.0760
fts.0800
fts.0825
fts.0832
fts.0861.new
fts.0861
fts.0862
fts.0865
fts.0866
fts.0880
fts.0884
fts.0891
fts.0954
fts.0970
fts.0980
fts.0982
fts.0990
fts.1020
fts.1051
fts.1055
fts.1060
fts.1100
fts.1130
fts.1420
fts.1740
fts.1762
fts.1816
fts.1821
fts.1832
fts.1944
fts.2105
fts.2114
fts.2210a
fts.2320
fts.2330
fts.2350
fts.2354

Fixed 690949; there are still 39 cases. (12 'compare: images too dissimilar' and
27 smaller visible differences. The 12 are:

fts.0825
fts.0832
fts.0861
fts.0861.new
fts.0866
fts.0980
fts.1420
fts.1740
fts.1762
fts.1821
fts.2210a
fts.2350

The 27 are:
fts.0060
fts.0070
fts.0080
fts.0110
fts.0210
fts.0220
fts.0230
fts.0740
fts.0750
fts.0760
fts.0800
fts.0862
fts.0865
fts.0880
fts.0884
fts.0891
fts.0970
fts.0982
fts.0990
fts.1051
fts.1055
fts.1130
fts.2105
fts.2114
fts.2320
fts.2330
fts.2354

fts.1821 fts.1832 fts.2210a shows IllegalAttributeDataType SetLineJoin error so
persumably it will work when that's fixed.

fts.0602 (outside of the 39) - Failed to interpret TT instructions for glyph
index 53.

The three invalid line joins are all trying to set value to 4. (PXL line joins
only goes up to 3). After reviewing what other driver do - most of them either
set default and/or output a warning, this is what I'd commit the next time:

diff --git a/base/gdevpx.c b/base/gdevpx.c
index c6f48dc..b23b9ed 100644
--- a/base/gdevpx.c
+++ b/base/gdevpx.c
@@ -1053,6 +1053,11 @@ pclxl_setlinejoin(gx_device_vector * vdev, gs_line_join join)
 {
     stream *s = gdev_vector_stream(vdev);
 
+    /* better not to set an invalid value */
+    if ((join < 0) || (join > 3)) {
+        eprintf1("Igoring invalid linejoin enumerator %d\n", join);
+        return 0;
+    }
     /* The PCL XL join styles just happen to be identical to PostScript. */
     px_put_ub(s, (byte) join);
     px_put_ac(s, pxaLineJoinStyle, pxtSetLineJoin);

Hin-Tak were you going to look at this more?  I've made it bountiable.

(In reply to comment #6)
> Hin-Tak were you going to look at this more?  I've made it bountiable.

Not to any extent - probably will come back to it from time to time. The diff in comment 5 was committed as r10400 . comment 4 was probably tested after r10403 but not sure if r10404 was included in the testing; as I remember many of the remaining diff were user pattern-related so 10404 was probably relevant.

A few of the problems were fixed by trunk@10403 (and possibly also trunk@10404).

commit 2c6b4186758461bfd17658f34abdedee4a111c0e
Author: Hin-Tak Leung <hintak_leung@yahoo.co.uk>
Date:   Mon Nov 30 02:41:19 2009 +0000

    set ROP to paint through character glyphs (pattern tiles).
 ... Fixes bug 690949, also some of the problems in bug 690585 (for ghostpdl).

This would have cut the numbers down to a bit lower than 39 cases, except there seems to be some new color-related issue so it stays at 38.

Created attachment 8598 [details]
pclxl_strip_copy_rop() should call gx_default_*

pclxl_strip_copy_rop() should
calling gx_default_strip_copy_rop() instead of returning 0.

This fixes 19 of the problems.

fts.0060
fts.0070
fts.0080
fts.0110
fts.0210
fts.0220
fts.0230
fts.0740
fts.0750
fts.0760
fts.0866
fts.0880
fts.0884
fts.0891
fts.1051
fts.1130
fts.2105
fts.2114
fts.2350

Filed 3 new bug reports for continuations. AFAIK there are only 38 differences, 19 is fixed by comment 9 (and continue for optimization in bug 693056), 2 shows interpreter failure, and 17 with slight gray shifts (bug 693055).

bug 693054  interpreter failure with fts.0980 and fts.0982
bug 693055 	gray shifts with color round-trip pxl->png vs png conversion
bug 693056  optimized version of pclxl_strip_copy_rop() 

The patch in comment 9 should be applied. Pending outcome of 0980/0982 after somebody else fixes the interpreter failure, this bug can be closed.

(In reply to comment #9)
> Created an attachment (id=8598) [details]
> pclxl_strip_copy_rop() should call gx_default_*
> 
> pclxl_strip_copy_rop() should
> calling gx_default_strip_copy_rop() instead of returning 0.

This patch also improves bug 692853. (without this patch, the raster pattern is skipped and output almost empty).

Created attachment 8668 [details]
pclxl_strip_copy_rop() should call gx_default_* when D is not involved.

Calling gx_default_* conditional on !rop3_uses_D(lop) .

gx_default_strip_copy_rop() cannot cope with rop3 involving the destination.

It turns out that out of the 19 files fixed by the previous patch, 17 uses only 0xfc(TSo) and 0xff(1) (and only fts.0884 fts.0891 uses 0xff(1)). The two files go back to being rendered wrongly are fts.2105 and fts.2114 .

I.e. this patch fixes rendering problems with 17 of the files (as well as the two samples in bug 692853, which uses only TSo), without causing interpreter failure in bug 693054.

I suggest committing the patch in comment 12, close this bug report and following up with trying to make the remaining 4 files work in bug 693056.

A small refinement suggestion: a warning should be emitted when dropped through to "return 0" - e.g. "copy_rop not implemented; raster may not render correctly.". Since nobody had noticed the lack of such much, I am not strongly for it though.

(In reply to comment #13)
> I suggest committing the patch in comment 12, close this bug report and
> following up with trying to make the remaining 4 files work in bug 693056.

Done you can also collect the bounty for this problem.

Fixed in:


commit 76aad1b9e4eb2ae30d42b995a3ba8998b8e0820f
Author: Henry Stiles <henry.stiles@artifex.com>
Date:   Wed Jun 13 13:29:35 2012 -0600

   Bug 690585 - Use the defalt strip copy rop routine when the
   destination is not included.

   Thanks to Hin-Tak Leung for this fix.

   CLUSTER UNTESTED

diff --git a/gs/base/gdevpx.c b/gs/base/gdevpx.c
index b8338a3..0d2bd33 100644
--- a/gs/base/gdevpx.c
+++ b/gs/base/gdevpx.c
@@ -1719,9 +1719,18 @@ pclxl_strip_copy_rop(gx_device * dev, const byte * sdata, int sourcex,
                     const gx_color_index * tcolors,
                     int x, int y, int width, int height,
                     int phase_x, int phase_y, gs_logical_operation_t lop)
-{                              /* We can't do general RasterOps yet. */
-/****** WORK IN PROGRESS ******/
-    return 0;
+{
+  /* Improvements possible here using PXL ROP3
+     for some combinations of args; use gx_default for now */
+  if (!rop3_uses_D(lop)) /* gx_default() cannot cope with D ops */
+    return gx_default_strip_copy_rop(dev, sdata, sourcex,
+                                     sraster, id,
+                                     scolors,
+                                     textures,
+                                     tcolors,
+                                     x, y, width, height,
+                                     phase_x, phase_y, lop);
+  return 0;
 }

 /* ------ High-level images ------ */