1035611 - Correctly handle untransforming projective transforms that cross the w=0 plane

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Matt Woodrow (:mattwoodrow)]

When trying to compute visibility for  layers/display items with a projective transformation, some of our input points (usually the window rectangle) map to points with w <= 0 in the local coordinate space of the layer. Attempting to find the bounds of the computed points in this case comes up with incorrect results.

Our current solution involves discarding any points where this is true, as well as finding lines between points that cross w=0 and interpolating a point with a w value just greater than 0.

Unfortunately this is somewhat hacky, and can produce incorrect results if the value we choose for w isn't close enough to 0. Going too close to 0 though puts us at risk of running out of precision.


Ideally we'd be able to solve this without hacky approximations. See bug 1031948 comment 9 for bjacob's full explanation of the math for this.

Comment 1

[Matt Woodrow (:mattwoodrow)]

w=0 is problematic in this case since we convert from homogenous coordinates (4d) to 3d ones by dividing by the w component. This results in a discontinuity when we cross w=0, and defining a rectangle by a set of points that exist on both sides of it doesn't make sense. Culling all points with w <= 0 seems to work for what we want.

Comment 2

[:kip (Kearwood Gilbert)]

Matrix4x4::ProjectRectBounds must never return std::numeric_limits<Float>::max() or any
other arbitrary large value in place of inifinity.  This often occurs when
aRect is an inversed projection matrix or when aRect is transformed to be
partly behind and in front of the camera (w=0 plane in homogenous
coordinates)

Some call-sites will call RoundGfxRectToAppRect which clips both the
extents and dimensions of the rect to be bounded by nscoord_MAX.
If we return a Rect that, when converted to nscoords, has a width or height
greater than nscoord_MAX, RoundGfxRectToAppRect will clip the overflow
off both the min and max end of the rect after clipping the extents of the
rect, resulting in a translation of the rect towards the infinite end.

The bounds returned by ProjectRectBounds are expected to be clipped only on
the edges beyond the bounds of the coordinate system; otherwise, the
clipped bounding box would be smaller than the correct one and result
bugs such as incorrect culling.  One example of this is Bug 1073056.

In Bug 1073056, due to floating point precision issues when dividing
w by 0.00001f in ComputePerspectivePlaneIntercept, the returned Rect
would inconsistently have dimensions greater or lesser than
nscoord_MAX.  This caused the layer paints to be incorrectly culled
and to appear black at chaotically selected pseudo-random angles of
rotation.

To address this without requiring all code to work in homogenous
coordinates or interpret infinite values correctly, a specialized
clipping function could be integrated into ProjectRectBounds.

Callers should pass an aClip value that represents the extents to clip
the result to, in the same coordinate system as aRect.

A patch that add the clipping functionality to
Matrix4x4::ProjectRectBounds and updates all call-sites
will follow.

Comment 3

[:kip (Kearwood Gilbert)]

Attachment: Bug 1035611 - Part 1: Updated Matrix4x4::ProjectRectBounds to properly handle infinite values when untransformed rects cross the w=0 plane

- Added specialized rect clipping functionality to Matrix4x4::ProjectRectBounds
so we don't have to return infinite values when rects cross the w=0 plane
in homogenous coordinate space.
- Updated callsites of ProjectRectBounds to pass a clipping rect that is
appropriate for the units that are returned.

Comment 4

[:kip (Kearwood Gilbert)]

This seems to fix Bug 1073056.  I have pushed to try to run it through its paces:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e1b7bf36ee0

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 8577523 [details] [diff] [review]
Bug 1035611 - Part 1: Updated Matrix4x4::ProjectRectBounds to properly handle infinite values when untransformed rects cross the w=0 plane

Review of attachment 8577523 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Kip! You want another reviewer - I'm no longer at Mozilla, and reviews like this are a great way to spread understanding of the codebase, so I wouldn't be doing guys a favor by actually doing this review now. My go-to reviewer for a patch like this would be Matt.

Comment 6

[:kip (Kearwood Gilbert)]

Comment on attachment 8577523 [details] [diff] [review]
Bug 1035611 - Part 1: Updated Matrix4x4::ProjectRectBounds to properly handle infinite values when untransformed rects cross the w=0 plane

Hi Matt, would you have some availability to review this patch, as a solution to this bug and bug 1073056?

I am working on a test based on the CSS in Bug 1073056, which I will add if you think the solution in my patch would be a good one.

Comment 7

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8577523 [details] [diff] [review]
Bug 1035611 - Part 1: Updated Matrix4x4::ProjectRectBounds to properly handle infinite values when untransformed rects cross the w=0 plane

Review of attachment 8577523 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

Comment 8

[Lawrence Mandel [:lmandel] (use needinfo)]

I marked this as check-in needed. Please clear that if this actually isn't ready to land.

This bug fixes bug 1073056, which is tracked for 37. What's the risk of regression due to this bug? Can we consider this for uplift for the 37 RC?

Comment 9

[:kip (Kearwood Gilbert)]

(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #8)
> I marked this as check-in needed. Please clear that if this actually isn't
> ready to land.
> 
> This bug fixes bug 1073056, which is tracked for 37. What's the risk of
> regression due to this bug? Can we consider this for uplift for the 37 RC?

This is ready to land.  I was hoping to add a reftest; however, the manner in
which reftest captures screenshots was masking this issue.

I would consider the risk of regression low to medium.  The patch has a notable effect on the bounding box calculations used to invalidate painting of 3d transformed layers with perspective.  Without this fix, these are quite broken, and I expect little effect outside of these cases.

Comment 10

[Lawrence Mandel [:lmandel] (use needinfo)]

The driver for fixing this bug in 37 is bug 1073056, which has shipped in 3 releases already. As we're at the RC point in the cycle, I think we should wait for 38. Please do get your reftest working and get this landed, preferably before Monday when 38 merges to Beta.(In reply to :kip (Kearwood Gilbert) from comment #9)

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/99153c410bad

Comment 12

[:kip (Kearwood Gilbert)]

Attachment: Bug 1035611 - Part 2: Test to ensure that transformed rects crossing the w=0 plane are clipped correctly

- An element is transformed such that its inverse projection transform
  results in a bounding box reaching an infinite / vanishing point.

Comment 13

[:kip (Kearwood Gilbert)]

Pushed to try to verify that reftest works on all platforms:

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

Comment 14

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8582702 [details] [diff] [review]
Bug 1035611 - Part 2: Test to ensure that transformed rects crossing the w=0 plane are clipped correctly

Review of attachment 8582702 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/reftests/transform/1035611-1.html
@@ +16,5 @@
> +	  width: 250px;
> +	  height: 100px;
> +	  perspective: 125px;
> +	}
> +	

Whitespace.

::: layout/reftests/transform/reftest.list
@@ +115,5 @@
>  # Bugs
>  == 601894-1.html 601894-ref.html
>  == 601894-2.html 601894-ref.html
>  == 830299-1.html 830299-1-ref.html
> +== 1035611-1.html 1035611-1-ref.html

This should probably go in the transform-3d folder, not transform.

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/99153c410bad

Comment 16

[:kip (Kearwood Gilbert)]

The reftest is not passing when using the Basic / Non-Accelerated compositor, likely due to Bug 1072898.

Comment 17

[:kip (Kearwood Gilbert)]

Attachment: Bug 1035611 - Part 2: Test to ensure that transformed rects crossing the w=0 plane are clipped correctly (v2 Patch),r=mattwoodrow

- An element is transformed such that its inverse projection transform
results in a bounding box reaching an infinite / vanishing point.

v2 Patch:
- Removed excess whitespace
- Moved reftest from layout/reftests/transform to layout/reftests/transform-3d
- Added fails-if(!layersGPUAccelerated) to reftest.list with a comment directing to Bug 1072898 which will need to be fixed to pass this test with HWA disabled.

Comment 18

[:kip (Kearwood Gilbert)]

Attachment: Bug 1035611 - Part 2: Test to ensure that transformed rects crossing the w=0 plane are clipped correctly (v3 Patch),r=mattwoodrow

- An element is transformed such that its inverse projection transform
results in a bounding box reaching an infinite / vanishing point.

v3 Patch:
- Removed extraneous blank line at the end of reftest.list

Comment 19

[:kip (Kearwood Gilbert)]

To ensure that the fails-if(!layersGPUAccelerated) condition catched all failures of the reftest, I have pushed to try again for all reftests:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6663c15cb104

Comment 20

[:kip (Kearwood Gilbert)]

Reftest now ready to commit.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/41ad91ce7cca

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/41ad91ce7cca

Comment 24

[Sylvestre Ledru [:Sylvestre]]

Kip, could you fill an uplift request to beta? thanks

Comment 25

[:kip (Kearwood Gilbert)]

Comment on attachment 8577523 [details] [diff] [review]
Bug 1035611 - Part 1: Updated Matrix4x4::ProjectRectBounds to properly handle infinite values when untransformed rects cross the w=0 plane

Approval Request Comment
[Feature/regressing bug #]:1035611
[User impact if declined]: CSS Transformed Elements with perspective may appear black when its inverse projection transform results in a bounding box reaching an infinite / vanishing point.
[Describe test coverage new/current, TreeHerder]: Implemented a reftest that reproduces the failure case.
[Risks and why]: Risk is moderate, with the patch changing a function that affects all 3d transformed elements.
[String/UUID change made/needed]: N/A

Comment 26

[:kip (Kearwood Gilbert)]

Comment on attachment 8583312 [details] [diff] [review]
Bug 1035611 - Part 2: Test to ensure that transformed rects crossing the w=0 plane are clipped correctly (v3 Patch),r=mattwoodrow

Approval Request Comment
[Feature/regressing bug #]:1035611
[User impact if declined]: CSS Transformed Elements with perspective may appear black when its inverse projection transform results in a bounding box reaching an infinite / vanishing point.
[Describe test coverage new/current, TreeHerder]: Implemented a reftest that reproduces the failure case.
[Risks and why]: Risk is moderate, with the patch changing a function that affects all 3d transformed elements.
[String/UUID change made/needed]: N/A

Comment 27

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8577523 [details] [diff] [review]
Bug 1035611 - Part 1: Updated Matrix4x4::ProjectRectBounds to properly handle infinite values when untransformed rects cross the w=0 plane

Sorry kip, with a moderate risk, I don't think we can take it in 38 :/ Sorry for the useless ping.