From 557002783a14eb75f951267d3a319040192819d1 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Thu, 11 Jan 2024 16:45:06 -0800 Subject: [PATCH 1/3] Fix hot reload of shaders fo rImpeller --- impeller/entity/contents/content_context.h | 8 +++++ .../contents/runtime_effect_contents.cc | 30 ++++++++-------- impeller/entity/entity_unittests.cc | 36 +++++++++++++++---- 3 files changed, 52 insertions(+), 22 deletions(-) diff --git a/impeller/entity/contents/content_context.h b/impeller/entity/contents/content_context.h index 94d4af747750c..1f2c915757487 100644 --- a/impeller/entity/contents/content_context.h +++ b/impeller/entity/contents/content_context.h @@ -745,6 +745,14 @@ class ContentContext { const std::function>()>& 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 ContentContextOptions& options) const { + runtime_effect_pipelines_.erase({unique_entrypoint_name, options}); + } + /// @brief Retrieve the currnent host buffer for transient storage. /// /// This is only safe to use from the raster threads. Other threads should diff --git a/impeller/entity/contents/runtime_effect_contents.cc b/impeller/entity/contents/runtime_effect_contents.cc index 0b1e049aeef21..4e90dca3e9a2e 100644 --- a/impeller/entity/contents/runtime_effect_contents.cc +++ b/impeller/entity/contents/runtime_effect_contents.cc @@ -80,7 +80,22 @@ bool RuntimeEffectContents::Render(const ContentContext& renderer, std::shared_ptr 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(), + options); context->GetPipelineLibrary()->RemovePipelinesWithEntryPoint(function); library->UnregisterFunction(runtime_stage_->GetEntrypoint(), ShaderStage::kFragment); @@ -119,13 +134,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. @@ -276,14 +284,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> { PipelineDescriptor desc; diff --git a/impeller/entity/entity_unittests.cc b/impeller/entity/entity_unittests.cc index 520d2ee9ee3db..23beb6a1dec7b 100644 --- a/impeller/entity/entity_unittests.cc +++ b/impeller/entity/entity_unittests.cc @@ -48,6 +48,7 @@ #include "impeller/renderer/command.h" #include "impeller/renderer/pipeline_descriptor.h" #include "impeller/renderer/render_pass.h" +#include "impeller/renderer/testing/mocks.h" #include "impeller/renderer/vertex_buffer_builder.h" #include "impeller/typographer/backends/skia/text_frame_skia.h" #include "impeller/typographer/backends/skia/typographer_context_skia.h" @@ -2139,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* 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(); contents->SetGeometry(Geometry::MakeCover()); @@ -2168,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)); } From f8092f75357caa1d36ac855a4761e60f4cea2f00 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Fri, 12 Jan 2024 14:43:57 -0800 Subject: [PATCH 2/3] Fix reloading of related shader pipelines --- impeller/entity/BUILD.gn | 1 + impeller/entity/contents/content_context.cc | 12 ++ impeller/entity/contents/content_context.h | 5 +- .../contents/content_context_unittests.cc | 139 ++++++++++++++++++ .../contents/runtime_effect_contents.cc | 3 +- 5 files changed, 154 insertions(+), 6 deletions(-) create mode 100644 impeller/entity/contents/content_context_unittests.cc diff --git a/impeller/entity/BUILD.gn b/impeller/entity/BUILD.gn index c3fc2a8dca355..6dd1db0680aa4 100644 --- a/impeller/entity/BUILD.gn +++ b/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", diff --git a/impeller/entity/contents/content_context.cc b/impeller/entity/contents/content_context.cc index 634ae0b8e6fa7..a285b4a8c4ea7 100644 --- a/impeller/entity/contents/content_context.cc +++ b/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 diff --git a/impeller/entity/contents/content_context.h b/impeller/entity/contents/content_context.h index 1f2c915757487..9857afffebd68 100644 --- a/impeller/entity/contents/content_context.h +++ b/impeller/entity/contents/content_context.h @@ -748,10 +748,7 @@ class ContentContext { /// Used by hot reload/hot restart to clear a cached pipeline from /// GetCachedRuntimeEffectPipeline. void ClearCachedRuntimeEffectPipeline( - const std::string& unique_entrypoint_name, - const ContentContextOptions& options) const { - runtime_effect_pipelines_.erase({unique_entrypoint_name, options}); - } + const std::string& unique_entrypoint_name) const; /// @brief Retrieve the currnent host buffer for transient storage. /// diff --git a/impeller/entity/contents/content_context_unittests.cc b/impeller/entity/contents/content_context_unittests.cc new file mode 100644 index 0000000000000..38d17c181843d --- /dev/null +++ b/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 + +#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 OnCreateBuffer( + const DeviceBufferDescriptor& desc) override { + return nullptr; + } + std::shared_ptr OnCreateTexture( + const TextureDescriptor& desc) override { + return nullptr; + } +}; + +class FakeContext : public Context { + public: + FakeContext() : Context(), allocator_(std::make_shared()) {} + + BackendType GetBackendType() const override { return BackendType::kVulkan; } + std::string DescribeGpuModel() const override { return ""; } + bool IsValid() const override { return false; } + const std::shared_ptr& GetCapabilities() const override { + return capabilities_; + } + std::shared_ptr GetResourceAllocator() const override { + return allocator_; + } + std::shared_ptr GetShaderLibrary() const { return nullptr; } + std::shared_ptr GetSamplerLibrary() const { return nullptr; } + std::shared_ptr GetPipelineLibrary() const { + return nullptr; + } + std::shared_ptr CreateCommandBuffer() const { return nullptr; } + void Shutdown() {} + + private: + std::shared_ptr allocator_; + std::shared_ptr capabilities_; +}; + +class FakePipeline : public Pipeline { + public: + FakePipeline() : Pipeline({}, PipelineDescriptor{}) {} + + bool IsValid() const override { return false; } +}; + +static std::shared_ptr CreateFakePipelineCallback() { + return std::make_shared(); +} + +TEST(ContentContext, CachesPipelines) { + auto context = std::make_shared(); + 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(); + 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 diff --git a/impeller/entity/contents/runtime_effect_contents.cc b/impeller/entity/contents/runtime_effect_contents.cc index 4e90dca3e9a2e..b68b049b25819 100644 --- a/impeller/entity/contents/runtime_effect_contents.cc +++ b/impeller/entity/contents/runtime_effect_contents.cc @@ -94,8 +94,7 @@ bool RuntimeEffectContents::Render(const ContentContext& renderer, options.primitive_type = geometry_result.type; if (function && runtime_stage_->IsDirty()) { - renderer.ClearCachedRuntimeEffectPipeline(runtime_stage_->GetEntrypoint(), - options); + renderer.ClearCachedRuntimeEffectPipeline(runtime_stage_->GetEntrypoint()); context->GetPipelineLibrary()->RemovePipelinesWithEntryPoint(function); library->UnregisterFunction(runtime_stage_->GetEntrypoint(), ShaderStage::kFragment); From e0b9457ee87fa2aa23f0b86ebd26d3f6407cba5e Mon Sep 17 00:00:00 2001 From: Dan Field Date: Fri, 12 Jan 2024 15:37:07 -0800 Subject: [PATCH 3/3] license bot --- ci/licenses_golden/excluded_files | 1 + 1 file changed, 1 insertion(+) diff --git a/ci/licenses_golden/excluded_files b/ci/licenses_golden/excluded_files index 8feec5940738a..9df7d08a331da 100644 --- a/ci/licenses_golden/excluded_files +++ b/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