New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revised backface visibility and 3D plane ordering #3131

Merged
merged 12 commits into from Oct 22, 2018

Conversation

Projects
None yet
10 participants
@kvark
Member

kvark commented Sep 26, 2018

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 Reviewable

@kvark kvark changed the title from New backface visibility mechanics to [WIP] New backface visibility mechanics Sep 26, 2018

@kvark

This comment has been minimized.

Member

kvark commented Sep 26, 2018

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

This comment has been minimized.

Darkspirit commented Sep 26, 2018

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

This comment has been minimized.

Member

kvark commented Sep 30, 2018

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

@kvark kvark changed the title from [WIP] New backface visibility mechanics to New backface visibility mechanics Sep 30, 2018

@kvark kvark changed the title from New backface visibility mechanics to [WIP] New backface visibility mechanics Sep 30, 2018

@emilio

This comment has been minimized.

Member

emilio commented Sep 30, 2018

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

@Gankro

This comment has been minimized.

Contributor

Gankro commented Sep 30, 2018

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

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Oct 2, 2018

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

@kvark

This comment has been minimized.

Member

kvark commented Oct 2, 2018

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

This comment has been minimized.

Contributor

staktrace commented Oct 2, 2018

@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

This comment has been minimized.

Contributor

staktrace commented Oct 2, 2018

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

@mstange

This comment has been minimized.

Contributor

mstange commented Oct 2, 2018

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

This comment has been minimized.

@kvark

This comment has been minimized.

Member

kvark commented Oct 3, 2018

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

This comment has been minimized.

Member

kvark commented Oct 3, 2018

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 kvark changed the title from [WIP] New backface visibility mechanics to New backface visibility mechanics Oct 3, 2018

@kvark

This comment has been minimized.

Member

kvark commented Oct 3, 2018

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

@Darkspirit

This comment has been minimized.

Darkspirit commented Oct 3, 2018

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

This comment has been minimized.

Member

kvark commented Oct 4, 2018

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

This comment has been minimized.

Collaborator

gw3583 commented Oct 4, 2018

@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

This comment has been minimized.

Collaborator

gw3583 commented Oct 4, 2018

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]

let leaf_picture = PicturePrimitive::new_image(

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

@kvark

This comment has been minimized.

Member

kvark commented Oct 4, 2018

@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

This comment has been minimized.

Contributor

bors-servo commented Oct 5, 2018

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

@gw3583

This comment has been minimized.

Collaborator

gw3583 commented Oct 5, 2018

@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.

@kvark kvark changed the title from New backface visibility mechanics to [WIP] New backface visibility mechanics Oct 12, 2018

@kvark kvark changed the title from [WIP] New backface visibility mechanics to [WIP] Revised backface visibility and 3D plane ordering Oct 12, 2018

@kvark

This comment has been minimized.

Member

kvark commented Oct 12, 2018

Rebased (basically re-writing most of the code), added a new reftest for the 3D ordering problem, and made some preparations for the implementation. Working on it...

@Gankro

This comment has been minimized.

Contributor

Gankro commented Oct 12, 2018

Just confirming you're not done and this still shouldn't be reviewed?

@kvark

This comment has been minimized.

Member

kvark commented Oct 12, 2018

@Gankro correct. I'll shout out when it's ready for looks ;)

@gw3583

Reviewed 6 of 20 files at r5.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @gw3583 and @kvark)

@gw3583

This comment has been minimized.

Collaborator

gw3583 commented Oct 21, 2018

@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?

@gw3583 gw3583 assigned gw3583 and unassigned gw3583 Oct 21, 2018

@gw3583

gw3583 approved these changes Oct 21, 2018

@kvark

This comment has been minimized.

Member

kvark commented Oct 22, 2018

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

This comment has been minimized.

Contributor

jrmuizel commented Oct 22, 2018

@kvark

This comment has been minimized.

Member

kvark commented Oct 22, 2018

@bors-servo are you there?

@jrmuizel

This comment has been minimized.

Contributor

jrmuizel commented Oct 22, 2018

@bors-servo: ping

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Oct 22, 2018

📌 Commit 6a622a1 has been approved by gw3583

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Oct 22, 2018

📌 Commit 6a622a1 has been approved by jrmuizel

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Oct 22, 2018

😪 I'm awake I'm awake

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Oct 22, 2018

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

bors-servo added a commit that referenced this pull request Oct 22, 2018

Auto merge of #3131 - kvark:backface, r=gw3583
Revised backface visibility and 3D plane ordering

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](#3131 (comment)) 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.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/3131)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Oct 22, 2018

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

@bors-servo bors-servo merged commit 6a622a1 into servo:master Oct 22, 2018

2 of 4 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
code-review/reviewable 20 files, 5 discussions left (gw3583, kvark)
Details
Taskcluster (pull_request) TaskGroup: success
Details
homu Test successful
Details

@kvark kvark deleted the kvark:backface branch Oct 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment