Skip to content

Add a pref in order to cap the canvas area to a factor of the window one (bug 1958015) #19755

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
May 9, 2025

Conversation

calixteman
Copy link
Contributor

@calixteman calixteman commented Apr 2, 2025

This way it helps to reduce the overall canvas dimensions and make the rendering faster. The drawback is that when scrolling, the page can be blurry in waiting for the rendering.

The default value is 200% on desktop and will be 100% for GeckoView.

@Snuffleupagus
Copy link
Collaborator

Snuffleupagus commented Apr 2, 2025

This limit really seems way too low to be practical/helpful, since I cannot even open e.g. the tracemonkey.pdf document at the auto-zoom level (i.e. 125%) without triggering use of detailViews which is quite annoying when scrolling through the document (since there's now lots of re-rendering happening all the time).

This way it helps to reduce the overall canvas dimensions and make the rendering faster. The drawback is that when scrolling, the page can be blurry in waiting for the rendering.

Maybe it makes the initial rendering slightly faster, but it seems that you'd be losing at least whatever time was saved (if not more) by now having significantly more re-rendering on scrolling.

@marco-c
Copy link
Contributor

marco-c commented Apr 2, 2025

What if we make the value dynamic based on the same factors we use for the auto-zoom level? If the text is very small, it's very likely the user will zoom and scroll, and so we might want to pre-render more.

@marco-c
Copy link
Contributor

marco-c commented Apr 2, 2025

Another idea: we could render with a smaller canvas, but pre-render more area around it (or even simply a larger canvas) while on idle.

@Snuffleupagus
Copy link
Collaborator

Given that the bug now links to a GeckoView-specific bug, I'd be more OK with the general idea of this patch if we use pre-processor statements to limit this functionality to the GV-viewer (in web/app_options.js) since this feels "unnecessary" on desktop.

Also, is this intended as a temporary solution until issue #6419 is fully fixed?

@marco-c
Copy link
Contributor

marco-c commented Apr 3, 2025

We were thinking of three things:

  1. finish up Add logic to track rendering area of various PDF ops #19043;
  2. support drawing in a worker;
  3. Implement tiling of canvas into smaller pieces #6419 (which can then benefit from drawing in parallel given 2).

@calixteman calixteman force-pushed the reduce_canvas_size branch from 81317d5 to 4d32b10 Compare April 3, 2025 16:51
@calixteman calixteman changed the title Add a pref in order to cap the canvas dimensions to a factor of the window ones (bug 1958015) Add a pref in order to cap the canvas areato a factor of the window one (bug 1958015) Apr 3, 2025
@calixteman
Copy link
Contributor Author

It isn't only improving speed for GV but for Desktop too when the pdf is large.
For example, with wuppertal_2012.pdf it takes around 18s to render at 210% with this patch when it takes several minutes without.
Maybe we can try to compute a better value when creating the first canvas and based on the time used to draw everything.
Something else which could improve the rendering of large pdfs like maps, would be to not draw what it isn't in the detail view.
For example wherever an image is we rescale it, but it's outside the view then it's useless, the same when drawing a group (and if it's partially rendered we should set a clip box), the same when stroking where we've to compute some scale factors, ...
If we implement tiling we'd have to do that so it's probably a good idea to start asap.

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.

Since the pixel-limits now also depend on the window size, what happens if the viewer is resized such that it becomes bigger: will re-rendering be triggered in that case (since I believe that it should)?

@calixteman calixteman force-pushed the reduce_canvas_size branch from 58b4405 to 3e0ad5e Compare May 7, 2025 17:14
@calixteman
Copy link
Contributor Author

calixteman commented May 7, 2025

For information, I profiled wuppertal_2012.pdf at 300% on desktop with a local dev server (Windows 11) and with a local build of Fenix on a real pixel 7 phone.
Here are the results (time spent in executeOperatorList in seconds);

Cap factor Desktop Fenix
-1 5.8 5.7
0 3.3 3.3
100 4.3 3.7
200 5.0 4.2
300 5.7 4.4

@calixteman calixteman requested a review from Snuffleupagus May 7, 2025 20:02
@Snuffleupagus Snuffleupagus changed the title Add a pref in order to cap the canvas areato a factor of the window one (bug 1958015) Add a pref in order to cap the canvas area to a factor of the window one (bug 1958015) May 8, 2025
@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: 0

Live output at: http://54.241.84.105:8877/22654cc9d9f2c5c/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/9b2deb50b26653a/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/22654cc9d9f2c5c/output.txt

Total script time: 29.78 mins

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

@calixteman calixteman force-pushed the reduce_canvas_size branch from 3e0ad5e to 9d9395e Compare May 9, 2025 07:26
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.

There's a small typo in the commit message: areato -> area to.

r=me, with a couple more comments; thank you.

…one (bug 1958015)

This way it helps to reduce the overall canvas dimensions and make the rendering faster.
The drawback is that when scrolling, the page can be blurry in waiting for the rendering.

The default value is 200% on desktop and will be 100% for GeckoView.
@calixteman calixteman force-pushed the reduce_canvas_size branch from 9d9395e to 1225c1e Compare May 9, 2025 11:57
@calixteman
Copy link
Contributor Author

/botio integrationtest

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

Command cmd_integrationtest from @calixteman received. Current queue size: 2

Live output at: http://54.193.163.58:8877/7795ea9dcec4b7b/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.241.84.105:8877/4868b2ac35c344f/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Failed

Full output at http://54.241.84.105:8877/4868b2ac35c344f/output.txt

Total script time: 12.49 mins

  • Integration Tests: FAILED

@calixteman calixteman merged commit a806f00 into mozilla:master May 9, 2025
9 checks passed
@calixteman calixteman deleted the reduce_canvas_size branch May 9, 2025 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants