Closed Bug 1585713 Opened 9 months ago Closed 8 months ago

Jittery Zooming using Pixel2 + WebRender

Categories

(Core :: Graphics: WebRender, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla72
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

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?

Flags: needinfo?(lsalzman)

NI for Andrew to get thoughts re: snapping

Flags: needinfo?(aosmond)

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.

Flags: needinfo?(aosmond)
See Also: → 1587441

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.

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)

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.

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?

Flags: needinfo?(lsalzman)

Re-needinfoing Lee

Flags: needinfo?(lsalzman)
See Also: → 1589666

So there are probably two changes we want to make here:

  1. 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.

  2. 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.

Flags: needinfo?(lsalzman)

Jamie, can you see if the patch I put up alleviates the issue?

Flags: needinfo?(jnicol)

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!

Flags: needinfo?(jnicol)

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.

Attachment #9106405 - Attachment description: Bug 1585713 - reduce WR font size limit from 512 to 384. r?jnicol → Bug 1585713 - reduce WR font size limit from 512 to 320. r?jnicol
Pushed by lsalzman@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e517398d7ab9
don't prune WR glyphs that were recently used. r=jnicol
https://hg.mozilla.org/integration/autoland/rev/3479e57e405a
disable subpixel positioning for oversized WR fonts. r=jnicol
https://hg.mozilla.org/integration/autoland/rev/613cdbb81116
reduce WR font size limit from 512 to 320. r=jnicol
Status: NEW → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla72
Assignee: nobody → lsalzman

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:

  • 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.
Flags: needinfo?(jnicol)

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.

Flags: needinfo?(jnicol)
You need to log in before you can comment on or make changes to this bug.