-
Notifications
You must be signed in to change notification settings - Fork 6k
[Impeller] Replace Impeller opacity peephole delegate with DL variant. #52707
Changes from 5 commits
9bdf84b
6deb635
3d5739a
326af19
d66b1a3
03f4ffb
0d36fe0
6a1a416
55ad6ee
74f8f84
49c7376
302e125
06ca207
88cd532
1f90efb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -494,30 +494,6 @@ TEST_P(AiksTest, CanEmptyPictureConvertToImage) { | |
ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture())); | ||
} | ||
|
||
TEST_P(AiksTest, CanRenderGroupOpacity) { | ||
Canvas canvas; | ||
|
||
Paint red; | ||
red.color = Color::Red(); | ||
Paint green; | ||
green.color = Color::Green().WithAlpha(0.5); | ||
Paint blue; | ||
blue.color = Color::Blue(); | ||
|
||
Paint alpha; | ||
alpha.color = Color::Red().WithAlpha(0.5); | ||
|
||
canvas.SaveLayer(alpha); | ||
|
||
canvas.DrawRect(Rect::MakeXYWH(000, 000, 100, 100), red); | ||
canvas.DrawRect(Rect::MakeXYWH(020, 020, 100, 100), green); | ||
canvas.DrawRect(Rect::MakeXYWH(040, 040, 100, 100), blue); | ||
|
||
canvas.Restore(); | ||
|
||
ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture())); | ||
} | ||
|
||
TEST_P(AiksTest, CoordinateConversionsAreCorrect) { | ||
Canvas canvas; | ||
|
||
|
@@ -1634,45 +1610,6 @@ TEST_P(AiksTest, PaintWithFilters) { | |
ASSERT_FALSE(paint.HasColorFilter()); | ||
} | ||
|
||
TEST_P(AiksTest, OpacityPeepHoleApplicationTest) { | ||
auto entity_pass = std::make_shared<EntityPass>(); | ||
auto rect = Rect::MakeLTRB(0, 0, 100, 100); | ||
Paint paint; | ||
paint.color = Color::White().WithAlpha(0.5); | ||
paint.color_filter = | ||
ColorFilter::MakeBlend(BlendMode::kSourceOver, Color::Blue()); | ||
|
||
// Paint has color filter, can't elide. | ||
auto delegate = std::make_shared<OpacityPeepholePassDelegate>(paint); | ||
ASSERT_FALSE(delegate->CanCollapseIntoParentPass(entity_pass.get())); | ||
|
||
paint.color_filter = nullptr; | ||
paint.image_filter = ImageFilter::MakeBlur(Sigma(1.0), Sigma(1.0), | ||
FilterContents::BlurStyle::kNormal, | ||
Entity::TileMode::kClamp); | ||
|
||
// Paint has image filter, can't elide. | ||
delegate = std::make_shared<OpacityPeepholePassDelegate>(paint); | ||
ASSERT_FALSE(delegate->CanCollapseIntoParentPass(entity_pass.get())); | ||
|
||
paint.image_filter = nullptr; | ||
paint.color = Color::Red(); | ||
|
||
// Paint has no alpha, can't elide; | ||
delegate = std::make_shared<OpacityPeepholePassDelegate>(paint); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Huh? Why can't opaque colors be peepholed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just old overly defensive code. |
||
ASSERT_FALSE(delegate->CanCollapseIntoParentPass(entity_pass.get())); | ||
|
||
// Positive test. | ||
Entity entity; | ||
entity.SetContents(SolidColorContents::Make( | ||
PathBuilder{}.AddRect(rect).TakePath(), Color::Red())); | ||
entity_pass->AddEntity(std::move(entity)); | ||
paint.color = Color::Red().WithAlpha(0.5); | ||
|
||
delegate = std::make_shared<OpacityPeepholePassDelegate>(paint); | ||
ASSERT_TRUE(delegate->CanCollapseIntoParentPass(entity_pass.get())); | ||
} | ||
|
||
TEST_P(AiksTest, DrawPaintAbsorbsClears) { | ||
Canvas canvas; | ||
canvas.DrawPaint({.color = Color::Red(), .blend_mode = BlendMode::kSource}); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -224,6 +224,7 @@ void Canvas::Save(bool create_subpass, | |
entry.transform = transform_stack_.back().transform; | ||
entry.cull_rect = transform_stack_.back().cull_rect; | ||
entry.clip_height = transform_stack_.back().clip_height; | ||
entry.distributed_opacity = transform_stack_.back().distributed_opacity; | ||
if (create_subpass) { | ||
entry.rendering_mode = Entity::RenderingMode::kSubpass; | ||
auto subpass = std::make_unique<EntityPass>(); | ||
|
@@ -827,6 +828,7 @@ void Canvas::AddRenderEntityToCurrentPass(Entity entity, bool reuse_depth) { | |
++current_depth_; | ||
} | ||
entity.SetClipDepth(current_depth_); | ||
entity.SetInheritedOpacity(transform_stack_.back().distributed_opacity); | ||
GetCurrentPass().AddEntity(std::move(entity)); | ||
} | ||
|
||
|
@@ -838,8 +840,16 @@ void Canvas::SaveLayer(const Paint& paint, | |
std::optional<Rect> bounds, | ||
const std::shared_ptr<ImageFilter>& backdrop_filter, | ||
ContentBoundsPromise bounds_promise, | ||
uint32_t total_content_depth) { | ||
uint32_t total_content_depth, | ||
bool can_distribute_opacity) { | ||
TRACE_EVENT0("flutter", "Canvas::saveLayer"); | ||
if (can_distribute_opacity) { | ||
FML_DCHECK(!backdrop_filter); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @flar I'm surprised but I hit this check in some tests. Do I need to assume that even if can_distribute_opacity is true, there may be other paint states (blend mode/ filters) that can still require a save layer? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Which tests? I can take a look and see if there might be something wrong with the DL checks. In particular, I think I only pass SrcOver for now even though other blend modes might cause problems. I disallow ColorFilters, but I allow ImageFilters because according to my investigations the opacity should be applied to the output of the ImageFilter so it shouldn't matter. (It's applied to the input of the ColorFilter, though, so I just disallow them even though I could maybe look at their details and find that some of them are compatible). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, are you specifically referring to backdrop filter vs opacity flag? I notice that I don't turn off the flag in that case, but perhaps I should. The flag is more about the contents and whether the contents are compatible. I also don't think I check the attributes that apply the SL to the parent. If they aren't SrcOver then I don't think we can do the opacity stuff. I also don't believe the "old code" (pre-reorg) does this either. Let me get the reorg back in and then I can revisit layer properties and how they affect the flag. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Filed flutter/flutter#148505 |
||
Save(false, total_content_depth, paint.blend_mode, backdrop_filter); | ||
transform_stack_.back().distributed_opacity *= paint.color.alpha; | ||
return; | ||
} | ||
|
||
Save(true, total_content_depth, paint.blend_mode, backdrop_filter); | ||
|
||
// The DisplayList bounds/rtree doesn't account for filters applied to parent | ||
|
@@ -861,13 +871,7 @@ void Canvas::SaveLayer(const Paint& paint, | |
new_layer_pass.SetRequiredMipCount(mip_count_visitor.GetRequiredMipCount()); | ||
} | ||
|
||
// Only apply opacity peephole on default blending. | ||
if (paint.blend_mode == BlendMode::kSourceOver) { | ||
new_layer_pass.SetDelegate( | ||
std::make_shared<OpacityPeepholePassDelegate>(paint)); | ||
} else { | ||
new_layer_pass.SetDelegate(std::make_shared<PaintPassDelegate>(paint)); | ||
} | ||
new_layer_pass.SetDelegate(std::make_shared<PaintPassDelegate>(paint)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this transfer the existing distributed opacity from the transform_stack.back() to the new layer pass? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Basically, if this saveLayer didn't pass along the distributed opacity above, then it needs to apply it to the blit-back to the parent and I don't see where that is happening. Should PaintPassDelegate carry along an extra parameter for the distributed opacity from outside the saveLayer? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch, this is fixed and a test case is added. |
||
} | ||
|
||
void Canvas::DrawTextFrame(const std::shared_ptr<TextFrame>& text_frame, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,7 +5,6 @@ | |
#include "impeller/aiks/paint_pass_delegate.h" | ||
|
||
#include "impeller/core/formats.h" | ||
#include "impeller/core/sampler_descriptor.h" | ||
#include "impeller/entity/contents/contents.h" | ||
#include "impeller/entity/contents/texture_contents.h" | ||
#include "impeller/entity/entity_pass.h" | ||
|
@@ -54,106 +53,4 @@ std::shared_ptr<FilterContents> PaintPassDelegate::WithImageFilter( | |
Entity::RenderingMode::kSubpass); | ||
} | ||
|
||
/// OpacityPeepholePassDelegate | ||
/// ---------------------------------------------- | ||
|
||
OpacityPeepholePassDelegate::OpacityPeepholePassDelegate(Paint paint) | ||
: paint_(std::move(paint)) {} | ||
|
||
// |EntityPassDelgate| | ||
OpacityPeepholePassDelegate::~OpacityPeepholePassDelegate() = default; | ||
|
||
// |EntityPassDelgate| | ||
bool OpacityPeepholePassDelegate::CanElide() { | ||
return paint_.blend_mode == BlendMode::kDestination; | ||
} | ||
|
||
// |EntityPassDelgate| | ||
bool OpacityPeepholePassDelegate::CanCollapseIntoParentPass( | ||
EntityPass* entity_pass) { | ||
// Passes with enforced bounds that clip the contents can not be safely | ||
// collapsed. | ||
if (entity_pass->GetBoundsLimitMightClipContent()) { | ||
return false; | ||
} | ||
|
||
// OpacityPeepholePassDelegate will only get used if the pass's blend mode is | ||
// SourceOver, so no need to check here. | ||
if (paint_.color.alpha <= 0.0 || paint_.color.alpha >= 1.0 || | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the alpha is 0 then we should skip the pass entirely, so I'm not sure why it is saying that it "can't collapse into parent" here (i.e. return false). Also, if the paint is opaque, then that isn't a reason not to collapse the pass...? It just means you have no partial opacity to distribute, but you can still collapse it and distribute the opacity of 1.0 (i.e. NOP on modifying the opacity). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I was just being overly conservative at the time. |
||
paint_.image_filter || paint_.color_filter) { | ||
return false; | ||
} | ||
|
||
// Note: determing whether any coverage intersects has quadradic complexity in | ||
// the number of rectangles, and depending on whether or not we cache at | ||
// different levels of the entity tree may end up cubic. In the interest of | ||
// proving whether or not this optimization is valuable, we only consider very | ||
// simple peephole optimizations here - where there is a single drawing | ||
// command wrapped in save layer. This would indicate something like an | ||
// Opacity or FadeTransition wrapping a very simple widget, like in the | ||
// CupertinoPicker. | ||
if (entity_pass->GetElementCount() > 3) { | ||
// Single paint command with a save layer would be: | ||
// 1. clip | ||
// 2. draw command | ||
// 3. restore. | ||
return false; | ||
} | ||
bool all_can_accept = true; | ||
std::vector<Rect> all_coverages; | ||
auto had_subpass = entity_pass->IterateUntilSubpass( | ||
[&all_coverages, &all_can_accept](Entity& entity) { | ||
const auto& contents = entity.GetContents(); | ||
if (!entity.CanInheritOpacity()) { | ||
all_can_accept = false; | ||
return false; | ||
} | ||
auto maybe_coverage = contents->GetCoverage(entity); | ||
if (maybe_coverage.has_value()) { | ||
auto coverage = maybe_coverage.value(); | ||
for (const auto& cv : all_coverages) { | ||
if (cv.IntersectsWithRect(coverage)) { | ||
all_can_accept = false; | ||
return false; | ||
} | ||
} | ||
all_coverages.push_back(coverage); | ||
} | ||
return true; | ||
}); | ||
if (had_subpass || !all_can_accept) { | ||
return false; | ||
} | ||
auto alpha = paint_.color.alpha; | ||
entity_pass->IterateUntilSubpass([&alpha](Entity& entity) { | ||
entity.SetInheritedOpacity(alpha); | ||
return true; | ||
}); | ||
return true; | ||
} | ||
|
||
// |EntityPassDelgate| | ||
std::shared_ptr<Contents> | ||
OpacityPeepholePassDelegate::CreateContentsForSubpassTarget( | ||
std::shared_ptr<Texture> target, | ||
const Matrix& effect_transform) { | ||
auto contents = TextureContents::MakeRect(Rect::MakeSize(target->GetSize())); | ||
contents->SetLabel("Subpass"); | ||
contents->SetTexture(target); | ||
contents->SetSourceRect(Rect::MakeSize(target->GetSize())); | ||
contents->SetOpacity(paint_.color.alpha); | ||
contents->SetDeferApplyingOpacity(true); | ||
|
||
return paint_.WithFiltersForSubpassTarget(std::move(contents), | ||
effect_transform); | ||
} | ||
|
||
// |EntityPassDelgate| | ||
std::shared_ptr<FilterContents> OpacityPeepholePassDelegate::WithImageFilter( | ||
const FilterInput::Variant& input, | ||
const Matrix& effect_transform) const { | ||
return paint_.WithImageFilter(input, effect_transform, | ||
Entity::RenderingMode::kSubpass); | ||
} | ||
|
||
} // namespace impeller |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be the case where I differ in assumptions. DL allows IF+opacity, but it looks like AIKS expects it to be disallowed...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We always fold the opacity into the image filter compositing, so in effect its as if the peephole wasn't applied.