Update `JpegImage.getData` to support `forceRGBoutput` for images with `numComponents === 1` (issue 6066) #6984

Snuffleupagus · 2016-02-13T13:05:50Z

A more robust solution for issue #6066.

As a temporary work-around for (the upstream) bug 1164199, we parsed all images in the Firefox addon during a short time.
Doing so uncovered an issue with our image handling (see #6066), for JPEG images with a DeviceGray ColorSpace and bpc !== 1 (bits per component).

As long as we let the browser handle image decoding in this case, this isn't going to be an issue, but I do think that we should proactively fix this to avoid future issues if we change where the images are decoded (in jpg.js vs in browser).
Also, we currently don't seem to have a test-case for that kind of image data.

Snuffleupagus · 2016-02-13T13:07:11Z

/botio test

pdfjsbot · 2016-02-13T13:07:12Z

From: Bot.io (Windows)

Received

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

Live output at: http://107.22.172.223:8877/03c240f23393c15/output.txt

pdfjsbot · 2016-02-13T13:07:12Z

From: Bot.io (Linux)

Received

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

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

pdfjsbot · 2016-02-13T13:27:36Z

From: Bot.io (Windows)

Success

Full output at http://107.22.172.223:8877/03c240f23393c15/output.txt

Total script time: 20.40 mins

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

pdfjsbot · 2016-02-13T13:28:49Z

From: Bot.io (Linux)

Failed

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

Total script time: 21.61 mins

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

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

Snuffleupagus · 2016-02-13T14:47:39Z

/botio-linux test

pdfjsbot · 2016-02-13T14:47:41Z

From: Bot.io (Linux)

Received

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

Live output at: http://107.21.233.14:8877/1a01196acabbb20/output.txt

pdfjsbot · 2016-02-13T15:09:30Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/1a01196acabbb20/output.txt

Total script time: 21.82 mins

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

Snuffleupagus · 2016-02-15T12:09:18Z

Note that the fast-path for JPEG images was added in PR #4528.
In PR #4892 it was tweaked, in order to fix a regression. Then in PR #6366 it was changed again to address another regression, and this PR tries to avoid yet another regression in this code.

brendandahl · 2016-02-16T17:14:28Z

Something that may be worth exploring is always using jpg.js. In a few basic tests I did it was actually faster to not use the browser to decode images.

Snuffleupagus · 2016-02-16T18:09:22Z

Something that may be worth exploring is always using jpg.js. In a few basic tests I did it was actually faster to not use the browser to decode images.

Yes, it could be interesting to compare the performance!
I've not looked at the performance side of things, but a while back I tested it locally, and there were a couple of regressions that would need to be addressed if we made the switch, and this patch would fix one of them.
@brendandahl Are you OK with landing this PR?

brendandahl · 2016-02-17T01:45:02Z

@brendandahl Are you OK with landing this PR?

What about fixing JpegImage.getData() for numComponents === 1 and forceRGB = true?

brendandahl · 2016-02-17T01:48:49Z

test/test_manifest.json

+       "md5": "b26eb08fc5ab2518ba8fde603bdfc46b",
+       "rounds": 1,
+       "link": true,
+       "firstPage": 1,


Do we really need to run both pages?

Not really, I've changed this in the new patch.

Snuffleupagus · 2016-02-17T15:39:23Z

What about fixing JpegImage.getData() for numComponents === 1 and forceRGB = true?

Is the new patch more in line with what you had in mind here?

/botio test

pdfjsbot · 2016-02-17T15:39:23Z

From: Bot.io (Windows)

Received

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

Live output at: http://107.22.172.223:8877/5fffab191b05d6e/output.txt

pdfjsbot · 2016-02-17T15:39:23Z

From: Bot.io (Linux)

Received

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

Live output at: http://107.21.233.14:8877/47c51abae870185/output.txt

pdfjsbot · 2016-02-17T15:59:46Z

From: Bot.io (Windows)

Success

Full output at http://107.22.172.223:8877/5fffab191b05d6e/output.txt

Total script time: 20.37 mins

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

pdfjsbot · 2016-02-17T16:00:57Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/47c51abae870185/output.txt

Total script time: 21.57 mins

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

brendandahl · 2016-02-17T22:08:23Z

src/core/jpg.js

+        var rgbData = new Uint8Array(width * height * 3);
+        var offset = 0;
+        for (var i = 0, ii = data.length; i < ii; i++) {
+          rgbData[offset++] = data[i];


I'm not sure if the jit is smart enough to realize it's accessing the same data[i] here. Best to probably stick it in a variable within the loop.

Done in the new commit.

brendandahl · 2016-02-17T22:08:58Z

Is the new patch more in line with what you had in mind here?

Yup, looks good with one nit fixed above.

…h `numComponents === 1` (issue 6066) *A more robust solution for issue 6066.* As a temporary work-around for (the upstream) [bug 1164199](https://bugzilla.mozilla.org/show_bug.cgi?id=1164199), we parsed *all* images in the Firefox addon during a short time. Doing so uncovered an issue with our image handling (see 6066), for JPEG images with a `DeviceGray` ColorSpace *and* `bpc !== 1` (bits per component). As long as we let the browser handle image decoding in this case, this isn't going to be an issue, but I do think that we should proactively fix this to avoid future issues if we change where the images are decoded (in `jpg.js` vs in browser). Also, we currently don't seem to have a test-case for that kind of image data.

Snuffleupagus · 2016-02-18T09:18:49Z

/botio test

pdfjsbot · 2016-02-18T09:18:52Z

From: Bot.io (Linux)

Received

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

Live output at: http://107.21.233.14:8877/93201b0394df9e3/output.txt

pdfjsbot · 2016-02-18T09:18:52Z

From: Bot.io (Windows)

Received

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

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

pdfjsbot · 2016-02-18T09:39:07Z

From: Bot.io (Windows)

Success

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

Total script time: 20.25 mins

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

pdfjsbot · 2016-02-18T09:40:40Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/93201b0394df9e3/output.txt

Total script time: 21.81 mins

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

Snuffleupagus · 2016-02-18T10:49:07Z

Is the new patch more in line with what you had in mind here?

Yup, looks good with one nit fixed above.

I'm assuming that this means r+, so having addressed the nit, I'm going to land this.
@brendandahl Thanks for reviewing!

/botio makeref

pdfjsbot · 2016-02-18T10:49:09Z

From: Bot.io (Linux)

Received

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

Live output at: http://107.21.233.14:8877/32831967815e323/output.txt

pdfjsbot · 2016-02-18T10:49:09Z

From: Bot.io (Windows)

Received

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

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

Update `JpegImage.getData` to support `forceRGBoutput` for images with `numComponents === 1` (issue 6066)

pdfjsbot · 2016-02-18T11:09:08Z

From: Bot.io (Windows)

Success

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

Total script time: 19.99 mins

Lint: Passed
Make references: Passed
Check references: Passed

pdfjsbot · 2016-02-18T11:10:09Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/32831967815e323/output.txt

Total script time: 21.00 mins

Lint: Passed
Make references: Passed
Check references: Passed

Snuffleupagus added image-quality image-conversion and removed image-quality labels Feb 13, 2016

brendandahl reviewed Feb 17, 2016
View reviewed changes

Snuffleupagus changed the title ~~Don't take the fast-path in PDFImage_createImageData for unmasked JPEG images with DeviceGray ColorSpaces~~ Update JpegImage.getData to support forceRGBoutput for images with numComponents === 1 (issue 6066) Feb 17, 2016

brendandahl reviewed Feb 17, 2016
View reviewed changes

Snuffleupagus added a commit that referenced this pull request Feb 18, 2016

Merge pull request #6984 from Snuffleupagus/issue-6066

8cdb696

Update `JpegImage.getData` to support `forceRGBoutput` for images with `numComponents === 1` (issue 6066)

Snuffleupagus merged commit 8cdb696 into mozilla:master Feb 18, 2016

Snuffleupagus deleted the issue-6066 branch February 18, 2016 11:37

Snuffleupagus mentioned this pull request Jul 11, 2017

[DO NOT MERGE] Use src/core/jpg.js for all JPEG image decoding #8637

Closed

Snuffleupagus mentioned this pull request Dec 29, 2017

Avoid truncating JPEG images with DeviceGray ColourSpaces when using the src/core/jpg.js built-in decoder #9327

Merged

Snuffleupagus mentioned this pull request Jan 27, 2020

Add a heuristic, in src/core/jpg.js, to handle JPEG images with a wildly incorrect SOF (Start of Frame) scanLines parameter (issue 10880) #11523

Merged

Snuffleupagus mentioned this pull request Apr 28, 2020

[api-minor] Decode all JPEG images with the built-in PDF.js decoder in src/core/jpg.js #11601

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update `JpegImage.getData` to support `forceRGBoutput` for images with `numComponents === 1` (issue 6066) #6984

Update `JpegImage.getData` to support `forceRGBoutput` for images with `numComponents === 1` (issue 6066) #6984

Snuffleupagus commented Feb 13, 2016

Snuffleupagus commented Feb 13, 2016

pdfjsbot commented Feb 13, 2016

pdfjsbot commented Feb 13, 2016

pdfjsbot commented Feb 13, 2016

pdfjsbot commented Feb 13, 2016

Snuffleupagus commented Feb 13, 2016

pdfjsbot commented Feb 13, 2016

pdfjsbot commented Feb 13, 2016

Snuffleupagus commented Feb 15, 2016

brendandahl commented Feb 16, 2016

Snuffleupagus commented Feb 16, 2016

brendandahl commented Feb 17, 2016

brendandahl Feb 17, 2016

Snuffleupagus Feb 17, 2016

Snuffleupagus commented Feb 17, 2016

pdfjsbot commented Feb 17, 2016

pdfjsbot commented Feb 17, 2016

pdfjsbot commented Feb 17, 2016

pdfjsbot commented Feb 17, 2016

brendandahl Feb 17, 2016

Snuffleupagus Feb 18, 2016

brendandahl commented Feb 17, 2016

Snuffleupagus commented Feb 18, 2016

pdfjsbot commented Feb 18, 2016

pdfjsbot commented Feb 18, 2016

pdfjsbot commented Feb 18, 2016

pdfjsbot commented Feb 18, 2016

Snuffleupagus commented Feb 18, 2016

pdfjsbot commented Feb 18, 2016

pdfjsbot commented Feb 18, 2016

pdfjsbot commented Feb 18, 2016

pdfjsbot commented Feb 18, 2016

Update JpegImage.getData to support forceRGBoutput for images with numComponents === 1 (issue 6066) #6984

Update JpegImage.getData to support forceRGBoutput for images with numComponents === 1 (issue 6066) #6984

Conversation

Snuffleupagus commented Feb 13, 2016

Snuffleupagus commented Feb 13, 2016

pdfjsbot commented Feb 13, 2016

From: Bot.io (Windows)

Received

pdfjsbot commented Feb 13, 2016

From: Bot.io (Linux)

Received

pdfjsbot commented Feb 13, 2016

From: Bot.io (Windows)

Success

pdfjsbot commented Feb 13, 2016

From: Bot.io (Linux)

Failed

Snuffleupagus commented Feb 13, 2016

pdfjsbot commented Feb 13, 2016

From: Bot.io (Linux)

Received

pdfjsbot commented Feb 13, 2016

From: Bot.io (Linux)

Success

Snuffleupagus commented Feb 15, 2016

brendandahl commented Feb 16, 2016

Snuffleupagus commented Feb 16, 2016

brendandahl commented Feb 17, 2016

brendandahl Feb 17, 2016

Choose a reason for hiding this comment

Snuffleupagus Feb 17, 2016

Choose a reason for hiding this comment

Snuffleupagus commented Feb 17, 2016

pdfjsbot commented Feb 17, 2016

From: Bot.io (Windows)

Received

pdfjsbot commented Feb 17, 2016

From: Bot.io (Linux)

Received

pdfjsbot commented Feb 17, 2016

From: Bot.io (Windows)

Success

pdfjsbot commented Feb 17, 2016

From: Bot.io (Linux)

Success

brendandahl Feb 17, 2016

Choose a reason for hiding this comment

Snuffleupagus Feb 18, 2016

Choose a reason for hiding this comment

brendandahl commented Feb 17, 2016

Snuffleupagus commented Feb 18, 2016

pdfjsbot commented Feb 18, 2016

From: Bot.io (Linux)

Received

pdfjsbot commented Feb 18, 2016

From: Bot.io (Windows)

Received

pdfjsbot commented Feb 18, 2016

From: Bot.io (Windows)

Success

pdfjsbot commented Feb 18, 2016

From: Bot.io (Linux)

Success

Snuffleupagus commented Feb 18, 2016

pdfjsbot commented Feb 18, 2016

From: Bot.io (Linux)

Received

pdfjsbot commented Feb 18, 2016

From: Bot.io (Windows)

Received

pdfjsbot commented Feb 18, 2016

From: Bot.io (Windows)

Success

pdfjsbot commented Feb 18, 2016

From: Bot.io (Linux)

Success

Update `JpegImage.getData` to support `forceRGBoutput` for images with `numComponents === 1` (issue 6066) #6984

Update `JpegImage.getData` to support `forceRGBoutput` for images with `numComponents === 1` (issue 6066) #6984