From 88fc864935b6186ed3e8922ba317cd5cfb2a44ad Mon Sep 17 00:00:00 2001 From: Jonah Williams Date: Wed, 24 Apr 2024 11:24:06 -0700 Subject: [PATCH] [Impeller] delete points compute shader. (#52346) We only use this for drawPoints, a rarely used API. On local tests, this is just as fast with the CPU backend implementations. While this was intended to be the first in a series of compute based rendering experiments, it hasn't really been worth the carrying cost. So lets shrink the complexity and and remove another shader to boot. Fixes https://github.com/flutter/flutter/issues/147184 --- ci/licenses_golden/licenses_flutter | 2 - impeller/aiks/aiks_unittests.cc | 8 - impeller/entity/BUILD.gn | 1 - impeller/entity/contents/content_context.cc | 6 - impeller/entity/contents/content_context.h | 12 -- impeller/entity/entity_unittests.cc | 18 --- .../entity/geometry/point_field_geometry.cc | 139 ++---------------- .../entity/geometry/point_field_geometry.h | 11 -- impeller/entity/shaders/geometry/points.comp | 67 --------- impeller/tools/malioc.json | 63 -------- testing/impeller_golden_tests_output.txt | 2 + 11 files changed, 12 insertions(+), 317 deletions(-) delete mode 100644 impeller/entity/shaders/geometry/points.comp diff --git a/ci/licenses_golden/licenses_flutter b/ci/licenses_golden/licenses_flutter index a76e2b9f70e1b..6eda042193958 100644 --- a/ci/licenses_golden/licenses_flutter +++ b/ci/licenses_golden/licenses_flutter @@ -40396,7 +40396,6 @@ ORIGIN: ../../../flutter/impeller/entity/shaders/gaussian_blur/kernel.glsl + ../ ORIGIN: ../../../flutter/impeller/entity/shaders/gaussian_blur/kernel.vert + ../../../flutter/LICENSE ORIGIN: ../../../flutter/impeller/entity/shaders/gaussian_blur/kernel_decal.frag + ../../../flutter/LICENSE ORIGIN: ../../../flutter/impeller/entity/shaders/gaussian_blur/kernel_nodecal.frag + ../../../flutter/LICENSE -ORIGIN: ../../../flutter/impeller/entity/shaders/geometry/points.comp + ../../../flutter/LICENSE ORIGIN: ../../../flutter/impeller/entity/shaders/glyph_atlas.frag + ../../../flutter/LICENSE ORIGIN: ../../../flutter/impeller/entity/shaders/glyph_atlas.vert + ../../../flutter/LICENSE ORIGIN: ../../../flutter/impeller/entity/shaders/glyph_atlas_color.frag + ../../../flutter/LICENSE @@ -43278,7 +43277,6 @@ FILE: ../../../flutter/impeller/entity/shaders/gaussian_blur/kernel.glsl FILE: ../../../flutter/impeller/entity/shaders/gaussian_blur/kernel.vert FILE: ../../../flutter/impeller/entity/shaders/gaussian_blur/kernel_decal.frag FILE: ../../../flutter/impeller/entity/shaders/gaussian_blur/kernel_nodecal.frag -FILE: ../../../flutter/impeller/entity/shaders/geometry/points.comp FILE: ../../../flutter/impeller/entity/shaders/glyph_atlas.frag FILE: ../../../flutter/impeller/entity/shaders/glyph_atlas.vert FILE: ../../../flutter/impeller/entity/shaders/glyph_atlas_color.frag diff --git a/impeller/aiks/aiks_unittests.cc b/impeller/aiks/aiks_unittests.cc index e1b860465775d..96bd9eccd63c8 100644 --- a/impeller/aiks/aiks_unittests.cc +++ b/impeller/aiks/aiks_unittests.cc @@ -2349,10 +2349,6 @@ TEST_P(AiksTest, DrawPaintTransformsBounds) { } TEST_P(AiksTest, CanDrawPoints) { - if (GetBackend() == PlaygroundBackend::kMetal) { - // https://github.com/flutter/flutter/issues/147184 - GTEST_SKIP() << "Draw Points is currently broken on the metal m1 backend."; - } std::vector points = { {0, 0}, // {100, 100}, // @@ -2447,10 +2443,6 @@ TEST_P(AiksTest, DrawAtlasAdvancedAndTransform) { } TEST_P(AiksTest, CanDrawPointsWithTextureMap) { - if (GetBackend() == PlaygroundBackend::kMetal) { - // https://github.com/flutter/flutter/issues/147184 - GTEST_SKIP() << "Draw Points is currently broken on the metal m1 backend."; - } auto texture = CreateTextureForFixture("table_mountain_nx.png", /*enable_mipmapping=*/true); diff --git a/impeller/entity/BUILD.gn b/impeller/entity/BUILD.gn index a919744158fd4..988a61765f6ba 100644 --- a/impeller/entity/BUILD.gn +++ b/impeller/entity/BUILD.gn @@ -81,7 +81,6 @@ impeller_shaders("modern_entity_shaders") { "shaders/gradients/linear_gradient_ssbo_fill.frag", "shaders/gradients/radial_gradient_ssbo_fill.frag", "shaders/gradients/sweep_gradient_ssbo_fill.frag", - "shaders/geometry/points.comp", ] } diff --git a/impeller/entity/contents/content_context.cc b/impeller/entity/contents/content_context.cc index d22bb4cc097a2..b6e6dde5b5806 100644 --- a/impeller/entity/contents/content_context.cc +++ b/impeller/entity/contents/content_context.cc @@ -450,12 +450,6 @@ ContentContext::ContentContext( tiled_texture_external_pipelines_.CreateDefault(*context_, options); } #endif // IMPELLER_ENABLE_OPENGLES - if (context_->GetCapabilities()->SupportsCompute()) { - auto pipeline_desc = - PointsComputeShaderPipeline::MakeDefaultPipelineDescriptor(*context_); - point_field_compute_pipelines_ = - context_->GetPipelineLibrary()->GetPipeline(pipeline_desc).Get(); - } is_valid_ = true; InitializeCommonlyUsedShadersIfNeeded(); diff --git a/impeller/entity/contents/content_context.h b/impeller/entity/contents/content_context.h index 5bbe6d0cda8f5..9e3011674e434 100644 --- a/impeller/entity/contents/content_context.h +++ b/impeller/entity/contents/content_context.h @@ -44,7 +44,6 @@ #include "impeller/entity/linear_to_srgb_filter.frag.h" #include "impeller/entity/morphology_filter.frag.h" #include "impeller/entity/morphology_filter.vert.h" -#include "impeller/entity/points.comp.h" #include "impeller/entity/porter_duff_blend.frag.h" #include "impeller/entity/porter_duff_blend.vert.h" #include "impeller/entity/radial_gradient_fill.frag.h" @@ -256,9 +255,6 @@ using FramebufferBlendSoftLightPipeline = using VerticesUberShader = RenderPipelineHandle; -/// Geometry Pipelines -using PointsComputeShaderPipeline = ComputePipelineBuilder; - #ifdef IMPELLER_ENABLE_OPENGLES using TiledTextureExternalPipeline = RenderPipelineHandle> GetPointComputePipeline() - const { - FML_DCHECK(GetDeviceCapabilities().SupportsCompute()); - return point_field_compute_pipelines_; - } - std::shared_ptr GetContext() const; const Capabilities& GetDeviceCapabilities() const; @@ -987,8 +977,6 @@ class ContentContext { mutable Variants framebuffer_blend_softlight_pipelines_; mutable Variants vertices_uber_shader_; - mutable std::shared_ptr> - point_field_compute_pipelines_; template std::shared_ptr> GetPipeline( diff --git a/impeller/entity/entity_unittests.cc b/impeller/entity/entity_unittests.cc index 30ed5395695a9..f4b77f77479c7 100644 --- a/impeller/entity/entity_unittests.cc +++ b/impeller/entity/entity_unittests.cc @@ -2519,24 +2519,6 @@ TEST_P(EntityTest, TiledTextureContentsIsOpaque) { ASSERT_FALSE(contents.IsOpaque()); } -TEST_P(EntityTest, PointFieldGeometryDivisions) { - // Square always gives 4 divisions. - ASSERT_EQ(PointFieldGeometry::ComputeCircleDivisions(24.0, false), 4u); - ASSERT_EQ(PointFieldGeometry::ComputeCircleDivisions(2.0, false), 4u); - ASSERT_EQ(PointFieldGeometry::ComputeCircleDivisions(200.0, false), 4u); - - ASSERT_EQ(PointFieldGeometry::ComputeCircleDivisions(0.5, true), 4u); - ASSERT_EQ(PointFieldGeometry::ComputeCircleDivisions(1.5, true), 8u); - ASSERT_EQ(PointFieldGeometry::ComputeCircleDivisions(5.5, true), 24u); - ASSERT_EQ(PointFieldGeometry::ComputeCircleDivisions(12.5, true), 34u); - ASSERT_EQ(PointFieldGeometry::ComputeCircleDivisions(22.3, true), 22u); - ASSERT_EQ(PointFieldGeometry::ComputeCircleDivisions(40.5, true), 40u); - ASSERT_EQ(PointFieldGeometry::ComputeCircleDivisions(100.0, true), 100u); - // Caps at 140. - ASSERT_EQ(PointFieldGeometry::ComputeCircleDivisions(1000.0, true), 140u); - ASSERT_EQ(PointFieldGeometry::ComputeCircleDivisions(20000.0, true), 140u); -} - TEST_P(EntityTest, PointFieldGeometryCoverage) { std::vector points = {{10, 20}, {100, 200}}; auto geometry = Geometry::MakePointField(points, 5.0, false); diff --git a/impeller/entity/geometry/point_field_geometry.cc b/impeller/entity/geometry/point_field_geometry.cc index 812f72eda0b79..d62b77a3b18d5 100644 --- a/impeller/entity/geometry/point_field_geometry.cc +++ b/impeller/entity/geometry/point_field_geometry.cc @@ -4,7 +4,8 @@ #include "impeller/entity/geometry/point_field_geometry.h" -#include "impeller/geometry/color.h" +#include "impeller/core/vertex_buffer.h" +#include "impeller/entity/geometry/geometry.h" #include "impeller/renderer/command_buffer.h" namespace impeller { @@ -18,38 +19,19 @@ GeometryResult PointFieldGeometry::GetPositionBuffer( const ContentContext& renderer, const Entity& entity, RenderPass& pass) const { - if (renderer.GetDeviceCapabilities().SupportsCompute()) { - return GetPositionBufferGPU(renderer, entity, pass); - } - auto vtx_builder = GetPositionBufferCPU(renderer, entity, pass); - if (!vtx_builder.has_value()) { - return {}; - } - - auto& host_buffer = renderer.GetTransientsBuffer(); - return { - .type = PrimitiveType::kTriangleStrip, - .vertex_buffer = vtx_builder->CreateVertexBuffer(host_buffer), - .transform = entity.GetShaderTransform(pass), - }; -} - -std::optional> -PointFieldGeometry::GetPositionBufferCPU(const ContentContext& renderer, - const Entity& entity, - RenderPass& pass) const { if (radius_ < 0.0) { - return std::nullopt; + return {}; } - auto transform = entity.GetTransform(); - auto determinant = transform.GetDeterminant(); + Matrix transform = entity.GetTransform(); + Scalar determinant = transform.GetDeterminant(); if (determinant == 0) { - return std::nullopt; + return {}; } Scalar min_size = 1.0f / sqrt(std::abs(determinant)); Scalar radius = std::max(radius_, min_size); + HostBuffer& host_buffer = renderer.GetTransientsBuffer(); VertexBufferBuilder vtx_builder; if (round_) { @@ -94,114 +76,13 @@ PointFieldGeometry::GetPositionBufferCPU(const ContentContext& renderer, } } - return vtx_builder; -} - -GeometryResult PointFieldGeometry::GetPositionBufferGPU( - const ContentContext& renderer, - const Entity& entity, - RenderPass& pass) const { - FML_DCHECK(renderer.GetDeviceCapabilities().SupportsCompute()); - if (radius_ < 0.0) { - return {}; - } - Scalar determinant = entity.GetTransform().GetDeterminant(); - if (determinant == 0) { - return {}; - } - - Scalar min_size = 1.0f / sqrt(std::abs(determinant)); - Scalar radius = std::max(radius_, min_size); - - size_t vertices_per_geom = ComputeCircleDivisions( - entity.GetTransform().GetMaxBasisLength() * radius, round_); - - size_t points_per_circle = 3 + (vertices_per_geom - 3) * 3; - size_t total = points_per_circle * points_.size(); - - std::shared_ptr cmd_buffer = - renderer.GetContext()->CreateCommandBuffer(); - std::shared_ptr compute_pass = cmd_buffer->CreateComputePass(); - HostBuffer& host_buffer = renderer.GetTransientsBuffer(); - - BufferView points_data = - host_buffer.Emplace(points_.data(), points_.size() * sizeof(Point), - DefaultUniformAlignment()); - - BufferView geometry_buffer = - host_buffer.Emplace(nullptr, total * sizeof(Point), - std::max(DefaultUniformAlignment(), alignof(Point))); - - BufferView output; - { - using PS = PointsComputeShader; - - compute_pass->SetPipeline(renderer.GetPointComputePipeline()); - compute_pass->SetCommandLabel("Points Geometry"); - - PS::FrameInfo frame_info; - frame_info.count = points_.size(); - frame_info.radius = round_ ? radius : radius * kSqrt2; - frame_info.radian_start = round_ ? 0.0f : kPiOver4; - frame_info.radian_step = k2Pi / vertices_per_geom; - frame_info.points_per_circle = points_per_circle; - frame_info.divisions_per_circle = vertices_per_geom; - - PS::BindFrameInfo(*compute_pass, host_buffer.EmplaceUniform(frame_info)); - PS::BindGeometryData(*compute_pass, geometry_buffer); - PS::BindPointData(*compute_pass, points_data); - - if (!compute_pass->Compute(ISize(total, 1)).ok()) { - return {}; - } - output = geometry_buffer; - } - - if (!compute_pass->EncodeCommands()) { - return {}; - } - if (!renderer.GetContext() - ->GetCommandQueue() - ->Submit({std::move(cmd_buffer)}) - .ok()) { - return {}; - } - - return { - .type = PrimitiveType::kTriangle, - .vertex_buffer = {.vertex_buffer = std::move(output), - .vertex_count = total, - .index_type = IndexType::kNone}, + return GeometryResult{ + .type = PrimitiveType::kTriangleStrip, + .vertex_buffer = vtx_builder.CreateVertexBuffer(host_buffer), .transform = entity.GetShaderTransform(pass), }; } -/// @brief Compute the number of vertices to divide each circle into. -/// -/// @return the number of vertices. -size_t PointFieldGeometry::ComputeCircleDivisions(Scalar scaled_radius, - bool round) { - if (!round) { - return 4; - } - - // Note: these values are approximated based on the values returned from - // the decomposition of 4 cubics performed by Path::CreatePolyline. - if (scaled_radius < 1.0) { - return 4; - } - if (scaled_radius < 2.0) { - return 8; - } - if (scaled_radius < 12.0) { - return 24; - } - if (scaled_radius < 22.0) { - return 34; - } - return std::min(scaled_radius, 140.0f); -} - // |Geometry| std::optional PointFieldGeometry::GetCoverage( const Matrix& transform) const { diff --git a/impeller/entity/geometry/point_field_geometry.h b/impeller/entity/geometry/point_field_geometry.h index c175b7de93023..14696a1652c9f 100644 --- a/impeller/entity/geometry/point_field_geometry.h +++ b/impeller/entity/geometry/point_field_geometry.h @@ -15,8 +15,6 @@ class PointFieldGeometry final : public Geometry { ~PointFieldGeometry() = default; - static size_t ComputeCircleDivisions(Scalar scaled_radius, bool round); - private: // |Geometry| GeometryResult GetPositionBuffer(const ContentContext& renderer, @@ -26,15 +24,6 @@ class PointFieldGeometry final : public Geometry { // |Geometry| std::optional GetCoverage(const Matrix& transform) const override; - GeometryResult GetPositionBufferGPU(const ContentContext& renderer, - const Entity& entity, - RenderPass& pass) const; - - std::optional> - GetPositionBufferCPU(const ContentContext& renderer, - const Entity& entity, - RenderPass& pass) const; - std::vector points_; Scalar radius_; bool round_; diff --git a/impeller/entity/shaders/geometry/points.comp b/impeller/entity/shaders/geometry/points.comp deleted file mode 100644 index 7d274ade49948..0000000000000 --- a/impeller/entity/shaders/geometry/points.comp +++ /dev/null @@ -1,67 +0,0 @@ -// 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 -#include - -// Unused, see See PointFieldGeometry::GetPositionBuffer -layout(local_size_x = 16) in; - -layout(std430) readonly buffer PointData { - // Size of this input data is frame_info.count; - vec2 points[]; -} -point_data; - -layout(std430) writeonly buffer GeometryData { - // Size of this output data is frame_info.count * points_per_circle; - vec2 geometry[]; -} -geometry_data; - -uniform FrameInfo { - uint count; - float16_t radius; - float16_t radian_start; - float16_t radian_step; - uint points_per_circle; - int divisions_per_circle; -} -frame_info; - -void main() { - uint ident = gl_GlobalInvocationID.x; - if (ident >= frame_info.count) { - return; - } - - vec2 center = point_data.points[ident]; - uint bufer_offset = ident * frame_info.points_per_circle; - - float16_t elapsed_angle = frame_info.radian_start; - - vec2 origin = - center + vec2(cos(elapsed_angle), sin(elapsed_angle)) * frame_info.radius; - geometry_data.geometry[bufer_offset++] = origin; - - elapsed_angle += frame_info.radian_step; - vec2 pt1 = - center + vec2(cos(elapsed_angle), sin(elapsed_angle)) * frame_info.radius; - geometry_data.geometry[bufer_offset++] = pt1; - - elapsed_angle += frame_info.radian_step; - vec2 pt2 = - center + vec2(cos(elapsed_angle), sin(elapsed_angle)) * frame_info.radius; - geometry_data.geometry[bufer_offset++] = pt2; - - for (int i = 0; i < frame_info.divisions_per_circle - 3; i++) { - geometry_data.geometry[bufer_offset++] = origin; - geometry_data.geometry[bufer_offset++] = pt2; - - elapsed_angle += frame_info.radian_step; - pt2 = center + - vec2(cos(elapsed_angle), sin(elapsed_angle)) * frame_info.radius; - geometry_data.geometry[bufer_offset++] = pt2; - } -} diff --git a/impeller/tools/malioc.json b/impeller/tools/malioc.json index ac45ec39759f1..fa385d70d019d 100644 --- a/impeller/tools/malioc.json +++ b/impeller/tools/malioc.json @@ -7633,69 +7633,6 @@ } } }, - "flutter/impeller/entity/points.comp.vkspv": { - "Mali-G78": { - "core": "Mali-G78", - "filename": "flutter/impeller/entity/points.comp.vkspv", - "has_uniform_computation": true, - "type": "Compute", - "variants": { - "Main": { - "fp16_arithmetic": 0, - "has_stack_spilling": false, - "performance": { - "longest_path_bound_pipelines": [ - null - ], - "longest_path_cycles": [ - null, - null, - null, - null, - null, - null - ], - "pipelines": [ - "arith_total", - "arith_fma", - "arith_cvt", - "arith_sfu", - "load_store", - "texture" - ], - "shortest_path_bound_pipelines": [ - "arith_total", - "arith_cvt" - ], - "shortest_path_cycles": [ - 0.046875, - 0.0, - 0.046875, - 0.0, - 0.0, - 0.0 - ], - "total_bound_pipelines": [ - "load_store" - ], - "total_cycles": [ - 0.296875, - 0.296875, - 0.28125, - 0.1875, - 5.0, - 0.0 - ] - }, - "shared_storage_used": 0, - "stack_spill_bytes": 0, - "thread_occupancy": 100, - "uniform_registers_used": 20, - "work_registers_used": 16 - } - } - } - }, "flutter/impeller/entity/porter_duff_blend.frag.vkspv": { "Mali-G78": { "core": "Mali-G78", diff --git a/testing/impeller_golden_tests_output.txt b/testing/impeller_golden_tests_output.txt index 33562e0ffd6d5..17055406dccd4 100644 --- a/testing/impeller_golden_tests_output.txt +++ b/testing/impeller_golden_tests_output.txt @@ -209,8 +209,10 @@ impeller_Play_AiksTest_CanDrawPaint_Vulkan.png impeller_Play_AiksTest_CanDrawPerspectiveTransformWithClips_Metal.png impeller_Play_AiksTest_CanDrawPerspectiveTransformWithClips_OpenGLES.png impeller_Play_AiksTest_CanDrawPerspectiveTransformWithClips_Vulkan.png +impeller_Play_AiksTest_CanDrawPointsWithTextureMap_Metal.png impeller_Play_AiksTest_CanDrawPointsWithTextureMap_OpenGLES.png impeller_Play_AiksTest_CanDrawPointsWithTextureMap_Vulkan.png +impeller_Play_AiksTest_CanDrawPoints_Metal.png impeller_Play_AiksTest_CanDrawPoints_OpenGLES.png impeller_Play_AiksTest_CanDrawPoints_Vulkan.png impeller_Play_AiksTest_CanEmptyPictureConvertToImage_Metal.png