Closed Bug 1035611 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Graphics, defect)

29 Branch
x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox37 + wontfix
firefox38 + wontfix
firefox39 --- fixed

People

(Reporter: mattwoodrow, Assigned: kip)

References

Details

Attachments

(2 files, 2 obsolete files)

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.
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: nobody → kgilbert
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.
- 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)
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)
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)
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+
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)
(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)
- 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)
Pushed to try to verify that reftest works on all platforms:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca0db173685d
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
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
The reftest is not passing when using the Basic / Non-Accelerated compositor, likely due to Bug 1072898.
- 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
- 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
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
Reftest now ready to commit.
Keywords: checkin-needed
Duplicate of this bug: 1073056
Kip, could you fill an uplift request to beta? thanks
Flags: needinfo?(kgilbert)
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?
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-
Depends on: 1187199
You need to log in before you can comment on or make changes to this bug.