Skip to content
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

Merged
merged 12 commits into from
Sep 24, 2024
5 changes: 4 additions & 1 deletion impeller/aiks/aiks_playground.cc
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,10 @@ bool AiksPlayground::OpenPlaygroundHere(
[&renderer, &callback](RenderTarget& render_target) -> bool {
#if EXPERIMENTAL_CANVAS
auto display_list = callback();
TextFrameDispatcher collector(renderer.GetContentContext(), Matrix());
TextFrameDispatcher collector(renderer.GetContentContext(), //
Matrix(), //
Rect::MakeMaximum() //
);
display_list->Dispatch(collector);

ExperimentalDlDispatcher impeller_dispatcher(
Expand Down
43 changes: 38 additions & 5 deletions impeller/display_list/dl_dispatcher.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Copy link
Contributor

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.

Copy link
Contributor

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...

Copy link
Member Author

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.

// This dispatcher does not track enough state to accurately compute
// cull rects with image filters.
Copy link
Contributor

Choose a reason for hiding this comment

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

For this, and other reasons, I think saveLayer should be a NOP.

  • First, DL has already done culling against things that appeared within the DL stream when it did the recording. So, the bounds you see here don't necessarily add any new information. The DL dispatch method is already culling things against the cull bounds that were provided in the initial dispatch call. DlBuilder already culled things against the bounds of save layers and clips when it did the recording. So, how is this particular enhancement really adding any value - other than some rare interactions where something might intersect the bounds here and also intersect the cull rect, but somehow not intersect the intersection of the two? If we were testing full geometries then strange cases like that might fall through the holes and we might be able to catch them with some more diligence here, but we are just testing bounds of the geometries, making it less likely to catch a corner case.
  • You could more easily do this same kind of calculation in the clip*() methods which don't have the fuzzy issues of saveLayer. Why choose saveLayer to enforce its possibly-fuzzily-potentially clipping when we aren't instrumenting the actual clip calls?
  • Really the important thing to track is how the filter manipulates the transform, but we aren't doing that. If anything, we should be setting the cull rect state to MaxRect here (like we have done in the non-experimental AIKS canvas) if there is a filter present, but that is not what the code here is doing...

I'd rather see this method just do:

  if (filter) {
    cull_rect_state.back() = Maximum;
  }

Copy link
Member Author

Choose a reason for hiding this comment

The 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);
Copy link
Contributor

Choose a reason for hiding this comment

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

Reverse this - transform bounds - use raw device cull bounds

Copy link
Member Author

Choose a reason for hiding this comment

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

but bounds is in the local coordinate space, so I need the device cull rect transformed into local coordinates?

Copy link
Contributor

Choose a reason for hiding this comment

The 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) {
Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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,
Expand Down
8 changes: 7 additions & 1 deletion impeller/display_list/dl_dispatcher.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 {

Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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_;
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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...

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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_;
};

Expand Down
5 changes: 4 additions & 1 deletion impeller/display_list/dl_playground.cc
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,10 @@ bool DlPlayground::OpenPlaygroundHere(DisplayListPlaygroundCallback callback) {
auto list = callback();

#if EXPERIMENTAL_CANVAS
TextFrameDispatcher collector(context.GetContentContext(), Matrix());
TextFrameDispatcher collector(context.GetContentContext(), //
Matrix(), //
Rect::MakeMaximum() //
);
list->Dispatch(collector);

ExperimentalDlDispatcher impeller_dispatcher(
Expand Down
7 changes: 5 additions & 2 deletions shell/common/snapshot_controller_impeller.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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) //
);
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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,
Expand Down
5 changes: 3 additions & 2 deletions shell/gpu/gpu_surface_gl_impeller.cc
Original file line number Diff line number Diff line change
Expand Up @@ -120,8 +120,9 @@ std::unique_ptr<SurfaceFrame> GPUSurfaceGLImpeller::AcquireFrame(

#if EXPERIMENTAL_CANVAS
auto skia_cull_rect = SkIRect::MakeWH(cull_rect.width, cull_rect.height);
impeller::TextFrameDispatcher collector(aiks_context->GetContentContext(),
impeller::Matrix());
impeller::TextFrameDispatcher collector(
aiks_context->GetContentContext(), impeller::Matrix(),
impeller::Rect::MakeSize(cull_rect));
display_list->Dispatch(collector, skia_cull_rect);

impeller::ExperimentalDlDispatcher impeller_dispatcher(
Expand Down
12 changes: 8 additions & 4 deletions shell/gpu/gpu_surface_metal_impeller.mm
Original file line number Diff line number Diff line change
Expand Up @@ -171,8 +171,10 @@
surface->SetFrameBoundary(surface_frame.submit_info().frame_boundary);

#if EXPERIMENTAL_CANVAS
impeller::TextFrameDispatcher collector(aiks_context->GetContentContext(),
impeller::Matrix());
impeller::TextFrameDispatcher collector(aiks_context->GetContentContext(), //
impeller::Matrix(), //
impeller::Rect::MakeSize(cull_rect.GetSize()) //
);
display_list->Dispatch(collector, sk_cull_rect);

impeller::ExperimentalDlDispatcher impeller_dispatcher(
Expand Down Expand Up @@ -299,8 +301,10 @@
impeller::IRect cull_rect = surface->coverage();
SkIRect sk_cull_rect = SkIRect::MakeWH(cull_rect.GetWidth(), cull_rect.GetHeight());
#if EXPERIMENTAL_CANVAS
impeller::TextFrameDispatcher collector(aiks_context->GetContentContext(),
impeller::Matrix());
impeller::TextFrameDispatcher collector(aiks_context->GetContentContext(), //
impeller::Matrix(), //
impeller::Rect::MakeSize(cull_rect.GetSize()) //
);
display_list->Dispatch(collector, sk_cull_rect);
impeller::RenderTarget render_target = surface->GetTargetRenderPassDescriptor();
impeller::ExperimentalDlDispatcher impeller_dispatcher(
Expand Down
5 changes: 3 additions & 2 deletions shell/gpu/gpu_surface_vulkan_impeller.cc
Original file line number Diff line number Diff line change
Expand Up @@ -81,8 +81,9 @@ std::unique_ptr<SurfaceFrame> GPUSurfaceVulkanImpeller::AcquireFrame(
}

#if EXPERIMENTAL_CANVAS
impeller::TextFrameDispatcher collector(aiks_context->GetContentContext(),
impeller::Matrix());
impeller::TextFrameDispatcher collector(
aiks_context->GetContentContext(), impeller::Matrix(),
impeller::Rect::MakeSize(cull_rect));
display_list->Dispatch(collector,
SkIRect::MakeWH(cull_rect.width, cull_rect.height));
impeller::ExperimentalDlDispatcher impeller_dispatcher(
Expand Down
8 changes: 5 additions & 3 deletions shell/platform/embedder/embedder_external_view.cc
Original file line number Diff line number Diff line change
Expand Up @@ -134,9 +134,11 @@ bool EmbedderExternalView::Render(const EmbedderRenderTarget& render_target,
impeller::IRect::MakeSize(impeller_target->GetRenderTargetSize());
SkIRect sk_cull_rect =
SkIRect::MakeWH(cull_rect.GetWidth(), cull_rect.GetHeight());

impeller::TextFrameDispatcher collector(aiks_context->GetContentContext(),
impeller::Matrix());
impeller::TextFrameDispatcher collector(
aiks_context->GetContentContext(), //
impeller::Matrix(), //
impeller::Rect::MakeSize(cull_rect.GetSize()) //
);
display_list->Dispatch(collector, sk_cull_rect);

impeller::ExperimentalDlDispatcher impeller_dispatcher(
Expand Down