From 0cfe5441edd821c8d89ac29f848465c1f5011c84 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Mon, 13 Mar 2023 23:02:49 -0700 Subject: [PATCH 1/3] Preserve order when regenerating GPU images --- common/graphics/texture.cc | 33 +++++++++++--- common/graphics/texture.h | 10 ++++- flow/texture_unittests.cc | 43 +++++++++++++++++++ .../display_list_deferred_image_gpu_skia.cc | 1 - 4 files changed, 78 insertions(+), 9 deletions(-) diff --git a/common/graphics/texture.cc b/common/graphics/texture.cc index 318dcc244b928..311c12453cd6c 100644 --- a/common/graphics/texture.cc +++ b/common/graphics/texture.cc @@ -14,7 +14,7 @@ Texture::Texture(int64_t id) : id_(id) {} Texture::~Texture() = default; -TextureRegistry::TextureRegistry() = default; +TextureRegistry::TextureRegistry() : image_counter_(0) {} void TextureRegistry::RegisterTexture(const std::shared_ptr& texture) { if (!texture) { @@ -26,7 +26,13 @@ void TextureRegistry::RegisterTexture(const std::shared_ptr& texture) { void TextureRegistry::RegisterContextListener( uintptr_t id, std::weak_ptr image) { - images_[id] = std::move(image); + size_t next_id = image_counter_.fetch_add(1); + auto const result = image_indices_.insert({id, next_id}); + if (!result.second) { + ordered_images_.erase(result.first->second); + result.first->second = next_id; + } + ordered_images_[next_id] = {next_id, std::move(image)}; } void TextureRegistry::UnregisterTexture(int64_t id) { @@ -39,7 +45,8 @@ void TextureRegistry::UnregisterTexture(int64_t id) { } void TextureRegistry::UnregisterContextListener(uintptr_t id) { - images_.erase(id); + ordered_images_.erase(image_indices_[id]); + image_indices_.erase(id); } void TextureRegistry::OnGrContextCreated() { @@ -47,11 +54,20 @@ void TextureRegistry::OnGrContextCreated() { it.second->OnGrContextCreated(); } - for (const auto& [id, weak_image] : images_) { + // Calling OnGrContextCreated may result in a subsequent call to + // RegisterContextListener from the listener, which may modify the map. + std::vector< + std::pair>>> + ordered_images(ordered_images_.begin(), ordered_images_.end()); + + for (const auto& [id, pair] : ordered_images) { + auto index_id = pair.first; + auto weak_image = pair.second; if (auto image = weak_image.lock()) { image->OnGrContextCreated(); } else { - images_.erase(id); + image_indices_.erase(index_id); + ordered_images_.erase(id); } } } @@ -61,11 +77,14 @@ void TextureRegistry::OnGrContextDestroyed() { it.second->OnGrContextDestroyed(); } - for (const auto& [id, weak_image] : images_) { + for (const auto& [id, pair] : ordered_images_) { + auto index_id = pair.first; + auto weak_image = pair.second; if (auto image = weak_image.lock()) { image->OnGrContextDestroyed(); } else { - images_.erase(id); + image_indices_.erase(index_id); + ordered_images_.erase(id); } } } diff --git a/common/graphics/texture.h b/common/graphics/texture.h index 69d9306fcba96..e199879ffdd0e 100644 --- a/common/graphics/texture.h +++ b/common/graphics/texture.h @@ -95,7 +95,15 @@ class TextureRegistry { private: std::map> mapping_; - std::map> images_; + std::atomic_size_t image_counter_; + // This map keeps track of registered context listeners by their own + // externally provided id. It indexes into ordered_images_. + std::map image_indices_; + // This map makes sure that iteration of images happens in insertion order + // (managed by image_counter_) so that images which depend on other images get + // re-created in the right order. + std::map>> + ordered_images_; FML_DISALLOW_COPY_AND_ASSIGN(TextureRegistry); }; diff --git a/flow/texture_unittests.cc b/flow/texture_unittests.cc index bb39dbfb39347..632385970495b 100644 --- a/flow/texture_unittests.cc +++ b/flow/texture_unittests.cc @@ -4,12 +4,36 @@ #include "flutter/common/graphics/texture.h" +#include + #include "flutter/flow/testing/mock_texture.h" +#include "gmock/gmock.h" #include "gtest/gtest.h" namespace flutter { namespace testing { +namespace { +using int_closure = std::function; + +struct TestContextListener : public ContextListener { + TestContextListener(uintptr_t p_id, + int_closure p_create, + int_closure p_destroy) + : id(p_id), create(p_create), destroy(p_destroy) {} + + virtual ~TestContextListener() = default; + + const uintptr_t id; + int_closure create; + int_closure destroy; + + void OnGrContextCreated() override { create(id); } + + void OnGrContextDestroyed() override { destroy(id); } +}; +} // namespace + TEST(TextureRegistryTest, UnregisterTextureCallbackTriggered) { TextureRegistry registry; auto mock_texture1 = std::make_shared(0); @@ -95,5 +119,24 @@ TEST(TextureRegistryTest, ReuseSameTextureSlot) { ASSERT_TRUE(mock_texture2->unregistered()); } +TEST(TextureRegistryTest, CallsOnGrContextCreatedInInsertionOrder) { + TextureRegistry registry; + std::vector create_order; + std::vector destroy_order; + auto create = [&](int id) { create_order.push_back(id); }; + auto destroy = [&](int id) { destroy_order.push_back(id); }; + auto a = std::make_shared(5, create, destroy); + auto b = std::make_shared(4, create, destroy); + auto c = std::make_shared(3, create, destroy); + registry.RegisterContextListener(a->id, a); + registry.RegisterContextListener(b->id, b); + registry.RegisterContextListener(c->id, c); + registry.OnGrContextDestroyed(); + registry.OnGrContextCreated(); + + EXPECT_THAT(create_order, ::testing::ElementsAre(5, 4, 3)); + EXPECT_THAT(destroy_order, ::testing::ElementsAre(5, 4, 3)); +} + } // namespace testing } // namespace flutter diff --git a/lib/ui/painting/display_list_deferred_image_gpu_skia.cc b/lib/ui/painting/display_list_deferred_image_gpu_skia.cc index 8b8dfd6fc791e..8d7e6a8c32a4c 100644 --- a/lib/ui/painting/display_list_deferred_image_gpu_skia.cc +++ b/lib/ui/painting/display_list_deferred_image_gpu_skia.cc @@ -137,7 +137,6 @@ void DlDeferredImageGPUSkia::ImageWrapper::OnGrContextCreated() { void DlDeferredImageGPUSkia::ImageWrapper::OnGrContextDestroyed() { FML_DCHECK(raster_task_runner_->RunsTasksOnCurrentThread()); - DeleteTexture(); } From d00fed47d01a8ce0b6c56f310390a5f600285d98 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Tue, 14 Mar 2023 08:57:45 -0700 Subject: [PATCH 2/3] Chinmay review --- common/graphics/texture.cc | 17 +++++++++-------- common/graphics/texture.h | 7 ++++--- flow/texture_unittests.cc | 2 +- testing/testing.gni | 3 ++- 4 files changed, 16 insertions(+), 13 deletions(-) diff --git a/common/graphics/texture.cc b/common/graphics/texture.cc index 311c12453cd6c..07025ff085779 100644 --- a/common/graphics/texture.cc +++ b/common/graphics/texture.cc @@ -26,7 +26,7 @@ void TextureRegistry::RegisterTexture(const std::shared_ptr& texture) { void TextureRegistry::RegisterContextListener( uintptr_t id, std::weak_ptr image) { - size_t next_id = image_counter_.fetch_add(1); + size_t next_id = image_counter_++; auto const result = image_indices_.insert({id, next_id}); if (!result.second) { ordered_images_.erase(result.first->second); @@ -56,9 +56,8 @@ void TextureRegistry::OnGrContextCreated() { // Calling OnGrContextCreated may result in a subsequent call to // RegisterContextListener from the listener, which may modify the map. - std::vector< - std::pair>>> - ordered_images(ordered_images_.begin(), ordered_images_.end()); + std::vector ordered_images( + ordered_images_.begin(), ordered_images_.end()); for (const auto& [id, pair] : ordered_images) { auto index_id = pair.first; @@ -77,14 +76,16 @@ void TextureRegistry::OnGrContextDestroyed() { it.second->OnGrContextDestroyed(); } - for (const auto& [id, pair] : ordered_images_) { - auto index_id = pair.first; - auto weak_image = pair.second; + auto it = ordered_images_.begin(); + while (it != ordered_images_.end()) { + auto index_id = it->second.first; + auto weak_image = it->second.second; if (auto image = weak_image.lock()) { image->OnGrContextDestroyed(); + it++; } else { image_indices_.erase(index_id); - ordered_images_.erase(id); + it = ordered_images_.erase(it); } } } diff --git a/common/graphics/texture.h b/common/graphics/texture.h index e199879ffdd0e..9b582a84da286 100644 --- a/common/graphics/texture.h +++ b/common/graphics/texture.h @@ -95,15 +95,16 @@ class TextureRegistry { private: std::map> mapping_; - std::atomic_size_t image_counter_; + size_t image_counter_; // This map keeps track of registered context listeners by their own // externally provided id. It indexes into ordered_images_. std::map image_indices_; // This map makes sure that iteration of images happens in insertion order // (managed by image_counter_) so that images which depend on other images get // re-created in the right order. - std::map>> - ordered_images_; + using InsertionOrderMap = + std::map>>; + InsertionOrderMap ordered_images_; FML_DISALLOW_COPY_AND_ASSIGN(TextureRegistry); }; diff --git a/flow/texture_unittests.cc b/flow/texture_unittests.cc index 632385970495b..0339be1d524f8 100644 --- a/flow/texture_unittests.cc +++ b/flow/texture_unittests.cc @@ -20,7 +20,7 @@ struct TestContextListener : public ContextListener { TestContextListener(uintptr_t p_id, int_closure p_create, int_closure p_destroy) - : id(p_id), create(p_create), destroy(p_destroy) {} + : id(p_id), create(std::move(p_create)), destroy(std::move(p_destroy)) {} virtual ~TestContextListener() = default; diff --git a/testing/testing.gni b/testing/testing.gni index 3fadcbbc71e16..2bb23b0b6f9dc 100644 --- a/testing/testing.gni +++ b/testing/testing.gni @@ -12,7 +12,8 @@ is_aot_test = # Unit tests targets are only enabled for host machines and Fuchsia right now declare_args() { - enable_unittests = current_toolchain == host_toolchain || is_fuchsia + # enable_unittests = current_toolchain == host_toolchain || is_fuchsia + enable_unittests = true #current_toolchain == host_toolchain || is_fuchsia } # Creates a translation unit that defines the flutter::testing::GetFixturesPath From c6421c56d36b7ef17ce3fbaffeab27170f22c77c Mon Sep 17 00:00:00 2001 From: Dan Field Date: Tue, 14 Mar 2023 09:54:02 -0700 Subject: [PATCH 3/3] Fix bad commit --- testing/testing.gni | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/testing/testing.gni b/testing/testing.gni index 2bb23b0b6f9dc..3fadcbbc71e16 100644 --- a/testing/testing.gni +++ b/testing/testing.gni @@ -12,8 +12,7 @@ is_aot_test = # Unit tests targets are only enabled for host machines and Fuchsia right now declare_args() { - # enable_unittests = current_toolchain == host_toolchain || is_fuchsia - enable_unittests = true #current_toolchain == host_toolchain || is_fuchsia + enable_unittests = current_toolchain == host_toolchain || is_fuchsia } # Creates a translation unit that defines the flutter::testing::GetFixturesPath