Skip to content

[api-minor] Improve performances with image masks (bug 857031) #14777

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.

Sign up for GitHub

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
Merged
merged 1 commit into from
Apr 17, 2022
Merged

[api-minor] Improve performances with image masks (bug 857031) #14777

merged 1 commit into from
Apr 17, 2022

Conversation

calixteman
Copy link
Contributor

  • it's the second part of the fix for https://bugzilla.mozilla.org/show_bug.cgi?id=857031;
  • some image masks can be used several times but at different positions;
  • an image need to be pre-process before to be rendered:
    • rescale it;
    • use the fill color/pattern.
  • the two operations above are time consuming so we can cache the generated canvas;
  • the cache key is based on the current transform matrix (without the translation part)
    and the current fill color when it isn't a pattern.
  • the rendering of the pdf in the above bug is really faster than without this patch.

@Snuffleupagus
Copy link
Collaborator

I'll hopefully have time to look at this tomorrow, however in the meantime I've got one immediate suggestion:
Let's do an update on mozilla-central now, such that e.g. PR #14754 and this one won't land in the exact same Firefox Nightly.

Given the sort of changes that are being made in these PRs, there's always a regression risk (since our test-coverage is likely not perfect) and it seems much easier to figure out exactly where something broke if we space-out the landing slightly.

@Snuffleupagus Snuffleupagus changed the title Improve performances with image masks (bug 857031) [api-minor] Improve performances with image masks (bug 857031) Apr 15, 2022
Snuffleupagus
Snuffleupagus requested changes Apr 15, 2022
View reviewed changes
@Snuffleupagus
Copy link
Collaborator

/botio test

@pdfjsbot
Copy link

From: Bot.io (Windows)

Received

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

Live output at: http://54.193.163.58:8877/58e9b8c4db27157/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.241.84.105:8877/45edd53d5a5f3cb/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/45edd53d5a5f3cb/output.txt

Total script time: 23.94 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Integration Tests: FAILED
  • Regression tests: FAILED
  different ref/snapshot: 8
  different first/second rendering: 1

Image differences available at: http://54.241.84.105:8877/45edd53d5a5f3cb/reftest-analyzer.html#web=eq.log

@pdfjsbot
Copy link

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/58e9b8c4db27157/output.txt

Total script time: 24.23 mins

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

@Snuffleupagus
Copy link
Collaborator

/botio integrationtest

@pdfjsbot
Copy link

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.241.84.105:8877/576c8b51dff1439/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)

Received

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

Live output at: http://54.193.163.58:8877/6f73b16c79342e6/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/576c8b51dff1439/output.txt

Total script time: 3.75 mins

  • Integration Tests: FAILED

Snuffleupagus
Snuffleupagus approved these changes Apr 16, 2022
View reviewed changes
Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment

Choose a reason for hiding this comment

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

I don't think that the integration-test failures are related to this patch, since similar failures were observed elsewhere as well.

How about adding the first page of the document in bug 857031 as a linked reference-test, to ensure that we have proper test-coverage for all this new code?

r=me, since this looks reasonable (although the src/display/canvas.js-code is one part of the code-base that I'm finding quite difficult to navigate at times).

src/display/canvas.js Outdated
@@ -1275,6 +1280,16 @@ class CanvasGraphics {
this.cachedCanvases.clear();
this.cachedPatterns.clear();

for (const cache of this._cachedBitmapsMap.values()) {
for (const canvas of cache.values()) {
if (canvas instanceof HTMLCanvasElement) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This code-path can be taken in Node.js environments as well, right?
In that case I don't believe that HTMLCanvasElement exists, which would throw an Error here and break things. Hence please add a typeof HTMLCanvasElement !== undefined check to avoid any problems.

src/display/canvas.js Outdated
);
const maskCtx = maskCanvas.context;
putBinaryImageMask(maskCtx, img);
const objToCanvas = ctx.mozCurrentTransform;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know that you're just moving pre-existing code, but how about giving this variable a more appropriate name (since I don't see how objToCanvas actually makes sense)?

src/display/canvas.js Outdated
Comment on lines 1380 to 1383
cacheKey = isPatternFill
? withoutTranslation
: [withoutTranslation, fillColor];
cacheKey = JSON.stringify(cacheKey);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Maybe just doing something like this instead?

cacheKey = JSON.stringify(
  isPatternFill ? withoutTranslation : [withoutTranslation, fillColor]
);

src/display/canvas.js Outdated
const objToCanvas = ctx.mozCurrentTransform;

let cache, cacheKey, scaled, maskCanvas;
if ((img.bitmap || img.data) && img.count >= 2) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Maybe using ... && img.count > 1 instead?

@calixteman
Copy link
Contributor Author

/botio test

@pdfjsbot
Copy link

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.241.84.105:8877/60ec0a11d072688/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/60ec0a11d072688/output.txt

Total script time: 2.13 mins

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

Image differences available at: http://54.241.84.105:8877/60ec0a11d072688/reftest-analyzer.html#web=eq.log

@calixteman
Improve performances with image masks (bug 857031)
f62d961 
- it's the second part of the fix for https://bugzilla.mozilla.org/show_bug.cgi?id=857031;
- some image masks can be used several times but at different positions;
- an image need to be pre-process before to be rendered:
  * rescale it;
  * use the fill color/pattern.
- the two operations above are time consuming so we can cache the generated canvas;
- the cache key is based on the current transform matrix (without the translation part)
  and the current fill color when it isn't a pattern.
- the rendering of the pdf in the above bug is really faster than without this patch.
@calixteman
Copy link
Contributor Author

/botio test

@pdfjsbot
Copy link

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.241.84.105:8877/0972f6fcf7ba7b8/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/0972f6fcf7ba7b8/output.txt

Total script time: 24.08 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Integration Tests: FAILED
  • Regression tests: FAILED
  different ref/snapshot: 5
  different first/second rendering: 1

Image differences available at: http://54.241.84.105:8877/0972f6fcf7ba7b8/reftest-analyzer.html#web=eq.log

@calixteman
Copy link
Contributor Author

/botio test

@pdfjsbot
Copy link

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.241.84.105:8877/aa520e2e6db1bd9/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)

Received

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

Live output at: http://54.193.163.58:8877/1e3fb9fcf5e40d8/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/aa520e2e6db1bd9/output.txt

Total script time: 23.75 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Integration Tests: FAILED
  • Regression tests: FAILED
  different ref/snapshot: 6

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

@pdfjsbot
Copy link

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/1e3fb9fcf5e40d8/output.txt

Total script time: 25.10 mins

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

@calixteman calixteman merged commit 379125c into mozilla:master Apr 17, 2022
@calixteman
Copy link
Contributor Author

/botio makeref

@pdfjsbot
Copy link

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.241.84.105:8877/4d274a910519d93/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)

Received

Command cmd_makeref from @calixteman received. Current queue size: 1

Live output at: http://54.193.163.58:8877/90e222f41b3d2fa/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)

Success

Full output at http://54.241.84.105:8877/4d274a910519d93/output.txt

Total script time: 22.08 mins

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

@pdfjsbot
Copy link

From: Bot.io (Windows)

Success

Full output at http://54.193.163.58:8877/90e222f41b3d2fa/output.txt

Total script time: 21.19 mins

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

@Snuffleupagus Snuffleupagus linked an issue Apr 18, 2022 that may be closed by this pull request
PDF rendering is quite slow with some PDFs, compared to other viewers #14652
Closed
@calixteman calixteman mentioned this pull request Apr 22, 2022
Closed
@Snuffleupagus Snuffleupagus mentioned this pull request May 8, 2022
Closed
@calixteman calixteman mentioned this pull request Oct 14, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Snuffleupagus Snuffleupagus Snuffleupagus approved these changes

Assignees
No one assigned
Labels
image-conversion performance
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

PDF rendering is quite slow with some PDFs, compared to other viewers
3 participants
@calixteman @Snuffleupagus @pdfjsbot