Make GPU coord transforms automatic

Make GPU coord transforms automatic

Adds a GrCoordTransform class and updates the framework to handle
coord transforms similar to how it handles textures with
GrTextureAccess. Renames GrGLEffectMatrix to GrGLCoordTransform and
slightly repurposes it to be used by the framework instead of effects.

R=bsalomon@google.com, robertphillips@google.com

Committed: https://code.google.com/p/skia/source/detail?r=11569

[Chris Dalton, 2013-09-26 21:46:39]

I went ahead abandoned the old one to make a new commit.

Generally I tried to make the changes that had the least impact on the
surrounding code.

[bsalomon, 2013-09-27 19:23:57]

I've started taking a look (comments inline) and have made it maybe 1/3 of the
way through.

Jim + Rob, I realize this is a long review, but I'd really like you guys to at
least look at the .h files and an effect or two that uses the new system. We're
going to live with this for a long time so let's make sure we're happy.

Background:
User installed effects (those on GrPaint) don't need access to the vertex shader
as they have no concept of vertices. Within GrContext we can tessellate however
we like. This is convenient because NVPR has no vertex shader (for the same
reason - paths have no vertices).

Effects do want to be able to access transformed local coordinates, though. E.g.
to implement an SkBitmapProcShader. Currently they rely on using
GrGLEffectMatrix to do this. GrGLEffectMatrix inserts the transformation into
the VS using the GrGLShaderBuilder and has helpers for effects to upload matrix
uniforms and account for the matrix code in the effect key. It also hides two
nasty pieces of business: y-flips for upside down textures and accounting for
the coord-change-matrix that compensates for any coordinate system changes done
by GrContext since the effect was installed.

Chris's change effectively promoted GrGLEffectMatrix from a helper class to a
fundamental building block of the shader generation system. This allows the
system to express the matrices as TexGen matrices in the context of NVPR. It
also allows the system to automatically upload matrices to uniforms and
accounting for different code generated for different matrices in the overall
program cache key. This is pretty nice because we have lots of effects doing the
same rote tasks using GrGLEffectMatrix.

This will make NVPR integration possible and also should remove a lot of
boilerplate code from effect subclasses.

https://codereview.chromium.org/24853002/diff/1/include/gpu/GrCoordTransform.h
File include/gpu/GrCoordTransform.h (right):

https://codereview.chromium.org/24853002/diff/1/include/gpu/GrCoordTransform....
include/gpu/GrCoordTransform.h:19: * work to make the transformed coordinates
available in their shaders.
... ^fragment shaders.

https://codereview.chromium.org/24853002/diff/1/include/gpu/GrCoordTransform....
include/gpu/GrCoordTransform.h:29: GrCoordTransform(GrCoordSet sourceCoords,
const SkMatrix& m, const GrTexture* texture) {
Maybe some documentation that the texture is just used to possibly infer a y
reversal to account for textures that are upside down by Skia's coord
convention.

https://codereview.chromium.org/24853002/diff/1/include/gpu/GrCoordTransform....
include/gpu/GrCoordTransform.h:33: GrCoordTransform(GrCoordSet sourceCoords,
const SkMatrix& m, bool reverseY = false) {
Is this constructor used in practice?

https://codereview.chromium.org/24853002/diff/1/include/gpu/GrCoordTransform....
include/gpu/GrCoordTransform.h:49: void reset(GrCoordSet sourceCoords, const
SkMatrix& m, bool reverseY = false) {
If this version could be private that would seem nice.

https://codereview.chromium.org/24853002/diff/1/include/gpu/GrTypes.h
File include/gpu/GrTypes.h (right):

https://codereview.chromium.org/24853002/diff/1/include/gpu/GrTypes.h#newcode244
include/gpu/GrTypes.h:244: * Built-in coordinate sets provided by the framework.
Does anything get worse if this is moved to a less fundamental header, (e.g.
GrEffect.h or GrCoodTransform.h)? I've got no problem with it being non-nested
but I don't imagine this is useful for a user of Ganesh that isn't extending the
effect system and might just serve as a point of confusion.

Could use a little more explanation:

/**
 * Coordinates available to a GrEffect subclasses. Effects express matrices via
a GrCoordTransform that are applied to these coordinates. The transformed
coordinates are made available in the the portion of fragment shader emitted by
the effect.
 */
enum GrCoordSet {
/** The src-space coordinate that maps to the fragment being rendered. These
coords account for any change of coordinate system done on the CPU by GrContext
before rendering and also are correct for draws that take explicit local coords
rather than inferring them from the primitive's positions (e.g. drawVertices).
These are usually the coords desired by a GrEffect. */

kLocal_GrCoordSet,
/** The actual vertex position. Note that GrContext may not draw using the
original view matrix specified by when a caller draws as it may have transformed
vertices into another space. This is usually not the coordinates desired by a
GrEffect.*/
kPosition_GrCoordSet
};

Also, I kinda like GrCoordType or simply GrCoords over GrCoordSet... Don't
really love any of them.

https://codereview.chromium.org/24853002/diff/1/src/core/SkXfermode.cpp
File src/core/SkXfermode.cpp (right):

https://codereview.chromium.org/24853002/diff/1/src/core/SkXfermode.cpp#newco...
src/core/SkXfermode.cpp:1090: EffectKey modeKey = xfer.mode();
Ahhh... so much nicer.

maybe simply

return drawEffect.castEffect<XferEffect>().mode();

https://codereview.chromium.org/24853002/diff/1/src/gpu/gl/GrGLCoordTransform.h
File src/gpu/gl/GrGLCoordTransform.h (right):

https://codereview.chromium.org/24853002/diff/1/src/gpu/gl/GrGLCoordTransform...
src/gpu/gl/GrGLCoordTransform.h:2: * Copyright 2012 Google Inc.
2013

[bsalomon, 2013-09-27 20:04:25]

Other than the million effects that are modified, this is a lot less code change
than I expected. Nice!

https://codereview.chromium.org/24853002/diff/1/src/effects/SkPerlinNoiseShad...
File src/effects/SkPerlinNoiseShader.cpp (right):

https://codereview.chromium.org/24853002/diff/1/src/effects/SkPerlinNoiseShad...
src/effects/SkPerlinNoiseShader.cpp:608: // (as opposed to 0 based, usually).
The same adjustment is in the shadeSpan() functions.
OMG (just in general, not directed at this change).

https://codereview.chromium.org/24853002/diff/1/src/gpu/gl/GrGLCoordTransform.h
File src/gpu/gl/GrGLCoordTransform.h (right):

https://codereview.chromium.org/24853002/diff/1/src/gpu/gl/GrGLCoordTransform...
src/gpu/gl/GrGLCoordTransform.h:32: private:
We usually do public / protected / private

https://codereview.chromium.org/24853002/diff/1/src/gpu/gl/GrGLEffect.cpp
File src/gpu/gl/GrGLEffect.cpp (right):

https://codereview.chromium.org/24853002/diff/1/src/gpu/gl/GrGLEffect.cpp#new...
src/gpu/gl/GrGLEffect.cpp:48: SkASSERT(0 == (value & key)); // keys for each
access ought not to overlap
keys for each *transform*...

https://codereview.chromium.org/24853002/diff/1/src/gpu/gl/GrGLEffect.h
File src/gpu/gl/GrGLEffect.h (right):

https://codereview.chromium.org/24853002/diff/1/src/gpu/gl/GrGLEffect.h#newco...
src/gpu/gl/GrGLEffect.h:94: static inline EffectKey GenKey(const GrDrawEffect&,
const GrGLCaps&) { return 0; }
does this get called?

https://codereview.chromium.org/24853002/diff/1/src/gpu/gl/GrGLShaderBuilder.h
File src/gpu/gl/GrGLShaderBuilder.h (right):

https://codereview.chromium.org/24853002/diff/1/src/gpu/gl/GrGLShaderBuilder....
src/gpu/gl/GrGLShaderBuilder.h:235: SkString ensureFSCoords2D(const
TransformedCoordsArray&, int index);
Might want to document that this may emit code into the FS so some unknowning
caller doesn't invoke it when they've emitted a half-finished line of code.

[jvanverth1, 2013-09-27 20:41:34]

Looks reasonable to me. The only thing that bothers me a little is that we're
bundling the matrix even in the case where we don't need it (positions). But I
don't see a clear way to avoid that.

+nits

https://codereview.chromium.org/24853002/diff/1/include/gpu/GrBackendEffectFa...
File include/gpu/GrBackendEffectFactory.h (right):

https://codereview.chromium.org/24853002/diff/1/include/gpu/GrBackendEffectFa...
include/gpu/GrBackendEffectFactory.h:38: * The framework automatically includes
coord transfors and texture accesses in their
Typo: transfors

https://codereview.chromium.org/24853002/diff/1/include/gpu/GrEffect.h
File include/gpu/GrEffect.h (right):

https://codereview.chromium.org/24853002/diff/1/include/gpu/GrEffect.h#newcod...
include/gpu/GrEffect.h:207: * pointer). The GrCoordTransform is typically a
member field of the GrEffet subclass. When the
Typo: GrEffet

https://codereview.chromium.org/24853002/diff/1/src/core/SkXfermode.cpp
File src/core/SkXfermode.cpp (right):

https://codereview.chromium.org/24853002/diff/1/src/core/SkXfermode.cpp#newco...
src/core/SkXfermode.cpp:938: : GrGLEffect(factory ) {
space

[Chris Dalton, 2013-09-27 23:33:44]

Thanks for the reviews everybody!

Patchset 2 should address all the comments except renaming GrCoordSet (that
should make it easier anyway).

When I moved the GrCoordSet enum to GrCoordTransform.h, I ended up having to
include that .h in GrGLCoortTransform.h, so I went ahead and updated it to use
GrCoordTransform instead of getting passed raw matrices.

WRT bundling the matrix even in the case where we don't need it, that's how skia
gpu has always worked, so I believe it's ok to put it off for a later commit.

This time it also detected the rename from GrGLEffectMatrix.h ->
GrGLCoordTransform.h.

https://codereview.chromium.org/24853002/diff/1/include/gpu/GrBackendEffectFa...
File include/gpu/GrBackendEffectFactory.h (right):

https://codereview.chromium.org/24853002/diff/1/include/gpu/GrBackendEffectFa...
include/gpu/GrBackendEffectFactory.h:38: * The framework automatically includes
coord transfors and texture accesses in their
On 2013/09/27 20:41:35, JimVV wrote:
> Typo: transfors

Done.

https://codereview.chromium.org/24853002/diff/1/include/gpu/GrCoordTransform.h
File include/gpu/GrCoordTransform.h (right):

https://codereview.chromium.org/24853002/diff/1/include/gpu/GrCoordTransform....
include/gpu/GrCoordTransform.h:19: * work to make the transformed coordinates
available in their shaders.
On 2013/09/27 19:23:57, bsalomon wrote:
> ... ^fragment shaders.

Done.

https://codereview.chromium.org/24853002/diff/1/include/gpu/GrCoordTransform....
include/gpu/GrCoordTransform.h:29: GrCoordTransform(GrCoordSet sourceCoords,
const SkMatrix& m, const GrTexture* texture) {
On 2013/09/27 19:23:57, bsalomon wrote:
> Maybe some documentation that the texture is just used to possibly infer a y
> reversal to account for textures that are upside down by Skia's coord
> convention.

Done.

https://codereview.chromium.org/24853002/diff/1/include/gpu/GrCoordTransform....
include/gpu/GrCoordTransform.h:33: GrCoordTransform(GrCoordSet sourceCoords,
const SkMatrix& m, bool reverseY = false) {
On 2013/09/27 19:23:57, bsalomon wrote:
> Is this constructor used in practice?

Removed.

https://codereview.chromium.org/24853002/diff/1/include/gpu/GrCoordTransform....
include/gpu/GrCoordTransform.h:49: void reset(GrCoordSet sourceCoords, const
SkMatrix& m, bool reverseY = false) {
On 2013/09/27 19:23:57, bsalomon wrote:
> If this version could be private that would seem nice.

Done.

https://codereview.chromium.org/24853002/diff/1/include/gpu/GrEffect.h
File include/gpu/GrEffect.h (right):

https://codereview.chromium.org/24853002/diff/1/include/gpu/GrEffect.h#newcod...
include/gpu/GrEffect.h:207: * pointer). The GrCoordTransform is typically a
member field of the GrEffet subclass. When the
On 2013/09/27 20:41:35, JimVV wrote:
> Typo: GrEffet

Done.

https://codereview.chromium.org/24853002/diff/1/src/core/SkXfermode.cpp
File src/core/SkXfermode.cpp (right):

https://codereview.chromium.org/24853002/diff/1/src/core/SkXfermode.cpp#newco...
src/core/SkXfermode.cpp:938: : GrGLEffect(factory ) {
On 2013/09/27 20:41:35, JimVV wrote:
> space

Done. (I left it there intentionally trying to minimize unrelated code changes.)

https://codereview.chromium.org/24853002/diff/1/src/core/SkXfermode.cpp#newco...
src/core/SkXfermode.cpp:1090: EffectKey modeKey = xfer.mode();
On 2013/09/27 19:23:57, bsalomon wrote:
> Ahhh... so much nicer.
> 
> maybe simply
> 
> return drawEffect.castEffect<XferEffect>().mode();

Done.

https://codereview.chromium.org/24853002/diff/1/src/effects/SkPerlinNoiseShad...
File src/effects/SkPerlinNoiseShader.cpp (right):

https://codereview.chromium.org/24853002/diff/1/src/effects/SkPerlinNoiseShad...
src/effects/SkPerlinNoiseShader.cpp:608: // (as opposed to 0 based, usually).
The same adjustment is in the shadeSpan() functions.
On 2013/09/27 20:04:25, bsalomon wrote:
> OMG (just in general, not directed at this change).

:)

https://codereview.chromium.org/24853002/diff/1/src/gpu/gl/GrGLCoordTransform.h
File src/gpu/gl/GrGLCoordTransform.h (right):

https://codereview.chromium.org/24853002/diff/1/src/gpu/gl/GrGLCoordTransform...
src/gpu/gl/GrGLCoordTransform.h:2: * Copyright 2012 Google Inc.
On 2013/09/27 19:23:57, bsalomon wrote:
> 2013

This is basically just a rename of GrGLEffectMatrix. When I re-upload I'll try
to make sure git notices that.

https://codereview.chromium.org/24853002/diff/1/src/gpu/gl/GrGLCoordTransform...
src/gpu/gl/GrGLCoordTransform.h:32: private:
On 2013/09/27 20:04:25, bsalomon wrote:
> We usually do public / protected / private

This is basically just a rename of GrGLEffectMatrix. When I re-upload I'll try
to make sure git notices that.

https://codereview.chromium.org/24853002/diff/1/src/gpu/gl/GrGLEffect.cpp
File src/gpu/gl/GrGLEffect.cpp (right):

https://codereview.chromium.org/24853002/diff/1/src/gpu/gl/GrGLEffect.cpp#new...
src/gpu/gl/GrGLEffect.cpp:48: SkASSERT(0 == (value & key)); // keys for each
access ought not to overlap
On 2013/09/27 20:04:25, bsalomon wrote:
> keys for each *transform*...

Done.

https://codereview.chromium.org/24853002/diff/1/src/gpu/gl/GrGLEffect.h
File src/gpu/gl/GrGLEffect.h (right):

https://codereview.chromium.org/24853002/diff/1/src/gpu/gl/GrGLEffect.h#newco...
src/gpu/gl/GrGLEffect.h:94: static inline EffectKey GenKey(const GrDrawEffect&,
const GrGLCaps&) { return 0; }
On 2013/09/27 20:04:25, bsalomon wrote:
> does this get called?

Yeah. A lot of effects don't need to make their own GenKey method anymore.
(That's why I modified the comment at the top to say "If they don't always emit
the same GLSL code," they must have a GenKey function ...)

https://codereview.chromium.org/24853002/diff/1/src/gpu/gl/GrGLShaderBuilder.h
File src/gpu/gl/GrGLShaderBuilder.h (right):

https://codereview.chromium.org/24853002/diff/1/src/gpu/gl/GrGLShaderBuilder....
src/gpu/gl/GrGLShaderBuilder.h:235: SkString ensureFSCoords2D(const
TransformedCoordsArray&, int index);
On 2013/09/27 20:04:25, bsalomon wrote:
> Might want to document that this may emit code into the FS so some unknowning
> caller doesn't invoke it when they've emitted a half-finished line of code.

Done.

[bsalomon, 2013-09-30 13:24:40]

lgtm.

I couldn't think of a better name for the coordinate enum.

Have you run the the unit test GLPrograms?

https://codereview.chromium.org/24853002/diff/1/src/gpu/gl/GrGLCoordTransform.h
File src/gpu/gl/GrGLCoordTransform.h (right):

https://codereview.chromium.org/24853002/diff/1/src/gpu/gl/GrGLCoordTransform...
src/gpu/gl/GrGLCoordTransform.h:32: private:
On 2013/09/27 23:33:45, Chris Dalton wrote:
> On 2013/09/27 20:04:25, bsalomon wrote:
> > We usually do public / protected / private
> 
> This is basically just a rename of GrGLEffectMatrix. When I re-upload I'll try
> to make sure git notices that.

Ok.

https://codereview.chromium.org/24853002/diff/1/src/gpu/gl/GrGLEffect.h
File src/gpu/gl/GrGLEffect.h (right):

https://codereview.chromium.org/24853002/diff/1/src/gpu/gl/GrGLEffect.h#newco...
src/gpu/gl/GrGLEffect.h:94: static inline EffectKey GenKey(const GrDrawEffect&,
const GrGLCaps&) { return 0; }
On 2013/09/27 23:33:45, Chris Dalton wrote:
> On 2013/09/27 20:04:25, bsalomon wrote:
> > does this get called?
> 
> Yeah. A lot of effects don't need to make their own GenKey method anymore.
> (That's why I modified the comment at the top to say "If they don't always
emit
> the same GLSL code," they must have a GenKey function ...)

Oh wow. I don't think I ever seen this pattern of static function overloading,
but it makes sense.

[robertphillips, 2013-09-30 14:26:54]

lgtm

[Chris Dalton, 2013-09-30 14:58:29]

This change added an assert exposed an existing bug. 5 bits wasn't enough for
the class id. Patchset 3 adjusts that. The GLProgram test passes now.

[commit-bot: I haz the power, 2013-09-30 14:59:57]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/cdalton@nvidia.com/24853002/70001

[commit-bot: I haz the power, 2013-09-30 15:00:11]

Failed to apply patch for src/gpu/gl/GrGLCoordTransform.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  A         src/gpu/gl/GrGLCoordTransform.cpp
  Copied src/gpu/gl/GrGLEffectMatrix.cpp -> src/gpu/gl/GrGLCoordTransform.cpp
  patching file src/gpu/gl/GrGLCoordTransform.cpp
  Hunk #3 FAILED at 44.
  Hunk #4 succeeded at 82 (offset 1 line).
  Hunk #5 succeeded at 95 (offset 1 line).
  Hunk #6 succeeded at 169 (offset 1 line).
  1 out of 6 hunks FAILED -- saving rejects to file
src/gpu/gl/GrGLCoordTransform.cpp.rej

Patch:   NR  src/gpu/gl/GrGLEffectMatrix.cpp->src/gpu/gl/GrGLCoordTransform.cpp
Index: src/gpu/gl/GrGLCoordTransform.cpp
diff --git a/src/gpu/gl/GrGLEffectMatrix.cpp b/src/gpu/gl/GrGLCoordTransform.cpp
similarity index 58%
rename from src/gpu/gl/GrGLEffectMatrix.cpp
rename to src/gpu/gl/GrGLCoordTransform.cpp
index
880df12acffb0e8850c7b647d43e0b4a87aa8c55..bfbae1e9a3bca6445fbb0294ec058a989e808ba5
100644
--- a/src/gpu/gl/GrGLEffectMatrix.cpp
+++ b/src/gpu/gl/GrGLCoordTransform.cpp
@@ -5,23 +5,24 @@
  * found in the LICENSE file.
  */
 
-#include "GrGLEffectMatrix.h"
+#include "GrGLCoordTransform.h"
 #include "GrDrawEffect.h"
 #include "GrTexture.h"
+#include "GrGLShaderBuilder.h"
 
-GrGLEffect::EffectKey GrGLEffectMatrix::GenKey(const SkMatrix& effectMatrix,
-                                               const GrDrawEffect& drawEffect,
-                                               CoordsType coordsType,
-                                               const GrTexture* texture) {
+GrGLCoordTransform::EffectKey GrGLCoordTransform::GenKey(const GrDrawEffect&
drawEffect,
+                                                         int transformIdx) {
+    const GrCoordTransform& transform =
(*drawEffect.effect())->coordTransform(transformIdx);
     EffectKey key = 0;
-    SkMatrix::TypeMask type0 = effectMatrix.getType();
+    SkMatrix::TypeMask type0 = transform.getMatrix().getType();
     SkMatrix::TypeMask type1;
-    if (GrEffect::kLocal_CoordsType == coordsType) {
+    if (kLocal_GrCoordSet == transform.sourceCoords()) {
         type1 = drawEffect.getCoordChangeMatrix().getType();
     } else {
         if (drawEffect.programHasExplicitLocalCoords()) {
             // We only make the key indicate that device coords are referenced
when the local coords
-            // are not actually determined by positions.
+            // are not actually determined by positions. Otherwise the local
coords var and position
+            // var are identical.
             key |= kPositionCoords_Flag;
         }
         type1 = SkMatrix::kIdentity_Mask;
@@ -29,7 +30,7 @@ GrGLEffect::EffectKey GrGLEffectMatrix::GenKey(const SkMatrix&
effectMatrix,
 
     int combinedTypes = type0 | type1;
 
-    bool reverseY = (NULL != texture) && kBottomLeft_GrSurfaceOrigin ==
texture->origin();
+    bool reverseY = transform.reverseY();
 
     if (SkMatrix::kPerspective_Mask & combinedTypes) {
         key |= kGeneral_MatrixType;
@@ -43,17 +44,15 @@ GrGLEffect::EffectKey GrGLEffectMatrix::GenKey(const
SkMatrix& effectMatrix,
     return key;
 }
 
-GrSLType GrGLEffectMatrix::emitCode(GrGLShaderBuilder* builder,
-                                    EffectKey key,
-                                    SkString* fsCoordName,
-                                    SkString* vsCoordName,
-                                    const char* suffix) {
+void GrGLCoordTransform::emitCode(GrGLShaderBuilder* builder,
+                                  EffectKey key,
+                                  TransformedCoords* transformedCoords,
+                                  int suffix) {
     GrGLShaderBuilder::VertexBuilder* vertexBuilder =
builder->getVertexBuilder();
     SkASSERT(NULL != vertexBuilder);
 
     GrSLType varyingType = kVoid_GrSLType;
     const char* uniName;
-    key &= kKeyMask;
     switch (key & kMatrixTypeKeyMask) {
         case kIdentity_MatrixType:
             fUniType = kVoid_GrSLType;
@@ -80,9 +79,9 @@ GrSLType GrGLEffectMatrix::emitCode(GrGLShaderBuilder*
builder,
     }
     SkString suffixedUniName;
     if (kVoid_GrSLType != fUniType) {
-        if (NULL != suffix) {
+        if (0 != suffix) {
             suffixedUniName.append(uniName);
-            suffixedUniName.append(suffix);
+            suffixedUniName.appendf("_%i", suffix);
             uniName = suffixedUniName.c_str();
         }
         fUni = builder->addUniform(GrGLShaderBuilder::kVertex_Visibility,
@@ -93,119 +92,67 @@ GrSLType GrGLEffectMatrix::emitCode(GrGLShaderBuilder*
builder,
 
     const char* varyingName = "MatrixCoord";
     SkString suffixedVaryingName;
-    if (NULL != suffix) {
+    if (0 != suffix) {
         suffixedVaryingName.append(varyingName);
-        suffixedVaryingName.append(suffix);
+        suffixedVaryingName.appendf("_%i", suffix);
         varyingName = suffixedVaryingName.c_str();
     }
     const char* vsVaryingName;
     const char* fsVaryingName;
     vertexBuilder->addVarying(varyingType, varyingName, &vsVaryingName,
&fsVaryingName);
 
-    const GrGLShaderVar* coords;
-    switch (fCoordsType) {
-        case GrEffect::kLocal_CoordsType:
-            SkASSERT(!(kPositionCoords_Flag & key));
-            coords = &vertexBuilder->localCoordsAttribute();
-            break;
-        case GrEffect::kPosition_CoordsType:
-            SkASSERT((kPositionCoords_Flag & key) ||
!vertexBuilder->hasExplicitLocalCoords());
-            coords = &vertexBuilder->positionAttribute();
-            break;
-        default:
-            coords = NULL; // prevents warning
-            GrCrash("Unexpected coords type.");
-    }
+    const GrGLShaderVar& coords = (kPositionCoords_Flag & key) ?
+                                      vertexBuilder->positionAttribute() :
+                                      vertexBuilder->localCoordsAttribute();
     // varying = matrix * coords (logically)
     switch (fUniType) {
         case kVoid_GrSLType:
             SkASSERT(kVec2f_GrSLType == varyingType);
-            vertexBuilder->vsCodeAppendf("\t%s = %s;\n", vsVaryingName,
coords->c_str());
+            vertexBuilder->vsCodeAppendf("\t%s = %s;\n", vsVaryingName,
coords.c_str());
             break;
         case kVec2f_GrSLType:
             SkASSERT(kVec2f_GrSLType == varyingType);
             vertexBuilder->vsCodeAppendf("\t%s = %s + %s;\n",
-                                         vsVaryingName, uniName,
coords->c_str());
+                                         vsVaryingName, uniName,
coords.c_str());
             break;
         case kMat33f_GrSLType: {
             SkASSERT(kVec2f_GrSLType == varyingType || kVec3f_GrSLType ==
varyingType);
             if (kVec2f_GrSLType == varyingType) {
                 vertexBuilder->vsCodeAppendf("\t%s = (%s * vec3(%s, 1)).xy;\n",
-                                             vsVaryingName, uniName,
coords->c_str());
+                                             vsVaryingName, uniName,
coords.c_str());
             } else {
                 vertexBuilder->vsCodeAppendf("\t%s = %s * vec3(%s, 1);\n",
-                                             vsVaryingName, uniName,
coords->c_str());
+                                             vsVaryingName, uniName,
coords.c_str());
             }
             break;
         }
         default:
             GrCrash("Unexpected uniform type.");
     }
-    if (NULL != vsCoordName) {
-        *vsCoordName = vsVaryingName;
-    }
-    if (NULL != fsCoordName) {
-        *fsCoordName = fsVaryingName;
-    }
-    return varyingType;
-}
-
-/**
-    * This is similar to emitCode except that it performs perspective division
in the FS if the
-    * texture coordinates have a w coordinate. The fsCoordName always refers to
a vec2f.
-    */
-void GrGLEffectMatrix::emitCodeMakeFSCoords2D(GrGLShaderBuilder* builder,
-                                              EffectKey key,
-                                              SkString* fsCoordName,
-                                              SkString* vsVaryingName,
-                                              GrSLType* vsVaryingType,
-                                              const char* suffix) {
-    SkString fsVaryingName;
-
-    GrSLType varyingType = this->emitCode(builder,
-                                          key,
-                                          &fsVaryingName,
-                                          vsVaryingName,
-                                          suffix);
-    if (kVec3f_GrSLType == varyingType) {
-
-        const char* coordName = "coords2D";
-        SkString suffixedCoordName;
-        if (NULL != suffix) {
-            suffixedCoordName.append(coordName);
-            suffixedCoordName.append(suffix);
-            coordName = suffixedCoordName.c_str();
-        }
-        builder->fsCodeAppendf("\tvec2 %s = %s.xy / %s.z;",
-                               coordName, fsVaryingName.c_str(),
fsVaryingName.c_str());
-        if (NULL != fsCoordName) {
-            *fsCoordName = coordName;
-        }
-    } else if(NULL != fsCoordName) {
-        *fsCoordName = fsVaryingName;
-    }
-    if (NULL != vsVaryingType) {
-        *vsVaryingType = varyingType;
-    }
+    SkASSERT(NULL != transformedCoords);
+    transformedCoords->fName = fsVaryingName;
+    transformedCoords->fType = varyingType;
+    transformedCoords->fVSName = vsVaryingName;
 }
 
-void GrGLEffectMatrix::setData(const GrGLUniformManager& uniformManager,
-                               const SkMatrix& matrix,
-                               const GrDrawEffect& drawEffect,
-                               const GrTexture* texture) {
+void GrGLCoordTransform::setData(const GrGLUniformManager& uniformManager,
+                                 const GrDrawEffect& drawEffect,
+                                 int transformIdx) {
     SkASSERT(fUni.isValid() != (kVoid_GrSLType == fUniType));
-    const SkMatrix& coordChangeMatrix = GrEffect::kLocal_CoordsType == fCoord…
(message too large)

[commit-bot: I haz the power, 2013-10-01 12:07:24]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/cdalton@nvidia.com/24853002/76001

[commit-bot: I haz the power, 2013-10-01 14:12:34]

Retried try job too often on Build-Ubuntu12-GCC-x86_64-Release-Trybot for
step(s) BuildBench, BuildEverything, BuildGm, BuildSkiaLib, BuildTests,
BuildTools
http://skiabot-master.pogerlabs.com:10117/buildstatus?builder=Build-Ubuntu12-...

[bsalomon, 2013-10-02 13:07:36]

Committed patchset #4 manually as r11569 (presubmit successful).