From 218b4161944ff87a5be745d5efc185e7c47aebec Mon Sep 17 00:00:00 2001 From: Jonah Williams Date: Wed, 28 Aug 2024 16:36:09 -0700 Subject: [PATCH] [Impeller] fix clip culling with exp canvas. (#54701) Fixes performance problem where image filters break clip culling, and lack of clip culling stops the clear color optimization from firing. on the current canvas the cull rect computation is slightly incorrect, as we drop it as soon as we get a image filter. With the new canvas, we have the actual render target sizes, so we can correctly cull without it. After switching to experimental canvas, I will remove the cull rect field from the canvas stack entry - as the clip coverage stack already performs basically the same culling. This fixes the performance issue on the uncached zoom page transition where we lose the clear color optimization too early. --- impeller/aiks/experimental_canvas.cc | 8 ++ impeller/entity/contents/clip_contents.cc | 25 ++--- impeller/entity/contents/contents.h | 11 +++ impeller/entity/entity_pass_clip_stack.cc | 25 +++-- impeller/entity/entity_pass_unittests.cc | 109 ++++++++++++++++++++++ 5 files changed, 155 insertions(+), 23 deletions(-) diff --git a/impeller/aiks/experimental_canvas.cc b/impeller/aiks/experimental_canvas.cc index 47e9d21e66a01..799a20acf78ff 100644 --- a/impeller/aiks/experimental_canvas.cc +++ b/impeller/aiks/experimental_canvas.cc @@ -11,6 +11,7 @@ #include "impeller/base/validation.h" #include "impeller/core/allocator.h" #include "impeller/core/formats.h" +#include "impeller/entity/contents/clip_contents.h" #include "impeller/entity/contents/framebuffer_blend_contents.h" #include "impeller/entity/contents/text_contents.h" #include "impeller/entity/entity.h" @@ -442,6 +443,12 @@ void ExperimentalCanvas::SaveLayer( entry.rendering_mode = Entity::RenderingMode::kSubpassAppendSnapshotTransform; transform_stack_.emplace_back(entry); + // The current clip aiks clip culling can not handle image filters. + // Remove this once we've migrated to exp canvas and removed it. + if (paint.image_filter) { + transform_stack_.back().cull_rect = std::nullopt; + } + // Start non-collapsed subpasses with a fresh clip coverage stack limited by // the subpass coverage. This is important because image filters applied to // save layers may transform the subpass texture after it's rendered, @@ -764,6 +771,7 @@ void ExperimentalCanvas::AddClipEntityToCurrentPass(Entity entity) { auto transform = entity.GetTransform(); entity.SetTransform( Matrix::MakeTranslation(Vector3(-GetGlobalPassPosition())) * transform); + // Ideally the clip depth would be greater than the current rendering // depth because any rendering calls that follow this clip operation will // pre-increment the depth and then be rendering above our clip depth, diff --git a/impeller/entity/contents/clip_contents.cc b/impeller/entity/contents/clip_contents.cc index f13f452734732..cd1fc5c863cff 100644 --- a/impeller/entity/contents/clip_contents.cc +++ b/impeller/entity/contents/clip_contents.cc @@ -53,8 +53,11 @@ Contents::ClipCoverage ClipContents::GetClipCoverage( case Entity::ClipOperation::kDifference: // This can be optimized further by considering cases when the bounds of // the current stencil will shrink. - return {.type = ClipCoverage::Type::kAppend, - .coverage = current_clip_coverage}; + return { + .type = ClipCoverage::Type::kAppend, // + .is_difference_or_non_square = true, // + .coverage = current_clip_coverage // + }; case Entity::ClipOperation::kIntersect: if (!geometry_) { return {.type = ClipCoverage::Type::kAppend, .coverage = std::nullopt}; @@ -64,8 +67,9 @@ Contents::ClipCoverage ClipContents::GetClipCoverage( return {.type = ClipCoverage::Type::kAppend, .coverage = std::nullopt}; } return { - .type = ClipCoverage::Type::kAppend, - .coverage = current_clip_coverage->Intersection(coverage.value()), + .type = ClipCoverage::Type::kAppend, // + .is_difference_or_non_square = !geometry_->IsAxisAlignedRect(), // + .coverage = current_clip_coverage->Intersection(coverage.value()), // }; } FML_UNREACHABLE(); @@ -91,19 +95,6 @@ bool ClipContents::Render(const ContentContext& renderer, using VS = ClipPipeline::VertexShader; - if (clip_op_ == Entity::ClipOperation::kIntersect && - geometry_->IsAxisAlignedRect() && - entity.GetTransform().IsTranslationScaleOnly()) { - std::optional coverage = - geometry_->GetCoverage(entity.GetTransform()); - if (coverage.has_value() && - coverage->Contains(Rect::MakeSize(pass.GetRenderTargetSize()))) { - // Skip axis-aligned intersect clips that cover the whole render target - // since they won't draw anything to the depth buffer. - return true; - } - } - VS::FrameInfo info; info.depth = GetShaderClipDepth(entity); diff --git a/impeller/entity/contents/contents.h b/impeller/entity/contents/contents.h index cc0cdeb65c8fd..6ee63e1bc7217 100644 --- a/impeller/entity/contents/contents.h +++ b/impeller/entity/contents/contents.h @@ -38,6 +38,17 @@ class Contents { enum class Type { kNoChange, kAppend, kRestore }; Type type = Type::kNoChange; + // TODO(jonahwilliams): this should probably use the Entity::ClipOperation + // enum, but that has transitive import errors. + bool is_difference_or_non_square = false; + + /// @brief This coverage is the outer coverage of the clip. + /// + /// For example, if the clip is a circular clip, this is the rectangle that + /// contains the circle and not the rectangle that is contained within the + /// circle. This means that we cannot use the coverage alone to determine if + /// a clip can be culled, and instead also use the somewhat hacky + /// "is_difference_or_non_square" field. std::optional coverage = std::nullopt; }; diff --git a/impeller/entity/entity_pass_clip_stack.cc b/impeller/entity/entity_pass_clip_stack.cc index 8e6aa1e8d625f..1f3749ce3b296 100644 --- a/impeller/entity/entity_pass_clip_stack.cc +++ b/impeller/entity/entity_pass_clip_stack.cc @@ -64,7 +64,7 @@ EntityPassClipStack::ClipStateResult EntityPassClipStack::ApplyClipState( case Contents::ClipCoverage::Type::kNoChange: break; case Contents::ClipCoverage::Type::kAppend: { - auto op = CurrentClipCoverage(); + auto maybe_coverage = CurrentClipCoverage(); // Compute the previous clip height. size_t previous_clip_height = 0; @@ -76,6 +76,24 @@ EntityPassClipStack::ClipStateResult EntityPassClipStack::ApplyClipState( previous_clip_height = clip_height_floor; } + if (!maybe_coverage.has_value()) { + // Running this append op won't impact the clip buffer because the + // whole screen is already being clipped, so skip it. + return result; + } + auto op = maybe_coverage.value(); + + // If the new clip coverage is bigger than the existing coverage for + // intersect clips, we do not need to change the clip region. + if (!global_clip_coverage.is_difference_or_non_square && + global_clip_coverage.coverage.has_value() && + global_clip_coverage.coverage.value().Contains(op)) { + subpass_state.clip_coverage.push_back(ClipCoverageLayer{ + .coverage = op, .clip_height = previous_clip_height + 1}); + + return result; + } + subpass_state.clip_coverage.push_back( ClipCoverageLayer{.coverage = global_clip_coverage.coverage, .clip_height = previous_clip_height + 1}); @@ -85,11 +103,6 @@ EntityPassClipStack::ClipStateResult EntityPassClipStack::ApplyClipState( subpass_state.clip_coverage.front().clip_height + subpass_state.clip_coverage.size() - 1); - if (!op.has_value()) { - // Running this append op won't impact the clip buffer because the - // whole screen is already being clipped, so skip it. - return result; - } } break; case Contents::ClipCoverage::Type::kRestore: { ClipRestoreContents* restore_contents = diff --git a/impeller/entity/entity_pass_unittests.cc b/impeller/entity/entity_pass_unittests.cc index fd25599393a4e..2c24eab20c3fb 100644 --- a/impeller/entity/entity_pass_unittests.cc +++ b/impeller/entity/entity_pass_unittests.cc @@ -130,6 +130,115 @@ TEST(EntityPassClipStackTest, AppendAndRestoreClipCoverage) { EXPECT_EQ(recorder.GetReplayEntities().size(), 0u); } +// Append two clip coverages, the second is larger the first. This +// should result in the second clip not requiring any update. +TEST(EntityPassClipStackTest, AppendLargerClipCoverage) { + EntityPassClipStack recorder = + EntityPassClipStack(Rect::MakeLTRB(0, 0, 100, 100)); + + ASSERT_EQ(recorder.GetClipCoverageLayers().size(), 1u); + + // Push a clip. + Entity entity; + EntityPassClipStack::ClipStateResult result = recorder.ApplyClipState( + Contents::ClipCoverage{ + .type = Contents::ClipCoverage::Type::kAppend, + .coverage = Rect::MakeLTRB(50, 50, 55, 55), + }, + entity, 0, Point(0, 0)); + EXPECT_TRUE(result.should_render); + EXPECT_TRUE(result.clip_did_change); + + // Push a clip with larger coverage than the previous state. + result = recorder.ApplyClipState( + Contents::ClipCoverage{ + .type = Contents::ClipCoverage::Type::kAppend, + .coverage = Rect::MakeLTRB(0, 0, 100, 100), + }, + entity, 0, Point(0, 0)); + + EXPECT_FALSE(result.should_render); + EXPECT_FALSE(result.clip_did_change); +} + +// Since clip entities return the outer coverage we can only cull axis aligned +// rectangles and intersect clips. +TEST(EntityPassClipStackTest, + AppendLargerClipCoverageWithDifferenceOrNonSquare) { + EntityPassClipStack recorder = + EntityPassClipStack(Rect::MakeLTRB(0, 0, 100, 100)); + + ASSERT_EQ(recorder.GetClipCoverageLayers().size(), 1u); + + // Push a clip. + Entity entity; + EntityPassClipStack::ClipStateResult result = recorder.ApplyClipState( + Contents::ClipCoverage{ + .type = Contents::ClipCoverage::Type::kAppend, + .coverage = Rect::MakeLTRB(50, 50, 55, 55), + }, + entity, 0, Point(0, 0)); + EXPECT_TRUE(result.should_render); + EXPECT_TRUE(result.clip_did_change); + + // Push a clip with larger coverage than the previous state. + result = recorder.ApplyClipState( + Contents::ClipCoverage{ + .type = Contents::ClipCoverage::Type::kAppend, + .is_difference_or_non_square = true, + .coverage = Rect::MakeLTRB(0, 0, 100, 100), + }, + entity, 0, Point(0, 0)); + + EXPECT_TRUE(result.should_render); + EXPECT_TRUE(result.clip_did_change); +} + +TEST(EntityPassClipStackTest, AppendDecreasingSizeClipCoverage) { + EntityPassClipStack recorder = + EntityPassClipStack(Rect::MakeLTRB(0, 0, 100, 100)); + + ASSERT_EQ(recorder.GetClipCoverageLayers().size(), 1u); + + // Push Clips that shrink in size. All should be applied. + Entity entity; + + for (auto i = 1; i < 20; i++) { + EntityPassClipStack::ClipStateResult result = recorder.ApplyClipState( + Contents::ClipCoverage{ + .type = Contents::ClipCoverage::Type::kAppend, + .coverage = Rect::MakeLTRB(i, i, 100 - i, 100 - i), + }, + entity, 0, Point(0, 0)); + EXPECT_TRUE(result.should_render); + EXPECT_TRUE(result.clip_did_change); + EXPECT_EQ(recorder.CurrentClipCoverage(), + Rect::MakeLTRB(i, i, 100 - i, 100 - i)); + } +} + +TEST(EntityPassClipStackTest, AppendIncreasingSizeClipCoverage) { + EntityPassClipStack recorder = + EntityPassClipStack(Rect::MakeLTRB(0, 0, 100, 100)); + + ASSERT_EQ(recorder.GetClipCoverageLayers().size(), 1u); + + // Push Clips that grow in size. All should be skipped. + Entity entity; + + for (auto i = 1; i < 20; i++) { + EntityPassClipStack::ClipStateResult result = recorder.ApplyClipState( + Contents::ClipCoverage{ + .type = Contents::ClipCoverage::Type::kAppend, + .coverage = Rect::MakeLTRB(0 - i, 0 - i, 100 + i, 100 + i), + }, + entity, 0, Point(0, 0)); + EXPECT_FALSE(result.should_render); + EXPECT_FALSE(result.clip_did_change); + EXPECT_EQ(recorder.CurrentClipCoverage(), Rect::MakeLTRB(0, 0, 100, 100)); + } +} + TEST(EntityPassClipStackTest, UnbalancedRestore) { EntityPassClipStack recorder = EntityPassClipStack(Rect::MakeLTRB(0, 0, 100, 100));