Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[Impeller] Refactor clip stack into separate testable class. #51656

Merged
auto-submit merged 13 commits into from
Mar 26, 2024
Merged

[Impeller] Refactor clip stack into separate testable class. #51656

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
60b868e
[Impeller] fix unbalanced restores.
Mar 24, 2024
4e8357a
Merge branch 'main' into fix_unbalanced_restore
Mar 25, 2024
7d56494
update licenses
Mar 25, 2024
c9509d8
Merge branch 'main' of github.com:flutter/engine into fix_unbalanced_…
Mar 25, 2024
815a370
Merge branch 'fix_unbalanced_restore' of github.com:jonahwilliams/eng…
Mar 25, 2024
8dbe8e8
Merge branch 'main' of github.com:flutter/engine into fix_unbalanced_…
Mar 25, 2024
0a60ba8
[Impeller] refactor clip stack and record/replay into separate class.
Mar 25, 2024
5cfbdb3
minor cleanups.
Mar 25, 2024
2c3fc3f
make clip stack share state.
Mar 25, 2024
29e56fd
remove unique ptr.
Mar 25, 2024
769c95c
fix clip values and add license files.
Mar 25, 2024
be351fd
Merge branch 'main' into refactor_clip_stack
Mar 26, 2024
b13cc79
Merge branch 'main' into refactor_clip_stack
Mar 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions ci/licenses_golden/licenses_flutter
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39694,6 +39694,8 @@ ORIGIN: ../../../flutter/impeller/entity/entity.cc + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/impeller/entity/entity.h + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/impeller/entity/entity_pass.cc + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/impeller/entity/entity_pass.h + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/impeller/entity/entity_pass_clip_stack.cc + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/impeller/entity/entity_pass_clip_stack.h + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/impeller/entity/entity_pass_delegate.cc + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/impeller/entity/entity_pass_delegate.h + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/impeller/entity/entity_pass_target.cc + ../../../flutter/LICENSE
Expand Down Expand Up @@ -42567,6 +42569,8 @@ FILE: ../../../flutter/impeller/entity/entity.cc
FILE: ../../../flutter/impeller/entity/entity.h
FILE: ../../../flutter/impeller/entity/entity_pass.cc
FILE: ../../../flutter/impeller/entity/entity_pass.h
FILE: ../../../flutter/impeller/entity/entity_pass_clip_stack.cc
FILE: ../../../flutter/impeller/entity/entity_pass_clip_stack.h
FILE: ../../../flutter/impeller/entity/entity_pass_delegate.cc
FILE: ../../../flutter/impeller/entity/entity_pass_delegate.h
FILE: ../../../flutter/impeller/entity/entity_pass_target.cc
Expand Down
17 changes: 0 additions & 17 deletions impeller/aiks/aiks_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3376,23 +3376,6 @@ TEST_P(AiksTest, CorrectClipDepthAssignedToEntities) {
}
}

TEST_P(AiksTest, EntityPassClipRecorderRestoresCancelOutClips) {
Canvas canvas;
canvas.Save();
canvas.ClipRRect(Rect::MakeLTRB(0, 0, 50, 50), {10, 10}, {});
canvas.DrawRRect(Rect::MakeLTRB(0, 0, 100, 100), {10, 10}, {});
canvas.Restore();
canvas.DrawRRect(Rect::MakeLTRB(0, 0, 50, 50), {10, 10}, {});

Picture picture = canvas.EndRecordingAsPicture();

AiksContext renderer(GetContext(), nullptr);
std::shared_ptr<Image> image = picture.ToImage(renderer, {300, 300});

EXPECT_EQ(
picture.pass->GetEntityPassClipRecorder().GetReplayEntities().size(), 0u);
}

} // namespace testing
} // namespace impeller

Expand Down
2 changes: 2 additions & 0 deletions impeller/entity/BUILD.gn
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,8 @@ impeller_component("entity") {
"entity.h",
"entity_pass.cc",
"entity_pass.h",
"entity_pass_clip_stack.cc",
"entity_pass_clip_stack.h",
"entity_pass_delegate.cc",
"entity_pass_delegate.h",
"entity_pass_target.cc",
Expand Down
137 changes: 22 additions & 115 deletions impeller/entity/entity_pass.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,13 @@
#include "impeller/base/strings.h"
#include "impeller/base/validation.h"
#include "impeller/core/formats.h"
#include "impeller/entity/contents/clip_contents.h"
#include "impeller/entity/contents/content_context.h"
#include "impeller/entity/contents/filters/color_filter_contents.h"
#include "impeller/entity/contents/filters/inputs/filter_input.h"
#include "impeller/entity/contents/framebuffer_blend_contents.h"
#include "impeller/entity/contents/texture_contents.h"
#include "impeller/entity/entity.h"
#include "impeller/entity/entity_pass_clip_stack.h"
#include "impeller/entity/inline_pass_context.h"
#include "impeller/geometry/color.h"
#include "impeller/geometry/rect.h"
Expand Down Expand Up @@ -409,9 +409,8 @@ bool EntityPass::Render(ContentContext& renderer,
return true;
});

ClipCoverageStack clip_coverage_stack = {ClipCoverageLayer{
.coverage = Rect::MakeSize(root_render_target.GetRenderTargetSize()),
.clip_depth = 0}};
EntityPassClipStack clip_stack = EntityPassClipStack(
Rect::MakeSize(root_render_target.GetRenderTargetSize()));
Copy link
Member

@bdero bdero Mar 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This patch moves from per-pass tracking of replay clips to a single list of tracked replay clips reused across all passes. So when replaying clips we need to be careful that only clips which impact the current pass are replayed, otherwise there may be perf issues if StC is turned on and fidelity bugs if StC is turned off.

Copy link
Member

@bdero bdero Mar 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, taking a closer look, maybe you're already handling this correctly. I see the state is held in a per-pass vector.

Copy link
Contributor Author

@jonahwilliams jonahwilliams Mar 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made this mistake initially but I believe thatI've fully corrected it. The clip stacks and record/replay entities are stored per-subpass, and for each subpass we push/pop a new set of data.


bool reads_from_onscreen_backdrop = GetTotalPassReads(renderer) > 0;
// In this branch path, we need to render everything to an offscreen texture
Expand All @@ -431,7 +430,7 @@ bool EntityPass::Render(ContentContext& renderer,
Point(), // global_pass_position
Point(), // local_pass_position
0, // pass_depth
clip_coverage_stack // clip_coverage_stack
clip_stack // clip_coverage_stack
)) {
// Validation error messages are triggered for all `OnRender()` failure
// cases.
Expand Down Expand Up @@ -537,7 +536,7 @@ bool EntityPass::Render(ContentContext& renderer,
Point(), // global_pass_position
Point(), // local_pass_position
0, // pass_depth
clip_coverage_stack); // clip_coverage_stack
clip_stack); // clip_coverage_stack
}

EntityPass::EntityResult EntityPass::GetEntityForElement(
Expand All @@ -548,7 +547,7 @@ EntityPass::EntityResult EntityPass::GetEntityForElement(
ISize root_pass_size,
Point global_pass_position,
uint32_t pass_depth,
ClipCoverageStack& clip_coverage_stack,
EntityPassClipStack& clip_coverage_stack,
size_t clip_depth_floor) const {
//--------------------------------------------------------------------------
/// Setup entity element.
Expand Down Expand Up @@ -625,13 +624,13 @@ EntityPass::EntityResult EntityPass::GetEntityForElement(
pass_context.EndPass();
}

if (clip_coverage_stack.empty()) {
if (!clip_coverage_stack.HasCoverage()) {
// The current clip is empty. This means the pass texture won't be
// visible, so skip it.
capture.CreateChild("Subpass Entity (Skipped: Empty clip A)");
return EntityPass::EntityResult::Skip();
}
auto clip_coverage_back = clip_coverage_stack.back().coverage;
auto clip_coverage_back = clip_coverage_stack.CurrentClipCoverage();
if (!clip_coverage_back.has_value()) {
capture.CreateChild("Subpass Entity (Skipped: Empty clip B)");
return EntityPass::EntityResult::Skip();
Expand Down Expand Up @@ -690,8 +689,7 @@ EntityPass::EntityResult EntityPass::GetEntityForElement(
// save layers may transform the subpass texture after it's rendered,
// causing parent clip coverage to get misaligned with the actual area that
// the subpass will affect in the parent pass.
ClipCoverageStack subpass_clip_coverage_stack = {ClipCoverageLayer{
.coverage = subpass_coverage, .clip_depth = subpass->clip_depth_}};
clip_coverage_stack.PushSubpass(subpass_coverage, subpass->clip_depth_);

// Stencil textures aren't shared between EntityPasses (as much of the
// time they are transient).
Expand All @@ -704,7 +702,7 @@ EntityPass::EntityResult EntityPass::GetEntityForElement(
subpass_coverage->GetOrigin() -
global_pass_position, // local_pass_position
++pass_depth, // pass_depth
subpass_clip_coverage_stack, // clip_coverage_stack
clip_coverage_stack, // clip_coverage_stack
subpass->clip_depth_, // clip_depth_floor
subpass_backdrop_filter_contents // backdrop_filter_contents
)) {
Expand All @@ -713,6 +711,8 @@ EntityPass::EntityResult EntityPass::GetEntityForElement(
return EntityPass::EntityResult::Failure();
}

clip_coverage_stack.PopSubpass();

// The subpass target's texture may have changed during OnRender.
auto subpass_texture =
subpass_target.GetRenderTarget().GetRenderTargetTexture();
Expand Down Expand Up @@ -757,7 +757,7 @@ bool EntityPass::RenderElement(Entity& element_entity,
InlinePassContext& pass_context,
int32_t pass_depth,
ContentContext& renderer,
ClipCoverageStack& clip_coverage_stack,
EntityPassClipStack& clip_coverage_stack,
Point global_pass_position) const {
auto result = pass_context.GetRenderPass(pass_depth);
if (!result.pass) {
Expand All @@ -770,7 +770,7 @@ bool EntityPass::RenderElement(Entity& element_entity,
if (result.just_created) {
// Restore any clips that were recorded before the backdrop filter was
// applied.
auto& replay_entities = clip_replay_->GetReplayEntities();
auto& replay_entities = clip_coverage_stack.GetReplayEntities();
for (const auto& entity : replay_entities) {
if (!entity.Render(renderer, *result.pass)) {
VALIDATION_LOG << "Failed to render entity for clip restore.";
Expand Down Expand Up @@ -801,7 +801,7 @@ bool EntityPass::RenderElement(Entity& element_entity,
}
}

auto current_clip_coverage = clip_coverage_stack.back().coverage;
auto current_clip_coverage = clip_coverage_stack.CurrentClipCoverage();
if (current_clip_coverage.has_value()) {
// Entity transforms are relative to the current pass position, so we need
// to check clip coverage in the same space.
Expand All @@ -826,81 +826,14 @@ bool EntityPass::RenderElement(Entity& element_entity,
element_entity.GetContents()->SetCoverageHint(
Rect::Intersection(element_coverage_hint, current_clip_coverage));

switch (clip_coverage.type) {
case Contents::ClipCoverage::Type::kNoChange:
break;
case Contents::ClipCoverage::Type::kAppend: {
auto op = clip_coverage_stack.back().coverage;
clip_coverage_stack.push_back(
ClipCoverageLayer{.coverage = clip_coverage.coverage,
.clip_depth = element_entity.GetClipDepth() + 1});
FML_DCHECK(clip_coverage_stack.back().clip_depth ==
clip_coverage_stack.front().clip_depth +
clip_coverage_stack.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 true;
}
} break;
case Contents::ClipCoverage::Type::kRestore: {
if (clip_coverage_stack.back().clip_depth <=
element_entity.GetClipDepth()) {
// Drop clip restores that will do nothing.
return true;
}

auto restoration_index = element_entity.GetClipDepth() -
clip_coverage_stack.front().clip_depth;
FML_DCHECK(restoration_index < clip_coverage_stack.size());

// We only need to restore the area that covers the coverage of the
// clip rect at target depth + 1.
std::optional<Rect> restore_coverage =
(restoration_index + 1 < clip_coverage_stack.size())
? clip_coverage_stack[restoration_index + 1].coverage
: std::nullopt;
if (restore_coverage.has_value()) {
// Make the coverage rectangle relative to the current pass.
restore_coverage = restore_coverage->Shift(-global_pass_position);
}
clip_coverage_stack.resize(restoration_index + 1);

if constexpr (ContentContext::kEnableStencilThenCover) {
// Skip all clip restores when stencil-then-cover is enabled.
if (clip_coverage_stack.back().coverage.has_value()) {
clip_replay_->RecordEntity(element_entity, clip_coverage.type);
}
return true;
}

if (!clip_coverage_stack.back().coverage.has_value()) {
// Running this restore op won't make anything renderable, so skip it.
return true;
}

auto restore_contents =
static_cast<ClipRestoreContents*>(element_entity.GetContents().get());
restore_contents->SetRestoreCoverage(restore_coverage);

} break;
}

#ifdef IMPELLER_ENABLE_CAPTURE
{
auto element_entity_coverage = element_entity.GetCoverage();
if (element_entity_coverage.has_value()) {
element_entity_coverage =
element_entity_coverage->Shift(global_pass_position);
element_entity.GetCapture().AddRect("Coverage", *element_entity_coverage,
{.readonly = true});
}
if (!clip_coverage_stack.AppendClipCoverage(clip_coverage, element_entity,
clip_depth_floor,
global_pass_position)) {
// If the entity's coverage change did not change the clip coverage, we
// don't need to render it.
return true;
Copy link
Contributor

@matanlurey matanlurey Mar 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe store the 1-line comment explaining why this state is terminal

Copy link
Contributor Author

@jonahwilliams jonahwilliams Mar 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

}
#endif

element_entity.SetClipDepth(element_entity.GetClipDepth() - clip_depth_floor);
clip_replay_->RecordEntity(element_entity, clip_coverage.type);
if (!element_entity.Render(renderer, *result.pass)) {
VALIDATION_LOG << "Failed to render entity.";
return false;
Expand All @@ -916,7 +849,7 @@ bool EntityPass::OnRender(
Point global_pass_position,
Point local_pass_position,
uint32_t pass_depth,
ClipCoverageStack& clip_coverage_stack,
EntityPassClipStack& clip_coverage_stack,
size_t clip_depth_floor,
std::shared_ptr<Contents> backdrop_filter_contents,
const std::optional<InlinePassContext::RenderPassResult>&
Expand Down Expand Up @@ -1256,30 +1189,4 @@ void EntityPass::SetEnableOffscreenCheckerboard(bool enabled) {
enable_offscreen_debug_checkerboard_ = enabled;
}

const EntityPassClipRecorder& EntityPass::GetEntityPassClipRecorder() const {
return *clip_replay_;
}

EntityPassClipRecorder::EntityPassClipRecorder() {}

void EntityPassClipRecorder::RecordEntity(const Entity& entity,
Contents::ClipCoverage::Type type) {
switch (type) {
case Contents::ClipCoverage::Type::kNoChange:
return;
case Contents::ClipCoverage::Type::kAppend:
rendered_clip_entities_.push_back(entity.Clone());
break;
case Contents::ClipCoverage::Type::kRestore:
if (!rendered_clip_entities_.empty()) {
rendered_clip_entities_.pop_back();
}
break;
}
}

const std::vector<Entity>& EntityPassClipRecorder::GetReplayEntities() const {
return rendered_clip_entities_;
}

} // namespace impeller
36 changes: 4 additions & 32 deletions impeller/entity/entity_pass.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#include "impeller/entity/contents/contents.h"
#include "impeller/entity/contents/filters/filter_contents.h"
#include "impeller/entity/entity.h"
#include "impeller/entity/entity_pass_clip_stack.h"
#include "impeller/entity/entity_pass_delegate.h"
#include "impeller/entity/inline_pass_context.h"
#include "impeller/renderer/render_target.h"
Expand Down Expand Up @@ -60,13 +61,6 @@ class EntityPass {
const Matrix& effect_transform,
Entity::RenderingMode rendering_mode)>;

struct ClipCoverageLayer {
std::optional<Rect> coverage;
size_t clip_depth;
};

using ClipCoverageStack = std::vector<ClipCoverageLayer>;

EntityPass();

~EntityPass();
Expand Down Expand Up @@ -252,7 +246,7 @@ class EntityPass {
InlinePassContext& pass_context,
int32_t pass_depth,
ContentContext& renderer,
ClipCoverageStack& clip_coverage_stack,
EntityPassClipStack& clip_coverage_stack,
Point global_pass_position) const;

EntityResult GetEntityForElement(const EntityPass::Element& element,
Expand All @@ -262,7 +256,7 @@ class EntityPass {
ISize root_pass_size,
Point global_pass_position,
uint32_t pass_depth,
ClipCoverageStack& clip_coverage_stack,
EntityPassClipStack& clip_coverage_stack,
size_t clip_depth_floor) const;

//----------------------------------------------------------------------------
Expand Down Expand Up @@ -329,7 +323,7 @@ class EntityPass {
Point global_pass_position,
Point local_pass_position,
uint32_t pass_depth,
ClipCoverageStack& clip_coverage_stack,
EntityPassClipStack& clip_coverage_stack,
size_t clip_depth_floor = 0,
std::shared_ptr<Contents> backdrop_filter_contents = nullptr,
const std::optional<InlinePassContext::RenderPassResult>&
Expand All @@ -354,8 +348,6 @@ class EntityPass {
bool enable_offscreen_debug_checkerboard_ = false;
std::optional<Rect> bounds_limit_;
ContentBoundsPromise bounds_promise_ = ContentBoundsPromise::kUnknown;
std::unique_ptr<EntityPassClipRecorder> clip_replay_ =
std::make_unique<EntityPassClipRecorder>();
int32_t required_mip_count_ = 1;

/// These values are incremented whenever something is added to the pass that
Expand All @@ -381,26 +373,6 @@ class EntityPass {
EntityPass& operator=(const EntityPass&) = delete;
};

/// @brief A class that tracks all clips that have been recorded in the current
/// entity pass stencil.
///
/// These clips are replayed when restoring the backdrop so that the
/// stencil buffer is left in an identical state.
class EntityPassClipRecorder {
public:
EntityPassClipRecorder();

~EntityPassClipRecorder() = default;

/// @brief Record the entity based on the provided coverage [type].
void RecordEntity(const Entity& entity, Contents::ClipCoverage::Type type);

const std::vector<Entity>& GetReplayEntities() const;

private:
std::vector<Entity> rendered_clip_entities_;
};

} // namespace impeller

#endif // FLUTTER_IMPELLER_ENTITY_ENTITY_PASS_H_
Loading