Disallow sk_FragCoord in SkRuntimeEffect
It's still accessible internally (and by default when using
SkMakeRuntimeEffect), but we don't want clients using this.
If we can come up with a simple and consistent coordinate
model that doesn't expose implementation details, we'll
revisit this.
Change-Id: I77726830480a286541ae887f9cd9822eb10ea913
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/430422
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
diff --git a/include/effects/SkRuntimeEffect.h b/include/effects/SkRuntimeEffect.h
index 6b0fa6f..f3eb3f5 100644
--- a/include/effects/SkRuntimeEffect.h
+++ b/include/effects/SkRuntimeEffect.h
@@ -104,6 +104,11 @@
// still largely ES3-unaware and can still fail or crash if post-ES2 features are used.
// This is only intended for use by tests and certain internally created effects.
bool enforceES2Restrictions = true;
+
+ // Similarly: Public SkSL does not allow access to sk_FragCoord. The semantics of that
+ // variable are confusing, and expose clients to implementation details of saveLayer and
+ // image filters.
+ bool allowFragCoord = false;
};
// If the effect is compiled successfully, `effect` will be non-null.
@@ -118,9 +123,15 @@
// values are flexible. They are listed as being 'vec4', but they can also be 'half4' or
// 'float4'. ('vec4' is an alias for 'float4').
+ // We can't use a default argument for `options` due to a bug in Clang.
+ // https://bugs.llvm.org/show_bug.cgi?id=36684
+
// Color filter SkSL requires an entry point that looks like:
// vec4 main(vec4 inColor) { ... }
static Result MakeForColorFilter(SkString sksl, const Options&);
+ static Result MakeForColorFilter(SkString sksl) {
+ return MakeForColorFilter(std::move(sksl), Options{});
+ }
// Shader SkSL requires an entry point that looks like:
// vec4 main(vec2 inCoords) { ... }
@@ -129,27 +140,25 @@
//
// Most shaders don't use the input color, so that parameter is optional.
static Result MakeForShader(SkString sksl, const Options&);
+ static Result MakeForShader(SkString sksl) {
+ return MakeForShader(std::move(sksl), Options{});
+ }
// Blend SkSL requires an entry point that looks like:
// vec4 main(vec4 srcColor, vec4 dstColor) { ... }
static Result MakeForBlender(SkString sksl, const Options&);
-
- // We can't use a default argument for `options` due to a bug in Clang.
- // https://bugs.llvm.org/show_bug.cgi?id=36684
- static Result MakeForColorFilter(SkString sksl) {
- return MakeForColorFilter(std::move(sksl), Options{});
- }
- static Result MakeForShader(SkString sksl) {
- return MakeForShader(std::move(sksl), Options{});
- }
static Result MakeForBlender(SkString sksl) {
return MakeForBlender(std::move(sksl), Options{});
}
+ // DSL entry points
+ static Result MakeForColorFilter(std::unique_ptr<SkSL::Program> program, const Options&);
static Result MakeForColorFilter(std::unique_ptr<SkSL::Program> program);
+ static Result MakeForShader(std::unique_ptr<SkSL::Program> program, const Options&);
static Result MakeForShader(std::unique_ptr<SkSL::Program> program);
+ static Result MakeForBlender(std::unique_ptr<SkSL::Program> program, const Options&);
static Result MakeForBlender(std::unique_ptr<SkSL::Program> program);
// Object that allows passing either an SkShader or SkColorFilter as a child
@@ -237,13 +246,15 @@
std::vector<SkSL::SampleUsage>&& sampleUsages,
uint32_t flags);
- static Result Make(std::unique_ptr<SkSL::Program> program, SkSL::ProgramKind kind);
+ static Result MakeFromSource(SkString sksl, const Options& options, SkSL::ProgramKind kind);
- static Result Make(SkString sksl, const Options& options, SkSL::ProgramKind kind);
+ static Result MakeFromDSL(std::unique_ptr<SkSL::Program> program,
+ const Options& options,
+ SkSL::ProgramKind kind);
- static Result Make(std::unique_ptr<SkSL::Program> program,
- const Options& options,
- SkSL::ProgramKind kind);
+ static Result MakeInternal(std::unique_ptr<SkSL::Program> program,
+ const Options& options,
+ SkSL::ProgramKind kind);
uint32_t hash() const { return fHash; }
bool usesSampleCoords() const { return (fFlags & kUsesSampleCoords_Flag); }
diff --git a/include/sksl/DSLRuntimeEffects.h b/include/sksl/DSLRuntimeEffects.h
index 194850f..3d9b990 100644
--- a/include/sksl/DSLRuntimeEffects.h
+++ b/include/sksl/DSLRuntimeEffects.h
@@ -8,10 +8,9 @@
#ifndef SKSL_DSL_RUNTIME_EFFECTS
#define SKSL_DSL_RUNTIME_EFFECTS
+#include "include/effects/SkRuntimeEffect.h"
#include "include/sksl/DSL.h"
-class SkRuntimeEffect;
-
namespace SkSL {
class Compiler;
@@ -22,7 +21,7 @@
void StartRuntimeShader(SkSL::Compiler* compiler);
-sk_sp<SkRuntimeEffect> EndRuntimeShader();
+sk_sp<SkRuntimeEffect> EndRuntimeShader(SkRuntimeEffect::Options options = {});
#endif
diff --git a/src/core/SkRuntimeEffect.cpp b/src/core/SkRuntimeEffect.cpp
index 63c3965..f62aea8 100644
--- a/src/core/SkRuntimeEffect.cpp
+++ b/src/core/SkRuntimeEffect.cpp
@@ -165,8 +165,9 @@
// in the IR generator would provide better errors messages (with locations).
#define RETURN_FAILURE(...) return Result{nullptr, SkStringPrintf(__VA_ARGS__)}
-SkRuntimeEffect::Result SkRuntimeEffect::Make(SkString sksl, const Options& options,
- SkSL::ProgramKind kind) {
+SkRuntimeEffect::Result SkRuntimeEffect::MakeFromSource(SkString sksl,
+ const Options& options,
+ SkSL::ProgramKind kind) {
std::unique_ptr<SkSL::Program> program;
{
// We keep this SharedCompiler in a separate scope to make sure it's destroyed before
@@ -184,20 +185,21 @@
RETURN_FAILURE("%s", compiler->errorText().c_str());
}
}
- return Make(std::move(program), options, kind);
+ return MakeInternal(std::move(program), options, kind);
}
-SkRuntimeEffect::Result SkRuntimeEffect::Make(std::unique_ptr<SkSL::Program> program,
- SkSL::ProgramKind kind) {
+SkRuntimeEffect::Result SkRuntimeEffect::MakeFromDSL(std::unique_ptr<SkSL::Program> program,
+ const Options& options,
+ SkSL::ProgramKind kind) {
// This factory is used for all DSL runtime effects, which don't have anything stored in the
// program's source. Populate it so that we can compute fHash, and serialize these effects.
program->fSource = std::make_unique<SkSL::String>(program->description());
- return Make(std::move(program), Options{}, kind);
+ return MakeInternal(std::move(program), options, kind);
}
-SkRuntimeEffect::Result SkRuntimeEffect::Make(std::unique_ptr<SkSL::Program> program,
- const Options& options,
- SkSL::ProgramKind kind) {
+SkRuntimeEffect::Result SkRuntimeEffect::MakeInternal(std::unique_ptr<SkSL::Program> program,
+ const Options& options,
+ SkSL::ProgramKind kind) {
SkSL::SharedCompiler compiler;
// Find 'main', then locate the sample coords parameter. (It might not be present.)
@@ -225,6 +227,12 @@
flags |= kUsesSampleCoords_Flag;
}
+ // TODO(skia:12202): When we can layer modules, implement this restriction by moving the
+ // declaration of sk_FragCoord to a private module.
+ if (!options.allowFragCoord && SkSL::Analysis::ReferencesFragCoords(*program)) {
+ RETURN_FAILURE("unknown identifier 'sk_FragCoord'");
+ }
+
// Color filters and blends are not allowed to depend on position (local or device) in any way.
// The signature of main, and the declarations in sksl_rt_colorfilter/sksl_rt_blend should
// guarantee this.
@@ -322,41 +330,57 @@
}
SkRuntimeEffect::Result SkRuntimeEffect::MakeForColorFilter(SkString sksl, const Options& options) {
- auto result = Make(std::move(sksl), options, SkSL::ProgramKind::kRuntimeColorFilter);
+ auto result = MakeFromSource(std::move(sksl), options, SkSL::ProgramKind::kRuntimeColorFilter);
SkASSERT(!result.effect || result.effect->allowColorFilter());
return result;
}
SkRuntimeEffect::Result SkRuntimeEffect::MakeForShader(SkString sksl, const Options& options) {
- auto result = Make(std::move(sksl), options, SkSL::ProgramKind::kRuntimeShader);
+ auto result = MakeFromSource(std::move(sksl), options, SkSL::ProgramKind::kRuntimeShader);
SkASSERT(!result.effect || result.effect->allowShader());
return result;
}
SkRuntimeEffect::Result SkRuntimeEffect::MakeForBlender(SkString sksl, const Options& options) {
- auto result = Make(std::move(sksl), options, SkSL::ProgramKind::kRuntimeBlender);
+ auto result = MakeFromSource(std::move(sksl), options, SkSL::ProgramKind::kRuntimeBlender);
SkASSERT(!result.effect || result.effect->allowBlender());
return result;
}
-SkRuntimeEffect::Result SkRuntimeEffect::MakeForColorFilter(std::unique_ptr<SkSL::Program> program) {
- auto result = Make(std::move(program), SkSL::ProgramKind::kRuntimeColorFilter);
+SkRuntimeEffect::Result SkRuntimeEffect::MakeForColorFilter(std::unique_ptr<SkSL::Program> program,
+ const Options& options) {
+ auto result = MakeFromDSL(std::move(program), options, SkSL::ProgramKind::kRuntimeColorFilter);
SkASSERT(!result.effect || result.effect->allowColorFilter());
return result;
}
-SkRuntimeEffect::Result SkRuntimeEffect::MakeForShader(std::unique_ptr<SkSL::Program> program) {
- auto result = Make(std::move(program), SkSL::ProgramKind::kRuntimeShader);
+SkRuntimeEffect::Result SkRuntimeEffect::MakeForShader(std::unique_ptr<SkSL::Program> program,
+ const Options& options) {
+ auto result = MakeFromDSL(std::move(program), options, SkSL::ProgramKind::kRuntimeShader);
SkASSERT(!result.effect || result.effect->allowShader());
return result;
}
-SkRuntimeEffect::Result SkRuntimeEffect::MakeForBlender(std::unique_ptr<SkSL::Program> program) {
- auto result = Make(std::move(program), SkSL::ProgramKind::kRuntimeBlender);
+SkRuntimeEffect::Result SkRuntimeEffect::MakeForBlender(std::unique_ptr<SkSL::Program> program,
+ const Options& options) {
+ auto result = MakeFromDSL(std::move(program), options, SkSL::ProgramKind::kRuntimeBlender);
SkASSERT(!result.effect || result.effect->allowBlender());
return result;
}
+SkRuntimeEffect::Result SkRuntimeEffect::MakeForColorFilter(
+ std::unique_ptr<SkSL::Program> program) {
+ return MakeForColorFilter(std::move(program), Options{});
+}
+
+SkRuntimeEffect::Result SkRuntimeEffect::MakeForShader(std::unique_ptr<SkSL::Program> program) {
+ return MakeForShader(std::move(program), Options{});
+}
+
+SkRuntimeEffect::Result SkRuntimeEffect::MakeForBlender(std::unique_ptr<SkSL::Program> program) {
+ return MakeForBlender(std::move(program), Options{});
+}
+
sk_sp<SkRuntimeEffect> SkMakeCachedRuntimeEffect(SkRuntimeEffect::Result (*make)(SkString sksl),
SkString sksl) {
SK_BEGIN_REQUIRE_DENSE
@@ -443,12 +467,14 @@
// be accounted for in `fHash`. If you've added a new field to Options and caused the static-
// assert below to trigger, please incorporate your field into `fHash` and update KnownOptions
// to match the layout of Options.
- struct KnownOptions { bool a, b; };
+ struct KnownOptions { bool forceNoInline, enforceES2Restrictions, allowFragCoord; };
static_assert(sizeof(Options) == sizeof(KnownOptions));
fHash = SkOpts::hash_fn(&options.forceNoInline,
sizeof(options.forceNoInline), fHash);
fHash = SkOpts::hash_fn(&options.enforceES2Restrictions,
sizeof(options.enforceES2Restrictions), fHash);
+ fHash = SkOpts::hash_fn(&options.allowFragCoord,
+ sizeof(options.allowFragCoord), fHash);
fFilterColorProgram = SkFilterColorProgram::Make(this);
}
diff --git a/src/core/SkRuntimeEffectPriv.h b/src/core/SkRuntimeEffectPriv.h
index 3b3cd96..9865c9f 100644
--- a/src/core/SkRuntimeEffectPriv.h
+++ b/src/core/SkRuntimeEffectPriv.h
@@ -16,6 +16,25 @@
#ifdef SK_ENABLE_SKSL
+class SkRuntimeEffectPriv {
+public:
+ // Helper function when creating an effect for a GrSkSLFP that verifies an effect will
+ // implement the constant output for constant input optimization flag.
+ static bool SupportsConstantOutputForConstantInput(sk_sp<SkRuntimeEffect> effect) {
+ return effect->getFilterColorProgram();
+ }
+
+ static SkRuntimeEffect::Options ES3Options() {
+ SkRuntimeEffect::Options options;
+ options.enforceES2Restrictions = false;
+ return options;
+ }
+
+ static void EnableFragCoord(SkRuntimeEffect::Options* options) {
+ options->allowFragCoord = true;
+ }
+};
+
// These internal APIs for creating runtime effects vary from the public API in two ways:
//
// 1) they're used in contexts where it's not useful to receive an error message;
@@ -38,6 +57,7 @@
SkRuntimeEffect::Result (*make)(SkString, const SkRuntimeEffect::Options&),
const char* sksl,
SkRuntimeEffect::Options options = SkRuntimeEffect::Options{}) {
+ SkRuntimeEffectPriv::EnableFragCoord(&options);
auto result = make(SkString{sksl}, options);
SkASSERTF(result.effect, "%s", result.errorText.c_str());
return result.effect;
@@ -123,21 +143,6 @@
bool fAlphaUnchanged;
};
-class SkRuntimeEffectPriv {
-public:
- // Helper function when creating an effect for a GrSkSLFP that verifies an effect will
- // implement the constant output for constant input optimization flag.
- static bool SupportsConstantOutputForConstantInput(sk_sp<SkRuntimeEffect> effect) {
- return effect->getFilterColorProgram();
- }
-
- static SkRuntimeEffect::Options ES3Options() {
- SkRuntimeEffect::Options options;
- options.enforceES2Restrictions = false;
- return options;
- }
-};
-
#endif // SK_ENABLE_SKSL
#endif // SkRuntimeEffectPriv_DEFINED
diff --git a/src/sksl/dsl/DSLRuntimeEffects.cpp b/src/sksl/dsl/DSLRuntimeEffects.cpp
index 0104e48..77df6a6 100644
--- a/src/sksl/dsl/DSLRuntimeEffects.cpp
+++ b/src/sksl/dsl/DSLRuntimeEffects.cpp
@@ -28,11 +28,11 @@
settings.fAllowNarrowingConversions = true;
}
-sk_sp<SkRuntimeEffect> EndRuntimeShader() {
+sk_sp<SkRuntimeEffect> EndRuntimeShader(SkRuntimeEffect::Options options) {
std::unique_ptr<SkSL::Program> program = ReleaseProgram();
SkRuntimeEffect::Result result;
if (program) {
- result = SkRuntimeEffect::MakeForShader(std::move(program));
+ result = SkRuntimeEffect::MakeForShader(std::move(program), options);
// TODO(skbug.com/11862): propagate errors properly
SkASSERTF(result.effect, "%s\n", result.errorText.c_str());
}
diff --git a/tests/SkDSLRuntimeEffectTest.cpp b/tests/SkDSLRuntimeEffectTest.cpp
index 9e61c56..3f283af 100644
--- a/tests/SkDSLRuntimeEffectTest.cpp
+++ b/tests/SkDSLRuntimeEffectTest.cpp
@@ -14,6 +14,7 @@
#include "include/effects/SkRuntimeEffect.h"
#include "include/gpu/GrDirectContext.h"
#include "include/sksl/DSLRuntimeEffects.h"
+#include "src/core/SkRuntimeEffectPriv.h"
#include "src/core/SkTLazy.h"
#include "src/gpu/GrColor.h"
#include "src/sksl/SkSLCompiler.h"
@@ -37,7 +38,9 @@
}
void end(bool expectSuccess = true) {
- sk_sp<SkRuntimeEffect> effect = EndRuntimeShader();
+ SkRuntimeEffect::Options options;
+ SkRuntimeEffectPriv::EnableFragCoord(&options);
+ sk_sp<SkRuntimeEffect> effect = EndRuntimeShader(options);
REPORTER_ASSERT(fReporter, effect ? expectSuccess : !expectSuccess);
if (effect) {
fBuilder.init(std::move(effect));
diff --git a/tests/SkRuntimeEffectTest.cpp b/tests/SkRuntimeEffectTest.cpp
index 6b0d428..79b05e7 100644
--- a/tests/SkRuntimeEffectTest.cpp
+++ b/tests/SkRuntimeEffectTest.cpp
@@ -221,12 +221,14 @@
DEF_TEST(SkRuntimeEffectForShader, r) {
// Tests that the shader factory rejects or accepts certain SkSL constructs
- auto test_valid = [r](const char* sksl) {
- auto [effect, errorText] = SkRuntimeEffect::MakeForShader(SkString(sksl));
+ auto test_valid = [r](const char* sksl, SkRuntimeEffect::Options options = {}) {
+ auto [effect, errorText] = SkRuntimeEffect::MakeForShader(SkString(sksl), options);
REPORTER_ASSERT(r, effect, "%s", errorText.c_str());
};
- auto test_invalid = [r](const char* sksl, const char* expected) {
+ auto test_invalid = [r](const char* sksl,
+ const char* expected,
+ SkRuntimeEffect::Options options = {}) {
auto [effect, errorText] = SkRuntimeEffect::MakeForShader(SkString(sksl));
REPORTER_ASSERT(r, !effect);
REPORTER_ASSERT(r,
@@ -258,8 +260,13 @@
test_invalid("half4 main() { return half4(1); }", "'main' parameter");
test_invalid("half4 main(half4 c) { return c; }", "'main' parameter");
- // sk_FragCoord should be available
- test_valid("half4 main(float2 p) { return sk_FragCoord.xy01; }");
+ // sk_FragCoord should be available, but only if we've enabled it via Options
+ test_invalid("half4 main(float2 p) { return sk_FragCoord.xy01; }",
+ "unknown identifier 'sk_FragCoord'");
+
+ SkRuntimeEffect::Options optionsWithFragCoord;
+ SkRuntimeEffectPriv::EnableFragCoord(&optionsWithFragCoord);
+ test_valid("half4 main(float2 p) { return sk_FragCoord.xy01; }", optionsWithFragCoord);
// Sampling a child shader requires that we pass explicit coords
test_valid("uniform shader child;"
@@ -344,7 +351,9 @@
: fReporter(r), fSurface(std::move(surface)) {}
void build(const char* src) {
- auto [effect, errorText] = SkRuntimeEffect::MakeForShader(SkString(src));
+ SkRuntimeEffect::Options options;
+ SkRuntimeEffectPriv::EnableFragCoord(&options);
+ auto [effect, errorText] = SkRuntimeEffect::MakeForShader(SkString(src), options);
if (!effect) {
REPORT_FAILURE(fReporter, "effect",
SkStringPrintf("Effect didn't compile: %s", errorText.c_str()));
@@ -684,7 +693,9 @@
std::thread threads[16];
for (auto& thread : threads) {
thread = std::thread([r]() {
- auto [effect, error] = SkRuntimeEffect::MakeForShader(SkString(kSource));
+ SkRuntimeEffect::Options options;
+ SkRuntimeEffectPriv::EnableFragCoord(&options);
+ auto [effect, error] = SkRuntimeEffect::MakeForShader(SkString(kSource), options);
REPORTER_ASSERT(r, effect);
});
}
