1620014 - Picture caching doesn't work on http://www.jagregory.com/abrash-black-book/ on Android

Status: RESOLVED FIXED

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

Description

[Jeff Muizelaar [:jrmuizel]]

When I'm zoomed out all the way I don't seem to get any picture caching.
I also see differences in text snapping while scrolling. I suspect these are related and that the problem might be in Gecko instead of WebRender

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Bert, what to see if you can figure out why WebRender thinks things are changing?

Comment 2

[Jeff Muizelaar [:jrmuizel]]

I can reproduce something similar on desktop by enabling apz.allow_zooming and zooming in a bit on that page

Comment 3

[Jeff Muizelaar [:jrmuizel]]

I think you'll also need mousewheel.with_control.action=5 on Windows

Comment 4

[Bert Peers [:bpeers]]

Hm, I'm not seeing anything suspicious with allow_zooming and action=5 on Windows (+Nvidia).
By holding Ctrl and using the mouse wheel I can "zoom in" on the mouse location, like a pinch zoom. So that seems to work.
For sure the entire picture-caching goes red but wobbling over the UI a bit to redraw the window makes everything green.
(Nothing unusual either when zooming out using ctrl- ).
Sorry if I misunderstood the repro steps or the symptom? Can you clarify a bit?

Comment 5

[Bert Peers [:bpeers]]

Glenn's comment & patch on Bug 1620147 reminded me of, and is very similar to, the constant rebasing of the origin that happens in open world games: simply shading, animating, colliding things as-is runs into floating point precision issues when thousands of units away from the origin. (Example)

If caching and/or snapping relies on consistent fractional values under scrolling, the values dealt with might become semi-random as all numerical precision is eaten up by the offset. I don't know how our coordinate system works so I might be way off (bad pun intended). We might need a moving virtual origin, unless it's too late for us to salvage -- then we'd need it on the Layout side.

Anyway, just a guess.

Comment 6

[Jamie Nicol [:jnicol] (on leave until 21st October)]

I believe this is very similar to bug 1589669.

The picture cache tiles are invalidated because the some prim origins don't match. The origins don't match because they were clamped to be inside the local_tile_rect meaning they move as ctx.fract_offset changes. And the fract offset does keep changing as we scroll. The fract offset come's from the picture's spatial node's transform.

In bug 1589669, I think the problem was that we were snapping the scroll offset in local space rather than device space. Changing snap_offset() to include the zoom, so that we snap in device space, fixed the issue.

Since then, however, bug 1609002 has landed. Now, in addition to the layout viewport offset (which is our scroll offset that we snap correctly) there is also a visual viewport offset, which is included in the animated transform property used for the zoom. The visual viewport offset does not get snapped, so the fractional offset varies as we scroll, so we constantly invalidate picture cache tiles.

I believe this affects any page after zooming in. But the reported URL happens to have a visual zoom by default, so it reproduces without manaully zooming. If there is no visual zoom there can be no visual offset, so we can't reproduce the bug.

(Also interesting is that zooming with your finger stuck to the screen avoids the bug, but "flinging" reproduces it. I guess whilst your finger is touching the screen, the scroll offset changes in 1 pixel increments, but when flinging it follows some momentum/friction equation so that is not the case. I don't think this is relevant to the bug though.)

I'm pretty sure we just want to snap the visual viewport offset in order to fix this, but I'm not sure where is the right place to do that. We sample the transform animation in SpatialNode::update_transform() for the parent node. This then gets passed to the child's update_transform() as state.coordinate_system_relative_scale_offset. This is where we snap the scroll offset when calculating self.content_transform. I guess we should snap the visual offset at the exact same place. Any thoughts, Andrew?

Comment 7

[Andrew Osmond [:aosmond] (he/him)]

Re comment 6, I agree that approach sounds correct. You can't snap without the animated transform like the primitives do, but the scroll offset should not affect the size of the primitives in any meaningful way (if anything, it might help reduce drawing artifacts). Would r+ :).

Comment 8

[Jamie Nicol [:jnicol] (on leave until 21st October)]

Attachment: Bug 1620014 - Snap animated transform offsets in webrender. r?aosmond

In bug 1589669 we ensured that webrender snaps the asynchronous scroll
offset correctly. Prior to that, the fractional offsets of picture
cache tiles were changing every frame, leading to excessive picture
cache invalidation. Since then, however, we have landed bug 1609002,
which made it so that asynchronous scrolling is split in to the scroll
offset and the visual viewport transform. The visual viewport
transform is sent to webrender as an animated transform property, and
since this was not being snapped it regressed the picture caching
invalidation fix. This change ensures that offset is snapped
correctly.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1ba2c8fbef07
Snap animated transform offsets in webrender. r=aosmond

Comment 10

[Jamie Nicol [:jnicol] (on leave until 21st October)]

The above patch fixes the invalidation whilst scrolling. When the scroll stops, however, we usually get a full-screen invalidation. Most of the tiles get invalidated because the prim UIDs don't match. And the prim UIDs don't match because when interning the data didn't match. Typically for a TextRunKey because the fractional offset of the GyphInstances have all changed. eg point: (1.0, 28.266602) before and during the scroll, and point: (1.0, 28.233398) immediately after.

I can easily believe that's expected, and perhaps even desirable for correctness? But wanted to confirm that is indeed the case. Does that make sense to you Andrew, or Glenn?

Comment 11

[Glenn Watson [:gw]]

Yea, it's probably expected, at least for now.

We should aim to do better here, in future (it's a bit tricky to get right without causing correctness issues in some cases, and naive quantizing also shows up in profiles, so it needs a bit more though to solve properly).

Comment 12

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/1ba2c8fbef07

Comment 13

[Jamie Nicol [:jnicol] (on leave until 21st October)]

Comment on attachment 9142333 [details]
Bug 1620014 - Snap animated transform offsets in webrender. r?aosmond

Beta/Release Uplift Approval Request

• User impact if declined: Sluggish scrolling on webrender on android. A regression from bug 1609002.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Small change. Only affects Android, because desktop platforms do not have pinch zooming enabled.
• String changes made/needed:

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9142333 [details]
Bug 1620014 - Snap animated transform offsets in webrender. r?aosmond

Fixes an Android-only WebRender perf issue, which is being enabled by default for Pixel 2 & 3 devices in GV76. Approved for 76.0rc1.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/add6869392f9

Comment 16

[Diana Rus]

Hi, did a check with Google Pixel 3 XL (Android 9) on Firefox Preview Nightly 200505 (Build #21260606) GV: 77.0a1-20200503093615 AS: 0.58.2

• it still seems that there is much more invalidation when scrolling than it should be, will leave the video here

Reproduced the issue on a geckview example apk from https://hg.mozilla.org/mozilla-central/rev/72e0cabfec72

Comment 17

[Jamie Nicol [:jnicol] (on leave until 21st October)]

(In reply to Diana Rus from comment #16)

Hi, did a check with Google Pixel 3 XL (Android 9) on Firefox Preview Nightly 200505 (Build #21260606) GV: 77.0a1-20200503093615 AS: 0.58.2

• it still seems that there is much more invalidation when scrolling than it should be, will leave the video here

Reproduced the issue on a geckview example apk from https://hg.mozilla.org/mozilla-central/rev/72e0cabfec72

I have filed bug 1642308 for the follow-up issue. Thanks!