New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Impeller] Improve performance of CupertinoPicker with opacity peephole #40101
Changes from 3 commits
7f9deb8
d2cb5ab
e9c957c
b72fd09
454c7b3
966bf06
d1145f2
1587dd6
fe306d7
080dc62
75c97b0
1494463
6dd0223
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 |
---|---|---|
|
@@ -6,10 +6,14 @@ | |
|
||
#include "impeller/entity/contents/contents.h" | ||
#include "impeller/entity/contents/texture_contents.h" | ||
#include "impeller/entity/entity_pass.h" | ||
#include "impeller/geometry/path_builder.h" | ||
|
||
namespace impeller { | ||
|
||
/// PaintPassDelegate | ||
/// ---------------------------------------------- | ||
|
||
PaintPassDelegate::PaintPassDelegate(Paint paint, std::optional<Rect> coverage) | ||
: paint_(std::move(paint)), coverage_(coverage) {} | ||
|
||
|
@@ -27,7 +31,7 @@ bool PaintPassDelegate::CanElide() { | |
} | ||
|
||
// |EntityPassDelgate| | ||
bool PaintPassDelegate::CanCollapseIntoParentPass() { | ||
bool PaintPassDelegate::CanCollapseIntoParentPass(EntityPass* entity_pass) { | ||
return false; | ||
} | ||
|
||
|
@@ -44,4 +48,90 @@ std::shared_ptr<Contents> PaintPassDelegate::CreateContentsForSubpassTarget( | |
effect_transform); | ||
} | ||
|
||
/// OpacityPeepholePassDelegate | ||
/// ---------------------------------------------- | ||
|
||
OpacityPeepholePassDelegate::OpacityPeepholePassDelegate( | ||
Paint paint, | ||
std::optional<Rect> coverage) | ||
: paint_(std::move(paint)), coverage_(coverage) {} | ||
|
||
// |EntityPassDelgate| | ||
OpacityPeepholePassDelegate::~OpacityPeepholePassDelegate() = default; | ||
|
||
// |EntityPassDelgate| | ||
std::optional<Rect> OpacityPeepholePassDelegate::GetCoverageRect() { | ||
return coverage_; | ||
} | ||
|
||
// |EntityPassDelgate| | ||
bool OpacityPeepholePassDelegate::CanElide() { | ||
return paint_.blend_mode == BlendMode::kDestination; | ||
} | ||
|
||
// |EntityPassDelgate| | ||
bool OpacityPeepholePassDelegate::CanCollapseIntoParentPass( | ||
EntityPass* entity_pass) { | ||
// 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->GetEntityCount() > 3) { | ||
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. One concern I have with working on this optimization now: subpasses are extra expensive right now on the metal backend due to the whole waitForScheduled, and also potentially due to lack of multithreaded encoding. We might tune these thresholds too high and end up trading a bunch of raster time for decreasing returns of GPU utilization. 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. Yes! We're terribly underutilizing the GPU here. Sibling EntityPasses are fundamentally independent and can be evaluated in parallel on the CPU (not a huge impact in this case) and executed in parallel on the GPU (absolutely enormous impact in this case). Once we have a proper synchronization solution, we can remove all of the host<->device syncs that are blocking the CPU and saturate the GPU with parallelizable work. 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. Although I think we added texture atlas sharing between EntityPasses at some point. That (and any other resource sharing between EntityPasses) needs to be reverted, of course. 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. Can we get around that by making glyph atlas construction happen eagerly? Otherwise, glyph atlas construction and upload takes a substantial amount of time. 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. or, more specifically, can we push construction of the glyph atlas including all text frames into a different thread and then use whatever resource tracking mechanism we create to ensure the subsequent commands that depend on this are ordered correctly? 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.
Yeah I think that's probably the best option -- just render the whole thing at once and upload everything just before the EntityPass run. Then the uploaded atlases become read only for the frame and don't restrict parallelism. |
||
// 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 i = 0; | ||
auto had_subpass = entity_pass->IterateAllFlatEntities([&](Entity& entity) { | ||
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. nit: avoid over-capturing (don't use 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. Done |
||
auto contents = entity.GetContents(); | ||
if (!contents->CanAcceptOpacity(entity)) { | ||
all_can_accept = false; | ||
return false; | ||
} | ||
i++; | ||
auto maybe_coverage = contents->GetCoverage(entity); | ||
if (maybe_coverage.has_value()) { | ||
auto coverage = maybe_coverage.value(); | ||
for (const auto& cv : all_coverages) { | ||
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. Can we make this even simpler by just bailing out if we don't get the structure in the comments above (i.e. clip, draw, restore)? A small change above could lead to a very expensive change down here without realizing it. 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. We don't have a good way to check what kind of entity something is. |
||
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; | ||
} | ||
entity_pass->IterateAllFlatEntities([&](Entity& entity) { | ||
entity.GetContents()->InheritOpacity(paint_.color.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->SetTexture(target); | ||
contents->SetSourceRect(Rect::MakeSize(target->GetSize())); | ||
contents->SetOpacity(paint_.color.alpha); | ||
contents->SetDeferApplyingOpacity(true); | ||
return paint_.WithFiltersForSubpassTarget(std::move(contents), | ||
effect_transform); | ||
} | ||
|
||
} // namespace impeller |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -83,6 +83,19 @@ class Contents { | |
|
||
void SetColorSourceSize(Size size) { color_source_size_ = size; } | ||
|
||
/// @brief Whether or not this contents can accept the opacity peephole | ||
/// optimization. | ||
/// | ||
/// By default all contents return false. Contents are responsible | ||
/// for determining whether or not their own geometries intersect in | ||
/// a way that makes accepting opacity impossible. It is always safe | ||
/// to return false, especially if computing overlap would be | ||
/// computationally expensive. | ||
virtual bool CanAcceptOpacity(const Entity& entity) const; | ||
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. Can this just be a single call. Say "virtual bool InheritOpacity(...)". The return value can indicate if the operation succeeded. 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. We have to make sure the entities in a pass aren't overlapping too, so we can't combine them. basically there would need to be two levels of checks:
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. sgtm |
||
|
||
/// @brief Inherit the provided opacity. | ||
virtual void InheritOpacity(Scalar opacity); | ||
|
||
protected: | ||
|
||
private: | ||
|
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -4,6 +4,7 @@ | |||
|
||||
#include "impeller/entity/contents/text_contents.h" | ||||
|
||||
#include <iostream> | ||||
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.
Suggested change
Not sure if github is showing that right, but what I'm trying to articulate is that this line can be deleted. 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 got it! |
||||
#include <optional> | ||||
#include <type_traits> | ||||
#include <utility> | ||||
|
@@ -50,6 +51,15 @@ void TextContents::SetColor(Color color) { | |||
color_ = color; | ||||
} | ||||
|
||||
bool TextContents::CanAcceptOpacity(const Entity& entity) const { | ||||
return !frame_.MaybeHasOverlapping(); | ||||
} | ||||
|
||||
void TextContents::InheritOpacity(Scalar opacity) { | ||||
auto color = color_; | ||||
color_ = color.WithAlpha(color.alpha * opacity); | ||||
} | ||||
|
||||
std::optional<Rect> TextContents::GetCoverage(const Entity& entity) const { | ||||
auto bounds = frame_.GetBounds(); | ||||
if (!bounds.has_value()) { | ||||
|
@@ -117,9 +127,9 @@ static bool CommonRender( | |||
// interpolated vertex information is also used in the fragment shader to | ||||
// sample from the glyph atlas. | ||||
|
||||
const std::array<Point, 4> unit_points = {Point{0, 0}, Point{1, 0}, | ||||
Point{0, 1}, Point{1, 1}}; | ||||
const std::array<uint32_t, 6> indices = {0, 1, 2, 1, 2, 3}; | ||||
constexpr std::array<Point, 4> unit_points = {Point{0, 0}, Point{1, 0}, | ||||
Point{0, 1}, Point{1, 1}}; | ||||
constexpr std::array<uint32_t, 6> indices = {0, 1, 2, 1, 2, 3}; | ||||
|
||||
VertexBufferBuilder<typename VS::PerVertexData> vertex_builder; | ||||
|
||||
|
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.
Does this deserve a TODO/bug to extend this to other blend modes?
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.
I don't think we can in general.