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#43698)

Fixes flutter/flutter#129862

This reverts commit fc9fc93.

These changes were exposing an underlying bug in the DisplayListMatrixClipTracker that has now been fixed independently (flutter#43664). There are no changes to the commit being relanded here, it has been tested on the Wondrous app which demonstrated the regression before the NOP culling feature was reverted and it now works fine due to the fix of the underlying cause.
  • Loading branch information
@flar @harryterkelsen
flar authored and harryterkelsen committed Jul 20, 2023
1 parent fc6d2a7 commit 83a2dd4
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 @@ -24,6 +24,7 @@
#include "third_party/skia/include/core/SkBBHFactory.h"
#include "third_party/skia/include/core/SkColorFilter.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 @@ -3018,5 +3019,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 83a2dd4

Please sign in to comment.