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

[Impeller] Ensure that reused textures are cleared before getting sampled by backdrop textures #46084

Merged
bdero merged 2 commits into from
Sep 20, 2023
Merged

[Impeller] Ensure that reused textures are cleared before getting sampled by backdrop textures #46084

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
64d713b
[Impeller] Ensure that reused textures are cleared before getting sam…
bdero Sep 20, 2023
530277d
Fix build failure and add comment
bdero Sep 20, 2023
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
24 changes: 24 additions & 0 deletions impeller/aiks/aiks_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2264,6 +2264,30 @@ TEST_P(AiksTest, DrawPaintAbsorbsClears) {
ASSERT_EQ(render_pass->GetCommands().size(), 0llu);
}

// This is important to enforce with texture reuse, since cached textures need
// to be cleared before reuse.
TEST_P(AiksTest,
ParentSaveLayerCreatesRenderPassWhenChildBackdropFilterIsPresent) {
Canvas canvas;
canvas.SaveLayer({}, std::nullopt, ImageFilter::MakeMatrix(Matrix(), {}));
canvas.DrawPaint({.color = Color::Red(), .blend_mode = BlendMode::kSource});
canvas.DrawPaint({.color = Color::CornflowerBlue().WithAlpha(0.75),
.blend_mode = BlendMode::kSourceOver});
canvas.Restore();

Picture picture = canvas.EndRecordingAsPicture();

std::shared_ptr<ContextSpy> spy = ContextSpy::Make();
std::shared_ptr<Context> real_context = GetContext();
std::shared_ptr<ContextMock> mock_context = spy->MakeContext(real_context);
AiksContext renderer(mock_context, nullptr);
std::shared_ptr<Image> image = picture.ToImage(renderer, {300, 300});

ASSERT_EQ(spy->render_passes_.size(), 3llu);
std::shared_ptr<RenderPass> render_pass = spy->render_passes_[0];
ASSERT_EQ(render_pass->GetCommands().size(), 0llu);
}

TEST_P(AiksTest, DrawRectAbsorbsClears) {
Canvas canvas;
canvas.DrawRect({0, 0, 300, 300},
Expand Down
8 changes: 1 addition & 7 deletions impeller/entity/entity_pass.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -537,6 +537,7 @@ EntityPass::EntityResult EntityPass::GetEntityForElement(
// The subpass will need to read from the current pass texture when
// rendering the backdrop, so if there's an active pass, end it prior to
// rendering the subpass.
pass_context.GetRenderPass(pass_depth);
Copy link
Contributor

@jonahwilliams jonahwilliams Sep 20, 2023

Choose a reason for hiding this comment

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

I'm not sure I fully understand this case 🤔

Copy link
Member Author

@bdero bdero Sep 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

If the very first thing we render in this EntityPass is a subpass that happens to have a backdrop filter, than that backdrop filter may wind up sampling from the raw, uncleared texture that came straight out of the texture cache. By calling pass_context.GetRenderPass here, we force the texture to pass through at least one RenderPass with the correct clear configuration before any sampling occurs.

Added this to a comment inline.

Copy link
Contributor

@jonahwilliams jonahwilliams Sep 20, 2023

Choose a reason for hiding this comment

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

Oh, I see - so we're attaching the texture but we never created any rendering commands to record to it, so we never cleared the values. hmm. Slight flaw in the render target cache design

pass_context.EndPass();
}

Expand Down Expand Up @@ -680,13 +681,6 @@ bool EntityPass::OnRender(
return false;
}

if (!collapsed_parent_pass &&
!GetClearColor(root_pass_size).IsTransparent()) {
// Force the pass context to create at least one new pass if the clear color
// is present.
pass_context.GetRenderPass(pass_depth);
}

auto render_element = [&stencil_depth_floor, &pass_context, &pass_depth,
&renderer, &stencil_coverage_stack,
&global_pass_position](Entity& element_entity) {
Expand Down