Bug 66114 - Computing world-space transform for LayerChromium and CCLayerImpl
Summary: Computing world-space transform for LayerChromium and CCLayerImpl
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-08-11 16:33 PDT by Shawn Singh
Modified: 2011-08-18 22:45 PDT (History)
5 users (show)

See Also:


Attachments
Patch (9.57 KB, patch)
2011-08-11 16:42 PDT, Shawn Singh
no flags Details | Formatted Diff | Diff
Patch (9.59 KB, patch)
2011-08-11 16:57 PDT, Shawn Singh
no flags Details | Formatted Diff | Diff
Patch (13.35 KB, patch)
2011-08-15 20:24 PDT, Shawn Singh
no flags Details | Formatted Diff | Diff
Patch (16.07 KB, patch)
2011-08-16 10:06 PDT, Shawn Singh
no flags Details | Formatted Diff | Diff
Patch (16.35 KB, patch)
2011-08-16 14:14 PDT, Shawn Singh
no flags Details | Formatted Diff | Diff
Patch (15.87 KB, patch)
2011-08-18 17:42 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 2011-08-11 16:33:29 PDT
Computing world-space transform for LayerChromium and CCLayerImpl
Comment 1 Shawn Singh 2011-08-11 16:42:24 PDT
Created attachment 103703 [details]
Patch
Comment 2 WebKit Review Bot 2011-08-11 16:45:39 PDT
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.
Comment 3 James Robinson 2011-08-11 16:56:12 PDT
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).
Comment 4 Shawn Singh 2011-08-11 16:57:28 PDT
Created attachment 103705 [details]
Patch
Comment 5 Adrienne Walker 2011-08-11 16:59:12 PDT
(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
Comment 6 Shawn Singh 2011-08-12 13:49:45 PDT
Will re-submit a new patch soon that includes a test case and fixes a FIXME in LayerRendererChromium::drawLayer(...)
Comment 7 Shawn Singh 2011-08-15 20:24:09 PDT
Created attachment 103999 [details]
Patch
Comment 8 Adrienne Walker 2011-08-16 10:04:58 PDT
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.
Comment 9 Shawn Singh 2011-08-16 10:06:18 PDT
Created attachment 104062 [details]
Patch
Comment 10 Shawn Singh 2011-08-16 10:54:49 PDT
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.
Comment 11 James Robinson 2011-08-16 13:06:00 PDT
"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.
Comment 12 Shawn Singh 2011-08-16 14:14:52 PDT
Created attachment 104088 [details]
Patch
Comment 13 Shawn Singh 2011-08-16 14:16:14 PDT
OK, submitted a new patch that is ready for review.
Comment 14 James Robinson 2011-08-16 16:47:59 PDT
Comment on attachment 104088 [details]
Patch

Great, looks good.
Comment 15 WebKit Review Bot 2011-08-16 18:24:57 PDT
Comment on attachment 104088 [details]
Patch

Clearing flags on attachment: 104088

Committed r93186: <http://trac.webkit.org/changeset/93186>
Comment 16 WebKit Review Bot 2011-08-16 18:25:02 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 Steve Block 2011-08-17 04:12:08 PDT
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.
Comment 18 Steve Block 2011-08-17 04:43:31 PDT
Reverted r93186 for reason:

Breaks LayoutTests on Chromium canary bots

Committed r93201: <http://trac.webkit.org/changeset/93201>
Comment 19 Adrienne Walker 2011-08-18 14:20:44 PDT
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.
Comment 20 Shawn Singh 2011-08-18 17:42:49 PDT
Created attachment 104427 [details]
Patch
Comment 21 Shawn Singh 2011-08-18 17:47:21 PDT
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.
Comment 22 James Robinson 2011-08-18 21:45:41 PDT
Comment on attachment 104427 [details]
Patch

R=me
Comment 23 WebKit Review Bot 2011-08-18 22:45:05 PDT
Comment on attachment 104427 [details]
Patch

Clearing flags on attachment: 104427

Committed r93387: <http://trac.webkit.org/changeset/93387>
Comment 24 WebKit Review Bot 2011-08-18 22:45:10 PDT
All reviewed patches have been landed.  Closing bug.