optimize computing bounds by calling SkMatrix44::map2()

optimize computing bounds by calling SkMatrix44::map2()
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=171820

[danakj, 2012-12-05 23:34:46]

+shawn

[jamesr, 2012-12-06 00:19:16]

What rev of skia does this depend on?  I don't have it in my checkout.

[jamesr, 2012-12-06 00:31:16]

I get weird graphical artifacts on the tab switcher with this patch applied. 
Mike - can you try running cc_unittests to see if that catches this?  Just
build+run that binary

[reed1, 2012-12-06 00:46:41]

On 2012/12/06 00:19:16, jamesr wrote:
> What rev of skia does this depend on?  I don't have it in my checkout.

6685

[reed1, 2012-12-06 00:47:15]

On 2012/12/06 00:31:16, jamesr wrote:
> I get weird graphical artifacts on the tab switcher with this patch applied. 
> Mike - can you try running cc_unittests to see if that catches this?  Just
> build+run that binary

ah, didn't know about cc_unittests. I've just been running my own unitests for
matrix44 and cc_perftests. I'll run those now.

[reed1, 2012-12-06 01:03:59]

On 2012/12/06 00:47:15, reed1 wrote:
> On 2012/12/06 00:31:16, jamesr wrote:
> > I get weird graphical artifacts on the tab switcher with this patch applied.

> > Mike - can you try running cc_unittests to see if that catches this?  Just
> > build+run that binary
> 
> ah, didn't know about cc_unittests. I've just been running my own unitests for
> matrix44 and cc_perftests. I'll run those now.

Hooray for unittests! I do see failures with this and the transform.cc patches,
so I have more work to do.

Fortunately, they all pass w/o these patches, but w/ the latest SkMatrix44, so
nothing *should* break tomorrow when we do the skia DEPS pull.
 
I will debug the unittests failures, and ping folks after all of those pass.

[danakj, 2012-12-06 01:07:26]

On Wed, Dec 5, 2012 at 8:03 PM,  <reed@google.com> wrote:
> On 2012/12/06 00:47:15, reed1 wrote:
>>
>> On 2012/12/06 00:31:16, jamesr wrote:
>> > I get weird graphical artifacts on the tab switcher with this patch
>> > applied.
>
>
>> > Mike - can you try running cc_unittests to see if that catches this?
>> > Just
>> > build+run that binary
>
>
>> ah, didn't know about cc_unittests. I've just been running my own unitests
>> for
>> matrix44 and cc_perftests. I'll run those now.
>
>
> Hooray for unittests! I do see failures with this and the transform.cc
> patches,
> so I have more work to do.
>
> Fortunately, they all pass w/o these patches, but w/ the latest SkMatrix44,
> so
> nothing *should* break tomorrow when we do the skia DEPS pull.
>
> I will debug the unittests failures, and ping folks after all of those pass.

You might want to also be running ui_unittests. Most of the MathUtil
tests will be moving there in time, but it has some basic tests for
geometry stuff already at the least.

[shawnsingh, 2012-12-06 03:00:55]

Looks great, except i'm not seeing where in SkMatrix44.cpp that the kPerspective
mask bit is being set ever?   So it wouldn't ever go through this path?  Am I
missing something obvious?

https://codereview.chromium.org/11442020/diff/1/cc/math_util.cc
File cc/math_util.cc (right):

https://codereview.chromium.org/11442020/diff/1/cc/math_util.cc#newcode116
cc/math_util.cc:116: quad[0] = srcRect.x();     quad[1] = srcRect.y();
nit:  point --> points

[reed1, 2012-12-06 05:50:20]

kPerspective_Mask is set in computeTypeMask()

However, I have removed that check for now. I don't see any perf difference w/
or w/o that extra code in cc_perftests.

will run this through try-bots once skia rev. 6685 lands in chrome.

https://codereview.chromium.org/11442020/diff/1/cc/math_util.cc
File cc/math_util.cc (right):

https://codereview.chromium.org/11442020/diff/1/cc/math_util.cc#newcode116
cc/math_util.cc:116: quad[0] = srcRect.x();     quad[1] = srcRect.y();
On 2012/12/06 03:00:55, shawnsingh wrote:
> nit:  point --> points

Done.

[reed1, 2012-12-06 16:41:46]

ptal

the matrix44 optimizations (+ map2() api) have landed in chrome.

[shawnsingh, 2012-12-07 04:26:50]

LGTM =)

[jamesr, 2012-12-07 04:30:39]

https://codereview.chromium.org/11442020/diff/1003/cc/math_util.cc
File cc/math_util.cc (right):

https://codereview.chromium.org/11442020/diff/1003/cc/math_util.cc#newcode124
cc/math_util.cc:124: return computeEnclosingClippedRect(*(const
HomogeneousCoordinate*)&result[0],
chromium code never uses c-style casts.  I'd suggest hoisting the cast to result
out of this call and using a static_cast<>, if that compiles, or even better
just doing member-by-member initialization and trust the compiler to fold it
together.

[reed1, 2012-12-07 15:46:13]

changed c-cast to reinterpret_cast<>

[danakj, 2012-12-07 16:31:55]

https://codereview.chromium.org/11442020/diff/11002/cc/math_util.cc
File cc/math_util.cc (right):

https://codereview.chromium.org/11442020/diff/11002/cc/math_util.cc#newcode124
cc/math_util.cc:124: return
computeEnclosingClippedRect(*reinterpret_cast<HomogeneousCoordinate*>(&result[0]),
Can we do this without casting please?

[tomhudson, 2012-12-07 16:36:37]

On 2012/12/07 16:31:55, danakj wrote:
> https://codereview.chromium.org/11442020/diff/11002/cc/math_util.cc
> File cc/math_util.cc (right):
> 
> https://codereview.chromium.org/11442020/diff/11002/cc/math_util.cc#newcode124
> cc/math_util.cc:124: return
>
computeEnclosingClippedRect(*reinterpret_cast<HomogeneousCoordinate*>(&result[0]),
> Can we do this without casting please?

How, concretely, do you suggest it be done?

[danakj, 2012-12-07 16:41:46]

On 2012/12/07 16:36:37, tomhudson wrote:
> On 2012/12/07 16:31:55, danakj wrote:
> > https://codereview.chromium.org/11442020/diff/11002/cc/math_util.cc
> > File cc/math_util.cc (right):
> > 
> >
https://codereview.chromium.org/11442020/diff/11002/cc/math_util.cc#newcode124
> > cc/math_util.cc:124: return
> >
>
computeEnclosingClippedRect(*reinterpret_cast<HomogeneousCoordinate*>(&result[0]),
> > Can we do this without casting please?
> 
> How, concretely, do you suggest it be done?

"or even better just doing member-by-member initialization and trust the
compiler to fold it together."

[reed1, 2012-12-07 16:43:42]

I will try the assignment. The entire motivation for the CL was performance,
specially to eliminate memcpys. I will run the perf-test with the cast and with
the assignment...

[reed1, 2012-12-07 18:08:13]

switch to local HomogeneousPoints on the stack.

[danakj, 2012-12-07 18:26:34]

Thanks. LGTM with nits and one question.

https://codereview.chromium.org/11442020/diff/11003/cc/math_util.cc
File cc/math_util.cc (right):

https://codereview.chromium.org/11442020/diff/11003/cc/math_util.cc#newcode116
cc/math_util.cc:116: quad[0] = srcRect.x();     quad[1] = srcRect.y();
nit: one statement per line

https://codereview.chromium.org/11442020/diff/11003/cc/math_util.cc#newcode118
cc/math_util.cc:118: quad[4] = srcRect.x();     quad[5] = srcRect.bottom();
Does the ordering here end up producing a twisted quad? Would putting the bottom
right in [4][5] and the bottom left in [6][7] produce the same result? I'd
prefer to produce a convex quad here if we can.

https://codereview.chromium.org/11442020/diff/11003/cc/math_util.cc#newcode124
cc/math_util.cc:124: HomogeneousCoordinate hc0(result[0],  result[1], 
result[2],  result[3]);
nit: one space between each.

we don't line things up across lines in general. that way a diff on one line
doesn't need to change unrelated lines in the future.

[reed1, 2012-12-07 19:12:26]

https://codereview.chromium.org/11442020/diff/11003/cc/math_util.cc
File cc/math_util.cc (right):

https://codereview.chromium.org/11442020/diff/11003/cc/math_util.cc#newcode116
cc/math_util.cc:116: quad[0] = srcRect.x();     quad[1] = srcRect.y();
On 2012/12/07 18:26:34, danakj wrote:
> nit: one statement per line

Done.

https://codereview.chromium.org/11442020/diff/11003/cc/math_util.cc#newcode118
cc/math_util.cc:118: quad[4] = srcRect.x();     quad[5] = srcRect.bottom();
On 2012/12/07 18:26:34, danakj wrote:
> Does the ordering here end up producing a twisted quad? Would putting the
bottom
> right in [4][5] and the bottom left in [6][7] produce the same result? I'd
> prefer to produce a convex quad here if we can.

Done.

https://codereview.chromium.org/11442020/diff/11003/cc/math_util.cc#newcode124
cc/math_util.cc:124: HomogeneousCoordinate hc0(result[0],  result[1], 
result[2],  result[3]);
On 2012/12/07 18:26:34, danakj wrote:
> nit: one space between each.
> 
> we don't line things up across lines in general. that way a diff on one line
> doesn't need to change unrelated lines in the future.

Done.

[jamesr, 2012-12-07 19:14:04]

lgtm2!