diff --git a/engine/src/flutter/impeller/renderer/backend/gles/test/texture_gles_unittests.cc b/engine/src/flutter/impeller/renderer/backend/gles/test/texture_gles_unittests.cc index ecada9846d04d..2a2090696218b 100644 --- a/engine/src/flutter/impeller/renderer/backend/gles/test/texture_gles_unittests.cc +++ b/engine/src/flutter/impeller/renderer/backend/gles/test/texture_gles_unittests.cc @@ -67,6 +67,38 @@ TEST_P(TextureGLESTest, CanSetSyncFence) { ASSERT_FALSE(sync_fence.has_value()); } +TEST_P(TextureGLESTest, TextureDtorDeletesFence) { + ContextGLES& context_gles = ContextGLES::Cast(*GetContext()); + if (!context_gles.GetReactor() + ->GetProcTable() + .GetDescription() + ->GetGlVersion() + .IsAtLeast(Version{3, 0, 0})) { + GTEST_SKIP() << "GL Version too low to test sync fence."; + } + + TextureDescriptor desc; + desc.storage_mode = StorageMode::kDevicePrivate; + desc.size = {100, 100}; + desc.format = PixelFormat::kR8G8B8A8UNormInt; + + auto texture = GetContext()->GetResourceAllocator()->CreateTexture(desc); + ASSERT_TRUE(!!texture); + + HandleGLES fence = + context_gles.GetReactor()->CreateHandle(HandleType::kFence); + bool fence_collected = false; + context_gles.GetReactor()->RegisterCleanupCallback( + fence, [&]() { fence_collected = true; }); + TextureGLES::Cast(*texture).SetFence(fence); + + texture.reset(); + ASSERT_TRUE(context_gles.GetReactor()->AddOperation( + [](const ReactorGLES& reactor) {})); + ASSERT_TRUE(context_gles.GetReactor()->React()); + EXPECT_TRUE(fence_collected); +} + TEST_P(TextureGLESTest, Binds2DTexture) { TextureDescriptor desc; desc.storage_mode = StorageMode::kDevicePrivate; diff --git a/engine/src/flutter/impeller/renderer/backend/gles/texture_gles.cc b/engine/src/flutter/impeller/renderer/backend/gles/texture_gles.cc index 6f3ea6c794777..b05b24ee30a89 100644 --- a/engine/src/flutter/impeller/renderer/backend/gles/texture_gles.cc +++ b/engine/src/flutter/impeller/renderer/backend/gles/texture_gles.cc @@ -150,11 +150,13 @@ TextureGLES::TextureGLES(std::shared_ptr reactor, type_(GetTextureTypeFromDescriptor( GetTextureDescriptor(), reactor_->GetProcTable().GetCapabilities())), - handle_(external_handle.has_value() - ? external_handle.value() - : (threadsafe ? reactor_->CreateHandle(ToHandleType(type_)) - : reactor_->CreateUntrackedHandle( - ToHandleType(type_)))), + handle_( + external_handle.has_value() + ? UniqueHandleGLES(reactor_, external_handle.value()) + : (threadsafe + ? UniqueHandleGLES(reactor_, ToHandleType(type_)) + : UniqueHandleGLES::MakeUntracked(reactor_, + ToHandleType(type_)))), is_wrapped_(fbo.has_value() || external_handle.has_value()), wrapped_fbo_(fbo) { // Ensure the texture descriptor itself is valid. @@ -174,16 +176,8 @@ TextureGLES::TextureGLES(std::shared_ptr reactor, is_valid_ = true; } -// |Texture| -TextureGLES::~TextureGLES() { - reactor_->CollectHandle(handle_); - if (!cached_fbo_.IsDead()) { - reactor_->CollectHandle(cached_fbo_); - } -} - void TextureGLES::Leak() { - handle_ = HandleGLES::DeadHandle(); + handle_.Release(); } // |Texture| @@ -194,7 +188,7 @@ bool TextureGLES::IsValid() const { // |Texture| void TextureGLES::SetLabel(std::string_view label) { #ifdef IMPELLER_DEBUG - reactor_->SetDebugLabel(handle_, label); + reactor_->SetDebugLabel(handle_.Get(), label); #endif // IMPELLER_DEBUG } @@ -202,7 +196,8 @@ void TextureGLES::SetLabel(std::string_view label) { void TextureGLES::SetLabel(std::string_view label, std::string_view trailing) { #ifdef IMPELLER_DEBUG if (reactor_->CanSetDebugLabels()) { - reactor_->SetDebugLabel(handle_, std::format("{} {}", label, trailing)); + reactor_->SetDebugLabel(handle_.Get(), + std::format("{} {}", label, trailing)); } #endif // IMPELLER_DEBUG } @@ -283,7 +278,7 @@ bool TextureGLES::OnSetContents(std::shared_ptr mapping, } ReactorGLES::Operation texture_upload = - [handle = handle_, // + [handle = handle_.Get(), // mapping, // format = gles_format.value(), // size = tex_descriptor.size, // @@ -418,7 +413,7 @@ void TextureGLES::InitializeContentsIfNecessary() { } const auto& gl = reactor_->GetProcTable(); - std::optional handle = reactor_->GetGLHandle(handle_); + std::optional handle = reactor_->GetGLHandle(handle_.Get()); if (!handle.has_value()) { VALIDATION_LOG << "Could not initialize the contents of texture."; return; @@ -531,7 +526,7 @@ std::optional TextureGLES::GetGLHandle() const { if (!IsValid()) { return std::nullopt; } - return reactor_->GetGLHandle(handle_); + return reactor_->GetGLHandle(handle_.Get()); } bool TextureGLES::Bind() { @@ -541,13 +536,12 @@ bool TextureGLES::Bind() { } const auto& gl = reactor_->GetProcTable(); - if (fence_.has_value()) { - std::optional fence = reactor_->GetGLFence(fence_.value()); + if (fence_.IsValid()) { + std::optional fence = reactor_->GetGLFence(fence_.Get()); if (fence.has_value()) { gl.WaitSync(fence.value(), 0, GL_TIMEOUT_IGNORED); } - reactor_->CollectHandle(fence_.value()); - fence_ = std::nullopt; + fence_.Reset(); } switch (type_) { @@ -711,21 +705,21 @@ std::optional TextureGLES::GetFBO() const { } void TextureGLES::SetFence(HandleGLES fence) { - FML_DCHECK(!fence_.has_value()); - fence_ = fence; + FML_DCHECK(!fence_.IsValid()); + fence_ = UniqueHandleGLES(reactor_, fence); } // Visible for testing. std::optional TextureGLES::GetSyncFence() const { - return fence_; + return fence_.IsValid() ? std::optional(fence_.Get()) : std::nullopt; } void TextureGLES::SetCachedFBO(HandleGLES fbo) { - cached_fbo_ = fbo; + cached_fbo_ = UniqueHandleGLES(reactor_, fbo); } const HandleGLES& TextureGLES::GetCachedFBO() const { - return cached_fbo_; + return cached_fbo_.Get(); } } // namespace impeller diff --git a/engine/src/flutter/impeller/renderer/backend/gles/texture_gles.h b/engine/src/flutter/impeller/renderer/backend/gles/texture_gles.h index e60a4378f068f..d6635b36c0940 100644 --- a/engine/src/flutter/impeller/renderer/backend/gles/texture_gles.h +++ b/engine/src/flutter/impeller/renderer/backend/gles/texture_gles.h @@ -13,6 +13,7 @@ #include "impeller/core/texture.h" #include "impeller/renderer/backend/gles/handle_gles.h" #include "impeller/renderer/backend/gles/reactor_gles.h" +#include "impeller/renderer/backend/gles/unique_handle_gles.h" namespace impeller { @@ -80,9 +81,6 @@ class TextureGLES final : public Texture, TextureDescriptor desc, bool threadsafe = false); - // |Texture| - ~TextureGLES() override; - // |Texture| bool IsValid() const override; @@ -173,8 +171,8 @@ class TextureGLES final : public Texture, private: std::shared_ptr reactor_; const Type type_; - HandleGLES handle_; - std::optional fence_ = std::nullopt; + UniqueHandleGLES handle_; + UniqueHandleGLES fence_; // Tracks which `(slice, mip_level)` pairs have had their storage allocated // by a `glTexImage2D` call. Allocation is performed lazily on first write // to a level so the only-renders-then-mipmaps path (Impeller's snapshot @@ -187,7 +185,7 @@ class TextureGLES final : public Texture, std::array, 6> slice_mip_initialized_ = {}; const bool is_wrapped_; const std::optional wrapped_fbo_; - HandleGLES cached_fbo_ = HandleGLES::DeadHandle(); + UniqueHandleGLES cached_fbo_; bool is_valid_ = false; TextureGLES(std::shared_ptr reactor, diff --git a/engine/src/flutter/impeller/renderer/backend/gles/unique_handle_gles.cc b/engine/src/flutter/impeller/renderer/backend/gles/unique_handle_gles.cc index d45dd4d79a550..89f67dc7f2aef 100644 --- a/engine/src/flutter/impeller/renderer/backend/gles/unique_handle_gles.cc +++ b/engine/src/flutter/impeller/renderer/backend/gles/unique_handle_gles.cc @@ -30,6 +30,10 @@ UniqueHandleGLES::UniqueHandleGLES(std::shared_ptr reactor, : reactor_(std::move(reactor)), handle_(handle) {} UniqueHandleGLES::~UniqueHandleGLES() { + CollectHandle(); +} + +void UniqueHandleGLES::CollectHandle() { if (!handle_.IsDead() && reactor_) { reactor_->CollectHandle(handle_); } @@ -43,9 +47,31 @@ bool UniqueHandleGLES::IsValid() const { return !handle_.IsDead(); } +void UniqueHandleGLES::Reset() { + CollectHandle(); + reactor_.reset(); + handle_ = HandleGLES::DeadHandle(); +} + +HandleGLES UniqueHandleGLES::Release() { + reactor_.reset(); + HandleGLES old_handle = handle_; + handle_ = HandleGLES::DeadHandle(); + return old_handle; +} + UniqueHandleGLES::UniqueHandleGLES(UniqueHandleGLES&& other) { std::swap(reactor_, other.reactor_); std::swap(handle_, other.handle_); } +UniqueHandleGLES& UniqueHandleGLES::operator=(UniqueHandleGLES&& other) { + if (this != &other) { + Reset(); + std::swap(reactor_, other.reactor_); + std::swap(handle_, other.handle_); + } + return *this; +} + } // namespace impeller diff --git a/engine/src/flutter/impeller/renderer/backend/gles/unique_handle_gles.h b/engine/src/flutter/impeller/renderer/backend/gles/unique_handle_gles.h index e5bbc33293bdc..10390c264a599 100644 --- a/engine/src/flutter/impeller/renderer/backend/gles/unique_handle_gles.h +++ b/engine/src/flutter/impeller/renderer/backend/gles/unique_handle_gles.h @@ -17,6 +17,8 @@ namespace impeller { /// class UniqueHandleGLES { public: + UniqueHandleGLES() = default; + UniqueHandleGLES(std::shared_ptr reactor, HandleType type); static UniqueHandleGLES MakeUntracked(std::shared_ptr reactor, @@ -27,18 +29,26 @@ class UniqueHandleGLES { ~UniqueHandleGLES(); UniqueHandleGLES(UniqueHandleGLES&&); + UniqueHandleGLES& operator=(UniqueHandleGLES&&); UniqueHandleGLES(const UniqueHandleGLES&) = delete; - UniqueHandleGLES& operator=(const UniqueHandleGLES&) = delete; const HandleGLES& Get() const; bool IsValid() const; + /// Collect the managed handle and replace it with a dead handle. + void Reset(); + + /// Release ownership of the handle. + HandleGLES Release(); + private: std::shared_ptr reactor_ = nullptr; HandleGLES handle_ = HandleGLES::DeadHandle(); + + void CollectHandle(); }; } // namespace impeller