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

Status: VERIFIED FIXED

(Core :: Graphics: Layers, defect)

Description

[:kip (Kearwood Gilbert)]

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.

Comment 1

[:kip (Kearwood Gilbert)]

I witnessed this issue on OSX 10.11 (GM Candidate) with 44.0a1 (2015-09-21)

Comment 2

[u279076]

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.

Comment 3

[:kip (Kearwood Gilbert)]

The black rendering only occurred with e10s disabled and only during animation.

Comment 4

[u279076]

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.

Comment 5

[u279076]

Here is the regression window for the blackscreen issue:
> mozilla-central: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=fb720c90eb49590ba55bf52a8a4826ffff9f528b&tochange=a6786bf8d71d4cf40c3d40e06d8e3c9866863475
> mozilla-inbound: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=5b59f670cf52f7c4bb5f2fde2c4174d64f7a1f7e&tochange=7d3ce51b85e3a9e79582d947bfdf3708c322328b
This blames bug 1157984 - needinfo :kip to look at that.

Comment 6

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: 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)?

Comment 7

[u279076]

https://youtu.be/Pf0CZE0Vh1o shows the stretch/jank with apz on and the misalignment when its turned off (with e10s off as well).

Comment 8

[Jerry Shih[:jerry] (UTC+8) (inactive)]

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

Comment 9

[:kip (Kearwood Gilbert)]

Attachment: Bug 1207326 - Part 1: Correct projection clipping rectangle

This patch corrects the issue.  I'll do some sanity checks then upload to review board.

This also corrects Bug 1205738.

Comment 10

[:kip (Kearwood Gilbert)]

Attachment: 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 11

[:kip (Kearwood Gilbert)]

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

Comment 12

[:kip (Kearwood Gilbert)]

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 13

[Matt Woodrow (:mattwoodrow)]

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.

Comment 14

[:kip (Kearwood Gilbert)]

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 15

[:kip (Kearwood Gilbert)]

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.

Comment 16

[:kip (Kearwood Gilbert)]

(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.

Comment 17

[:kip (Kearwood Gilbert)]

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

Comment 18

[:kip (Kearwood Gilbert)]

(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 19

[Matt Woodrow (:mattwoodrow)]

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.

Comment 20

[u279076]

(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.

Comment 21

[:kip (Kearwood Gilbert)]

Attachment: 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.

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 22

[:kip (Kearwood Gilbert)]

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

Comment 23

[:kip (Kearwood Gilbert)]

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

Comment 24

[:kip (Kearwood Gilbert)]

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 25

[:kip (Kearwood Gilbert)]

Attachment: MozReview Request: Bug 1207326 - Part 2: Add reftest

Bug 1207326 - Part 2: Add reftest

Comment 26

[:kip (Kearwood Gilbert)]

(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.

Comment 27

[:kip (Kearwood Gilbert)]

(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 28

[:kip (Kearwood Gilbert)]

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.

Comment 29

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8666164 [details]
MozReview Request: Bug 1207326 - Part 2: Add reftest

https://reviewboard.mozilla.org/r/20447/#review18369

Comment 30

[:kip (Kearwood Gilbert)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c90bfcd824e2df8e25e9b2eedf690ffac2df38fd
Bug 1207326 - Part 1: Correct projection clipping rectangle,r=matt.woodrow

https://hg.mozilla.org/integration/mozilla-inbound/rev/cc2beb2c6e3c4e142847ed81408c7019f6ac8ed9
Bug 1207326 - Part 2: Add reftest,r=jmuizelaar

Comment 31

[:kip (Kearwood Gilbert)]

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]:

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c90bfcd824e2
https://hg.mozilla.org/mozilla-central/rev/cc2beb2c6e3c

Comment 34

[u279076]

Flagging myself to test this tomorrow.

Comment 35

[u279076]

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.

Comment 36

[Jet Villegas (inactive)]

[Tracking Requested - why for this release]: See comment 31

Comment 37

[Liz Henry (:lizzard) (relman/hg->git project)]

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

Comment 38

[Liz Henry (:lizzard) (relman/hg->git project)]

kip, do you want to also uplift the test to aurora?

Comment 39

[:kip (Kearwood Gilbert)]

(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.

Comment 40

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8666164 [details]
MozReview Request: Bug 1207326 - Part 2: Add reftest

Approved for uplift to aurora (see comment 31)

Comment 41

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c4cd3a110e3f
https://hg.mozilla.org/releases/mozilla-aurora/rev/0def0c7e4d67

Comment 42

[Milan Sreckovic [:milan] (needinfo for best results)]

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?

Comment 44

[:kip (Kearwood Gilbert)]

Clearing old need-info.  If you still need info from me on this, please ask.