Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Impeller] Delete any remaining GL objects during destruction of the ReactorGLES #56361

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
110 changes: 60 additions & 50 deletions impeller/renderer/backend/gles/reactor_gles.cc
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,55 @@

namespace impeller {

static std::optional<GLuint> 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<ProcTableGLES> gl)
: proc_table_(std::move(gl)) {
if (!proc_table_ || !proc_table_->IsValid()) {
Expand All @@ -23,7 +72,17 @@ ReactorGLES::ReactorGLES(std::unique_ptr<ProcTableGLES> 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_;
Expand Down Expand Up @@ -99,55 +158,6 @@ bool ReactorGLES::RegisterCleanupCallback(const HandleGLES& handle,
return false;
}

static std::optional<GLuint> 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();
Expand Down
6 changes: 6 additions & 0 deletions impeller/renderer/backend/gles/test/mock_gles.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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<decltype(mockDeleteQueriesEXT), //
decltype(glDeleteQueriesEXT)>::value);

Expand Down Expand Up @@ -198,6 +202,8 @@ const ProcTableGLES::Resolver kMockResolverGLES = [](const char* name) {
return reinterpret_cast<void*>(&mockEndQueryEXT);
} else if (strcmp(name, "glDeleteQueriesEXT") == 0) {
return reinterpret_cast<void*>(&mockDeleteQueriesEXT);
} else if (strcmp(name, "glDeleteTextures") == 0) {
return reinterpret_cast<void*>(&mockDeleteTextures);
} else if (strcmp(name, "glGetQueryObjectui64vEXT") == 0) {
return reinterpret_cast<void*>(mockGetQueryObjectui64vEXT);
} else if (strcmp(name, "glGetQueryObjectuivEXT") == 0) {
Expand Down
17 changes: 17 additions & 0 deletions impeller/renderer/backend/gles/test/reactor_unittests.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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<ProcTableGLES>(resolver);
auto worker = std::make_shared<TestWorker>();
auto reactor = std::make_shared<ReactorGLES>(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