From 92aac1e1b3af3b2f7ed8d353cc3c3ac6ef2724a9 Mon Sep 17 00:00:00 2001 From: Brian Osman Date: Wed, 5 Aug 2020 16:48:58 -0400 Subject: [PATCH] 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 Reviewed-by: Mike Klein Commit-Queue: Brian Osman --- include/effects/SkRuntimeEffect.h | 8 ++++--- src/core/SkRuntimeEffect.cpp | 20 ++++++++++++---- src/sksl/SkSLAnalysis.cpp | 39 ++++++++++++++----------------- src/sksl/SkSLAnalysis.h | 3 +++ tests/SkRuntimeEffectTest.cpp | 15 ++++++++++++ 5 files changed, 56 insertions(+), 29 deletions(-) diff --git a/include/effects/SkRuntimeEffect.h b/include/effects/SkRuntimeEffect.h index 84be79f3b0..7a1b281bf5 100644 --- a/include/effects/SkRuntimeEffect.h +++ b/include/effects/SkRuntimeEffect.h @@ -128,7 +128,7 @@ public: // 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 @@ private: std::vector&& sampleUsages, std::vector&& varyings, size_t uniformSize, - bool mainHasSampleCoords); + bool usesSampleCoords, + bool allowColorFilter); using SpecializeResult = std::tuple, SkString>; SpecializeResult specialize(SkSL::Program& baseProgram, const void* inputs, @@ -180,7 +181,8 @@ private: std::vector fVaryings; size_t fUniformSize; - bool fMainFunctionHasSampleCoords; + bool fUsesSampleCoords; + bool fAllowColorFilter; }; /** diff --git a/src/core/SkRuntimeEffect.cpp b/src/core/SkRuntimeEffect.cpp index 7721164c5c..8a0193d08a 100644 --- a/src/core/SkRuntimeEffect.cpp +++ b/src/core/SkRuntimeEffect.cpp @@ -121,7 +121,13 @@ SkRuntimeEffect::EffectResult SkRuntimeEffect::Make(SkString sksl) { 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 inVars; std::vector uniformVars; @@ -262,7 +268,8 @@ SkRuntimeEffect::EffectResult SkRuntimeEffect::Make(SkString sksl) { std::move(sampleUsages), std::move(varyings), uniformSize, - mainHasSampleCoords)); + usesSampleCoords, + allowColorFilter)); return std::make_tuple(std::move(effect), SkString()); } @@ -292,7 +299,8 @@ SkRuntimeEffect::SkRuntimeEffect(SkString sksl, std::vector&& sampleUsages, std::vector&& 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 @@ SkRuntimeEffect::SkRuntimeEffect(SkString sksl, , 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 @@ sk_sp SkRuntimeEffect::makeColorFilter(sk_sp inputs) { 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 a221f11dec..3b20ac996c 100644 --- a/src/sksl/SkSLAnalysis.cpp +++ b/src/sksl/SkSLAnalysis.cpp @@ -129,34 +129,21 @@ protected: 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 @@ SampleUsage Analysis::GetSampleUsage(const Program& program, const Variable& fp) 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 f8b346061d..99d9ea779d 100644 --- a/src/sksl/SkSLAnalysis.h +++ b/src/sksl/SkSLAnalysis.h @@ -25,7 +25,10 @@ struct Variable; 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 84e0f9a6c1..0ebe63f67b 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 @@ DEF_TEST(SkRuntimeEffectInvalid, r) { "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 surface)