The Wayback Machine - https://web.archive.org/web/20221222064414/https://github.com/mozilla/pdf.js/pull/5669
Skip to content
Sign up
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Avoid out of range array access in JBIG2 decoder #5669

Merged
merged 1 commit into from Jan 26, 2015
Merged

Avoid out of range array access in JBIG2 decoder #5669

merged 1 commit into from Jan 26, 2015

Conversation

fkaelberer
Copy link
Contributor

@fkaelberer fkaelberer commented Jan 23, 2015

Fixes #5663. Chrome rendering times are down from 4 seconds to <1.5s.

Benchmark with document from #2909:

browser Count Baseline(ms) Current(ms) +/- % Result(P<.05)
chrome 180 3215 997 -2218 -69.00 faster
firefoxNightly 180 1156 1139 -18 -1.53

@fkaelberer fkaelberer force-pushed the avoidOutOfRangeArrayAccessInJbig2Decoder branch from 7a1e803 to 8013100 Compare Jan 23, 2015
@Snuffleupagus
Copy link
Collaborator

Snuffleupagus commented Jan 23, 2015

/botio test

@pdfjsbot
Copy link
Collaborator

pdfjsbot commented Jan 23, 2015

From: Bot.io (Windows)

Received

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

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

@pdfjsbot
Copy link
Collaborator

pdfjsbot commented Jan 23, 2015

From: Bot.io (Linux)

Received

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

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

@pdfjsbot
Copy link
Collaborator

pdfjsbot commented Jan 23, 2015

From: Bot.io (Windows)

Failed

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

Total script time: 3.84 mins

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

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

@pdfjsbot
Copy link
Collaborator

pdfjsbot commented Jan 23, 2015

From: Bot.io (Linux)

Success

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

Total script time: 23.03 mins

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

@yurydelendik
Copy link
Contributor

yurydelendik commented Jan 26, 2015

/botio-windows test

@pdfjsbot
Copy link
Collaborator

pdfjsbot commented Jan 26, 2015

From: Bot.io (Windows)

Received

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

Live output at: http://107.22.172.223:8877/768c15980bd259d/output.txt

@pdfjsbot
Copy link
Collaborator

pdfjsbot commented Jan 26, 2015

From: Bot.io (Windows)

Success

Full output at http://107.22.172.223:8877/768c15980bd259d/output.txt

Total script time: 17.93 mins

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

@timvandermeij
Copy link
Contributor

timvandermeij commented Jan 26, 2015

/botio-linux preview

@pdfjsbot
Copy link
Collaborator

pdfjsbot commented Jan 26, 2015

From: Bot.io (Linux)

Received

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

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

@pdfjsbot
Copy link
Collaborator

pdfjsbot commented Jan 26, 2015

From: Bot.io (Linux)

Success

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

Total script time: 0.78 mins

Published

timvandermeij added a commit that referenced this pull request Jan 26, 2015
@timvandermeij
Merge pull request #5669 from fkaelberer/avoidOutOfRangeArrayAccessIn…
40f9f77 
…Jbig2Decoder

Avoid out of range array access in JBIG2 decoder
@timvandermeij timvandermeij merged commit 40f9f77 into mozilla:master Jan 26, 2015
@timvandermeij
Copy link
Contributor

timvandermeij commented Jan 26, 2015

Really nice, thank you!

@CodingFabian
Copy link
Contributor

CodingFabian commented Jan 26, 2015

I was just looking at the code, and I wondered if you have tested unrolling that magic or yourself

contextLabel = ((contextLabel & OLD_PIXEL_MASK) << 1) |
                         (j + 3 < width ? row2[j + 3] << 11 : 0) |
                       (j + 4 < width ? row1[j + 4] << 4 : 0) | pixel;

could be

contextLabel = (contextLabel & OLD_PIXEL_MASK) << 1;
if (contextLabel) {
  continue;
} 
if (j + 3 < width) {
 contextLabel = row2[j + 3] << 11;
 if (contextLabel) continue;
}
if (j + 4 < width) {
 contextLabel = row1[j + 4] << 4;
 if (contextLabel) continue;
}
contextLabel = pixel;

I dont have the setup handy, maybe if thats interesting for you, try it out and let me know. I know the code looks way uglier than before, but maybe its even worth.

@fkaelberer
Copy link
Contributor Author

fkaelberer commented Jan 27, 2015

@CodingFabian Before I committed this fix, I have tested similar code to avoid the branching in the j + x < width branching in the loop, but the performance gain was below 1 or 2% for ~15 additional lines of code. But I preferred simplicity/readability over the marginal speed gain. And don't confuse | with ||: you can't continue early if contextLabel is != 0 :-]

@fkaelberer fkaelberer deleted the avoidOutOfRangeArrayAccessInJbig2Decoder branch Jan 27, 2015
speedplane pushed a commit to speedplane/pdf.js that referenced this pull request Feb 24, 2015
@timvandermeij
Merge pull request mozilla#5669 from fkaelberer/avoidOutOfRangeArrayA…
5c2eb6c 
…ccessInJbig2Decoder

Avoid out of range array access in JBIG2 decoder
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
image-jbig2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Attached PDF Renders very slowly
6 participants
@fkaelberer @Snuffleupagus @pdfjsbot @yurydelendik @timvandermeij @CodingFabian
Morty Proxy This is a proxified and sanitized view of the page, visit original site.