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>

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<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 @@ private:
     std::vector<Varying>  fVaryings;
 
     size_t fUniformSize;
-    bool   fMainFunctionHasSampleCoords;
+    bool   fUsesSampleCoords;
+    bool   fAllowColorFilter;
 };
 
 /**

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<const SkSL::Variable*> inVars;
     std::vector<const SkSL::Variable*> 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<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 @@ 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<SkColorFilter> SkRuntimeEffect::makeColorFilter(sk_sp<SkData> inputs) {
     if (!fChildren.empty()) {
         return nullptr;
     }
+    if (!fAllowColorFilter) {
+        return nullptr;
+    }
     if (!inputs) {
         inputs = SkData::MakeEmpty();
     }

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
+// Visitor that searches through the program for references to a particular builtin variable
-// sample coordinates provided by the parent FP or main program.
+class BuiltinVariableVisitor : public ProgramVisitor {
-class SampleCoordsVisitor : public ProgramVisitor {
+public:
-protected:
+    BuiltinVariableVisitor(int builtin) : fBuiltin(builtin) {}
-    // 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;
-    }
 
     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) {
+bool Analysis::ReferencesBuiltin(const Program& program, int builtin) {
-    SampleCoordsVisitor visitor;
+    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
 

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);
 };
 
 /**

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<SkSurface> surface)