If the transform contains perspective component, the clip rect don't affect by this transform.

[mephisto41]

When calculating the LCCR. If current transform or parent transform has
perspective component, then we shouldn't transform the LCCR by this
transform because we only consider 2d component for LCCR. Once the
perspective component is presence, the LCCR would be wrong.

---

This change is 

[bors-servo]

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

[glennw]

I'm a little unsure about this - shouldn't the LCCR take into account the transform with perspective and be conservative @kvark ?

[kvark]

@glennw that is correct. LCCR should provide a conservative local rectangle bound even with transformations involved. Relevant code:

            NodeType::ReferenceFrame(ref info) => {
                self.combined_local_viewport_rect =
                    info.transform.with_destination::<LayerPixel>()
                             .inverse_rect_footprint(&scrolled_parent_combined_clip);
                self.reference_frame_relative_scroll_offset = LayerVector2D::zero();
                (info.transform, state.parent_accumulated_scroll_offset)

[glennw]

Can we try to find out why the code @kvark referenced above doesn't seem to get the conservative bounding rect in this case?

[glennw]

@kvark Looks like this has been updated. @mephisto41 is it ready for review?

[bors-servo]

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

[mephisto41]

@glennw , I think this is ready for review now.

[glennw]

@mephisto41 Ideally I'd like to get @kvark to take a look at this. I thought that the inverse rect footprint calculation should handle perspective transforms.

Unfortunately @kvark is traveling this week, apologies for the delay. Is this an urgent thing to get merged? (If so - we could possibly merge it and follow up when @kvark is back).

We should also do a gecko try run and run the servo wpt tests on this - I can run those locally tomorrow if you're not sure how to do those?

[mephisto41]

@mephisto41 Ideally I'd like to get @kvark to take a look at this. I thought that the inverse rect footprint calculation should handle perspective transforms.

Yap, footprint calculation handle perspective transforms. But it ignores all perspective and 3d components (a.k.a third row and third column and m34 of matrix)

Unfortunately @kvark is traveling this week, apologies for the delay. Is this an urgent thing to get merged? (If so - we could possibly merge it and follow up when @kvark is back).

Not urgent, I can wait @kvark :)

We should also do a gecko try run and run the servo wpt tests on this - I can run those locally tomorrow if you're not sure how to do those?

Sure, thank you very much. 👍

[glennw]

@mephisto41 OK - the wrench and servo tests pass, but it looks like there are quite a lot of gecko failures that will need to be investigated:

https://hg.mozilla.org/try/rev/3b88c7245af9af6976d639f5bddac6096ad3c55e
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b88c7245af9af6976d639f5bddac6096ad3c55e

[mephisto41]

Ah, I'll check the gecko failures first.

[mephisto41]

After some experiments, I cannot find a very good way to solve this. In my test case, I have 2 stacking-contexts as follow.

1. SC with perspective(300), matrix is

[1, 0, 0, 0, 
 0, 1, 0, 0, 
 0, 0, 1, -0.0033333334, 
 0, 0, 0, 1]

scrolled_parent_combined_clip is TypedRect(1920×1080 at (0,0))
LCCR is TypedRect(1920×1080 at (0,0))

2. SC with translate-Z(-300) scale(2), matrix is

[2, 0, 0, 0, 
 0, 2, 0, 0, 
 0, 0, 2, 0, 
 0, 0, -300, 1]

scrolled_parent_combined_clip is TypedRect(1920×1080 at (0,0))
LCCR is TypedRect(960×540 at (0,0))

The problem is when calculating the LCCR for second SC, we only consider its 2d components. In above example, we have perspective(300) in first SC and translateZ(-300) in second SC which means the final scale components should be (1, 1) so that the LCCR should be TypedRect(1920x1080 at(0,0)). I don't have a easy way to fix this.

@glennw @kvark , Do you have any suggestion for this? Thanks.

[kvark]

@mephisto41

The problem is when calculating the LCCR for second SC, we only consider its 2d components.

Actually, the second one is fine. It's the first one that should not have resulted in 0,0 .. 1922,1080. For example, a point at (4000.0, 0.0, -1000.0) in SC1 would project to (923.0769, 0) in SC0, which is well within bounds. Thus, (4000.0, 0.0) should be inside LCCR for SC1. In fact, LCCR for SC1 is not bound at all (read: infinity) since for any large enough X & Y, you can match Z so that the projection fits within the box for SC0.

The code in question is here:

    fn inverse_project(&self, target: &TypedPoint2D<f32, Dst>) -> Option<TypedPoint2D<f32, Src>> {
        let m: TypedTransform2D<f32, Src, Dst>;
        m = TypedTransform2D::column_major(
            self.m11 - target.x * self.m14,
            self.m21 - target.x * self.m24,
            self.m41 - target.x * self.m44,
            self.m12 - target.y * self.m14,
            self.m22 - target.y * self.m24,
            self.m42 - target.y * self.m44,
        );
        m.inverse().map(|inv| TypedPoint2D::new(inv.m31, inv.m32))
    }

The proof was on a piece of paper somewhere, and now is a good time as ever to try deriving the formula again to find out what my original error was.

[mephisto41]

Hi @glennw, as we discussed couple days ago. This version completely skip LCCR if transform has perspective component. How do you think about this version?

I also push a try in gecko and it looks fine. https://treeherder.mozilla.org/#/jobs?repo=try&revision=b31b8f950142686f6107f7037c680fa5f30a1d3d&selectedJob=134828657

[glennw]

@mephisto41 Thanks for following this up! I think this looks fine - it's not the perfect solution, but it does add new test passes in the try run, and it does has a reftest to make sure we don't regress it with any follow up changes. @kvark Are you happy to have this merged?

[kvark]

@glennw it's a good step for at least as long as inverse_project remains unfixed. We'll need to revisit the issue afterwards.
Are gecko failures addressed?

[mephisto41]

@kvark, no more gecko failures (see this try). We even fix two more tests in reftest (see r8 for 2 UNEXPECTED-PASS tests) with this patch.

[kvark]

thank you!
❤️ those unexpected passes :)
@bors-servo r+

[bors-servo]

📌 Commit 90aa4c2 has been approved by kvark

[bors-servo]

⌛ Testing commit 90aa4c2 with merge 8f0e0c6...

[bors-servo]

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