Skip to content

Commit

Permalink
Revert "Reland "add non-rendering operation culling to DisplayListBui…
Browse files Browse the repository at this point in the history 
…lder" (#41463)" (#43721)

Reverts #43698

A framework tree test started failing on the engine roll with this PR: flutter/flutter#130643

The test failure is in https://ci.chromium.org/ui/p/flutter/builders/prod/Linux_android%20hybrid_android_views_integration_test/8517/overview

```
[2023-07-14 19:33:21.980926] [STDOUT] stdout: [   +6 ms] I/PlatformViewsController( 9988): Using hybrid composition for platform view: 5
[2023-07-14 19:33:22.767236] [STDOUT] stdout: [ +786 ms] 00:19 �[32m+4�[0m�[31m -1�[0m: Flutter surface with hybrid composition Uses FlutterImageView when Android view is on the screen �[1m�[31m[E]�[0m�[0m
[2023-07-14 19:33:22.767765] [STDOUT] stdout: [        ]   Expected: '|-FlutterView\n'
[2023-07-14 19:33:22.767815] [STDOUT] stdout: [        ]               '  |-FlutterSurfaceView\n'
[2023-07-14 19:33:22.767924] [STDOUT] stdout: [        ]               '  |-FlutterImageView\n'
[2023-07-14 19:33:22.768084] [STDOUT] stdout: [        ]               '  |-ViewGroup\n'
[2023-07-14 19:33:22.768162] [STDOUT] stdout: [        ]               '    |-ViewGroup\n'
[2023-07-14 19:33:22.768800] [STDOUT] stdout: [        ]               '  |-FlutterImageView\n'
[2023-07-14 19:33:22.768835] [STDOUT] stdout: [        ]               ''
[2023-07-14 19:33:22.768853] [STDOUT] stdout: [        ]     Actual: '|-FlutterView\n'
[2023-07-14 19:33:22.768882] [STDOUT] stdout: [        ]               '  |-FlutterSurfaceView\n'
[2023-07-14 19:33:22.768900] [STDOUT] stdout: [        ]               '  |-FlutterImageView\n'
[2023-07-14 19:33:22.768917] [STDOUT] stdout: [        ]               '  |-ViewGroup\n'
[2023-07-14 19:33:22.768956] [STDOUT] stdout: [        ]               '    |-ViewGroup\n'
[2023-07-14 19:33:22.769119] [STDOUT] stdout: [        ]               ''
[2023-07-14 19:33:22.769156] [STDOUT] stdout: [        ]      Which: is different. Both strings start the same, but the actual value is missing the following trailing characters:   |-Flutte ...
[2023-07-14 19:33:22.779280] [STDOUT] stdout: [  +10 ms]   package:matcher/src/expect/expect.dart 149:31     fail
[2023-07-14 19:33:22.779326] [STDOUT] stdout: [        ]   package:matcher/src/expect/expect.dart 144:3      _expect
[2023-07-14 19:33:22.780315] [STDOUT] stdout: [        ]   package:matcher/src/expect/expect.dart 56:3       expect
[2023-07-14 19:33:22.780345] [STDOUT] stdout: [        ]   test_driver/main_test.dart 124:7                  main.<fn>.<fn>
[2023-07-14 19:33:22.780356] [STDOUT] stdout: [        ]   ===== asynchronous gap ===========================
[2023-07-14 19:33:22.780365] [STDOUT] stdout: [        ]   package:test_api/src/backend/declarer.dart 215:9  Declarer.test.<fn>.<fn>
[2023-07-14 19:33:22.780376] [STDOUT] stdout: [        ]   ===== asynchronous gap ===========================
[2023-07-14 19:33:22.780385] [STDOUT] stdout: [        ]   package:test_api/src/backend/declarer.dart 213:7  Declarer.test.<fn>
[2023-07-14 19:33:22.780395] [STDOUT] stdout: [        ]   ===== asynchronous gap ===========================
[2023-07-14 19:33:22.780405] [STDOUT] stdout: [        ]   package:test_api/src/backend/invoker.dart 258:9   Invoker._waitForOutstandingCallbacks.<fn>
[2023-07-14 19:33:22.780415] [STDOUT] stdout: [        ] 00:19 �[32m+4�[0m�[31m -1�[0m: Flutter surface with hybrid composition (tearDownAll)�[0m
[2023-07-14 19:33:22.907295] [STDOUT] stdout: [ +126 ms] 00:19 �[32m+4�[0m�[31m -1�[0m: (tearDownAll)�[0m
[2023-07-14 19:33:22.947855] [STDOUT] stdout: [  +41 ms] 00:19 �[32m+4�[0m�[31m -1�[0m: �[31mSome tests failed.�[0m
```

This change in that roll looks like it may be related.
  • Loading branch information
@zanderso
zanderso authored Jul 15, 2023
1 parent c96f995 commit 886a14e
Show file tree
Hide file tree
Showing 20 changed files with 311 additions and 1,267 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(1, 0, 0, 0));
xforms.push_back(SkRSXform::Make(0, 0, 0, 0));
}

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

DisplayList::DisplayList(DisplayListStorage&& storage,
size_t byte_count,
Expand All @@ -33,7 +32,6 @@ 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 @@ -44,7 +42,6 @@ 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: 0 additions & 16 deletions display_list/display_list.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -265,19 +265,6 @@ 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 @@ -287,7 +274,6 @@ 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 @@ -306,8 +292,6 @@ 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: 0 additions & 160 deletions display_list/display_list_unittests.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
#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 @@ -3019,164 +3018,5 @@ 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 886a14e

Please sign in to comment.