[non-e10s] CSS Transformed Element with Perspective is Culled incorrectly

VERIFIED FIXED in Firefox 43

Status

()

defect
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: kip, Assigned: kip)

Tracking

({regression})

Trunk
mozilla44
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 ?, firefox43+ fixed, firefox44 verified)

Details

()

Attachments

(3 attachments, 2 obsolete attachments)

When the moving screenshot on the "phone" is scrolled partially out of the top of the window's viewport, it is turns black.

This behavior regressed together with the alignment issue identified in Bug 1204657, which was fixed in Bug 1204824.  It is not yet confirmed if the regression causing the black rendering was regressed by the same patch that regressed the alignment issue.
I witnessed this issue on OSX 10.11 (GM Candidate) with 44.0a1 (2015-09-21)
I'm seeing a couple different issues here:
1) With e10s and apz on I get a stretching/jank issue but no black screen
2) With e10s on and apz off the stretching/jank goes away but still no black screen
3) With e10s off the black screen issue appears

Testing with Firefox 44.0a1 2015-09-22 on a Macbook Pro 13" Retina Late-2013 with El Capitan.
The black rendering only occurred with e10s disabled and only during animation.
While investigating the regression I encountered a flickering/misalignment issue. I'm not sure if it's a different symptom of the same issue so I'll leave that range here too.

Regression range for flickering/misalignment issue:
> mozilla-central: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e10e2e8d8bf2&tochange=5787d784485e
> mozilla-inbound: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=dc07aa95d2c91a5ffd5bd10773e159a21e16102f&tochange=7674044400c834c9ddddd981d3b525388beeb6d5
This would appear to have been introduced by bug 1170966.

I'm still investigating if the flickering/misalignment went away at some point and the black screen is a new issue, or if we're dealing with the same issue.
Flags: needinfo?(hshih)
Summary: CSS Transformed Element with Perspective is Culled incorrectly → [non-e10s] CSS Transformed Element with Perspective is Culled incorrectly
Posted video css3d.mp4
I test with nightly 44.0a1(2015-09-22) with e10s off, but I still can't see the black screen for the link https://stripe.com/relay .

Anthony, is my str correct(only show the bottom 1/4 of the phone)?
Flags: needinfo?(hshih) → needinfo?(anthony.s.hughes)
https://youtu.be/Pf0CZE0Vh1o shows the stretch/jank with apz on and the misalignment when its turned off (with e10s off as well).
Flags: needinfo?(anthony.s.hughes)
I still can't see the black image here with nightly 44.0a1(2015-09-23).
Here is my testing video. The apz and e10s are off.
Do I miss something?

I'm confused by comment 4 and comment 5. Which one affect the black issue?

My video:
https://www.youtube.com/watch?v=GnCQf5jTVec
Flags: needinfo?(anthony.s.hughes)
This patch corrects the issue.  I'll do some sanity checks then upload to review board.

This also corrects Bug 1205738.
Flags: needinfo?(kgilbert)
See Also: → 1205738
Bug 1207326 - Part 1: Correct projection clipping rectangle
- The clipping rectangle used in a call to Matrix4x4::TransformAndClipRect
  within CompositorOGL::DrawQuad is now using mRenderBound, with an
  offset applied consistent with use later in the function.
Attachment #8665607 - Flags: review?(matt.woodrow)
Comment on attachment 8665607 [details]
MozReview Request: Bug 1207326 - Part 1: Correct projection clipping rectangle

Bug 1207326 - Part 1: Correct projection clipping rectangle
- The clipping rectangle used in a call to Matrix4x4::TransformAndClipRect
  within CompositorOGL::DrawQuad is now using mRenderBound, with an
  offset applied consistent with use later in the function.
Comment on attachment 8665607 [details]
MozReview Request: Bug 1207326 - Part 1: Correct projection clipping rectangle

https://reviewboard.mozilla.org/r/20265/#review18241

::: gfx/layers/opengl/CompositorOGL.cpp:995
(Diff revision 1)
> +  Rect destRect = aTransform.TransformAndClipBounds(aRect, renderBound);

I don't understand this.

mRenderBound is the size of the destination surface, aClipRect is the post-transform clip to apply to the quad.

If anything we should take the intersection of these two things.
Attachment #8665607 - Flags: review?(matt.woodrow)
Comment on attachment 8665607 [details]
MozReview Request: Bug 1207326 - Part 1: Correct projection clipping rectangle

Bug 1207326 - Part 1: Correct projection clipping rectangle
- The clipping rectangle used in a call to Matrix4x4::TransformAndClipRect
  within CompositorOGL::DrawQuad is now using mRenderBound, with an
  offset applied consistent with use later in the function.
Attachment #8665607 - Flags: review?(matt.woodrow)
Comment on attachment 8665494 [details] [diff] [review]
Bug 1207326 - Part 1: Correct projection clipping rectangle

This patch is obsolete.  Please see the MozReview uploaded patch instead.
Attachment #8665494 - Attachment is obsolete: true
(In reply to Matt Woodrow (:mattwoodrow) from comment #13)
> Comment on attachment 8665607 [details]
> MozReview Request: Bug 1207326 - Part 1: Correct projection clipping
> rectangle
> 
> https://reviewboard.mozilla.org/r/20265/#review18241
> 
> ::: gfx/layers/opengl/CompositorOGL.cpp:995
> (Diff revision 1)
> > +  Rect destRect = aTransform.TransformAndClipBounds(aRect, renderBound);
> 
> I don't understand this.
> 
> mRenderBound is the size of the destination surface, aClipRect is the
> post-transform clip to apply to the quad.
> 
> If anything we should take the intersection of these two things.

Thanks for reviewing this, Matt.

Prior to the patch that regressed this issue, the destRect represented a transformed bounds that was not clipped (and did not correctly handle projection matrices).  The intent was to clip the transformed bounds to a rectangle just to avoid having infinite values in the event that the transformed layer crosses the w=0 plane.  This is done with the TransformAndClipBounds function; however, aClipRect did not include the offset and could cause the code following to erraneously cull the quad.

I agree with your assessment; however, as using the intersection of the clipping rectangle and mRenderBound enables more aggressive culling (and potential performance gains).  Also, it will result in better accuracy for mPixelsFilled, calculated from destRect.

I have updated the patch with your suggestion and pushed to try as an added sanity check.
(In reply to :kip (Kearwood Gilbert) from comment #17)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=103bc2e525e4

This try push includes the changes described in Comment 13 and Comment 16.
Comment on attachment 8665607 [details]
MozReview Request: Bug 1207326 - Part 1: Correct projection clipping rectangle

https://reviewboard.mozilla.org/r/20265/#review18247

Makes sense, thanks for the explanation.
Attachment #8665607 - Flags: review?(matt.woodrow) → review+
(In reply to Jerry Shih[:jerry] (UTC+8) from comment #8)
> I still can't see the black image here with nightly 44.0a1(2015-09-23).
> Here is my testing video. The apz and e10s are off.
> Do I miss something?
> 
> I'm confused by comment 4 and comment 5. Which one affect the black issue?

Comment 4 is the regression window for the issue where having APZ enabled causes stretching and jank.
Comment 5 is the regression window for the issue where having e10s disabled causes a black screen on the phone image.

I'll retest this once Kip's changes land in Firefox Nightly.
Flags: needinfo?(anthony.s.hughes)
This test is not yet working within the reftest framework -- If I add reftest-wait and a long setTimeout, I can see that the reftest is displaying the failure (blank page); however, the base64 encoded image in the logs are showing the result expected for a pass
Comment on attachment 8665607 [details]
MozReview Request: Bug 1207326 - Part 1: Correct projection clipping rectangle

Bug 1207326 - Part 1: Correct projection clipping rectangle
- The clipping rectangle used in a call to Matrix4x4::TransformAndClipRect
  within CompositorOGL::DrawQuad is now using mRenderBound, with an
  offset applied consistent with use later in the function.
Bug 1207326 - Part 2: Add reftest
Attachment #8666164 - Flags: review?(jmuizelaar)
(In reply to :kip (Kearwood Gilbert) from comment #22)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=424fa84778c4

This try push includes both the reftest and the fix.
(In reply to :kip (Kearwood Gilbert) from comment #23)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=0fba164a5dc9

This try push includes the reftest, but not the test.

While running the reftest locally on OSX 10.11, I was able to see the failure detected; however, the screenshot grabbed by the reftest framework represented the non-failure case.

I added a reftest-wait and a setTimeout() to slow down the test to see that it was working when running as a reftest.  It appears that the screenshot process itself masks the bug on OSX 10.11.

Hopefully other platforms will be able to detect the failure.
Comment on attachment 8666158 [details]
Test the reproduces the issue, with or without e10s enabled.  If you see a box, the test has passed.  If the page is blank, the test has failed.

Please see the patch in the mozreview instead.
Attachment #8666158 - Attachment is obsolete: true
Comment on attachment 8666164 [details]
MozReview Request: Bug 1207326 - Part 2: Add reftest

https://reviewboard.mozilla.org/r/20447/#review18369
Attachment #8666164 - Flags: review?(jmuizelaar) → review+
Blocks: 1205738
Blocks: 1205515
Blocks: 1205449
Blocks: 1204819
Comment on attachment 8665607 [details]
MozReview Request: Bug 1207326 - Part 1: Correct projection clipping rectangle

Approval Request Comment
[Feature/regressing bug #]: 1157984
[User impact if declined]: Elements may be missing, flicker or appear black.  Examples identified in Bug 1207326, Bug 1205738, Bug 1205515, Bug 1205449, and Bug 1204819.  This affects e10s, non-e10s, and multiple platforms.
[Describe test coverage new/current, TreeHerder]: Created reftest, pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=424fa84778c4
[Risks and why]: Moderate risk.  Affects platforms using the OpenGL compositor.  Regressions to watch for would include culling errors causing objects to be missing or black.  Without fix, popular sites such as Google Maps and B2G's camera application are affected.
[String/UUID change made/needed]:
Attachment #8665607 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/c90bfcd824e2
https://hg.mozilla.org/mozilla-central/rev/cc2beb2c6e3c
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Duplicate of this bug: 1204819
Flagging myself to test this tomorrow.
Flags: needinfo?(anthony.s.hughes)
The black screen and misalignment issues on the phone image have both been resolved. However, the jankiness with APZ on still exists. I'll file a follow-up bug report for that.
Status: RESOLVED → VERIFIED
Flags: needinfo?(anthony.s.hughes)
[Tracking Requested - why for this release]: See comment 31
Comment on attachment 8665607 [details]
MozReview Request: Bug 1207326 - Part 1: Correct projection clipping rectangle

Approved for aurora uplift; a little risky but important to fix
Attachment #8665607 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
kip, do you want to also uplift the test to aurora?
Flags: needinfo?(kgilbert)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #38)
> kip, do you want to also uplift the test to aurora?

Yes, please also uplift the test.
Flags: needinfo?(kgilbert)
Comment on attachment 8666164 [details]
MozReview Request: Bug 1207326 - Part 2: Add reftest

Approved for uplift to aurora (see comment 31)
Attachment #8666164 - Flags: approval-mozilla-aurora+
Kip, with 42 being a "big" release, something like this would be a regression we'd like to avoid.  How comfortable are you with us attempting a beta uplift?
Flags: needinfo?(kgilbert)
Duplicate of this bug: 1205515
Clearing old need-info.  If you still need info from me on this, please ask.
Flags: needinfo?(kgilbert)
You need to log in before you can comment on or make changes to this bug.