From eae69f89fe85dbbb68c45fb0e0c898b013184105 Mon Sep 17 00:00:00 2001 From: Jason Simmons Date: Tue, 5 Nov 2024 12:56:13 -0800 Subject: [PATCH] [Impeller] Delete any remaining GL objects during destruction of the ReactorGLES (flutter/engine#56361) At shutdown time the ReactorGLES may still be holding handles of GL objects. These objects should be cleaned up when the reactor is deleted. This leak can be seen by running DlGoldenTest.ShimmerTest, which takes a series of screenshots. Each screenshot creates an AiksContext. Without this change, the textures in the AiksContext's ReactorGLES will be leaked after the AiksContext is destroyed. --- .../renderer/backend/gles/reactor_gles.cc | 110 ++++++++++-------- .../renderer/backend/gles/test/mock_gles.cc | 6 + .../backend/gles/test/reactor_unittests.cc | 17 +++ 3 files changed, 83 insertions(+), 50 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 dca10dff3d..4ccbb47891 100644 --- a/engine/src/flutter/impeller/renderer/backend/gles/reactor_gles.cc +++ b/engine/src/flutter/impeller/renderer/backend/gles/reactor_gles.cc @@ -13,6 +13,55 @@ namespace impeller { +static std::optional CreateGLHandle(const ProcTableGLES& gl, + HandleType type) { + GLuint handle = GL_NONE; + switch (type) { + case HandleType::kUnknown: + return std::nullopt; + case HandleType::kTexture: + gl.GenTextures(1u, &handle); + return handle; + case HandleType::kBuffer: + gl.GenBuffers(1u, &handle); + return handle; + case HandleType::kProgram: + return gl.CreateProgram(); + case HandleType::kRenderBuffer: + gl.GenRenderbuffers(1u, &handle); + return handle; + case HandleType::kFrameBuffer: + gl.GenFramebuffers(1u, &handle); + return handle; + } + return std::nullopt; +} + +static bool CollectGLHandle(const ProcTableGLES& gl, + HandleType type, + GLuint handle) { + switch (type) { + case HandleType::kUnknown: + return false; + case HandleType::kTexture: + gl.DeleteTextures(1u, &handle); + return true; + case HandleType::kBuffer: + gl.DeleteBuffers(1u, &handle); + return true; + case HandleType::kProgram: + gl.DeleteProgram(handle); + return true; + case HandleType::kRenderBuffer: + gl.DeleteRenderbuffers(1u, &handle); + return true; + case HandleType::kFrameBuffer: + gl.DeleteFramebuffers(1u, &handle); + return true; + } + return false; +} + ReactorGLES::ReactorGLES(std::unique_ptr gl) : proc_table_(std::move(gl)) { if (!proc_table_ || !proc_table_->IsValid()) { @@ -23,7 +72,17 @@ ReactorGLES::ReactorGLES(std::unique_ptr gl) is_valid_ = true; } -ReactorGLES::~ReactorGLES() = default; +ReactorGLES::~ReactorGLES() { + if (CanReactOnCurrentThread()) { + for (auto& handle : handles_) { + if (handle.second.name.has_value()) { + CollectGLHandle(*proc_table_, handle.first.type, + handle.second.name.value()); + } + } + proc_table_->Flush(); + } +} bool ReactorGLES::IsValid() const { return is_valid_; @@ -99,55 +158,6 @@ bool ReactorGLES::RegisterCleanupCallback(const HandleGLES& handle, return false; } -static std::optional CreateGLHandle(const ProcTableGLES& gl, - HandleType type) { - GLuint handle = GL_NONE; - switch (type) { - case HandleType::kUnknown: - return std::nullopt; - case HandleType::kTexture: - gl.GenTextures(1u, &handle); - return handle; - case HandleType::kBuffer: - gl.GenBuffers(1u, &handle); - return handle; - case HandleType::kProgram: - return gl.CreateProgram(); - case HandleType::kRenderBuffer: - gl.GenRenderbuffers(1u, &handle); - return handle; - case HandleType::kFrameBuffer: - gl.GenFramebuffers(1u, &handle); - return handle; - } - return std::nullopt; -} - -static bool CollectGLHandle(const ProcTableGLES& gl, - HandleType type, - GLuint handle) { - switch (type) { - case HandleType::kUnknown: - return false; - case HandleType::kTexture: - gl.DeleteTextures(1u, &handle); - return true; - case HandleType::kBuffer: - gl.DeleteBuffers(1u, &handle); - return true; - case HandleType::kProgram: - gl.DeleteProgram(handle); - return true; - case HandleType::kRenderBuffer: - gl.DeleteRenderbuffers(1u, &handle); - return true; - case HandleType::kFrameBuffer: - gl.DeleteFramebuffers(1u, &handle); - return true; - } - return false; -} - HandleGLES ReactorGLES::CreateHandle(HandleType type, GLuint external_handle) { if (type == HandleType::kUnknown) { return HandleGLES::DeadHandle(); diff --git a/engine/src/flutter/impeller/renderer/backend/gles/test/mock_gles.cc b/engine/src/flutter/impeller/renderer/backend/gles/test/mock_gles.cc index d000dad400..7a8399cede 100644 --- a/engine/src/flutter/impeller/renderer/backend/gles/test/mock_gles.cc +++ b/engine/src/flutter/impeller/renderer/backend/gles/test/mock_gles.cc @@ -160,6 +160,10 @@ void mockDeleteQueriesEXT(GLsizei size, const GLuint* queries) { RecordGLCall("glDeleteQueriesEXT"); } +void mockDeleteTextures(GLsizei size, const GLuint* queries) { + RecordGLCall("glDeleteTextures"); +} + static_assert(CheckSameSignature::value); @@ -198,6 +202,8 @@ const ProcTableGLES::Resolver kMockResolverGLES = [](const char* name) { return reinterpret_cast(&mockEndQueryEXT); } else if (strcmp(name, "glDeleteQueriesEXT") == 0) { return reinterpret_cast(&mockDeleteQueriesEXT); + } else if (strcmp(name, "glDeleteTextures") == 0) { + return reinterpret_cast(&mockDeleteTextures); } else if (strcmp(name, "glGetQueryObjectui64vEXT") == 0) { return reinterpret_cast(mockGetQueryObjectui64vEXT); } else if (strcmp(name, "glGetQueryObjectuivEXT") == 0) { diff --git a/engine/src/flutter/impeller/renderer/backend/gles/test/reactor_unittests.cc b/engine/src/flutter/impeller/renderer/backend/gles/test/reactor_unittests.cc index 97f15c4a5d..efa7b576be 100644 --- a/engine/src/flutter/impeller/renderer/backend/gles/test/reactor_unittests.cc +++ b/engine/src/flutter/impeller/renderer/backend/gles/test/reactor_unittests.cc @@ -43,5 +43,22 @@ TEST(ReactorGLES, CanAttachCleanupCallbacksToHandles) { EXPECT_EQ(value, 1); } +TEST(ReactorGLES, DeletesHandlesDuringShutdown) { + auto mock_gles = MockGLES::Init(); + ProcTableGLES::Resolver resolver = kMockResolverGLES; + auto proc_table = std::make_unique(resolver); + auto worker = std::make_shared(); + auto reactor = std::make_shared(std::move(proc_table)); + reactor->AddWorker(worker); + + reactor->CreateHandle(HandleType::kTexture, 123); + + reactor.reset(); + + auto calls = mock_gles->GetCapturedCalls(); + EXPECT_TRUE(std::find(calls.begin(), calls.end(), "glDeleteTextures") != + calls.end()); +} + } // namespace testing } // namespace impeller