Skip to content

Don't update the visible canvas at 60 fps (bug 1936605) #19856

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

Merged
merged 1 commit into from
Apr 29, 2025

Conversation

calixteman
Copy link
Contributor

Instead, we update the visible canvas every 500ms. With large canvas, updating at 60fps lead to a lot gfx transactions and it can take a lot of time. For example, with wuppertal_2012.pdf on Windows, displaying it at 150% takes around 14 min !!! without this patch when it takes only around 14 sec with. Even at 30% it helps to improve the performance by around 20%.

@calixteman
Copy link
Contributor Author

/botio test

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

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

Live output at: http://54.193.163.58:8877/45a5c603b34d1d4/output.txt

@moz-tools-bot
Copy link
Collaborator

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/d5b6ec797818efa/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Failed

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

Total script time: 30.69 mins

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

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/45a5c603b34d1d4/output.txt

Total script time: 58.82 mins

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

@calixteman
Copy link
Contributor Author

/botio-linux preview

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

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

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

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Success

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

Total script time: 0.87 mins

Published

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 think this begs the question if we should just do all rendering in the background, since that'd simplify things considerably.

As-is, my primary "issue" with this is that it adds a bunch of (quite frankly undesirable) complexity to the _createCanvas-method and it'd be nice not having to do that.

Additionally, it seems that this will effectively make the Stepper (in debugger.mjs) unusable so this should probably be controlled by a proper AppOptions-entry rather than a code-constant (such that it can be disabled when loading the debugger).

@calixteman
Copy link
Contributor Author

I thought about doing all the rendering in background but in case the pdf is too long to render then the user could think that something is broken, so with this version we give some signals that something is happening.

@Snuffleupagus
Copy link
Collaborator

I thought about doing all the rendering in background but in case the pdf is too long to render then the user could think that something is broken, so with this version we give some signals that something is happening.

Doesn't the loading indicators spinners, that exist on both page-element itself and in the page-number input (in the toolbar), already help show that rendering is happening?

@calixteman
Copy link
Contributor Author

I thought about doing all the rendering in background but in case the pdf is too long to render then the user could think that something is broken, so with this version we give some signals that something is happening.

Doesn't the loading indicators spinners, that exist on both page-element itself and in the page-number input (in the toolbar), already help show that rendering is happening?

They should help for sure but I'm not totally sure that users in general know that when the spinner is spinning then it means that the rendering is in progress.
For example they could think that there is a network issue/slowdown or something is wrong with Firefox itself.
So at least with a partial rendering we give a clear signal that something is happening: no doubt.
I want to avoid people trying to reload their tab or start to zoom while waiting (especially on mobile) just see if something happens (like I can do myself when I think something is stuck even if there's a hourglass, a spinner or whatever).

@Snuffleupagus
Copy link
Collaborator

What happens if rendering is paused, because another page becomes more visible before the delay has been reached?
Should we ensure that we "forcibly" invoke #showCanvas in that case, to avoid having a blank canvas in the viewer?

@calixteman calixteman force-pushed the bug1936605 branch 2 times, most recently from 00ba485 to cc9acd5 Compare April 25, 2025 19:28
@Snuffleupagus
Copy link
Collaborator

Something else that occurred to me is that we may not want to disable this unconditionally when the debugger is used, such that e.g. http://localhost:8888/web/viewer.html#pdfBug=Stats can be used to still get "correct" performance numbers.

diff --git a/web/app.js b/web/app.js
index 1e245ec38..d94c99fb2 100644
--- a/web/app.js
+++ b/web/app.js
@@ -326,8 +326,6 @@ const PDFViewerApplication = {
       }
     }
     if (params.has("pdfbug")) {
-      AppOptions.setAll({ pdfBug: true, fontExtraProperties: true });
-
       const enabled = params.get("pdfbug").split(",");
       try {
         await loadPDFBug();
@@ -335,6 +333,12 @@ const PDFViewerApplication = {
       } catch (ex) {
         console.error("_parseHashParams:", ex);
       }
+
+      const debugOpts = { pdfBug: true, fontExtraProperties: true };
+      if (globalThis.StepperManager?.enabled) {
+        debugOpts.minDurationToUpdateCanvas = 0;
+      }
+      AppOptions.setAll(debugOpts);
     }
     // It is not possible to change locale for the (various) extension builds.
     if (

@Snuffleupagus
Copy link
Collaborator

Another question, should we lower the time before the page loading indicator is displayed now?

--loading-icon-delay: 400ms;

Currently it's obvious that rendering is happening, since the page renders incrementally and visibly, however with the changes in this patch there will be less initial indication of rendering being in progress.

@calixteman
Copy link
Contributor Author

Another question, should we lower the time before the page loading indicator is displayed now?

--loading-icon-delay: 400ms;

Currently it's obvious that rendering is happening, since the page renders incrementally and visibly, however with the changes in this patch there will be less initial indication of rendering being in progress.

Honestly, I'm not sure that a "normal" human would make a difference between having 100 or 200 ms here vs 400ms.
I mean the time used to formalize the thought "what's happening with pdf ???" and the display of the spinner is probably something around that.
At least in not reducing the value we don't loose few cycles (and mW).
It's the kind of thing we could change in a follow-up if some users complain.

@Snuffleupagus
Copy link
Collaborator

/botio-linux preview

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

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

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

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Success

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

Total script time: 1.09 mins

Published

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.

This seems to work correctly in my testing, but perhaps this is the sort of feature that could benefit from having QA test zooming (e.g. with big maps) a bit once this patch reaches mozilla-central?

r=me, thank you.

Instead, we update the visible canvas every 500ms.
With large canvas, updating at 60fps lead to a lot gfx transactions and it can take a lot of time.
For example, with wuppertal_2012.pdf on Windows, displaying it at 150% takes around 14 min !!! without
this patch when it takes only around 14 sec with. Even at 30% it helps to improve the performance
by around 20%.
@calixteman
Copy link
Contributor Author

/botio test

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.241.84.105:8877/212106326d2a07f/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

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

Live output at: http://54.193.163.58:8877/4e60bd3e706dc0f/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Failed

Full output at http://54.241.84.105:8877/212106326d2a07f/output.txt

Total script time: 30.42 mins

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

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/4e60bd3e706dc0f/output.txt

Total script time: 61.23 mins

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

@calixteman calixteman merged commit b8de9a3 into mozilla:master Apr 29, 2025
7 checks passed
@calixteman calixteman deleted the bug1936605 branch April 29, 2025 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants