Computing world-space transform for LayerChromium and CCLayerImpl
Created attachment 103703 [details] Patch
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.
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).
Created attachment 103705 [details] Patch
(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
Will re-submit a new patch soon that includes a test case and fixes a FIXME in LayerRendererChromium::drawLayer(...)
Created attachment 103999 [details] Patch
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.
Created attachment 104062 [details] Patch
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.
"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.
Created attachment 104088 [details] Patch
OK, submitted a new patch that is ready for review.
Comment on attachment 104088 [details] Patch Great, looks good.
Comment on attachment 104088 [details] Patch Clearing flags on attachment: 104088 Committed r93186: <http://trac.webkit.org/changeset/93186>
All reviewed patches have been landed. Closing bug.
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.
Reverted r93186 for reason: Breaks LayoutTests on Chromium canary bots Committed r93201: <http://trac.webkit.org/changeset/93201>
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.
Created attachment 104427 [details] Patch
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 on attachment 104427 [details] Patch R=me
Comment on attachment 104427 [details] Patch Clearing flags on attachment: 104427 Committed r93387: <http://trac.webkit.org/changeset/93387>