Start adding unit tests of SkRuntimeEffect
- Change SkRuntimeEffect::Make so *it* can fail, and returns
[Effect, ErrorText].
- Initial tests just test for expected failure conditions.
Next steps are to add tests for effects that should work,
and to validate results on CPU and GPU.
Change-Id: Ibac8c3046104577434034263e9e4a4b177e89129
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/261095
Reviewed-by: Brian Salomon <bsalomon@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
diff --git a/gn/tests.gni b/gn/tests.gni
index 30c33fe..c7acde0 100644
--- a/gn/tests.gni
+++ b/gn/tests.gni
@@ -242,6 +242,7 @@
"$_tests/SkRasterPipelineTest.cpp",
"$_tests/SkRemoteGlyphCacheTest.cpp",
"$_tests/SkResourceCacheTest.cpp",
+ "$_tests/SkRuntimeEffectTest.cpp",
"$_tests/SkSLErrorTest.cpp",
"$_tests/SkSLFPTest.cpp",
"$_tests/SkSLGLSLTest.cpp",
diff --git a/src/core/SkColorFilter.cpp b/src/core/SkColorFilter.cpp
index 4c88a85..0abc01b 100644
--- a/src/core/SkColorFilter.cpp
+++ b/src/core/SkColorFilter.cpp
@@ -440,12 +440,11 @@
SkAutoMutexExclusive ama(fByteCodeMutex);
if (!fByteCode) {
- auto [byteCode, errorCount, errorText] = fEffect->toByteCode();
- if (errorCount) {
+ auto [byteCode, errorText] = fEffect->toByteCode();
+ if (!byteCode) {
SkDebugf("%s\n", errorText.c_str());
return false;
}
- SkASSERT(byteCode);
fByteCode = std::move(byteCode);
}
ctx->byteCode = fByteCode.get();
@@ -491,13 +490,15 @@
SkString sksl;
buffer.readString(&sksl);
sk_sp<SkData> inputs = buffer.readByteArrayAsData();
- return sk_sp<SkFlattenable>(new SkRuntimeColorFilter(SkRuntimeEffect::Make(std::move(sksl)),
- std::move(inputs), nullptr));
+
+ auto effect = std::get<0>(SkRuntimeEffect::Make(std::move(sksl)));
+ return sk_sp<SkFlattenable>(new SkRuntimeColorFilter(std::move(effect), std::move(inputs),
+ nullptr));
}
SkRuntimeColorFilterFactory::SkRuntimeColorFilterFactory(SkString sksl,
SkRuntimeColorFilterFn cpuFunc)
- : fEffect(SkRuntimeEffect::Make(std::move(sksl)))
+ : fEffect(std::get<0>(SkRuntimeEffect::Make(std::move(sksl))))
, fCpuFunc(cpuFunc) {}
SkRuntimeColorFilterFactory::~SkRuntimeColorFilterFactory() = default;
@@ -518,7 +519,7 @@
}
bool SkRuntimeColorFilterFactory::testCompile() const {
- return fEffect && fEffect->isValid();
+ return fEffect != nullptr;
}
#endif // SK_SUPPORT_GPU
diff --git a/src/core/SkRuntimeEffect.cpp b/src/core/SkRuntimeEffect.cpp
index ac84431..0898d77 100644
--- a/src/core/SkRuntimeEffect.cpp
+++ b/src/core/SkRuntimeEffect.cpp
@@ -15,8 +15,19 @@
return nextIndex++;
}
-sk_sp<SkRuntimeEffect> SkRuntimeEffect::Make(SkString sksl) {
- return sk_sp<SkRuntimeEffect>(new SkRuntimeEffect(std::move(sksl)));
+SkRuntimeEffect::EffectResult SkRuntimeEffect::Make(SkString sksl) {
+ auto compiler = std::make_unique<SkSL::Compiler>();
+ auto program = compiler->convertProgram(SkSL::Program::kPipelineStage_Kind,
+ SkSL::String(sksl.c_str(), sksl.size()),
+ SkSL::Program::Settings());
+ if (!program) {
+ return std::make_pair(nullptr, SkString(compiler->errorText().c_str()));
+ }
+ SkASSERT(!compiler->errorCount());
+
+ sk_sp<SkRuntimeEffect> effect(new SkRuntimeEffect(std::move(sksl), std::move(compiler),
+ std::move(program)));
+ return std::make_pair(std::move(effect), SkString());
}
size_t SkRuntimeEffect::Variable::sizeInBytes() const {
@@ -38,17 +49,13 @@
return element_size(fType) * fCount;
}
-SkRuntimeEffect::SkRuntimeEffect(SkString sksl)
+SkRuntimeEffect::SkRuntimeEffect(SkString sksl, std::unique_ptr<SkSL::Compiler> compiler,
+ std::unique_ptr<SkSL::Program> baseProgram)
: fIndex(new_sksl_index())
- , fSkSL(std::move(sksl)) {
- fBaseProgram = fCompiler.convertProgram(SkSL::Program::kPipelineStage_Kind,
- SkSL::String(fSkSL.c_str(), fSkSL.size()),
- SkSL::Program::Settings());
- if (!fBaseProgram) {
- SkDebugf("%s\n", fCompiler.errorText().c_str());
- return;
- }
- SkASSERT(!fCompiler.errorCount());
+ , fSkSL(std::move(sksl))
+ , fCompiler(std::move(compiler))
+ , fBaseProgram(std::move(baseProgram)) {
+ SkASSERT(fCompiler && fBaseProgram);
size_t offset = 0;
auto gather_variables = [this, &offset](SkSL::Modifiers::Flag flag) {
@@ -68,7 +75,7 @@
SkASSERT((var.fModifiers.fLayout.fFlags & SkSL::Layout::kTracked_Flag) == 0);
if (var.fModifiers.fFlags & flag) {
- if (&var.fType == fCompiler.context().fFragmentProcessor_Type.get()) {
+ if (&var.fType == fCompiler->context().fFragmentProcessor_Type.get()) {
fChildren.push_back(var.fName);
continue;
}
@@ -94,7 +101,7 @@
#define SET_TYPES(cpuType, gpuType) do { v.fType = cpuType; } while (false)
#endif
- const SkSL::Context& ctx(fCompiler.context());
+ const SkSL::Context& ctx(fCompiler->context());
if (type == ctx.fBool_Type.get()) {
SET_TYPES(Variable::Type::kBool, kVoid_GrSLType);
} else if (type == ctx.fInt_Type.get()) {
@@ -168,9 +175,9 @@
SkSL::Program::Settings settings;
settings.fCaps = shaderCaps;
- auto baseProgram = fCompiler.convertProgram(SkSL::Program::kPipelineStage_Kind,
- SkSL::String(fSkSL.c_str(), fSkSL.size()),
- settings);
+ auto baseProgram = fCompiler->convertProgram(SkSL::Program::kPipelineStage_Kind,
+ SkSL::String(fSkSL.c_str(), fSkSL.size()),
+ settings);
SkASSERT(baseProgram);
std::unordered_map<SkSL::String, SkSL::Program::Settings::Value> inputMap;
@@ -203,16 +210,16 @@
}
}
- auto specialized = fCompiler.specialize(*baseProgram, inputMap);
- bool optimized = fCompiler.optimize(*specialized);
+ auto specialized = fCompiler->specialize(*baseProgram, inputMap);
+ bool optimized = fCompiler->optimize(*specialized);
if (!optimized) {
- SkDebugf("%s\n", fCompiler.errorText().c_str());
+ SkDebugf("%s\n", fCompiler->errorText().c_str());
SkASSERT(false);
return false;
}
- if (!fCompiler.toPipelineStage(*specialized, outCode, outFormatArgs, outFunctions)) {
- SkDebugf("%s\n", fCompiler.errorText().c_str());
+ if (!fCompiler->toPipelineStage(*specialized, outCode, outFormatArgs, outFunctions)) {
+ SkDebugf("%s\n", fCompiler->errorText().c_str());
SkASSERT(false);
return false;
}
@@ -221,8 +228,7 @@
}
#endif
-std::tuple<std::unique_ptr<SkSL::ByteCode>, int, SkString> SkRuntimeEffect::toByteCode() {
- auto byteCode = fCompiler.toByteCode(*fBaseProgram);
- return std::make_tuple(std::move(byteCode), fCompiler.errorCount(),
- SkString(fCompiler.errorText().c_str()));
+SkRuntimeEffect::ByteCodeResult SkRuntimeEffect::toByteCode() {
+ auto byteCode = fCompiler->toByteCode(*fBaseProgram);
+ return std::make_tuple(std::move(byteCode), SkString(fCompiler->errorText().c_str()));
}
diff --git a/src/core/SkRuntimeEffect.h b/src/core/SkRuntimeEffect.h
index c1bc0ed..cb42d3e 100644
--- a/src/core/SkRuntimeEffect.h
+++ b/src/core/SkRuntimeEffect.h
@@ -59,11 +59,14 @@
size_t sizeInBytes() const;
};
- static sk_sp<SkRuntimeEffect> Make(SkString sksl);
+ // [Effect, ErrorText]
+ // If successful, Effect != nullptr, otherwise, ErrorText contains the reason for failure.
+ using EffectResult = std::tuple<sk_sp<SkRuntimeEffect>, SkString>;
+
+ static EffectResult Make(SkString sksl);
const SkString& source() const { return fSkSL; }
int index() const { return fIndex; }
- bool isValid() const { return fBaseProgram != nullptr; }
size_t inputSize() const;
size_t childCount() const { return fChildren.size(); }
@@ -76,16 +79,20 @@
std::vector<SkSL::Compiler::GLSLFunction>* outFunctions);
#endif
- // Returns ByteCode, ErrorCount, ErrorText
- std::tuple<std::unique_ptr<SkSL::ByteCode>, int, SkString> toByteCode();
+ // [ByteCode, ErrorText]
+ // If successful, ByteCode != nullptr, otherwise, ErrorText contains the reason for failure.
+ using ByteCodeResult = std::tuple<std::unique_ptr<SkSL::ByteCode>, SkString>;
+
+ ByteCodeResult toByteCode();
private:
- SkRuntimeEffect(SkString sksl);
+ SkRuntimeEffect(SkString sksl, std::unique_ptr<SkSL::Compiler> compiler,
+ std::unique_ptr<SkSL::Program> baseProgram);
int fIndex;
SkString fSkSL;
- SkSL::Compiler fCompiler;
+ std::unique_ptr<SkSL::Compiler> fCompiler;
std::unique_ptr<SkSL::Program> fBaseProgram;
std::vector<Variable> fInAndUniformVars;
std::vector<SkString> fChildren;
diff --git a/src/effects/SkOverdrawColorFilter.cpp b/src/effects/SkOverdrawColorFilter.cpp
index cdfc35b..a698df6 100644
--- a/src/effects/SkOverdrawColorFilter.cpp
+++ b/src/effects/SkOverdrawColorFilter.cpp
@@ -90,12 +90,12 @@
std::unique_ptr<GrFragmentProcessor> SkOverdrawColorFilter::asFragmentProcessor(
GrRecordingContext* context, const GrColorInfo&) const {
- static auto overdrawEffect = SkRuntimeEffect::Make(SkString(SKSL_OVERDRAW_SRC));
+ static auto effect = std::get<0>(SkRuntimeEffect::Make(SkString(SKSL_OVERDRAW_SRC)));
SkColor4f floatColors[kNumColors];
for (int i = 0; i < kNumColors; ++i) {
floatColors[i] = SkColor4f::FromBytes_RGBA(fColors[i]);
}
- return GrSkSLFP::Make(context, overdrawEffect, "Overdraw", floatColors, sizeof(floatColors));
+ return GrSkSLFP::Make(context, effect, "Overdraw", floatColors, sizeof(floatColors));
}
#endif
diff --git a/src/effects/imagefilters/SkArithmeticImageFilter.cpp b/src/effects/imagefilters/SkArithmeticImageFilter.cpp
index 04fbb6a..307d5dc 100644
--- a/src/effects/imagefilters/SkArithmeticImageFilter.cpp
+++ b/src/effects/imagefilters/SkArithmeticImageFilter.cpp
@@ -386,16 +386,13 @@
ctx.colorSpace());
paint.addColorFragmentProcessor(std::move(foregroundFP));
- static auto arithmeticEffect = SkRuntimeEffect::Make(SkString(SKSL_ARITHMETIC_SRC));
+ static auto effect = std::get<0>(SkRuntimeEffect::Make(SkString(SKSL_ARITHMETIC_SRC)));
ArithmeticFPInputs inputs;
static_assert(sizeof(inputs.k) == sizeof(fK), "struct size mismatch");
memcpy(inputs.k, fK, sizeof(inputs.k));
inputs.enforcePMColor = fEnforcePMColor;
- std::unique_ptr<GrFragmentProcessor> xferFP = GrSkSLFP::Make(context,
- arithmeticEffect,
- "Arithmetic",
- &inputs,
- sizeof(inputs));
+ std::unique_ptr<GrFragmentProcessor> xferFP = GrSkSLFP::Make(context, effect, "Arithmetic",
+ &inputs, sizeof(inputs));
if (xferFP) {
((GrSkSLFP&) *xferFP).addChild(std::move(bgFP));
paint.addColorFragmentProcessor(std::move(xferFP));
diff --git a/src/gpu/SkGr.cpp b/src/gpu/SkGr.cpp
index 86f811a..5a69649 100644
--- a/src/gpu/SkGr.cpp
+++ b/src/gpu/SkGr.cpp
@@ -469,8 +469,8 @@
grPaint->numColorFragmentProcessors() > 0) {
int32_t ditherRange = dither_range_type_for_config(ct);
if (ditherRange >= 0) {
- static auto ditherEffect = SkRuntimeEffect::Make(SkString(SKSL_DITHER_SRC));
- auto ditherFP = GrSkSLFP::Make(context, ditherEffect, "Dither",
+ static auto effect = std::get<0>(SkRuntimeEffect::Make(SkString(SKSL_DITHER_SRC)));
+ auto ditherFP = GrSkSLFP::Make(context, effect, "Dither",
&ditherRange, sizeof(ditherRange));
if (ditherFP) {
grPaint->addColorFragmentProcessor(std::move(ditherFP));
diff --git a/src/gpu/effects/GrSkSLFP.cpp b/src/gpu/effects/GrSkSLFP.cpp
index 708a5c1..e269fc7 100644
--- a/src/gpu/effects/GrSkSLFP.cpp
+++ b/src/gpu/effects/GrSkSLFP.cpp
@@ -284,21 +284,21 @@
int type = d->fRandom->nextULessThan(3);
switch (type) {
case 0: {
- static auto ditherEffect = SkRuntimeEffect::Make(SkString(SKSL_DITHER_SRC));
+ static auto effect = std::get<0>(SkRuntimeEffect::Make(SkString(SKSL_DITHER_SRC)));
int rangeType = d->fRandom->nextULessThan(3);
- auto result = GrSkSLFP::Make(d->context(), ditherEffect, "Dither",
+ auto result = GrSkSLFP::Make(d->context(), effect, "Dither",
&rangeType, sizeof(rangeType));
return std::unique_ptr<GrFragmentProcessor>(result.release());
}
case 1: {
- static auto arithmeticEffect = SkRuntimeEffect::Make(SkString(SKSL_ARITHMETIC_SRC));
+ static auto effect = std::get<0>(SkRuntimeEffect::Make(SkString(SKSL_ARITHMETIC_SRC)));
ArithmeticFPInputs inputs;
inputs.k[0] = d->fRandom->nextF();
inputs.k[1] = d->fRandom->nextF();
inputs.k[2] = d->fRandom->nextF();
inputs.k[3] = d->fRandom->nextF();
inputs.enforcePMColor = d->fRandom->nextBool();
- auto result = GrSkSLFP::Make(d->context(), arithmeticEffect, "Arithmetic",
+ auto result = GrSkSLFP::Make(d->context(), effect, "Arithmetic",
&inputs, sizeof(inputs));
result->addChild(GrConstColorProcessor::Make(
SK_PMColor4fWHITE,
@@ -306,12 +306,12 @@
return std::unique_ptr<GrFragmentProcessor>(result.release());
}
case 2: {
- static auto overdrawEffect = SkRuntimeEffect::Make(SkString(SKSL_OVERDRAW_SRC));
+ static auto effect = std::get<0>(SkRuntimeEffect::Make(SkString(SKSL_OVERDRAW_SRC)));
SkColor4f inputs[6];
for (int i = 0; i < 6; ++i) {
inputs[i] = SkColor4f::FromBytes_RGBA(d->fRandom->nextU());
}
- auto result = GrSkSLFP::Make(d->context(), overdrawEffect, "Overdraw",
+ auto result = GrSkSLFP::Make(d->context(), effect, "Overdraw",
&inputs, sizeof(inputs));
return std::unique_ptr<GrFragmentProcessor>(result.release());
}
diff --git a/src/shaders/SkRTShader.cpp b/src/shaders/SkRTShader.cpp
index bf2a104..45cac5c 100644
--- a/src/shaders/SkRTShader.cpp
+++ b/src/shaders/SkRTShader.cpp
@@ -50,12 +50,11 @@
SkAutoMutexExclusive ama(fByteCodeMutex);
if (!fByteCode) {
- auto [byteCode, errorCount, errorText] = fEffect->toByteCode();
- if (errorCount) {
+ auto [byteCode, errorText] = fEffect->toByteCode();
+ if (!byteCode) {
SkDebugf("%s\n", errorText.c_str());
return false;
}
- SkASSERT(byteCode);
fByteCode = std::move(byteCode);
}
ctx->byteCode = fByteCode.get();
@@ -119,8 +118,8 @@
// We don't have a way to ensure that indices are consistent and correct when deserializing.
// Perhaps we should have a hash table to map strings to indices? For now, all shaders get a
// new unique ID after serialization.
- return sk_sp<SkFlattenable>(new SkRTShader(SkRuntimeEffect::Make(std::move(sksl)),
- std::move(inputs), localMPtr,
+ auto effect = std::get<0>(SkRuntimeEffect::Make(std::move(sksl)));
+ return sk_sp<SkFlattenable>(new SkRTShader(std::move(effect), std::move(inputs), localMPtr,
children.data(), children.size(), isOpaque));
}
@@ -145,7 +144,7 @@
#endif
SkRuntimeShaderFactory::SkRuntimeShaderFactory(SkString sksl, bool isOpaque)
- : fEffect(SkRuntimeEffect::Make(std::move(sksl)))
+ : fEffect(std::get<0>(SkRuntimeEffect::Make(std::move(sksl))))
, fIsOpaque(isOpaque) {}
SkRuntimeShaderFactory::SkRuntimeShaderFactory(const SkRuntimeShaderFactory&) = default;
@@ -159,7 +158,6 @@
sk_sp<SkShader> SkRuntimeShaderFactory::make(sk_sp<SkData> inputs, const SkMatrix* localMatrix,
sk_sp<SkShader>* children, size_t childCount) {
return fEffect
- && fEffect->isValid()
&& inputs->size() >= fEffect->inputSize()
&& childCount >= fEffect->childCount()
? sk_sp<SkShader>(new SkRTShader(fEffect, std::move(inputs), localMatrix,
diff --git a/src/sksl/SkSLIRGenerator.cpp b/src/sksl/SkSLIRGenerator.cpp
index c416db6..fa7c83c 100644
--- a/src/sksl/SkSLIRGenerator.cpp
+++ b/src/sksl/SkSLIRGenerator.cpp
@@ -271,6 +271,11 @@
baseType->kind() == Type::Kind::kMatrix_Kind) {
fErrors.error(decls.fOffset, "'in' variables may not have matrix type");
}
+ if ((modifiers.fFlags & Modifiers::kIn_Flag) &&
+ (modifiers.fFlags & Modifiers::kUniform_Flag)) {
+ fErrors.error(decls.fOffset,
+ "'in uniform' variables only permitted within fragment processors");
+ }
if (modifiers.fLayout.fWhen.fLength) {
fErrors.error(decls.fOffset, "'when' is only permitted within fragment processors");
}
diff --git a/tests/SkRuntimeEffectTest.cpp b/tests/SkRuntimeEffectTest.cpp
new file mode 100644
index 0000000..c6a521c
--- /dev/null
+++ b/tests/SkRuntimeEffectTest.cpp
@@ -0,0 +1,41 @@
+/*
+ * Copyright 2019 Google LLC
+ *
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE file.
+ */
+
+#include "src/core/SkRuntimeEffect.h"
+#include "tests/Test.h"
+
+DEF_TEST(SkRuntimeEffectInvalidPrograms, r) {
+ auto test = [r](const char* src, const char* expected) {
+ auto [effect, errorText] = SkRuntimeEffect::Make(SkString(src));
+ REPORTER_ASSERT(r, !effect);
+ REPORTER_ASSERT(r, errorText.contains(expected),
+ "Expected error message to contain \"%s\". Actual message: \"%s\"",
+ expected, errorText.c_str());
+ };
+
+ // Features that are only allowed in .fp files (key, in uniform, ctype, when, tracked).
+ // Ensure that these fail, and the error messages contain the relevant keyword.
+ test("layout(key) in bool Input;"
+ "void main(float x, float y, inout half4 color) {}",
+ "key");
+
+ test("in uniform float Input;"
+ "void main(float x, float y, inout half4 color) {}",
+ "in uniform");
+
+ test("layout(ctype=SkRect) float4 Input;"
+ "void main(float x, float y, inout half4 color) {}",
+ "ctype");
+
+ test("in bool Flag; layout(when=Flag) uniform float Input;"
+ "void main(float x, float y, inout half4 color) {}",
+ "when");
+
+ test("layout(tracked) uniform float Input;"
+ "void main(float x, float y, inout half4 color) {}",
+ "tracked");
+}
diff --git a/tests/SkSLGLSLTest.cpp b/tests/SkSLGLSLTest.cpp
index cea6949..28a8340 100644
--- a/tests/SkSLGLSLTest.cpp
+++ b/tests/SkSLGLSLTest.cpp
@@ -1245,11 +1245,11 @@
SkSL::Program::kVertex_Kind);
test(r,
- "in uniform float4 sk_RTAdjust; in float4 pos; void main() { sk_Position = pos; }",
+ "uniform float4 sk_RTAdjust; in float4 pos; void main() { sk_Position = pos; }",
*SkSL::ShaderCapsFactory::CannotUseFragCoord(),
"#version 400\n"
"out vec4 sk_FragCoord_Workaround;\n"
- "in uniform vec4 sk_RTAdjust;\n"
+ "uniform vec4 sk_RTAdjust;\n"
"in vec4 pos;\n"
"void main() {\n"
" sk_FragCoord_Workaround = (gl_Position = pos);\n"
diff --git a/tools/viewer/SkSLSlide.cpp b/tools/viewer/SkSLSlide.cpp
index cdcfb0d..7379131 100644
--- a/tools/viewer/SkSLSlide.cpp
+++ b/tools/viewer/SkSLSlide.cpp
@@ -73,8 +73,8 @@
}
bool SkSLSlide::rebuild() {
- auto effect = SkRuntimeEffect::Make(fSkSL);
- if (!effect || !effect->isValid()) {
+ auto [effect, errorText] = SkRuntimeEffect::Make(fSkSL);
+ if (!effect) {
return false;
}