Bug 80741 – [chromium] Use projectQuad to apply inverse mapRect

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

[Dana Jansens]

[chromium] Can't inverse mapRect a transform that gives 3d components

[Dana Jansens]

Created attachment 131143 [details]
Patch

[Shawn Singh]

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?

[Dana Jansens]

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.

[Dana Jansens]

Created attachment 131145 [details]
Patch

Shawn rocks! Using projectQuad works, switched to using that.

[Dana Jansens]

Created attachment 131426 [details]
Patch

[Shawn Singh]

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...!!

[Dana Jansens]

Created attachment 131429 [details]
Patch

Right! Thanks

[Dana Jansens]

Verified that draw culling with CCOcclusionTracker works correctly with the platform/chromium/3d-corners layout test after this projectQuads change is applied.

[Shawn Singh]

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?

[Dana Jansens]

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.

[Dana Jansens]

Created attachment 131468 [details]
Patch

[Dana Jansens]

Created attachment 131471 [details]
Patch

Include comment for explaining result in last test.

[Dana Jansens]

Created attachment 131473 [details]
Patch

And referenced the wrong path to the layout test in the new comment.

[Adrienne Walker]

Comment on attachment 131473 [details]
Patch

R=me.  It's nice to have unit tests for these cases.  :)

[WebKit Review Bot]

Comment on attachment 131473 [details]
Patch

Clearing flags on attachment: 131473

Committed r110529: <http://trac.webkit.org/changeset/110529>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Hajime Morrita]

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().

[Hajime Morrita]

http://chromegw.corp.google.com/i/chromium.webkit/builders/Webkit%20Mac%20Builder/builds/20170/steps/compile/logs/stdio

[Dana Jansens]

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.

[Dana Jansens]

Created attachment 131612 [details]
Patch

[WebKit Review Bot]

Comment on attachment 131612 [details]
Patch

Clearing flags on attachment: 131612

Committed r110569: <http://trac.webkit.org/changeset/110569>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.