[Impeller] Fix hot reload for shaders on Impeller (#49739)

I broke this in #49543

While investigating flutter/flutter#141235 I
realized that the specific things that were wrong there were probably
artifacts of some intermediate issue in the previous patch - they no
longer appear. However, I had broken hot reload.

This patch fixes it and updates the `EntityTest.RuntimeEffect` to
artificially run the rendering callback a few times simulating a hot
reload. The test fails without my changes here.

ci/licenses_golden/excluded_files

@@ -143,6 +143,7 @@
 ../../../flutter/impeller/display_list/skia_conversions_unittests.cc
 ../../../flutter/impeller/docs
 ../../../flutter/impeller/entity/contents/checkerboard_contents_unittests.cc
+../../../flutter/impeller/entity/contents/content_context_unittests.cc
 ../../../flutter/impeller/entity/contents/filters/directional_gaussian_blur_filter_contents_unittests.cc
 ../../../flutter/impeller/entity/contents/filters/gaussian_blur_filter_contents_unittests.cc
 ../../../flutter/impeller/entity/contents/filters/inputs/filter_input_unittests.cc

impeller/entity/BUILD.gn

@@ -266,6 +266,7 @@ impeller_component("entity_unittests") {
 
   sources = [
     "contents/checkerboard_contents_unittests.cc",
+    "contents/content_context_unittests.cc",
     "contents/filters/directional_gaussian_blur_filter_contents_unittests.cc",
     "contents/filters/gaussian_blur_filter_contents_unittests.cc",
     "contents/filters/inputs/filter_input_unittests.cc",

impeller/entity/contents/content_context.cc

@@ -504,4 +504,16 @@ ContentContext::GetCachedRuntimeEffectPipeline(
   return it->second;
 }
 
+void ContentContext::ClearCachedRuntimeEffectPipeline(
+    const std::string& unique_entrypoint_name) const {
+  for (auto it = runtime_effect_pipelines_.begin();
+       it != runtime_effect_pipelines_.end();) {
+    if (it->first.unique_entrypoint_name == unique_entrypoint_name) {
+      it = runtime_effect_pipelines_.erase(it);
+    } else {
+      it++;
+    }
+  }
+}
+
 }  // namespace impeller

impeller/entity/contents/content_context.h

@@ -745,6 +745,11 @@ class ContentContext {
       const std::function<std::shared_ptr<Pipeline<PipelineDescriptor>>()>&
           create_callback) const;
 
+  /// Used by hot reload/hot restart to clear a cached pipeline from
+  /// GetCachedRuntimeEffectPipeline.
+  void ClearCachedRuntimeEffectPipeline(
+      const std::string& unique_entrypoint_name) const;
+
   /// @brief Retrieve the currnent host buffer for transient storage.
   ///
   /// This is only safe to use from the raster threads. Other threads should

impeller/entity/contents/content_context_unittests.cc

@@ -0,0 +1,139 @@
+// Copyright 2013 The Flutter Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include <cstdint>
+
+#include "gtest/gtest.h"
+
+#include "impeller/core/allocator.h"
+#include "impeller/core/device_buffer_descriptor.h"
+#include "impeller/entity/contents/content_context.h"
+#include "impeller/geometry/color.h"
+#include "impeller/renderer/capabilities.h"
+#include "impeller/renderer/pipeline.h"
+#include "impeller/renderer/pipeline_descriptor.h"
+
+namespace impeller {
+namespace testing {
+
+class FakeAllocator : public Allocator {
+ public:
+  FakeAllocator() : Allocator() {}
+
+  uint16_t MinimumBytesPerRow(PixelFormat format) const override { return 0; }
+  ISize GetMaxTextureSizeSupported() const override { return ISize(); }
+
+  std::shared_ptr<DeviceBuffer> OnCreateBuffer(
+      const DeviceBufferDescriptor& desc) override {
+    return nullptr;
+  }
+  std::shared_ptr<Texture> OnCreateTexture(
+      const TextureDescriptor& desc) override {
+    return nullptr;
+  }
+};
+
+class FakeContext : public Context {
+ public:
+  FakeContext() : Context(), allocator_(std::make_shared<FakeAllocator>()) {}
+
+  BackendType GetBackendType() const override { return BackendType::kVulkan; }
+  std::string DescribeGpuModel() const override { return ""; }
+  bool IsValid() const override { return false; }
+  const std::shared_ptr<const Capabilities>& GetCapabilities() const override {
+    return capabilities_;
+  }
+  std::shared_ptr<Allocator> GetResourceAllocator() const override {
+    return allocator_;
+  }
+  std::shared_ptr<ShaderLibrary> GetShaderLibrary() const { return nullptr; }
+  std::shared_ptr<SamplerLibrary> GetSamplerLibrary() const { return nullptr; }
+  std::shared_ptr<PipelineLibrary> GetPipelineLibrary() const {
+    return nullptr;
+  }
+  std::shared_ptr<CommandBuffer> CreateCommandBuffer() const { return nullptr; }
+  void Shutdown() {}
+
+ private:
+  std::shared_ptr<Allocator> allocator_;
+  std::shared_ptr<const Capabilities> capabilities_;
+};
+
+class FakePipeline : public Pipeline<PipelineDescriptor> {
+ public:
+  FakePipeline() : Pipeline({}, PipelineDescriptor{}) {}
+
+  bool IsValid() const override { return false; }
+};
+
+static std::shared_ptr<FakePipeline> CreateFakePipelineCallback() {
+  return std::make_shared<FakePipeline>();
+}
+
+TEST(ContentContext, CachesPipelines) {
+  auto context = std::make_shared<FakeContext>();
+  ContentContext content_context(context, nullptr);
+  ContentContextOptions optionsA{.blend_mode = BlendMode::kSourceOver};
+  ContentContextOptions optionsB{.blend_mode = BlendMode::kSource};
+
+  auto pipelineA = content_context.GetCachedRuntimeEffectPipeline(
+      "A", optionsA, CreateFakePipelineCallback);
+
+  auto pipelineA2 = content_context.GetCachedRuntimeEffectPipeline(
+      "A", optionsA, CreateFakePipelineCallback);
+
+  auto pipelineA3 = content_context.GetCachedRuntimeEffectPipeline(
+      "A", optionsB, CreateFakePipelineCallback);
+
+  auto pipelineB = content_context.GetCachedRuntimeEffectPipeline(
+      "B", optionsB, CreateFakePipelineCallback);
+
+  ASSERT_EQ(pipelineA.get(), pipelineA2.get());
+  ASSERT_NE(pipelineA.get(), pipelineA3.get());
+  ASSERT_NE(pipelineB.get(), pipelineA.get());
+}
+
+TEST(ContentContext, InvalidatesAllPipelinesWithSameUniqueNameOnClear) {
+  auto context = std::make_shared<FakeContext>();
+  ContentContext content_context(context, nullptr);
+  ContentContextOptions optionsA{.blend_mode = BlendMode::kSourceOver};
+  ContentContextOptions optionsB{.blend_mode = BlendMode::kSource};
+
+  auto pipelineA = content_context.GetCachedRuntimeEffectPipeline(
+      "A", optionsA, CreateFakePipelineCallback);
+
+  auto pipelineA2 = content_context.GetCachedRuntimeEffectPipeline(
+      "A", optionsB, CreateFakePipelineCallback);
+
+  auto pipelineB = content_context.GetCachedRuntimeEffectPipeline(
+      "B", optionsB, CreateFakePipelineCallback);
+
+  ASSERT_TRUE(pipelineA);
+  ASSERT_TRUE(pipelineA2);
+  ASSERT_TRUE(pipelineB);
+
+  ASSERT_EQ(pipelineA, content_context.GetCachedRuntimeEffectPipeline(
+                           "A", optionsA, CreateFakePipelineCallback));
+  ASSERT_EQ(pipelineA2, content_context.GetCachedRuntimeEffectPipeline(
+                            "A", optionsB, CreateFakePipelineCallback));
+  ASSERT_EQ(pipelineB, content_context.GetCachedRuntimeEffectPipeline(
+                           "B", optionsB, CreateFakePipelineCallback));
+
+  content_context.ClearCachedRuntimeEffectPipeline("A");
+
+  ASSERT_NE(pipelineA, content_context.GetCachedRuntimeEffectPipeline(
+                           "A", optionsA, CreateFakePipelineCallback));
+  ASSERT_NE(pipelineA2, content_context.GetCachedRuntimeEffectPipeline(
+                            "A", optionsB, CreateFakePipelineCallback));
+  ASSERT_EQ(pipelineB, content_context.GetCachedRuntimeEffectPipeline(
+                           "B", optionsB, CreateFakePipelineCallback));
+
+  content_context.ClearCachedRuntimeEffectPipeline("B");
+
+  ASSERT_NE(pipelineB, content_context.GetCachedRuntimeEffectPipeline(
+                           "B", optionsB, CreateFakePipelineCallback));
+}
+
+}  // namespace testing
+}  // namespace impeller

impeller/entity/contents/runtime_effect_contents.cc

@@ -80,7 +80,21 @@ bool RuntimeEffectContents::Render(const ContentContext& renderer,
   std::shared_ptr<const ShaderFunction> function = library->GetFunction(
       runtime_stage_->GetEntrypoint(), ShaderStage::kFragment);
 
+  //--------------------------------------------------------------------------
+  /// Resolve geometry and content context options.
+  ///
+
+  auto geometry_result =
+      GetGeometry()->GetPositionBuffer(renderer, entity, pass);
+  auto options = OptionsFromPassAndEntity(pass, entity);
+  if (geometry_result.prevent_overdraw) {
+    options.stencil_compare = CompareFunction::kEqual;
+    options.stencil_operation = StencilOperation::kIncrementClamp;
+  }
+  options.primitive_type = geometry_result.type;
+
   if (function && runtime_stage_->IsDirty()) {
+    renderer.ClearCachedRuntimeEffectPipeline(runtime_stage_->GetEntrypoint());
     context->GetPipelineLibrary()->RemovePipelinesWithEntryPoint(function);
     library->UnregisterFunction(runtime_stage_->GetEntrypoint(),
                                 ShaderStage::kFragment);
@@ -119,13 +133,6 @@ bool RuntimeEffectContents::Render(const ContentContext& renderer,
     runtime_stage_->SetClean();
   }
 
-  //--------------------------------------------------------------------------
-  /// Resolve geometry.
-  ///
-
-  auto geometry_result =
-      GetGeometry()->GetPositionBuffer(renderer, entity, pass);
-
   //--------------------------------------------------------------------------
   /// Set up the command. Defer setting up the pipeline until the descriptor set
   /// layouts are known from the uniforms.
@@ -275,14 +282,6 @@ bool RuntimeEffectContents::Render(const ContentContext& renderer,
   }
 
   /// Now that the descriptor set layouts are known, get the pipeline.
-
-  auto options = OptionsFromPassAndEntity(pass, entity);
-  if (geometry_result.prevent_overdraw) {
-    options.stencil_compare = CompareFunction::kEqual;
-    options.stencil_operation = StencilOperation::kIncrementClamp;
-  }
-  options.primitive_type = geometry_result.type;
-
   auto create_callback =
       [&]() -> std::shared_ptr<Pipeline<PipelineDescriptor>> {
     PipelineDescriptor desc;

impeller/entity/entity_unittests.cc

@@ -2140,14 +2140,10 @@ TEST_P(EntityTest, RuntimeEffect) {
   ASSERT_TRUE(runtime_stage);
   ASSERT_TRUE(runtime_stage->IsDirty());
 
-  bool first_frame = true;
+  bool expect_dirty = true;
   Pipeline<PipelineDescriptor>* first_pipeline = nullptr;
   auto callback = [&](ContentContext& context, RenderPass& pass) -> bool {
-    if (first_frame) {
-      first_frame = false;
-    } else {
-      assert(runtime_stage->IsDirty() == false);
-    }
+    EXPECT_EQ(runtime_stage->IsDirty(), expect_dirty);
 
     auto contents = std::make_shared<RuntimeEffectContents>();
     contents->SetGeometry(Geometry::MakeCover());
@@ -2169,13 +2165,38 @@ TEST_P(EntityTest, RuntimeEffect) {
     Entity entity;
     entity.SetContents(contents);
     bool result = contents->Render(context, entity, pass);
-    if (!first_pipeline) {
+
+    if (expect_dirty) {
+      EXPECT_NE(first_pipeline, pass.GetCommands().back().pipeline.get());
       first_pipeline = pass.GetCommands().back().pipeline.get();
     } else {
       EXPECT_EQ(pass.GetCommands().back().pipeline.get(), first_pipeline);
     }
+
+    expect_dirty = false;
     return result;
   };
+
+  // Simulate some renders and hot reloading of the shader.
+  auto content_context = GetContentContext();
+  {
+    RenderTarget target;
+    testing::MockRenderPass mock_pass(GetContext(), target);
+    callback(*content_context, mock_pass);
+    callback(*content_context, mock_pass);
+
+    // Dirty the runtime stage.
+    runtime_stages = OpenAssetAsRuntimeStage("runtime_stage_example.frag.iplr");
+    runtime_stage =
+        runtime_stages[PlaygroundBackendToRuntimeStageBackend(GetBackend())];
+
+    ASSERT_TRUE(runtime_stage->IsDirty());
+    expect_dirty = true;
+
+    callback(*content_context, mock_pass);
+    callback(*content_context, mock_pass);
+  }
+
   ASSERT_TRUE(OpenPlaygroundHere(callback));
 }