add localmatrix-shader

add localmatrix-shader

BUG=skia:

Committed: http://code.google.com/p/skia/source/detail?r=14633

[scroggo, 2014-05-07 14:15:41]

https://codereview.chromium.org/272593002/diff/40001/include/core/SkShader.h
File include/core/SkShader.h (right):

https://codereview.chromium.org/272593002/diff/40001/include/core/SkShader.h#...
include/core/SkShader.h:408: *  Note: onwership of the proxy is not transferred
(though a ref is taken).
ownership*

https://codereview.chromium.org/272593002/diff/40001/src/core/SkShader.cpp
File src/core/SkShader.cpp (right):

https://codereview.chromium.org/272593002/diff/40001/src/core/SkShader.cpp#ne...
src/core/SkShader.cpp:354: class SkLocalMatrixShader : public SkShader {
Would it make sense to put this in its own file?

https://codereview.chromium.org/272593002/diff/40001/src/core/SkShader.cpp#ne...
src/core/SkShader.cpp:401: SkShader::Context*
SkLocalMatrixShader::onCreateContext(const ContextRec& rec,
Doesn't this class need to also override asNewEffect (and pass in the local
matrix) so the GPU can handle it as well?

Running your Sample and switching to using the GPU draws nothing where your new
shader is used.

This should also forward on calls to asABitmap and asAGradient (and the new ones
that I'm adding in a separate CL, but I'll add those in that CL) so PDF (and
ultimately, Android, once I convert them to inspecting our shaders) will draw it
correctly.

https://codereview.chromium.org/272593002/diff/40001/src/core/SkShader.cpp#ne...
src/core/SkShader.cpp:406: tmp.setConcat(*rec.fLocalMatrix, fProxyLocalMatrix);
This ordering seems backwards to me. It appears to be reversed from that of
SkShader::computeTotalInverse and SkPictureShader::refBitmapShader, where we
multiply getLocalMatrix * rec.fLocalMatrix.

[reed1, 2014-05-07 20:37:31]

https://codereview.chromium.org/272593002/diff/40001/include/core/SkShader.h
File include/core/SkShader.h (right):

https://codereview.chromium.org/272593002/diff/40001/include/core/SkShader.h#...
include/core/SkShader.h:408: *  Note: onwership of the proxy is not transferred
(though a ref is taken).
On 2014/05/07 14:15:41, scroggo wrote:
> ownership*

Done.

https://codereview.chromium.org/272593002/diff/40001/src/core/SkShader.cpp
File src/core/SkShader.cpp (right):

https://codereview.chromium.org/272593002/diff/40001/src/core/SkShader.cpp#ne...
src/core/SkShader.cpp:354: class SkLocalMatrixShader : public SkShader {
On 2014/05/07 14:15:41, scroggo wrote:
> Would it make sense to put this in its own file?

If it get big enough, yes. When I begin, it was pretty darn tiny, but we'll see.

https://codereview.chromium.org/272593002/diff/40001/src/core/SkShader.cpp#ne...
src/core/SkShader.cpp:401: SkShader::Context*
SkLocalMatrixShader::onCreateContext(const ContextRec& rec,
On 2014/05/07 14:15:41, scroggo wrote:
> Doesn't this class need to also override asNewEffect (and pass in the local
> matrix) so the <span id="abbrex-spnAbbr_25790" class="abbrex-Abbr
abbrex-Abbr_GPU abbrex-AbbrUnknown abbrex-AbbrUnknownVisible">GPU</span> can
handle it as well?
> 
> Running your Sample and switching to using the <span id="abbrex-spnAbbr_25791"
class="abbrex-Abbr abbrex-Abbr_GPU abbrex-AbbrUnknown
abbrex-AbbrUnknownVisible">GPU</span> draws nothing where your new
> shader is used.
> 
> This should also forward on calls to asABitmap and asAGradient (and the new
ones
> that I'm adding in a separate <span id="abbrex-spnAbbr_25792"
class="abbrex-Abbr abbrex-Abbr_CL abbrex-AbbrUnknown
abbrex-AbbrUnknownVisible">CL</span>, but I'll add those in that <span
id="abbrex-spnAbbr_25793" class="abbrex-Abbr abbrex-Abbr_CL abbrex-AbbrUnknown
abbrex-AbbrUnknownVisible">CL</span>) so <span id="abbrex-spnAbbr_25794"
class="abbrex-Abbr abbrex-Abbr_PDF abbrex-AbbrUnknown
abbrex-AbbrUnknownVisible">PDF</span> (and
> ultimately, Android, once I convert them to inspecting our shaders) will draw
it
> correctly.

Indeed, I need to forward all of those guys. Maybe its own subclass just for
proxying...

https://codereview.chromium.org/272593002/diff/40001/src/core/SkShader.cpp#ne...
src/core/SkShader.cpp:406: tmp.setConcat(*rec.fLocalMatrix, fProxyLocalMatrix);
On 2014/05/07 14:15:41, scroggo wrote:
> This ordering seems backwards to me. It appears to be reversed from that of
> SkShader::computeTotalInverse and SkPictureShader::refBitmapShader, where we
> multiply getLocalMatrix * rec.fLocalMatrix.

Good catch. Done.

[scroggo, 2014-05-07 22:01:15]

lgtm

https://codereview.chromium.org/272593002/diff/60001/src/core/SkShader.cpp
File src/core/SkShader.cpp (right):

https://codereview.chromium.org/272593002/diff/60001/src/core/SkShader.cpp#ne...
src/core/SkShader.cpp:22: #ifdef SK_TRACK_SHADER_LIFETIME
It might be nice if this was part of SkInstCount. Just a thought for future
generalization - my first thought was why not use that, but it doesn't let you
track each creation/destruction, as far as I know.

https://codereview.chromium.org/272593002/diff/60001/src/core/SkShader.cpp#ne...
src/core/SkShader.cpp:23: static int32_t gShaderCounter;
Does this need to be indented?

[reed1, 2014-05-07 22:05:05]

https://codereview.chromium.org/272593002/diff/60001/src/core/SkShader.cpp
File src/core/SkShader.cpp (right):

https://codereview.chromium.org/272593002/diff/60001/src/core/SkShader.cpp#ne...
src/core/SkShader.cpp:23: static int32_t gShaderCounter;
On 2014/05/07 22:01:15, scroggo wrote:
> Does this need to be indented?

We seem to do both practices. in SkTypes.h we indent for clarity, at least
sometimes.

[reed1, 2014-05-07 22:05:43]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-05-07 22:06:11]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/reed@google.com/272593002/100001

[commit-bot: I haz the power, 2014-05-07 22:14:09]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  Build-Mac10.8-Clang-x86-Release-Trybot on tryserver.chromium
  Build-Ubuntu13.10-GCC4.8-x86_64-Release-Trybot on tryserver.chromium

[reed1, 2014-05-07 22:20:02]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-05-07 22:20:43]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/reed@google.com/272593002/120001

[commit-bot: I haz the power, 2014-05-07 22:26:40]

Change committed as 14633