From 48881a9cf1abf064b1a8adf34101e8ca89035622 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Thu, 1 Jun 2023 08:39:46 -0700 Subject: [PATCH 1/4] avoid VBB for text contents --- impeller/core/host_buffer.cc | 13 +++ impeller/core/host_buffer.h | 15 ++++ impeller/entity/contents/text_contents.cc | 98 +++++++++++------------ 3 files changed, 76 insertions(+), 50 deletions(-) diff --git a/impeller/core/host_buffer.cc b/impeller/core/host_buffer.cc index 825cd86250ac5..13164dd4b988f 100644 --- a/impeller/core/host_buffer.cc +++ b/impeller/core/host_buffer.cc @@ -56,6 +56,19 @@ BufferView HostBuffer::Emplace(const void* buffer, size_t length) { return BufferView{shared_from_this(), GetBuffer(), Range{old_length, length}}; } +BufferView HostBuffer::EmplaceCallback(size_t length, + size_t align, + EmplaceProc cb) { + auto old_length = GetLength(); + if (!Truncate(old_length + length)) { + return {}; + } + generation_++; + cb(GetBuffer() + old_length); + + return BufferView{shared_from_this(), GetBuffer(), Range{old_length, length}}; +} + std::shared_ptr HostBuffer::GetDeviceBuffer( Allocator& allocator) const { if (generation_ == device_buffer_generation_) { diff --git a/impeller/core/host_buffer.h b/impeller/core/host_buffer.h index e23c3814fd6c7..981aea9f09fc4 100644 --- a/impeller/core/host_buffer.h +++ b/impeller/core/host_buffer.h @@ -95,6 +95,21 @@ class HostBuffer final : public std::enable_shared_from_this, size_t length, size_t align); + using EmplaceProc = std::function; + + //---------------------------------------------------------------------------- + /// @brief Emplace non-uniform data (like contiguous vertices) onto the + /// host buffer directly. The buffer ptr is guaranteed to have at + /// least enough storage for the length of data. + /// + /// @param[in] cb A callback that will be passed a ptr to the + /// underlying + /// host buffer. + /// + /// @return The buffer view. + /// + BufferView EmplaceCallback(size_t length, size_t align, EmplaceProc cb); + private: mutable std::shared_ptr device_buffer_; mutable size_t device_buffer_generation_ = 0u; diff --git a/impeller/entity/contents/text_contents.cc b/impeller/entity/contents/text_contents.cc index c4cd636ac2998..a756a142967ff 100644 --- a/impeller/entity/contents/text_contents.cc +++ b/impeller/entity/contents/text_contents.cc @@ -135,60 +135,58 @@ static bool CommonRender(const ContentContext& renderer, // interpolated vertex information is also used in the fragment shader to // sample from the glyph atlas. - constexpr std::array unit_points = {Point{0, 0}, Point{1, 0}, + constexpr std::array unit_points = {Point{0, 0}, Point{1, 0}, + Point{0, 1}, Point{1, 0}, Point{0, 1}, Point{1, 1}}; - constexpr std::array indices = {0, 1, 2, 1, 2, 3}; - - VertexBufferBuilder vertex_builder; - - size_t count = 0; - for (const auto& run : frame.GetRuns()) { - count += run.GetGlyphPositions().size(); - } - - vertex_builder.Reserve(count * 4); - vertex_builder.ReserveIndices(count * 6); - - uint32_t index_offset = 0u; - for (auto i = 0u; i < count; i++) { - for (const auto& index : indices) { - vertex_builder.AppendIndex(index + index_offset); - } - index_offset += 4; - } + auto& host_buffer = pass.GetTransientsBuffer(); + size_t vertex_count = 0; for (const auto& run : frame.GetRuns()) { - const Font& font = run.GetFont(); - - for (const auto& glyph_position : run.GetGlyphPositions()) { - FontGlyphPair font_glyph_pair{font, glyph_position.glyph}; - auto atlas_glyph_bounds = atlas->FindFontGlyphBounds(font_glyph_pair); - if (!atlas_glyph_bounds.has_value()) { - VALIDATION_LOG << "Could not find glyph position in the atlas."; - return false; - } - Vector4 atlas_glyph_bounds_vec = Vector4( - atlas_glyph_bounds->origin.x, atlas_glyph_bounds->origin.y, - atlas_glyph_bounds->size.width, atlas_glyph_bounds->size.height); - Vector4 glyph_bounds_vec = - Vector4(glyph_position.glyph.bounds.origin.x, - glyph_position.glyph.bounds.origin.y, - glyph_position.glyph.bounds.size.width, - glyph_position.glyph.bounds.size.height); - - for (const auto& point : unit_points) { - vertex_builder.AppendVertex(VS::PerVertexData{ - .atlas_glyph_bounds = atlas_glyph_bounds_vec, - .glyph_bounds = glyph_bounds_vec, - .unit_position = point, - .glyph_position = glyph_position.position, - }); - } - } + vertex_count += run.GetGlyphPositions().size(); } - auto vertex_buffer = - vertex_builder.CreateVertexBuffer(pass.GetTransientsBuffer()); - cmd.BindVertices(vertex_buffer); + vertex_count *= 6; + + auto buffer_view = host_buffer.EmplaceCallback( + vertex_count * sizeof(VS::PerVertexData), alignof(VS::PerVertexData), + [&](uint8_t* contents) { + VS::PerVertexData vtx; + size_t vertex_offset = 0; + for (const auto& run : frame.GetRuns()) { + const Font& font = run.GetFont(); + for (const auto& glyph_position : run.GetGlyphPositions()) { + FontGlyphPair font_glyph_pair{font, glyph_position.glyph}; + auto maybe_atlas_glyph_bounds = + atlas->FindFontGlyphBounds(font_glyph_pair); + if (!maybe_atlas_glyph_bounds.has_value()) { + VALIDATION_LOG << "Could not find glyph position in the atlas."; + continue; + } + auto atlas_glyph_bounds = maybe_atlas_glyph_bounds.value(); + vtx.atlas_glyph_bounds = Vector4( + atlas_glyph_bounds.origin.x, atlas_glyph_bounds.origin.y, + atlas_glyph_bounds.size.width, atlas_glyph_bounds.size.height); + vtx.glyph_bounds = Vector4(glyph_position.glyph.bounds.origin.x, + glyph_position.glyph.bounds.origin.y, + glyph_position.glyph.bounds.size.width, + glyph_position.glyph.bounds.size.height); + vtx.glyph_position = glyph_position.position; + + for (const auto& point : unit_points) { + vtx.unit_position = point; + ::memcpy(contents + vertex_offset, &vtx, + sizeof(VS::PerVertexData)); + vertex_offset += sizeof(VS::PerVertexData); + } + } + } + }); + + cmd.BindVertices({ + .vertex_buffer = buffer_view, + .index_buffer = {}, + .vertex_count = vertex_count, + .index_type = IndexType::kNone, + }); return pass.AddCommand(cmd); } From 2dbe8dff82dac4a90e4456732d4ab409dd9fb84c Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Thu, 1 Jun 2023 09:10:58 -0700 Subject: [PATCH 2/4] tidy --- impeller/core/host_buffer.cc | 2 +- impeller/core/host_buffer.h | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/impeller/core/host_buffer.cc b/impeller/core/host_buffer.cc index 13164dd4b988f..51ebecd6e6da8 100644 --- a/impeller/core/host_buffer.cc +++ b/impeller/core/host_buffer.cc @@ -58,7 +58,7 @@ BufferView HostBuffer::Emplace(const void* buffer, size_t length) { BufferView HostBuffer::EmplaceCallback(size_t length, size_t align, - EmplaceProc cb) { + const EmplaceProc& cb) { auto old_length = GetLength(); if (!Truncate(old_length + length)) { return {}; diff --git a/impeller/core/host_buffer.h b/impeller/core/host_buffer.h index 981aea9f09fc4..d5b2a40d73c2d 100644 --- a/impeller/core/host_buffer.h +++ b/impeller/core/host_buffer.h @@ -108,7 +108,9 @@ class HostBuffer final : public std::enable_shared_from_this, /// /// @return The buffer view. /// - BufferView EmplaceCallback(size_t length, size_t align, EmplaceProc cb); + BufferView EmplaceCallback(size_t length, + size_t align, + const EmplaceProc& cb); private: mutable std::shared_ptr device_buffer_; From 7248d3b3e1108322e3f608c004f460ebf751b447 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Thu, 1 Jun 2023 10:36:31 -0700 Subject: [PATCH 3/4] fix alignment --- impeller/core/host_buffer.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/impeller/core/host_buffer.h b/impeller/core/host_buffer.h index d5b2a40d73c2d..3cfbb4e44bfdf 100644 --- a/impeller/core/host_buffer.h +++ b/impeller/core/host_buffer.h @@ -103,8 +103,7 @@ class HostBuffer final : public std::enable_shared_from_this, /// least enough storage for the length of data. /// /// @param[in] cb A callback that will be passed a ptr to the - /// underlying - /// host buffer. + /// underlying host buffer. /// /// @return The buffer view. /// From b95e6443a9e0291fd27ea09b92e934bb17b4d698 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Thu, 1 Jun 2023 14:46:12 -0700 Subject: [PATCH 4/4] cleanup --- impeller/core/host_buffer.cc | 9 ++++++--- impeller/core/host_buffer.h | 12 ++++++------ impeller/entity/contents/text_contents.cc | 2 +- 3 files changed, 13 insertions(+), 10 deletions(-) diff --git a/impeller/core/host_buffer.cc b/impeller/core/host_buffer.cc index 51ebecd6e6da8..c4f6af7754363 100644 --- a/impeller/core/host_buffer.cc +++ b/impeller/core/host_buffer.cc @@ -56,9 +56,12 @@ BufferView HostBuffer::Emplace(const void* buffer, size_t length) { return BufferView{shared_from_this(), GetBuffer(), Range{old_length, length}}; } -BufferView HostBuffer::EmplaceCallback(size_t length, - size_t align, - const EmplaceProc& cb) { +BufferView HostBuffer::Emplace(size_t length, + size_t align, + const EmplaceProc& cb) { + if (!cb) { + return {}; + } auto old_length = GetLength(); if (!Truncate(old_length + length)) { return {}; diff --git a/impeller/core/host_buffer.h b/impeller/core/host_buffer.h index 3cfbb4e44bfdf..ec2965d75c1b7 100644 --- a/impeller/core/host_buffer.h +++ b/impeller/core/host_buffer.h @@ -98,18 +98,18 @@ class HostBuffer final : public std::enable_shared_from_this, using EmplaceProc = std::function; //---------------------------------------------------------------------------- - /// @brief Emplace non-uniform data (like contiguous vertices) onto the - /// host buffer directly. The buffer ptr is guaranteed to have at - /// least enough storage for the length of data. + /// @brief Emplaces undefined data onto the managed buffer and gives the + /// caller a chance to update it using the specified callback. The + /// buffer is guaranteed to have enough space for length bytes. It + /// is the responsibility of the caller to not exceed the bounds + /// of the buffer returned in the EmplaceProc. /// /// @param[in] cb A callback that will be passed a ptr to the /// underlying host buffer. /// /// @return The buffer view. /// - BufferView EmplaceCallback(size_t length, - size_t align, - const EmplaceProc& cb); + BufferView Emplace(size_t length, size_t align, const EmplaceProc& cb); private: mutable std::shared_ptr device_buffer_; diff --git a/impeller/entity/contents/text_contents.cc b/impeller/entity/contents/text_contents.cc index a756a142967ff..a4e46641b2e14 100644 --- a/impeller/entity/contents/text_contents.cc +++ b/impeller/entity/contents/text_contents.cc @@ -146,7 +146,7 @@ static bool CommonRender(const ContentContext& renderer, } vertex_count *= 6; - auto buffer_view = host_buffer.EmplaceCallback( + auto buffer_view = host_buffer.Emplace( vertex_count * sizeof(VS::PerVertexData), alignof(VS::PerVertexData), [&](uint8_t* contents) { VS::PerVertexData vtx;