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

[Impeller] fix clip culling with exp canvas. #54701

Merged
merged 4 commits into from
Aug 28, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
17 changes: 17 additions & 0 deletions impeller/aiks/experimental_canvas.cc
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,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"
Expand Down Expand Up @@ -431,6 +432,14 @@ void ExperimentalCanvas::SaveLayer(
entry.rendering_mode = Entity::RenderingMode::kSubpassAppendSnapshotTransform;
transform_stack_.emplace_back(entry);

// The DisplayList bounds/rtree doesn't account for filters applied to parent
// layers, and so sub-DisplayLists are getting culled as if no filters are
// applied.
// See also: https://github.com/flutter/flutter/issues/139294
if (paint.image_filter) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Once we switch to exp canvas we can delete the cull rect and just use the clip stack which is 99% the same data.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the intention here is that if something outside of a DL applies an image filter then you need to cull the sub-dl by the "GetSourceCoverage" of that filter. No, it doesn't take that into account, the caller specifying the cull rect was supposed to...

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, wait, layers. We do expand the bounds of sub-layers by a filter applied on a parent layer...

void DisplayListBuilder::TransferLayerBounds(const SkRect& content_bounds) {

If rtree, then here:

if (AdjustRTreeRects(rtree_data_.value(), *filter, matrix, clip,

And if no rtree, then here:

if (!filter->map_device_bounds(global_ibounds, matrix, global_ibounds)) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This comment isn't really accurate. I updated it to document that the cull rect logic gets deleted with new canvas.

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,
Expand Down Expand Up @@ -774,6 +783,14 @@ void ExperimentalCanvas::AddClipEntityToCurrentPass(Entity entity) {
current_clip_coverage->Shift(-GetGlobalPassPosition());
}

// Skip rendering the clip if it is fully contained by the current render
// target.
if (entity.GetContents()->CanSkip(
render_passes_.back().inline_pass_context->GetTexture()->GetSize(),
entity.GetTransform())) {
return;
}

auto clip_coverage = entity.GetClipCoverage(current_clip_coverage);
if (clip_coverage.coverage.has_value()) {
clip_coverage.coverage =
Expand Down
27 changes: 16 additions & 11 deletions impeller/entity/contents/clip_contents.cc
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,20 @@ bool ClipContents::CanInheritOpacity(const Entity& entity) const {

void ClipContents::SetInheritedOpacity(Scalar opacity) {}

bool ClipContents::CanSkip(ISize render_pass_size, const Matrix& ctm) const {
if (clip_op_ == Entity::ClipOperation::kIntersect &&
geometry_->IsAxisAlignedRect() && ctm.IsTranslationScaleOnly()) {
std::optional<Rect> coverage = geometry_->GetCoverage(ctm);
if (coverage.has_value() &&
coverage->Contains(Rect::MakeSize(render_pass_size))) {
// Skip axis-aligned intersect clips that cover the whole render target
// since they won't draw anything to the depth buffer.
Copy link
Contributor

Choose a reason for hiding this comment

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

The DL code can deal with non-axis alignment but unfortunately it doesn't have access to this information. If we are axis aligned then this is the fast answer, but if we aren't then a slightly slower answer is to reverse transform the existin clip bounds corners by the ctm and then see if they are inside the geometry.

See:

bool DisplayListMatrixClipState::rect_covers_cull(const DlRect& content) const {

Also, it uses a different test - IsAligned2D instead of IsTranslationScaleOnly. Aligned allows for 90 degree rotations I think.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, why only do this on the render pass size? Why not skip it if it covers the entire existing clip bounds?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm, we actually are also checking the clip bounds. These should already include the render pass size.

https://github.com/flutter/engine/blob/main/impeller/aiks/experimental_canvas.cc#L769-L797

Maybe this logic isn't necessary at all

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh, so the problem is that we always report the clip coverage as changing:

https://github.com/flutter/engine/blob/main/impeller/entity/entity_pass_clip_stack.cc#L69-L82

Even if the new clip coverage is bigger than the old clip coverage. We could move this check into the clip coverage stack and simplify this everywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, lets just move this logic into the clip coverage stack

return true;
}
}
return false;
}

bool ClipContents::Render(const ContentContext& renderer,
const Entity& entity,
RenderPass& pass) const {
Expand All @@ -91,17 +105,8 @@ bool ClipContents::Render(const ContentContext& renderer,

using VS = ClipPipeline::VertexShader;

if (clip_op_ == Entity::ClipOperation::kIntersect &&
geometry_->IsAxisAlignedRect() &&
entity.GetTransform().IsTranslationScaleOnly()) {
std::optional<Rect> 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;
}
if (CanSkip(pass.GetRenderTargetSize(), entity.GetTransform())) {
return true;
}

VS::FrameInfo info;
Expand Down
2 changes: 2 additions & 0 deletions impeller/entity/contents/clip_contents.h
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ class ClipContents final : public Contents {

void SetClipOperation(Entity::ClipOperation clip_op);

bool CanSkip(ISize render_pass_size, const Matrix& transform) const override;

// |Contents|
std::optional<Rect> GetCoverage(const Entity& entity) const override;

Expand Down
4 changes: 4 additions & 0 deletions impeller/entity/contents/contents.cc
Original file line number Diff line number Diff line change
Expand Up @@ -182,4 +182,8 @@ void Contents::SetColorSourceSize(Size size) {
color_source_size_ = size;
}

bool Contents::CanSkip(ISize render_pass_size, const Matrix& transform) const {
return false;
}

} // namespace impeller
7 changes: 7 additions & 0 deletions impeller/entity/contents/contents.h
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,13 @@ class Contents {
///
void SetCoverageHint(std::optional<Rect> coverage_hint);

//----------------------------------------------------------------------------
/// @brief Whether this entity can be entirely skipped during rendering.
///
/// TODO(jonahwilliams): remove this method which was only added for clipping
/// once experimental canvas lands.
virtual bool CanSkip(ISize render_pass_size, const Matrix& transform) const;

const std::optional<Rect>& GetCoverageHint() const;

//----------------------------------------------------------------------------
Expand Down
29 changes: 29 additions & 0 deletions impeller/entity/entity_unittests.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1532,6 +1532,35 @@ TEST_P(EntityTest, ClipContentsShouldRenderIsCorrect) {
}
}

TEST_P(EntityTest, ClipContentsCanSkip) {
// Intersect Clips can be skipped if the render target coverage is fully
// inside of them.

auto clip = std::make_shared<ClipContents>();
clip->SetGeometry(Geometry::MakeRect(Rect::MakeLTRB(0, 0, 100, 100)));
clip->SetClipOperation(Entity::ClipOperation::kIntersect);

// Fully Contained
EXPECT_TRUE(clip->CanSkip(ISize::MakeWH(50, 50), {}));

// Not Intersect
clip->SetClipOperation(Entity::ClipOperation::kDifference);
EXPECT_FALSE(clip->CanSkip(ISize::MakeWH(50, 50), {}));

// Not Contained
clip->SetClipOperation(Entity::ClipOperation::kIntersect);
EXPECT_FALSE(clip->CanSkip(ISize::MakeWH(200, 200), {}));

// Not Scale Translate
clip->SetClipOperation(Entity::ClipOperation::kIntersect);
EXPECT_FALSE(
clip->CanSkip(ISize::MakeWH(50, 50), Matrix::MakeRotationX(Radians(2))));

// Not Axis Aligned Rectangle.
clip->SetGeometry(Geometry::MakeOval(Rect::MakeLTRB(0, 0, 50, 50)));
EXPECT_FALSE(clip->CanSkip(ISize::MakeWH(50, 50), {}));
}

TEST_P(EntityTest, ClipContentsGetClipCoverageIsCorrect) {
// Intersection: No stencil coverage, no geometry.
{
Expand Down