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].

<!-- 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

engine/src/flutter/impeller/renderer/backend/gles/handle_gles.h

@@ -79,6 +79,7 @@ class HandleGLES {
   HandleType type_ = HandleType::kUnknown;
   std::optional<UniqueID> name_;
   std::size_t hash_;
+  std::optional<uint64_t> untracked_id_;
 
   friend class ReactorGLES;
 

engine/src/flutter/impeller/renderer/backend/gles/reactor_gles.cc

@@ -126,6 +126,10 @@ const ProcTableGLES& ReactorGLES::GetProcTable() const {
 
 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 @@ 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<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,6 +233,12 @@ HandleGLES ReactorGLES::CreateHandle(HandleType type, GLuint external_handle) {
 }
 
 void ReactorGLES::CollectHandle(HandleGLES handle) {
+  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) {
@@ -225,6 +247,7 @@ void ReactorGLES::CollectHandle(HandleGLES handle) {
       found->second.pending_collection = true;
     }
   }
+}
 
 bool ReactorGLES::React() {
   if (!CanReactOnCurrentThread()) {

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<GLStorage> name;

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<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 @@ const ProcTableGLES::Resolver kMockResolverGLES = [](const char* name) {
     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);
   }

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<std::string> captured_calls_;
+  std::optional<uint64_t> next_texture_;
 
   MockGLES(const MockGLES&) = delete;
 

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 <algorithm>
 #include <memory>
 #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<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;

engine/src/flutter/impeller/renderer/backend/gles/texture_gles.cc

@@ -193,7 +193,7 @@ TextureGLES::TextureGLES(std::shared_ptr<ReactorGLES> 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<GLuint> handle = reactor_->GetGLHandle(handle_);
   if (!handle.has_value()) {
     VALIDATION_LOG << "Could not initialize the contents of texture.";
     return;