Make SkRuntimeShaderBuilder safe for reuse
Previously, if you snapped off a shader and then changed uniforms
(without drawing & flushing), we'd trigger the SkData assert about
calling writeable_data when not-uniquely-owned. Now we lazily copy the
SkData when necessary.
Includes unit test that previously failed.
Bug: skia:10667
Change-Id: If8d9dd8106d41e66560d760cb36ed83371791fc7
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/313678
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Mike Klein <mtklein@google.com>
diff --git a/include/effects/SkRuntimeEffect.h b/include/effects/SkRuntimeEffect.h
index 44ea848..1d173f0 100644
--- a/include/effects/SkRuntimeEffect.h
+++ b/include/effects/SkRuntimeEffect.h
@@ -184,14 +184,15 @@
* SkRuntimeShaderBuilder builder(effect);
* builder.uniform("some_uniform_float") = 3.14f;
* builder.uniform("some_uniform_matrix") = SkM44::Rotate(...);
- * builder.child("some_child_effect") = mySkImage->makeShader(...);
+ * builder.child("some_child_effect") = mySkImage->makeShader(...);
* ...
* sk_sp<SkShader> shader = builder.makeShader(nullptr, false);
*
* Note that SkRuntimeShaderBuilder is built entirely on the public API of SkRuntimeEffect,
* so can be used as-is or serve as inspiration for other interfaces or binding techniques.
*/
-struct SkRuntimeShaderBuilder {
+class SkRuntimeShaderBuilder {
+public:
SkRuntimeShaderBuilder(sk_sp<SkRuntimeEffect>);
~SkRuntimeShaderBuilder();
@@ -209,8 +210,8 @@
} else if (sizeof(val) != fVar->sizeInBytes()) {
SkDEBUGFAIL("Incorrect value size");
} else {
- memcpy(SkTAddOffset<void>(fOwner->fUniforms->writable_data(), fVar->fOffset),
- &val, sizeof(val));
+ memcpy(SkTAddOffset<void>(fOwner->writableUniformData(), fVar->fOffset),
+ &val, sizeof(val));
}
return *this;
}
@@ -221,8 +222,7 @@
} else if (fVar->sizeInBytes() != 9 * sizeof(float)) {
SkDEBUGFAIL("Incorrect value size");
} else {
- float* data = SkTAddOffset<float>(fOwner->fUniforms->writable_data(),
- fVar->fOffset);
+ float* data = SkTAddOffset<float>(fOwner->writableUniformData(), fVar->fOffset);
data[0] = val.get(0); data[1] = val.get(3); data[2] = val.get(6);
data[3] = val.get(1); data[4] = val.get(4); data[5] = val.get(7);
data[6] = val.get(2); data[7] = val.get(5); data[8] = val.get(8);
@@ -241,11 +241,16 @@
int fIndex; // -1 if the child was not found
};
+ const SkRuntimeEffect* effect() const { return fEffect.get(); }
+
BuilderUniform uniform(const char* name) { return { this, fEffect->findUniform(name) }; }
BuilderChild child(const char* name) { return { this, fEffect->findChild(name) }; }
sk_sp<SkShader> makeShader(const SkMatrix* localMatrix, bool isOpaque);
+private:
+ void* writableUniformData();
+
sk_sp<SkRuntimeEffect> fEffect;
sk_sp<SkData> fUniforms;
std::vector<sk_sp<SkShader>> fChildren;
diff --git a/src/core/SkRuntimeEffect.cpp b/src/core/SkRuntimeEffect.cpp
index 0816806..8a99a9f 100644
--- a/src/core/SkRuntimeEffect.cpp
+++ b/src/core/SkRuntimeEffect.cpp
@@ -1050,6 +1050,13 @@
SkRuntimeShaderBuilder::~SkRuntimeShaderBuilder() = default;
+void* SkRuntimeShaderBuilder::writableUniformData() {
+ if (!fUniforms->unique()) {
+ fUniforms = SkData::MakeWithCopy(fUniforms->data(), fUniforms->size());
+ }
+ return fUniforms->writable_data();
+}
+
sk_sp<SkShader> SkRuntimeShaderBuilder::makeShader(const SkMatrix* localMatrix, bool isOpaque) {
return fEffect->makeShader(fUniforms, fChildren.data(), fChildren.size(), localMatrix, isOpaque);
}
diff --git a/tests/SkRuntimeEffectTest.cpp b/tests/SkRuntimeEffectTest.cpp
index 281f4a6..2ddf507 100644
--- a/tests/SkRuntimeEffectTest.cpp
+++ b/tests/SkRuntimeEffectTest.cpp
@@ -161,7 +161,7 @@
"Got : [ %08x %08x %08x %08x ]\n"
"SkSL:\n%s\n",
TL, TR, BL, BR, actual[0], actual[1], actual[2],
- actual[3], fBuilder->fEffect->source().c_str()));
+ actual[3], fBuilder->effect()->source().c_str()));
}
}
@@ -267,3 +267,21 @@
DEF_GPUTEST_FOR_RENDERING_CONTEXTS(SkRuntimeEffectSimple_GPU, r, ctxInfo) {
test_RuntimeEffect_Shaders(r, ctxInfo.directContext());
}
+
+DEF_TEST(SkRuntimeShaderBuilderReuse, r) {
+ const char* kSource = R"(
+ uniform half x;
+ half4 main() { return half4(x); }
+ )";
+
+ sk_sp<SkRuntimeEffect> effect = std::get<0>(SkRuntimeEffect::Make(SkString(kSource)));
+ REPORTER_ASSERT(r, effect);
+
+ // Test passes if this sequence doesn't assert. skbug.com/10667
+ SkRuntimeShaderBuilder b(std::move(effect));
+ b.uniform("x") = 0.0f;
+ auto shader_0 = b.makeShader(nullptr, false);
+
+ b.uniform("x") = 1.0f;
+ auto shader_1 = b.makeShader(nullptr, true);
+}