Bug 86870 - [chromium] add back-face visibility check for renderSurfaces
Summary: [chromium] add back-face visibility check for renderSurfaces
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: 84195
  Show dependency treegraph
 
Reported: 2012-05-18 10:49 PDT by Shawn Singh
Modified: 2012-05-18 15:28 PDT (History)
5 users (show)

See Also:


Attachments
Patch (6.99 KB, patch)
2012-05-18 12:05 PDT, Shawn Singh
no flags Details | Formatted Diff | Diff
added transformIsKnown check (7.30 KB, patch)
2012-05-18 13:30 PDT, Shawn Singh
no flags Details | Formatted Diff | Diff
Fixed comment and added useful helper function (8.74 KB, patch)
2012-05-18 14:30 PDT, Shawn Singh
enne: review+
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-05-18 10:49:24 PDT
This patch is related to https://bugs.webkit.org/show_bug.cgi?id=84195, but it might be convenient if this lands first, to reduce conflicts just in case we want to merge it back to m20.
I'm currently testing this patch, both by itself and in conjunction with https://bugs.webkit.org/show_bug.cgi?id=84195, and its passing all unit tests and layout tests everywhere.

The bug was that when a parent has preserves3D, but the layer does not, then it creates a renderSurface, and we do not correctly check backface visibility in that case.  This caused a few pages that use flip mechanisms to break
Comment 1 Shawn Singh 2012-05-18 12:05:11 PDT
Created attachment 142760 [details]
Patch
Comment 2 Shawn Singh 2012-05-18 12:06:19 PDT
Comment on attachment 142760 [details]
Patch

forgot to update the other unit test, will submit new patch in a bit
Comment 3 Vangelis Kokkevis 2012-05-18 12:23:32 PDT
Comment on attachment 142760 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:386
> +        if (layer->parent() && layer->parent()->preserves3D() && !layer->doubleSided() && combinedTransform.isBackFaceVisible())

Do we have to worry about combinedTransform's that are animated on the thread? It seems from:

https://bugs.webkit.org/show_bug.cgi?id=82370

that we always need to have a RS on the main tree for layers that could get one on the impl side.
Comment 4 Shawn Singh 2012-05-18 13:30:24 PDT
Created attachment 142777 [details]
added transformIsKnown check

ah, thanks... that is what was going wrong.  This patch now passes all unit tests, no obvious regression on layout tests.
Comment 5 Adrienne Walker 2012-05-18 13:43:22 PDT
Comment on attachment 142777 [details]
added transformIsKnown check

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:388
> +        // If this surface flattens its entire subtree, then we need to check back-face visibility before continuing with this surface.
> +        // Cannot early exit here, however, if the transform is animating and not known on the main thread.
> +        if (layer->parent() && layer->parent()->preserves3D() && transformToParentIsKnown(layer) && !layer->doubleSided() && combinedTransform.isBackFaceVisible())

Don't you also need to check !layer->preserves3D() to know that this layer flattens its subtree? I think a "flattensSubtreeToSurface()" helper might improve readability too.
Comment 6 Shawn Singh 2012-05-18 14:01:07 PDT
(In reply to comment #5)
> (From update of attachment 142777 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=142777&action=review
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:388
> > +        // If this surface flattens its entire subtree, then we need to check back-face visibility before continuing with this surface.
> > +        // Cannot early exit here, however, if the transform is animating and not known on the main thread.
> > +        if (layer->parent() && layer->parent()->preserves3D() && transformToParentIsKnown(layer) && !layer->doubleSided() && combinedTransform.isBackFaceVisible())
> 
> Don't you also need to check !layer->preserves3D() to know that this layer flattens its subtree? I think a "flattensSubtreeToSurface()" helper might improve readability too.

Actually the code is correct, its my comment that is misleading and wrong.   The comment should say "if this surface is already in a w3c rendering context, then we should check whether the back face is visible"

The full situation can be understood in the context of https://bugs.webkit.org/show_bug.cgi?id=84195.  Different transforms need to be used to check back-face visibility, depending on whether we're in a 3d rendering context or not.

I'll add the appropriate helper function and fix the comment.
Comment 7 Shawn Singh 2012-05-18 14:30:41 PDT
Created attachment 142784 [details]
Fixed comment and added useful helper function

Tested again manually, unit, layout, all seems good.
Comment 8 Adrienne Walker 2012-05-18 15:05:08 PDT
Comment on attachment 142784 [details]
Fixed comment and added useful helper function

R=me.
Comment 9 Shawn Singh 2012-05-18 15:27:30 PDT
Committed r117645: <http://trac.webkit.org/changeset/117645>
Comment 10 Shawn Singh 2012-05-18 15:28:00 PDT
(In reply to comment #8)
> (From update of attachment 142784 [details])
> R=me.

Thanks!