Move ViewMatrix off of drawstate

Move ViewMatrix off of drawstate

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/8059eb9f6e24ed609393fbda4ad71edea03ac258

[joshualitt, 2014-12-29 17:31:19]

joshualitt@google.com changed reviewers:
+ bsalomon@google.com, joshualitt@google.com, jvanverth@google.com

[joshualitt, 2014-12-29 17:31:20]

This broad but shallow change moves the drawstate to the geometry processor. 
The follow on CL will remove the viewmatrix from the optdrawstate.

Adding Jim for the very minor changes to textcontexts.

[bsalomon, 2014-12-29 18:35:46]

https://codereview.chromium.org/815553003/diff/40001/src/gpu/GrClipMaskManage...
File src/gpu/GrClipMaskManager.cpp (right):

https://codereview.chromium.org/815553003/diff/40001/src/gpu/GrClipMaskManage...
src/gpu/GrClipMaskManager.cpp:97: if (path_needs_SW_renderer(this->getContext(),
fClipTarget, drawState, viewMatrix,
So AFAICT you tunneled the original draw's matrix down to here. I don't think
that is right. It should be the matrix for the clipping elements, which is
either I() or translate only. It should be the same matrix we pass to
drawElement(). I don't think setupClipping() needs a matrix at all (which is
nice because I saw we're querying GP for the vm, and I don't think we should
ever have to do that once GPs fully apply the matrix themselves).

https://codereview.chromium.org/815553003/diff/40001/src/gpu/effects/GrDashin...
File src/gpu/effects/GrDashingEffect.cpp (right):

https://codereview.chromium.org/815553003/diff/40001/src/gpu/effects/GrDashin...
src/gpu/effects/GrDashingEffect.cpp:356: gp.reset(GrDashingEffect::Create(color,
SkMatrix::I(), edgeType, devInfo, strokeWidth,
For GPs that are only ever instantiated with I() should we just remove the param
from the GP Create function? I imagine most of these GPs won't work with a
general matrix. When the GP subclasses are fully handling the matrices
(inserting a uni or whatever) it might simplify things for them to just assume
their position attrs are already in device space.

[joshualitt, 2014-12-29 19:23:38]

https://codereview.chromium.org/815553003/diff/40001/src/gpu/GrClipMaskManage...
File src/gpu/GrClipMaskManager.cpp (right):

https://codereview.chromium.org/815553003/diff/40001/src/gpu/GrClipMaskManage...
src/gpu/GrClipMaskManager.cpp:97: if (path_needs_SW_renderer(this->getContext(),
fClipTarget, drawState, viewMatrix,
On 2014/12/29 18:35:46, bsalomon wrote:
> So AFAICT you tunneled the original draw's matrix down to here. I don't think
> that is right. It should be the matrix for the clipping elements, which is
> either I() or translate only. It should be the same matrix we pass to
> drawElement().

Brian, are you sure about this?

I could be wrong about this, but the original call stack is
setupClipping->useSWOnlyPath->path_needs_SW_renderer->getPathRenderer

getPathRenderer is what needs the viewMatrix.  In the above call stack, we were
passing the original drawstate passed in from the drawTarget, because we hadn't
yet called createAlphaClipMask(which is what calls drawElement).

What am I missing?

https://codereview.chromium.org/815553003/diff/40001/src/gpu/effects/GrDashin...
File src/gpu/effects/GrDashingEffect.cpp (right):

https://codereview.chromium.org/815553003/diff/40001/src/gpu/effects/GrDashin...
src/gpu/effects/GrDashingEffect.cpp:356: gp.reset(GrDashingEffect::Create(color,
SkMatrix::I(), edgeType, devInfo, strokeWidth,
On 2014/12/29 18:35:46, bsalomon wrote:
> For GPs that are only ever instantiated with I() should we just remove the
param
> from the GP Create function? I imagine most of these GPs won't work with a
> general matrix. When the GP subclasses are fully handling the matrices
> (inserting a uni or whatever) it might simplify things for them to just assume
> their position attrs are already in device space.

Acknowledged.

[bsalomon, 2014-12-29 19:29:01]

https://codereview.chromium.org/815553003/diff/40001/src/gpu/GrClipMaskManage...
File src/gpu/GrClipMaskManager.cpp (right):

https://codereview.chromium.org/815553003/diff/40001/src/gpu/GrClipMaskManage...
src/gpu/GrClipMaskManager.cpp:97: if (path_needs_SW_renderer(this->getContext(),
fClipTarget, drawState, viewMatrix,
On 2014/12/29 19:23:38, joshualitt wrote:
> On 2014/12/29 18:35:46, bsalomon wrote:
> > So AFAICT you tunneled the original draw's matrix down to here. I don't
think
> > that is right. It should be the matrix for the clipping elements, which is
> > either I() or translate only. It should be the same matrix we pass to
> > drawElement().
> 
> Brian, are you sure about this?
> 
> I could be wrong about this, but the original call stack is
> setupClipping->useSWOnlyPath->path_needs_SW_renderer->getPathRenderer
> 
> getPathRenderer is what needs the viewMatrix.  In the above call stack, we
were
> passing the original drawstate passed in from the drawTarget, because we
hadn't
> yet called createAlphaClipMask(which is what calls drawElement).
> 
> What am I missing?

Hmm... that seems incorrect. If we were using the original ds then that was a
bug. The code is supposed to be checking whether if we went down the
createAlphaClipMask path we would sw raster a path. If so we instead render the
entire clip in sw and upload. It's supposed to figure out what would happen in
createAlphaClipMask.

[joshualitt, 2014-12-29 19:50:32]

On 2014/12/29 19:29:01, bsalomon wrote:
>
https://codereview.chromium.org/815553003/diff/40001/src/gpu/GrClipMaskManage...
> File src/gpu/GrClipMaskManager.cpp (right):
> 
>
https://codereview.chromium.org/815553003/diff/40001/src/gpu/GrClipMaskManage...
> src/gpu/GrClipMaskManager.cpp:97: if
(path_needs_SW_renderer(this->getContext(),
> fClipTarget, drawState, viewMatrix,
> On 2014/12/29 19:23:38, joshualitt wrote:
> > On 2014/12/29 18:35:46, bsalomon wrote:
> > > So AFAICT you tunneled the original draw's matrix down to here. I don't
> think
> > > that is right. It should be the matrix for the clipping elements, which is
> > > either I() or translate only. It should be the same matrix we pass to
> > > drawElement().
> > 
> > Brian, are you sure about this?
> > 
> > I could be wrong about this, but the original call stack is
> > setupClipping->useSWOnlyPath->path_needs_SW_renderer->getPathRenderer
> > 
> > getPathRenderer is what needs the viewMatrix.  In the above call stack, we
> were
> > passing the original drawstate passed in from the drawTarget, because we
> hadn't
> > yet called createAlphaClipMask(which is what calls drawElement).
> > 
> > What am I missing?
> 
> Hmm... that seems incorrect. If we were using the original ds then that was a
> bug. The code is supposed to be checking whether if we went down the
> createAlphaClipMask path we would sw raster a path. If so we instead render
the
> entire clip in sw and upload. It's supposed to figure out what would happen in
> createAlphaClipMask.

feedback incorporated

[bsalomon, 2014-12-29 20:26:58]

https://codereview.chromium.org/815553003/diff/60001/src/gpu/GrClipMaskManage...
File src/gpu/GrClipMaskManager.cpp (right):

https://codereview.chromium.org/815553003/diff/60001/src/gpu/GrClipMaskManage...
src/gpu/GrClipMaskManager.cpp:83: const SkMatrix& viewMatrix,
Maybe take the skvector translate here instead of a matrix?

https://codereview.chromium.org/815553003/diff/60001/src/gpu/GrClipMaskManage...
src/gpu/GrClipMaskManager.cpp:526: const SkMatrix& translate,
Seems weird to pass essentially the same thing twice. Maybe recreate the
translate matrix in this function? Pretty minimal work and clear IMO.

https://codereview.chromium.org/815553003/diff/60001/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

https://codereview.chromium.org/815553003/diff/60001/src/gpu/GrContext.cpp#ne...
src/gpu/GrContext.cpp:756: invert,
Seems like every caller of the public drawing functions on GrAARR passes both a
VM and its inverse. Is it necessary or can the GrAARR do the inverting?

[joshualitt, 2014-12-29 20:54:29]

On 2014/12/29 20:26:58, bsalomon wrote:
>
https://codereview.chromium.org/815553003/diff/60001/src/gpu/GrClipMaskManage...
> File src/gpu/GrClipMaskManager.cpp (right):
> 
>
https://codereview.chromium.org/815553003/diff/60001/src/gpu/GrClipMaskManage...
> src/gpu/GrClipMaskManager.cpp:83: const SkMatrix& viewMatrix,
> Maybe take the skvector translate here instead of a matrix?
> 
>
https://codereview.chromium.org/815553003/diff/60001/src/gpu/GrClipMaskManage...
> src/gpu/GrClipMaskManager.cpp:526: const SkMatrix& translate,
> Seems weird to pass essentially the same thing twice. Maybe recreate the
> translate matrix in this function? Pretty minimal work and clear IMO.
> 
> https://codereview.chromium.org/815553003/diff/60001/src/gpu/GrContext.cpp
> File src/gpu/GrContext.cpp (right):
> 
>
https://codereview.chromium.org/815553003/diff/60001/src/gpu/GrContext.cpp#ne...
> src/gpu/GrContext.cpp:756: invert,
> Seems like every caller of the public drawing functions on GrAARR passes both
a
> VM and its inverse. Is it necessary or can the GrAARR do the inverting?

feedback incorporated

[bsalomon, 2014-12-29 22:08:56]

On 2014/12/29 20:54:29, joshualitt wrote:
> On 2014/12/29 20:26:58, bsalomon wrote:
> >
>
https://codereview.chromium.org/815553003/diff/60001/src/gpu/GrClipMaskManage...
> > File src/gpu/GrClipMaskManager.cpp (right):
> > 
> >
>
https://codereview.chromium.org/815553003/diff/60001/src/gpu/GrClipMaskManage...
> > src/gpu/GrClipMaskManager.cpp:83: const SkMatrix& viewMatrix,
> > Maybe take the skvector translate here instead of a matrix?
> > 
> >
>
https://codereview.chromium.org/815553003/diff/60001/src/gpu/GrClipMaskManage...
> > src/gpu/GrClipMaskManager.cpp:526: const SkMatrix& translate,
> > Seems weird to pass essentially the same thing twice. Maybe recreate the
> > translate matrix in this function? Pretty minimal work and clear IMO.
> > 
> > https://codereview.chromium.org/815553003/diff/60001/src/gpu/GrContext.cpp
> > File src/gpu/GrContext.cpp (right):
> > 
> >
>
https://codereview.chromium.org/815553003/diff/60001/src/gpu/GrContext.cpp#ne...
> > src/gpu/GrContext.cpp:756: invert,
> > Seems like every caller of the public drawing functions on GrAARR passes
both
> a
> > VM and its inverse. Is it necessary or can the GrAARR do the inverting?
> 
> feedback incorporated

lgtm

[joshualitt, 2014-12-29 22:59:39]

The CQ bit was checked by joshualitt@google.com

[commit-bot: I haz the power, 2014-12-29 22:59:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/815553003/80001

[commit-bot: I haz the power, 2014-12-29 23:10:11]

Committed patchset #5 (id:80001) as
https://skia.googlesource.com/skia/+/8059eb9f6e24ed609393fbda4ad71edea03ac258