Bug 66114 – Computing world-space transform for LayerChromium and CCLayerImpl

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Shawn Singh]

Computing world-space transform for LayerChromium and CCLayerImpl

[Shawn Singh]

Created attachment 103703 [details]
Patch

[WebKit Review Bot]

Attachment 103703 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:11:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 3 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[James Robinson]

Looks like you need to configure your editor to expand tabs to spaces when editing ChangeLogs (for some reason my vim always gets confused when editing those).

[Shawn Singh]

Created attachment 103705 [details]
Patch

[Adrienne Walker]

(In reply to comment #3)
> Looks like you need to configure your editor to expand tabs to spaces when editing ChangeLogs (for some reason my vim always gets confused when editing those).

echo "autocmd BufEnter ChangeLog set expandtab" >> ~/.vimrc

[Shawn Singh]

Will re-submit a new patch soon that includes a test case and fixes a FIXME in LayerRendererChromium::drawLayer(...)

[Shawn Singh]

Created attachment 103999 [details]
Patch

[Adrienne Walker]

Comment on attachment 103999 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1087
> +        // FIXME: A more correct and efficient way to do this would be to transform
> +        //        a simple unit vector in the z-axis, instead of mapQuad.
> +        //        However, at this time there is no TransformationMatrix routine to transform mathematical
> +        //        vectors (where the 4th matrix column does not "translate" the vector, unlike points)

I don't understand this comment.

The normal of the layer in untransformed space is the z-axis (0, 0, 1, 0).  Multiplying that by a TransformationMatrix t into screen space gives you the vector (t.m13(), t.m23(), t.m33(), t.m43()).  To check if that vector is backfacing with respect to some plane, take the dot product of the normal of that plane with that vector.  In this case, we care about the xy plane, so the normal is the z-axis again.  Taking the dot product of these two vectors gives you the scalar quantity t.m33(), which if it's less than zero implies that the layer is backfacing with respect to the xy plane in screen space.

Is my math wrong here? You should just be able to simplify all of this with a "layer->worldSpaceDrawTransform().m33() < 0" test?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:223
> +    TransformationMatrix m_worldSpaceDrawTransform;

Bikeshedding nit: I know what you mean by "world space", but I don't think that's a useful term here.  It's also not a draw transform, because we're not drawing with it.  I think "screen space" or "device space" makes more sense to me.  Maybe jamesr has a preference here.

[Shawn Singh]

Created attachment 104062 [details]
Patch

[Shawn Singh]

1.  will use the efficient m33() instead of the entire mapQuad.  Then that "FIXME" comment is no longer necessary either.

2. jamesr, can you please suggest what transform terminology you feel is most clear?  We discussed and felt that m_screenSpaceTransform is best.

[James Robinson]

"screen space" is a little confusing as well, because sometimes we use "screen space" to refer to the currently bound framebuffer - for example the edge AA code does this.  Maybe screen space is correct for this and the edge AA code is wrong.

[Shawn Singh]

Created attachment 104088 [details]
Patch

[Shawn Singh]

OK, submitted a new patch that is ready for review.

[James Robinson]

Comment on attachment 104088 [details]
Patch

Great, looks good.

[WebKit Review Bot]

Comment on attachment 104088 [details]
Patch

Clearing flags on attachment: 104088

Committed r93186: <http://trac.webkit.org/changeset/93186>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Steve Block]

It looks like this change has caused LayoutTests/platform/chromium/compositing/backface-visibility-transformed.html to fail in GPU mode on multiple Chromium canary bots due to an image diff.

For example, on the WebKit Linux bot, the first failure is http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Linux/builds/11977

Note that this test is skipped in CPU mode ...
BUGCR15733 SKIP WONTFIX CPU : platform/chromium/compositing = PASS TIMEOUT FAIL
This means that the flakiness dashboard isn't very helpful.

Looking at the actual result from the above build, the image output is a red trapezoid, rather than a green one, so the test seems truly broken, rather than in need of a minor rebaseline.

[Steve Block]

Reverted r93186 for reason:

Breaks LayoutTests on Chromium canary bots

Committed r93201: <http://trac.webkit.org/changeset/93201>

[Adrienne Walker]

Sorry, my mistake.  The code in your 104062 attachment above is correct.  My math is wrong because you can't just consider the transformed normal when there's a perspective transform involved.  Reverting back to the original code that recalculates the normal of the transformed layer from the transformed vertices is the right way to do this.

[Shawn Singh]

Created attachment 104427 [details]
Patch

[Shawn Singh]

on previous patch, layout tests were giving false positives on cygwin;  fixed the cygwin problem in https://bugs.webkit.org/show_bug.cgi?id=47240

Fixed the patch and re-tested, hopefully it is correct now.

[James Robinson]

Comment on attachment 104427 [details]
Patch

R=me

[WebKit Review Bot]

Comment on attachment 104427 [details]
Patch

Clearing flags on attachment: 104427

Committed r93387: <http://trac.webkit.org/changeset/93387>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.