Migrate from MathUtil::inverse() to gfx::Transform::GetInverse()

Migrate from MathUtil::inverse() to gfx::Transform::GetInverse()

BUG=159972, 163769
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175363

[shawnsingh, 2012-12-18 23:07:45]

PTAL thanks!

[danakj, 2012-12-19 05:23:06]

I'm concerned that GetIdentity doesn't change the matrix to identity in the
non-invertible case.

We do GetIdentity and only DCHECK the result in places. Combine that with
kSkipInitialization, and we have undefined runtime behaviour. I don't feel
comfortable with that.

https://codereview.chromium.org/11644008/diff/1/cc/direct_renderer.cc
File cc/direct_renderer.cc (right):

https://codereview.chromium.org/11644008/diff/1/cc/direct_renderer.cc#newcode191
cc/direct_renderer.cc:191:
frame.currentRenderPass->transform_to_root_target.GetInverse(&inverseTransform);
I don't see a check for invertible here. Should we do something with the return
value from GetInverse()?

Maybe early out and return renderPassScissor unintersected?

https://codereview.chromium.org/11644008/diff/1/cc/gl_renderer.cc
File cc/gl_renderer.cc (right):

https://codereview.chromium.org/11644008/diff/1/cc/gl_renderer.cc#newcode871
cc/gl_renderer.cc:871: deviceTransform.GetInverse(&inverseDeviceTransform);
We checked deviceTransform.IsInvertible way up near the top of this method.

Instead, can we do the GetInverse() up there, so we don't have to do the work of
IsInvertible+Invert?

https://codereview.chromium.org/11644008/diff/1/cc/layer_impl.cc
File cc/layer_impl.cc (right):

https://codereview.chromium.org/11644008/diff/1/cc/layer_impl.cc#newcode240
cc/layer_impl.cc:240:
screenSpaceTransform().GetInverse(&inverseScreenSpaceTransform);
If GetInverse() returns false, seems like we shouldn't use it? Should we pretend
the region is empty in that case, and cause impl scrolling when it might not
expect? Or pretend the region is infinite and scroll on the main thread when we
might have been ok to impl scroll? I guess the latter is safer?

https://codereview.chromium.org/11644008/diff/1/cc/layer_tree_host.cc
File cc/layer_tree_host.cc (right):

https://codereview.chromium.org/11644008/diff/1/cc/layer_tree_host.cc#newcode761
cc/layer_tree_host.cc:761: m_implTransform.GetInverse(&inverseImplTransform);
You could DCHECK the result of GetInverse() here to save an IsInvertible().

https://codereview.chromium.org/11644008/diff/1/cc/layer_tree_host_common.cc
File cc/layer_tree_host_common.cc (right):

https://codereview.chromium.org/11644008/diff/1/cc/layer_tree_host_common.cc#...
cc/layer_tree_host_common.cc:60: transform.GetInverse(&surfaceToLayer);
If it's not invertible, should we return layerBoundRect or something else? Or
should we DCHECK the return value here?

https://codereview.chromium.org/11644008/diff/1/cc/layer_tree_host_common.cc#...
cc/layer_tree_host_common.cc:326:
partialLayerOriginTransform.GetInverse(&inversePartialLayerOriginTransform);
DCHECK the return value?

https://codereview.chromium.org/11644008/diff/1/cc/layer_tree_host_common.cc#...
cc/layer_tree_host_common.cc:381:
layer->renderSurface()->drawTransform().GetInverse(&inverseSurfaceDrawTransform);
DCHECK the return value? Or should we do something else?

https://codereview.chromium.org/11644008/diff/1/cc/layer_tree_host_common.cc#...
cc/layer_tree_host_common.cc:746:
renderSurface->drawTransform().GetInverse(&inverseSurfaceDrawTransform);
DCHECK the return value?

https://codereview.chromium.org/11644008/diff/1/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/11644008/diff/1/cc/layer_tree_host_impl.cc#ne...
cc/layer_tree_host_impl.cc:502:
rootLayer->screenSpaceTransform().GetInverse(&transformToLayerSpace);
You can DCHECK the return value here instead of IsInvertible() up above.

https://codereview.chromium.org/11644008/diff/1/cc/layer_tree_host_impl.cc#ne...
cc/layer_tree_host_impl.cc:1313:
layerImpl.screenSpaceTransform().GetInverse(&inverseScreenSpaceTransform);
Can DCHECK here instead of IsInvertible also.

https://codereview.chromium.org/11644008/diff/1/cc/test/geometry_test_utils.cc
File cc/test/geometry_test_utils.cc (right):

https://codereview.chromium.org/11644008/diff/1/cc/test/geometry_test_utils.c...
cc/test/geometry_test_utils.cc:63: gfx::Transform result;
I guess this should be kSkipInitialization as well?

https://codereview.chromium.org/11644008/diff/1/cc/test/geometry_test_utils.h
File cc/test/geometry_test_utils.h (right):

https://codereview.chromium.org/11644008/diff/1/cc/test/geometry_test_utils.h...
cc/test/geometry_test_utils.h:78: gfx::Transform inverse(const gfx::Transform&
transform);
how about InvertTransform()?

a) we don't have to rename it later for gtfo reasons.
b) without a class scope, inverse() is a lot more ambiguous.

https://codereview.chromium.org/11644008/diff/1/ui/gfx/transform.h
File ui/gfx/transform.h (right):

https://codereview.chromium.org/11644008/diff/1/ui/gfx/transform.h#newcode25
ui/gfx/transform.h:25: kSkipInitialization
Wishing for shorter synonym for this, but I got nothin..

https://codereview.chromium.org/11644008/diff/1/ui/gfx/transform_unittest.cc
File ui/gfx/transform_unittest.cc (right):

https://codereview.chromium.org/11644008/diff/1/ui/gfx/transform_unittest.cc#...
ui/gfx/transform_unittest.cc:1193: gfx::Transform inverse_translation;
kSkipInitialization?

https://codereview.chromium.org/11644008/diff/1/ui/gfx/transform_unittest.cc#...
ui/gfx/transform_unittest.cc:1208: gfx::Transform inverse_scale;
kSkipInit?

https://codereview.chromium.org/11644008/diff/1/ui/gfx/transform_unittest.cc#...
ui/gfx/transform_unittest.cc:1218: // Try to invert a matrix that is not
invertible.
Is this really desirable? Why don't we set it to the identity.

https://codereview.chromium.org/11644008/diff/1/ui/gfx/transform_unittest.cc#...
ui/gfx/transform_unittest.cc:1220: gfx::Transform uninvertible;
kSkipInitialization?

[danakj, 2012-12-19 16:56:06]

On 2012/12/19 05:23:06, danakj wrote:
> I'm concerned that GetIdentity doesn't change the matrix to identity in the
> non-invertible case.

GetInverse*, i meant.

[shawnsingh, 2012-12-19 19:41:51]

On 2012/12/19 16:56:06, danakj wrote:
> On 2012/12/19 05:23:06, danakj wrote:
> > I'm concerned that GetIdentity doesn't change the matrix to identity in the
> > non-invertible case.
> 
> GetInverse*, i meant.

So how about the following pattern for this patch:  We must either (1) properly
handle the return value of GetInverse, or (2) make sure we do not skip
initialization that initializes the transform to identity.  That way we can
ensure we are at least equivalent to existing semantics (by option 2), and in
places where we are changing semantics (using option 1) we know it will be
correct, too.

I guess it would be even nicer to enforce something by design, rather than by
convention, but I'm not seeing any practical way to do that without affecting
performance.

[danakj, 2012-12-19 19:45:16]

On Wed, Dec 19, 2012 at 2:41 PM, <shawnsingh@chromium.org> wrote:

> On 2012/12/19 16:56:06, danakj wrote:
>
>> On 2012/12/19 05:23:06, danakj wrote:
>> > I'm concerned that GetIdentity doesn't change the matrix to identity in
>> the
>> > non-invertible case.
>>
>
>  GetInverse*, i meant.
>>
>
> So how about the following pattern for this patch:  We must either (1)
> properly
> handle the return value of GetInverse, or (2) make sure we do not skip
> initialization that initializes the transform to identity.  That way we can
> ensure we are at least equivalent to existing semantics (by option 2), and
> in
> places where we are changing semantics (using option 1) we know it will be
> correct, too.
>
> I guess it would be even nicer to enforce something by design, rather than
> by
> convention, but I'm not seeing any practical way to do that without
> affecting
> performance.
>

Given that non-invertible is an edge case, I'm not sure I buy the argument
that we can't afford to set the matrix to identity when GetInverse() fails.
Why do you think we can't?

[shawnsingh, 2012-12-19 19:53:09]

> Why do you think we can't?


because I was being blockheaded and missed that obvious approach =)

new patch will come sometime later today.

[Ian Vollick, 2012-12-19 21:02:22]

On 2012/12/19 19:53:09, shawnsingh wrote:
> > Why do you think we can't?
> 
> 
> because I was being blockheaded and missed that obvious approach =)
> 
> new patch will come sometime later today.

I'm sorry for taking so long to comment on this. I think this approach is fine
for now (using the identity if uninvertible),  but I'm a bit worried that we'll
have more uninvertible matrices than we think. The spec lets people build
matrices directly (http://www.w3.org/TR/css3-transforms/), so it seems like
there's a good chance there will be matrices in the wild that can't be inverted.
In particular, I'm thinking about matrices that throw away the z component. It
looks like these inverse matrices are most often used to unproject points and
rects. I wonder if we could do better if we wrote an 'unproject rect' function?
If we're 2D and stay 2D, a bunch of the matrix entries don't matter. We can
probably unproject fine, even if the full 4x4 matrix is technically
uninvertible. We might also be more efficient.

[shawnsingh, 2012-12-27 22:45:07]

PTAL.

here is the status of the patch:

1. I made sure that all uses of GetInverse() use the return value.

2. In some of those cases, however, I needed to put a TODO that suggests we need
a better way of handling it.  For now, this patch retains equivalent behavior to
previous code, which returns an identity matrix when trying invert an
uninvertible matrix.  To reduce debugging pain and to keep the possibility of
merging this patch back to m25, I would prefer to chip away at those TODO's in a
separate patch from this.

3. cc_perftests improves by approx 1.5-2% with this patch.  So we should
consider merging it back to m25 after it lands.  Let me know

[danakj, 2013-01-02 14:45:16]

https://codereview.chromium.org/11644008/diff/13001/cc/direct_renderer.cc
File cc/direct_renderer.cc (right):

https://codereview.chromium.org/11644008/diff/13001/cc/direct_renderer.cc#new...
cc/direct_renderer.cc:189: if
(frame.currentRenderPass->transform_to_root_target.GetInverse(&inverseTransform))
{
I'm still not a fan that after the if() statement, if it was false, you have an
inverseTransform variable that is uninitialized data.

I don't see why setting the transform to identity in GetInverse() when it is
going to return false makes this patch any harder to merge back to M25?

Also, I don't expect it would have any impact on perftest at all?

[shawnsingh, 2013-01-02 18:36:01]

Thanks for taking a look - one more patch with identity initialization in
GetInverse() coming soon.

https://codereview.chromium.org/11644008/diff/13001/cc/direct_renderer.cc
File cc/direct_renderer.cc (right):

https://codereview.chromium.org/11644008/diff/13001/cc/direct_renderer.cc#new...
cc/direct_renderer.cc:189: if
(frame.currentRenderPass->transform_to_root_target.GetInverse(&inverseTransform))
{
The performance improvement comes from avoiding duplicate initialization,
mostly, with kSkipInitialization.  It possibly also comes from initializing by
reference arg rather than return value, but last I checked RVO was working fine
at least on linux.

Yes, I suppose we can initialize-to-identity in the gfx::Transform::GetInverse()
function itself.  I didn't realize that's what you had intended.   

The "harder to merge" part I was concerned about is the changing of semantics to
properly handle all the cases that we are currently not handling correctly - we
should simply never be using the transform if GetInverse returns false.  This
patch doesn't address that issue, and I don't think it should =)

[shawnsingh, 2013-01-04 20:54:08]

PTAL - danakj, this latest patch should be what you wanted =)

Thanks in advance!

[danakj, 2013-01-04 21:56:07]

LGTM

https://codereview.chromium.org/11644008/diff/21001/cc/layer_tree_host.cc
File cc/layer_tree_host.cc (right):

https://codereview.chromium.org/11644008/diff/21001/cc/layer_tree_host.cc#new...
cc/layer_tree_host.cc:801: // here, or DCHECK that the transform is invertible.
Thanks, I like how this makes current potential bugs really glaring.

https://codereview.chromium.org/11644008/diff/21001/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/11644008/diff/21001/cc/layer_tree_host_impl.c...
cc/layer_tree_host_impl.cc:506:
DCHECK(rootLayer->screenSpaceTransform().IsInvertible());
This DCHECK is redundant with the DCHECK(didInvert) now.

https://codereview.chromium.org/11644008/diff/21001/cc/layer_tree_host_impl.c...
cc/layer_tree_host_impl.cc:512: // no perspective, so mapping is sufficient (as
opposed to projecting).
^_^

https://codereview.chromium.org/11644008/diff/21001/cc/layer_tree_host_impl.c...
cc/layer_tree_host_impl.cc:1284: DCHECK(didInvert);
This is going to potentially bomb when we start impl-touch-scrolling non-root
layers, no? Should this be a TODO?

https://codereview.chromium.org/11644008/diff/21001/cc/test/geometry_test_uti...
File cc/test/geometry_test_utils.cc (right):

https://codereview.chromium.org/11644008/diff/21001/cc/test/geometry_test_uti...
cc/test/geometry_test_utils.cc:62: gfx::Transform result;
SkipInitialization?

[commit-bot: I haz the power, 2013-01-07 05:43:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shawnsingh@chromium.org/11644008/25002

[commit-bot: I haz the power, 2013-01-07 05:54:24]

Retried try job too often on ios_dbg_simulator for step(s) sync_unit_tests,
unit_tests