Bug 90795

yes, please make it use #if everywhere

Created attachment 116904 [details] [review]
updated patch for --enable-cmyk, includes cmake

I updated the patch.
* All tests now use "#if SPLASH_CMYK". Of about 150 tests, a small number used #ifdef or #if defined() instead.  I changed those few tests so that all tests are now consistent.
* I added cmake support. You can now enable CMYK support with "cmake -DSPLASH_CMYK=ON ."
William

Is there any reason to make it off default?

I.e. will stuff be slower if we compile in cmyk?

Or shall we rather make it on by default and let people off it if the want?

Created attachment 116937 [details]
file that needs poppler with SPLASH_CMYK for pdftops -level1sep to work

I left SPLASH_CMYK disabled by default because it has always been disabled by default.

I think that it should always be enabled, and it shouldn't be an option to enable or disable.

pdftops needs SPLASH_CMYK to work correctly, and building poppler with SPLASH_CMYK has almost no performance penalty.

The best argument for leaving it an option is that Derek Noonburg made it an option in xpdf, and he knew what he was doing, so maybe he had a good reason to disable it.

For more details:
"pdftops -level1sep" requires SPLASH_CMYK to generate correct output for files that it needs to rasterize.  I have attached an example file.

SPLASH_CMYK mainly enables "case" entries and "if" statements for splashModeCMYK8 and splashModeDeviceN8 in Splash.cc and SplashOutputDev.cc except for a few places like SplashOutputDev::univariateShadedFill() where it adds the line
  pattern->getShading()->getColorSpace()->createMapping(bitmap->getSeparationList(), SPOT_NCOMPS);

I suspect that the main cost of enabling SPLASH_CMYK is a small increase in code size and rather than any change in performance.

Fedora 20 and RHEL 7 (and I suspect all Fedora/RHEL/CentOS distributions) build poppler without SPLASH_CMYK, which is now the main reason why I still have to build pdftops from source.

Thomas, can you comment on that line in univariateShadedFill? maybe it should be also guarded by  "if" statements for splashModeCMYK8 and splashModeDeviceN8 ?

Here are more places that use SPLASH_CMYK:

poppler/PSOutputDev.cc
  if (level == psLevel1Sep || level == psLevel2Sep ||
      level == psLevel3Sep || globalParams->getOverprintPreview())
  it sets up numComps = 4 and new SplashOutputDev(splashModeCMYK8)
  When SPLASH_CMYK is not set, it sets up numComps = 3 and splashModeRGB8 instead which breaks pdftops.cc

splash/SplashBitmap.cc SplashBitmap::getRGBLine() and SplashBitmap::getXBGRLine() add a check for separationList->getLength() > 0

poppler/SplashOutputDev.cc convertGfxColor() adds an initialization for color[3] = 0.

poppler/SplashOutputDev.cc SplashOutputDev::drawMaskedImage() and SplashOutputDev::drawSoftMaskedImage() and SplashOutputDev::univariateShadedFill() call
  colorMap->getColorSpace()->createMapping(bitmap->getSeparationList(), SPOT_NCOMPS);

poppler/SplashOutputDev.cc SplashOutputDev::beginTransparencyGroup() has
    } else if (blendingColorSpace->getMode() == csDeviceCMYK ||
               (blendingColorSpace->getMode() == csICCBased &&
                blendingColorSpace->getNComps() == 4)) {
      colorMode = splashModeCMYK8;
  This might be another way that poppler can work incorrectly if SPLASH_CMYK is disabled.

splash/SplashState.cc SplashState::SplashState() (several different versions) initializes 
  for (i = 0; i < 256; ++i) {
    cmykTransferC[i] = (Guchar)i;
    cmykTransferM[i] = (Guchar)i;
    cmykTransferY[i] = (Guchar)i;
    cmykTransferK[i] = (Guchar)i;
    for (int cp = 0; cp < SPOT_NCOMPS+4; cp++)
      deviceNTransfer[cp][i] = (Guchar)i;
  This might be an example of a place that adds a very small performance penalty when SPLASH_CMYK is enabled.

splash/SplashState.cc SplashState::setTransfer() does more copying when SPLASH_STATE is enabled, similar to SplashState()

splash/SplashState.h is several times larger
  Guchar cmykTransferC[256],
         cmykTransferM[256],
         cmykTransferY[256],
         cmykTransferK[256];
  Guchar deviceNTransfer[SPOT_NCOMPS+4][256];

splash/SplashTypes.h splashClearColor(), splashColorCopy() and splashColorXor() copy more fields
  dest[3] = src[3];
  for (int i = SPOT_NCOMPS; i < SPOT_NCOMPS + 4; i++)
    dest[i] = src[i];

Ok, then i feel we'll have to go with it disabled by default for the moment. I'll put this on my review/todo queue.

(In reply to Albert Astals Cid from comment #6)
> Thomas, can you comment on that line in univariateShadedFill? maybe it
> should be also guarded by  "if" statements for splashModeCMYK8 and
> splashModeDeviceN8 ?

It's either possible to guard it or just to remove the #if SPLASH_CMYK completely in this case.
All these places, also the one William pointed to, are mostly introduced by me to implement the overprint functionality. Overprint is just defined and therefore can only be simulated in separated colorspaces like CMYK and DeviceN (s. (8.6.7 of the PDF spec)

I was thinking of something similar -- make a new OutputDev flag like enableSplashCMYK to protect all of the SPLASH_CMYK code that is not obviously protected inside a "case" or "if" for splashModeCMYK8 or splashModeDeviceN8, although gcc might complain about possibly uninitialized variables.

poppler/PSOutputDev.cc could set the flag when
  (level == psLevel1Sep || level == psLevel2Sep ||
   level == psLevel3Sep || globalParams->getOverprintPreview())

My main goal is to have pdftops -level1sep run as fast as possible because I periodically have to push batches of tens to thousands of PDFs through it, and I expect that I will always need to build poppler specially for pdftops with options like -DSPLASH_CMYK=1 -Ofast -DNDEBUG --enable-static --disable-shared that would not be acceptable for someone trying to build libpoppler.so. That is why I proposed making a configure option for a release build. https://bugs.freedesktop.org/show_bug.cgi?id=90796

I would be happy enough to see a configure option for SPLASH_CMYK that explains why you would want to enable it.  I think that most distributions build pdftops SPLASH_CMYK disabled, and someone who does not know the poppler internals would have a hard time connecting invalid output from pdftops with the currently undocumented SPLASH_CMYK flag that they must manually add to CXXFLAGS.

pdftops is not the only affected utility. pdftoppm currently has a bug when SPLASH_CMYK is enabled. https://bugs.freedesktop.org/show_bug.cgi?id=90570

Ok, so settled, let's ship it disabled by default until you guys can come up with patches that enabling it doesn't break "the common" usage.

At this point is there any patch that does this? Is it "updated patch for --enable-cmyk, includes cmake" ?

Yes, my updated patch does that.
It creates the --enable-cmyk option and leaves it off by default in both configure and cmake.
This matches xpdf-3.03 which has a --enable-cmyk configure option that is off by default.
The patch also normalizes all of the SPLASH_CMYK tests to #if. A few tests had #ifdef so compiling with -DSPLASH_CMYK=0 would generate bad code.

pushed