From e1d3866629f1e7c35521f1607f85c6a9dde41138 Mon Sep 17 00:00:00 2001 From: Jason Simmons Date: Mon, 4 Nov 2024 13:10:04 -0800 Subject: [PATCH 1/3] [Impeller] Delete any remaining GL objects during destruction of the ReactorGLES 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 | 106 +++++++++--------- .../renderer/backend/gles/test/mock_gles.cc | 6 + .../backend/gles/test/reactor_unittests.cc | 15 +++ 3 files changed, 77 insertions(+), 50 deletions(-) diff --git a/impeller/renderer/backend/gles/reactor_gles.cc b/impeller/renderer/backend/gles/reactor_gles.cc index dca10dff3d88c..1e7ef8a5088c2 100644 --- a/impeller/renderer/backend/gles/reactor_gles.cc +++ b/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,13 @@ ReactorGLES::ReactorGLES(std::unique_ptr gl) is_valid_ = true; } -ReactorGLES::~ReactorGLES() = default; +ReactorGLES::~ReactorGLES() { + for (auto& handle : handles_) { + CollectGLHandle(*proc_table_, handle.first.type, + handle.second.name.value()); + } + proc_table_->Flush(); +} bool ReactorGLES::IsValid() const { return is_valid_; @@ -99,55 +154,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/impeller/renderer/backend/gles/test/mock_gles.cc b/impeller/renderer/backend/gles/test/mock_gles.cc index d000dad400947..7a8399cede36c 100644 --- a/impeller/renderer/backend/gles/test/mock_gles.cc +++ b/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/impeller/renderer/backend/gles/test/reactor_unittests.cc b/impeller/renderer/backend/gles/test/reactor_unittests.cc index 97f15c4a5df5f..7d5db7c34a4d6 100644 --- a/impeller/renderer/backend/gles/test/reactor_unittests.cc +++ b/impeller/renderer/backend/gles/test/reactor_unittests.cc @@ -43,5 +43,20 @@ 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 reactor = std::make_shared(std::move(proc_table)); + + 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 From 77297dbfbaed88d5cda381cdc62d1c0dc3aa4a64 Mon Sep 17 00:00:00 2001 From: Jason Simmons Date: Mon, 4 Nov 2024 13:39:20 -0800 Subject: [PATCH 2/3] CanReactOnCurrentThread check --- impeller/renderer/backend/gles/reactor_gles.cc | 10 ++++++---- .../renderer/backend/gles/test/reactor_unittests.cc | 2 ++ 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/impeller/renderer/backend/gles/reactor_gles.cc b/impeller/renderer/backend/gles/reactor_gles.cc index 1e7ef8a5088c2..2854f3276d45b 100644 --- a/impeller/renderer/backend/gles/reactor_gles.cc +++ b/impeller/renderer/backend/gles/reactor_gles.cc @@ -73,11 +73,13 @@ ReactorGLES::ReactorGLES(std::unique_ptr gl) } ReactorGLES::~ReactorGLES() { - for (auto& handle : handles_) { - CollectGLHandle(*proc_table_, handle.first.type, - handle.second.name.value()); + if (CanReactOnCurrentThread()) { + for (auto& handle : handles_) { + CollectGLHandle(*proc_table_, handle.first.type, + handle.second.name.value()); + } + proc_table_->Flush(); } - proc_table_->Flush(); } bool ReactorGLES::IsValid() const { diff --git a/impeller/renderer/backend/gles/test/reactor_unittests.cc b/impeller/renderer/backend/gles/test/reactor_unittests.cc index 7d5db7c34a4d6..efa7b576be82b 100644 --- a/impeller/renderer/backend/gles/test/reactor_unittests.cc +++ b/impeller/renderer/backend/gles/test/reactor_unittests.cc @@ -47,7 +47,9 @@ 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); From 601fff36a946849a4999cb016cd4201b599067d1 Mon Sep 17 00:00:00 2001 From: Jason Simmons Date: Tue, 5 Nov 2024 10:36:53 -0800 Subject: [PATCH 3/3] handle optional check --- impeller/renderer/backend/gles/reactor_gles.cc | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/impeller/renderer/backend/gles/reactor_gles.cc b/impeller/renderer/backend/gles/reactor_gles.cc index 2854f3276d45b..4ccbb47891b38 100644 --- a/impeller/renderer/backend/gles/reactor_gles.cc +++ b/impeller/renderer/backend/gles/reactor_gles.cc @@ -75,8 +75,10 @@ ReactorGLES::ReactorGLES(std::unique_ptr gl) ReactorGLES::~ReactorGLES() { if (CanReactOnCurrentThread()) { for (auto& handle : handles_) { - CollectGLHandle(*proc_table_, handle.first.type, - handle.second.name.value()); + if (handle.second.name.has_value()) { + CollectGLHandle(*proc_table_, handle.first.type, + handle.second.name.value()); + } } proc_table_->Flush(); }