Remove workarounds to support legacy coord transforms
Bug: skia:10416
Change-Id: I2f1b87521174d18afc59f12832441010cb94ea3f
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/299294
Reviewed-by: Brian Salomon <bsalomon@google.com>
Commit-Queue: Michael Ludwig <michaelludwig@google.com>
diff --git a/src/gpu/GrPrimitiveProcessor.cpp b/src/gpu/GrPrimitiveProcessor.cpp
index 2be07b1..ca384f0 100644
--- a/src/gpu/GrPrimitiveProcessor.cpp
+++ b/src/gpu/GrPrimitiveProcessor.cpp
@@ -15,23 +15,17 @@
* the transform code is applied.
*/
enum SampleFlag {
- kExplicitlySampled_Flag = 0b0000001, // GrFP::isSampledWithExplicitCoords()
+ kExplicitlySampled_Flag = 0b00001, // GrFP::isSampledWithExplicitCoords()
- kLegacyCoordTransform_Flag = 0b0000010, // !GrFP::coordTransform(i)::isNoOp()
+ kLegacyCoordTransform_Flag = 0b00010, // !GrFP::coordTransform(i)::isNoOp()
- kNone_SampleMatrix_Flag = 0b0000100, // GrFP::sampleMatrix()::isNoOp()
- kConstUniform_SampleMatrix_Flag = 0b0001000, // GrFP::sampleMatrix()::isConstUniform()
- kVariable_SampleMatrix_Flag = 0b0001100, // GrFP::sampleMatrix()::isVariable()
-
- // Legacy coord transforms specialize on identity, S+T, no-perspective, and general matrix types
- // FIXME these (and kLegacyCoordTransform) can be removed once all FPs no longer use them
- kLCT_ScaleTranslate_Matrix_Flag = 0b0010000, // GrFP::coordTransform(i)::isScaleTranslate()
- kLCT_NoPersp_Matrix_Flag = 0b0100000, // !GrFP::coordTransform(i)::hasPerspective()
- kLCT_General_Matrix_Flag = 0b0110000, // any other matrix type
+ kNone_SampleMatrix_Flag = 0b00100, // GrFP::sampleMatrix()::isNoOp()
+ kConstUniform_SampleMatrix_Flag = 0b01000, // GrFP::sampleMatrix()::isConstUniform()
+ kVariable_SampleMatrix_Flag = 0b01100, // GrFP::sampleMatrix()::isVariable()
// Currently, sample(matrix) only specializes on no-perspective or general.
// FIXME add new flags as more matrix types are supported.
- kPersp_Matrix_Flag = 0b1000000, // GrFP::sampleMatrix()::fHasPerspective
+ kPersp_Matrix_Flag = 0b10000, // GrFP::sampleMatrix()::fHasPerspective
};
GrPrimitiveProcessor::GrPrimitiveProcessor(ClassID classID) : GrProcessor(classID) {}
@@ -42,31 +36,16 @@
}
uint32_t GrPrimitiveProcessor::computeCoordTransformsKey(const GrFragmentProcessor& fp) const {
- // This is highly coupled with the code in GrGLSLGeometryProcessor::emitTransforms().
- // At this point, all effects either don't use legacy coord transforms, or only use 1.
- SkASSERT(fp.numCoordTransforms() <= 1);
+ // This is highly coupled with the code in GrGLSLGeometryProcessor::collectTransforms().
+ // At this point, all effects do not use really coord transforms; they may implicitly report
+ // a noop coord transform but this does not impact the key.
+ SkASSERT(fp.numCoordTransforms() == 0 ||
+ (fp.numCoordTransforms() == 1 && fp.coordTransform(0).isNoOp()));
uint32_t key = 0;
if (fp.isSampledWithExplicitCoords()) {
key |= kExplicitlySampled_Flag;
}
- if (fp.numCoordTransforms() > 0) {
- const GrCoordTransform& coordTransform = fp.coordTransform(0);
- if (!coordTransform.isNoOp()) {
- // A true identity matrix shouldn't result in a coord transform; proxy normalization
- // and flipping will eventually present as a scale+translate matrix.
- SkASSERT(!coordTransform.matrix().isIdentity() || coordTransform.normalize() ||
- coordTransform.reverseY());
- key |= kLegacyCoordTransform_Flag;
- if (coordTransform.matrix().isScaleTranslate()) {
- key |= kLCT_ScaleTranslate_Matrix_Flag;
- } else if (!coordTransform.matrix().hasPerspective()) {
- key |= kLCT_NoPersp_Matrix_Flag;
- } else {
- key |= kLCT_General_Matrix_Flag;
- }
- }
- }
switch(fp.sampleMatrix().fKind) {
case SkSL::SampleMatrix::Kind::kNone:
diff --git a/src/gpu/GrPrimitiveProcessor.h b/src/gpu/GrPrimitiveProcessor.h
index c774183..09face1 100644
--- a/src/gpu/GrPrimitiveProcessor.h
+++ b/src/gpu/GrPrimitiveProcessor.h
@@ -14,8 +14,6 @@
#include "src/gpu/GrShaderVar.h"
#include "src/gpu/GrSwizzle.h"
-class GrCoordTransform;
-
/*
* The GrPrimitiveProcessor represents some kind of geometric primitive. This includes the shape
* of the primitive and the inherent color of the primitive. The GrPrimitiveProcessor is
diff --git a/src/gpu/gl/builders/GrGLProgramBuilder.cpp b/src/gpu/gl/builders/GrGLProgramBuilder.cpp
index 230298f..5973b89 100644
--- a/src/gpu/gl/builders/GrGLProgramBuilder.cpp
+++ b/src/gpu/gl/builders/GrGLProgramBuilder.cpp
@@ -15,7 +15,6 @@
#include "src/core/SkWriteBuffer.h"
#include "src/gpu/GrAutoLocaleSetter.h"
#include "src/gpu/GrContextPriv.h"
-#include "src/gpu/GrCoordTransform.h"
#include "src/gpu/GrPersistentCacheUtils.h"
#include "src/gpu/GrProgramDesc.h"
#include "src/gpu/GrShaderCaps.h"
diff --git a/src/gpu/glsl/GrGLSLFragmentProcessor.cpp b/src/gpu/glsl/GrGLSLFragmentProcessor.cpp
index eb29003..fb70e45 100644
--- a/src/gpu/glsl/GrGLSLFragmentProcessor.cpp
+++ b/src/gpu/glsl/GrGLSLFragmentProcessor.cpp
@@ -5,7 +5,6 @@
* found in the LICENSE file.
*/
-#include "src/gpu/GrCoordTransform.h"
#include "src/gpu/GrFragmentProcessor.h"
#include "src/gpu/GrProcessor.h"
#include "src/gpu/glsl/GrGLSLFragmentProcessor.h"
diff --git a/src/gpu/glsl/GrGLSLFragmentShaderBuilder.cpp b/src/gpu/glsl/GrGLSLFragmentShaderBuilder.cpp
index 6ae76f9..48c9053 100644
--- a/src/gpu/glsl/GrGLSLFragmentShaderBuilder.cpp
+++ b/src/gpu/glsl/GrGLSLFragmentShaderBuilder.cpp
@@ -153,45 +153,13 @@
// transforms), the value that would have been passed to _coords is lifted to the vertex shader
// and stored in a unique varying. In that case it uses that variable and does not have a
// second actual argument for _coords.
- // FIXME: Once GrCoordTransforms are gone, and we can more easily associated this varying with
- // the sample call site, then invokeChild() can pass the varying in, instead of requiring this
- // dynamic signature.
- int paramCount;
+ // FIXME: An alternative would be to have all FP functions have a float2 argument, and the
+ // parent FP invokes it with the varying reference when it's been lifted to the vertex shader.
+ int paramCount = 2;
GrShaderVar params[] = { GrShaderVar(args.fInputColor, kHalf4_GrSLType),
GrShaderVar(args.fSampleCoord, kFloat2_GrSLType) };
- if (args.fFp.isSampledWithExplicitCoords()) {
- // All invokeChild() that point to 'fp' will evaluate these expressions and pass the float2
- // in, so we need the 2nd argument.
- paramCount = 2;
-
- // FIXME: This is only needed for the short term until FPs no longer put transformation
- // data in a GrCoordTransform (and we can then mark the parameter as read-only)
- if (args.fTransformedCoords.count() > 0) {
- SkASSERT(args.fTransformedCoords.count() == 1);
-
- const GrShaderVar& transform = args.fTransformedCoords[0].fTransform;
- switch (transform.getType()) {
- case kFloat4_GrSLType:
- // This is a scale+translate, so there's no perspective division needed
- this->codeAppendf("%s = %s * %s.xz + %s.yw;\n", args.fSampleCoord,
- args.fSampleCoord,
- transform.c_str(),
- transform.c_str());
- break;
- case kFloat3x3_GrSLType:
- this->codeAppend("{\n");
- this->codeAppendf("float3 _coords3 = (%s * %s.xy1);\n",
- transform.c_str(), args.fSampleCoord);
- this->codeAppendf("%s = _coords3.xy / _coords3.z;\n", args.fSampleCoord);
- this->codeAppend("}\n");
- break;
- default:
- SkASSERT(transform.getType() == kVoid_GrSLType);
- break;
- }
- }
- } else {
+ if (!args.fFp.isSampledWithExplicitCoords()) {
// Sampled with a const/uniform matrix and/or a legacy coord transform. The actual
// transformation code is emitted in the vertex shader, so this only has to access it.
// Add a float2 _coords variable that maps to the associated varying and replaces the
@@ -210,7 +178,7 @@
// and since we won't actually have a function parameter for local coords, add
// it as a local variable.
this->codeAppendf("float2 %s = %s.xy / %s.z;\n", args.fSampleCoord,
- varying.getName().c_str(), varying.getName().c_str());
+ varying.getName().c_str(), varying.getName().c_str());
break;
default:
SkDEBUGFAILF("Unexpected varying type for coord: %s %d\n",
@@ -218,7 +186,7 @@
break;
}
}
- }
+ } // else the function keeps its two arguments
this->codeAppendf("half4 %s;\n", args.fOutputColor);
fp->emitCode(args);
diff --git a/src/gpu/glsl/GrGLSLGeometryProcessor.cpp b/src/gpu/glsl/GrGLSLGeometryProcessor.cpp
index 1444c0c..2433a70 100644
--- a/src/gpu/glsl/GrGLSLGeometryProcessor.cpp
+++ b/src/gpu/glsl/GrGLSLGeometryProcessor.cpp
@@ -20,12 +20,10 @@
GrGPArgs gpArgs;
this->onEmitCode(args, &gpArgs);
- // FIXME This must always be called at the moment, even when fLocalCoordVar is uninitialized
- // and void because collectTransforms registers the uniforms for legacy coord transforms, which
- // still need to be added even if the FPs are sampled explicitly. When they are gone, we only
- // need to call this if the local coord isn't void (plus verify that FPs really don't need it).
- this->collectTransforms(args.fVertBuilder, args.fVaryingHandler, args.fUniformHandler,
- gpArgs.fLocalCoordVar, args.fFPCoordTransformHandler);
+ if (gpArgs.fLocalCoordVar.getType() != kVoid_GrSLType) {
+ this->collectTransforms(args.fVertBuilder, args.fVaryingHandler, args.fUniformHandler,
+ gpArgs.fLocalCoordVar, args.fFPCoordTransformHandler);
+ }
if (args.fGP.willUseTessellationShaders()) {
// Tessellation shaders are temporarily responsible for integrating their own code strings
@@ -75,8 +73,7 @@
const GrShaderVar& localCoordsVar,
FPCoordTransformHandler* handler) {
SkASSERT(localCoordsVar.getType() == kFloat2_GrSLType ||
- localCoordsVar.getType() == kFloat3_GrSLType ||
- localCoordsVar.getType() == kVoid_GrSLType /* until coord transforms are gone */);
+ localCoordsVar.getType() == kFloat3_GrSLType);
// Cached varyings produced by parent FPs. If parent FPs introduce transformations, but all
// subsequent children are not transformed, they should share the same varying.
std::unordered_map<const GrFragmentProcessor*, GrShaderVar> localCoordsMap;
@@ -98,43 +95,19 @@
for (int i = 0; *handler; ++*handler, ++i) {
auto [coordTransform, fp] = handler->get();
+ // FIXME: GrCoordTransform is used solely as a vehicle for iterating over all FPs that
+ // require sample coordinates directly. We should make this iteration lighter weight.
+ SkASSERT(coordTransform.isNoOp());
- // FPs that use the legacy coord transform system will need a uniform registered for them
- // to hold the coord transform's matrix.
- GrShaderVar transformVar;
// FPs that use local coordinates need a varying to convey the coordinate. This may be the
// base GP's local coord if transforms have to be computed in the FS, or it may be a unique
// varying that computes the equivalent transformation hierarchy in the VS.
GrShaderVar varyingVar;
- // If this is true, the FP's signature takes a float2 local coordinate. Otherwise, it
- // doesn't use local coordinates, or it can be lifted to a varying and referenced directly.
- bool localCoordComputedInFS = fp.isSampledWithExplicitCoords();
- if (!coordTransform.isNoOp()) {
- // Legacy coord transform that actually is doing something. This matrix is the last
- // transformation to affect the local coordinate.
- SkString strUniName;
- strUniName.printf("CoordTransformMatrix_%d", i);
- auto flag = localCoordComputedInFS ? kFragment_GrShaderFlag
- : kVertex_GrShaderFlag;
- auto& uni = fInstalledTransforms.push_back();
- if (fp.isSampledWithExplicitCoords() && coordTransform.matrix().isScaleTranslate()) {
- uni.fType = kFloat4_GrSLType;
- } else {
- uni.fType = kFloat3x3_GrSLType;
- }
- uni.fHandle =
- uniformHandler->addUniform(&fp, flag, uni.fType, strUniName.c_str());
- transformVar = uniformHandler->getUniformVariable(uni.fHandle);
- } else {
- // Must stay parallel with calls to handler
- fInstalledTransforms.push_back();
- }
-
// If the FP references local coords, we need to make sure the vertex shader sets up the
// right transforms or pass-through variables for the FP to evaluate in the fragment shader
if (fp.referencesSampleCoords()) {
- if (localCoordComputedInFS) {
+ if (fp.isSampledWithExplicitCoords()) {
// If the FP local coords are evaluated in the fragment shader, we only need to
// produce the original local coordinate to pass into the root; any other situation,
// the FP will have a 2nd parameter to its function and the caller sends the coords
@@ -168,19 +141,11 @@
node = node->parent();
}
- // Legacy coord transform workaround (if the transform hierarchy appears identity
- // but we have GrCoordTransform that does something, we still need to record a
- // varying for it).
- if (!coordOwner && !coordTransform.isNoOp()) {
- coordOwner = &fp;
- }
-
if (coordOwner) {
// The FP will use coordOwner's varying; add varying if this was the first use
if (transformedLocalCoord.getType() == kVoid_GrSLType) {
GrGLSLVarying v(kFloat2_GrSLType);
- if (coordTransform.matrix().hasPerspective() ||
- GrSLTypeVecLength(localCoordsVar.getType()) == 3 ||
+ if (GrSLTypeVecLength(localCoordsVar.getType()) == 3 ||
coordOwner->hasPerspectiveTransform()) {
v = GrGLSLVarying(kFloat3_GrSLType);
}
@@ -189,17 +154,11 @@
varyingHandler->addVarying(strVaryingName.c_str(), &v);
fTransformInfos.push_back({GrShaderVar(v.vsOut(), v.type()),
- transformVar.getName(),
localCoordsVar,
coordOwner});
transformedLocalCoord = GrShaderVar(SkString(v.fsIn()), v.type(),
GrShaderVar::TypeModifier::In);
- if (coordOwner->numCoordTransforms() < 1 ||
- coordOwner->coordTransform(0).isNoOp()) {
- // As long as a legacy coord transform doesn't get in the way, we can
- // reuse this expression for children (see comment in emitTransformCode)
- localCoordsMap[coordOwner] = transformedLocalCoord;
- }
+ localCoordsMap[coordOwner] = transformedLocalCoord;
}
varyingVar = transformedLocalCoord;
@@ -210,11 +169,13 @@
}
}
- if (varyingVar.getType() != kVoid_GrSLType || transformVar.getType() != kVoid_GrSLType) {
- handler->specifyCoordsForCurrCoordTransform(transformVar, varyingVar);
+ if (varyingVar.getType() != kVoid_GrSLType) {
+ handler->specifyCoordsForCurrCoordTransform(GrShaderVar(), varyingVar);
} else {
handler->omitCoordsForCurrCoordTransform();
}
+ // Must stay parallel with calls to handler
+ fInstalledTransforms.push_back();
}
}
@@ -222,63 +183,53 @@
GrGLSLUniformHandler* uniformHandler) {
std::unordered_map<const GrFragmentProcessor*, GrShaderVar> localCoordsMap;
for (const auto& tr : fTransformInfos) {
- // If we recorded a transform info, its sample matrix must be const/uniform, or we have a
- // legacy coord transform that actually does something.
- SkASSERT(tr.fFP->sampleMatrix().isConstUniform() ||
- (tr.fFP->sampleMatrix().isNoOp() && !tr.fMatrix.isEmpty()));
+ // If we recorded a transform info, its sample matrix must be const/uniform
+ SkASSERT(tr.fFP->sampleMatrix().isConstUniform());
SkString localCoords;
// Build a concatenated matrix expression that we apply to the root local coord.
// If we have an expression cached from an early FP in the hierarchy chain, we can stop
// there instead of going all the way to the GP.
SkString transformExpression;
- if (!tr.fMatrix.isEmpty()) {
- // We have both a const/uniform sample matrix and a legacy coord transform
- transformExpression.printf("%s", tr.fMatrix.c_str());
- }
- // If the sample matrix is kNone, then the current transform expression of just the
- // coord transform matrix is sufficient.
- if (tr.fFP->sampleMatrix().isConstUniform()) {
- const auto* base = tr.fFP;
- while(base) {
- GrShaderVar cachedBaseCoord = localCoordsMap[base];
- if (cachedBaseCoord.getType() != kVoid_GrSLType) {
- // Can stop here, as this varying already holds all transforms from higher FPs
- if (cachedBaseCoord.getType() == kFloat3_GrSLType) {
- localCoords = cachedBaseCoord.getName();
- } else {
- localCoords = SkStringPrintf("%s.xy1", cachedBaseCoord.getName().c_str());
- }
- break;
- } else if (base->sampleMatrix().isConstUniform()) {
- // The FP knows the matrix expression it's sampled with, but its parent defined
- // the uniform (when the expression is not a constant).
- GrShaderVar uniform = uniformHandler->liftUniformToVertexShader(
- *base->parent(), SkString(base->sampleMatrix().fExpression));
-
- // Accumulate the base matrix expression as a preConcat
- SkString matrix;
- if (uniform.getType() != kVoid_GrSLType) {
- SkASSERT(uniform.getType() == kFloat3x3_GrSLType);
- matrix = uniform.getName();
- } else {
- // No uniform found, so presumably this is a constant
- matrix = SkString(base->sampleMatrix().fExpression);
- }
-
- if (!transformExpression.isEmpty()) {
- transformExpression.append(" * ");
- }
- transformExpression.appendf("(%s)", matrix.c_str());
+ const auto* base = tr.fFP;
+ while(base) {
+ GrShaderVar cachedBaseCoord = localCoordsMap[base];
+ if (cachedBaseCoord.getType() != kVoid_GrSLType) {
+ // Can stop here, as this varying already holds all transforms from higher FPs
+ if (cachedBaseCoord.getType() == kFloat3_GrSLType) {
+ localCoords = cachedBaseCoord.getName();
} else {
- // This intermediate FP is just a pass through and doesn't need to be built
- // in to the expression, but must visit its parents in case they add transforms
- SkASSERT(base->sampleMatrix().isNoOp());
+ localCoords = SkStringPrintf("%s.xy1", cachedBaseCoord.getName().c_str());
+ }
+ break;
+ } else if (base->sampleMatrix().isConstUniform()) {
+ // The FP knows the matrix expression it's sampled with, but its parent defined
+ // the uniform (when the expression is not a constant).
+ GrShaderVar uniform = uniformHandler->liftUniformToVertexShader(
+ *base->parent(), SkString(base->sampleMatrix().fExpression));
+
+ // Accumulate the base matrix expression as a preConcat
+ SkString matrix;
+ if (uniform.getType() != kVoid_GrSLType) {
+ SkASSERT(uniform.getType() == kFloat3x3_GrSLType);
+ matrix = uniform.getName();
+ } else {
+ // No uniform found, so presumably this is a constant
+ matrix = SkString(base->sampleMatrix().fExpression);
}
- base = base->parent();
+ if (!transformExpression.isEmpty()) {
+ transformExpression.append(" * ");
+ }
+ transformExpression.appendf("(%s)", matrix.c_str());
+ } else {
+ // This intermediate FP is just a pass through and doesn't need to be built
+ // in to the expression, but must visit its parents in case they add transforms
+ SkASSERT(base->sampleMatrix().isNoOp());
}
+
+ base = base->parent();
}
if (localCoords.isEmpty()) {
@@ -304,18 +255,7 @@
vb->codeAppend(";\n");
vb->codeAppend("}\n");
- if (tr.fMatrix.isEmpty()) {
- // Subtle work around: only cache the intermediate varying when there's no extra
- // coord transform. If the FP uses a coord transform for a legacy effect, but also
- // delegates to a child FP, we want the coordinates pre-GrCoordTransform to be sent
- // to the child FP, but have the FP use the post-coordtransform legacy values
- // (e.g. sampling a texture and relying on the GrCoordTransform for normalization
- // and mixing with a child FP that should not be normalized).
- // FIXME: It's not really possible to apply this logic cleanly when transforms
- // have been moved to the FS; in practice this doesn't seem to occur in our tests and
- // the issue will go away once legacy coord transforms only have no-op matrices.
- localCoordsMap.insert({ tr.fFP, tr.fOutputCoords });
- }
+ localCoordsMap.insert({ tr.fFP, tr.fOutputCoords });
}
}
diff --git a/src/gpu/glsl/GrGLSLGeometryProcessor.h b/src/gpu/glsl/GrGLSLGeometryProcessor.h
index a0a35df..f9c2d6d 100644
--- a/src/gpu/glsl/GrGLSLGeometryProcessor.h
+++ b/src/gpu/glsl/GrGLSLGeometryProcessor.h
@@ -124,8 +124,6 @@
struct TransformInfo {
// The vertex-shader output variable to assign the transformed coordinates to
GrShaderVar fOutputCoords;
- // The name of a coord transform uniform to apply
- SkString fMatrix;
// The coordinate to be transformed
GrShaderVar fLocalCoords;
// The leaf FP of a transform hierarchy to be evaluated in the vertex shader;
