From 184cc830246057b0bee03cfe2cb2a7fdfbba531b Mon Sep 17 00:00:00 2001 From: Jonah Williams Date: Thu, 25 Jan 2024 10:51:23 -0800 Subject: [PATCH 1/2] Revert "[Impeller] round up subpass coverage when it is close to (and smaller) than root pass size." (#50041) Reverts flutter/engine#49925 This did not improve benchmarks: https://flutter-flutter-perf.skia.org/e/?queries=device_type%3DPixel_7_Pro%26sub_result%3D90th_percentile_frame_rasterizer_time_millis%26sub_result%3D99th_percentile_frame_rasterizer_time_millis%26sub_result%3Daverage_frame_rasterizer_time_millis%26sub_result%3Dworst_frame_rasterizer_time_millis%26test%3Dnew_gallery_impeller_old_zoom__transition_perf&selected=commit%3D38910%26name%3D%252Carch%253Dintel%252Cbranch%253Dmaster%252Cconfig%253Ddefault%252Cdevice_type%253DPixel_7_Pro%252Cdevice_version%253Dnone%252Chost_type%253Dlinux%252Csub_result%253D99th_percentile_frame_rasterizer_time_millis%252Ctest%253Dnew_gallery_impeller_old_zoom__transition_perf%252C From follow up investigation: some of the routes hit this optimization, while some did not as the the bounds size seems to depend on the exact contents. --- impeller/aiks/aiks_unittests.cc | 48 ---------------------------- impeller/entity/entity_pass.cc | 55 +++------------------------------ impeller/entity/entity_pass.h | 3 -- 3 files changed, 4 insertions(+), 102 deletions(-) diff --git a/impeller/aiks/aiks_unittests.cc b/impeller/aiks/aiks_unittests.cc index fd0166511c899..a5041faab3879 100644 --- a/impeller/aiks/aiks_unittests.cc +++ b/impeller/aiks/aiks_unittests.cc @@ -3928,54 +3928,6 @@ TEST_P(AiksTest, GaussianBlurMipMapImageFilter) { #endif } -TEST_P(AiksTest, SaveLayersCloseToRootPassSizeAreScaledUp) { - Canvas canvas; - // Create a subpass with no bounds hint and an entity coverage of (95, 95). - canvas.SaveLayer({ - .color = Color::Black().WithAlpha(0.5), - }); - canvas.DrawRect(Rect::MakeLTRB(0, 0, 10, 10), {.color = Color::Red()}); - canvas.DrawRect(Rect::MakeLTRB(0, 0, 95, 95), {.color = Color::Blue()}); - canvas.Restore(); - - Picture picture = canvas.EndRecordingAsPicture(); - std::shared_ptr cache = - std::make_shared(GetContext()->GetResourceAllocator()); - AiksContext aiks_context(GetContext(), nullptr, cache); - picture.ToImage(aiks_context, {100, 100}); - - for (auto it = cache->GetTextureDataBegin(); it != cache->GetTextureDataEnd(); - ++it) { - EXPECT_EQ(it->texture->GetTextureDescriptor().size, ISize(100, 100)); - } -} - -TEST_P(AiksTest, SaveLayersCloseToRootPassSizeAreNotScaledUpPastBoundsHint) { - Canvas canvas; - canvas.SaveLayer( - { - .color = Color::Black().WithAlpha(0.5), - }, - Rect::MakeSize(ISize(95, 95))); - canvas.DrawRect(Rect::MakeLTRB(0, 0, 100, 100), {.color = Color::Red()}); - canvas.DrawRect(Rect::MakeLTRB(50, 50, 150, 150), {.color = Color::Blue()}); - canvas.Restore(); - - Picture picture = canvas.EndRecordingAsPicture(); - std::shared_ptr cache = - std::make_shared(GetContext()->GetResourceAllocator()); - AiksContext aiks_context(GetContext(), nullptr, cache); - picture.ToImage(aiks_context, {100, 100}); - - // We expect a single 100x100 texture and the rest should be 95x95. - EXPECT_EQ(cache->GetTextureDataBegin()->texture->GetTextureDescriptor().size, - ISize(100, 100)); - for (auto it = ++cache->GetTextureDataBegin(); - it != cache->GetTextureDataEnd(); ++it) { - EXPECT_EQ(it->texture->GetTextureDescriptor().size, ISize(95, 95)); - } -} - TEST_P(AiksTest, ImageColorSourceEffectTransform) { // Compare with https://fiddle.skia.org/c/6cdc5aefb291fda3833b806ca347a885 diff --git a/impeller/entity/entity_pass.cc b/impeller/entity/entity_pass.cc index 52e44c9c3efc9..701910f5e893a 100644 --- a/impeller/entity/entity_pass.cc +++ b/impeller/entity/entity_pass.cc @@ -23,9 +23,7 @@ #include "impeller/entity/entity.h" #include "impeller/entity/inline_pass_context.h" #include "impeller/geometry/color.h" -#include "impeller/geometry/matrix.h" #include "impeller/geometry/rect.h" -#include "impeller/geometry/size.h" #include "impeller/renderer/command_buffer.h" #ifdef IMPELLER_DEBUG @@ -69,13 +67,6 @@ std::optional EntityPass::GetBoundsLimit() const { return bounds_limit_; } -std::optional EntityPass::GetTransformedBoundsLimit() const { - if (bounds_limit_.has_value()) { - return bounds_limit_->TransformBounds(transform_); - } - return std::nullopt; -} - void EntityPass::AddEntity(Entity entity) { if (entity.GetBlendMode() == BlendMode::kSourceOver && entity.GetContents()->IsOpaque()) { @@ -199,11 +190,12 @@ std::optional EntityPass::GetSubpassCoverage( return std::nullopt; } - auto user_bounds_coverage = subpass.GetTransformedBoundsLimit(); - if (!user_bounds_coverage.has_value()) { + if (!subpass.bounds_limit_.has_value()) { return entities_coverage; } - return entities_coverage->Intersection(user_bounds_coverage.value()); + auto user_bounds_coverage = + subpass.bounds_limit_->TransformBounds(subpass.transform_); + return entities_coverage->Intersection(user_bounds_coverage); } EntityPass* EntityPass::GetSuperpass() const { @@ -493,43 +485,6 @@ bool EntityPass::Render(ContentContext& renderer, clip_coverage_stack); // clip_coverage_stack } -// When a subpass size is close to, but still smaller than the root pass -// size and smaller than the bounds hint, we may scale it up to the root -// pass size. This will improve performance by improving the efficiency of -// the render target cache, as only textures with exactly the same sizes + -// descriptors can be recycled. -static ISize MaybeRoundUpTextureSize(ISize subpass_size, - ISize root_pass_size, - std::optional bounds_limit) { - // If the subpass is already bigger than the root pass size, - // return the existing subpass size. - if (subpass_size.width > root_pass_size.width || - subpass_size.height > root_pass_size.height) { - return subpass_size; - } - - // If there is a bounds limit and it is tigher than the root pass size, - // return the existing subpass size. This case could be removed if we - // conditionally inserted clips/scissor instead. - if (bounds_limit.has_value()) { - auto bounds_size = bounds_limit->GetSize(); - if (bounds_size.width < root_pass_size.width || - bounds_size.height < root_pass_size.height) { - return subpass_size; - } - } - - // If the subpass size is within 10% of the root pass size, round up - // to the root pass size. - if (root_pass_size.width - subpass_size.width <= - (0.1 * root_pass_size.width) && - root_pass_size.height - subpass_size.height <= - (0.1 * root_pass_size.height)) { - return root_pass_size; - } - return subpass_size; -} - EntityPass::EntityResult EntityPass::GetEntityForElement( const EntityPass::Element& element, ContentContext& renderer, @@ -663,8 +618,6 @@ EntityPass::EntityResult EntityPass::GetEntityForElement( return EntityPass::EntityResult::Skip(); } - subpass_size = MaybeRoundUpTextureSize( - subpass_size, root_pass_size, subpass->GetTransformedBoundsLimit()); auto subpass_target = CreateRenderTarget( renderer, // renderer subpass_size, // size diff --git a/impeller/entity/entity_pass.h b/impeller/entity/entity_pass.h index f1d4b51715b6e..7de68d0d9ad11 100644 --- a/impeller/entity/entity_pass.h +++ b/impeller/entity/entity_pass.h @@ -157,9 +157,6 @@ class EntityPass { required_mip_count_ = mip_count; } - /// @brief Return the local bounds transformed intro screen coordinate space. - std::optional GetTransformedBoundsLimit() const; - //---------------------------------------------------------------------------- /// @brief Computes the coverage of a given subpass. This is used to /// determine the texture size of a given subpass before it's rendered From b2167a93c1a0470f5d45553fc332e2914d03b29f Mon Sep 17 00:00:00 2001 From: Jonah Williams Date: Thu, 25 Jan 2024 10:58:57 -0800 Subject: [PATCH 2/2] [Impeller] write vertices geometry data to host buffer. (#49741) Now that host buffer writes go directly to device buffers, there is no advantage to creating dedicated device buffers for the vertices contents. Fixes https://github.com/flutter/flutter/issues/141202 --- impeller/entity/geometry/vertices_geometry.cc | 155 +++++++----------- 1 file changed, 59 insertions(+), 96 deletions(-) diff --git a/impeller/entity/geometry/vertices_geometry.cc b/impeller/entity/geometry/vertices_geometry.cc index fd9e1770a9ffc..cfcc05bb51b6f 100644 --- a/impeller/entity/geometry/vertices_geometry.cc +++ b/impeller/entity/geometry/vertices_geometry.cc @@ -4,10 +4,10 @@ #include "impeller/entity/geometry/vertices_geometry.h" +#include #include -#include -#include "impeller/core/formats.h" +#include "impeller/core/buffer_view.h" namespace impeller { @@ -118,34 +118,22 @@ GeometryResult VerticesGeometry::GetPositionBuffer( size_t total_vtx_bytes = vertex_count * sizeof(float) * 2; size_t total_idx_bytes = index_count * sizeof(uint16_t); - DeviceBufferDescriptor buffer_desc; - buffer_desc.size = total_vtx_bytes + total_idx_bytes; - buffer_desc.storage_mode = StorageMode::kHostVisible; - - auto buffer = - renderer.GetContext()->GetResourceAllocator()->CreateBuffer(buffer_desc); + auto vertex_buffer = renderer.GetTransientsBuffer().Emplace( + reinterpret_cast(vertices_.data()), total_vtx_bytes, + alignof(float)); - if (!buffer->CopyHostBuffer( - reinterpret_cast(vertices_.data()), - Range{0, total_vtx_bytes}, 0)) { - return {}; - } - if (index_count > 0 && - !buffer->CopyHostBuffer( - reinterpret_cast(const_cast(indices_.data())), - Range{0, total_idx_bytes}, total_vtx_bytes)) { - return {}; + BufferView index_buffer = {}; + if (index_count) { + index_buffer = renderer.GetTransientsBuffer().Emplace( + indices_.data(), total_idx_bytes, alignof(uint16_t)); } return GeometryResult{ .type = GetPrimitiveType(), .vertex_buffer = { - .vertex_buffer = {.buffer = buffer, - .range = Range{0, total_vtx_bytes}}, - .index_buffer = {.buffer = buffer, - .range = - Range{total_vtx_bytes, total_idx_bytes}}, + .vertex_buffer = vertex_buffer, + .index_buffer = index_buffer, .vertex_count = index_count > 0 ? index_count : vertex_count, .index_type = index_count > 0 ? IndexType::k16bit : IndexType::kNone, @@ -163,47 +151,34 @@ GeometryResult VerticesGeometry::GetPositionColorBuffer( auto index_count = indices_.size(); auto vertex_count = vertices_.size(); - - std::vector vertex_data(vertex_count); - { - for (auto i = 0u; i < vertex_count; i++) { - vertex_data[i] = { - .position = vertices_[i], - .color = colors_[i], - }; - } - } - - size_t total_vtx_bytes = vertex_data.size() * sizeof(VS::PerVertexData); + size_t total_vtx_bytes = vertex_count * sizeof(VS::PerVertexData); size_t total_idx_bytes = index_count * sizeof(uint16_t); - DeviceBufferDescriptor buffer_desc; - buffer_desc.size = total_vtx_bytes + total_idx_bytes; - buffer_desc.storage_mode = StorageMode::kHostVisible; - - auto buffer = - renderer.GetContext()->GetResourceAllocator()->CreateBuffer(buffer_desc); - - if (!buffer->CopyHostBuffer(reinterpret_cast(vertex_data.data()), - Range{0, total_vtx_bytes}, 0)) { - return {}; - } - if (index_count > 0 && - !buffer->CopyHostBuffer( - reinterpret_cast(const_cast(indices_.data())), - Range{0, total_idx_bytes}, total_vtx_bytes)) { - return {}; + auto vertex_buffer = renderer.GetTransientsBuffer().Emplace( + total_vtx_bytes, alignof(VS::PerVertexData), [&](uint8_t* data) { + VS::PerVertexData* vtx_contents = + reinterpret_cast(data); + for (auto i = 0u; i < vertices_.size(); i++) { + VS::PerVertexData vertex_data = { + .position = vertices_[i], + .color = colors_[i], + }; + std::memcpy(vtx_contents++, &vertex_data, sizeof(VS::PerVertexData)); + } + }); + + BufferView index_buffer = {}; + if (index_count > 0) { + index_buffer = renderer.GetTransientsBuffer().Emplace( + indices_.data(), total_idx_bytes, alignof(uint16_t)); } return GeometryResult{ .type = GetPrimitiveType(), .vertex_buffer = { - .vertex_buffer = {.buffer = buffer, - .range = Range{0, total_vtx_bytes}}, - .index_buffer = {.buffer = buffer, - .range = - Range{total_vtx_bytes, total_idx_bytes}}, + .vertex_buffer = vertex_buffer, + .index_buffer = index_buffer, .vertex_count = index_count > 0 ? index_count : vertex_count, .index_type = index_count > 0 ? IndexType::k16bit : IndexType::kNone, @@ -226,54 +201,42 @@ GeometryResult VerticesGeometry::GetPositionUVBuffer( auto uv_transform = texture_coverage.GetNormalizingTransform() * effect_transform; auto has_texture_coordinates = HasTextureCoordinates(); - std::vector vertex_data(vertex_count); - { - for (auto i = 0u; i < vertex_count; i++) { - auto vertex = vertices_[i]; - auto texture_coord = - has_texture_coordinates ? texture_coordinates_[i] : vertices_[i]; - auto uv = uv_transform * texture_coord; - // From experimentation we need to clamp these values to < 1.0 or else - // there can be flickering. - vertex_data[i] = { - .position = vertex, - .texture_coords = - Point(std::clamp(uv.x, 0.0f, 1.0f - kEhCloseEnough), - std::clamp(uv.y, 0.0f, 1.0f - kEhCloseEnough)), - }; - } - } - size_t total_vtx_bytes = vertex_data.size() * sizeof(VS::PerVertexData); + size_t total_vtx_bytes = vertices_.size() * sizeof(VS::PerVertexData); size_t total_idx_bytes = index_count * sizeof(uint16_t); - - DeviceBufferDescriptor buffer_desc; - buffer_desc.size = total_vtx_bytes + total_idx_bytes; - buffer_desc.storage_mode = StorageMode::kHostVisible; - - auto buffer = - renderer.GetContext()->GetResourceAllocator()->CreateBuffer(buffer_desc); - - if (!buffer->CopyHostBuffer(reinterpret_cast(vertex_data.data()), - Range{0, total_vtx_bytes}, 0)) { - return {}; - } - if (index_count > 0u && - !buffer->CopyHostBuffer( - reinterpret_cast(const_cast(indices_.data())), - Range{0, total_idx_bytes}, total_vtx_bytes)) { - return {}; + auto vertex_buffer = renderer.GetTransientsBuffer().Emplace( + total_vtx_bytes, alignof(VS::PerVertexData), [&](uint8_t* data) { + VS::PerVertexData* vtx_contents = + reinterpret_cast(data); + for (auto i = 0u; i < vertices_.size(); i++) { + auto vertex = vertices_[i]; + auto texture_coord = + has_texture_coordinates ? texture_coordinates_[i] : vertices_[i]; + auto uv = uv_transform * texture_coord; + // From experimentation we need to clamp these values to < 1.0 or else + // there can be flickering. + VS::PerVertexData vertex_data = { + .position = vertex, + .texture_coords = + Point(std::clamp(uv.x, 0.0f, 1.0f - kEhCloseEnough), + std::clamp(uv.y, 0.0f, 1.0f - kEhCloseEnough)), + }; + std::memcpy(vtx_contents++, &vertex_data, sizeof(VS::PerVertexData)); + } + }); + + BufferView index_buffer = {}; + if (index_count > 0) { + index_buffer = renderer.GetTransientsBuffer().Emplace( + indices_.data(), total_idx_bytes, alignof(uint16_t)); } return GeometryResult{ .type = GetPrimitiveType(), .vertex_buffer = { - .vertex_buffer = {.buffer = buffer, - .range = Range{0, total_vtx_bytes}}, - .index_buffer = {.buffer = buffer, - .range = - Range{total_vtx_bytes, total_idx_bytes}}, + .vertex_buffer = vertex_buffer, + .index_buffer = index_buffer, .vertex_count = index_count > 0 ? index_count : vertex_count, .index_type = index_count > 0 ? IndexType::k16bit : IndexType::kNone,