Help welcome.

Last time I looked at the code to try to get a handle on this I got lost in trying to figure out how the PDF dictionary works and gave up.  The biggest issue here is understanding the dictionary code and structure so that the rendering intents for each object can be found and used.   So this needs to be looked at by someone who knows how the dictionary works.  Someone like me, who has not worked with the dictionary code at all, is more likely to screw things up than to make them better.

Reading from a dictionary is ready one of the easiest things to do in poppler. Maybe you can start by attaching some sample pdf we do not process correctly? And pointing out which of the fields we need to read?

I will ask on the OpenICC list for some example PDF that use rendering intents.

Jan-Peter Homann recommended the Altona Test Suite for testing rendering intent on the OpenICC mailing list. See the links below.

----------

Handlingg of Rendering Intents (and also using the Output Intent as target profile can be tested with the Altona testsuite.
Look at the orange green rectangels bottom right and read the documentation

Best regards
Jan-Peter

http://www.eci.org/lib/exe/fetch.php?id=en%3Adownloads&cache=cache&media=downloads:altona_test_suite:altona_visual_1v2a_x3.pdf

http://www.eci.org/lib/exe/fetch.php?id=en%3Adownloads&cache=cache&media=downloads:altona_test_suite:altonatestsuite_documentation_eng.pdf

This doesn't really answer my question, you said your problem was "getting to read a value of a dictionary". Which value? Which dictionary?

Sorry, I am not sure exactly how the PDF dictionary is structured or used.  But the section of the spec. that talks about rendering intents is 8.6.5.8 in the PDF 1.7 specification where the relevent part says:

"Rendering intents shall be specified with the ri operator (see 8.4.4, "Graphics State Operators"), the RI entry in a graphics state parameter dictionary (see 8.4.5, "Graphics State Parameter Dictionaries"), or with the Intent entry in image dictionaries (see 8.9.5, "Image Dictionaries"). The value shall be a name identifying the rendering intent. Table 70 lists the standard rendering intents that shall be recognized. Figure L.5 in Annex L illustrates their effects. These intents have been chosen to correspond to those defined by the International Color Consortium (ICC), an industry organization that has developed standards for device-independent colour.  If a conforming reader does not recognize the specified name, it shall use the RelativeColorimetric intent by default."

What is needed is to figure out the correct rendering intent to use when creating transforms based on the dictionary contents.

Setting the intent for the transforms happens in GfxColorSpace *GfxICCBasedColorSpace::copy() where the code looks like this (the below starts at line 1512 in version 0.16.2):


    cmsHPROFILE dhp = displayProfile;
    if (dhp == NULL) dhp = RGBProfile;
    unsigned int cst = getCMSColorSpaceType(cmsGetColorSpace(hp));
    unsigned int dNChannels = getCMSNChannels(cmsGetColorSpace(dhp));
    unsigned int dcst = getCMSColorSpaceType(cmsGetColorSpace(dhp));
    cmsHTRANSFORM transform;
    if ((transform = cmsCreateTransform(hp,
	   COLORSPACE_SH(cst) |CHANNELS_SH(nCompsA) | BYTES_SH(1),
	   dhp,
	   COLORSPACE_SH(dcst) |
	     CHANNELS_SH(dNChannels) | BYTES_SH(1),
	  INTENT_RELATIVE_COLORIMETRIC,0)) == 0) { ...

Where INTENT_RELATIVE_COLORIMETRIC is hard coded.  This instead needs to be based on the rendering intent that should be part of the of the dictionary data for ICCBased objects.  Either this is the document level default rendering intent or it is the object specific rendering intent.

Also the intents listed in Table 70 are the basic ICC intents.  LCMS and some other CMS's also support relative colormetric + black point compensation.  It would be nice if this was also supported by poppler. 

Also the above code makes the assumption that all objects in the PDF are 8 bits/channel.  This may actually be true for most PDFs but what happens if it is passed a PDF with a 16 bit/channel image or perhaps a PDF with a float encoded TIFF that is embedded?  I see in section 8.9.3 of the spec that the dictionary should have the number of bits per colour component so this should not be difficult to fix.  But perhaps this should be in a separate bug report.

Created attachment 43180 [details] [review]
Patch

This patch correctly reads the intent from the pdf, but the altona pdf renders exactly the same way with it than without it, so i'll leave to you guys that understand what color management is why it is not working.

BTW this discussion should be happening in the mailing list and not here.

I applied the patch and added some debugging statements so that I could see what it was doing and it is picking up the RI values from the PDF file and it appears to be passing the correct RI values into the cmsCreateTransform() call.  But I am also seeing that it does not make a difference in the rendered results when I run qt4/demos/poppler_qt4viewer. That is I am seeing differences in places where there should be none or at most only a small difference (IE. #34 - big difference - and #35 - very small difference) and not seeing differences where it should exist in the other tests in the PDF (IE. #36, #37 and #38) where RI should affect the output.

I don't know why this is happening but there are clearly issues with how things are rendered even without the rendering intent issue (#34 for example) and rendering intent is clearly not having an effect on how things are rendered.    Since I am looking at this on a monitor it is possible that the gamut of the output profile (this would be sRGB in this case since the viewer does not know how to get the ICC_PROFILE atom and setup a correct transform) is large enough that for these objects relative colormetric would give the same or nearly the same results as a perceptual transform.  That is if the gamut of the output device is large enough the two transforms may not result in different output.  This is intended for testing with a printer device profile which will have a significantly smaller gamut and this will cause relative colormetric to clip some colors and perceptual to compress the output color to fit in the small space which would make the affect of different RIs more visible.

I will ask others on the OpenICC list to have a look to see if someone can spot what it going on.

ping?

Is there a way, if it is not yet possible to have poppler (I use pdftoppm mainly) use the rendering intent tagged into each object in the PDF, to change the default rendering intent?

I would like it to use Perceptual.

Created attachment 83659 [details] [review]
use icc profile in OutputIntents if possible

You were already quite close to a solution, Albert. There is just one main difference in my patch: it tries to use the icc profile in OutputIntents, if possible, and that solves it.
If You look at the patch and compare it with Yours, You'll see that the most other changes come due to the fact that You change the colorspace caching from gfx to outputdevice in the meantime, but to keep Your patch running we at least need the state to get the current rendering intent.
Another small change is in caching: because the transformation depends now on the current rendering intent, a cached icc based colorspace can not be used if the rendering intent changed in the meantime. But that was just missing in Your patch.
And as I already told on the mailing list: to see differences in the altona pdf (and much easier to see in the GWG 7er series), You have to render in CMYK (because the profile in OutputIntents is a CMYK profile). Therefore You need to compile with the SPLASH_CMYK directive and can't use the -png-format parameter, use -jpegcmyk instead as pdftoppm parameter.

Two open points in my opinion: 
1. Because the display profile now depends on the PDF, we can't use a static variable for it anymore. It will not make any difference if just one apllication uses the poppler library at the same time, and so I would prefer to solve that in a second step (store the display profile in the doc instance and use the static as default).
2. We have now the second case where we definitely need the library compiled with SPLASH_CMYK (the other is the overprint usage). Shouldn't we make SPLASH_CMYK defined by default, and perhaps remove it completely in an additional step? I know that the library become bigger in size, but is this really such important?

What happened to the cropbox code?

(In reply to comment #12)
> You have to render in
> CMYK (because the profile in OutputIntents is a CMYK profile). 

I don't understand that, we have some code around that converts CMYK->RBG, no? Why do i need to render in CMYK?

> Two open points in my opinion: 
> 1. Because the display profile now depends on the PDF, we can't use a static
> variable for it anymore. It will not make any difference if just one
> apllication uses the poppler library at the same time, and so I would prefer
> to solve that in a second step (store the display profile in the doc
> instance and use the static as default).

You mean setDisplayProfile? As I understand that is the profile of your display (i.e. your monitor), and yes I guess it may happen that you are running a single instance of poppler on multiple monitors each of them with different display profiles, but doesn't seem like an urgent need (i mean, yes let's fix it, but it's a bit weird, isn't it?

> 2. We have now the second case where we definitely need the library compiled
> with SPLASH_CMYK (the other is the overprint usage). Shouldn't we make
> SPLASH_CMYK defined by default, and perhaps remove it completely in an
> additional step? I know that the library become bigger in size, but is this
> really such important?

This is ortogonal to this patch/bug, I'd prefer if you raise the discussion on the mainling list, i don't have a huge opinion on it tbh.

(In reply to comment #13)
> What happened to the cropbox code?

Attentive as always, Albert. I made a mistake when moving duplicate code to the new init() method. I'll repair it!

(In reply to comment #14)
> (In reply to comment #12)
> > You have to render in
> > CMYK (because the profile in OutputIntents is a CMYK profile). 
> 
> I don't understand that, we have some code around that converts CMYK->RBG,
> no? Why do i need to render in CMYK?

Because the profile in OutputIntents is a CMYK profile, the transform->doTransform will give as CMYK values. If You render in RGB, GfxICCBasedColorSpace::getRGB will be called, which then will not use the transformation but will call alt->getRGB(), so You will loose the effects. 

But during writing this I got another idea: We can examine the colorspace of the output device (or if YOu want the former display profile) and can use cmsCreateMultiprofileTransform with the 
icc profile of colorspace -> icc profile of output intents -> display profile
to get RGB values. Then we also need no SPLASH_CMYK for this case...

(In reply to comment #15)
> (In reply to comment #13)
> > What happened to the cropbox code?
> 
> Attentive as always, Albert. I made a mistake when moving duplicate code to
> the new init() method. I'll repair it!

After I applied the patch again, I saw that the cropbox code is done in the new init function. The review confused me, too: The code is there twice in both Gfx constructors, but because I moved it to the init function, the review shows the removement in the second constructor :-)

Created attachment 83768 [details] [review]
Use icc profile in OutputIntents

I wasn't able to get GfxICCBasedColorSpace::getRGB running with lcms if the icc profile in OutputIntents is a CMYK connector, neither with cmsCreateMultiprofileTransform() nor with cmsCreateProofingTransform(): I always got wrong RGB colors back. But I'm neither a CMM nor a lcms specialist.
So I give up, but to get it running I now use the transform to get the correct cmyk values and then convert it to RGB with the poppler utils. So with this patch it is no more necessary to compile poppler with SPLASH_CMYK.
And, much more important: You'll see now the effects also in okular (and it should be working also with cairo, but that's untested).

Thomas when runnning master+your patch with 031_ReadMe_Ghent_Output_Patch.pdf i get a crash, also having a look at altona_visual_1v2a_x3.pdf it seems like the first of the bottom right squares is fixed (i.e. all the square is the same color), but the second one is broken (i.e. each half now is of different color).

Can you have a look at these issues?

(In reply to comment #19)
> Thomas when runnning master+your patch with
> 031_ReadMe_Ghent_Output_Patch.pdf i get a crash

I can't reproduce that. And even more strange: the readme file has neither an OutputIntents nor any ICC based colorspace, there are only two image objects with an Intent dictionary entry (which of course does nothing beside set in in GfxState, because the colorspace of the image itself is not an icc based one).
So if You didn't make any mistakes, please provide me the backtrace of the crash!

>, also having a look at
> altona_visual_1v2a_x3.pdf it seems like the first of the bottom right
> squares is fixed (i.e. all the square is the same color), but the second one
> is broken (i.e. each half now is of different color).

Oh, I see. Seems to be something with different rendering intents on the page, but I need more time to find out where I made the mistake!

ok, maybe something was wrong on my side, try to have a go at fixing the crash and once that's sorted we can go again at it and see if i can still reproduce the wrong coloring

(In reply to comment #21)
> ok, maybe something was wrong on my side, try to have a go at fixing the
> crash and once that's sorted we can go again at it and see if i can still
> reproduce the wrong coloring

You still will be able to reproduce the wrong coloring in altona_visual_1v2a_x3.pdf. I digged into it and already encountered two problems:
1. When the patch should completely support rendering intents, we need four versions of XYZ2DisplayTransform, one for each renderíng intent and we must support this in GfxCalGrayColorSpace, GfxCalRGBColorSpace and (which is important for this PDF) GfxLabColorSpace!
2. Since You commited the usage of getRGBLine() today early morning, and the output intent is here a CMYK profile, we need to support this in lineTransform and getRGBLine(), too.
With other words, the patch becomes bigger and bigger, and we probably can commit this only to master.

But I'm still not completely satisfied with my current solution, so please stop any further investigation and wait until I upload a new patch which solves the altona issues completely.

Created attachment 84384 [details] [review]
Use icc profile in OutputIntents

This patch renders now also the squares of the altona file according the documentation.

BTW, during my lot of steps with patches I was also able to reproduce the crash in the ghent readme file. The reason for it was setRenderingIntent and the storage of renderinIntent in the state together with copying and destroying states. To fix this, I decided to store renderingIntent in another way.

I haven't test this patch in cmyk/overprint mode, which would be necessary so that all tests in the altona file run successfully. I'll do that after You commit the CMYK etxnesions of bug 66928, perhaps we then need some additional adjustments in the CMYK / DeviceN area.

Created attachment 84464 [details] [review]
Use icc profile in OutputIntents

Sorry, made some small mistakes in my patch caused crashes under some circumstances, so here a correction of it!

Just to confirm, I should be able to run this with pdftoppm and see the changes on the documents that have different intents, right?

(In reply to comment #25)
> Just to confirm, I should be able to run this with pdftoppm and see the
> changes on the documents that have different intents, right?

Yes, with this patch PDFs that are PDF-X conform are rendered as they should be rendered, regardless if You use "normal" rendering in RGB or use CMYK rendering.
But even if it does the right rendering, I'm not really happy with it, one point is the usage of static variables in a dynamic linked library with values which depends on the PDF which is rendered.
As I see in another comment You plan a longer vacation, so perhaps the better idea is to schedule the investigation of patches to this bug after that, keeping also in mind that the last patch is propbably no more appliable to master.

Yeah i'll be offline until 20Sep, so let's wait until after I come back, deal?

Created attachment 85242 [details] [review]
Use icc profile in OutputIntents

Instead of using static variables I moved now everything to GfxState class variables in case of icc profile usage from OutputIntents. So different PDFs with OutputIntents can now be rendered at the same time. I tested it in both modes, CMYK and RGB, the rendered output is now correct.

Pretty nice side effect: the allocated memory needed for lcms usage is freed at the appropriate places...

Hmmm, your patch makes it so that in the first Gfx constructor we first do

  for (i = 0; i < 6; ++i) {
    baseMatrix[i] = state->getCTM()[i];
  }
  formDepth = 0;
  ocState = gTrue;
  parser = NULL;
  abortCheckCbk = abortCheckCbkA;
  abortCheckCbkData = abortCheckCbkDataA;

  // set crop box
  if (cropBox) {
    state->moveTo(cropBox->x1, cropBox->y1);
    state->lineTo(cropBox->x2, cropBox->y1);
    state->lineTo(cropBox->x2, cropBox->y2);
    state->lineTo(cropBox->x1, cropBox->y2);
    state->closePath();
    state->clip();
    out->clip(state);
    state->clearPath();
  }

as part of init() and then call 

  out->startPage(pageNum, state, xref);
  out->setDefaultCTM(state->getCTM());
  out->updateAll(state);

as part of the constructor, this used to be the other way around. Do we need/want this?

(In reply to comment #29)
> Hmmm, your patch makes it so that in the first Gfx constructor we first do
> 
> :    :    :
> as part of the constructor, this used to be the other way around. Do we
> need/want this?

Hmmh, I missed that, when I tried to remove redundant code. It's probably better to go back to the old constructors and then just call something like initDisplayProfile() at the end of both constructors, where initDisplayProfile do the new stuff. Is it okay for You that I change my patch after You commit bug 68420?

Yep, no worries

Created attachment 86965 [details] [review]
Use icc profile in OutputIntents

And here now a rebased patch with initDisplayProfile() instead of init()

Commited!