add local-matrix to shader::context

add local-matrix to shader::context

BUG=skia:

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

[reed1, 2014-05-06 13:34:32]

This removes the need for localMatrix as a getter/setter in shader, by tunneling
the temp localmatrix for drawVertices into the context.

I think we could also rewrite drawVertices to not use SkComposeShader at all,
and possibly make it faster too, but this CL keeps it for now (smaller change).

[Dominik Grewe, 2014-05-06 13:48:50]

https://codereview.chromium.org/263293005/diff/20001/src/core/SkDraw.cpp
File src/core/SkDraw.cpp (right):

https://codereview.chromium.org/263293005/diff/20001/src/core/SkDraw.cpp#newc...
src/core/SkDraw.cpp:2569: if (!blitter->resetShaderContext(rec)) {
'shader->setLocalMatrix()' is changing the LM of the paint's original shader.
This is not necessarily the shader referred to by the blitter. If we've created
the compose shader above, we're only changing the compose shader's LM but not
that of 'shader'.

[reed1, 2014-05-06 14:07:57]

https://codereview.chromium.org/263293005/diff/20001/src/core/SkDraw.cpp
File src/core/SkDraw.cpp (right):

https://codereview.chromium.org/263293005/diff/20001/src/core/SkDraw.cpp#newc...
src/core/SkDraw.cpp:2569: if (!blitter->resetShaderContext(rec)) {
On 2014/05/06 13:48:51, Dominik Grewe wrote:
> 'shader->setLocalMatrix()' is changing the <span id="abbrex-spnAbbr_21751"
class="abbrex-Abbr abbrex-Abbr_LM abbrex-AbbrUnknown
abbrex-AbbrUnknownVisible">LM</span> of the paint's original shader.
> This is not necessarily the shader referred to by the blitter. If we've
created
> the compose shader above, we're only changing the compose shader's <span
id="abbrex-spnAbbr_21752" class="abbrex-Abbr abbrex-Abbr_LM abbrex-AbbrUnknown
abbrex-AbbrUnknownVisible">LM</span> but not
> that of 'shader'.

Correct, but composeshader forwards that LM to its children, so we still draw
correctly.
Note that TriColorShader ignores *all* local-matrices (as it should).

[Dominik Grewe, 2014-05-06 14:20:59]

On 2014/05/06 14:07:57, reed1 wrote:
> https://codereview.chromium.org/263293005/diff/20001/src/core/SkDraw.cpp
> File src/core/SkDraw.cpp (right):
> 
>
https://codereview.chromium.org/263293005/diff/20001/src/core/SkDraw.cpp#newc...
> src/core/SkDraw.cpp:2569: if (!blitter->resetShaderContext(rec)) {
> On 2014/05/06 13:48:51, Dominik Grewe wrote:
> > 'shader->setLocalMatrix()' is changing the <span id="abbrex-spnAbbr_21751"
> class="abbrex-Abbr abbrex-Abbr_LM abbrex-AbbrUnknown
> abbrex-AbbrUnknownVisible">LM</span> of the paint's original shader.
> > This is not necessarily the shader referred to by the blitter. If we've
> created
> > the compose shader above, we're only changing the compose shader's <span
> id="abbrex-spnAbbr_21752" class="abbrex-Abbr abbrex-Abbr_LM abbrex-AbbrUnknown
> abbrex-AbbrUnknownVisible">LM</span> but not
> > that of 'shader'.
> 
> Correct, but composeshader forwards that LM to its children, so we still draw
> correctly.
> Note that TriColorShader ignores *all* local-matrices (as it should).

Where does it do the forwarding? In SkComposeShader::onCreateContext() we create
a new ContextRec for the children that ignores the LM.
Or are you planning to change that? If so, is it always safe to assume that
that's the correct behaviour?

[Dominik Grewe, 2014-05-06 14:21:00]

On 2014/05/06 14:07:57, reed1 wrote:
> https://codereview.chromium.org/263293005/diff/20001/src/core/SkDraw.cpp
> File src/core/SkDraw.cpp (right):
> 
>
https://codereview.chromium.org/263293005/diff/20001/src/core/SkDraw.cpp#newc...
> src/core/SkDraw.cpp:2569: if (!blitter->resetShaderContext(rec)) {
> On 2014/05/06 13:48:51, Dominik Grewe wrote:
> > 'shader->setLocalMatrix()' is changing the <span id="abbrex-spnAbbr_21751"
> class="abbrex-Abbr abbrex-Abbr_LM abbrex-AbbrUnknown
> abbrex-AbbrUnknownVisible">LM</span> of the paint's original shader.
> > This is not necessarily the shader referred to by the blitter. If we've
> created
> > the compose shader above, we're only changing the compose shader's <span
> id="abbrex-spnAbbr_21752" class="abbrex-Abbr abbrex-Abbr_LM abbrex-AbbrUnknown
> abbrex-AbbrUnknownVisible">LM</span> but not
> > that of 'shader'.
> 
> Correct, but composeshader forwards that LM to its children, so we still draw
> correctly.
> Note that TriColorShader ignores *all* local-matrices (as it should).

Where does it do the forwarding? In SkComposeShader::onCreateContext() we create
a new ContextRec for the children that ignores the LM.
Or are you planning to change that? If so, is it always safe to assume that
that's the correct behaviour?

[reed1, 2014-05-06 14:23:31]

On 2014/05/06 14:21:00, Dominik Grewe wrote:
> On 2014/05/06 14:07:57, reed1 wrote:
> > https://codereview.chromium.org/263293005/diff/20001/src/core/SkDraw.cpp
> > File src/core/SkDraw.cpp (right):
> > 
> >
>
https://codereview.chromium.org/263293005/diff/20001/src/core/SkDraw.cpp#newc...
> > src/core/SkDraw.cpp:2569: if (!blitter->resetShaderContext(rec)) {
> > On 2014/05/06 13:48:51, Dominik Grewe wrote:
> > > 'shader->setLocalMatrix()' is changing the <span id="abbrex-spnAbbr_21751"
> > class="abbrex-Abbr abbrex-Abbr_LM abbrex-AbbrUnknown
> > abbrex-AbbrUnknownVisible">LM</span> of the paint's original shader.
> > > This is not necessarily the shader referred to by the blitter. If we've
> > created
> > > the compose shader above, we're only changing the compose shader's <span
> > id="abbrex-spnAbbr_21752" class="abbrex-Abbr abbrex-Abbr_LM
abbrex-AbbrUnknown
> > abbrex-AbbrUnknownVisible">LM</span> but not
> > > that of 'shader'.
> > 
> > Correct, but composeshader forwards that LM to its children, so we still
draw
> > correctly.
> > Note that TriColorShader ignores *all* local-matrices (as it should).
> 
> Where does it do the forwarding? In SkComposeShader::onCreateContext() we
create
> a new ContextRec for the children that ignores the LM.
> Or are you planning to change that? If so, is it always safe to assume that
> that's the correct behaviour?

Each sub-shader sees the LM in its rec (newRec in that case). If the particular
shader doesn't distinguish between the CTM and LM anyway, they get it for free
when they ask for the totalInverse.

Can you think of a test that would show the problem you think you are seeing? I
am running the vertices gm, and it is drawing correctly.

[Dominik Grewe, 2014-05-06 14:28:14]

On 2014/05/06 14:23:31, reed1 wrote:
> On 2014/05/06 14:21:00, Dominik Grewe wrote:
> > On 2014/05/06 14:07:57, reed1 wrote:
> > > https://codereview.chromium.org/263293005/diff/20001/src/core/SkDraw.cpp
> > > File src/core/SkDraw.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/263293005/diff/20001/src/core/SkDraw.cpp#newc...
> > > src/core/SkDraw.cpp:2569: if (!blitter->resetShaderContext(rec)) {
> > > On 2014/05/06 13:48:51, Dominik Grewe wrote:
> > > > 'shader->setLocalMatrix()' is changing the <span
id="abbrex-spnAbbr_21751"
> > > class="abbrex-Abbr abbrex-Abbr_LM abbrex-AbbrUnknown
> > > abbrex-AbbrUnknownVisible">LM</span> of the paint's original shader.
> > > > This is not necessarily the shader referred to by the blitter. If we've
> > > created
> > > > the compose shader above, we're only changing the compose shader's <span
> > > id="abbrex-spnAbbr_21752" class="abbrex-Abbr abbrex-Abbr_LM
> abbrex-AbbrUnknown
> > > abbrex-AbbrUnknownVisible">LM</span> but not
> > > > that of 'shader'.
> > > 
> > > Correct, but composeshader forwards that LM to its children, so we still
> draw
> > > correctly.
> > > Note that TriColorShader ignores *all* local-matrices (as it should).
> > 
> > Where does it do the forwarding? In SkComposeShader::onCreateContext() we
> create
> > a new ContextRec for the children that ignores the LM.
> > Or are you planning to change that? If so, is it always safe to assume that
> > that's the correct behaviour?
> 
> Each sub-shader sees the LM in its rec (newRec in that case). If the
particular
> shader doesn't distinguish between the CTM and LM anyway, they get it for free
> when they ask for the totalInverse.
> 
> Can you think of a test that would show the problem you think you are seeing?
I
> am running the vertices gm, and it is drawing correctly.

Sorry, my bad. I didn't see that we initialize newRec with the values of the
ContextRec we're passing in to onCreateContext.

[reed1, 2014-05-06 15:15:54]

are there other comments?

[scroggo, 2014-05-06 15:40:30]

lgtm

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

https://codereview.chromium.org/263293005/diff/40001/include/core/SkShader.h#...
include/core/SkShader.h:135: const SkMatrix* fMatrix;    // the current matrix
in the canvas
nit: spacing

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

https://codereview.chromium.org/263293005/diff/40001/src/core/SkDraw.cpp#newc...
src/core/SkDraw.cpp:2407: if (!this->getCTM().invert(&ctmInv)) {
So if I understand correctly, the reason this doesn't just look at
this->getTotalInverse() is because that accounts for the local matrices, and
this does not want to?

https://codereview.chromium.org/263293005/diff/40001/src/core/SkDraw.cpp#newc...
src/core/SkDraw.cpp:2568: rec.fLocalMatrix = &tempM;
Why not set this in the constructor?

[reed1, 2014-05-06 17:04:50]

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

https://codereview.chromium.org/263293005/diff/40001/include/core/SkShader.h#...
include/core/SkShader.h:135: const SkMatrix* fMatrix;    // the current matrix
in the canvas
On 2014/05/06 15:40:30, scroggo wrote:
> nit: spacing

Done.

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

https://codereview.chromium.org/263293005/diff/40001/src/core/SkDraw.cpp#newc...
src/core/SkDraw.cpp:2407: if (!this->getCTM().invert(&ctmInv)) {
On 2014/05/06 15:40:30, scroggo wrote:
> So if I understand correctly, the reason this doesn't just look at
> this->getTotalInverse() is because that accounts for the local matrices, and
> this does not want to?

Yes. Added a comment to explain that.

https://codereview.chromium.org/263293005/diff/40001/src/core/SkDraw.cpp#newc...
src/core/SkDraw.cpp:2568: rec.fLocalMatrix = &tempM;
On 2014/05/06 15:40:30, scroggo wrote:
> Why not set this in the constructor?

half dozen. At some point I find arguments harder to know what's going on, since
they are not named.

[reed1, 2014-05-06 17:09:42]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-05-06 17:10:01]

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

[commit-bot: I haz the power, 2014-05-06 17:16:07]

Change committed as 14592