1581448 - Add support for specifying extra per-primitive flags in WR display lists

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, enhancement)

Comment 1

[Glenn Watson [:gw]]

Attachment: Bug 1581448 - Add support for specifying extra per-primitive flags in WR display lists

This patch replaces the is_backface_visible bool in the common
per-primitive data in the display list with a PrimitiveFlags
enumeration. This will allow Gecko to specify extra information
about certain primitive, such as tagging scroll bars.

Comment 2

[Glenn Watson [:gw]]

It would be very useful for the WR picture caching code to know which primitives are part of scrollbars. This patch adds a bitfield to the common information about a primitive, to allow marking a primitive as part of a scrollbar.

I briefly discussed this on IRC with Matt, who said that Gecko has the information needed to provide these annotations and pointed me to where to look in the Gecko code. But I got a bit lost, I don't know the Gecko code very well at all.

Questions:

• Does this API seem suitable for allowing Gecko to provide these annotations?
• If so, and once this lands, would someone who knows the Gecko DL code be able to add the Gecko-side parts to set the flag on scrollbar primitives appropriately?

Comment 3

[Jeff Muizelaar [:jrmuizel]]

I'm going to redirect my ni to mstange as he can probably give a better answer with less investigation. If we don't here from him or Matt (it's TPAC this week) than I'll look closer.

Comment 4

[Markus Stange [:mstange]]

Glenn, just to make sure, you want this piece of information on the scrollbar thumb, not the scrollbar track, right? Also, can you describe why it is useful for WR to be able to treat the scrollbar thumb differently from something that has an async transform animation? (The scrollbar thumb already has an animated transform property ID.)

The nsDisplayOwnLayer item we create for scrollbar thumbs returns true from nsDisplayOwnLayer::IsScrollThumbLayer(). I think the information would need to be propagated from this section in nsDisplayOwnLayer::CreateWebRenderCommands to where we create the WR primitive for the stacking context. Maybe by storing the return value of IsScrollThumbLayer() in a new field in the wr::StackingContextParams (and a corresponding new field in StackingContextHelper), and then passing it as parameters through DisplayListBuilder::PushStackingContext and wr_dp_push_stacking_context.

Comment 5

[Glenn Watson [:gw]]

I was thinking of having it for both the thumb and the track, but just the thumb would probably also be good enough.

There's a couple of reasons:

1. In the current picture caching code, it'd make it easy for WR to identify that these items shouldn't be part of the cached content slice. This would avoid invalidations occurring in content tiles due to the scroll bar moving. It's currently hard to reliably distinguish these from other fixed position elements in the content at the beginning / end of the display list for an iframe.

2. In future, I think it might make sense to place the root scroll bar elements into their own OS compositor layer.

If those don't make sense, or you can think of other possible ways to achieve those outcomes, I'd be open to any other suggestions too!

Comment 6

[Markus Stange [:mstange]]

Ok, sounds good. We also have nsDisplayOwnLayer::IsScrollbarContainer() for the entire scrollbar. So I think we need two WR flags, PrimitiveFlags::IS_SCROLLBAR_CONTAINER and PrimitiveFlags::IS_SCROLLBAR_THUMB.

Comment 7

[Glenn Watson [:gw]]

I updated the patch to have those two flags. Thanks for taking a look!

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/af9417bd8105
Add support for specifying extra per-primitive flags in WR display lists r=kvark,nical

Comment 9

[Andreea Pavel [:apavel]]

Backe dout for WR bustages

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=af9417bd810595fd0dcf122136f481b64bfec3bc&selectedJob=267189302

Failure log: https://taskcluster-artifacts.net/SJMORUJcTZCirLBCEfzdKQ/0/public/logs/live_backing.log

Backout: https://hg.mozilla.org/integration/autoland/rev/432fadcfe39ce8252e9d002adaa4f53bb467d215

Comment 10

[Glenn Watson [:gw]]

Fixed up the patch and updated in phab. Just doing a try run rebased on current autoland to make sure there are no more conflicts:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=deb8e9c6561b3ad6b0d9b3ae2243473da6b2b1b3

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8bec7810fc09
Add support for specifying extra per-primitive flags in WR display lists r=kvark,nical

Comment 12

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/8bec7810fc09