[chromium] Can't inverse mapRect a transform that gives 3d components
Created attachment 131143 [details] Patch
Comment on attachment 131143 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131143&action=review > Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.cpp:280 > + // FIXME: Can't invert a mapRect on transforms that produce 3d components Actually now I'm confused... would it not work to use projectQuad instead of mapRect for the inverse (just a few lines below in this function)? 3d should then work as expected, i think, except for pending clipping bugs?
Comment on attachment 131143 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131143&action=review >> Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.cpp:280 >> + // FIXME: Can't invert a mapRect on transforms that produce 3d components > > Actually now I'm confused... would it not work to use projectQuad instead of mapRect for the inverse (just a few lines below in this function)? 3d should then work as expected, i think, except for pending clipping bugs? Transform rotateY 30 degrees Original contentRect 0,0,200,200 Becomes transformedRect something like 0,0,170,170 If you transform with the inverse() you get 0,0,170,170 rotated around Y by -30 degrees, giving you something like 0,0,150,150 Numbers are somewhat imaginary but I was seeing similar numbers from mapRect. I fail to see how projectRect would be any different in this case given the 2d inputs.
Created attachment 131145 [details] Patch Shawn rocks! Using projectQuad works, switched to using that.
Created attachment 131426 [details] Patch
Comment on attachment 131426 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131426&action=review > Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.cpp:277 > + projectedQuad.setP2(transform.projectPoint(q.p2(), clamped)); you'll have to use different clamped bools and OR them together...!!
Created attachment 131429 [details] Patch Right! Thanks
Verified that draw culling with CCOcclusionTracker works correctly with the platform/chromium/3d-corners layout test after this projectQuads change is applied.
The fix LGTM... I didn't look closely at the tests, though. Dana suggested that I chime in about projectQuad. here's the situation - Whenever we intend to inverse-map from parent to child, we should actually use projectQuad. projectQuad is a little misleading though its technically not incorrect. A clearer way to think of it is that inverse + projectQuad == unprojectQuad. And that is the operation we need: we have a rect described in surface space, and we want to understand where that rect is located with respect to the layer - not only the layer's space, but the layer's plane itself. Conveniently, when there is no perspective projection, I think inverse.projectQuad() operation does the same thing as inverse.mapRect(). Dana needs to duplicate projectQuad() from the TransformationMatrix code, just for temporary, because there is a deep rooted bug that breaks unproject() for layers that are behind the camera. (I'm working on it, ETA early next week?) In my personal opinion, using projectQuad for temporary like this is reasonable, and we will make sure to clean up as we fix it properly. Hopefully that clears things up?
Created attachment 131465 [details] Patch Added a unit test for the clamped case of projectQuad(). Test3dTransform and TestPerspectiveTransform both fail without the use of TransformationMatrix::projectQuad. TestPerspectiveTransformBehindCamera fails without our custom projectQuad() implementation that passes on the clamped value.
Created attachment 131468 [details] Patch
Created attachment 131471 [details] Patch Include comment for explaining result in last test.
Created attachment 131473 [details] Patch And referenced the wrong path to the layout test in the new comment.
Comment on attachment 131473 [details] Patch R=me. It's nice to have unit tests for these cases. :)
Comment on attachment 131473 [details] Patch Clearing flags on attachment: 131473 Committed r110529: <http://trac.webkit.org/changeset/110529>
All reviewed patches have been landed. Closing bug.
Rolled out at http://trac.webkit.org/changeset/110538 due to a Mac build breakage. It looks that the unit test derives a subclass from template class and use a method in superclass. I guess this happens because GCC cannot resolve the name of such method call and requires class name like A<B, C>::method() instead of method().
http://chromegw.corp.google.com/i/chromium.webkit/builders/Webkit%20Mac%20Builder/builds/20170/steps/compile/logs/stdio
Ah, sorry Hajime. I went through this with the rest of the file, and didn't update the new tests to reflect what I had learned.
Created attachment 131612 [details] Patch
Comment on attachment 131612 [details] Patch Clearing flags on attachment: 131612 Committed r110569: <http://trac.webkit.org/changeset/110569>