From 4da499244ff409e60ee41239f2577c0f6f55e852 Mon Sep 17 00:00:00 2001 From: Brandon DeRosier Date: Wed, 2 Nov 2022 03:40:54 -0700 Subject: [PATCH] [Impeller] Add ability to unregister shaders (flutter/engine#37229) --- .../backend/gles/shader_library_gles.cc | 19 +++++ .../backend/gles/shader_library_gles.h | 3 + .../backend/metal/shader_library_mtl.h | 3 + .../backend/metal/shader_library_mtl.mm | 71 ++++++++++++++----- .../backend/vulkan/shader_library_vk.cc | 20 ++++++ .../backend/vulkan/shader_library_vk.h | 5 ++ .../impeller/renderer/shader_library.h | 2 + .../runtime_stage/runtime_stage_unittests.cc | 17 ++++- 8 files changed, 120 insertions(+), 20 deletions(-) diff --git a/engine/src/flutter/impeller/renderer/backend/gles/shader_library_gles.cc b/engine/src/flutter/impeller/renderer/backend/gles/shader_library_gles.cc index bcf5c83e0b..f3c273c63e 100644 --- a/engine/src/flutter/impeller/renderer/backend/gles/shader_library_gles.cc +++ b/engine/src/flutter/impeller/renderer/backend/gles/shader_library_gles.cc @@ -137,4 +137,23 @@ void ShaderLibraryGLES::RegisterFunction(std::string name, callback(true); } +// |ShaderLibrary| +void ShaderLibraryGLES::UnregisterFunction(std::string name, + ShaderStage stage) { + ReaderLock lock(functions_mutex_); + + const auto key = ShaderKey{name, stage}; + + auto found = functions_.find(key); + if (found != functions_.end()) { + VALIDATION_LOG << "Library function named " << name + << " was not found, so it couldn't be unregistered."; + return; + } + + functions_.erase(found); + + return; +} + } // namespace impeller diff --git a/engine/src/flutter/impeller/renderer/backend/gles/shader_library_gles.h b/engine/src/flutter/impeller/renderer/backend/gles/shader_library_gles.h index a5e9098092..06750077d6 100644 --- a/engine/src/flutter/impeller/renderer/backend/gles/shader_library_gles.h +++ b/engine/src/flutter/impeller/renderer/backend/gles/shader_library_gles.h @@ -43,6 +43,9 @@ class ShaderLibraryGLES final : public ShaderLibrary { std::shared_ptr code, RegistrationCallback callback) override; + // |ShaderLibrary| + void UnregisterFunction(std::string name, ShaderStage stage) override; + FML_DISALLOW_COPY_AND_ASSIGN(ShaderLibraryGLES); }; diff --git a/engine/src/flutter/impeller/renderer/backend/metal/shader_library_mtl.h b/engine/src/flutter/impeller/renderer/backend/metal/shader_library_mtl.h index 3ee884901b..d38d1b5a33 100644 --- a/engine/src/flutter/impeller/renderer/backend/metal/shader_library_mtl.h +++ b/engine/src/flutter/impeller/renderer/backend/metal/shader_library_mtl.h @@ -51,6 +51,9 @@ class ShaderLibraryMTL final : public ShaderLibrary { std::shared_ptr code, RegistrationCallback callback) override; + // |ShaderLibrary| + void UnregisterFunction(std::string name, ShaderStage stage) override; + id GetDevice() const; void RegisterLibrary(id library); diff --git a/engine/src/flutter/impeller/renderer/backend/metal/shader_library_mtl.mm b/engine/src/flutter/impeller/renderer/backend/metal/shader_library_mtl.mm index 57ab6d9bec..94fb1e335b 100644 --- a/engine/src/flutter/impeller/renderer/backend/metal/shader_library_mtl.mm +++ b/engine/src/flutter/impeller/renderer/backend/metal/shader_library_mtl.mm @@ -54,36 +54,38 @@ std::shared_ptr ShaderLibraryMTL::GetFunction( ShaderKey key(name, stage); - if (auto found = functions_.find(key); found != functions_.end()) { - return found->second; - } - id function = nil; { ReaderLock lock(libraries_mutex_); + + if (auto found = functions_.find(key); found != functions_.end()) { + return found->second; + } + for (size_t i = 0, count = [libraries_ count]; i < count; i++) { function = [libraries_[i] newFunctionWithName:@(name.data())]; if (function) { break; } } - } - if (function == nil) { - return nullptr; - } + if (function == nil) { + return nullptr; + } - if (function.functionType != ToMTLFunctionType(stage)) { - VALIDATION_LOG << "Library function named " << name - << " was for an unexpected shader stage."; - return nullptr; - } + if (function.functionType != ToMTLFunctionType(stage)) { + VALIDATION_LOG << "Library function named " << name + << " was for an unexpected shader stage."; + return nullptr; + } - auto func = std::shared_ptr(new ShaderFunctionMTL( - library_id_, function, {name.data(), name.size()}, stage)); - functions_[key] = func; - return func; + auto func = std::shared_ptr(new ShaderFunctionMTL( + library_id_, function, {name.data(), name.size()}, stage)); + functions_[key] = func; + + return func; + } } id ShaderLibraryMTL::GetDevice() const { @@ -141,6 +143,41 @@ void ShaderLibraryMTL::RegisterFunction(std::string name, // unused }]; } +// |ShaderLibrary| +void ShaderLibraryMTL::UnregisterFunction(std::string name, ShaderStage stage) { + ReaderLock lock(libraries_mutex_); + + // Find the shader library containing this function name and remove it. + + bool found_library = false; + for (size_t i = [libraries_ count] - 1; i >= 0; i--) { + id function = + [libraries_[i] newFunctionWithName:@(name.data())]; + if (function) { + [libraries_ removeObjectAtIndex:i]; + found_library = true; + break; + } + } + if (!found_library) { + VALIDATION_LOG << "Library containing function " << name + << " was not found, so it couldn't be unregistered."; + } + + // Remove the shader from the function cache. + + ShaderKey key(name, stage); + + auto found = functions_.find(key); + if (found == functions_.end()) { + VALIDATION_LOG << "Library function named " << name + << " was not found, so it couldn't be unregistered."; + return; + } + + functions_.erase(found); +} + void ShaderLibraryMTL::RegisterLibrary(id library) { WriterLock lock(libraries_mutex_); [libraries_ addObject:library]; diff --git a/engine/src/flutter/impeller/renderer/backend/vulkan/shader_library_vk.cc b/engine/src/flutter/impeller/renderer/backend/vulkan/shader_library_vk.cc index 3e8c40cac3..23f7ad1b9b 100644 --- a/engine/src/flutter/impeller/renderer/backend/vulkan/shader_library_vk.cc +++ b/engine/src/flutter/impeller/renderer/backend/vulkan/shader_library_vk.cc @@ -114,6 +114,8 @@ bool ShaderLibraryVK::IsValid() const { std::shared_ptr ShaderLibraryVK::GetFunction( std::string_view name, ShaderStage stage) { + ReaderLock lock(functions_mutex_); + const auto key = ShaderKey{{name.data(), name.size()}, stage}; auto found = functions_.find(key); if (found != functions_.end()) { @@ -122,4 +124,22 @@ std::shared_ptr ShaderLibraryVK::GetFunction( return nullptr; } +// |ShaderLibrary| +void ShaderLibraryVK::UnregisterFunction(std::string name, ShaderStage stage) { + ReaderLock lock(functions_mutex_); + + const auto key = ShaderKey{name, stage}; + + auto found = functions_.find(key); + if (found != functions_.end()) { + VALIDATION_LOG << "Library function named " << name + << " was not found, so it couldn't be unregistered."; + return; + } + + functions_.erase(found); + + return; +} + } // namespace impeller diff --git a/engine/src/flutter/impeller/renderer/backend/vulkan/shader_library_vk.h b/engine/src/flutter/impeller/renderer/backend/vulkan/shader_library_vk.h index 0b446757d6..c02eedb569 100644 --- a/engine/src/flutter/impeller/renderer/backend/vulkan/shader_library_vk.h +++ b/engine/src/flutter/impeller/renderer/backend/vulkan/shader_library_vk.h @@ -6,6 +6,7 @@ #include "flutter/fml/macros.h" #include "impeller/base/comparable.h" +#include "impeller/base/thread.h" #include "impeller/renderer/backend/vulkan/vk.h" #include "impeller/renderer/shader_key.h" #include "impeller/renderer/shader_library.h" @@ -23,6 +24,7 @@ class ShaderLibraryVK final : public ShaderLibrary { private: friend class ContextVK; const UniqueID library_id_; + mutable RWMutex functions_mutex_; ShaderFunctionMap functions_; bool is_valid_ = false; @@ -34,6 +36,9 @@ class ShaderLibraryVK final : public ShaderLibrary { std::shared_ptr GetFunction(std::string_view name, ShaderStage stage) override; + // |ShaderLibrary| + void UnregisterFunction(std::string name, ShaderStage stage) override; + FML_DISALLOW_COPY_AND_ASSIGN(ShaderLibraryVK); }; diff --git a/engine/src/flutter/impeller/renderer/shader_library.h b/engine/src/flutter/impeller/renderer/shader_library.h index 8a5bfbeec0..3c1b4f9a73 100644 --- a/engine/src/flutter/impeller/renderer/shader_library.h +++ b/engine/src/flutter/impeller/renderer/shader_library.h @@ -33,6 +33,8 @@ class ShaderLibrary : public std::enable_shared_from_this { std::shared_ptr code, RegistrationCallback callback); + virtual void UnregisterFunction(std::string name, ShaderStage stage) = 0; + protected: ShaderLibrary(); diff --git a/engine/src/flutter/impeller/runtime_stage/runtime_stage_unittests.cc b/engine/src/flutter/impeller/runtime_stage/runtime_stage_unittests.cc index 99efecefa8..15b585a817 100644 --- a/engine/src/flutter/impeller/runtime_stage/runtime_stage_unittests.cc +++ b/engine/src/flutter/impeller/runtime_stage/runtime_stage_unittests.cc @@ -207,9 +207,20 @@ TEST_P(RuntimeStageTest, CanRegisterStage) { reg.set_value(result); })); ASSERT_TRUE(future.get()); - auto function = - library->GetFunction(stage.GetEntrypoint(), ShaderStage::kFragment); - ASSERT_NE(function, nullptr); + { + auto function = + library->GetFunction(stage.GetEntrypoint(), ShaderStage::kFragment); + ASSERT_NE(function, nullptr); + } + + // Check if unregistering works. + + library->UnregisterFunction(stage.GetEntrypoint(), ShaderStage::kFragment); + { + auto function = + library->GetFunction(stage.GetEntrypoint(), ShaderStage::kFragment); + ASSERT_EQ(function, nullptr); + } } TEST_P(RuntimeStageTest, CanCreatePipelineFromRuntimeStage) {