Skip to content

Commit

Permalink
Reland "add non-rendering operation culling to DisplayListBuilder" (f…
Browse files Browse the repository at this point in the history 
…lutter#41463) (flutter#42584)

This reverts commit 2553def.

Fixes flutter/flutter#125338

This PR should fix the blendmode/color analysis that caused failures for flutter#41463 as well as the bounds failures for inverted rectangles that caused failures for flutter#42330 (incorporating the fix from flutter#42556).

The description from the [previous PR](flutter#41463) updated with the new name of the DL property:

---------------------------------
This optimization avoids recording unnecessary render operations that will not affect the output and also eliminates the need for "draw detection" mechanisms like `DlOpSpy` and `CanvasSpy` by remembering if any non-transparent operations were included. The `DlOpSpy` unit tests were updated to check if the results from that object match the new `DisplayList::modifies_transparent_black()` method.

Fixes flutter/flutter#125338

In addition, this change will unblock some other Issues:

- flutter/flutter#125318
- flutter/flutter#125403
  • Loading branch information
@flar
flar authored Jun 11, 2023
1 parent 788437e commit 9bb0a59
Show file tree
Hide file tree
Showing 20 changed files with 1,267 additions and 311 deletions.
2 changes: 1 addition & 1 deletion display_list/benchmarking/dl_complexity_unittests.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -423,7 +423,7 @@ TEST(DisplayListComplexity, DrawAtlas) {
std::vector<SkRSXform> xforms;
for (int i = 0; i < 10; i++) {
rects.push_back(SkRect::MakeXYWH(0, 0, 10, 10));
xforms.push_back(SkRSXform::Make(0, 0, 0, 0));
xforms.push_back(SkRSXform::Make(1, 0, 0, 0));
}

DisplayListBuilder builder;
Expand Down
5 changes: 4 additions & 1 deletion display_list/display_list.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@ DisplayList::DisplayList()
unique_id_(0),
bounds_({0, 0, 0, 0}),
can_apply_group_opacity_(true),
is_ui_thread_safe_(true) {}
is_ui_thread_safe_(true),
modifies_transparent_black_(false) {}

DisplayList::DisplayList(DisplayListStorage&& storage,
size_t byte_count,
Expand All @@ -32,6 +33,7 @@ DisplayList::DisplayList(DisplayListStorage&& storage,
const SkRect& bounds,
bool can_apply_group_opacity,
bool is_ui_thread_safe,
bool modifies_transparent_black,
sk_sp<const DlRTree> rtree)
: storage_(std::move(storage)),
byte_count_(byte_count),
Expand All @@ -42,6 +44,7 @@ DisplayList::DisplayList(DisplayListStorage&& storage,
bounds_(bounds),
can_apply_group_opacity_(can_apply_group_opacity),
is_ui_thread_safe_(is_ui_thread_safe),
modifies_transparent_black_(modifies_transparent_black),
rtree_(std::move(rtree)) {}

DisplayList::~DisplayList() {
Expand Down
16 changes: 16 additions & 0 deletions display_list/display_list.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,19 @@ class DisplayList : public SkRefCnt {
bool can_apply_group_opacity() const { return can_apply_group_opacity_; }
bool isUIThreadSafe() const { return is_ui_thread_safe_; }

/// @brief Indicates if there are any rendering operations in this
/// DisplayList that will modify a surface of transparent black
/// pixels.
///
/// This condition can be used to determine whether to create a cleared
/// surface, render a DisplayList into it, and then composite the
/// result into a scene. It is not uncommon for code in the engine to
/// come across such degenerate DisplayList objects when slicing up a
/// frame between platform views.
bool modifies_transparent_black() const {
return modifies_transparent_black_;
}

private:
DisplayList(DisplayListStorage&& ptr,
size_t byte_count,
Expand All @@ -274,6 +287,7 @@ class DisplayList : public SkRefCnt {
const SkRect& bounds,
bool can_apply_group_opacity,
bool is_ui_thread_safe,
bool modifies_transparent_black,
sk_sp<const DlRTree> rtree);

static uint32_t next_unique_id();
Expand All @@ -292,6 +306,8 @@ class DisplayList : public SkRefCnt {

const bool can_apply_group_opacity_;
const bool is_ui_thread_safe_;
const bool modifies_transparent_black_;

const sk_sp<const DlRTree> rtree_;

void Dispatch(DlOpReceiver& ctx,
Expand Down
160 changes: 160 additions & 0 deletions display_list/display_list_unittests.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include "flutter/testing/testing.h"

#include "third_party/skia/include/core/SkPictureRecorder.h"
#include "third_party/skia/include/core/SkRSXform.h"
#include "third_party/skia/include/core/SkSurface.h"

namespace flutter {
Expand Down Expand Up @@ -3016,5 +3017,164 @@ TEST_F(DisplayListTest, DrawUnorderedRoundRectPathCCW) {
check_inverted_bounds(renderer, "DrawRoundRectPath Counter-Clockwise");
}

TEST_F(DisplayListTest, NopOperationsOmittedFromRecords) {
auto run_tests = [](const std::string& name,
void init(DisplayListBuilder & builder, DlPaint & paint),
uint32_t expected_op_count = 0u) {
auto run_one_test =
[init](const std::string& name,
void build(DisplayListBuilder & builder, DlPaint & paint),
uint32_t expected_op_count = 0u) {
DisplayListBuilder builder;
DlPaint paint;
init(builder, paint);
build(builder, paint);
auto list = builder.Build();
if (list->op_count() != expected_op_count) {
FML_LOG(ERROR) << *list;
}
ASSERT_EQ(list->op_count(), expected_op_count) << name;
ASSERT_TRUE(list->bounds().isEmpty()) << name;
};
run_one_test(
name + " DrawColor",
[](DisplayListBuilder& builder, DlPaint& paint) {
builder.DrawColor(paint.getColor(), paint.getBlendMode());
},
expected_op_count);
run_one_test(
name + " DrawPaint",
[](DisplayListBuilder& builder, DlPaint& paint) {
builder.DrawPaint(paint);
},
expected_op_count);
run_one_test(
name + " DrawRect",
[](DisplayListBuilder& builder, DlPaint& paint) {
builder.DrawRect({10, 10, 20, 20}, paint);
},
expected_op_count);
run_one_test(
name + " Other Draw Ops",
[](DisplayListBuilder& builder, DlPaint& paint) {
builder.DrawLine({10, 10}, {20, 20}, paint);
builder.DrawOval({10, 10, 20, 20}, paint);
builder.DrawCircle({50, 50}, 20, paint);
builder.DrawRRect(SkRRect::MakeRectXY({10, 10, 20, 20}, 5, 5), paint);
builder.DrawDRRect(SkRRect::MakeRectXY({5, 5, 100, 100}, 5, 5),
SkRRect::MakeRectXY({10, 10, 20, 20}, 5, 5),
paint);
builder.DrawPath(kTestPath1, paint);
builder.DrawArc({10, 10, 20, 20}, 45, 90, true, paint);
SkPoint pts[] = {{10, 10}, {20, 20}};
builder.DrawPoints(PointMode::kLines, 2, pts, paint);
builder.DrawVertices(TestVertices1, DlBlendMode::kSrcOver, paint);
builder.DrawImage(TestImage1, {10, 10}, DlImageSampling::kLinear,
&paint);
builder.DrawImageRect(TestImage1, SkRect{0.0f, 0.0f, 10.0f, 10.0f},
SkRect{10.0f, 10.0f, 25.0f, 25.0f},
DlImageSampling::kLinear, &paint);
builder.DrawImageNine(TestImage1, {10, 10, 20, 20},
{10, 10, 100, 100}, DlFilterMode::kLinear,
&paint);
SkRSXform xforms[] = {{1, 0, 10, 10}, {0, 1, 10, 10}};
SkRect rects[] = {{10, 10, 20, 20}, {10, 20, 30, 20}};
builder.DrawAtlas(TestImage1, xforms, rects, nullptr, 2,
DlBlendMode::kSrcOver, DlImageSampling::kLinear,
nullptr, &paint);
builder.DrawTextBlob(TestBlob1, 10, 10, paint);

// Dst mode eliminates most rendering ops except for
// the following two, so we'll prune those manually...
if (paint.getBlendMode() != DlBlendMode::kDst) {
builder.DrawDisplayList(TestDisplayList1, paint.getOpacity());
builder.DrawShadow(kTestPath1, paint.getColor(), 1, true, 1);
}
},
expected_op_count);
run_one_test(
name + " SaveLayer",
[](DisplayListBuilder& builder, DlPaint& paint) {
builder.SaveLayer(nullptr, &paint, nullptr);
builder.DrawRect({10, 10, 20, 20}, DlPaint());
builder.Restore();
},
expected_op_count);
run_one_test(
name + " inside Save",
[](DisplayListBuilder& builder, DlPaint& paint) {
builder.Save();
builder.DrawRect({10, 10, 20, 20}, paint);
builder.Restore();
},
expected_op_count);
};
run_tests("transparent color", //
[](DisplayListBuilder& builder, DlPaint& paint) {
paint.setColor(DlColor::kTransparent());
});
run_tests("0 alpha", //
[](DisplayListBuilder& builder, DlPaint& paint) {
// The transparent test above already tested transparent
// black (all 0s), we set White color here so we can test
// the case of all 1s with a 0 alpha
paint.setColor(DlColor::kWhite());
paint.setAlpha(0);
});
run_tests("BlendMode::kDst", //
[](DisplayListBuilder& builder, DlPaint& paint) {
paint.setBlendMode(DlBlendMode::kDst);
});
run_tests("Empty rect clip", //
[](DisplayListBuilder& builder, DlPaint& paint) {
builder.ClipRect(SkRect::MakeEmpty(), ClipOp::kIntersect, false);
});
run_tests("Empty rrect clip", //
[](DisplayListBuilder& builder, DlPaint& paint) {
builder.ClipRRect(SkRRect::MakeEmpty(), ClipOp::kIntersect,
false);
});
run_tests("Empty path clip", //
[](DisplayListBuilder& builder, DlPaint& paint) {
builder.ClipPath(SkPath(), ClipOp::kIntersect, false);
});
run_tests("Transparent SaveLayer", //
[](DisplayListBuilder& builder, DlPaint& paint) {
DlPaint save_paint;
save_paint.setColor(DlColor::kTransparent());
builder.SaveLayer(nullptr, &save_paint);
});
run_tests("0 alpha SaveLayer", //
[](DisplayListBuilder& builder, DlPaint& paint) {
DlPaint save_paint;
// The transparent test above already tested transparent
// black (all 0s), we set White color here so we can test
// the case of all 1s with a 0 alpha
save_paint.setColor(DlColor::kWhite());
save_paint.setAlpha(0);
builder.SaveLayer(nullptr, &save_paint);
});
run_tests("Dst blended SaveLayer", //
[](DisplayListBuilder& builder, DlPaint& paint) {
DlPaint save_paint;
save_paint.setBlendMode(DlBlendMode::kDst);
builder.SaveLayer(nullptr, &save_paint);
});
run_tests(
"Nop inside SaveLayer",
[](DisplayListBuilder& builder, DlPaint& paint) {
builder.SaveLayer(nullptr, nullptr);
paint.setBlendMode(DlBlendMode::kDst);
},
2u);
run_tests("DrawImage inside Culled SaveLayer", //
[](DisplayListBuilder& builder, DlPaint& paint) {
DlPaint save_paint;
save_paint.setColor(DlColor::kTransparent());
builder.SaveLayer(nullptr, &save_paint);
builder.DrawImage(TestImage1, {10, 10}, DlImageSampling::kLinear);
});
}

} // namespace testing
} // namespace flutter
Loading

0 comments on commit 9bb0a59

Please sign in to comment.