1642308 - Full screen picture-cache invalidation occurs whenever a new display list is received when scrolling whilst zoomed

Status: RESOLVED FIXED

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

Description

[Jamie Nicol [:jnicol]]

Scrolling while zoomed used to cause full screen picture cache invalidation on every frame. This was because due to the zoom != 1.0, the async scroll offset was changing by a non-whole number every frame. This meant the subpixel position of the picture cache tiles kept changing, causing them to be invalidated. In bug 1589669 we ensured that the scroll offset was snapped, then in bug 1620014, we did the same for the visual viewport offset. With both these methods of async scrolling snapped (the scroll offset and visual viewport offset) the subpixel position remains constant between frames, meaning we do not invalidate.

However, as noted in bug 1620014 comment 16, there is still more picture cache invalidation than desired. Rather than occur on every frame, however, it now only occurs whenever a new display list is received. This might be every 128 or 256 pixels, depending on the platform. My understanding is that while we are now snapping the async scroll offsets, the main-thread scroll offset is not snapped. Each item in the new display list will therefore have moved by a fractional amount, causing everything to be invalidated.

Currently this is probably only an issue on android, but when desktop zooming is enabled it will be an issue on desktop too.

Comment 1

[Jamie Nicol [:jnicol]]

Probably a naive question but what is the benefit of scrolling in fractional amounts in the first place? I feel like if we only scrolled in whole pixels that would solve this issue (and would make snapping the visual viewport offset in webrender unnecessary, which would be nice as I keep causing new regressions with each attempt to fix that!)

Comment 2

[Botond Ballo [:botond]]

(In reply to Jamie Nicol [:jnicol] from comment #0)

However, as noted in bug 1620014 comment 16, there is still more picture cache invalidation than desired. Rather than occur on every frame, however, it now only occurs whenever a new display list is received. This might be every 128 or 256 pixels, depending on the platform. My understanding is that while we are now snapping the async scroll offsets, the main-thread scroll offset is not snapped. Each item in the new display list will therefore have moved by a fractional amount, causing everything to be invalidated.

My understanding is that main thread does at least make an attempt to snap its scroll offset: https://searchfox.org/mozilla-central/rev/5e4d4827aa005d031580d2d17a01bae1af138b2e/layout/generic/nsGfxScrollFrame.cpp#2774

Comment 3

[Botond Ballo [:botond]]

(In reply to Jamie Nicol [:jnicol] from comment #1)

Probably a naive question but what is the benefit of scrolling in fractional amounts in the first place? I feel like if we only scrolled in whole pixels that would solve this issue (and would make snapping the visual viewport offset in webrender unnecessary, which would be nice as I keep causing new regressions with each attempt to fix that!)

There are probably other reasons as well, but at least one reason these days is that the web specs are moving in the direction of the web-exposed scroll offsets being fractional (see Rendering Independent Scroll Offsets).

Comment 4

[Botond Ballo [:botond]]

It's also not entirely clear to me what "only scrolling in whole pixels" would mean. If it meant "only whole CSS pixels", the scroll offset in screen pixels could still be fractional when zoomed. If it meant "only whole screen pixels", then the scroll offset in CSS pixels would have to change (to a different fractional value) when zooming to yield a whole screen pixel at the current zoom level.

Comment 5

[Jamie Nicol [:jnicol]]

Thanks Botond. "Scrolling in whole pixels" probably isn't the answer, but I'm not sure what is.

I've refreshed my memory a bit more on what the problem is in webrender. It's wrong to say "the subpixel position of picture cache tiles kept changing", it's more that the subpixel position of all the glyphs changes:

When we are checking for invalidation of picture cache tiles, every TextRun gets marked invalid because the prim UIDs do not match the previous frame's. A new prim UID means the interner lookup failed and a new prim was created, and the interner lookup failed because the fractional position of each GlyphInstance in the TextRunKey has changed. The position is set here, derived from the spatial node's (unsnapped) external_scroll_offset and the (snapped?) prim_info.rect.

So each new display list updates the external scroll offset to a new fractional value, which changes every glyph position, which causes (almost) full screen invalidation (only the tiles containing text runs).

Andrew, do you have any ideas about how to solve this?

Comment 6

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

Hmmmm. The glyph position is supposed to be stable during animations. For normal animations, we do this:

https://searchfox.org/mozilla-central/rev/559b25eb41c1cbffcb90a34e008b8288312fcd25/layout/painting/nsDisplayList.cpp#8008

to request we rasterize the glyphs in local space during the animation. Doing the same thing for the zoom animation would probably help, given from what you linked to me before, it follows a different path:

https://searchfox.org/mozilla-central/rev/559b25eb41c1cbffcb90a34e008b8288312fcd25/layout/painting/nsDisplayList.cpp#6352

To be clear, we only use local raster space while the animation is happened -- before it starts and after it is complete, we must switch away from local raster space.

Comment 7

[Kris Taeleman (:ktaeleman)]

@jamie: desktop zooming is default enabled on 81 now, are you seeing this issue on desktop and how bad is it?

Comment 8

[Jamie Nicol [:jnicol]]

I can reproduce on desktop very easily. If you enable gfx.webrender.debug.picture-caching, zoom in slightly to any page, and then scroll continuously, you will see the entire screen flash red pretty frequently.

On simple pages my laptop is powerful enough to render the entire screen fairly quickly, so I wouldn't notice anything without the picture caching debug overlay open. But on more complex pages or less powerful machines it might be noticeable.

Comment 9

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

Attachment: Bug 1642308 - Fix fract offset calculations for picture cache tile invalidation.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Assigning to Andrew per discussion in #gfx. The patch above apparently helps with some pages but not all so there's some more work needed.

Comment 11

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

I'm not sure this will solve all of the invalidation concerns, but I think it will solve the ones related to fractional offsets and snapping highlighted in this bug.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/37708d37f554
Fix some snapping related picture cache tile rect calculations. r=gw

Comment 13

[Narcis Beleuzu [:NarcisB]]

Backed out for dt failures on browser_aboutdebugging_persist_connection.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/9e61e67ae323fc6b32a5a8a0a3e367899a5b0afd
Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=317238591&repo=autoland&lineNumber=2782

Comment 14

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

(In reply to Narcis Beleuzu [:NarcisB] from comment #13)

Backed out for dt failures on browser_aboutdebugging_persist_connection.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/9e61e67ae323fc6b32a5a8a0a3e367899a5b0afd
Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=317238591&repo=autoland&lineNumber=2782

This code doesn't run on that (non-WR) target.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1b7bc7ee74b4
Fix some snapping related picture cache tile rect calculations. r=gw

Comment 16

[Dorel Luca [:dluca]]

Backed out changeset 1b7bc7ee74b4 (bug 1642308) for Wd failures in webdriver/tests/find_elements/find.py. CLOSED TREE

Log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=317317571&repo=autoland&lineNumber=12694

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&revision=8cee8448e4d28cb7dc58fe963f836e97c0e8c616

Backout:
https://hg.mozilla.org/integration/autoland/rev/cd6a0390baa7286082df981c1d644bef6a346088

Comment 17

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

This is troubling in that these tests should be running without WR but the logs suggest that may not be the case....

Comment 18

[Kartikaya Gupta (email:[e-mail])]

This seems wrong: https://searchfox.org/mozilla-central/rev/7ef5cefd0468b8f509efe38e0212de2398f4c8b3/gfx/config/gfxConfigManager.cpp#237

The test harness should be setting MOZ_WEBRENDER=0 to force-disable WR, but we're apparently only respecting it in some cases?

Comment 19

[Kartikaya Gupta (email:[e-mail])]

Previously we respected it except if we were in the marionette test that specifically exercises the rollout code: https://hg.mozilla.org/mozilla-central/rev/7ba7a04cfa43596cdd87041c50563adc55445dd3#l10.442

Comment 20

[Kris Taeleman (:ktaeleman)]

Andrew: Are you still working on this, we have a dependency on this for DTZ

Comment 21

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

I can't land without the (unrelated) assert in the test case getting fixed.....

Comment 22

[Glenn Watson [:gw]]

Attachment: 0001-WIP-Move-device-valid-rect-to-descriptor.patch

I think I found the cause of the assert, although I couldn't repro locally.

The way that tiles get recycled between scenes with new display lists can make storing prev_device_valid_rect in Tile sometimes be incorrect. I moved it to be in the TileDescriptor struct and try runs seem to pass OK with that. The TileDescriptor struct can be relied on to correctly store information from a previous frame for invalidation purposes.

Pending try with attached patch is https://treeherder.mozilla.org/#/jobs?repo=try&revision=992c653dbc5678b8fbe3f2f6c0de3dbf3967cb75

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5303394c527b
Fix some snapping related picture cache tile rect calculations. r=gw

Comment 24

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/5303394c527b

Comment 25

[Sandor Molnar[:smolnar]]

Backed out for causing crashes on OSX (Bug 1658986)

Backout link: https://hg.mozilla.org/mozilla-central/rev/7b8c743766da14e917085a9bb61e6ed1eed4d5bb

We see this crashes in CI

https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&resultStatus=pending%2Crunning%2Ctestfailed%2Cbusted%2Cexception%2Crunnable&searchStr=osx&revision=0058e174c5d0a4f751398b5aefcce42adc511535&selectedTaskRun=OAXu_w0bSaCvoZ3JmPcy0g.0

Comment 26

[Kartikaya Gupta (email:[e-mail])]

If I'm following things correctly, gw fixed the assertion problem in bug 1672077, so this can reland now?

Comment 27

[Kartikaya Gupta (email:[e-mail])]

I'm also assuming for now that we're not going to try to uplift this (and dependencies) to Fx83.

Comment 28

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

try after rebase/sorting conflicts: https://treeherder.mozilla.org/#/jobs?repo=try&revision=671bea54f464712381039e81042409eb4df902da

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/404ed30949a4
Fix some snapping related picture cache tile rect calculations. r=gw

Comment 30

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/404ed30949a4

Comment 31

[Cristian Brindusan [:cbrindusan]]

Backed out changeset 404ed30949a4 (Bug 1642308) for causing (Bug 1674480).
https://hg.mozilla.org/mozilla-central/rev/5df33161b72469d00b912d04dc4f94f4dc468f11

Comment 32

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

jrmuizel tested the two different parts of this patch and confirmed the fractional offset piece has no regression, and the valid rect piece does. I've filed bug 1675030 to track the valid rect piece of this patch and will reland the fractional offset piece.

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/af9de2112747
Fix fract offset calculations for picture cache tile invalidation. r=gw

Comment 34

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/af9de2112747