Skip to content
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

Improve image mask handling again #4406

Merged
merged 2 commits into from
Mar 10, 2014

Conversation

nnethercote
Copy link
Contributor

The first patch fixes a what I believe is a bug in the handling of image masks when the amount of data falls short of height*width. This causes the test for issue2984.pdf to fail, but that's because the previous rendering was incorrect, featuring lots of thin horizontal lines at the tops of many letters. This patch causes those lines to disappear, which is the same result given by evince and Mac Preview.

The second patch optimizates mask handling by transferring image mask arrays where possible, instead of copying. I have two test files involving image masks, and the reduction in peak RSS was as follows:

  • bach: ~310 MiB to ~280 MiB
  • 37335: ~255 MiB to ~230 MiB

@Snuffleupagus
Copy link
Collaborator

/botio test

@pdfjsbot
Copy link

pdfjsbot commented Mar 7, 2014

From: Bot.io (Linux)


Received

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

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

@pdfjsbot
Copy link

pdfjsbot commented Mar 7, 2014

From: Bot.io (Windows)


Received

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

Live output at: http://107.22.172.223:8877/56ce53a7a85efba/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Mar 7, 2014

From: Bot.io (Linux)


Failed

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

Total script time: 24.48 mins

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

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

@pdfjsbot
Copy link

pdfjsbot commented Mar 7, 2014

From: Bot.io (Windows)


Failed

Full output at http://107.22.172.223:8877/56ce53a7a85efba/output.txt

Total script time: 35.55 mins

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

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

// Invert if necessary.
if (inverseDecode) {
for (var i = 0; i < actualLength; i++) {
data[i] = ~data[i];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we corrupting the original PDF data here? Does it make sense to do inversion on copy only?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we haven't made a copy, that's because we've decided it's safe to transfer, which means we've effectively taken possession of the array. So modifying it should be ok.

I can explain this better in the comment, if you like.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, yes, comment would be nice. Thanks

@timvandermeij
Copy link
Contributor

/botio-windows preview

@pdfjsbot
Copy link

pdfjsbot commented Mar 7, 2014

From: Bot.io (Windows)


Received

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

Live output at: http://107.22.172.223:8877/3de895a63740db3/output.txt

@timvandermeij
Copy link
Contributor

Fixes #3411. Thanks!

@@ -146,10 +146,12 @@ var PartialEvaluator = (function PartialEvaluatorClosure() {
var bitStrideLength = (width + 7) >> 3;
var imgArray = image.getBytes(bitStrideLength * height);
var decode = dict.get('Decode', 'D');
var canTransfer = image instanceof DecodeStream;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jshint needs definition

@nnethercote
Copy link
Contributor Author

I updated the globals list for jshint, and expanded the comment.

@timvandermeij
Copy link
Contributor

/botio test

@pdfjsbot
Copy link

pdfjsbot commented Mar 8, 2014

From: Bot.io (Windows)


Received

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

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

@pdfjsbot
Copy link

pdfjsbot commented Mar 8, 2014

From: Bot.io (Linux)


Received

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

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

@pdfjsbot
Copy link

pdfjsbot commented Mar 8, 2014

From: Bot.io (Linux)


Failed

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

Total script time: 25.31 mins

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

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

@pdfjsbot
Copy link

pdfjsbot commented Mar 8, 2014

From: Bot.io (Windows)


Failed

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

Total script time: 35.41 mins

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

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

@nnethercote
Copy link
Contributor Author

I don't know why bug921760 and issue1796 are failing. I only changed comments in my updated patches, and the earlier versions passed. Those tests pass for me locally with the updated patches. Do the failures have some other cause?

@Snuffleupagus
Copy link
Collaborator

I don't know why bug921760 and issue1796 are failing.

You can safely ignore those failures! They happened because we forgot to generate new reference images in PR #4318, but that has been fixed now.

Let me re-run the tests, just to make sure.
/botio test

@pdfjsbot
Copy link

pdfjsbot commented Mar 8, 2014

From: Bot.io (Linux)


Received

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

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

@pdfjsbot
Copy link

pdfjsbot commented Mar 8, 2014

From: Bot.io (Windows)


Received

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

Live output at: http://107.22.172.223:8877/4445b4930d770e1/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Mar 8, 2014

From: Bot.io (Linux)


Failed

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

Total script time: 25.31 mins

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

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

@pdfjsbot
Copy link

pdfjsbot commented Mar 8, 2014

From: Bot.io (Windows)


Failed

Full output at http://107.22.172.223:8877/4445b4930d770e1/output.txt

Total script time: 35.50 mins

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

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

@nnethercote
Copy link
Contributor Author

So now the only "failure" is with issue2984, which is expected. Thanks for re-triggering the tests.

@brendandahl
Copy link
Contributor

/botio makeref

@pdfjsbot
Copy link

From: Bot.io (Linux)


Received

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

Live output at: http://107.21.233.14:8877/81d6863e1a7b884/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

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

Live output at: http://107.22.172.223:8877/21ff2fa69035fe6/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux)


Success

Full output at http://107.21.233.14:8877/81d6863e1a7b884/output.txt

Total script time: 25.42 mins

  • Lint: Passed
  • Make references: Passed
  • Check references: Passed

@pdfjsbot
Copy link

From: Bot.io (Windows)


Success

Full output at http://107.22.172.223:8877/21ff2fa69035fe6/output.txt

Total script time: 35.33 mins

  • Lint: Ignored
  • Make references: Passed
  • Check references: Passed

brendandahl added a commit that referenced this pull request Mar 10, 2014
@brendandahl brendandahl merged commit 57e896d into mozilla:master Mar 10, 2014
@nnethercote nnethercote deleted the fix-and-transfer-masks branch March 14, 2014 04:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants