From 41f40fda0f81a54c1ee19c0b3f48e759f2a7fc81 Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Fri, 6 Dec 2024 10:08:26 -0800 Subject: [PATCH 1/6] moved device buffer to an untracked handle --- impeller/renderer/backend/gles/BUILD.gn | 1 + .../backend/gles/device_buffer_gles.cc | 23 ++++++--- .../backend/gles/device_buffer_gles.h | 3 +- .../gles/device_buffer_gles_unittests.cc | 49 +++++++++++++++++++ .../renderer/backend/gles/test/mock_gles.cc | 9 ++++ .../renderer/backend/gles/test/mock_gles.h | 2 + 6 files changed, 79 insertions(+), 8 deletions(-) create mode 100644 impeller/renderer/backend/gles/device_buffer_gles_unittests.cc diff --git a/impeller/renderer/backend/gles/BUILD.gn b/impeller/renderer/backend/gles/BUILD.gn index 4c8598a449eb4..a28309f5fa828 100644 --- a/impeller/renderer/backend/gles/BUILD.gn +++ b/impeller/renderer/backend/gles/BUILD.gn @@ -15,6 +15,7 @@ impeller_component("gles_unittests") { testonly = true sources = [ "buffer_bindings_gles_unittests.cc", + "device_buffer_gles_unittests.cc", "test/capabilities_unittests.cc", "test/formats_gles_unittests.cc", "test/gpu_tracer_gles_unittests.cc", diff --git a/impeller/renderer/backend/gles/device_buffer_gles.cc b/impeller/renderer/backend/gles/device_buffer_gles.cc index b072a8ebc0a63..979d1c6357b2d 100644 --- a/impeller/renderer/backend/gles/device_buffer_gles.cc +++ b/impeller/renderer/backend/gles/device_buffer_gles.cc @@ -17,14 +17,12 @@ DeviceBufferGLES::DeviceBufferGLES(DeviceBufferDescriptor desc, std::shared_ptr backing_store) : DeviceBuffer(desc), reactor_(std::move(reactor)), - handle_(reactor_ ? reactor_->CreateHandle(HandleType::kBuffer) - : HandleGLES::DeadHandle()), backing_store_(std::move(backing_store)) {} // |DeviceBuffer| DeviceBufferGLES::~DeviceBufferGLES() { - if (!handle_.IsDead()) { - reactor_->CollectHandle(handle_); + if (handle_.has_value() && !handle_->IsDead()) { + reactor_->CollectHandle(*handle_); } } @@ -57,7 +55,11 @@ bool DeviceBufferGLES::OnCopyHostBuffer(const uint8_t* source, } std::optional DeviceBufferGLES::GetHandle() const { - return reactor_->GetGLHandle(handle_); + if (handle_.has_value()) { + return reactor_->GetGLHandle(*handle_); + } else { + return std::nullopt; + } } void DeviceBufferGLES::Flush(std::optional range) const { @@ -90,7 +92,11 @@ bool DeviceBufferGLES::BindAndUploadDataIfNecessary(BindingType type) const { return false; } - auto buffer = reactor_->GetGLHandle(handle_); + if (!handle_.has_value()) { + handle_ = reactor_->CreateUntrackedHandle(HandleType::kBuffer); + } + + auto buffer = reactor_->GetGLHandle(*handle_); if (!buffer.has_value()) { return false; } @@ -118,7 +124,10 @@ bool DeviceBufferGLES::BindAndUploadDataIfNecessary(BindingType type) const { // |DeviceBuffer| bool DeviceBufferGLES::SetLabel(std::string_view label) { #ifdef IMPELLER_DEBUG - reactor_->SetDebugLabel(handle_, label); + FML_CHECK(handle_.has_value()); + if (handle_.has_value()) { + reactor_->SetDebugLabel(*handle_, label); + } #endif // IMPELLER_DEBUG return true; } diff --git a/impeller/renderer/backend/gles/device_buffer_gles.h b/impeller/renderer/backend/gles/device_buffer_gles.h index 8255995f9d83a..87f8435ccd426 100644 --- a/impeller/renderer/backend/gles/device_buffer_gles.h +++ b/impeller/renderer/backend/gles/device_buffer_gles.h @@ -45,7 +45,8 @@ class DeviceBufferGLES final private: std::shared_ptr reactor_; - HandleGLES handle_; + // Mutable for lazy evaluation. + mutable std::optional handle_; mutable std::shared_ptr backing_store_; mutable std::optional dirty_range_ = std::nullopt; mutable bool initialized_ = false; diff --git a/impeller/renderer/backend/gles/device_buffer_gles_unittests.cc b/impeller/renderer/backend/gles/device_buffer_gles_unittests.cc new file mode 100644 index 0000000000000..1fe910faab0d9 --- /dev/null +++ b/impeller/renderer/backend/gles/device_buffer_gles_unittests.cc @@ -0,0 +1,49 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "flutter/testing/testing.h" // IWYU pragma: keep +#include "gtest/gtest.h" +#include "impeller/renderer/backend/gles/device_buffer_gles.h" +#include "impeller/renderer/backend/gles/test/mock_gles.h" + +namespace impeller { +namespace testing { + +using ::testing::_; + +namespace { +class TestWorker : public ReactorGLES::Worker { + public: + bool CanReactorReactOnCurrentThreadNow( + const ReactorGLES& reactor) const override { + return true; + } +}; +} // namespace + +TEST(DeviceBufferGLESTest, BindUniformData) { + auto mock_gles_impl = std::make_unique(); + + EXPECT_CALL(*mock_gles_impl, GenBuffers(1, _)).Times(1); + + std::shared_ptr mock_gled = + MockGLES::Init(std::move(mock_gles_impl)); + 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); + + std::shared_ptr backing_store = std::make_shared(); + ASSERT_TRUE(backing_store->Truncate(Bytes{sizeof(float)})); + DeviceBufferGLES device_buffer(DeviceBufferDescriptor{.size = sizeof(float)}, + reactor, backing_store); + EXPECT_FALSE(device_buffer.GetHandle().has_value()); + EXPECT_TRUE(device_buffer.BindAndUploadDataIfNecessary( + DeviceBufferGLES::BindingType::kUniformBuffer)); + EXPECT_TRUE(device_buffer.GetHandle().has_value()); +} + +} // namespace testing +} // namespace impeller diff --git a/impeller/renderer/backend/gles/test/mock_gles.cc b/impeller/renderer/backend/gles/test/mock_gles.cc index 5b890d989f37a..c62ff4fe71483 100644 --- a/impeller/renderer/backend/gles/test/mock_gles.cc +++ b/impeller/renderer/backend/gles/test/mock_gles.cc @@ -174,6 +174,13 @@ void mockGenTextures(GLsizei n, GLuint* textures) { CallMockMethod(&IMockGLESImpl::GenTextures, n, textures); } +static_assert(CheckSameSignature::value); + +void mockGenBuffers(GLsizei n, GLuint* buffers) { + CallMockMethod(&IMockGLESImpl::GenBuffers, n, buffers); +} + static_assert(CheckSameSignature::value); @@ -248,6 +255,8 @@ const ProcTableGLES::Resolver kMockResolverGLES = [](const char* name) { return reinterpret_cast(mockGenTextures); } else if (strcmp(name, "glObjectLabelKHR") == 0) { return reinterpret_cast(mockObjectLabelKHR); + } else if (strcmp(name, "glGenBuffers") == 0) { + return reinterpret_cast(mockGenBuffers); } else { return reinterpret_cast(&doNothing); } diff --git a/impeller/renderer/backend/gles/test/mock_gles.h b/impeller/renderer/backend/gles/test/mock_gles.h index 35b041b13d5e2..681b894ac410b 100644 --- a/impeller/renderer/backend/gles/test/mock_gles.h +++ b/impeller/renderer/backend/gles/test/mock_gles.h @@ -35,6 +35,7 @@ class IMockGLESImpl { GLenum target, GLuint64* result) {} virtual void DeleteQueriesEXT(GLsizei size, const GLuint* queries) {} + virtual void GenBuffers(GLsizei n, GLuint* buffers) {} }; class MockGLESImpl : public IMockGLESImpl { @@ -68,6 +69,7 @@ class MockGLESImpl : public IMockGLESImpl { DeleteQueriesEXT, (GLsizei size, const GLuint* queries), (override)); + MOCK_METHOD(void, GenBuffers, (GLsizei n, GLuint* buffers), (override)); }; /// @brief Provides a mocked version of the |ProcTableGLES| class. From b03264e40512205369e89bb55964efd975230da4 Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Fri, 6 Dec 2024 14:01:18 -0800 Subject: [PATCH 2/6] licenses --- ci/licenses_golden/excluded_files | 1 + 1 file changed, 1 insertion(+) diff --git a/ci/licenses_golden/excluded_files b/ci/licenses_golden/excluded_files index dc786d51f0486..45e6771274782 100644 --- a/ci/licenses_golden/excluded_files +++ b/ci/licenses_golden/excluded_files @@ -183,6 +183,7 @@ ../../../flutter/impeller/golden_tests/README.md ../../../flutter/impeller/playground ../../../flutter/impeller/renderer/backend/gles/buffer_bindings_gles_unittests.cc +../../../flutter/impeller/renderer/backend/gles/device_buffer_gles_unittests.cc ../../../flutter/impeller/renderer/backend/gles/test ../../../flutter/impeller/renderer/backend/gles/unique_handle_gles_unittests.cc ../../../flutter/impeller/renderer/backend/metal/allocator_mtl_unittests.mm From d61b61f0751ae5b6223dd6501410d5d2d45f9b98 Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Mon, 9 Dec 2024 11:19:10 -0800 Subject: [PATCH 3/6] made sure that setting the label happens after we get the handle too --- impeller/renderer/backend/gles/device_buffer_gles.cc | 10 ++++------ impeller/renderer/backend/gles/device_buffer_gles.h | 1 + 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/impeller/renderer/backend/gles/device_buffer_gles.cc b/impeller/renderer/backend/gles/device_buffer_gles.cc index 979d1c6357b2d..32ec5858bf8c2 100644 --- a/impeller/renderer/backend/gles/device_buffer_gles.cc +++ b/impeller/renderer/backend/gles/device_buffer_gles.cc @@ -94,6 +94,9 @@ bool DeviceBufferGLES::BindAndUploadDataIfNecessary(BindingType type) const { if (!handle_.has_value()) { handle_ = reactor_->CreateUntrackedHandle(HandleType::kBuffer); + if (handle_.has_value() && label_.has_value()) { + reactor_->SetDebugLabel(*handle_, *label_); + } } auto buffer = reactor_->GetGLHandle(*handle_); @@ -123,12 +126,7 @@ bool DeviceBufferGLES::BindAndUploadDataIfNecessary(BindingType type) const { // |DeviceBuffer| bool DeviceBufferGLES::SetLabel(std::string_view label) { -#ifdef IMPELLER_DEBUG - FML_CHECK(handle_.has_value()); - if (handle_.has_value()) { - reactor_->SetDebugLabel(*handle_, label); - } -#endif // IMPELLER_DEBUG + label_ = label; return true; } diff --git a/impeller/renderer/backend/gles/device_buffer_gles.h b/impeller/renderer/backend/gles/device_buffer_gles.h index 87f8435ccd426..17ffb153ea6b2 100644 --- a/impeller/renderer/backend/gles/device_buffer_gles.h +++ b/impeller/renderer/backend/gles/device_buffer_gles.h @@ -46,6 +46,7 @@ class DeviceBufferGLES final private: std::shared_ptr reactor_; // Mutable for lazy evaluation. + std::optional label_; mutable std::optional handle_; mutable std::shared_ptr backing_store_; mutable std::optional dirty_range_ = std::nullopt; From 1c2a482fbfc1752113cc02e5fa979310ad5690aa Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Mon, 9 Dec 2024 11:25:36 -0800 Subject: [PATCH 4/6] oops messed up comment --- impeller/renderer/backend/gles/device_buffer_gles.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/impeller/renderer/backend/gles/device_buffer_gles.h b/impeller/renderer/backend/gles/device_buffer_gles.h index 17ffb153ea6b2..f080d4e3e0ffe 100644 --- a/impeller/renderer/backend/gles/device_buffer_gles.h +++ b/impeller/renderer/backend/gles/device_buffer_gles.h @@ -45,8 +45,8 @@ class DeviceBufferGLES final private: std::shared_ptr reactor_; - // Mutable for lazy evaluation. std::optional label_; + // Mutable for lazy evaluation. mutable std::optional handle_; mutable std::shared_ptr backing_store_; mutable std::optional dirty_range_ = std::nullopt; From 3bc76b2c16d5576510dd05d4c7ac565f72d1859c Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Mon, 9 Dec 2024 11:38:57 -0800 Subject: [PATCH 5/6] brought back #if IMPELLER_DEBUG --- impeller/renderer/backend/gles/device_buffer_gles.cc | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/impeller/renderer/backend/gles/device_buffer_gles.cc b/impeller/renderer/backend/gles/device_buffer_gles.cc index 32ec5858bf8c2..78394a8608ff5 100644 --- a/impeller/renderer/backend/gles/device_buffer_gles.cc +++ b/impeller/renderer/backend/gles/device_buffer_gles.cc @@ -94,9 +94,11 @@ bool DeviceBufferGLES::BindAndUploadDataIfNecessary(BindingType type) const { if (!handle_.has_value()) { handle_ = reactor_->CreateUntrackedHandle(HandleType::kBuffer); +#ifdef IMPELLER_DEBUG if (handle_.has_value() && label_.has_value()) { reactor_->SetDebugLabel(*handle_, *label_); } +#endif } auto buffer = reactor_->GetGLHandle(*handle_); @@ -127,6 +129,11 @@ bool DeviceBufferGLES::BindAndUploadDataIfNecessary(BindingType type) const { // |DeviceBuffer| bool DeviceBufferGLES::SetLabel(std::string_view label) { label_ = label; +#ifdef IMPELLER_DEBUG + if (handle_.has_value()) { + reactor_->SetDebugLabel(*handle_, label); + } +#endif // IMPELLER_DEBUG return true; } From c0add79f5adceefffbfe8e0f676ddc6f2aa82825 Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Mon, 9 Dec 2024 11:40:13 -0800 Subject: [PATCH 6/6] only store the label if we are impeller debug --- impeller/renderer/backend/gles/device_buffer_gles.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/impeller/renderer/backend/gles/device_buffer_gles.cc b/impeller/renderer/backend/gles/device_buffer_gles.cc index 78394a8608ff5..0a7478d8f7e95 100644 --- a/impeller/renderer/backend/gles/device_buffer_gles.cc +++ b/impeller/renderer/backend/gles/device_buffer_gles.cc @@ -128,8 +128,8 @@ bool DeviceBufferGLES::BindAndUploadDataIfNecessary(BindingType type) const { // |DeviceBuffer| bool DeviceBufferGLES::SetLabel(std::string_view label) { - label_ = label; #ifdef IMPELLER_DEBUG + label_ = label; if (handle_.has_value()) { reactor_->SetDebugLabel(*handle_, label); }