From 31e4626fee00e3415eed02abb0c2ffec1379e7e5 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Thu, 14 Dec 2023 09:58:21 -0800 Subject: [PATCH 1/4] [Impeller] distinguish between no clear color and transparent black clear color. --- impeller/aiks/aiks_unittests.cc | 19 ++++++++++++++++++ impeller/aiks/paint_pass_delegate.cc | 1 - impeller/entity/entity_pass.cc | 30 +++++++++++++++++----------- impeller/entity/entity_pass.h | 6 +++++- 4 files changed, 42 insertions(+), 14 deletions(-) diff --git a/impeller/aiks/aiks_unittests.cc b/impeller/aiks/aiks_unittests.cc index c1c46ecde95f4..b60e96cd37e63 100644 --- a/impeller/aiks/aiks_unittests.cc +++ b/impeller/aiks/aiks_unittests.cc @@ -4555,5 +4555,24 @@ TEST_P(AiksTest, GaussianBlurWithoutDecalSupport) { ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture())); } +TEST_P(AiksTest, SubpassWithClearColorOptimization) { + Canvas canvas; + + // Use a non-srcOver blend mode to ensure that we don't detect this as an + // opacity peephole optimization. + canvas.SaveLayer( + {.color = Color::Blue().WithAlpha(0.5), .blend_mode = BlendMode::kSource}, + Rect::MakeLTRB(0, 0, 200, 200)); + canvas.DrawPaint( + {.color = Color::BlackTransparent(), .blend_mode = BlendMode::kSource}); + canvas.Restore(); + + canvas.SaveLayer( + {.color = Color::Blue(), .blend_mode = BlendMode::kDestinationOver}); + canvas.Restore(); + + ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture())); +} + } // namespace testing } // namespace impeller diff --git a/impeller/aiks/paint_pass_delegate.cc b/impeller/aiks/paint_pass_delegate.cc index 2b249ed2a8e28..fe42af0b7c2aa 100644 --- a/impeller/aiks/paint_pass_delegate.cc +++ b/impeller/aiks/paint_pass_delegate.cc @@ -10,7 +10,6 @@ #include "impeller/entity/contents/texture_contents.h" #include "impeller/entity/entity_pass.h" #include "impeller/geometry/color.h" -#include "impeller/geometry/path_builder.h" namespace impeller { diff --git a/impeller/entity/entity_pass.cc b/impeller/entity/entity_pass.cc index 4ac61827c2865..86dd2f57e1e9a 100644 --- a/impeller/entity/entity_pass.cc +++ b/impeller/entity/entity_pass.cc @@ -342,7 +342,8 @@ bool EntityPass::Render(ContentContext& renderer, if (reads_from_onscreen_backdrop) { auto offscreen_target = CreateRenderTarget(renderer, root_render_target.GetRenderTargetSize(), - GetClearColor(render_target.GetRenderTargetSize())); + GetClearColor(render_target.GetRenderTargetSize()) + .value_or(Color::BlackTransparent())); if (!OnRender(renderer, // renderer capture, // capture @@ -447,7 +448,8 @@ bool EntityPass::Render(ContentContext& renderer, } // Set up the clear color of the root pass. - color0.clear_color = GetClearColor(render_target.GetRenderTargetSize()); + color0.clear_color = GetClearColor(render_target.GetRenderTargetSize()) + .value_or(Color::BlackTransparent()); root_render_target.SetColorAttachment(color0, 0); EntityPassTarget pass_target( @@ -600,9 +602,10 @@ EntityPass::EntityResult EntityPass::GetEntityForElement( } auto subpass_target = CreateRenderTarget( - renderer, // renderer - subpass_size, // size - subpass->GetClearColor(subpass_size)); // clear_color + renderer, // renderer + subpass_size, // size + subpass->GetClearColor(subpass_size) + .value_or(Color::BlackTransparent())); // clear_color if (!subpass_target.IsValid()) { VALIDATION_LOG << "Subpass render target is invalid."; @@ -845,8 +848,7 @@ bool EntityPass::OnRender( } auto clear_color_size = pass_target.GetRenderTarget().GetRenderTargetSize(); - if (!collapsed_parent_pass && - !GetClearColor(clear_color_size).IsTransparent()) { + if (!collapsed_parent_pass && GetClearColor(clear_color_size).has_value()) { // Force the pass context to create at least one new pass if the clear color // is present. pass_context.GetRenderPass(pass_depth); @@ -1139,21 +1141,25 @@ void EntityPass::SetBlendMode(BlendMode blend_mode) { flood_clip_ = Entity::IsBlendModeDestructive(blend_mode); } -Color EntityPass::GetClearColor(ISize target_size) const { - Color result = Color::BlackTransparent(); +std::optional EntityPass::GetClearColor(ISize target_size) const { if (backdrop_filter_proc_) { - return result; + return std::nullopt; } + std::optional result = std::nullopt; for (const Element& element : elements_) { auto [entity_color, blend_mode] = ElementAsBackgroundColor(element, target_size); if (!entity_color.has_value()) { break; } - result = result.Blend(entity_color.value(), blend_mode); + result = result.value_or(Color::BlackTransparent()) + .Blend(entity_color.value(), blend_mode); + } + if (result.has_value()) { + return result->Premultiply(); } - return result.Premultiply(); + return result; } void EntityPass::SetBackdropFilter(BackdropFilterProc proc) { diff --git a/impeller/entity/entity_pass.h b/impeller/entity/entity_pass.h index 1ff08f077ebba..92d46c7a5b07d 100644 --- a/impeller/entity/entity_pass.h +++ b/impeller/entity/entity_pass.h @@ -136,7 +136,11 @@ class EntityPass { void SetBlendMode(BlendMode blend_mode); - Color GetClearColor(ISize size = ISize::Infinite()) const; + /// @brief Return the clear color of the pass entities. + /// + /// If the returned value is std::nullopt, the clear color can be assumed to + /// be transparent black. + std::optional GetClearColor(ISize size = ISize::Infinite()) const; void SetBackdropFilter(BackdropFilterProc proc); From aa39960420353ba49baba9940ed38de44a620bc7 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Thu, 14 Dec 2023 10:15:39 -0800 Subject: [PATCH 2/4] ++ --- impeller/aiks/aiks_unittests.cc | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/impeller/aiks/aiks_unittests.cc b/impeller/aiks/aiks_unittests.cc index b60e96cd37e63..9baa4c8649796 100644 --- a/impeller/aiks/aiks_unittests.cc +++ b/impeller/aiks/aiks_unittests.cc @@ -2926,19 +2926,18 @@ TEST_P(AiksTest, ClearColorOptimizationDoesNotApplyForBackdropFilters) { Picture picture = canvas.EndRecordingAsPicture(); std::optional actual_color; + bool found_subpass = false; picture.pass->IterateAllElements([&](EntityPass::Element& element) -> bool { if (auto subpass = std::get_if>(&element)) { actual_color = subpass->get()->GetClearColor(); + found_subpass = true; } // Fail if the first element isn't a subpass. return true; }); - ASSERT_TRUE(actual_color.has_value()); - if (!actual_color) { - return; - } - ASSERT_EQ(actual_color.value(), Color::BlackTransparent()); + EXPECT_TRUE(found_subpass); + EXPECT_FALSE(actual_color.has_value()); } TEST_P(AiksTest, CollapsedDrawPaintInSubpass) { From 831663976a1ec7a49b21daa63eb52dcce2f34f75 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Thu, 14 Dec 2023 12:19:59 -0800 Subject: [PATCH 3/4] aaron review. --- impeller/aiks/aiks_unittests.cc | 3 +++ impeller/entity/entity_pass.cc | 16 ++++++++-------- impeller/entity/entity_pass.h | 10 ++++++---- 3 files changed, 17 insertions(+), 12 deletions(-) diff --git a/impeller/aiks/aiks_unittests.cc b/impeller/aiks/aiks_unittests.cc index 9baa4c8649796..d04c102548525 100644 --- a/impeller/aiks/aiks_unittests.cc +++ b/impeller/aiks/aiks_unittests.cc @@ -4570,6 +4570,9 @@ TEST_P(AiksTest, SubpassWithClearColorOptimization) { {.color = Color::Blue(), .blend_mode = BlendMode::kDestinationOver}); canvas.Restore(); + // This playground should appear blank on CI since we are only drawing + // transparent black. If the clear color optimization is broken, the texture + // will be filled with NaNs and may produce a magenta texture on macOS or iOS. ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture())); } diff --git a/impeller/entity/entity_pass.cc b/impeller/entity/entity_pass.cc index 86dd2f57e1e9a..13442eb55faf9 100644 --- a/impeller/entity/entity_pass.cc +++ b/impeller/entity/entity_pass.cc @@ -19,7 +19,6 @@ #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/solid_color_contents.h" #include "impeller/entity/contents/texture_contents.h" #include "impeller/entity/entity.h" #include "impeller/entity/inline_pass_context.h" @@ -342,8 +341,7 @@ bool EntityPass::Render(ContentContext& renderer, if (reads_from_onscreen_backdrop) { auto offscreen_target = CreateRenderTarget(renderer, root_render_target.GetRenderTargetSize(), - GetClearColor(render_target.GetRenderTargetSize()) - .value_or(Color::BlackTransparent())); + GetClearColorOrDefault(render_target.GetRenderTargetSize())); if (!OnRender(renderer, // renderer capture, // capture @@ -448,8 +446,7 @@ bool EntityPass::Render(ContentContext& renderer, } // Set up the clear color of the root pass. - color0.clear_color = GetClearColor(render_target.GetRenderTargetSize()) - .value_or(Color::BlackTransparent()); + color0.clear_color = GetClearColorOrDefault(render_target.GetRenderTargetSize()); root_render_target.SetColorAttachment(color0, 0); EntityPassTarget pass_target( @@ -604,8 +601,7 @@ EntityPass::EntityResult EntityPass::GetEntityForElement( auto subpass_target = CreateRenderTarget( renderer, // renderer subpass_size, // size - subpass->GetClearColor(subpass_size) - .value_or(Color::BlackTransparent())); // clear_color + subpass->GetClearColorOrDefault(subpass_size)); // clear_color if (!subpass_target.IsValid()) { VALIDATION_LOG << "Subpass render target is invalid."; @@ -960,7 +956,7 @@ bool EntityPass::OnRender( // If all previous elements were skipped due to clear color // optimization, then provide the clear color as the foreground of the // advanced blend. - foreground_color = GetClearColor(clear_color_size); + foreground_color = GetClearColorOrDefault(clear_color_size); coverage = Rect::MakeSize(clear_color_size); } else { coverage = result.entity.GetCoverage(); @@ -1141,6 +1137,10 @@ void EntityPass::SetBlendMode(BlendMode blend_mode) { flood_clip_ = Entity::IsBlendModeDestructive(blend_mode); } +Color EntityPass::GetClearColorOrDefault(ISize size) const { + return GetClearColor(size).value_or(Color::BlackTransparent()); +} + std::optional EntityPass::GetClearColor(ISize target_size) const { if (backdrop_filter_proc_) { return std::nullopt; diff --git a/impeller/entity/entity_pass.h b/impeller/entity/entity_pass.h index 92d46c7a5b07d..39f5449ff323b 100644 --- a/impeller/entity/entity_pass.h +++ b/impeller/entity/entity_pass.h @@ -136,12 +136,14 @@ class EntityPass { void SetBlendMode(BlendMode blend_mode); - /// @brief Return the clear color of the pass entities. - /// - /// If the returned value is std::nullopt, the clear color can be assumed to - /// be transparent black. + /// @brief Return the premultiplied clear color of the pass entities, if any. std::optional GetClearColor(ISize size = ISize::Infinite()) const; + /// @brief Return the premultiplied clear color of the pass entities. + /// + /// If the entity pass has no clear color, this will return transparent black. + Color GetClearColorOrDefault(ISize size = ISize::Infinite()) const; + void SetBackdropFilter(BackdropFilterProc proc); void SetEnableOffscreenCheckerboard(bool enabled); From c761bb3f381e4d08b1c7865fdaaa639ef9f8a136 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Thu, 14 Dec 2023 12:21:14 -0800 Subject: [PATCH 4/4] ++ --- impeller/entity/entity_pass.cc | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/impeller/entity/entity_pass.cc b/impeller/entity/entity_pass.cc index 13442eb55faf9..f5131167517e6 100644 --- a/impeller/entity/entity_pass.cc +++ b/impeller/entity/entity_pass.cc @@ -339,9 +339,9 @@ bool EntityPass::Render(ContentContext& renderer, // and then blit the results onto the onscreen texture. If using this branch, // there's no need to set up a stencil attachment on the root render target. if (reads_from_onscreen_backdrop) { - auto offscreen_target = - CreateRenderTarget(renderer, root_render_target.GetRenderTargetSize(), - GetClearColorOrDefault(render_target.GetRenderTargetSize())); + auto offscreen_target = CreateRenderTarget( + renderer, root_render_target.GetRenderTargetSize(), + GetClearColorOrDefault(render_target.GetRenderTargetSize())); if (!OnRender(renderer, // renderer capture, // capture @@ -446,7 +446,8 @@ bool EntityPass::Render(ContentContext& renderer, } // Set up the clear color of the root pass. - color0.clear_color = GetClearColorOrDefault(render_target.GetRenderTargetSize()); + color0.clear_color = + GetClearColorOrDefault(render_target.GetRenderTargetSize()); root_render_target.SetColorAttachment(color0, 0); EntityPassTarget pass_target( @@ -599,8 +600,8 @@ EntityPass::EntityResult EntityPass::GetEntityForElement( } auto subpass_target = CreateRenderTarget( - renderer, // renderer - subpass_size, // size + renderer, // renderer + subpass_size, // size subpass->GetClearColorOrDefault(subpass_size)); // clear_color if (!subpass_target.IsValid()) {