Revised backface visibility and 3D plane ordering

[kvark]

Fixes #3114

Backface visibility logic changes

• the property is not propagated hierarchically
• the property is inherited only by the pictures directly spawned from a primitive
• backface-visibility is determined in the coordinate space of the containing block. For 3D-participating contexts, this is the parent of the 3D sub-tree root.

This isn't obvious, and neither it is guaranteed to be correct. It's just a rather simple scheme that makes us behave properly on the issues listed in #3114. Feedback is welcome!
Update: this should be correct. Gecko changes are also needed though.

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

cc @gankro @mstange (and thanks for fruitful discussion!)

Plane splitting re-architecture

All the interactions with plane_split crate have been moved from batching code to prepare_xxx_for_render. The code is better structured now, with most of the plane splitting logic in one place inside a (relatively small) picture module, organized as helper functions. Batching still does a bit of work to actually make draw instances from the ordered planes, but it's not longer a computational work, it's just shuffling data.

Plane split logic changes

Whenever we iterate the children of a stacking context that participates in plane splitting, and we encounter a child which is another stacking context, we break the immediate children sequence and organize a new picture from them. Any following immediate children will be going to another picture, therefore, and the order or rendering is preserved this way.

---

This change is 

[kvark]

AAAnd we have a fail in /css/css-transforms/transform3d-backface-visibility-006.html
So this new model needs more thought (marking as WIP)

[Darkspirit]

https://codepen.io/desandro/pen/LmWoWe
Regression: Not only the text (still) flickers, now there is even a red & blue flicker on the back side.

Hackerone is fixed.

[kvark]

Worked quite a bit more on the issue, made it function in accordance to the draft spec.
Pending try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3ca6af02e87b720df1f145b7662900eaa6ce712c

[emilio]

Not sure how much the draft spec is web-compatible, probably worth checking with @mattwoodrow.

[Gankra]

The spec has historically been inconsistent with the implementations, which is why Matt and I didn't bother looking at it

[bors-servo]

☔ The latest upstream changes (presumably #3143) made this pull request unmergeable. Please resolve the merge conflicts.

[kvark]

I've been looking at backface-visibility-2 myself and with @mstange , and it looks like the failure is caused by Gecko not properly propagating the backface visibility property.

Supposedly, error happens here:
https://searchfox.org/mozilla-central/rev/6ddb5fb144993fb5de044e2e8d900d7643b98a4d/layout/painting/nsDisplayList.cpp#8767

The front face element is red color and has the backface visibility disabled. It's parented to the "card" div that establishes a preserve-3d context, and that is rotated backwards.

Observed behavior: Gecko spawns a (flat) stacking context (SC) around it, which gets backface visibility enabled. Since this SC serves as a preserve-3d hierarchy boundary, and the element still has backface visibility disabled, it turns out to be invisible (regardless of the transforms).

Expected behavior: either Gecko specifies the element as is (without wrapping into SC), or the backface-visibility property gets moved up to the SC wrap.

@staktrace would you happen to know how (or if?) this should be fixed on Gecko side? Attaching the display lists dump generated by Gecko and passed to WR. Alternatively, I'll keep poking at it.

dl-dump.txt

[staktrace]

@kvark - I'm not totally following your explanation. What I see in the display list dump is this (simplified):

div id=card     --> produces rotation SC with is_backface_visible = true
  div id=front  --> produces rect with is_backface_visible = false
  div id=back   --> produces rotation SC with is_backface_visible = false
                --> also produces rect with is_backface_visible = false

This seems like it matches the HTML exactly, since the backface-visibility:hidden is set on two inner divs and not the card div. Are you saying that we should be propagating the is_backface_visible=false to the outer SC? If so, what happens to other elements that may live inside it and that have is_backface_visible=true?

[staktrace]

... although I may have oversimplified things. There's a lot of reference frames and duplicate stacking contexts as well.

[mstange]

div id=front also produces a stacking context in line 30 of the dump:

        GenericDisplayItem { item: PushStackingContext(PushStackingContextDisplayItem { stacking_context: StackingContext { transform_style: Flat, mix_blend_mode: Normal, clip_node_id: None, raster_space: Screen } }), clip_and_scroll: ClipAndScrollInfo { scroll_node_id: Spatial(4, PipelineId(1, 7)), clip_node_id: None }, info: PrimitiveInfo { rect: TypedRect(0.0×0.0 at (0.0,0.0)), clip_rect: TypedRect(0.0×0.0 at (0.0,0.0)), is_backface_visible: true, tag: None } }

This stacking context has is_backface_visible: true, which is the problematic part here.

[kvark]

Third try push - https://treeherder.mozilla.org/#/jobs?repo=try&revision=e1c69444dcd1e9d574b3d6b1d796613bc26c081f

[kvark]

Suggested Gecko patch by @staktrace :

diff --git a/layout/painting/nsDisplayList.cpp b/layout/painting/nsDisplayList.cpp
--- a/layout/painting/nsDisplayList.cpp
+++ b/layout/painting/nsDisplayList.cpp
@@ -8686,16 +8686,24 @@ nsDisplayTransform::ShouldBuildLayerEven
 bool
 nsDisplayTransform::CreateWebRenderCommands(
   mozilla::wr::DisplayListBuilder& aBuilder,
   mozilla::wr::IpcResourceUpdateQueue& aResources,
   const StackingContextHelper& aSc,
   WebRenderLayerManager* aManager,
   nsDisplayListBuilder* aDisplayListBuilder)
 {
+  if (mNoExtendContext) {
+    // This is a wrapper nsDisplayTransform that separates descendants from
+    // the 3D context of the ancestors. WR doesn't need a stacking context for
+    // this so we can just generate the WR items for the descendants.
+    return mStoredList.CreateWebRenderCommands(
+        aBuilder, aResources, sc, aManager, aDisplayListBuilder);
+  }
+
   // We want to make sure we don't pollute the transform property in the WR
   // stacking context by including the position of this frame (relative to the
   // parent reference frame). We need to keep those separate; the position of
   // this frame goes into the stacking context bounds while the transform goes
   // into the transform.
   LayoutDevicePoint position;
Matrix4x4 newTransformMatrix = GetTransformForRendering(&position);

[kvark]

Investigated a bit more, found a problem with WR side code. Fixed, rebased, and pushed here.
Local testing shows everything to be great 🤞
New try push, including the Gecko patch above: https://treeherder.mozilla.org/#/jobs?repo=try&revision=eceaeb8eb63881833e6c752ec92ecd04788a82ec

[kvark]

Getting new failure in split-non-ortho1.html, presumably related to (but not necessarily caused by) the Gecko patch... Need to investigate more.

[Darkspirit]

mozregression --repo try --launch eceaeb8eb63881833e6c752ec92ecd04788a82ec --pref gfx.webrender.all:true -a https://keithclark.co.uk/labs/css-fps/nojs/
Screencast: https://cloud.terrax.net/s/nsTJSy8tZqtt6XC
Left side (your try build): At some point it's just black.
Right: Current Nightly (which looks the same as the latest try build from wr-future-update).

[kvark]

Here is what's going on with split-non-ortho1.html. It has non-transformed items (inside preserve-3D context) mixed with transformed ones, but transformation is only within the 2D plane (so nothing to plane split or sort, really). The expectation of the test is that the order elements are provided in DOM is preserved then when rendering them.

In the old (Gecko!) code, each of the elements would spawn a separate stacking context, and WR is able to preserve the order of these when plane-splitting.

In the new Gecko code, some elements no longer have a separate stacking context, so WR groups them into the same picture, which participates in ordering and plane splitting as a whole... thus, the drawing order gets broken.

TL;DR; the proposed Gecko patch unveils a problem in WR 3D ordering that was hidden before. Either the patch needs to be rewritten (i.e. by propagating backface-visibility into the spawned contexts), or WR needs to be fixed w.r.t. ordering in this case. I'd prefer the latter, and I'm looking into it, but it's unfortunate that this very PR is not quite strongly related to this blocking fix.

[gw3583]

@kvark Interesting problem! A couple of comments:

• There is code in WR that supposedly deals with having un-transformed content inside a 3d rendering context in addition to planes that undergo plane splitting. I had to implement this to get some of the WPT tests passing. It's quite likely it's broken in some (perhaps most) cases though! It's a bit subtle to describe in a GH comment, but perhaps ping me tomorrow and we can discuss the way it currently is meant to work, and why it's failing in this case.

• I'm working on a patch in preparation for picture caching at the moment that does alter how we push / pop stacking contexts during flattening. It's probably not relevant here, but just raising it now so we can include that in the discussion mentioned above.

[gw3583]

The TL;DR is that when a 3d rendering context is established, we add two pictures.

The first picture [1] should in theory get any un-transformed content added to it. This picture is parented to the 3d render context container picture [2]. Any preserve-3d items that are encountered are then parented to the 3d container in [2].

The idea is that we end up with a picture for the un-transformed content to be flattened into, which is then sorted with the transformed content. At the time it seemed like this matched the spec, but it's quite likely it has flaws!

[1]

webrender/webrender/src/display_list_flattener.rs

Line 1021 in d558877

let leaf_picture = PicturePrimitive::new_image(

[2]

webrender/webrender/src/display_list_flattener.rs

Line 1104 in d558877

// If establishing a 3d context, we need to add a picture

[kvark]

@gw3583 thanks for the info! Let's talk this over tonight on IRC.

In the meantime, the way our 3D stacking contexts are treated, I think, should be the following:

• in push_stacking_context if the relative transform to the previous one doesn't have Z component, then just really flatten it into the parent, don't spawn a new plane for splitting
• if the relative transform does have Z, then "break the picture" as in - close the one containing all untransformed items, and then open a new one if any show up after that stacking context.

That scheme would preserve the ordering with regards to the most 3D cases (outside of obscure ones where there is an inversely transformed context inside a transformed one, and the app expects the order to be preserved ... yikes!).

[bors-servo]

☔ The latest upstream changes (presumably #3167) made this pull request unmergeable. Please resolve the merge conflicts.

[gw3583]

@kvark I certainly haven't looked into this in detail, so I don't have a full understanding of the problem yet. That said, when I implemented the picture code for this, it seemed like it should match the specification and not require any workarounds like you've mentioned above.

I think you already ran into this btw, but the draft spec is quite different from what browsers implement. The spec I used as a reference is https://drafts.csswg.org/css-transforms-2/#transform-functions (linked from the MDN docs at https://developer.mozilla.org/en-US/docs/Web/CSS/transform). I had a quick read again and it seems like we should be matching the specification as far as I can tell (most likely I'm missing something though, since we clearly have an issue here!).

The other thing to mention regarding your comment above - I don't think we can change any behavior in push_stacking_context that is based on the transform values. This is because the flattening is done once per scene and doesn't know the future value of the transforms. For example, property animation could be used to change the transform completely without a new scene being built. So, if we do need to do something like this, it'll probably need to be in the picture preparation code.

Next time we're online, let's discuss this in detail - I also have some other semi-related thoughts on how to optimize plane-splitting in the common case, that may end up factoring into this stuff.

[gw3583]

@kvark OK, this looks good to me! There are a few nits in the review to solve, but that's all that remains. The other question is whether we want to merge this so close to the beta merge date for Gecko. Do we have any thoughts on possible risk / whether we should hold off on merging this until some point later in the week?

[kvark]

Thanks for the review @gw3583 !

The other question is whether we want to merge this so close to the beta merge date for Gecko. Do we have any thoughts on possible risk / whether we should hold off on merging this until some point later in the week?

AFAIK, Beta is branching out today, and in order for this code to ship there we'd need another WR update in FF. I'd like this code to stay a bit in Nightly before shipping. Confirmed with @staktrace on IRC that we aren't planning on a WR update before the branch. Any further hotfixes can be cherry-picked, so the PR isn't blocking us to do anything. Let's proceed 🚂

@bors-servo r=gw3583

[jrmuizel]

@bors-servo r+

[kvark]

@bors-servo are you there?

[jrmuizel]

@bors-servo: ping

[bors-servo]

📌 Commit 6a622a1 has been approved by gw3583

[bors-servo]

📌 Commit 6a622a1 has been approved by jrmuizel

[bors-servo]

😪 I'm awake I'm awake

[bors-servo]

⌛ Testing commit 6a622a1 with merge 838ce47...

[bors-servo]

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: jrmuizel
Pushing 838ce47 to master...