From 8c736f414423246d915893a3d236c6f46c61f3bf Mon Sep 17 00:00:00 2001 From: gaaclarke <30870216+gaaclarke@users.noreply.github.com> Date: Wed, 4 Dec 2024 18:06:44 -0800 Subject: [PATCH] Implements the naming of untracked gles handles (flutter/engine#56948) issue: https://github.com/flutter/flutter/issues/159745 https://github.com/flutter/engine/pull/56927 introduced untracked handles, but naming them didn't work. This adds a test to make sure they work. I kept naming thread-safe since it isn't happening often anyways. [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style --- .../renderer/backend/gles/proc_table_gles.cc | 2 +- .../renderer/backend/gles/proc_table_gles.h | 2 +- .../renderer/backend/gles/reactor_gles.cc | 14 ++++++++++--- .../renderer/backend/gles/test/mock_gles.cc | 20 +++++++++++++++++-- .../backend/gles/test/reactor_unittests.cc | 17 ++++++++++++++++ 5 files changed, 48 insertions(+), 7 deletions(-) diff --git a/engine/src/flutter/impeller/renderer/backend/gles/proc_table_gles.cc b/engine/src/flutter/impeller/renderer/backend/gles/proc_table_gles.cc index cf2482d6fb..3f0d518879 100644 --- a/engine/src/flutter/impeller/renderer/backend/gles/proc_table_gles.cc +++ b/engine/src/flutter/impeller/renderer/backend/gles/proc_table_gles.cc @@ -360,7 +360,7 @@ static bool ResourceIsLive(const ProcTableGLES& gl, bool ProcTableGLES::SetDebugLabel(DebugResourceType type, GLint name, - const std::string& label) const { + std::string_view label) const { if (debug_label_max_length_ <= 0) { return true; } diff --git a/engine/src/flutter/impeller/renderer/backend/gles/proc_table_gles.h b/engine/src/flutter/impeller/renderer/backend/gles/proc_table_gles.h index f17c42949d..746d26c668 100644 --- a/engine/src/flutter/impeller/renderer/backend/gles/proc_table_gles.h +++ b/engine/src/flutter/impeller/renderer/backend/gles/proc_table_gles.h @@ -311,7 +311,7 @@ class ProcTableGLES { bool SetDebugLabel(DebugResourceType type, GLint name, - const std::string& label) const; + std::string_view label) const; void PushDebugGroup(const std::string& string) const; 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 4f869fe050..726bcd2d37 100644 --- a/engine/src/flutter/impeller/renderer/backend/gles/reactor_gles.cc +++ b/engine/src/flutter/impeller/renderer/backend/gles/reactor_gles.cc @@ -386,15 +386,23 @@ void ReactorGLES::SetupDebugGroups() { void ReactorGLES::SetDebugLabel(const HandleGLES& handle, std::string_view label) { + FML_DCHECK(handle.GetType() != HandleType::kFence); if (!can_set_debug_labels_) { return; } if (handle.IsDead()) { return; } - WriterLock handles_lock(handles_mutex_); - if (auto found = handles_.find(handle); found != handles_.end()) { - found->second.pending_debug_label = label; + if (handle.untracked_id_.has_value()) { + FML_DCHECK(CanReactOnCurrentThread()); + const auto& gl = GetProcTable(); + gl.SetDebugLabel(ToDebugResourceType(handle.GetType()), + handle.untracked_id_.value(), label); + } else { + WriterLock handles_lock(handles_mutex_); + if (auto found = handles_.find(handle); found != handles_.end()) { + found->second.pending_debug_label = label; + } } } 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 2296351ed9..9b1965ab2b 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 @@ -83,6 +83,9 @@ void mockGetIntegerv(GLenum name, int* value) { case GL_MAX_COMBINED_TEXTURE_IMAGE_UNITS: *value = 8; break; + case GL_MAX_LABEL_LENGTH_KHR: + *value = 64; + break; default: *value = 0; break; @@ -170,6 +173,8 @@ static_assert(CheckSameSignature::value); void mockGenTextures(GLsizei n, GLuint* textures) { RecordGLCall("glGenTextures"); @@ -182,8 +187,17 @@ void mockGenTextures(GLsizei n, GLuint* textures) { } } -static_assert(CheckSameSignature::value); +static_assert(CheckSameSignature::value); + +void mockObjectLabelKHR(GLenum identifier, + GLuint name, + GLsizei length, + const GLchar* label) { + RecordGLCall("glObjectLabelKHR"); +} +static_assert(CheckSameSignature::value); std::shared_ptr MockGLES::Init( const std::optional>& extensions, @@ -230,6 +244,8 @@ const ProcTableGLES::Resolver kMockResolverGLES = [](const char* name) { return reinterpret_cast(mockUniform1fv); } else if (strcmp(name, "glGenTextures") == 0) { return reinterpret_cast(mockGenTextures); + } else if (strcmp(name, "glObjectLabelKHR") == 0) { + return reinterpret_cast(mockObjectLabelKHR); } else { return reinterpret_cast(&doNothing); } 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 e83ffc2aaa..e2c49fb27c 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 @@ -91,6 +91,23 @@ TEST(ReactorGLES, UntrackedHandle) { calls.end()); } +TEST(ReactorGLES, NameUntrackedHandle) { + 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); + mock_gles->GetCapturedCalls(); + reactor->SetDebugLabel(handle, "hello, joe!"); + std::vector calls = mock_gles->GetCapturedCalls(); + EXPECT_TRUE(std::find(calls.begin(), calls.end(), "glObjectLabelKHR") != + calls.end()); +} + TEST(ReactorGLES, PerThreadOperationQueues) { auto mock_gles = MockGLES::Init(); ProcTableGLES::Resolver resolver = kMockResolverGLES;