Added the ability to make untracked opengles handles (migrated textures) (#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].
<!-- Links -->
[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
diff --git a/impeller/renderer/backend/gles/handle_gles.h b/impeller/renderer/backend/gles/handle_gles.h
index 80220f5..808918f 100644
--- a/impeller/renderer/backend/gles/handle_gles.h
+++ b/impeller/renderer/backend/gles/handle_gles.h
@@ -79,6 +79,7 @@
HandleType type_ = HandleType::kUnknown;
std::optional<UniqueID> name_;
std::size_t hash_;
+ std::optional<uint64_t> untracked_id_;
friend class ReactorGLES;
diff --git a/impeller/renderer/backend/gles/reactor_gles.cc b/impeller/renderer/backend/gles/reactor_gles.cc
index a86b211..4f869fe 100644
--- a/impeller/renderer/backend/gles/reactor_gles.cc
+++ b/impeller/renderer/backend/gles/reactor_gles.cc
@@ -126,6 +126,10 @@
std::optional<ReactorGLES::GLStorage> 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 @@
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 @@
return false;
}
+HandleGLES ReactorGLES::CreateUntrackedHandle(HandleType type) {
+ FML_DCHECK(CanReactOnCurrentThread());
+ auto new_handle = HandleGLES::Create(type);
+ std::optional<ReactorGLES::GLStorage> 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 @@
}
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/impeller/renderer/backend/gles/reactor_gles.h b/impeller/renderer/backend/gles/reactor_gles.h
index 09dc092..4919193 100644
--- a/impeller/renderer/backend/gles/reactor_gles.h
+++ b/impeller/renderer/backend/gles/reactor_gles.h
@@ -174,6 +174,16 @@
///
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 @@
union {
GLuint handle;
GLsync sync;
+ uint64_t integer;
};
};
+ static_assert(sizeof(GLStorage) == sizeof(uint64_t));
struct LiveHandle {
std::optional<GLStorage> name;
diff --git a/impeller/renderer/backend/gles/test/mock_gles.cc b/impeller/renderer/backend/gles/test/mock_gles.cc
index e94e74d..2296351 100644
--- a/impeller/renderer/backend/gles/test/mock_gles.cc
+++ b/impeller/renderer/backend/gles/test/mock_gles.cc
@@ -171,6 +171,17 @@
RecordGLCall("glUniform1fv");
}
+void mockGenTextures(GLsizei n, GLuint* textures) {
+ RecordGLCall("glGenTextures");
+ if (auto mock_gles = g_mock_gles.lock()) {
+ std::optional<uint64_t> next_texture;
+ std::swap(mock_gles->next_texture_, next_texture);
+ if (next_texture.has_value()) {
+ textures[0] = next_texture.value();
+ }
+ }
+}
+
static_assert(CheckSameSignature<decltype(mockUniform1fv), //
decltype(glUniform1fv)>::value);
@@ -217,6 +228,8 @@
return reinterpret_cast<void*>(mockGetQueryObjectuivEXT);
} else if (strcmp(name, "glUniform1fv") == 0) {
return reinterpret_cast<void*>(mockUniform1fv);
+ } else if (strcmp(name, "glGenTextures") == 0) {
+ return reinterpret_cast<void*>(mockGenTextures);
} else {
return reinterpret_cast<void*>(&doNothing);
}
diff --git a/impeller/renderer/backend/gles/test/mock_gles.h b/impeller/renderer/backend/gles/test/mock_gles.h
index 0ce2bcb..aa5a66c 100644
--- a/impeller/renderer/backend/gles/test/mock_gles.h
+++ b/impeller/renderer/backend/gles/test/mock_gles.h
@@ -50,8 +50,11 @@
~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 @@
ProcTableGLES proc_table_;
std::vector<std::string> captured_calls_;
+ std::optional<uint64_t> next_texture_;
MockGLES(const MockGLES&) = delete;
diff --git a/impeller/renderer/backend/gles/test/reactor_unittests.cc b/impeller/renderer/backend/gles/test/reactor_unittests.cc
index c43b9b1..e83ffc2 100644
--- a/impeller/renderer/backend/gles/test/reactor_unittests.cc
+++ b/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 <algorithm>
#include <memory>
#include "flutter/fml/synchronization/waitable_event.h"
#include "flutter/testing/testing.h" // IWYU pragma: keep
@@ -61,6 +62,35 @@
calls.end());
}
+TEST(ReactorGLES, UntrackedHandle) {
+ std::shared_ptr<MockGLES> 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);
+
+ mock_gles->SetNextTexture(1234u);
+ HandleGLES handle = reactor->CreateUntrackedHandle(HandleType::kTexture);
+ EXPECT_FALSE(handle.IsDead());
+ std::optional<GLuint> 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<std::string> 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/impeller/renderer/backend/gles/texture_gles.cc b/impeller/renderer/backend/gles/texture_gles.cc
index 748fb5e..0b96430 100644
--- a/impeller/renderer/backend/gles/texture_gles.cc
+++ b/impeller/renderer/backend/gles/texture_gles.cc
@@ -193,7 +193,7 @@
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 @@
}
const auto& gl = reactor_->GetProcTable();
- auto handle = reactor_->GetGLHandle(handle_);
+ std::optional<GLuint> handle = reactor_->GetGLHandle(handle_);
if (!handle.has_value()) {
VALIDATION_LOG << "Could not initialize the contents of texture.";
return;
