Bug 84059 – Re-implement backFaceVisibility to avoid dealing with perspective w < 0 problem

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Shawn Singh]

When CCLayerTreeHostCommon checks back-face visibility by transforming a normal, it currently uses un-clipped transforms that can be incorrect on perspective.

[Shawn Singh]

Created attachment 137401 [details]
Patch

tested on osx layout tests and unit tests, no obvious regressions

[Vangelis Kokkevis]

There's an interesting discussion in this thread:
http://lists.w3.org/Archives/Public/public-css-bugzilla/2012Mar/0078.html

which mentions that Chrome's implementation of backface visibility may be wrong altogether. The spec suggests a very straightforward implementation.  We should try it out!

There's also a few good examples in that thread that we should use for testing.

[Shawn Singh]

Comment on attachment 137401 [details]
Patch

Thanks for that information!

Removing r=?  in the meantime while I look into that.

[Shawn Singh]

Update:

It is possible to make a completely equivalent, more straightforward implementation, we just have to check if inverseTransform().m33() < 0.  

Note, however, the W3C spec seems to have an incorrect rationale about how to compute backface culling  (we should bring this up to them), and so our implementation is not exactly what the W3C spec suggests.  I think this is true of firefox and safari too; it should be just the spec that needs to be corrected.

The spec did have another recent adjustment that chromium has to adapt to - instead of using the screen space transform, we should be using the draw transform.  This way, if the layer is in a preserves-3d setting, it will compute visibility with the transform that projects it to the target surface.  Otherwise, if the layer is not in a 3d mode, it should use the layer's own matrix (which for backface visibility purposes, is equivalent to the draw transform).

New patch coming soon =)

[Shawn Singh]

I will keep the transforms as screenSpaceTransforms for now.  Pushing that portion of the fix to https://bugs.webkit.org/show_bug.cgi?id=84195.

[Shawn Singh]

Created attachment 137618 [details]
Patch

[Vangelis Kokkevis]

Comment on attachment 137618 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=137618&action=review

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:45
> +bool CCLayerTreeHostCommon::backFaceIsVisible(const TransformationMatrix& transform)

Wouldn't it be more consistent with the rest of the static helper methods in this file to keep this as a standalone function instead of a method of LTHC ?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:60
> +    if (transform.isInvertible()) {

Have you looked into avoiding doing an full inverse and rather just computing m33() directly?

[Shawn Singh]

(In reply to comment #7)
> (From update of attachment 137618 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=137618&action=review
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:45
> > +bool CCLayerTreeHostCommon::backFaceIsVisible(const TransformationMatrix& transform)
> 
> Wouldn't it be more consistent with the rest of the static helper methods in this file to keep this as a standalone function instead of a method of LTHC ?

Actually, I was thinking we should make all other helper functions members of CCLayerTreeHostCommon, too.  (not LTHC)

The reason is for testability.  If we make it a local hidden static, the unit test I wrote would explode and not be worth writing.  As it is, however, its simple and isolated (and accessible) so its very easy to get that additional blanket of test coverage.

Is that OK with you and Enne?


> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:60
> > +    if (transform.isInvertible()) {
> 
> Have you looked into avoiding doing an full inverse and rather just computing m33() directly?

No I didn't...  I'll look into it and submit a new patch if it works out =)

[Shawn Singh]

Created attachment 137740 [details]
Patch

[Shawn Singh]

(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 137618 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=137618&action=review
> > 
> > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:45
> > > +bool CCLayerTreeHostCommon::backFaceIsVisible(const TransformationMatrix& transform)
> > 
> > Wouldn't it be more consistent with the rest of the static helper methods in this file to keep this as a standalone function instead of a method of LTHC ?
> 
> Actually, I was thinking we should make all other helper functions members of CCLayerTreeHostCommon, too.  (not LTHC)
> 
> The reason is for testability.  If we make it a local hidden static, the unit test I wrote would explode and not be worth writing.  As it is, however, its simple and isolated (and accessible) so its very easy to get that additional blanket of test coverage.
> 
> Is that OK with you and Enne?
> 

The new patch makes this a non-issue anyway.  I had to put isBackFaceVisible() in TransformationMatrix to access the hidden static determinant functionality there, or else there was going to be a lot of redundant, error-prone code.

[Shawn Singh]

Still waiting for review - thanks in advance =)

[Dana Jansens]

Comment on attachment 137740 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=137740&action=review

Yay for

> Source/WebCore/ChangeLog:3
> +        [chromium] re-implement backFaceVisibility to avoid dealing with perspective w<0 problem

Probably need to change this. It's not [chromium] specific anymore either right?

[Dana Jansens]

> Yay for

doing this in TransformationMatrix :) Hah.. it left the start of a thought in my browser cookies.

[Shawn Singh]

Created attachment 138393 [details]
Patch

Still waiting for official review - this latest patch just re-names the bug and updates the ChangeLog as per Dana's feedback.  Thanks in advance!

[Adrienne Walker]

Comment on attachment 138393 [details]
Patch

This feels like deja vu: https://bugs.webkit.org/show_bug.cgi?id=66114

I don't think you can just transform the normal when there's a perspective transform involved.

[Shawn Singh]

(In reply to comment #15)
> (From update of attachment 138393 [details])
> This feels like deja vu: https://bugs.webkit.org/show_bug.cgi?id=66114
> 
> I don't think you can just transform the normal when there's a perspective transform involved.

Thanks for looking at this =)

I looked at 66114, and there is a difference between our mistake on that bug and this patch.  The difference is that in that bug we tried to use the transform.m33() which is incorrect.  In this patch we are using transform.inverse().m33() which I believe is correct.

While making this patch, I did an empirical test that showed (1) transform.m33() method is clearly not the same as our old cross-product method, but (2) when I did the same test for transform.inverse.m33(), the results seemed _exactly_ the same as our old cross-product method for any 3d CSS website I had tried.

I believe Vangelis also had gone through reasoning of why its correct during perspective transform, if he has anything to add?

[Shawn Singh]

Just to be clear, I do think its OK to transform a normal when there's a perspective involved.  Implicitly, we were doing the same thing with the cross product method, too.

[Vangelis Kokkevis]

> 
> I believe Vangelis also had gone through reasoning of why its correct during perspective transform, if he has anything to add?

Yes, I do agree that Shawn's new implementation (which transforms the normal by the inverse-transpose of the draw matrix) is the correct way to determine backface visibility and gets around the clipping issue that the previous implementation had.

[Adrienne Walker]

Comment on attachment 138393 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=138393&action=review

Ah, I see.  The inverse does make all the difference.  Thanks for the explanation.

> Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:364
> +    bool isBackFaceVisible() const;
> +

This definitely needs a comment.  The back face of what?  With respect to what?

[Shawn Singh]

(In reply to comment #19)
> (From update of attachment 138393 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138393&action=review
> 
> Ah, I see.  The inverse does make all the difference.  Thanks for the explanation.
> 
> > Source/WebCore/platform/graphics/transforms/TransformationMatrix.h:364
> > +    bool isBackFaceVisible() const;
> > +
> 
> This definitely needs a comment.  The back face of what?  With respect to what?

Awesome - thanks!   I'll add the comment and land it through the commit queue.

[Shawn Singh]

Created attachment 139055 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 139055 [details]
Patch for landing

Clearing flags on attachment: 139055

Committed r115386: <http://trac.webkit.org/changeset/115386>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.