Bug 46600

Summary: [patch] pdftops git 25Feb12 restore deleted binary code and level2sep fixed
Product: poppler Reporter: William Bader <williambader>
Component: utilsAssignee: poppler-bugs <poppler-bugs>
Status: RESOLVED FIXED QA Contact:
Severity: normal    
Priority: medium CC: Thomas.Freitag, williambader
Version: unspecified   
Hardware: x86 (IA32)   
OS: All   
Whiteboard:
i915 platform: i915 features:
Attachments: patch against poppler cloned from git on 25Feb12
patch against poppler cloned from git on 25Feb12

Description William Bader 2012-02-24 21:24:05 UTC
Created attachment 57618 [details] [review]
patch against poppler cloned from git on 25Feb12

This patch restores a set of my old patches that are present in the 0.18.4 release tarball but dropped in git.
I reworked the level2sep and level3sep code to combine better with recent changes to checkPageSlice().
The patch restores these changes:
restore the implementation of -binary
restore the fix that level2sep and level3sep must write cmyk instead of rgb
restore the conversion of bitmaps with all gray to mono8
I ran it through valgrind and fixed two places that didn't free memory:
fix unfreed lookup[] table in GfxState
fix duplicated fontIDs allocation in PSOutputDev
I also fixed the CMYK misspelling in Stream.
William
Comment 1 William Bader 2012-02-24 21:47:18 UTC
Created attachment 57620 [details] [review]
patch against poppler cloned from git on 25Feb12

The same patches, but I restored unnecessary white space and line ordering changes to make the patches smaller.
Comment 2 Thomas Freitag 2012-02-25 01:46:26 UTC
As far as I can see the detection, if a cmyk image is contains only gray values, is wrong: if cyan, magenta and yellow have the same value it's not true that we get a pure gray value, they must be 0! And if we do it in that way, we'll get problems with overprint. Therefore it should be:

if (numComps == 4) {
  int compCyan;
  isGray = gTrue;
  while ((compCyan = str0->getChar()) != EOF) {
    if (compCyan != 0 && str0->getChar() != 0 ||
        str0->getChar() != 0) {
      isGray = gFalse;
      break;
    }
    str0->getChar();
  }
: : :

But probably we never will run in a situation where a CMYK image will contain only values where C = M = Y
Comment 3 Albert Astals Cid 2012-02-25 07:08:34 UTC
William, there's still a few unneeded changes like

-GBool CMKYGrayEncoder::fillBuf() {
+GBool CMYKGrayEncoder::fillBuf() {

change you try to remove them?
Comment 4 William Bader 2012-02-25 09:20:04 UTC
Thomas,

>As far as I can see the detection, if a cmyk image is contains only gray
values, is wrong

It is necessary for my application.
I sometimes get PDFs that contain embedded images which are meant to be gray but for some reason (I suspect being copied from an RGB logo meant for the web), pdftops -level1sep generates gray with C=M=Y=gray level, K=0 instead of C=M=Y=0, K=gray level.  When I separate it for printing on a press, it makes C, M and Y plates instead of a single K plate.  Often no one notices until almost the print deadline, and then they have to rush to rebuild the PDF and push it back through the application to generate a new page.
My CMYK to gray change solves that problem.  It is necessary for my application and probably useful for anyone else who will need to do color separations on output from pdftops.
If you think that it is wrong for overprinting, then I can add a command line option to enable it, or I can disable it when -overprinting is specified.
Do you have a PDF with overprinting that doesn't work with my change?
I think that printing gray images a K instead of as a mix of CMY should improve the overprinting situation because adjacent C, M or Y can be overprinted onto K if necessary, and printing K as a combination of CMY creates registration issues, especially with small text, and ink coverage levels at dark shades of gray.

>But probably we never will run in a situation where a CMYK image will contain
only values where C = M = Y

That is one of the reasons why I think that the change is relatively safe.  The image probably comes from a gray or black web image like a gif for a company logo that was created in RGB and then embedded into a PDF.  The image really should be black or gray.

William
Comment 5 William Bader 2012-02-25 09:22:22 UTC
Albert,

>there's still a few unneeded changes like
>-GBool CMKYGrayEncoder::fillBuf() {
>+GBool CMYKGrayEncoder::fillBuf() {

If it were only a comment, I would agree that it doesn't matter, but as a function name, it also describes the order of the color components. The misspelled name might tempt someone to reorganize an image buffer to reverse the Y and K components.

William
Comment 6 Albert Astals Cid 2012-02-26 03:51:22 UTC
Oh, i did not see the swtich between the letters :D

Thomas what do you think? You are infact vetoing to a code that was already there before your code so if you don't have a strong reason i'd like to commit it
Comment 7 Thomas Freitag 2012-02-26 07:43:24 UTC
(In reply to comment #6)
> Oh, i did not see the swtich between the letters :D
> 
> Thomas what do you think? You are infact vetoing to a code that was already
> there before your code so if you don't have a strong reason i'd like to commit
> it

Are You right? Was that code really in? Then commit it!
On the other hand: If we covert a CMYk image to a grayscale image only if c = m = k we loose saturation and luminance, and who can decide if it is just because the PDF creator makes a mistake?
When You say that the code was in already You propably talk about the other bug, didn't You?

Thomas
Comment 8 Thomas Freitag 2012-02-26 08:52:57 UTC
(In reply to comment #4)
> If you think that it is wrong for overprinting, then I can add a command line
> option to enable it, or I can disable it when -overprinting is specified.
I can live with it at least to disable it in case of overprinting, but I would prefer an additional option to enable it because of my comment 7.

Thomas
Comment 9 William Bader 2012-02-26 10:05:09 UTC
Thomas,

>I can live with it at least to disable it in case of overprinting, but I would
prefer an additional option to enable it

If that would make the patches acceptable, I could do it in a day or two.
Is -makeblack a suitable name for the option?

>If we covert a CMYk image to a grayscale image only if c = m = k we loose saturation and luminance,

It happens only if every dot in the image has c = m = y.

I agree that it would be an issue if it happened on a dot-by-dot basis, which is why RIPs usually have configurable settings for black generation.

Also, for printing, ink saturation above 200% often makes a brownish mess rather than gray, and large areas with high saturation on a web press can weaken the paper enough to cause web breaks.  It looks fine on the screen, but it isn't printable.

For my purposes, images with c = m = y in every dot mean a black image that was saved in an rgb format and not embedded correctly in the PDF.

In any case, I have no problem making it a configurable parameter on the pdftops command line.

William
Comment 10 Albert Astals Cid 2012-02-26 13:23:08 UTC
Well, if you agree it is a "wrong conversion" and it's just a workaround for broken files i'd prefer it to use that separate command line switch you mention.
Comment 11 William Bader 2012-02-26 14:04:30 UTC
OK, I'll add a command line option.
I consider it a user error because people using a cmyk workflow for printing on a press shouldn't send in images that look gray or black on the screen but draw with c=m=y > 0 and k=0.
It ends up being my problem because the users consider it my error when a page looks fine on the screen but doesn't print right.
William
Comment 12 Albert Astals Cid 2012-02-26 14:12:41 UTC
Excuse my total knowledge of printing, colors and all this stuff, so why if c=m=y > 0 and k=0 looks gray on a screen would not look gray when printed?
Comment 13 William Bader 2012-02-26 15:32:17 UTC
> why if c=m=y > 0 and k=0 looks gray on a screen would not look gray when printed?

In theory, it should look gray, but in practice, the mixture of dots of the different inks doesn't absorb colors the same way that real black ink does.
Also, you have issues with registration.  The web must pass through a cylinder for each color, and it is hard to keep everything synchronized to the size of a dot (newspapers typically print at 1024 dpi), so the c, m and y dots are never in perfect alignment.  Small text in cmy instead of k can be hard to read due misalignments.
http://en.wikipedia.org/wiki/CMYK
http://en.wikipedia.org/wiki/Gamut

William
Comment 14 Thomas Freitag 2012-02-27 05:57:30 UTC
I apologize for any inconvenience my argueing may cause: I just discussed it with my collegue who works in PostScript and PDF at the very early beginning and has much more practical experiences with it, where I saw more the theoretical aspects. He means that it would be okay to convert CMYK in either case to grayscale if C = M = Y.
So even if You're not amused and have already done some unnecessary work: I can follow the arguments of my collegue and William and would now prefer to have it without any additional option. I promise, next time I'll discuss it with my collegue first.

Thomas
Comment 15 William Bader 2012-02-27 06:43:30 UTC
Thomas,
Do you still want me to make any changes, or are the current patches OK?
I didn't start anything yet.
William
Comment 16 Thomas Freitag 2012-02-27 06:51:18 UTC
(In reply to comment #15)
> Thomas,
> Do you still want me to make any changes, or are the current patches OK?
> I didn't start anything yet.
> William

No, from my changed point of view the current patches are ok. Happy to hear that You haven't start anything yet. For my excuse: I discussed it with my collegue because I got doubts after reading Your comments.

Thomas
Comment 17 James Cloos 2012-02-27 12:18:15 UTC
> Excuse my total knowledge of printing, colors and all this stuff, so why if
> c=m=y > 0 and k=0 looks gray on a screen would not look gray when printed?

The reality of mixing ink.

Some home and office printers have ink or toner which does manage to
look grey (or black) when mixed like that.

Notably, though, the ink used by offset presses does not.

The typical comparison is that c=m=y looks like mud on such a press.

(And for the same reason that earth or mud look brown, in fact.)
Comment 18 Albert Astals Cid 2012-02-27 14:15:19 UTC
William I can not commit your patch as it does lots of different stuff that the old patch did not do.

Can you explain why you need the Splash.cc change?
Comment 19 William Bader 2012-02-27 17:27:28 UTC
The Splash.cc change makes some of my files run faster because most of the images in my files have the special case where pipeRun() is just a copy, and the change extracts the tests to figure that out outside the loop so it is done once per row instead of once per bit in simpler cases.

If you want, you can throw that part away.  It doesn't interfere with anything else.  Do you need me to make a new patch without that part?

William
Comment 20 Albert Astals Cid 2012-02-29 14:19:35 UTC
I've pushed everything except the Splash.cc change. If you want that in please open a new bug report and attach a new patch and some benchmarks to support your case :-)

Thanks and sorry for the mess on losing this features :-/
Comment 21 William Bader 2012-02-29 17:30:24 UTC
>I've pushed everything except the Splash.cc change.

Thanks!

>If you want that in please open a new bug report and attach a new patch and some benchmarks to support your case :-)

How much of a difference do you need?  I think that the difference was around 2% percent, which probably isn't worth it.

William

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.