commit b6bd0d2094b6d81cd22eba60ea91e311fe536d27
author Brian Osman <brianosman@google.com> Thu Aug 27 10:51:22 2020 -0400
committer Skia Commit-Bot <skia-commit-bot@chromium.org> Thu Aug 27 16:09:37 2020 +0000
tree 04b93b776223d98012bb9516dcdf7d6f18377f4a
parent 318ad4bb19599f93dae9182961f310a54a8a7812

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>

include/effects/SkRuntimeEffect.h

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;

src/core/SkRuntimeEffect.cpp

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

tests/SkRuntimeEffectTest.cpp

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);
+}