From b3274bd826252fbe5cc0bc7be0a50f38c3cbcf4b Mon Sep 17 00:00:00 2001 From: Brandon DeRosier Date: Sun, 22 May 2022 14:08:32 -0700 Subject: [PATCH 1/2] Don't discard GLES buffer for each copy; respect destination offset --- .../renderer/backend/gles/allocator_gles.cc | 9 ++++- .../backend/gles/buffer_bindings_gles.cc | 4 +- .../backend/gles/device_buffer_gles.cc | 37 ++++++++++++------- .../backend/gles/device_buffer_gles.h | 15 ++++++-- 4 files changed, 44 insertions(+), 21 deletions(-) diff --git a/impeller/renderer/backend/gles/allocator_gles.cc b/impeller/renderer/backend/gles/allocator_gles.cc index 52b3ede4a447c..84c328f7af002 100644 --- a/impeller/renderer/backend/gles/allocator_gles.cc +++ b/impeller/renderer/backend/gles/allocator_gles.cc @@ -3,7 +3,9 @@ // found in the LICENSE file. #include "impeller/renderer/backend/gles/allocator_gles.h" +#include +#include "impeller/base/allocation.h" #include "impeller/base/config.h" #include "impeller/renderer/backend/gles/device_buffer_gles.h" #include "impeller/renderer/backend/gles/texture_gles.h" @@ -24,7 +26,12 @@ bool AllocatorGLES::IsValid() const { // |Allocator| std::shared_ptr AllocatorGLES::CreateBuffer(StorageMode mode, size_t length) { - return std::make_shared(reactor_, length, mode); + auto buffer = std::make_shared(); + if (!buffer->Truncate(length)) { + return nullptr; + } + return std::make_shared(reactor_, std::move(buffer), length, + mode); } // |Allocator| diff --git a/impeller/renderer/backend/gles/buffer_bindings_gles.cc b/impeller/renderer/backend/gles/buffer_bindings_gles.cc index 8ff893b82038e..33cd7e627a62b 100644 --- a/impeller/renderer/backend/gles/buffer_bindings_gles.cc +++ b/impeller/renderer/backend/gles/buffer_bindings_gles.cc @@ -192,8 +192,8 @@ bool BufferBindingsGLES::BindUniformBuffer(const ProcTableGLES& gl, return false; } const auto& device_buffer_gles = DeviceBufferGLES::Cast(*device_buffer); - const uint8_t* buffer_ptr = device_buffer_gles.GetBufferData()->GetMapping() + - buffer.resource.range.offset; + const uint8_t* buffer_ptr = + device_buffer_gles.GetBufferData() + buffer.resource.range.offset; if (metadata->members.empty()) { VALIDATION_LOG << "Uniform buffer had no members. This is currently " diff --git a/impeller/renderer/backend/gles/device_buffer_gles.cc b/impeller/renderer/backend/gles/device_buffer_gles.cc index 6ee452972cce6..d16bf23d405de 100644 --- a/impeller/renderer/backend/gles/device_buffer_gles.cc +++ b/impeller/renderer/backend/gles/device_buffer_gles.cc @@ -4,6 +4,9 @@ #include "impeller/renderer/backend/gles/device_buffer_gles.h" +#include +#include + #include "flutter/fml/trace_event.h" #include "impeller/base/allocation.h" #include "impeller/base/config.h" @@ -12,12 +15,14 @@ namespace impeller { DeviceBufferGLES::DeviceBufferGLES(ReactorGLES::Ref reactor, + std::shared_ptr buffer, size_t size, StorageMode mode) : DeviceBuffer(size, mode), reactor_(std::move(reactor)), handle_(reactor_ ? reactor_->CreateHandle(HandleType::kBuffer) - : HandleGLES::DeadHandle()) {} + : HandleGLES::DeadHandle()), + buffer_(std::move(buffer)) {} // |DeviceBuffer| DeviceBufferGLES::~DeviceBufferGLES() { @@ -35,21 +40,23 @@ bool DeviceBufferGLES::CopyHostBuffer(const uint8_t* source, return false; } - if (offset + source_range.length > size_) { - // Out of bounds of this buffer. + if (!reactor_) { return false; } - if (!reactor_) { + if (offset + source_range.length > size_) { + // Out of bounds of this buffer. return false; } - auto mapping = - CreateMappingWithCopy(source + source_range.offset, source_range.length); - if (!mapping) { + if (offset + source_range.length > buffer_->GetLength()) { return false; } - data_ = std::move(mapping); + + std::memmove(buffer_->GetBuffer() + offset, source + source_range.offset, + source_range.length); + dirty_ = true; + return true; } @@ -78,11 +85,13 @@ bool DeviceBufferGLES::BindAndUploadDataIfNecessary(BindingType type) const { gl.BindBuffer(target_type, buffer.value()); - if (!uploaded_) { + if (dirty_) { TRACE_EVENT0("impeller", "BufferData"); - gl.BufferData(target_type, data_->GetSize(), data_->GetMapping(), + gl.BufferData(target_type, buffer_->GetLength(), buffer_->GetBuffer(), GL_STATIC_DRAW); - uploaded_ = true; + dirty_ = false; + has_uploaded_ = true; + reactor_->SetDebugLabel(handle_, label_); } @@ -92,7 +101,7 @@ bool DeviceBufferGLES::BindAndUploadDataIfNecessary(BindingType type) const { // |DeviceBuffer| bool DeviceBufferGLES::SetLabel(const std::string& label) { label_ = label; - if (uploaded_) { + if (has_uploaded_) { reactor_->SetDebugLabel(handle_, label_); } return true; @@ -105,7 +114,7 @@ bool DeviceBufferGLES::SetLabel(const std::string& label, Range range) { return SetLabel(label); } -std::shared_ptr DeviceBufferGLES::GetBufferData() const { - return data_; +const uint8_t* DeviceBufferGLES::GetBufferData() const { + return buffer_->GetBuffer(); } } // namespace impeller diff --git a/impeller/renderer/backend/gles/device_buffer_gles.h b/impeller/renderer/backend/gles/device_buffer_gles.h index bd2a48f98b340..ecc996a840f97 100644 --- a/impeller/renderer/backend/gles/device_buffer_gles.h +++ b/impeller/renderer/backend/gles/device_buffer_gles.h @@ -4,7 +4,10 @@ #pragma once +#include + #include "flutter/fml/macros.h" +#include "impeller/base/allocation.h" #include "impeller/base/backend_cast.h" #include "impeller/renderer/backend/gles/reactor_gles.h" #include "impeller/renderer/device_buffer.h" @@ -15,12 +18,15 @@ class DeviceBufferGLES final : public DeviceBuffer, public BackendCast { public: - DeviceBufferGLES(ReactorGLES::Ref reactor, size_t size, StorageMode mode); + DeviceBufferGLES(ReactorGLES::Ref reactor, + std::shared_ptr buffer, + size_t size, + StorageMode mode); // |DeviceBuffer| ~DeviceBufferGLES() override; - std::shared_ptr GetBufferData() const; + const uint8_t* GetBufferData() const; enum class BindingType { kArrayBuffer, @@ -33,8 +39,9 @@ class DeviceBufferGLES final ReactorGLES::Ref reactor_; HandleGLES handle_; std::string label_; - mutable std::shared_ptr data_; - mutable bool uploaded_ = false; + mutable std::shared_ptr buffer_; + mutable bool dirty_ = false; + mutable bool has_uploaded_ = false; // |DeviceBuffer| bool CopyHostBuffer(const uint8_t* source, From 6c71fad3b170b4402862e4b032a58907e7fc0a9f Mon Sep 17 00:00:00 2001 From: Brandon DeRosier Date: Mon, 23 May 2022 12:25:39 -0700 Subject: [PATCH 2/2] Address comments --- .../renderer/backend/gles/allocator_gles.cc | 9 ++++--- .../backend/gles/device_buffer_gles.cc | 25 +++++++++---------- .../backend/gles/device_buffer_gles.h | 6 ++--- 3 files changed, 20 insertions(+), 20 deletions(-) diff --git a/impeller/renderer/backend/gles/allocator_gles.cc b/impeller/renderer/backend/gles/allocator_gles.cc index 84c328f7af002..5f08256ab0031 100644 --- a/impeller/renderer/backend/gles/allocator_gles.cc +++ b/impeller/renderer/backend/gles/allocator_gles.cc @@ -3,6 +3,7 @@ // found in the LICENSE file. #include "impeller/renderer/backend/gles/allocator_gles.h" + #include #include "impeller/base/allocation.h" @@ -26,12 +27,12 @@ bool AllocatorGLES::IsValid() const { // |Allocator| std::shared_ptr AllocatorGLES::CreateBuffer(StorageMode mode, size_t length) { - auto buffer = std::make_shared(); - if (!buffer->Truncate(length)) { + auto backing_store = std::make_shared(); + if (!backing_store->Truncate(length)) { return nullptr; } - return std::make_shared(reactor_, std::move(buffer), length, - mode); + return std::make_shared(reactor_, std::move(backing_store), + length, mode); } // |Allocator| diff --git a/impeller/renderer/backend/gles/device_buffer_gles.cc b/impeller/renderer/backend/gles/device_buffer_gles.cc index d16bf23d405de..702e34a97b85c 100644 --- a/impeller/renderer/backend/gles/device_buffer_gles.cc +++ b/impeller/renderer/backend/gles/device_buffer_gles.cc @@ -15,14 +15,14 @@ namespace impeller { DeviceBufferGLES::DeviceBufferGLES(ReactorGLES::Ref reactor, - std::shared_ptr buffer, + std::shared_ptr backing_store, size_t size, StorageMode mode) : DeviceBuffer(size, mode), reactor_(std::move(reactor)), handle_(reactor_ ? reactor_->CreateHandle(HandleType::kBuffer) : HandleGLES::DeadHandle()), - buffer_(std::move(buffer)) {} + backing_store_(std::move(backing_store)) {} // |DeviceBuffer| DeviceBufferGLES::~DeviceBufferGLES() { @@ -49,13 +49,13 @@ bool DeviceBufferGLES::CopyHostBuffer(const uint8_t* source, return false; } - if (offset + source_range.length > buffer_->GetLength()) { + if (offset + source_range.length > backing_store_->GetLength()) { return false; } - std::memmove(buffer_->GetBuffer() + offset, source + source_range.offset, - source_range.length); - dirty_ = true; + std::memmove(backing_store_->GetBuffer() + offset, + source + source_range.offset, source_range.length); + ++generation_; return true; } @@ -85,12 +85,11 @@ bool DeviceBufferGLES::BindAndUploadDataIfNecessary(BindingType type) const { gl.BindBuffer(target_type, buffer.value()); - if (dirty_) { + if (upload_generation_ != generation_) { TRACE_EVENT0("impeller", "BufferData"); - gl.BufferData(target_type, buffer_->GetLength(), buffer_->GetBuffer(), - GL_STATIC_DRAW); - dirty_ = false; - has_uploaded_ = true; + gl.BufferData(target_type, backing_store_->GetLength(), + backing_store_->GetBuffer(), GL_STATIC_DRAW); + upload_generation_ = generation_; reactor_->SetDebugLabel(handle_, label_); } @@ -101,7 +100,7 @@ bool DeviceBufferGLES::BindAndUploadDataIfNecessary(BindingType type) const { // |DeviceBuffer| bool DeviceBufferGLES::SetLabel(const std::string& label) { label_ = label; - if (has_uploaded_) { + if (upload_generation_ > 0) { reactor_->SetDebugLabel(handle_, label_); } return true; @@ -115,6 +114,6 @@ bool DeviceBufferGLES::SetLabel(const std::string& label, Range range) { } const uint8_t* DeviceBufferGLES::GetBufferData() const { - return buffer_->GetBuffer(); + return backing_store_->GetBuffer(); } } // namespace impeller diff --git a/impeller/renderer/backend/gles/device_buffer_gles.h b/impeller/renderer/backend/gles/device_buffer_gles.h index ecc996a840f97..8305167845c01 100644 --- a/impeller/renderer/backend/gles/device_buffer_gles.h +++ b/impeller/renderer/backend/gles/device_buffer_gles.h @@ -39,9 +39,9 @@ class DeviceBufferGLES final ReactorGLES::Ref reactor_; HandleGLES handle_; std::string label_; - mutable std::shared_ptr buffer_; - mutable bool dirty_ = false; - mutable bool has_uploaded_ = false; + mutable std::shared_ptr backing_store_; + mutable uint32_t generation_ = 0; + mutable uint32_t upload_generation_ = 0; // |DeviceBuffer| bool CopyHostBuffer(const uint8_t* source,