-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
Conversation
/botio test |
From: Bot.io (Windows)ReceivedCommand cmd_test from @calixteman received. Current queue size: 0 Live output at: http://54.193.163.58:8877/45a5c603b34d1d4/output.txt |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @calixteman received. Current queue size: 0 Live output at: http://54.241.84.105:8877/d5b6ec797818efa/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.241.84.105:8877/d5b6ec797818efa/output.txt Total script time: 30.69 mins
|
From: Bot.io (Windows)FailedFull output at http://54.193.163.58:8877/45a5c603b34d1d4/output.txt Total script time: 58.82 mins
|
/botio-linux preview |
From: Bot.io (Linux m4)ReceivedCommand cmd_preview from @calixteman received. Current queue size: 0 Live output at: http://54.241.84.105:8877/b25e54400a20b38/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.241.84.105:8877/b25e54400a20b38/output.txt Total script time: 0.87 mins Published |
There was a problem hiding this 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).
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. |
What happens if rendering is paused, because another page becomes more visible before the delay has been reached? |
00ba485
to
cc9acd5
Compare
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 ( |
Another question, should we lower the time before the page loading indicator is displayed now? Line 31 in b47b248
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. |
/botio-linux preview |
From: Bot.io (Linux m4)ReceivedCommand cmd_preview from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.241.84.105:8877/dce77563326740a/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.241.84.105:8877/dce77563326740a/output.txt Total script time: 1.09 mins Published |
There was a problem hiding this 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%.
/botio test |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @calixteman received. Current queue size: 1 Live output at: http://54.241.84.105:8877/212106326d2a07f/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @calixteman received. Current queue size: 1 Live output at: http://54.193.163.58:8877/4e60bd3e706dc0f/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.241.84.105:8877/212106326d2a07f/output.txt Total script time: 30.42 mins
|
From: Bot.io (Windows)FailedFull output at http://54.193.163.58:8877/4e60bd3e706dc0f/output.txt Total script time: 61.23 mins
|
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%.