From d14d2505f3aa748450ae11ca0996743c2eebf99a Mon Sep 17 00:00:00 2001 From: Jason Simmons Date: Fri, 7 Mar 2025 18:50:23 +0000 Subject: [PATCH] [Impeller] Store the TextureGLES cached framebuffer object as a reactor handle (#164761) TextureGLES references may be owned by garbage collected objects. If a GC drops the last reference to a TextureGLES, then the TextureGLES destructor will run on a thread that does not have an EGL context. That will cause failures when the destructor tries to delete the cached FBO held by the TextureGLES. This PR replaces the raw FBO handle with a ReactorGLES untracked handle. The ReactorGLES will schedule deletion of the underlying FBO on a thread that can call GLES APIs. --- .../renderer/backend/gles/reactor_gles.cc | 2 +- .../renderer/backend/gles/reactor_gles.h | 2 +- .../renderer/backend/gles/render_pass_gles.cc | 22 ++++++++++++------- .../renderer/backend/gles/texture_gles.cc | 8 +++---- .../renderer/backend/gles/texture_gles.h | 8 +++---- 5 files changed, 24 insertions(+), 18 deletions(-) diff --git a/engine/src/flutter/impeller/renderer/backend/gles/reactor_gles.cc b/engine/src/flutter/impeller/renderer/backend/gles/reactor_gles.cc index 111388f400..2cb47560a7 100644 --- a/engine/src/flutter/impeller/renderer/backend/gles/reactor_gles.cc +++ b/engine/src/flutter/impeller/renderer/backend/gles/reactor_gles.cc @@ -200,7 +200,7 @@ bool ReactorGLES::RegisterCleanupCallback(const HandleGLES& handle, return false; } -HandleGLES ReactorGLES::CreateUntrackedHandle(HandleType type) { +HandleGLES ReactorGLES::CreateUntrackedHandle(HandleType type) const { FML_DCHECK(CanReactOnCurrentThread()); auto new_handle = HandleGLES::Create(type); std::optional gl_handle = diff --git a/engine/src/flutter/impeller/renderer/backend/gles/reactor_gles.h b/engine/src/flutter/impeller/renderer/backend/gles/reactor_gles.h index a05dd22b8f..b3eff12bbf 100644 --- a/engine/src/flutter/impeller/renderer/backend/gles/reactor_gles.h +++ b/engine/src/flutter/impeller/renderer/backend/gles/reactor_gles.h @@ -180,7 +180,7 @@ class ReactorGLES { /// creating/accessing the handle. /// @param type The type of handle to create. /// @return The reactor handle. - HandleGLES CreateUntrackedHandle(HandleType type); + HandleGLES CreateUntrackedHandle(HandleType type) const; //---------------------------------------------------------------------------- /// @brief Collect a reactor handle. diff --git a/engine/src/flutter/impeller/renderer/backend/gles/render_pass_gles.cc b/engine/src/flutter/impeller/renderer/backend/gles/render_pass_gles.cc index cbfd26f503..e0d1d4a7ff 100644 --- a/engine/src/flutter/impeller/renderer/backend/gles/render_pass_gles.cc +++ b/engine/src/flutter/impeller/renderer/backend/gles/render_pass_gles.cc @@ -211,7 +211,6 @@ void RenderPassGLES::ResetGLState(const ProcTableGLES& gl) { } #endif // IMPELLER_DEBUG - GLuint fbo = GL_NONE; TextureGLES& color_gles = TextureGLES::Cast(*pass_data.color_attachment); const bool is_default_fbo = color_gles.IsWrapped(); @@ -222,14 +221,21 @@ void RenderPassGLES::ResetGLState(const ProcTableGLES& gl) { } } else { // Create and bind an offscreen FBO. - GLuint cached_fbo = color_gles.GetCachedFBO(); - if (cached_fbo != GL_NONE) { - fbo = cached_fbo; - gl.BindFramebuffer(GL_FRAMEBUFFER, fbo); + if (!color_gles.GetCachedFBO().IsDead()) { + auto fbo = reactor.GetGLHandle(color_gles.GetCachedFBO()); + if (!fbo.has_value()) { + return false; + } + gl.BindFramebuffer(GL_FRAMEBUFFER, fbo.value()); } else { - gl.GenFramebuffers(1u, &fbo); - color_gles.SetCachedFBO(fbo); - gl.BindFramebuffer(GL_FRAMEBUFFER, fbo); + HandleGLES cached_fbo = + reactor.CreateUntrackedHandle(HandleType::kFrameBuffer); + color_gles.SetCachedFBO(cached_fbo); + auto fbo = reactor.GetGLHandle(cached_fbo); + if (!fbo.has_value()) { + return false; + } + gl.BindFramebuffer(GL_FRAMEBUFFER, fbo.value()); if (!color_gles.SetAsFramebufferAttachment( GL_FRAMEBUFFER, TextureGLES::AttachmentType::kColor0)) { 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 39fe43e6b3..7f6e910335 100644 --- a/engine/src/flutter/impeller/renderer/backend/gles/texture_gles.cc +++ b/engine/src/flutter/impeller/renderer/backend/gles/texture_gles.cc @@ -219,8 +219,8 @@ TextureGLES::TextureGLES(std::shared_ptr reactor, // |Texture| TextureGLES::~TextureGLES() { reactor_->CollectHandle(handle_); - if (cached_fbo_ != GL_NONE) { - reactor_->GetProcTable().DeleteFramebuffers(1, &cached_fbo_); + if (!cached_fbo_.IsDead()) { + reactor_->CollectHandle(cached_fbo_); } } @@ -659,11 +659,11 @@ std::optional TextureGLES::GetSyncFence() const { return fence_; } -void TextureGLES::SetCachedFBO(GLuint fbo) { +void TextureGLES::SetCachedFBO(HandleGLES fbo) { cached_fbo_ = fbo; } -GLuint TextureGLES::GetCachedFBO() const { +const HandleGLES& TextureGLES::GetCachedFBO() const { return cached_fbo_; } 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 5d7a18507a..c1feb9030e 100644 --- a/engine/src/flutter/impeller/renderer/backend/gles/texture_gles.h +++ b/engine/src/flutter/impeller/renderer/backend/gles/texture_gles.h @@ -136,10 +136,10 @@ class TextureGLES final : public Texture, /// /// The color0 texture used by the 2D renderer will use this texture /// object to store the associated FBO the first time it is used. - void SetCachedFBO(GLuint fbo); + void SetCachedFBO(HandleGLES fbo); - /// Retrieve the cached FBO object, or GL_NONE if there is no object. - GLuint GetCachedFBO() const; + /// Retrieve the cached FBO object, or a dead handle if there is no object. + const HandleGLES& GetCachedFBO() const; // Visible for testing. std::optional GetSyncFence() const; @@ -155,7 +155,7 @@ class TextureGLES final : public Texture, mutable std::bitset<6> slices_initialized_ = 0; const bool is_wrapped_; const std::optional wrapped_fbo_; - GLuint cached_fbo_ = GL_NONE; + HandleGLES cached_fbo_ = HandleGLES::DeadHandle(); bool is_valid_ = false; TextureGLES(std::shared_ptr reactor,