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

RESOLVED FIXED in Firefox 39

Status

()

Core
Graphics
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: mattwoodrow, Assigned: kip)

Tracking

29 Branch
mozilla39
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox37+ wontfix, firefox38+ wontfix, firefox39 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

4 years ago
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.
(Reporter)

Comment 1

4 years ago
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.
(Assignee)

Updated

3 years ago
Assignee: nobody → kgilbert
status-firefox37: --- → affected
status-firefox38: --- → affected
status-firefox39: --- → affected
(Assignee)

Updated

3 years ago
Blocks: 1073056
(Assignee)

Comment 2

3 years ago
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.
(Assignee)

Comment 3

3 years ago
Created 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

- 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.
Attachment #8577523 - Flags: review?(jacob.benoit.1)
(Assignee)

Comment 4

3 years ago
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 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.
Attachment #8577523 - Flags: review?(jacob.benoit.1)
(Assignee)

Comment 6

3 years ago
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.
Attachment #8577523 - Flags: review?(matt.woodrow)
(Reporter)

Comment 7

3 years ago
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!
Attachment #8577523 - Flags: review?(matt.woodrow) → review+
tracking-firefox37: --- → +
Keywords: checkin-needed
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?
Flags: needinfo?(kgilbert)
(Assignee)

Comment 9

3 years ago
(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.
Flags: needinfo?(kgilbert)
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)
status-firefox37: affected → wontfix
tracking-firefox38: --- → +
https://hg.mozilla.org/integration/mozilla-inbound/rev/99153c410bad
Keywords: checkin-needed
(Assignee)

Comment 12

3 years ago
Created attachment 8582702 [details] [diff] [review]
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.
Attachment #8582702 - Flags: review?(matt.woodrow)
(Assignee)

Comment 13

3 years ago
Pushed to try to verify that reftest works on all platforms:

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

Comment 14

3 years ago
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.
Attachment #8582702 - Flags: review?(matt.woodrow) → review+
https://hg.mozilla.org/mozilla-central/rev/99153c410bad
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox39: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
(Assignee)

Comment 16

3 years ago
The reftest is not passing when using the Basic / Non-Accelerated compositor, likely due to Bug 1072898.
(Assignee)

Comment 17

3 years ago
Created attachment 8583306 [details] [diff] [review]
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.
Attachment #8582702 - Attachment is obsolete: true
(Assignee)

Comment 18

3 years ago
Created 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

- 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
Attachment #8583306 - Attachment is obsolete: true
(Assignee)

Comment 19

3 years ago
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
(Assignee)

Comment 20

3 years ago
Reftest now ready to commit.
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/41ad91ce7cca
Flags: in-testsuite+
Keywords: checkin-needed
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1073056
https://hg.mozilla.org/mozilla-central/rev/41ad91ce7cca
Kip, could you fill an uplift request to beta? thanks
Flags: needinfo?(kgilbert)
(Assignee)

Comment 25

3 years ago
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
Flags: needinfo?(kgilbert)
Attachment #8577523 - Flags: approval-mozilla-beta?
(Assignee)

Comment 26

3 years ago
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
Attachment #8583312 - Flags: approval-mozilla-beta?
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.
Attachment #8577523 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #8583312 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
status-firefox38: affected → wontfix

Updated

2 years ago
Depends on: 1187199
You need to log in before you can comment on or make changes to this bug.