implement CalRGB color space #5165

kkujala · 2014-08-10T18:27:30Z

Both whitespace and blackspace support are implemented.

yurydelendik · 2014-08-10T18:58:36Z

Could you also provide test PDF document(s) ?

kkujala · 2014-08-10T19:15:30Z

Of course :) It was already included but I forgot to add the test.manifest changes. I've update the patch with that now.

And if one compares pdf.js and acrobat then for the most part they are almost the same. Only on few pages acrobat goes to disco.

kkujala · 2014-08-10T20:20:52Z

I'll update the patch a bit more, so don't merge just yet.
EDIT: ready for review now, I modified the test pdf to include info section.

kkujala · 2014-08-10T21:58:38Z

Addressed the review nits from @timvandermeij. Thanks for the review!

timvandermeij · 2014-08-11T10:00:53Z

/botio-linux preview

pdfjsbot · 2014-08-11T10:00:54Z

From: Bot.io (Linux)

Received

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

Live output at: http://107.21.233.14:8877/8a76ae3d32067fe/output.txt

pdfjsbot · 2014-08-11T10:01:40Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/8a76ae3d32067fe/output.txt

Total script time: 0.76 mins

Published

Viewer: http://107.21.233.14:8877/8a76ae3d32067fe/web/viewer.html
B2G Viewer: http://107.21.233.14:8877/8a76ae3d32067fe/extensions/b2g/content/web/viewer.html
Extension: http://107.21.233.14:8877/8a76ae3d32067fe/extensions/firefox/pdf.js.xpi
Extension (AMO): http://107.21.233.14:8877/8a76ae3d32067fe/extensions/firefox/pdf.js.amo.xpi

timvandermeij · 2014-08-11T10:04:28Z

/botio test

pdfjsbot · 2014-08-11T10:04:30Z

From: Bot.io (Linux)

Received

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

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

pdfjsbot · 2014-08-11T10:04:30Z

From: Bot.io (Windows)

Received

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

Live output at: http://107.22.172.223:8877/13ac6169e22edee/output.txt

pdfjsbot · 2014-08-11T10:24:16Z

From: Bot.io (Windows)

Failed

Full output at http://107.22.172.223:8877/13ac6169e22edee/output.txt

Total script time: 19.77 mins

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

Image differences available at: http://107.22.172.223:8877/13ac6169e22edee/reftest-analyzer.html#web=eq.log

pdfjsbot · 2014-08-11T10:27:02Z

From: Bot.io (Linux)

Failed

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

Total script time: 22.55 mins

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

Image differences available at: http://107.21.233.14:8877/aa1c1de090e4bd9/reftest-analyzer.html#web=eq.log

yurydelendik · 2014-08-11T12:52:29Z

test/test_manifest.json

+    {  "id": "calrgb",
+       "file": "pdfs/calrgb.pdf",
+       "md5": "29e6f9dc529523cbed41401eb1b4165e",
+       "rounds": 1,


Let's specify lastPage=8 -- 36 pages is too many

yurydelendik · 2014-08-11T13:00:40Z

Looks good. However it will be nice to see less objects created in the function, i.e. move const matrix definitions outsize, and accept destination matrix as parameter vs returning new one: function fn(a, b, result) { result[0] = a[0] + b[0]; ... } vs function fn(a, b) { return [a[0] * b[0]]; }. You can create temporary matrices in a closure and assign them to the meaningfully-named variables in the functions before call (that will also allow to re-use the same temp for different calls if needed)

CodingFabian · 2014-08-11T13:28:41Z

whats causing the failure for 1597? all other pdf viewers render the white as ffffff not as fefeff. Even pdfjs before the patch did. is that rounding issue? is there an alpha layer above it?

kkujala · 2014-08-11T20:45:51Z

@yurydelendik, I created those temp matrices as you suggested. On the positive side I was able to nuke three functions and on the negative side few places became a little less readable but only a little. Overall I think it is better.

Btw, do you want me to hoist the const matrices outside of the CalRGBCS class? There would be then even less object creation but they would be global then and I don't know if it makes any difference performance wise.

What would be the best way to test the gains from this temp matrices optimization? Speed and memory wise?

@CodingFabian, I'll check what's going on there.

And I'll squash the commits if Yury thinks my changes are ok.

yurydelendik · 2014-08-11T21:47:23Z

negative side few places became a little less readable but only a little. Overall I think it is better.
It is good. Let me add some improvements. It does not matter how you will name the temporary matrices, e.g. tempMatrix1, tempMatrix2, etc. will work -- it will be useful to reuse those, e.g.

 normalizeWhitePointToFlat([cs.XW, cs.YW, cs.ZW], [X, Y, Z],
 tempXYZ_FlatMatrix);
 var XYZ_Flat = tempXYZ_FlatMatrix;

 compensateBlackPoint([cs.XB, cs.YB, cs.ZB], XYZ_Flat, tempXYZ_BlackMatrix);
 var XYZ_Black = tempXYZ_BlackMatrix;

 normalizeWhitePointToD65(FLAT_WHITEPOINT_MATRIX, XYZ_Black,
 tempXYZ_D65Matrix);
 var XYZ_D65 = tempXYZ_D65Matrix;

will look as:

 var  XYZ_Flat = tempMatrix1;
 normalizeWhitePointToFlat([cs.XW, cs.YW, cs.ZW], [X, Y, Z], XYZ_Flat);

 var XYZ_Black = tempMatrix2;
 compensateBlackPoint([cs.XB, cs.YB, cs.ZB], XYZ_Flat, XYZ_Black);

 var XYZ_D65 = tempMatrix1;
 normalizeWhitePointToD65(FLAT_WHITEPOINT_MATRIX, XYZ_Black, XYZ_D65);

or even:

 var  XYZ_Flat = tempMatrix1, XYZ = tempMatrix2;
 XYZ[0] = X; XYZ[1] = Y; XYZ[2] = Z;
 normalizeWhitePointToFlat(cs.XYZ_White, XYZ, XYZ_Flat);

What would be the best way to test the gains from this temp matrices optimization? Speed and memory wise?

I'm afraid we cannot effectively measure speed/memory on any of the PDF. However, you can modify getRgb to run convertToRgb hundreds thousand times and clock the execution time.

Using Float32Array/Float32Array instead of JS Array might speed up the execution as well.

yurydelendik · 2014-08-11T21:48:07Z

src/core/colorspace.js

+    var LMS_Flat = tempLMS_FlatMatrix;
+
+    matrixProduct(BRADFORD_SCALE_INVERSE_MATRIX, LMS_Flat,
+                  tempBradfordInverseMatrix);


result instead of tempBradfordInverseMatrix can be used here

kkujala · 2014-08-11T22:19:09Z

Yes, it looks better like that. I'll make the changes. Did you have an opinion about the location of the const matrices? I.e is it better to have them inside the class?

yurydelendik · 2014-08-11T22:47:45Z

Did you have an opinion about the location of the const matrices? I.e is it better to have them inside the class?

Yes, inside of class closure is fine (as you have them now)

kkujala · 2014-08-12T20:18:21Z

I've made the temp matrix changes. I'll still check that one regression failure and I'll also modify the test pdf so that 8 pages long.

yurydelendik · 2014-08-12T21:17:43Z

Really nice.

Not sure, but making the arrays typed might give you some perf improvements, e.g. var BRADFORD_SCALE_MATRIX = [0.8951, ..]; => var BRADFORD_SCALE_MATRIX = new Float32Array([0.8951, ...]); and var temp = [0,0,0]; => var temp = new Float32Array(3);;

kkujala · 2014-08-14T20:42:48Z

@yurydelendik, I changed all the matrices to typed arrays and I updated the test pdf to be more compact. It currently has 17 pages and when I ran the browsertest it took 10 seconds. I can add the last page = 8 to the test manifest if you think it is still needed.

@CodingFabian, It was as you suspected a rounding error. The value for red was 254,999.., which was then truncated to 254. I modified the code to use Math.round and now it is 255.

yurydelendik · 2014-08-14T20:47:23Z

I can add the last page = 8 to the test manifest if you think it is still needed.

Our reftest hardness stores images of every page, we are trying to not burden it with unneeded memory usage.

Both whitespace and blackspace support are implemented.

kkujala · 2014-08-14T20:52:17Z

Sure, I preemptively moved all the interesting pages to the front and more esoteric pages to the end, so it's cool with me. Patch updated.

yurydelendik · 2014-08-15T13:50:03Z

Awesome. This looks good.

/botio test

pdfjsbot · 2014-08-15T13:50:06Z

From: Bot.io (Windows)

Received

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

Live output at: http://107.22.172.223:8877/7f30cc69e0ffcd8/output.txt

pdfjsbot · 2014-08-15T13:50:06Z

From: Bot.io (Linux)

Received

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

Live output at: http://107.21.233.14:8877/62302241cef33d8/output.txt

pdfjsbot · 2014-08-15T14:09:49Z

From: Bot.io (Windows)

Failed

Full output at http://107.22.172.223:8877/7f30cc69e0ffcd8/output.txt

Total script time: 19.73 mins

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

Image differences available at: http://107.22.172.223:8877/7f30cc69e0ffcd8/reftest-analyzer.html#web=eq.log

pdfjsbot · 2014-08-15T14:12:41Z

From: Bot.io (Linux)

Failed

Full output at http://107.21.233.14:8877/62302241cef33d8/output.txt

Total script time: 22.59 mins

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

Image differences available at: http://107.21.233.14:8877/62302241cef33d8/reftest-analyzer.html#web=eq.log

yurydelendik · 2014-08-15T14:42:44Z

/botio makeref

pdfjsbot · 2014-08-15T14:42:45Z

From: Bot.io (Windows)

Received

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

Live output at: http://107.22.172.223:8877/59644205210b29a/output.txt

pdfjsbot · 2014-08-15T14:42:45Z

From: Bot.io (Linux)

Received

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

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

pdfjsbot · 2014-08-15T15:04:03Z

From: Bot.io (Windows)

Success

Full output at http://107.22.172.223:8877/59644205210b29a/output.txt

Total script time: 21.31 mins

Lint: Passed
Make references: Passed
Check references: Passed

pdfjsbot · 2014-08-15T15:05:14Z

From: Bot.io (Linux)

Success

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

Total script time: 22.48 mins

Lint: Passed
Make references: Passed
Check references: Passed

implement CalRGB color space

yurydelendik · 2014-08-15T15:19:56Z

Thank you

kkujala · 2014-08-15T18:29:37Z

Super, 1 year's worth of effort comes into fruition :) CalGray blackpoint should be farely easy now. Crossreferencing #252.

yurydelendik reviewed Aug 11, 2014
View reviewed changes

timvandermeij added the 1-core label Aug 11, 2014

yurydelendik reviewed Aug 11, 2014
View reviewed changes

implement CalRGB color space

1e4a7f9

Both whitespace and blackspace support are implemented.

yurydelendik added a commit that referenced this pull request Aug 15, 2014

Merge pull request #5165 from kkujala/calrgb

fa8d385

implement CalRGB color space

yurydelendik merged commit fa8d385 into mozilla:master Aug 15, 2014

Snuffleupagus mentioned this pull request Sep 5, 2014

PDF file with CalRGB colorspace renders blank #5270

Closed

Snuffleupagus mentioned this pull request Nov 5, 2014

PDF.JS 1.0.712 has rendering problems with some of the images #5474

Closed

implement CalRGB color space #5165

implement CalRGB color space #5165

Conversation

kkujala commented Aug 10, 2014

yurydelendik commented Aug 10, 2014

kkujala commented Aug 10, 2014

kkujala commented Aug 10, 2014

kkujala commented Aug 10, 2014

timvandermeij commented Aug 11, 2014

pdfjsbot commented Aug 11, 2014

From: Bot.io (Linux)

Received

pdfjsbot commented Aug 11, 2014

From: Bot.io (Linux)

Success

Published

timvandermeij commented Aug 11, 2014

pdfjsbot commented Aug 11, 2014

From: Bot.io (Linux)

Received

pdfjsbot commented Aug 11, 2014

From: Bot.io (Windows)

Received

pdfjsbot commented Aug 11, 2014

From: Bot.io (Windows)

Failed

pdfjsbot commented Aug 11, 2014

From: Bot.io (Linux)

Failed

yurydelendik Aug 11, 2014

Choose a reason for hiding this comment

yurydelendik commented Aug 11, 2014

CodingFabian commented Aug 11, 2014

kkujala commented Aug 11, 2014

yurydelendik commented Aug 11, 2014

yurydelendik Aug 11, 2014

Choose a reason for hiding this comment

kkujala commented Aug 11, 2014

yurydelendik commented Aug 11, 2014

kkujala commented Aug 12, 2014

yurydelendik commented Aug 12, 2014

kkujala commented Aug 14, 2014

yurydelendik commented Aug 14, 2014

kkujala commented Aug 14, 2014

yurydelendik commented Aug 15, 2014

pdfjsbot commented Aug 15, 2014

From: Bot.io (Windows)

Received

pdfjsbot commented Aug 15, 2014

From: Bot.io (Linux)

Received

pdfjsbot commented Aug 15, 2014

From: Bot.io (Windows)

Failed

pdfjsbot commented Aug 15, 2014

From: Bot.io (Linux)

Failed

yurydelendik commented Aug 15, 2014

pdfjsbot commented Aug 15, 2014

From: Bot.io (Windows)

Received

pdfjsbot commented Aug 15, 2014

From: Bot.io (Linux)

Received

pdfjsbot commented Aug 15, 2014

From: Bot.io (Windows)

Success

pdfjsbot commented Aug 15, 2014

From: Bot.io (Linux)

Success

yurydelendik commented Aug 15, 2014

kkujala commented Aug 15, 2014