Bug 84059 - Re-implement backFaceVisibility to avoid dealing with perspective w < 0 problem
Summary: Re-implement backFaceVisibility to avoid dealing with perspective w < 0 problem
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Shawn Singh
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-16 12:07 PDT by Shawn Singh
Modified: 2012-04-26 16:59 PDT (History)
6 users (show)

See Also:


Attachments
Patch (10.56 KB, patch)
2012-04-16 14:21 PDT, Shawn Singh
no flags Details | Formatted Diff | Diff
Patch (9.50 KB, patch)
2012-04-17 15:17 PDT, Shawn Singh
no flags Details | Formatted Diff | Diff
Patch (12.26 KB, patch)
2012-04-18 12:39 PDT, Shawn Singh
no flags Details | Formatted Diff | Diff
Patch (12.32 KB, patch)
2012-04-23 11:38 PDT, Shawn Singh
no flags Details | Formatted Diff | Diff
Patch for landing (12.68 KB, patch)
2012-04-26 13:20 PDT, Shawn Singh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Shawn Singh 2012-04-16 12:07:48 PDT
When CCLayerTreeHostCommon checks back-face visibility by transforming a normal, it currently uses un-clipped transforms that can be incorrect on perspective.
Comment 1 Shawn Singh 2012-04-16 14:21:08 PDT
Created attachment 137401 [details]
Patch

tested on osx layout tests and unit tests, no obvious regressions
Comment 2 Vangelis Kokkevis 2012-04-16 14:52:30 PDT
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.
Comment 3 Shawn Singh 2012-04-16 15:58:15 PDT
Comment on attachment 137401 [details]
Patch

Thanks for that information!

Removing r=?  in the meantime while I look into that.
Comment 4 Shawn Singh 2012-04-17 12:43:38 PDT
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 =)
Comment 5 Shawn Singh 2012-04-17 14:42:25 PDT
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.
Comment 6 Shawn Singh 2012-04-17 15:17:46 PDT
Created attachment 137618 [details]
Patch
Comment 7 Vangelis Kokkevis 2012-04-17 21:40:08 PDT
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?
Comment 8 Shawn Singh 2012-04-18 09:50:40 PDT
(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 =)
Comment 9 Shawn Singh 2012-04-18 12:39:02 PDT
Created attachment 137740 [details]
Patch
Comment 10 Shawn Singh 2012-04-18 12:42:26 PDT
(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.
Comment 11 Shawn Singh 2012-04-20 13:49:38 PDT
Still waiting for review - thanks in advance =)
Comment 12 Dana Jansens 2012-04-20 14:02:40 PDT
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?
Comment 13 Dana Jansens 2012-04-20 14:03:46 PDT
> Yay for

doing this in TransformationMatrix :) Hah.. it left the start of a thought in my browser cookies.
Comment 14 Shawn Singh 2012-04-23 11:38:46 PDT
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!
Comment 15 Adrienne Walker 2012-04-25 09:51:54 PDT
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.
Comment 16 Shawn Singh 2012-04-25 10:19:13 PDT
(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?
Comment 17 Shawn Singh 2012-04-25 10:20:50 PDT
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.
Comment 18 Vangelis Kokkevis 2012-04-25 10:30:04 PDT
> 
> 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.
Comment 19 Adrienne Walker 2012-04-26 11:33:23 PDT
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?
Comment 20 Shawn Singh 2012-04-26 11:43:20 PDT
(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.
Comment 21 Shawn Singh 2012-04-26 13:20:10 PDT
Created attachment 139055 [details]
Patch for landing
Comment 22 WebKit Review Bot 2012-04-26 16:59:43 PDT
Comment on attachment 139055 [details]
Patch for landing

Clearing flags on attachment: 139055

Committed r115386: <http://trac.webkit.org/changeset/115386>
Comment 23 WebKit Review Bot 2012-04-26 16:59:49 PDT
All reviewed patches have been landed.  Closing bug.