Make SkShader store localM directly rather than as a separate alloc.

Make SkShader store localM directly rather than as a separate alloc.

May cause very slight GM changes in gpu two pt radial/conical radients.
Committed: https://code.google.com/p/skia/source/detail?r=6221

[bsalomon, 2012-10-31 15:10:05]

more than 40 net LOC removed.

https://codereview.appspot.com/6821056/diff/6001/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.appspot.com/6821056/diff/6001/src/gpu/SkGpuDevice.cpp#newc...
src/gpu/SkGpuDevice.cpp:587: // since our texture coords will be in local space,
we whack the texture
Moved this to up before we try to get the texture. Also, we didn't return false
when localM failed to invert before!

[robertphillips, 2012-10-31 15:43:03]

LGTM + 1 nit & two questions

https://codereview.appspot.com/6821056/diff/6001/include/core/SkMatrix.h
File include/core/SkMatrix.h (right):

https://codereview.appspot.com/6821056/diff/6001/include/core/SkMatrix.h#newc...
include/core/SkMatrix.h:343: if (inverse && inverse != this) {
NULL != inverse?

https://codereview.appspot.com/6821056/diff/6001/include/core/SkMatrix.h#newc...
include/core/SkMatrix.h:629: 
Do we want SK_WARN_UNUSED_RESULT on this decl too?

https://codereview.appspot.com/6821056/diff/6001/src/core/SkDraw.cpp
File src/core/SkDraw.cpp (right):

https://codereview.appspot.com/6821056/diff/6001/src/core/SkDraw.cpp#newcode2391
src/core/SkDraw.cpp:2391: if (texture_to_matrix(state, vertices, textures,
&tempM)) {
Is this savedLocalM access guarded against an uninitialized memory read?

[bsalomon, 2012-10-31 15:45:15]

https://codereview.appspot.com/6821056/diff/6001/include/core/SkMatrix.h
File include/core/SkMatrix.h (right):

https://codereview.appspot.com/6821056/diff/6001/include/core/SkMatrix.h#newc...
include/core/SkMatrix.h:343: if (inverse && inverse != this) {
On 2012/10/31 15:43:04, robertphillips wrote:
> NULL != inverse?
Will do.

https://codereview.appspot.com/6821056/diff/6001/include/core/SkMatrix.h#newc...
include/core/SkMatrix.h:629: 
On 2012/10/31 15:43:04, robertphillips wrote:
> Do we want SK_WARN_UNUSED_RESULT on this decl too?

Can't hurt, will do.

https://codereview.appspot.com/6821056/diff/6001/src/core/SkDraw.cpp
File src/core/SkDraw.cpp (right):

https://codereview.appspot.com/6821056/diff/6001/src/core/SkDraw.cpp#newcode2391
src/core/SkDraw.cpp:2391: if (texture_to_matrix(state, vertices, textures,
&tempM)) {
On 2012/10/31 15:43:04, robertphillips wrote:
> Is this savedLocalM access guarded against an uninitialized memory read?

I assume so only because the next line accesses "shader". savedLocalM is always
set if shader is non-NULL.

[reed1, 2012-10-31 16:16:04]

can we land the matrix changes (w/ possible bench?) ahead of this CL?

https://codereview.appspot.com/6821056/diff/6001/include/core/SkMatrix.h
File include/core/SkMatrix.h (right):

https://codereview.appspot.com/6821056/diff/6001/include/core/SkMatrix.h#newc...
include/core/SkMatrix.h:342: if (this->isIdentity()) {
Do we think the check against self is important/worth-it ?

[bsalomon, 2012-10-31 16:22:10]

>can we land the matrix changes (w/ possible bench?) ahead of this CL?

Can do. We already have an invert_maprect* suite of tests (including an
_identity version) that should cover it.

https://codereview.appspot.com/6821056/diff/6001/include/core/SkMatrix.h
File include/core/SkMatrix.h (right):

https://codereview.appspot.com/6821056/diff/6001/include/core/SkMatrix.h#newc...
include/core/SkMatrix.h:342: if (this->isIdentity()) {
On 2012/10/31 16:16:04, reed1 wrote:
> Do we think the check against self is important/worth-it ?

Probably not

[reed1, 2012-10-31 16:43:00]

lgtm w/ questions

https://codereview.appspot.com/6821056/diff/6001/src/core/SkDraw.cpp
File src/core/SkDraw.cpp (right):

https://codereview.appspot.com/6821056/diff/6001/src/core/SkDraw.cpp#newcode2391
src/core/SkDraw.cpp:2391: if (texture_to_matrix(state, vertices, textures,
&tempM)) {
On 2012/10/31 15:45:15, bsalomon wrote:
> On 2012/10/31 15:43:04, robertphillips wrote:
> > Is this savedLocalM access guarded against an uninitialized memory read?
> 
> I assume so only because the next line accesses "shader". savedLocalM is
always
> set if shader is non-NULL.

True, tho note that now we *always* call postConcat, even if savedLocalM is
identity (which may be common).

https://codereview.appspot.com/6821056/diff/6001/src/core/SkDraw.cpp#newcode2413
src/core/SkDraw.cpp:2413: if (NULL != shader) {
note: now we always call this, even when the matrix is (often?) identity. Not
saying it matters, just a diff.

https://codereview.appspot.com/6821056/diff/6001/src/core/SkShader.cpp
File src/core/SkShader.cpp (right):

https://codereview.appspot.com/6821056/diff/6001/src/core/SkShader.cpp#newcode63
src/core/SkShader.cpp:63: const SkMatrix& origMatrix) {
Not sure we need this change, since it means a copy right before a setter
(setConcat).

[bsalomon, 2012-10-31 17:43:44]

Make SkShader store localM directly rather than as a separate alloc.

[bsalomon, 2012-10-31 17:44:51]

https://codereview.appspot.com/6821056/diff/6001/src/core/SkDraw.cpp
File src/core/SkDraw.cpp (right):

https://codereview.appspot.com/6821056/diff/6001/src/core/SkDraw.cpp#newcode2391
src/core/SkDraw.cpp:2391: if (texture_to_matrix(state, vertices, textures,
&tempM)) {
On 2012/10/31 16:43:00, reed1 wrote:
> On 2012/10/31 15:45:15, bsalomon wrote:
> > On 2012/10/31 15:43:04, robertphillips wrote:
> > > Is this savedLocalM access guarded against an uninitialized memory read?
> > 
> > I assume so only because the next line accesses "shader". savedLocalM is
> always
> > set if shader is non-NULL.
> 
> True, tho note that now we *always* call postConcat, even if savedLocalM is
> identity (which may be common).
postConcat checks for identity.

https://codereview.appspot.com/6821056/diff/6001/src/core/SkDraw.cpp#newcode2413
src/core/SkDraw.cpp:2413: if (NULL != shader) {
On 2012/10/31 16:43:00, reed1 wrote:
> note: now we always call this, even when the matrix is (often?) identity. Not
> saying it matters, just a diff.

Yup, I assumed it didn't really matter

https://codereview.appspot.com/6821056/diff/6001/src/core/SkShader.cpp
File src/core/SkShader.cpp (right):

https://codereview.appspot.com/6821056/diff/6001/src/core/SkShader.cpp#newcode63
src/core/SkShader.cpp:63: const SkMatrix& origMatrix) {
On 2012/10/31 16:43:00, reed1 wrote:
> Not sure we need this change, since it means a copy right before a setter
> (setConcat).

Done.