Closed Bug 1589666 Opened 9 months ago Closed 8 months ago

Disable picture caching when async zooming

Categories

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

Unspecified
Android
defect

Tracking

()

RESOLVED FIXED
mozilla72
Tracking Status
firefox72 --- fixed

People

(Reporter: jnicol, Assigned: jnicol)

References

Details

Attachments

(1 file)

When async zooming, picture caching isn't much use because every picture cache tile will be fully invalidated every frame. Zooming can on some frames require a lot of texture upload (due to re-rasterizing glyphs in a new raster space), so rendering in to a picture cache tile, then blitting that tile to the screen, seems like a bad use of precious memory bandwidth.

Note that currently disabling picture caching (by setting gfx.webrender.picture-caching to false) makes async zoom much slower because it breaks the raster space trick and we rasterize glyphs every frame (bug 1566069). Perhaps the solution here would "turn off" picture caching at a much lower level so that won't be an issue, or perhaps that needs fixed. Something to be aware of anyway.

Glenn, could you point me towards where the best place to "turn off" picture caching would be?

Glenn, can you help with the above? ^

Flags: needinfo?(gwatson)

If feasible, the ideal place to disable picture caching when zooming is:

https://searchfox.org/mozilla-central/source/gfx/wr/webrender/src/picture.rs#3636

This is near the start of each frame where we determine from the 'requested' composite mode of a picture what the 'actual' composite mode of the picture is for this frame (so we can dynamically enable/disable picture caching at this point). We already use this to disable picture caching in the case where the transform of the picture cache root has a perspective transform.

One caveat - it's not common that we dynamically switch from picture caching on to off and vice versa, so this code path may not be that well tested / buggy. If you do encounter weird glitches between enabling and disabling picture caching, we should fix them!

Flags: needinfo?(gwatson)

For unrelated reasons, I was just testing something out with this code path today and realized it is broken - so I guess nothing currently relies on that working. I'll try to investigate it further tomorrow.

I think, instead, the best option will be to change the enable_picture_caching field in both Renderer and FrameBuilderConfig.

You'll have to plumb some code to do this (probably via setting the value in the FrameBuilderConfig based on whether zoom is enabled, and propagating that to the renderer in the Frame struct). It's probably also necessary to set the picture cache composite above (mentioned in the comments above) based on the value in FrameBuilderConfig.

I haven't tested that at all, but from a quick look at the code, that would be enough to allow you to dynamically enable / disable picture caching per-frame. I can probably take a look at implementing this tomorrow, unless you want to look at implementing that yourself?

Depends on: 1592810

Is there anything left we need do here or was what we landed in 1592810 enough?

Flags: needinfo?(jnicol)

I believe I still need to hook the zooming code up to Glenn's changes. Will try to get round to that tomorrow!

Flags: needinfo?(jnicol)

Whilst pinch zooming, every picture cache tile gets completely
invalidated every frame. It is therefore a waste of memory bandwidth
to render in to picture cache tiles then composite those to the
screen. This change dynamically disables picture caching for frames in
which we are pinch zooming. The exception is if we are using a native
compositor, in which case picture caching will remain enabled, because
it relies on picture caching to work, and does not waste memory
bandwidth.

Pushed by jnicol@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dd0e1c0d6acc
Disable picture caching whilst pinch-zooming. r=gw
Status: NEW → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla72
You need to log in before you can comment on or make changes to this bug.