Skip to content
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

Fix z-ordering of transformed content inside preserve-3d context. #2369

Merged
merged 1 commit into from Feb 3, 2018

Conversation

@glennw
Copy link
Member

glennw commented Feb 1, 2018

When a 3d rendering context is established, add an extra picture
(that is never composited to an intermediate surface). This extra
pictures serves as a 3d rendering context container.

Add a normal picture as a child of that container which is marked
as using an intermediate surface. Any untransformed content that
is included as part of the 3d rendering context will be placed in
here, and be rendered in the correct order. If no untransformed
content is added, then this picture won't be allocated and will
have no effect on intermediate surface allocation.

This gets us some ordering and correctness fixes. It still doesn't
dynamically avoid plane-splitting when it's not needed. However,
with this in place, and the recent changes to add PictureState,
adding this optimization will be relatively simple.

Fixes #2337.


This change is Reviewable

@glennw
Copy link
Member Author

glennw commented Feb 1, 2018

@staktrace
Copy link
Contributor

staktrace commented Feb 1, 2018

I looked at the mask-layer-3.html failure, and that one at least is a gecko bug. The gecko DL for the <div id="c"> has an empty clipchain, but in gecko land it inherits the clips from enclosing items which include the clip with the rounded corners. When we translate this DL to a WR DL we don't explicitly generate any clip for that item - however there is a stacking context introduced by <div id="b"> and so that makes it so that the rounded-corner clip doesn't get inherited down. This is something I can fix on the gecko side as part of my clipping rewrite to use the new clip chains API.

@glennw
Copy link
Member Author

glennw commented Feb 1, 2018

I investigated the other test failure. Interestingly in this case, the main reftest draws correctly, it is the -ref file that fails to draw anything. I recorded the Gecko DL and reduced the test case to the YAML below. In this case, the display list appears to have set the transform-style to preserve-3d but there is no preserve-3d in the source HTML file (for the -ref image).

So I think that points to a Gecko DL bug, but ideally @staktrace could verify that.

If that's the case, then both the unexpected FAILs here are Gecko DL bugs, and this should be ready for review and merge. r? @kvark

---
root:
  items:
    -
      type: "stacking-context"
      items:
        -
          bounds: [0, 0, 1887, 1971]
          "clip-rect": [0, 0, 1887, 1971]
          type: clip
          id: 1
          "content-size": [1887, 1971]
        -
          "clip-rect": [0, 0, 1887, 1971]
          "clip-and-scroll": 1
          type: "scroll-frame"
          id: 2
          "content-size": [1887, 1971]
          bounds: [0, 0, 1887, 1971]
        -
          bounds: [0, 0, 1887, 1971]
          "clip-rect": [0, 0, 1887, 1971]
          "clip-and-scroll": 2
          type: clip
          id: 3
          "content-size": [1887, 1971]
        -
          "clip-and-scroll": 2
          type: "stacking-context"
          filters:
            - opacity(0.5)
          items:
            -
              "clip-and-scroll": 3
              type: "stacking-context"
              transform: [1, 0, 0, 0, 0, 1, 0, 0, 0, 0, 1, 0, 12, 150, 0, 1]
              "transform-style": "preserve-3d"
              perspective: [1, 0, 0, 0, 0, 1, 0, 0, -0.6, -0.6, 1, -0.006666667, 0, 0, 0, 1]
              items:
                -
                  "clip-and-scroll": 3
                  type: "stacking-context"
                  transform: [1, 0, 0, 0, 0, 0, -1, 0, 0, 1, 0, 0, 0, 180, 90, 1]
                  items:
                    -
                      bounds: [0, 0, 180, 180]
                      "clip-rect": [0, 0, 180, 180]
                      "clip-and-scroll": 3
                      type: rect
                      color: 255 0 255 1.0000
@glennw
Copy link
Member Author

glennw commented Feb 1, 2018

(The clip and scroll here for the inner element looks wrong too, I think - similarly to the other test failure?)

@staktrace
Copy link
Contributor

staktrace commented Feb 2, 2018

@glennw Yeah that seems like a Gecko bug

@glennw
Copy link
Member Author

glennw commented Feb 2, 2018

Thanks, so I think this should be fine to merge once the review is done, and we'll add 2 new failure annotations to the WR update.

r? @kvark

@kvark
Copy link
Member

kvark commented Feb 3, 2018

Reviewed 4 of 4 files at r1.
Review status: all files reviewed at latest revision, 3 unresolved discussions.


webrender/src/frame_builder.rs, line 480 at r1 (raw file):

                .iter()
                .rev()
                .find(|sc| sc.rendering_context_3d_prim_index.is_some())

wouldn't this be the same primitive ID as self.picture_stack.lat().unwrap()?


webrender/src/frame_builder.rs, line 561 at r1 (raw file):

        }

        if participating {

let's rename to something like participate_in_plane_split


wrench/reftests/split/ordering.yaml, line 5 at r1 (raw file):

  items:
    -
      type: "stacking-context"

how does the original code handle this case? where do things go wrong?


Comments from Reviewable

When a 3d rendering context is established, add an extra picture
(that is never composited to an intermediate surface). This extra
pictures serves as a 3d rendering context container.

Add a normal picture as a child of that container which is marked
as using an intermediate surface. Any untransformed content that
is included as part of the 3d rendering context will be placed in
here, and be rendered in the correct order. If no untransformed
content is added, then this picture won't be allocated and will
have no effect on intermediate surface allocation.

This gets us some ordering and correctness fixes. It still doesn't
dynamically avoid plane-splitting when it's not needed. However,
with this in place, and the recent changes to add PictureState,
adding this optimization will be relatively simple.

Fixes #2337.
@glennw glennw force-pushed the glennw:order-for-real-2 branch from 614a92f to e980335 Feb 3, 2018
@glennw
Copy link
Member Author

glennw commented Feb 3, 2018

Review status: 3 of 4 files reviewed at latest revision, 3 unresolved discussions.


webrender/src/frame_builder.rs, line 480 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

wouldn't this be the same primitive ID as self.picture_stack.lat().unwrap()?

It would be in the first level of a preserve-3d hierarchy, but not if you have a nested preserve-3d (since the primitive ID there is only stored as Some(..) when an establishing 3d context is found).


webrender/src/frame_builder.rs, line 561 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

let's rename to something like participate_in_plane_split

Done.


wrench/reftests/split/ordering.yaml, line 5 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

how does the original code handle this case? where do things go wrong?

The original code in batch.rs draws all the un-transformed content first, then the planes that were participating. The spec says that the untransformed content should be drawn on a plane at z=0 and interact with the other plane splits.


Comments from Reviewable

@glennw
Copy link
Member Author

glennw commented Feb 3, 2018

@kvark Thanks, comments addressed.

@kvark
Copy link
Member

kvark commented Feb 3, 2018

Sounds good, thank you!
@bors-servo r+

@bors-servo
Copy link
Contributor

bors-servo commented Feb 3, 2018

📌 Commit e980335 has been approved by kvark

@bors-servo
Copy link
Contributor

bors-servo commented Feb 3, 2018

Testing commit e980335 with merge 7809537...

bors-servo added a commit that referenced this pull request Feb 3, 2018
Fix z-ordering of transformed content inside preserve-3d context.

When a 3d rendering context is established, add an extra picture
(that is never composited to an intermediate surface). This extra
pictures serves as a 3d rendering context container.

Add a normal picture as a child of that container which is marked
as using an intermediate surface. Any untransformed content that
is included as part of the 3d rendering context will be placed in
here, and be rendered in the correct order. If no untransformed
content is added, then this picture won't be allocated and will
have no effect on intermediate surface allocation.

This gets us some ordering and correctness fixes. It still doesn't
dynamically avoid plane-splitting when it's not needed. However,
with this in place, and the recent changes to add PictureState,
adding this optimization will be relatively simple.

Fixes #2337.

<!-- 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/2369)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Feb 3, 2018

☀️ Test successful - status-appveyor, status-taskcluster, status-travis
Approved by: kvark
Pushing 7809537 to master...

@bors-servo bors-servo merged commit e980335 into servo:master Feb 3, 2018
3 of 4 checks passed
3 of 4 checks passed
code-review/reviewable 1 file, 3 discussions left (kvark)
Details
Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
@staktrace
Copy link
Contributor

staktrace commented Feb 4, 2018

@glennw This PR seems to have caused windows debug jobs to start failing in gecko CI - see last part of https://bugzilla.mozilla.org/show_bug.cgi?id=1434723#c6. Not really sure why, there's no obvious root cause in the log.

Also FYI I updated https://wiki.mozilla.org/Platform/GFX/Quantum_Render#Try_syntax to include windows in the try push syntax - I neglected to do this when we added the windows jobs, sorry!

@staktrace
Copy link
Contributor

staktrace commented Feb 4, 2018

Actually it might be a gecko-side problem. See comment 7.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants
You can’t perform that action at this time.