Jittery Zooming using Pixel2 + WebRender
Categories
(Core :: Graphics: WebRender, defect, P3)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox72 | --- | fixed |
People
(Reporter: jbonisteel, Assigned: lsalzman)
References
Details
Attachments
(3 files)
STR:
Look at: https://news.ycombinator.com/item?id=21135424
Zoom in and out.
The text appears to jitter as you zoom in and out (using the Pixel2, Fenix Nightly)
Compared the same page in Chrome and it does not appear to jitter
|
Reporter | |
Updated•6 years ago
|
|
||
Comment 1•6 years ago
|
||
I think this is due to the snapping changing as we zoom. Maybe Andrew or Lee might know something.
Lee, could your subpixel glyph positioning work help with this, or is that completely unrelated?
|
|
Reporter | |
Comment 2•6 years ago
|
||
NI for Andrew to get thoughts re: snapping
|
||
Comment 3•6 years ago
|
||
You can sort of reproduce this on desktop by setting apz.allow_zooming to true. Here's a profile from desktop:
https://perfht.ml/30TaHb6. It looks like we're doing too much glyph rasterization.
|
|
||
Comment 4•6 years ago
|
||
I originally misunderstood this bug as meaning individual glyphs jumping around, which is something I've noticed, rather than the zooming itself being jittery due to performance. I filed bug 1587441 for the other bug.
Yes, it seems on pages with lots of text our zooming performance can be bad. It seems to have gotten worse recently, and mozregression points to bug 1583707. Which seems to make sense as it has presumably increased the amount of glyph rasterization.
|
|
||
Comment 5•6 years ago
|
||
Here are some initial thoughts:
- I'm not sure why subpixel positioning has made an impact - I can't see an increase in the number of glyphs being rasterized. I don't think I understand what subpixel positioning actual entails yet.
- We are still correctly only rasterizing glyphs at certain zoom levels.
- We appear to rasterize slightly offscreen glyphs, however - how do we decide which glyphs are required? is there a display port type thing? or do we do all glyphs in a text run even if only a tiny part of the run is onscreen?
- I can't get a symbolicated profile for some reason, but I can see a lot of our time is in TextureUploader::upload(). This is uploading the data from a CPU buffer in to a PBO. It might be worth looking in to rasterizing directly in to a mapped PBO.
- Does picture caching make sense whilst async-zooming? If I turn on the texture cache view I can see the picture cache tiles changing every frame whilst zooming, which seems wasteful. Note however that at present disabling picture-caching prevents the only-rasterize-at-certain-zooms trick from working (bug 1566069)
|
|
||
Comment 6•6 years ago
|
||
We appear to rasterize slightly offscreen glyphs, however - how do we decide which glyphs are required? is there a display port type thing? or do we do all glyphs in a text run even if only a tiny part of the run is onscreen?
I presume this is because we render a "display port" of picture cache tiles.
|
|
||
Comment 7•6 years ago
|
||
Profiler symbols were fixed in bug 1588086.
So there seems to be 2 ways to approach this problem
a) make texture upload faster - I will see if I can tackle this in another bug.
b) stop us re-rasterizing and therefore uploading too many glyphs - I will deal with that here.
The raster space trick to prevent us rasterizing too many glyphs is working on this page until we reach RasterSpace(8). At this scale the glyphs are very large and therefore taking up lots of memory. And because of subpixel positioning there are possibly 4x as many. So we are now hitting the max glyph cache size, and pruning glyphs. Unfortunately we are pruning glyphs that are still in use, so we therefore need to re-rasterize and re-upload them for the next frame. and the next frame, etc. This cache limit was introduced in bug 1560520.
The limit is currently set at 6MB. I think this is too low, but I certainly don't want to reintroduce the bug which this limit fixed, so we probably need to tread carefully. It probably makes sense not to purge glyphs which were used in the previous frame, if that is possible. And it also seems we sort the GlyphKeyCaches to try to prune the least-recently-used first, but we don't sort the GlyphKeys themselves within those caches.
Lee, what do you think is the best course of action here?
|
Assignee | |
Comment 9•6 years ago
•
|
||
So there are probably two changes we want to make here:
-
implement a size threshold above which we disable subpixel positioning. If we're already enforcing a font-size limit in WebRender anyway, the glyphs are going to look like mush with subpixel positioning on because of it. Normally when non-WR Skia rendering hits its own internal font size limit, it renders the glyph as a path and disables subpixel positioning for us automagically. So it makes sense to just do this ahead of time down in Gecko when WR is used.
-
we might want to implement some sort of 1-frame lag in pruning glyphs to allow them to not get dumped if we need them that frame.
|
|
Assignee | |
Comment 10•6 years ago
|
||
|
|
Assignee | |
Comment 11•6 years ago
|
||
Jamie, can you see if the patch I put up alleviates the issue?
|
|
||
Comment 12•6 years ago
|
||
This definitely helps! It seems to roughly half the amount of texture upload per frame for me in terms of bytes. And it seems to reduce the upload time from around 40% to around 25% of renderer time.
I'd be interested to see how much further your option 1) improves things too!
|
|
||
Comment 13•6 years ago
|
||
Done some more testing. The patch helps most when zooming in slowly (staying within the same raster scale), or scrolling while zoomed. Because in these cases, most of the glyphs required for a frame were present in the last frame. So by ensuring we don't prune glyphs that are still in use we get a big win.
It doesn't, however, help much when repeatedly zooming in and out large amounts (causing us to jump between raster scales). The reason being that when we change raster scales the glyphs from the previous raster scale will no longer be used, so will be pruned. Then when we zoom back to the original raster scale we need those glpyhs again, so must rerasterize them. I'm not sure how important this is in the real world, but it's very noticable when repeatedly zooming in and out. Increasing the glyph cache limit to say 20MB makes a huge improvement. I know we'd rather avoid doing that, but hopefully Lee's plans to disable subpixel positioning above a threshold, and reduce the max glyph size, will help keep us under the current limit so have the same effect.
|
|
Assignee | |
Comment 14•6 years ago
|
||
Depends on D51340
|
|
Assignee | |
Comment 15•6 years ago
|
||
Depends on D51746
|
||
Updated•6 years ago
|
|
||
Comment 16•6 years ago
|
||
|
||
Comment 17•6 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/e517398d7ab9
https://hg.mozilla.org/mozilla-central/rev/3479e57e405a
https://hg.mozilla.org/mozilla-central/rev/613cdbb81116
|
||
Updated•6 years ago
|
|
||
Comment 18•6 years ago
|
||
Hi, I wanted to reproduce your issue with Firefox Nightly 68.2a1 (2019-10-02) and check the fix on Firefox Preview Nightly , with a Google Pixel 3a(Android 9) and I enabled the WebRenderer through about:config->gfx.webrender.enabled.
Pre-requisites:
- https://news.ycombinator.com/item?id=21135424
Scenario with which I tried to reproduce: - slowly zooming in and out on the page
- repeatedly zooming in and out
- Please view the attached video.
And checked on Firefox Preview Nightly 191205 (Build # 13391806) and GeckoView 72.0a1-20191202091209 but to me seems to be mostly the same: - Please view the attached video.
For Firefox Preview Nightly application is this the intended outcome? Are there any other preference to set?
Note:
- On Chrome I tried to reproduce this but it is more stable, no jittery present when zooming in or out, nor when scrolling on a zoomed page.
|
|
||
Comment 19•6 years ago
|
||
Hi Diana, thanks for testing!
Unfortunately the patches on this bug didn't do much by themselves to improve things, but did lay some important groundwork. So your results are to be expected. Bug 1598380 should make a noticable difference, so it will be worthwhile testing again when that has landed.
Description
•