From 16f5821cc4adf05b63ff1edc0d3839c3e5d35c49 Mon Sep 17 00:00:00 2001 From: gaaclarke <30870216+gaaclarke@users.noreply.github.com> Date: Wed, 4 Dec 2024 10:51:10 -0800 Subject: [PATCH] Added the ability to make untracked opengles handles (migrated textures) (flutter/engine#56927) issue: https://github.com/flutter/flutter/issues/159745 This tweaks ReactorGLES so that one can easily opt out of it's mutex/hash map for cases where we can statically reason about the safety of doing so. The goal here was to make migration of existing code really easy to do. It may be in the future that everything is an untracked handle? We can move there in baby steps. Potential follow up PRs: - Move `Pipeline` to use untracked handles - Move `DeviceBufferGLES` to use untracked handles - Add a new method to synchronously delete untracked handles - Start storing handles to be deleted in its own vector, so handles_ doesn't need to be used for deleting untracked handles ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I added new tests to check the change I am making or feature I am adding, or the PR is [test-exempt]. See [testing the engine] for instructions on writing and running engine tests. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I signed the [CLA]. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. [Contributor Guide]: https://github.com/flutter/flutter/blob/master/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/master/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/master/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/master/docs/contributing/Style-guide-for-Flutter-repo.md [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style [testing the engine]: https://github.com/flutter/engine/blob/main/docs/testing/Testing-the-engine.md [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/master/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/master/docs/contributing/Chat.md --- .../renderer/backend/gles/handle_gles.h | 1 + .../renderer/backend/gles/reactor_gles.cc | 33 ++++++++++++++++--- .../renderer/backend/gles/reactor_gles.h | 12 +++++++ .../renderer/backend/gles/test/mock_gles.cc | 13 ++++++++ .../renderer/backend/gles/test/mock_gles.h | 4 +++ .../backend/gles/test/reactor_unittests.cc | 30 +++++++++++++++++ .../renderer/backend/gles/texture_gles.cc | 4 +-- 7 files changed, 90 insertions(+), 7 deletions(-) diff --git a/engine/src/flutter/impeller/renderer/backend/gles/handle_gles.h b/engine/src/flutter/impeller/renderer/backend/gles/handle_gles.h index 80220f5e3a..808918fe8e 100644 --- a/engine/src/flutter/impeller/renderer/backend/gles/handle_gles.h +++ b/engine/src/flutter/impeller/renderer/backend/gles/handle_gles.h @@ -79,6 +79,7 @@ class HandleGLES { HandleType type_ = HandleType::kUnknown; std::optional name_; std::size_t hash_; + std::optional untracked_id_; friend class ReactorGLES; 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 a86b2119ab..4f869fe050 100644 --- a/engine/src/flutter/impeller/renderer/backend/gles/reactor_gles.cc +++ b/engine/src/flutter/impeller/renderer/backend/gles/reactor_gles.cc @@ -126,6 +126,10 @@ const ProcTableGLES& ReactorGLES::GetProcTable() const { std::optional ReactorGLES::GetHandle( const HandleGLES& handle) const { + if (handle.untracked_id_.has_value()) { + return ReactorGLES::GLStorage{.integer = handle.untracked_id_.value()}; + } + ReaderLock handles_lock(handles_mutex_); if (auto found = handles_.find(handle); found != handles_.end()) { if (found->second.pending_collection) { @@ -187,6 +191,7 @@ bool ReactorGLES::RegisterCleanupCallback(const HandleGLES& handle, if (handle.IsDead()) { return false; } + FML_DCHECK(!handle.untracked_id_.has_value()); WriterLock handles_lock(handles_mutex_); if (auto found = handles_.find(handle); found != handles_.end()) { found->second.callback = fml::ScopedCleanupClosure(callback); @@ -195,6 +200,17 @@ bool ReactorGLES::RegisterCleanupCallback(const HandleGLES& handle, return false; } +HandleGLES ReactorGLES::CreateUntrackedHandle(HandleType type) { + FML_DCHECK(CanReactOnCurrentThread()); + auto new_handle = HandleGLES::Create(type); + std::optional gl_handle = + CreateGLHandle(GetProcTable(), type); + if (gl_handle.has_value()) { + new_handle.untracked_id_ = gl_handle.value().integer; + } + return new_handle; +} + HandleGLES ReactorGLES::CreateHandle(HandleType type, GLuint external_handle) { if (type == HandleType::kUnknown) { return HandleGLES::DeadHandle(); @@ -217,12 +233,19 @@ HandleGLES ReactorGLES::CreateHandle(HandleType type, GLuint external_handle) { } void ReactorGLES::CollectHandle(HandleGLES handle) { - WriterLock handles_lock(handles_mutex_); - if (auto found = handles_.find(handle); found != handles_.end()) { - if (!found->second.pending_collection) { - handles_to_collect_count_ += 1; + if (handle.untracked_id_.has_value()) { + LiveHandle live_handle(GLStorage{.integer = handle.untracked_id_.value()}); + live_handle.pending_collection = true; + WriterLock handles_lock(handles_mutex_); + handles_[handle] = std::move(live_handle); + } else { + WriterLock handles_lock(handles_mutex_); + if (auto found = handles_.find(handle); found != handles_.end()) { + if (!found->second.pending_collection) { + handles_to_collect_count_ += 1; + } + found->second.pending_collection = true; } - found->second.pending_collection = true; } } 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 09dc092097..49191932e0 100644 --- a/engine/src/flutter/impeller/renderer/backend/gles/reactor_gles.h +++ b/engine/src/flutter/impeller/renderer/backend/gles/reactor_gles.h @@ -174,6 +174,16 @@ class ReactorGLES { /// HandleGLES CreateHandle(HandleType type, GLuint external_handle = GL_NONE); + /// @brief Create a handle that is not managed by `ReactorGLES`. + /// @details This behaves just like `CreateHandle` but it doesn't add the + /// handle to ReactorGLES::handles_ and the creation is executed + /// synchronously, so it must be called from a proper thread. The benefit of + /// this is that it avoid synchronization and hash table lookups when + /// creating/accessing the handle. + /// @param type The type of handle to create. + /// @return The reactor handle. + HandleGLES CreateUntrackedHandle(HandleType type); + //---------------------------------------------------------------------------- /// @brief Collect a reactor handle. /// @@ -255,8 +265,10 @@ class ReactorGLES { union { GLuint handle; GLsync sync; + uint64_t integer; }; }; + static_assert(sizeof(GLStorage) == sizeof(uint64_t)); struct LiveHandle { std::optional name; 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 e94e74dc28..2296351ed9 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 @@ -171,6 +171,17 @@ void mockUniform1fv(GLint location, GLsizei count, const GLfloat* value) { RecordGLCall("glUniform1fv"); } +void mockGenTextures(GLsizei n, GLuint* textures) { + RecordGLCall("glGenTextures"); + if (auto mock_gles = g_mock_gles.lock()) { + std::optional next_texture; + std::swap(mock_gles->next_texture_, next_texture); + if (next_texture.has_value()) { + textures[0] = next_texture.value(); + } + } +} + static_assert(CheckSameSignature::value); @@ -217,6 +228,8 @@ const ProcTableGLES::Resolver kMockResolverGLES = [](const char* name) { return reinterpret_cast(mockGetQueryObjectuivEXT); } else if (strcmp(name, "glUniform1fv") == 0) { return reinterpret_cast(mockUniform1fv); + } else if (strcmp(name, "glGenTextures") == 0) { + return reinterpret_cast(mockGenTextures); } else { return reinterpret_cast(&doNothing); } diff --git a/engine/src/flutter/impeller/renderer/backend/gles/test/mock_gles.h b/engine/src/flutter/impeller/renderer/backend/gles/test/mock_gles.h index 0ce2bcb6e3..aa5a66c296 100644 --- a/engine/src/flutter/impeller/renderer/backend/gles/test/mock_gles.h +++ b/engine/src/flutter/impeller/renderer/backend/gles/test/mock_gles.h @@ -50,8 +50,11 @@ class MockGLES final { ~MockGLES(); + void SetNextTexture(uint64_t next_texture) { next_texture_ = next_texture; } + private: friend void RecordGLCall(const char* name); + friend void mockGenTextures(GLsizei n, GLuint* textures); explicit MockGLES(ProcTableGLES::Resolver resolver = kMockResolverGLES); @@ -59,6 +62,7 @@ class MockGLES final { ProcTableGLES proc_table_; std::vector captured_calls_; + std::optional next_texture_; MockGLES(const MockGLES&) = delete; 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 c43b9b1b96..e83ffc2aaa 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 @@ -2,6 +2,7 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +#include #include #include "flutter/fml/synchronization/waitable_event.h" #include "flutter/testing/testing.h" // IWYU pragma: keep @@ -61,6 +62,35 @@ TEST(ReactorGLES, DeletesHandlesDuringShutdown) { calls.end()); } +TEST(ReactorGLES, UntrackedHandle) { + std::shared_ptr 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); + + mock_gles->SetNextTexture(1234u); + HandleGLES handle = reactor->CreateUntrackedHandle(HandleType::kTexture); + EXPECT_FALSE(handle.IsDead()); + std::optional glint = reactor->GetGLHandle(handle); + EXPECT_TRUE(glint.has_value()); + if (glint.has_value()) { + EXPECT_EQ(1234u, *glint); + } + mock_gles->GetCapturedCalls(); + reactor->CollectHandle(handle); + std::vector calls = mock_gles->GetCapturedCalls(); + EXPECT_TRUE(std::find(calls.begin(), calls.end(), "glDeleteTextures") == + calls.end()); + // Without an operation nothing is cleaned up. + EXPECT_TRUE(reactor->AddOperation([&](const ReactorGLES&) {})); + EXPECT_TRUE(reactor->React()); + calls = mock_gles->GetCapturedCalls(); + EXPECT_FALSE(std::find(calls.begin(), calls.end(), "glDeleteTextures") == + calls.end()); +} + TEST(ReactorGLES, PerThreadOperationQueues) { auto mock_gles = MockGLES::Init(); ProcTableGLES::Resolver resolver = kMockResolverGLES; 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 748fb5ee25..0b96430638 100644 --- a/engine/src/flutter/impeller/renderer/backend/gles/texture_gles.cc +++ b/engine/src/flutter/impeller/renderer/backend/gles/texture_gles.cc @@ -193,7 +193,7 @@ TextureGLES::TextureGLES(std::shared_ptr reactor, type_(GetTextureTypeFromDescriptor(GetTextureDescriptor())), handle_(external_handle.has_value() ? external_handle.value() - : reactor_->CreateHandle(ToHandleType(type_))), + : reactor_->CreateUntrackedHandle(ToHandleType(type_))), is_wrapped_(fbo.has_value() || external_handle.has_value()), wrapped_fbo_(fbo) { // Ensure the texture descriptor itself is valid. @@ -407,7 +407,7 @@ void TextureGLES::InitializeContentsIfNecessary() const { } const auto& gl = reactor_->GetProcTable(); - auto handle = reactor_->GetGLHandle(handle_); + std::optional handle = reactor_->GetGLHandle(handle_); if (!handle.has_value()) { VALIDATION_LOG << "Could not initialize the contents of texture."; return;