Use the Alternate entry, if it exists, in ICCBased Colour Space dictionaries (issue 5836, issue 5939, issue 6055) #6112

Snuffleupagus · 2015-06-13T19:24:35Z

Fixes #5836.
Fixes #5939.
Fixes #6055.

The relevant part of the specification: http://www.adobe.com/content/dam/Adobe/en/devnet/acrobat/pdfs/PDF32000_2008.pdf#G7.3804856.

timvandermeij · 2015-06-13T21:33:33Z

Nice work! I couldn't quite figure out what was wrong, but I'm glad you did! I hope my comment in #5836 didn't distract you too much, as it seemed like that was only caused by the issue, but not the issue itself.

One question though: the spec says, for the Alternate entry, that "non-conforming readers may use this colour space" (specifically here: https://www.adobe.com/content/dam/Adobe/en/devnet/acrobat/pdfs/PDF32000_2008.pdf#page=158&zoom=auto,-274,757). It sounds like we should check if the color space is unsupported and then fall back to the alternate color space, whereas in your patch we directly use the alternate color space without further checking. Personally I think that is fine, as it fixes the issues and the alternate entry is specified on purpose, but do you think we need to do more checking, if even possible?

Ninja edit: mupdf seems to do the same as your patch (http://mupdf.com/docs/browse/source/pdf/pdf-colorspace.c.html) and, slightly more obscured, Poppler does it too (https://github.com/danigm/poppler/blob/0011805e22193b690b99a53dcb9986ce04eb3eb4/poppler/GfxState.cc#L1412), so we should be fine.

Snuffleupagus · 2015-06-13T21:59:02Z

Good questions! I don't have very convincing answers, but this is my take on it:

[...], whereas in your patch we directly use the alternate color space without further checking.

I also thought about that when trying to make sense of the specification, but I don't know what that should look like while also fixing the referenced issues.
Without actual test-cases it seems difficult to be sure how this should be implemented, since just trying to adhere to the specification has shown to be a bit of a problem in practice :-)

[...], but do you think we need to do more checking, if even possible?

Ideally we probably should, but again without actual test-cases (i.e. PDF files found in the wild), that seems difficult to me.
Given that we currently have three example files that this patch fixes, and unfortunately quite poor test-coverage for this part of the code-base, I think I'd rather risk breaking some other PDF files with this change. At least that way, we might actually get test-cases to work with :-P

timvandermeij · 2015-06-13T22:01:26Z

Agreed, and since mupdf and poppler also work exactly this way (see the ninja edit in my previous comment), this should be fine. I'll review and test this once the Windows bot is alive again.

Snuffleupagus · 2015-06-13T22:09:14Z

Actually, based both on the specification and the implementation in mupdf, we should perhaps try to check that the number of components match? I'll check to see if that is simple to implement.

Snuffleupagus · 2015-06-13T22:43:17Z

I've just uploaded a new version, which checks that the components match and (indirectly) that it's not a Pattern colour space. Unfortunately this requires, as far as I can see, that we actually parse the CS which is somewhat unfortunate. However, in the referenced PDF files, this doesn't seem to affect the rendering times in any noticeable way.

/botio-linux preview

Snuffleupagus · 2015-06-13T22:59:53Z

One question though: the spec says, for the Alternate entry, that "non-conforming readers may use this colour space" [...]

Actually, the complete quote is:

(Optional) An alternate colour space that shall be used in case the one specified in the stream data is not supported. Non-conforming readers may use this colour space. [...]

Since we currently completely ignore the contents of the stream data, I think that just using the /Alternate CS, if it exists, is exactly the right thing to do here!
Having re-read the paragraph a few times, I think that we're now doing what the specification says here.

Snuffleupagus · 2015-06-14T10:09:45Z

src/core/colorspace.js

+          if (alt) {
+            var altIR = ColorSpace.parseToIR(alt, xref, res);
+            // Parse the /Alternate CS to ensure that the number of components
+            // are correct, and that it is not a PatternCS.


I'm going to push a new version of the patch, which slightly improves this comment. No functional code changes are made, though!

…onaries (issue 5836, issue 5939, issue 6055) Fixes 5836. Fixes 5939. Fixes 6055.

timvandermeij · 2015-06-14T13:38:03Z

/botio-linux preview

pdfjsbot · 2015-06-14T13:38:03Z

From: Bot.io (Linux)

Received

Command cmd_preview from @timvandermeij received. Current queue size: 0

Live output at: http://107.21.233.14:8877/25e87eaf6cda590/output.txt

pdfjsbot · 2015-06-14T13:38:45Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/25e87eaf6cda590/output.txt

Total script time: 0.70 mins

Published

timvandermeij · 2015-06-14T13:43:14Z

/botio test

pdfjsbot · 2015-06-14T13:43:15Z

From: Bot.io (Linux)

Received

Command cmd_test from @timvandermeij received. Current queue size: 0

Live output at: http://107.21.233.14:8877/af607a7388c3b63/output.txt

yurydelendik · 2015-06-14T13:55:46Z

/botio-windows test

pdfjsbot · 2015-06-14T13:55:47Z

From: Bot.io (Windows)

Received

Command cmd_test from @yurydelendik received. Current queue size: 0

Live output at: http://107.22.172.223:8877/d3f364d1be8a7fb/output.txt

pdfjsbot · 2015-06-14T14:01:14Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/af607a7388c3b63/output.txt

Total script time: 17.99 mins

Font tests: Passed
Unit tests: Passed
Regression tests: Passed

pdfjsbot · 2015-06-14T14:13:38Z

From: Bot.io (Windows)

Success

Full output at http://107.22.172.223:8877/d3f364d1be8a7fb/output.txt

Total script time: 17.85 mins

Font tests: Passed
Unit tests: Passed
Regression tests: Passed

timvandermeij · 2015-06-14T17:00:54Z

/botio makeref

pdfjsbot · 2015-06-14T17:00:54Z

From: Bot.io (Linux)

Received

Command cmd_makeref from @timvandermeij received. Current queue size: 0

Live output at: http://107.21.233.14:8877/6f830d1fc1fd4ba/output.txt

pdfjsbot · 2015-06-14T17:00:54Z

From: Bot.io (Windows)

Received

Command cmd_makeref from @timvandermeij received. Current queue size: 0

Live output at: http://107.22.172.223:8877/1dd94994592d4c5/output.txt

pdfjsbot · 2015-06-14T17:18:38Z

From: Bot.io (Windows)

Success

Full output at http://107.22.172.223:8877/1dd94994592d4c5/output.txt

Total script time: 17.73 mins

Lint: Passed
Make references: Passed
Check references: Passed

pdfjsbot · 2015-06-14T17:18:48Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/6f830d1fc1fd4ba/output.txt

Total script time: 17.90 mins

Lint: Passed
Make references: Passed
Check references: Passed

Use the Alternate entry, if it exists, in ICCBased Colour Space dictionaries (issue 5836, issue 5939, issue 6055)

timvandermeij · 2015-06-14T17:23:41Z

Thank you!

Snuffleupagus · 2015-06-14T22:33:50Z

@timvandermeij Thank you too!
Your questions prompted me to go back and really read the specification; so you helped to significantly improve the quality of the final patch :-)

Snuffleupagus added core image-quality labels Jun 13, 2015

timvandermeij self-assigned this Jun 13, 2015

Snuffleupagus reviewed Jun 14, 2015
View reviewed changes

Use the Alternate entry, if it exists, in ICCBased Colour Space dicti…

bc5e43b

…onaries (issue 5836, issue 5939, issue 6055) Fixes 5836. Fixes 5939. Fixes 6055.

Snuffleupagus modified the milestone: 2015 Q2 Jun 14, 2015

timvandermeij added a commit that referenced this pull request Jun 14, 2015

Merge pull request #6112 from Snuffleupagus/iccbased-alternate

c8fd9c8

Use the Alternate entry, if it exists, in ICCBased Colour Space dictionaries (issue 5836, issue 5939, issue 6055)

timvandermeij merged commit c8fd9c8 into mozilla:master Jun 14, 2015

Snuffleupagus deleted the iccbased-alternate branch June 14, 2015 21:16

Snuffleupagus mentioned this pull request Jun 16, 2015

getAll is not a function Lab ColorSpace #6124

Closed

Snuffleupagus mentioned this pull request Dec 20, 2017

pdf completely rendered blank #9285

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the Alternate entry, if it exists, in ICCBased Colour Space dictionaries (issue 5836, issue 5939, issue 6055) #6112

Use the Alternate entry, if it exists, in ICCBased Colour Space dictionaries (issue 5836, issue 5939, issue 6055) #6112

Snuffleupagus commented Jun 13, 2015

timvandermeij commented Jun 13, 2015

Snuffleupagus commented Jun 13, 2015

timvandermeij commented Jun 13, 2015

Snuffleupagus commented Jun 13, 2015

Snuffleupagus commented Jun 13, 2015

Snuffleupagus commented Jun 13, 2015

Snuffleupagus Jun 14, 2015

timvandermeij commented Jun 14, 2015

pdfjsbot commented Jun 14, 2015

pdfjsbot commented Jun 14, 2015

timvandermeij commented Jun 14, 2015

pdfjsbot commented Jun 14, 2015

yurydelendik commented Jun 14, 2015

pdfjsbot commented Jun 14, 2015

pdfjsbot commented Jun 14, 2015

pdfjsbot commented Jun 14, 2015

timvandermeij commented Jun 14, 2015

pdfjsbot commented Jun 14, 2015

pdfjsbot commented Jun 14, 2015

pdfjsbot commented Jun 14, 2015

pdfjsbot commented Jun 14, 2015

timvandermeij commented Jun 14, 2015

Snuffleupagus commented Jun 14, 2015

Use the Alternate entry, if it exists, in ICCBased Colour Space dictionaries (issue 5836, issue 5939, issue 6055) #6112

Use the Alternate entry, if it exists, in ICCBased Colour Space dictionaries (issue 5836, issue 5939, issue 6055) #6112

Conversation

Snuffleupagus commented Jun 13, 2015

timvandermeij commented Jun 13, 2015

Snuffleupagus commented Jun 13, 2015

timvandermeij commented Jun 13, 2015

Snuffleupagus commented Jun 13, 2015

Snuffleupagus commented Jun 13, 2015

Snuffleupagus commented Jun 13, 2015

Snuffleupagus Jun 14, 2015

Choose a reason for hiding this comment

timvandermeij commented Jun 14, 2015

pdfjsbot commented Jun 14, 2015

From: Bot.io (Linux)

Received

pdfjsbot commented Jun 14, 2015

From: Bot.io (Linux)

Success

Published

timvandermeij commented Jun 14, 2015

pdfjsbot commented Jun 14, 2015

From: Bot.io (Linux)

Received

yurydelendik commented Jun 14, 2015

pdfjsbot commented Jun 14, 2015

From: Bot.io (Windows)

Received

pdfjsbot commented Jun 14, 2015

From: Bot.io (Linux)

Success

pdfjsbot commented Jun 14, 2015

From: Bot.io (Windows)

Success

timvandermeij commented Jun 14, 2015

pdfjsbot commented Jun 14, 2015

From: Bot.io (Linux)

Received

pdfjsbot commented Jun 14, 2015

From: Bot.io (Windows)

Received

pdfjsbot commented Jun 14, 2015

From: Bot.io (Windows)

Success

pdfjsbot commented Jun 14, 2015

From: Bot.io (Linux)

Success

timvandermeij commented Jun 14, 2015

Snuffleupagus commented Jun 14, 2015