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.

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

William, there's still a few unneeded changes like

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

change you try to remove them?

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

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

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

(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

(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

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

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.

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

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?

> 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

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

Thomas,
Do you still want me to make any changes, or are the current patches OK?
I didn't start anything yet.
William

(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

> 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.)

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?

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

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 :-/

>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