Disallow runtime effect color filters that depend on position
Some of these checks are currently redundant (we don't allow color
filters to have children right now). But the next CL will re-add that
capability, and the unit tests here will ensure we don't re-break things
by allowing child-sampling to violate the color filter invariant.
Change-Id: I54c10d8b1d1e376c13347296765185d42b9f644a
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/308285
Reviewed-by: Michael Ludwig <michaelludwig@google.com>
Reviewed-by: Mike Klein <mtklein@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
diff --git a/include/effects/SkRuntimeEffect.h b/include/effects/SkRuntimeEffect.h
index 84be79f..7a1b281 100644
--- a/include/effects/SkRuntimeEffect.h
+++ b/include/effects/SkRuntimeEffect.h
@@ -128,7 +128,7 @@
// Returns index of the named child, or -1 if not found
int findChild(const char* name) const;
- bool usesSampleCoords() const { return fMainFunctionHasSampleCoords; }
+ bool usesSampleCoords() const { return fUsesSampleCoords; }
static void RegisterFlattenables();
~SkRuntimeEffect() override;
@@ -141,7 +141,8 @@
std::vector<SkSL::SampleUsage>&& sampleUsages,
std::vector<Varying>&& varyings,
size_t uniformSize,
- bool mainHasSampleCoords);
+ bool usesSampleCoords,
+ bool allowColorFilter);
using SpecializeResult = std::tuple<std::unique_ptr<SkSL::Program>, SkString>;
SpecializeResult specialize(SkSL::Program& baseProgram, const void* inputs,
@@ -180,7 +181,8 @@
std::vector<Varying> fVaryings;
size_t fUniformSize;
- bool fMainFunctionHasSampleCoords;
+ bool fUsesSampleCoords;
+ bool fAllowColorFilter;
};
/**
diff --git a/src/core/SkRuntimeEffect.cpp b/src/core/SkRuntimeEffect.cpp
index 7721164..8a0193d 100644
--- a/src/core/SkRuntimeEffect.cpp
+++ b/src/core/SkRuntimeEffect.cpp
@@ -121,7 +121,13 @@
SkASSERT(!compiler->errorCount());
bool hasMain = false;
- bool mainHasSampleCoords = SkSL::Analysis::ReferencesSampleCoords(*program);
+ const bool usesSampleCoords = SkSL::Analysis::ReferencesSampleCoords(*program);
+ const bool usesFragCoords = SkSL::Analysis::ReferencesFragCoords(*program);
+
+ // Color filters are not allowed to depend on position (local or device) in any way, but they
+ // can sample children with matrices or explicit coords. Because the children are color filters,
+ // we know (by induction) that they don't use those coords, so we keep the overall invariant.
+ const bool allowColorFilter = !usesSampleCoords && !usesFragCoords;
std::vector<const SkSL::Variable*> inVars;
std::vector<const SkSL::Variable*> uniformVars;
@@ -262,7 +268,8 @@
std::move(sampleUsages),
std::move(varyings),
uniformSize,
- mainHasSampleCoords));
+ usesSampleCoords,
+ allowColorFilter));
return std::make_tuple(std::move(effect), SkString());
}
@@ -292,7 +299,8 @@
std::vector<SkSL::SampleUsage>&& sampleUsages,
std::vector<Varying>&& varyings,
size_t uniformSize,
- bool mainHasSampleCoords)
+ bool usesSampleCoords,
+ bool allowColorFilter)
: fHash(SkGoodHash()(sksl))
, fSkSL(std::move(sksl))
, fBaseProgram(std::move(baseProgram))
@@ -301,7 +309,8 @@
, fSampleUsages(std::move(sampleUsages))
, fVaryings(std::move(varyings))
, fUniformSize(uniformSize)
- , fMainFunctionHasSampleCoords(mainHasSampleCoords) {
+ , fUsesSampleCoords(usesSampleCoords)
+ , fAllowColorFilter(allowColorFilter) {
SkASSERT(fBaseProgram);
SkASSERT(SkIsAlign4(fUniformSize));
SkASSERT(fUniformSize <= this->inputSize());
@@ -1048,6 +1057,9 @@
if (!fChildren.empty()) {
return nullptr;
}
+ if (!fAllowColorFilter) {
+ return nullptr;
+ }
if (!inputs) {
inputs = SkData::MakeEmpty();
}
diff --git a/src/sksl/SkSLAnalysis.cpp b/src/sksl/SkSLAnalysis.cpp
index a221f11..3b20ac9 100644
--- a/src/sksl/SkSLAnalysis.cpp
+++ b/src/sksl/SkSLAnalysis.cpp
@@ -129,34 +129,21 @@
typedef ProgramVisitor INHERITED;
};
-// Visitor that searches through a main function of the program for reference to the
-// sample coordinates provided by the parent FP or main program.
-class SampleCoordsVisitor : public ProgramVisitor {
-protected:
- // Only bother recursing through the main function for the sample coord builtin
- bool visitProgramElement(const ProgramElement& pe) override {
- if (pe.fKind == ProgramElement::kFunction_Kind) {
- // Both kFragmentProcessor and kPipelineStage types use the first argument for
- // the main coords builtin. If that isn't in the signature, there's no need to
- // recurse deeper.
- const FunctionDeclaration& func = ((const FunctionDefinition&) pe).fDeclaration;
- if (func.fName == "main" && func.fParameters.size() >= 1 &&
- func.fParameters.front()->fType == *this->program().fContext->fFloat2_Type) {
- return this->INHERITED::visitProgramElement(pe);
- }
- }
- // No recursion, but returning false will allow visitor to continue to siblings
- return false;
- }
+// Visitor that searches through the program for references to a particular builtin variable
+class BuiltinVariableVisitor : public ProgramVisitor {
+public:
+ BuiltinVariableVisitor(int builtin) : fBuiltin(builtin) {}
bool visitExpression(const Expression& e) override {
if (e.fKind == Expression::kVariableReference_Kind) {
const VariableReference& var = (const VariableReference&) e;
- return var.fVariable.fModifiers.fLayout.fBuiltin == SK_MAIN_COORDS_BUILTIN;
+ return var.fVariable.fModifiers.fLayout.fBuiltin == fBuiltin;
}
return this->INHERITED::visitExpression(e);
}
+ int fBuiltin;
+
typedef ProgramVisitor INHERITED;
};
@@ -170,11 +157,19 @@
return visitor.visit(program);
}
-bool Analysis::ReferencesSampleCoords(const Program& program) {
- SampleCoordsVisitor visitor;
+bool Analysis::ReferencesBuiltin(const Program& program, int builtin) {
+ BuiltinVariableVisitor visitor(builtin);
return visitor.visit(program);
}
+bool Analysis::ReferencesSampleCoords(const Program& program) {
+ return Analysis::ReferencesBuiltin(program, SK_MAIN_COORDS_BUILTIN);
+}
+
+bool Analysis::ReferencesFragCoords(const Program& program) {
+ return Analysis::ReferencesBuiltin(program, SK_FRAGCOORD_BUILTIN);
+}
+
////////////////////////////////////////////////////////////////////////////////
// ProgramVisitor
diff --git a/src/sksl/SkSLAnalysis.h b/src/sksl/SkSLAnalysis.h
index f8b3460..99d9ea7 100644
--- a/src/sksl/SkSLAnalysis.h
+++ b/src/sksl/SkSLAnalysis.h
@@ -25,7 +25,10 @@
struct Analysis {
static SampleUsage GetSampleUsage(const Program& program, const Variable& fp);
+ static bool ReferencesBuiltin(const Program& program, int builtin);
+
static bool ReferencesSampleCoords(const Program& program);
+ static bool ReferencesFragCoords(const Program& program);
};
/**
diff --git a/tests/SkRuntimeEffectTest.cpp b/tests/SkRuntimeEffectTest.cpp
index 84e0f9a..0ebe63f 100644
--- a/tests/SkRuntimeEffectTest.cpp
+++ b/tests/SkRuntimeEffectTest.cpp
@@ -7,6 +7,7 @@
#include "include/core/SkBitmap.h"
#include "include/core/SkCanvas.h"
+#include "include/core/SkColorFilter.h"
#include "include/core/SkPaint.h"
#include "include/core/SkSurface.h"
#include "include/effects/SkRuntimeEffect.h"
@@ -80,6 +81,20 @@
"expression");
}
+DEF_TEST(SkRuntimeEffectInvalidColorFilters, r) {
+ auto test = [r](const char* sksl) {
+ auto [effect, errorText] = SkRuntimeEffect::Make(SkString(sksl));
+ REPORTER_ASSERT(r, effect);
+ REPORTER_ASSERT(r, effect->makeShader(nullptr, nullptr, 0, nullptr, false));
+ REPORTER_ASSERT(r, !effect->makeColorFilter(nullptr));
+ };
+
+ // Runtime effects that use sample coords or sk_FragCoord are valid shaders,
+ // but not valid color filters
+ test("void main(float2 p, inout half4 color) { color.rg = half2(p); }");
+ test("void main(float2 p, inout half4 color) { color.rg = half2(sk_FragCoord.xy); }");
+}
+
class TestEffect {
public:
TestEffect(skiatest::Reporter* r, sk_sp<SkSurface> surface)