Bug 695047

Created attachment 10701 [details]
correct tiger testfile pjl

uploaded the wrong test file.

The wrong one (attachment 10700 [details]) is another example of mode 9 with CMY though.

The parser getting confused issue is filed separately as Bug 695050 .

Don't know if anyone is still interested, but I have created a patch to handle KCMY simple color space 4 plane raster graphics. I had a use case (interpreting output from a legacy platform DeskJet 550C driver) that required it. It should also be noted that Ghostscript's own DeskJet output drivers use this mode too.

Looks like HP added ESC*r-4U with the DJ550C and all subsequent PCL3c DeskJet printers, but not PCL5c for some reason. Four plane KCMY simple color mode is also documented in the HP GL/2 & HP RTL Reference Manual, so DesignJet plotters likely support it as well.

(In reply to Chris Mason from comment #3)
> Don't know if anyone is still interested, but I have created a patch to
> handle KCMY simple color space 4 plane raster graphics.

Please post (attach) - I'd like to see it myself; it is up to the Artifex to decide whether to include it... but posting it would be a first step forward, in any case.

(In reply to Hin-Tak Leung from comment #4)
> Please post (attach) - I'd like to see it myself; it is up to the Artifex to
> decide whether to include it... but posting it would be a first step
> forward, in any case.

Just saw this. I'll post a patch when I'm back at my development system, but that may be awhile. Modification was pretty straight forward though. Modify pccid.c to recognize "-4" as a valid parameter and pcindxed.c to handle the extra plane of data.

Note that GhostPCL's current behavior matches that of real PCL5c printers. They will default to black and white if ESC*r<code>U is set to something the printer doesn't recognize.

*** Bug 705003 has been marked as a duplicate of this bug. ***

(In reply to Chris Mason from comment #5)
> (In reply to Hin-Tak Leung from comment #4)
> > Please post (attach) - I'd like to see it myself; it is up to the Artifex to
> > decide whether to include it... but posting it would be a first step
> > forward, in any case.
> 
> Just saw this. I'll post a patch when I'm back at my development system, but
> that may be awhile. Modification was pretty straight forward though. Modify
> pccid.c to recognize "-4" as a valid parameter and pcindxed.c to handle the
> extra plane of data.
> 
> Note that GhostPCL's current behavior matches that of real PCL5c printers.
> They will default to black and white if ESC*r<code>U is set to something the
> printer doesn't recognize.

I'm not sure if you are still interested in supplying a patch but we had another user ask about this, so we would consider looking at it.  As you said we don't support KCMY because it is not part of PCL5c.  It will not work properly when combined with other PCL5c operators. Raster operation and transparency certainly won't work, they only handle RGB or CMY.  That has made me reluctant to include it in our code, but I'm not seeing a huge downside and other interpreters are supporting it.  We would pay a bounty for the work.

(In reply to Henry Stiles from comment #7)
> I'm not sure if you are still interested in supplying a patch but we had
> another user ask about this, so we would consider looking at it.  As you
> said we don't support KCMY because it is not part of PCL5c.  It will not
> work properly when combined with other PCL5c operators. Raster operation and
> transparency certainly won't work, they only handle RGB or CMY.  That has
> made me reluctant to include it in our code, but I'm not seeing a huge
> downside and other interpreters are supporting it.  We would pay a bounty
> for the work.

I actually forgot about this. I can whip up a patch file, just need to sync up with the latest tree on my dev box. I ran Rich's PCL dump against my patched copy of GhostPCL and the graph colors did output as expected.

Note, I have not extensively regression tested this patch. This may blow up with edge cases!

Created attachment 22232 [details]
Add ESC*r-4U KCMY Simple Color Space Support to GhostPCL

(In reply to Henry Stiles from comment #7)

> 
> I'm not sure if you are still interested in supplying a patch but we had
> another user ask about this, so we would consider looking at it.  As you
> said we don't support KCMY because it is not part of PCL5c.  It will not
> work properly when combined with other PCL5c operators. Raster operation and
> transparency certainly won't work, they only handle RGB or CMY.  That has
> made me reluctant to include it in our code, but I'm not seeing a huge
> downside and other interpreters are supporting it.  We would pay a bounty
> for the work.

If the issue is how it combines with other PCL5c operators, it would make sense to include a switch to enable it.

(In reply to Henry Stiles from comment #7)
> (In reply to Chris Mason from comment #5)
> > (In reply to Hin-Tak Leung from comment #4)
> > > Please post (attach) - I'd like to see it myself; it is up to the Artifex to
> > > decide whether to include it... but posting it would be a first step
> > > forward, in any case.
> > 
> > Just saw this. I'll post a patch when I'm back at my development system, but
> > that may be awhile. Modification was pretty straight forward though. Modify
> > pccid.c to recognize "-4" as a valid parameter and pcindxed.c to handle the
> > extra plane of data.
> > 
> > Note that GhostPCL's current behavior matches that of real PCL5c printers.
> > They will default to black and white if ESC*r<code>U is set to something the
> > printer doesn't recognize.
> 
> I'm not sure if you are still interested in supplying a patch but we had
> another user ask about this, so we would consider looking at it.  As you
> said we don't support KCMY because it is not part of PCL5c.  It will not
> work properly when combined with other PCL5c operators. Raster operation and
> transparency certainly won't work, they only handle RGB or CMY.  That has
> made me reluctant to include it in our code, but I'm not seeing a huge
> downside and other interpreters are supporting it.  We would pay a bounty
> for the work.

Henry - I see that Chris has uploaded the patch to address this issue.  Could it be incorporated into the main code now?

(In reply to Rich M from comment #11)
> (In reply to Henry Stiles from comment #7)
> > (In reply to Chris Mason from comment #5)
> > > (In reply to Hin-Tak Leung from comment #4)
> > > > Please post (attach) - I'd like to see it myself; it is up to the Artifex to
> > > > decide whether to include it... but posting it would be a first step
> > > > forward, in any case.
> > > 
> > > Just saw this. I'll post a patch when I'm back at my development system, but
> > > that may be awhile. Modification was pretty straight forward though. Modify
> > > pccid.c to recognize "-4" as a valid parameter and pcindxed.c to handle the
> > > extra plane of data.
> > > 
> > > Note that GhostPCL's current behavior matches that of real PCL5c printers.
> > > They will default to black and white if ESC*r<code>U is set to something the
> > > printer doesn't recognize.
> > 
> > I'm not sure if you are still interested in supplying a patch but we had
> > another user ask about this, so we would consider looking at it.  As you
> > said we don't support KCMY because it is not part of PCL5c.  It will not
> > work properly when combined with other PCL5c operators. Raster operation and
> > transparency certainly won't work, they only handle RGB or CMY.  That has
> > made me reluctant to include it in our code, but I'm not seeing a huge
> > downside and other interpreters are supporting it.  We would pay a bounty
> > for the work.
> 
> Henry - I see that Chris has uploaded the patch to address this issue. 
> Could it be incorporated into the main code now?

To start that process Chris will need to process the Artifex Contributor License Agreement, here: https://artifex.com/contributor/.  That can be printed, signed, scanned, and emailed back to me.  Use my Bugzilla account name for the email address.

There are 3 commits for this bug:

Improve mode 9 decompression:
5a52b29c386eeb54edcc15bc8b2a08720f85ad5a

A fix for the command scanner:
1a7757ab4bb43799d0e2a1f4714d72bfa945a2e3

KCMY support:
f9769b248df3cbf8081bbee3e1e1656b33432982

The problem in comment #0 about the parsing flaw is not, in fact, the KCMY colour space. It is a fault in the command scanner which is fixed in the second commit.

That then exposed a problem with mode 9 decompression which is partially fixed by the first commit.

With both of these fixes in place we can see (more or less) the tiger in monochrome.

The third commit adds some support for the KCMY colour space. The tiger now renders in colour but not terribly well. I suspect this is because the raster image processing doesn't know how to deal with the 4 colour space.

However, I'm going to close this bug report because the described problem and the actual scanner problem causing the rendering of umpteen pages are both resolved.

There is an existing bug for mode 9 compression which is now bountiable.

If anyone would like to work on the raster data in KCMY then that should be a new bug report. I would try to get that made bountiable if anyone would like to look at it.

Hi Ken, there is a mistake in the merge - clearly this is dead code, as the "if (bits > 2)..." already sets bits to 2. You are supposed to remove the "bits > 2" part. 


@@ -485,6 +491,8 @@ set_default_entries(pcl_cs_indexed_t * pindexed, int start, int num, bool gl2)
 
     if (bits > 2)
         bits = 2;
+    if (bits > 3)
+        bits = 3;
     if (gl2)
         porder = gl2_order[bits];
     /* check for a rgb or colorimetric.  If the colorimetric is being
@@ -495,7 +503,10 @@ set_default_entries(pcl_cs_indexed_t * pindexed, int start, int num, bool gl2)

Created attachment 24346 [details]
minor fix to the committed fix.

ATTN: Ken

There was a mistake in applying the main patch.

(In reply to Hin-Tak Leung from comment #14)
> Hi Ken, there is a mistake in the merge - clearly this is dead code, as the
> "if (bits > 2)..." already sets bits to 2. You are supposed to remove the
> "bits > 2" part. 

Yes, you're quite right. The original patch wouldn't apply for me so I was forced to do it manually and missed that. Surprisingly Coverity static analysis also didn't pick it up.

I'll push a commit with this in later.

(In reply to Ken Sharp from comment #16)
> (In reply to Hin-Tak Leung from comment #14)
> > Hi Ken, there is a mistake in the merge - clearly this is dead code, as the
> > "if (bits > 2)..." already sets bits to 2. You are supposed to remove the
> > "bits > 2" part. 
> 
> Yes, you're quite right. The original patch wouldn't apply for me so I was
> forced to do it manually and missed that. Surprisingly Coverity static
> analysis also didn't pick it up.
> 
> I'll push a commit with this in later.

I fact, I won't. Allowing bits = 3 causes about 1/6 of all our test files to Seg Fault, presumably because we simply don't support bits > 2. The test file 'works' as well with the bits limited to 2 as it does with the limit of 3, so I'm going to stick with the bits > 2 and remove the dead code (bits > 3).

Seems odd that this should cause seg faults, but it clearly does.

But "bits >3..." gives a more correct result. It needs a bit more code else where to fix the segfault, I guess.

The wrong colouring is filed as bug 706748.

(In reply to Hin-Tak Leung from comment #18)
> But "bits >3..." gives a more correct result. It needs a bit more code else
> where to fix the segfault, I guess.
> 
> The wrong colouring is filed as bug 706748.

To follow up on this. The committed changes currently in the tree made to set_dev_specific_default_palette() are what are causing the segfaults when bits > 3. It also broke ESC*r-4U support.

Once I reverted the default palette filling code back to what I originally submitted in the patch, all my test files and your mode 9 compression test tiger in bug 706748 and the sample in this bug decode correctly with correct colors.

I don't know why Ken changed the palette filling code, unless there was a code path elsewhere that was breaking because of it.

(In reply to Chris Mason from comment #19)
...
> I don't know why Ken changed the palette filling code, unless there was a
> code path elsewhere that was breaking because of it.

Actually Ken already explained #17 - at the beginning there was a mistaken merge (which passes the regression test of the existing corpus of test files collected over the last three decades), but correcting the mistake actually causes the 1/6 of the regression test files to fail. i.e. it is as you said, a code path elsewhere would break 1/6 of the existing tests. The mistake actually passes them.

So the mistake is left as is, until and unless the code-path beyond can be fixed to pass.

Hin-Tek,

If you want to test. Make the following changes to pcindxed.c.

in set_dev_specific_default_palette(), remove/comment out

if (num > 8)
        j = 4;

in set_default_entries(), set

if (bits > 3)
        bits = 3;

Your test output should render correctly.

Looks like setting j = 4, which only happens when ESC*r-4U is active is causing an overflow somewhere. It doesn't appear to actually be needed.... at least in my testing.

(In reply to Chris Mason from comment #21)
> If you want to test...

I already did before I posted above - no need to convince me :-).

Being a long-term contributor, I shall try to explain Ken's/Artifex position on their behalf: no submitted patch should fail the regression test. None. So although the initial merge has a fault, it stays as is, and the "fix" cannot be committed, until and unless the regression failure can be adddressed.

So unless you come up with "if (bits > 3)" AND some additional code which adresses the regression failure, there is no point going on about "if bits > 3" being more correct, etc.

Ken points out in our private exchange that I might still have access to the reading the regression logs, in which case (if I still have some of the historical files, etc) I might be able to look at why it fails; but no promises, etc. Meanwhile, if you really want to look at the regression, throwing random valid color PCL files at "if (bits > 3)" and see what further change is needed would be a direction to take. If you want to pursuit it...

I could have made other mistakes, because the automated merge of the patch didn't work for me. Probably the files had diverged in the interim. So I was forced to do the job manually.

The primary change after that, as Hin-Tak indicated was the bits > 3 which causes seg faults. We can't have seg faults, especially not on that many files.

The other change was indeed the default palette code, because the original palette code caused 2 Quality Logic test files to render with an incorrect colour (well, a different colour, but it looked incorrect compared to the rest of the test).

(In reply to Chris Mason from comment #21)

> if (bits > 3)
>         bits = 3;

Ken said the segfault with this is quite common (1 in 6). Apparently the color pcl test file I posted recently on a different matter, in:

https://bugs.ghostscript.com/show_bug.cgi?id=706753

is such a case. If you feel like pursuing this matter, that color pcl test file could be a starting point.

This commit:
https://git.ghostscript.com/?p=ghostpdl.git;a=commit;h=86cc8f7d7d62424994c86d8e9d8b35a292d99158

Fixes the colours, I'd make an error with the fix for the crashes, because I didn't (and to some extent, still don't) completely understand the PCL code.

This commit causes no regression differences, no crashes, and draws the example file here in what I believe to be the correct colours, certainly an improvement.

Created attachment 24376 [details]
Minor fix to remove dead code.

Last commit left in some now unused variables. See patch to fix.

Done :
e9e1318207a0e961397c4f3eabc939e4cd4e8d86

Thanks Ken for continuing on this, and got to a satisfactory conclusion.

It is worth noting your final change (adding a new kcmy colour space define etc) overlaps some of my WIP from ages ago, and with some relatively simple additional code, also make a few of the test files in bug 694997 (which has a different way of setting colour models, and some extended / unusual colour models) somewhat correct - some just looks correct, some at 2x2, some showing only the black layer for a colour input, etc.

Also mentioning on the side, a small correction to our e-mail exchange: the HP people didn't stop adding new graphic instructions around 2014 as I thought they did to the linux driver hpijs/hpcups. Since they added the ability to load opaque binary plug-ins around the same time, for exotic functions in multi-function devices ( eg. scanner + printer + fax...), they put some print modes of newer printers into those closed-source binary plug-ins too. So it is sort of a step backwards. Anyway, if/when we want to support newer printers / newer print modes, again...

(In reply to Hin-Tak Leung from comment #28)
> Also mentioning on the side, a small correction to our e-mail exchange: the
> HP people didn't stop adding new graphic instructions around 2014 as I
> thought they did to the linux driver hpijs/hpcups. Since they added the
> ability to load opaque binary plug-ins around the same time, for exotic
> functions in multi-function devices ( eg. scanner + printer + fax...), they
> put some print modes of newer printers into those closed-source binary
> plug-ins too. So it is sort of a step backwards. Anyway, if/when we want to
> support newer printers / newer print modes, again...

They may still just support PCL3GUI for printing and the extensions are for the other devices to function. Or the printer has a fancy photo printing mode that HP wants to keep secret and/or is patented. I noticed Epson pulls the same thing, their Windows drivers support higher resolution ESC/P-Raster printing vs. the open source Linux ones.

By 2014, most every HP model would have come with driverless printing support and the required PDLs as well. The DeskJet 2700 I have here for testing only supports PCL3GUI, URF/Airprint, and PCLm. I wouldn't expect too much, if any, further evolution in PCL these days.