Bug 80741 - [chromium] Use projectQuad to apply inverse mapRect
Summary: [chromium] Use projectQuad to apply inverse mapRect
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: Dana Jansens
URL:
Keywords:
Depends on: 80613 80969
Blocks: 80743
  Show dependency treegraph
 
Reported: 2012-03-09 17:32 PST by Dana Jansens
Modified: 2012-03-13 08:44 PDT (History)
9 users (show)

See Also:


Attachments
Patch (5.63 KB, patch)
2012-03-09 17:34 PST, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (5.71 KB, patch)
2012-03-09 18:10 PST, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (6.29 KB, patch)
2012-03-12 15:41 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (6.45 KB, patch)
2012-03-12 15:50 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (9.02 KB, patch)
2012-03-12 17:26 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (8.82 KB, patch)
2012-03-12 17:30 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (9.14 KB, patch)
2012-03-12 17:44 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (9.16 KB, patch)
2012-03-12 17:46 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (9.17 KB, patch)
2012-03-13 08:00 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dana Jansens 2012-03-09 17:32:15 PST
[chromium] Can't inverse mapRect a transform that gives 3d components
Comment 1 Dana Jansens 2012-03-09 17:34:10 PST
Created attachment 131143 [details]
Patch
Comment 2 Shawn Singh 2012-03-09 17:42:02 PST
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 3 Dana Jansens 2012-03-09 17:45:56 PST
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.
Comment 4 Dana Jansens 2012-03-09 18:10:13 PST
Created attachment 131145 [details]
Patch

Shawn rocks! Using projectQuad works, switched to using that.
Comment 5 Dana Jansens 2012-03-12 15:41:33 PDT
Created attachment 131426 [details]
Patch
Comment 6 Shawn Singh 2012-03-12 15:42:38 PDT
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...!!
Comment 7 Dana Jansens 2012-03-12 15:50:15 PDT
Created attachment 131429 [details]
Patch

Right! Thanks
Comment 8 Dana Jansens 2012-03-12 15:58:14 PDT
Verified that draw culling with CCOcclusionTracker works correctly with the platform/chromium/3d-corners layout test after this projectQuads change is applied.
Comment 9 Shawn Singh 2012-03-12 16:05:30 PDT
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?
Comment 10 Dana Jansens 2012-03-12 17:26:37 PDT
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.
Comment 11 Dana Jansens 2012-03-12 17:30:28 PDT
Created attachment 131468 [details]
Patch
Comment 12 Dana Jansens 2012-03-12 17:44:04 PDT
Created attachment 131471 [details]
Patch

Include comment for explaining result in last test.
Comment 13 Dana Jansens 2012-03-12 17:46:36 PDT
Created attachment 131473 [details]
Patch

And referenced the wrong path to the layout test in the new comment.
Comment 14 Adrienne Walker 2012-03-12 17:54:24 PDT
Comment on attachment 131473 [details]
Patch

R=me.  It's nice to have unit tests for these cases.  :)
Comment 15 WebKit Review Bot 2012-03-12 19:27:37 PDT
Comment on attachment 131473 [details]
Patch

Clearing flags on attachment: 131473

Committed r110529: <http://trac.webkit.org/changeset/110529>
Comment 16 WebKit Review Bot 2012-03-12 19:27:43 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 Hajime Morrita 2012-03-12 22:41:13 PDT
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().
Comment 19 Dana Jansens 2012-03-13 07:57:19 PDT
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.
Comment 20 Dana Jansens 2012-03-13 08:00:56 PDT
Created attachment 131612 [details]
Patch
Comment 21 WebKit Review Bot 2012-03-13 08:44:02 PDT
Comment on attachment 131612 [details]
Patch

Clearing flags on attachment: 131612

Committed r110569: <http://trac.webkit.org/changeset/110569>
Comment 22 WebKit Review Bot 2012-03-13 08:44:20 PDT
All reviewed patches have been landed.  Closing bug.