Bug 68986

I found the reason for the regression in test "C" (patch "C" in altona notation): The transparency group with knockout has two subgroups with blend mode multiply, the first one draws "TEXT TO", the second one draws "KNOCK OUT". The blend mode multiply says that we have to multiply the current color with the backdrop colour, and my implementation of knockout in bug 12185 multiplies in case of "KNOCK OUT" the blue current color with the darkened blue of "TEXT TO" where it overlaps, but the enclosing knockout transparency group forces that the "TEXT TO" has to be removed first where it overlaps and the current color has to be multiplied with the former pink background color again.

After days of working and thinking about it I find now a solution, it was really complicate to solve it.

BTW, also the cairo backend has big problems with it: it ignores the blend mode completely and in the overlapping region the resulting color becomes white. But I myself can only give additional hints, I'm not able to solve it in the cairo backend.

The patch I'll upload in a few minutes will unfortunately only solve the knockout problem, all the other tests in the altona PDF still fails :-(

Created attachment 85949 [details] [review]
knockout transparency group with subgroups which use blend modes

The patch consists of two parts:
1. The changes in SplashOutputDev::beginTransparencyGroup and Splash::pipeRun accomplishes that the correct backdrop color will be used for multiplying.
2. The changes in SplashOutputDev::paintTransparencyGroup and the extension of Splash::blitTransparent is optimization: because of antialiasing effects when drawing "knock out" the contour of "knock out" becomes visible where it overlaps "text to". The opimization removes the antialiasing effects in the overlapping area.

I've regtested the patch and a few tests (only 10) fails. After looking at the failed tests I could classify them into three groups:
1. I can't see the differences in the "diff" png, because the differences are too small.
2. I see some small color changes. I couldn't submit that the new color or the old color is the right one, but I'm given to the new color.
3. I used the openjpeg integration for my regtests and in some of my tests I got damaged rendering outputs when creating the reference png. This always happens in area of JPEG2000 compressed images, and these kind of failed tests have differences in exact that area. When I configured with --disable-libopenjpeg and use my patch, the output seems to be okay, but I haven't recreated the references without the usage the openjpeg integration and rerun the regression test again.

Argg, the thing is crashing here inside openjpeg 1.3 :'-(

I'll guess i'll run the regular regression test and then compile poppler without openjpeg and compare before and after just for this file.

Thomas, makes sense to you?

(In reply to comment #3)
> Argg, the thing is crashing here inside openjpeg 1.3 :'-(
> 
> I'll guess i'll run the regular regression test and then compile poppler
> without openjpeg and compare before and after just for this file.
> 
> Thomas, makes sense to you?

No problem, seems to be okay for me!

Hmmm, with this patch on master, the only changes i can find are in "Q" in the spiral, and the colors look worse with the patch than without the patch. Can you try? Maybe you uploaded the wrong patch?¿

Created attachment 87015 [details]
Changes in test "C"

(In reply to comment #5)
> Hmmm, with this patch on master, the only changes i can find are in "Q" in
> the spiral, and the colors look worse with the patch than without the patch.
> Can you try? Maybe you uploaded the wrong patch?¿

Oh, I missed the side effects in test "Q", but I don't think that these changes come due to "knockout" problems. I think the reason for it are blend mode problems and I hopefully can solve them with a patch for test "G", too. But if not, I plan to look at the tests step by step. And even if the colors changes a little bit for test "Q", the rendering result is not perfect either with or without the patch.

But I'm wondering why You missed the changes in "C". I attach the result with the patch, s. the second line.
I create this output with
git checkout -f
git pull
git apply 68986.0001.patch
./configure --disable-libopenjpeg

But if You don't want to commit the patches step by step, it's up to You. But because of the complexity of the splash pipe run implementation all futur patches will cause probably changes in that area, and it will be harder for me to check the differences of the several patches. But it's of course doable.

Created attachment 87224 [details]
Output of knockout patch w/o optimization

Before I start to have a look at test "G", I looked last weekend again at my knockout patch and why it impacts on test "Q". I recognized that the reason for it is the optimization I pointed to in comment 2 at point 2, and that this optimization will not work with all blend modes.
Therefore I removed the optimization once again and got the attached result for patch "C". Have a look at the antialiasing effects where "knockout" overlaps "text to" in the second line.

Created attachment 87225 [details]
Output of knockout patch if switch of antialiasing in case of knockout

So I searched for another solution, and the easiest one is to switch off antialising in case of a knockout transparency group should really knockout former paintings.
I can live with this, and before continueing with test "G" I would like to regtest the changed patch.
@Albert: What kind of output would You prefer? Output of comment 7 or this one?

Created attachment 87236 [details] [review]
knockout transparency group w/o antialiasing

This patch produces output of attachment 87225 [details] (It would be quite easy to remove the statements which switch off antialiasing)

Created attachment 87237 [details] [review]
knockout transparency group w/o font antialiasing

Sorry, I indeed regtested the patch of comment 9, BUT we run again into the problem of comment 14 in bug 12185 (which PDF is unfortunately still not in my PDF suite :-( ). So here a small correction for it!

A few additional remarks to test "D": 
if pdftoppm is just called with -png, and so splash works internally with RGB, the result for line 3 of this test (the gray shown squares) is incredible, even with the solution for bug 34053: there is nearly no difference / contrast in the gray values.
The reason for this is in my opinion that the background color gray is produced DeviceCMYK 0.32/0.32/0.32/0.32.
If we compile poppler with #define SPLASH_CMYK 1 and then call pdftoppm with -png -overprint (only reason for the usage of the overprint option is to force splask working in CMYK mode) the rendering result is much better.
(in my eyes an additional reason to remove the define and always compile the SPLASH_CMYK branch!)
Therefore I will not search for a solution!

Created attachment 87453 [details] [review]
correction for knockout transparency groups

Working on a patch for test "G" I encountered, that there is still a problem with knockout transparency groups namely if they are combined with the isolated option. So I solved that first, and so here the new corrected patch.
Because of the changes I made a new regression test against actual git master, and the only relevant (visible) changes I encountered are on this PDF.
For a better recognition of the changes I used another version of this altona PDF: eci_altona-test-suite-v2_technical2_one-patch-per-page_x4.pdf (available on the same URL). The changes are here on page 3, 7 and 14. 3 and 7 are okay because that's what my patch is for, and the changes on page 14 are in the center of the image, but the look and feel is still well enough in my eyes and the output is quite near to the reference image.

Aaaaaaaaaand I'm back after a long time, sorry. So ran the regtest and yes, found out that only the B rendering in the "G patch" changes.

Is taht out should it be, want me to commit? Once commited i should *not* close this bug since there's still fixes needed, right?

Same for me Albert: after uploading the last patch for this PDF I haven't really more time to look at the other regressions. So bug is in abeyance for me in the moment.
But yes, commit and leave this bug open. I think that patch solves the knockout feature nearly completely.

pushed

Created attachment 97471 [details]
output of test "P"

After a really long time I yet found to continue working on this bug. As You probably remember, the openjpeg community created a bugfix for our LAB image problem and announced it to be in version 2.1. Therefore I checked out this openjpeg beta release, compiled it and applies my patch based on Adrian's one of bug 58906.
Even if it renders now the LAB image, there are still big differences between the rendered output and the reference image, s attachment. Therefore I looked into the technical parameters of test "P' (s. http://www.eci.org/_media/de/eci_altona-test-suite-v2_technical2_documentation_eng-4.pdf), and I wondered that they speak nearly about avery image of a image with ICC profile, but the PDF contains nearly always only images with DeviceXXXX colorspace, but and that's funny, the dictionaries have a Intent member.  
Therefore I looked into the PDF specification again and found the following interesting statement in section 8.6.5.6:

When a device colour space is selected, the ColorSpace subdictionary of the current resource dictionary (see 7.8.3, "Resource Dictionaries") is checked for the presence of an entry designating a corresponding default colour space (DefaultGray, DefaultRGB, or DefaultCMYK, corresponding to DeviceGray, DeviceRGB, or DeviceCMYK, respectively). If such an entry is present, its value shall be used as the colour space for the operation currently being performed.

I'm quite sure that I already stumbled about this clue long time ago and fixed that, but I encountered that either this fix is lost or it was not complete. So I implemented this in a first try in Gfx::doImage() and, surprising, the regression was gone :-)

Created attachment 97472 [details] [review]
use Default colorspaces if present instead of Device colorspaces

Since the comment in section 8.6.5.6 of the PDF spec doesn't apply only to images, I implemented a solution for it now in GfxColorSpace::parse(), even if this means a lot more changes. But it completely solves the usage of Default colorspaces in any situation.
I regtested this patch and found only a few changes, always when a Default colorspace exists.

Created attachment 97905 [details]
wrong output for patch "N"

The complete wrong output for patch "N" if compiled with SPLASH_CMYK and using the overprint parameter for pdftoppm is caused by the fact that in this case DeviceN is used and the blend functions completely erase background with spot colors.
BTW, if You run pdftoppm without the overprint parameter, You'll get a better output, BUT: the output is shown in  http://www.eci.org/_media/de/eci_altona-test-suite-v2_technical2_documentation_eng-4.pdf as a sample for incorrect output. The reason for the incorrect output is that the spot color orange is first converted to its RGB components and then the blend function is applied. This is definitely wrong on separable blend modes. The PDF spec says:

When performing blending operations in subtractive colour spaces (DeviceCMYK, Separation, and DeviceN), the colour component values shall be complemented (subtracted from 1.0) before the blend function is applied and the results of the function shall then be complemented back before being used.

Created attachment 97906 [details] [review]
solves blend mode problem in CYMK and DeviceN for separable blend modes

This patch fixes the incorrect output for patch "N" if using the overprint parameter, and if using the overprint parameter we get the correct output as expected in the eci documentation.

BTW, it also solves nearly every regression in patch "S" to "Z" and patch "G", if the overprint parameter is used because then the PDF is rendered in DeviceN bitmap and the blend modes now are applied correctly!

Created attachment 97907 [details]
pending regression in patch "G"

The pending regression in patch "G" is probably caused by the usage of an blending colorspace. If a blending colorspace is given, the blend functions have to be applied in this colorspace. Poppler completely ignores this parameter.

I'll try to fix that after You commit the patches I already uploaded. Where as I see the other regressions as a bug in poppler, this would be an enhancement in my eyes, therefore it can wait.

Created attachment 98479 [details] [review]
solves blend mode problem in CYMK and DeviceN for separable blend modes

Examining patches "S" to "W" I encountered that my patch of comment 19 need special handling for the blending modes difference and exclusion in the spot color planes.
This patch therefore has two small changes and replaces the patch of comment 19, and it solves patches "S" to "W" now, too.
The only things missing now for a complete solution for the test PDF from eci is the handling of blending colorspaces in general (s. comment 20) and blending colorspaces in isolated groups (s. output for patches "X" and "Z")

And i got here 5 months late \o/

No seriously i'm awful.

From what i read from the log your suggestion is:
 * Configure without openjpeg
 * Try patches one by one

Is that right?

Can you give me the order of the patches?

(In reply to Albert Astals Cid from comment #22)
> And i got here 5 months late \o/
> 
> No seriously i'm awful.

No problem :-)

> 
> From what i read from the log your suggestion is:
>  * Configure without openjpeg
>  * Try patches one by one
> 
> Is that right?

Yes.

> 
> Can you give me the order of the patches?

1. comment 17
2. comment 21

But the order doesn't really matter, they repair two issues which are independent from each other.

Tried patch from comment 17. It doesn't compile the cairo side, i added a first parameter but not sure that is the correct fix.

Also it crashes in https://bugs.kde.org/attachment.cgi?id=75198 i guess you need to properly pass the recursion parameter somewhere since from a quick look at the gdb stack seems it's getting into infinite recursion somwhere.

Can you please fix both issues?

Created attachment 107654 [details] [review]
use Default colorspaces if present instead of Device colorspaces

Quite easy to solve :-), here the repaired patch.

Sorry for any inconvenience

When using this patch with one file the black turns to gray, i've rendered with Adobe, mupdf, gs, firefox, chromium and they all agree it should look black. The file is sadly 18M so i will have to send it to you by mail instead of attach. Tell me what you think.

The output You see with my patch is correct in some way, I try to explain:

1. The said PDF consists of one image with colorspace DeviceCMYK
2. The PDF specifies that the DefaultCMYK is an icc based colorspace
3. The PDF has no OutputIntents, therefore poppler took the sRGB profile to convert the CMYK image to sRGB
4. But the produced PNG has neither a profile nor it has the marker that it is sRBG, it just tells it is RGB

So it is simply a problem of color transformation. I send You in private a display.icc profile which converts the image to RGB without any further information, and in addition a profile which is quite similar to the one poppler uses.

Created attachment 107733 [details] [review]
use Default colorspaces if present instead of Device colorspaces / experimentell

I cant't stop thinking about it, and after encountered as I told in comment 27 it's more or less a problem with the profile and/or lcms I digged a little bit more and searched the internet. I found a discussion of black point compensation, and figured out that lcms2 has a special option for it. I set it in this patch, and great, the output is black again.

But I'm not sure wether really set it globally everytime, under wether circumstances or make it an option. See i.e. gimp, https://mail.gnome.org/archives/commits-list/2013-December/msg01573.html. They made it an option.

I haven't regtested my patch, I just proofed that it restores the desired output for the said PDF and want to show You that. Therefore I also titled this patch as experimentell. At least You should see that my implementation of the usage of default colorspaces should be correct and the problems with the said PDF have nothing to do with it.

What's the difference between both patches, just cmsFLAGS_BLACKPOINTCOMPENSATION ?

(In reply to Albert Astals Cid from comment #29)
> What's the difference between both patches, just
> cmsFLAGS_BLACKPOINTCOMPENSATION ?

Yes. Sorry, should have tell that. And it has nothing to do with the usage of default colorspaces, it just fixes that lcms transforms the said pdf more gray than black.

Created attachment 107841 [details]
This file is totally white when it had rendering before

This file is totally white when it had rendering before. Any idea why?

Created attachment 107866 [details] [review]
use Default colorspaces if present instead of Device colorspaces

Hmmh, Albert: The DefaultGray is an ICC based colorspace, but the mask colorspace of the image (DeviceGray) should be alway of mode csDeviceGray. Since masking is done by 8 bit, I omit now to look for DefaultGray in this masks.

So this patch solves the problem of comment 31. I removed cmsFLAGS_BLACKPOINTCOMPENSATION again, if You want to have it please insert it manually again.

Ok, patch from comment 32 commited, let's go for patch from comment 21 next

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/poppler/poppler/issues/613.