1525087 - z-order is (still) wrong on CSS 3D animation (plane-splitting)

Status: RESOLVED FIXED

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

Description

[Dzmitry Malyshau [:kvark]]

This is a follow-up to https://bugzilla.mozilla.org/show_bug.cgi?id=1523347
The original issue was fixed, but the test case then showed a different issues with the code: problems occurring when the planes are intersected very close to polygon vertices.

It's a truly nice test case: https://bug1210882.bmoattachments.org/attachment.cgi?id=8669060

Comment 1

[Dzmitry Malyshau [:kvark]]

Have an upstream fix ready

Comment 2

[Dzmitry Malyshau [:kvark]]

Attachment: Plane split dependency update to 0.13.5

Update to have https://github.com/servo/plane-split/pull/27 included

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/553538a675ec
Plane split dependency update to 0.13.5 r=kats

Comment 4

[Dorel Luca [:dluca]]

Backed out changeset 553538a675ec (bug 1525087) for causing Wrench failure. CLOSED TREE

Log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=226034546&repo=autoland&lineNumber=2834

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=553538a675ecaf39eebcaf54d6dce1e5d159cca0

Backout:
https://hg.mozilla.org/integration/autoland/rev/dafd7af89bd73b6d4ba56c663594e35840e7d197

Comment 5

[Dzmitry Malyshau [:kvark]]

I just realized that I got mixed up two local copies of firefox, and wasn't looking at the right reftest log of Wrench, d'oh... Fixing this now.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2cbf16256f4e
Plane split dependency update to 0.13.5 r=kats

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/2cbf16256f4e

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Do we have a reftest for this change somewhere? We should add one if it's not too hard.

Comment 9

[Dzmitry Malyshau [:kvark]]

The test was added upstream in https://github.com/servo/plane-split/pull/27/files#diff-1d48fd532a729d28ec7d631fe632f084R289
As long as we ensure that CI passes on upstream before updating the dependency, Gecko will be safe. Is this a common practice to also add a Gecko test in such cases? Technically, we have multiple layers of testing:

1. plane splitting internal tests
2. wrench tests
3. gecko tests

My understanding is that the higher (read: earlier) we can move a test, the better, and there should be no need to duplicate the effort at all 3 levels.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Agreed. I didn't see the test added to plane-split for some reason. I guess my eyes gloss over changes to third_party/rust :/

Comment 11

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 9041259 [details]
Plane split dependency update to 0.13.5

Beta/Release Uplift Approval Request

Feature/Bug causing the regression

None

User impact if declined

3D transformed content will sometimes show up wrong with WebRender

Is this code covered by automated tests?

Yes

Has the fix been verified in Nightly?

Yes

Needs manual test from QE?

No

If yes, steps to reproduce

List of other uplifts needed

None

Risk to taking this patch

Low

Why is the change risky/not risky? (and alternatives if risky)

This is a very low risk change. It has an impact when WebRender is on and we'd like to have it for our release experiment on 66

String changes made/needed

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

We're about halfway through beta at this point, and I would worry if this wasn't behind a pref. Is none of this code used if WebRender is preffed off?

Comment 13

[Jeff Muizelaar [:jrmuizel]]

(In reply to Liz Henry (:lizzard) (use needinfo) from comment #12)

We're about halfway through beta at this point, and I would worry if this wasn't behind a pref. Is none of this code used if WebRender is preffed off?

None of it is used if WebRender is preffed off

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 9041259 [details]
Plane split dependency update to 0.13.5

Fix to support WebRender experiments in 66, OK to uplift for beta 8.

Comment 15

[Narcis Beleuzu [:NarcisB]]

There are conflicts while uplifting to beta:
warning: conflicts while merging Cargo.lock! (edit, then use 'hg resolve --mark')
warning: conflicts while merging gfx/wr/Cargo.lock! (edit, then use 'hg resolve --mark')
warning: conflicts while merging third_party/rust/plane-split/.cargo-checksum.json! (edit, then use 'hg resolve --mark')
warning: conflicts while merging third_party/rust/plane-split/Cargo.toml! (edit, then use 'hg resolve --mark')
warning: conflicts while merging third_party/rust/plane-split/src/bsp.rs! (edit, then use 'hg resolve --mark')

:kvark , can you please provide a patch for beta?

Comment 16

[Dzmitry Malyshau [:kvark]]

Attachment: beta-plane-split.patch

Here is the patch applied to beta.

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/99fa26897e41