-
Notifications
You must be signed in to change notification settings - Fork 6k
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] add basic culling checks during text frame dispatcher. #55168
Changes from 6 commits
8edf328
2a1e880
a166676
3c3cbc7
b6be14c
82b8b06
d1e0b8a
2906092
90b85aa
2397f96
a0b04d0
e620776
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 |
---|---|---|
|
@@ -1368,22 +1368,40 @@ Canvas& ExperimentalDlDispatcher::GetCanvas() { | |
//// Text Frame Dispatcher | ||
|
||
TextFrameDispatcher::TextFrameDispatcher(const ContentContext& renderer, | ||
const Matrix& initial_matrix) | ||
: renderer_(renderer), matrix_(initial_matrix) {} | ||
const Matrix& initial_matrix, | ||
const Rect cull_rect) | ||
: renderer_(renderer), matrix_(initial_matrix) { | ||
cull_rect_state_.push_back(cull_rect); | ||
} | ||
|
||
void TextFrameDispatcher::save() { | ||
stack_.emplace_back(matrix_); | ||
cull_rect_state_.push_back(cull_rect_state_.back()); | ||
} | ||
|
||
void TextFrameDispatcher::saveLayer(const DlRect& bounds, | ||
const flutter::SaveLayerOptions options, | ||
const flutter::DlImageFilter* backdrop) { | ||
save(); | ||
|
||
// This dispatcher does not track enough state to accurately compute | ||
// cull rects with image filters. | ||
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. For this, and other reasons, I think saveLayer should be a NOP.
I'd rather see this method just do:
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. Changed this to set to max cull rect if an image filter is encountered. |
||
if (paint_.image_filter) { | ||
cull_rect_state_.push_back(bounds); | ||
} else { | ||
auto new_cull_rect = GetCurrentLocalCullingBounds().Intersection(bounds); | ||
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. Reverse this - transform bounds - use raw device cull bounds 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. but 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. You need them both in the same coordinate space. The state rect is in device coordinates. You thus need to compute an answer in device coordinates. But, you transform the state rect into local coordinates to do the math and then push its result without transforming it back. Even if you transformed it back, it is less efficient then transforming the bounds to device coords and doing the math there - naturally ending up with an answer already in the desired answer's coordinate space. |
||
if (new_cull_rect.has_value()) { | ||
cull_rect_state_.push_back(new_cull_rect.value()); | ||
} else { | ||
cull_rect_state_.push_back(Rect::MakeLTRB(0, 0, 0, 0)); | ||
} | ||
} | ||
} | ||
|
||
void TextFrameDispatcher::restore() { | ||
matrix_ = stack_.back(); | ||
stack_.pop_back(); | ||
cull_rect_state_.pop_back(); | ||
} | ||
|
||
void TextFrameDispatcher::translate(DlScalar tx, DlScalar ty) { | ||
|
@@ -1459,14 +1477,29 @@ void TextFrameDispatcher::drawTextFrame( | |
); | ||
} | ||
|
||
const Rect TextFrameDispatcher::GetCurrentLocalCullingBounds() const { | ||
auto cull_rect = cull_rect_state_.back(); | ||
if (!cull_rect.IsEmpty() && !cull_rect.IsMaximum()) { | ||
Matrix inverse = matrix_.Invert(); | ||
cull_rect = cull_rect.TransformBounds(inverse); | ||
} | ||
return cull_rect; | ||
} | ||
|
||
void TextFrameDispatcher::drawDisplayList( | ||
const sk_sp<flutter::DisplayList> display_list, | ||
DlScalar opacity) { | ||
[[maybe_unused]] size_t stack_depth = stack_.size(); | ||
save(); | ||
Paint old_paint = paint_; | ||
paint_ = Paint{}; | ||
display_list->Dispatch(*this); | ||
|
||
auto cull_rect = IRect::RoundOut(GetCurrentLocalCullingBounds()); | ||
display_list->Dispatch(*this, SkIRect::MakeLTRB(cull_rect.GetLeft(), // | ||
cull_rect.GetTop(), // | ||
cull_rect.GetRight(), // | ||
cull_rect.GetBottom() // | ||
)); | ||
restore(); | ||
paint_ = old_paint; | ||
FML_DCHECK(stack_depth == stack_.size()); | ||
|
@@ -1553,8 +1586,8 @@ std::shared_ptr<Texture> DisplayListToTexture( | |
} | ||
|
||
SkIRect sk_cull_rect = SkIRect::MakeWH(size.width, size.height); | ||
impeller::TextFrameDispatcher collector(context.GetContentContext(), | ||
impeller::Matrix()); | ||
impeller::TextFrameDispatcher collector( | ||
context.GetContentContext(), impeller::Matrix(), Rect::MakeSize(size)); | ||
display_list->Dispatch(collector, sk_cull_rect); | ||
impeller::ExperimentalDlDispatcher impeller_dispatcher( | ||
context.GetContentContext(), target, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,8 @@ | |
#include "impeller/aiks/experimental_canvas.h" | ||
#include "impeller/aiks/paint.h" | ||
#include "impeller/entity/contents/content_context.h" | ||
#include "impeller/geometry/color.h" | ||
#include "impeller/geometry/rect.h" | ||
|
||
namespace impeller { | ||
|
||
|
@@ -330,7 +332,8 @@ class TextFrameDispatcher : public flutter::IgnoreAttributeDispatchHelper, | |
public flutter::IgnoreDrawDispatchHelper { | ||
public: | ||
TextFrameDispatcher(const ContentContext& renderer, | ||
const Matrix& initial_matrix); | ||
const Matrix& initial_matrix, | ||
const Rect cull_rect); | ||
void save() override; | ||
|
||
void saveLayer(const DlRect& bounds, | ||
|
@@ -387,9 +390,12 @@ class TextFrameDispatcher : public flutter::IgnoreAttributeDispatchHelper, | |
void setStrokeJoin(flutter::DlStrokeJoin join) override; | ||
|
||
private: | ||
const Rect GetCurrentLocalCullingBounds() const; | ||
|
||
const ContentContext& renderer_; | ||
Matrix matrix_; | ||
std::vector<Matrix> stack_; | ||
std::vector<Rect> cull_rect_state_; | ||
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 don't think you need a stack. At least not yet. SaveLayer bounds culling already happened so using a stack so that you can incorporate that into the culling of DL dispatching isn't likely to benefit much. DL already culled - by failing to record - anything that was distinctly outside of a clip set in the DL stream, including the bounds of SaveLayers. So trying to intersect the cull_rect as you read through the current one isn't likely to improve the culling much compared to just culling against the (inverse transformed) frame bounds. If you remove the code in SaveLayer does it change the results much for your test case? If not then you don't really need a stack here until we do a full clip-based culling and even that would be icing on the cake rather than a critical step in the process. 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 do still need to account for image filters, which aren't captured in the transform state. The locally transformed device bounds may be too small in some cases. 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. Are you sure? I don't think regular display list culling takes those into account. It just forwards the inverse-mapped clip bounds to the new Dispatch call. Keep in mind that the bounds recorded in the RTree include any expansion due the filters applied to them, even including every filter on every saveLayer that they are inside of. That kind of inverts the responsibility there, but I'd have to look at it more to be sure this is right. If it is wrong then maybe we're doing the rendering side of culling wrong as well... 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'm sure the culling takes that into account, but this is more of a problem with the cull rect computation right? (assuming no other canvas ops) Suppose I have a NxM device rect and a saveLayer with a 2x scaling image filter on it. If I need to dispatch to a sub display list inside of that savelayer, It wouldn't be correct to use the NxM cull rect, I'd need a 2Nx2M cull rect. For other transforms I can invert the transform matrix but we don't track the image filter transforms easily yet. 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. Within a display list that has already been taken care of by the way that the RTree rects are accumulated, but I think you are correct that it is as if there is an additional "transform" that goes on when you hit a saveLayer that has a filter. So, just as you have to take the original cull rect and un-transform it by the CTM, you also have to un-transform it by the actions of the SaveLayers for similar reasons. So, yes, it is still an issue that this code punts on the filter un-transform of the savelayers as you mentioned in the code. |
||
Paint paint_; | ||
}; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -73,8 +73,11 @@ sk_sp<DlImage> DoMakeRasterSnapshot( | |
); | ||
} | ||
|
||
impeller::TextFrameDispatcher collector(context->GetContentContext(), | ||
impeller::Matrix()); | ||
impeller::TextFrameDispatcher collector( | ||
context->GetContentContext(), // | ||
impeller::Matrix(), // | ||
impeller::Rect::MakeSize(render_target_size) // | ||
); | ||
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. This pattern of "first dispatch to a TFD and then to a DlDispatcher" happens in a number of places. It feels like a convenience method might be nice. 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. In #55019 I refactored this dispatching into a reusable static method. |
||
display_list->Dispatch(collector, SkIRect::MakeSize(size)); | ||
impeller::ExperimentalDlDispatcher impeller_dispatcher( | ||
context->GetContentContext(), target, | ||
|
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.
bounds here are in local coordinates, I think you need to transform them.
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.
Also, your cull rect state doesn't look balanced. You always pop it in restore, but you push_back here. If save() doesn't push_back then restore will unbalance it. If save() does do a stack dup, then this code will over balance it...
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.
My understanding was that the cull rect always need to be in local (to the display list) coordinate space. So I need to transform the cull rect into local space and then intersect with the bounds.