1595909 - Image masks with different keys being added during scrolling

Status: NEW

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

Description

[Glenn Watson [:gw]]

Attachment: 0001-WIP.patch

On some pages, I see a lot of picture cache tiles being invalidated when there is no apparent visual change.

One of the causes is that clips are being interned with different IDs because they are being sent in new display lists with different image keys for the mask data.

STR:

1. Apply the attached patch - this logs out information every time a clip is interned.
2. Open https://reddit.com/r/rust
3. Wait for page load
4. Scroll down the page
5. Observe logs similar to below, where the same clip is being interned but with a different rasterized image key for the mask.

An example of logging for one clip (each interning of it occurs on subsequent frames):

ItemUid { uid: 1421 }: ClipItemKey { kind: ImageMask(RectangleKey { x: 513.0, y: 4972.0, w: 577.0, h: 261.0 }, ImageKey(IdNamespace(3), 159), false), spatial_node_index: SpatialNodeIndex(5) }

ItemUid { uid: 1434 }: ClipItemKey { kind: ImageMask(RectangleKey { x: 513.0, y: 4972.0, w: 577.0, h: 261.0 }, ImageKey(IdNamespace(3), 166), false), spatial_node_index: SpatialNodeIndex(5) }

ItemUid { uid: 1453 }: ClipItemKey { kind: ImageMask(RectangleKey { x: 513.0, y: 4972.0, w: 577.0, h: 261.0 }, ImageKey(IdNamespace(3), 173), false), spatial_node_index: SpatialNodeIndex(5) }

In each case, everything about the clip node is the same except for the image key, which WR interprets as new mask data, causing the tile to invalidate.

Comment 1

[Glenn Watson [:gw]]

Each time a frame occurs where the clip masks are re-interned, 7 clip masks get updated:

ItemUid { uid: 1447 }: ClipItemKey { kind: ImageMask(RectangleKey { x: 513.0, y: 371.0, w: 582.0, h: 261.0 }, ImageKey(IdNamespace(3), 167), false), spatial_node_index: SpatialNodeIndex(5) }
ItemUid { uid: 1448 }: ClipItemKey { kind: ImageMask(RectangleKey { x: 513.0, y: 746.0, w: 537.0, h: 22.0 }, ImageKey(IdNamespace(3), 168), false), spatial_node_index: SpatialNodeIndex(5) }
ItemUid { uid: 1449 }: ClipItemKey { kind: ImageMask(RectangleKey { x: 513.0, y: 2463.0, w: 582.0, h: 250.0 }, ImageKey(IdNamespace(3), 169), false), spatial_node_index: SpatialNodeIndex(5) }
ItemUid { uid: 1450 }: ClipItemKey { kind: ImageMask(RectangleKey { x: 513.0, y: 2834.0, w: 582.0, h: 261.0 }, ImageKey(IdNamespace(3), 170), false), spatial_node_index: SpatialNodeIndex(5) }
ItemUid { uid: 1451 }: ClipItemKey { kind: ImageMask(RectangleKey { x: 513.0, y: 4004.0, w: 582.0, h: 261.0 }, ImageKey(IdNamespace(3), 171), false), spatial_node_index: SpatialNodeIndex(5) }
ItemUid { uid: 1452 }: ClipItemKey { kind: ImageMask(RectangleKey { x: 513.0, y: 4375.0, w: 582.0, h: 158.0 }, ImageKey(IdNamespace(3), 172), false), spatial_node_index: SpatialNodeIndex(5) }
ItemUid { uid: 1453 }: ClipItemKey { kind: ImageMask(RectangleKey { x: 513.0, y: 4972.0, w: 577.0, h: 261.0 }, ImageKey(IdNamespace(3), 173), false), spatial_node_index: SpatialNodeIndex(5) }

Comment 2

[Jessie [:jbonisteel] pls NI]

Nical, any suggestions about this?

Comment 3

[Nicolas Silva [:nical]]

Two things:

• (1) the image mask logic in WebRenderCommandBuilder.cpp doesn't have a code path to update the existing image key, which is inefficient for the resource cache (the atlas allocator has more work to do, and we end up keeping several versions of the mask in the cache if it doesn't get evicted right away. Also if only a portion of the mask changes we could use the dirty rect to upload less, but we miss this opportunity here). That doesn't explain why the masks are invalidated in the first place.
• (2) The mask is generated in WebRenderCommandBuilder::BuildWrMaskImage( which contains a branch that will invalidate under certain conditions. The one we are hitting on this reddit page is !itemRect.IsEqualInterior(maskData->mItemRect). I don't know this code (Jeff, halp!) so it's unclear why the position matters. I tried to replace it with itemRect.Size() != maskData->mItemRect.Size(), it doesn't seem to break the page and lets us invalidate a lot less, but that might not be correct.

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Changing this itemRect.Size() != maskData->mItemRect.Size() seems reasonable to me. I think the original test was there to match how conservative we were before adding BuildWrMaskImage. Can you put up a patch?

Comment 5

[Nicolas Silva [:nical]]

Attachment: Bug 1595909 - Don't invalidate blob masks when their position changes. r=jrmuizel

Comment 6

[Nicolas Silva [:nical]]

The patch breaks layout/reftests/invalidation/filter-userspace-offset.svg https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f340c846342c7329263165708a920cd6ea9835b

Comment 7

[Jeff Muizelaar [:jrmuizel]]

Attempt for pernosco trace: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0d3044f767925770db9377c5f0e6f496ed7a8235

Comment 8

[Jeff Muizelaar [:jrmuizel]]

I didn't get a chance to debug this much but I did talk to mstange (the author of the test) about it. The test seems involve a foreignObject element that moves while its mask stays in the same position. Since I'm on PTO next week I'd recommend trying to figure something out with mstange.

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:gw, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 10

[Glenn Watson [:gw]]

The patches for this bug that are useful will be Nical's.

Comment 11

[Jeff Muizelaar [:jrmuizel]]

Here's the pernosco trace: https://pernos.co/debug/Oikf4yEz6evm-xsL5G8J_Q/index.html

Comment 12

[Jeff Muizelaar [:jrmuizel]]

The filterUnits="userSpaceOnUse" is contributing to this not working. This property acts as a sort of position:fixed for masks, but we don't know about it in the WebRenderCommandBuilder code. It's not clear how the non-WebRender code handles this case. I'm going to look at that to hopefully get some inspiration.

Comment 13

[Jeff Muizelaar [:jrmuizel]]

The non-WebRender code does not use MaskLayers for this case. But perhaps we can force it to.

Comment 14

[Jeff Muizelaar [:jrmuizel]]

So non-WebRender has two different kinds of invalidation it does for MaskLayers:
CSSMaskLayerUserData and MaskLayerUserData

CSSMaskLayerUserData has the full bounds which explains how it manages to work.

Comment 15

[Jeff Muizelaar [:jrmuizel]]

When not using an active mask layer, FrameLayerBuilder avoids this problem by keeping things relative to the animated geometry root.

It's not clear what the best way for us to solve this is.

Comment 16

[Jessie [:jbonisteel] pls NI]

Should be looked at further by someone who knows Gecko well. Miko, want to take a look?

Comment 17

[Jeff Muizelaar [:jrmuizel]]

Maybe we can just solve this by instead of comparing items bounds, comparing item bounds relative to the animated geometry root. Does that seem practical Miko?

Comment 18

[Miko Mynttinen]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #17)

Maybe we can just solve this by instead of comparing items bounds, comparing item bounds relative to the animated geometry root. Does that seem practical Miko?

I do not know enough about scrolling and masking to answer that. Markus probably knows better, he also added the reftest that was mentioned in comment 6.

Comment 19

[Markus Stange [:mstange]]

Let me know if this is still an issue.