-
Notifications
You must be signed in to change notification settings - Fork 10.1k
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
Render high-res partial page views when falling back to CSS zoom (bug 1492303) #19128
base: master
Are you sure you want to change the base?
Conversation
2f715cb
to
5ad7743
Compare
4d80dcc
to
074c515
Compare
After discussing this patch with @calixteman, I updated it to prioritize rendering the full css-zoomed canvas rather than the high-resolution one. This means that, when zooming in, you'll first see the css-zoomed canvas, and then it will be replaced by the high-res one once it's ready. It's a slightly worse experience, however it guarantees that we do not regress in cases where the user starts moving around before that we are done rendering the background canvas (because if only the high-res canvas is there, they'll see just white until when the new high-res canvas is rendered). I also changed the logic that decides when to re-render the high-res canvas to not only do it once the user scrolls past its edges, but when the user is close to scrolling past the edges. |
074c515
to
b711cf5
Compare
69a98fc
to
3408061
Compare
Unfortunately this approach has a problem right now: since the various SVG paths for drawings are inserted inside the |
3408061
to
9c13462
Compare
Fixing the drawing problem required changing the new canvas to be in the existing |
9c13462
to
feea3e4
Compare
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.
Leaving a few more comments, mostly for the first patch since I've only begun looking at the Render high-res partial page views when falling back to CSS zoom
commit (and that one requires careful checking).
Also, is this new functionality anything that we could "easily" test with e.g. integration-tests?
feea3e4
to
ad7f77d
Compare
I'm working on adding some meaningful tests. |
From: Bot.io (Linux m4)FailedFull output at http://54.241.84.105:8877/01b208fd694bf18/output.txt Total script time: 11.77 mins
|
From: Bot.io (Windows)FailedFull output at http://54.193.163.58:8877/f2b8799df1036f9/output.txt Total script time: 26.62 mins
|
The tests for this PR now passed 🎉 There are some timeouts regarding the FreeText editor tests, and I'm getting flaky timeouts on them locally too (both on this branch, and on 8358ab6, which is the commit that this branch is based on). I'll try rebasing, since I see that there are new commits that touch those tests.
|
6c4f41a
to
cf84cac
Compare
This base class contains the generic logic for: - Creating a canvas and showing when appropriate - Rendering in the canvas - Keeping track of the rendering state
2e9c1c3
to
10a1d0b
Compare
10a1d0b
to
d4f14e0
Compare
I'm still working on a bug found by @calixteman about some race condition between scrolling and zooming: Sometimes, when quickly scrolling and zooming, we render the contents of the detail canvas with the right scale, but we use a previous scale to determine how large it should be. |
Does this PR fix issue #12957 as well? |
Half way. With the current release you can only see that PDF if the zoom is low, and with this PR you can also see if the zoom is high (when the detail view triggers). I still see it as blank at "medium" zoom level, probably because the canvas is too tall for Firefox but its area is too small for the css-only/detail-view logic to apply. A good fix would be to trigger the css-only/detail-view logic earlier, not just based on the canvas area but also on its width/height. I'd leave that issue open for now. |
The last commit should fix #19128 (comment). Unfortunately it is very racy behavior and I'm not able to write a test.
I have been scrolling and zooming around the Tracemonkey PDF for 5 minutes (with |
f6ed873
to
08f526f
Compare
I was playing with this on Firefox on Android, and noticed that the way this PR handled the detail canvas is quite annoying: when scrolling/zooming, the rendering gets cancelled very often and thus there is no new canvas to replace the old one. The last commit fixes it. Here is a video with the before/after (before is the before the page reload, after is after the page reload). Record_2025-01-23-12-26-52.mp4 |
… 1492303) When rendering big PDF pages at high zoom levels, we currently fall back to CSS zoom to avoid rendering canvases with too many pixels. This causes zoomed in PDF to look blurry, and the text to be potentially unreadable. This commit adds support for rendering _part_ of a page (called `PDFPageDetailView` in the code), so that we can render portion of a page in a smaller canvas without hiting the maximun canvas size limit. Specifically, we render an area of that page that is slightly larger than the area that is visible on the screen (100% larger in each direction, unless we have to limit it due to the maximum canvas size). As the user scrolls around the page, we re-render a new area centered around what is currently visible.
When scrolling quickly, the constant re-rendering of the detail view significantly affects rendering performance, causing Firefox to not render even the _background canvas_, which is just a static canvas not being re-drawn by JavaScript. This commit changes the viewer to only render the detail view while scrolling if its rendering hasn't just been cancelled. This means that: - when the user is scrolling slowly, we have enough time to render the detail view before that we need to change its area, so the user always sees the full screen as high resolution. - when the user is scrolling quickly, as soon as we have to cancel a rendering we just give up, and the user will see the lower resolution canvas. When then the user stops scrolling, we render the detail view for the new visible area.
7f0cf9c
to
29be17c
Compare
…om (bug 1492303) Keep the `.detailView` object when preserving the detail canvas
29be17c
to
f413386
Compare
// finishing rendering the new detail before they get past the rendered | ||
// area. | ||
|
||
const MOVEMENT_TRESHOLD = 0.5; |
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.
nit: -TRESHOLD+THRESHOLD
if ( | ||
this.#scrollTimeoutId !== null && | ||
pageView instanceof PDFPageDetailView && | ||
pageView.renderingCancelled | ||
) { | ||
// If we are scrolling and the rendering of the detail view was just | ||
// cancelled, it's because the user is scrolling too quickly and so | ||
// we constantly need to re-render a different area. | ||
// Don't attempt to re-rendering it: this will be done once the user | ||
// stops scrolling. | ||
return true; | ||
} |
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 don't know how much it matters in practice, but won't returning here without trigger rendering effectively stop any possible pre-rendering of other currently invisible pages?
Until either another page becomes more visible or scrolling stops, it seems that the getHighestPriority
-method will now always break here.
Would it make (any) sense to e.g. set the detailView-renderingState to FINISHED
when renderingCancelled === true
such that other pre-rendering can still run?
Commit:
To play around with this patch, you need to use a large PDF (the one I'm using is https://www.gtt.to.it/cms/risorse/urbana/mappa/mapparete.pdf) and zoom in: in the current release the text becomes blurry, while with this patch it remains sharp. Also, if you set
maxCanvasPixels
to a smaller value (for example, 8M) you'll more clearly see the effects also at lower zoom levels. Note however that ifmaxCanvasPixels
is set to less than 9 time the number of pixels visible in your window you'll get more frequent re-renders as you scroll.The
PDFPageDetailView
class still process every single operation of the PDF, even if many of them will actually be outside of the canvas and thus not be actually rendered. This is acceptable because we are only rendering one "detail view" per page at a time, and we are not splitting the page in multiple tiles that might appear at the same time on the screen. In the future however #19043 could improve the performance of this patch, and it would enable (for example) tiling a page into multiple detail views when printing it.There are some very minor TODOs that I left in the code, for some areas I need review feedback for. Also, I still need to find the best way to write tests.
Fixes #14147
Before:
After:
https://www.gtt.to.it/cms/risorse/urbana/mappa/mapparete.pdf at 400% zoom
Before:
After:
Fixes #14193