Skip to content

Commit 886a14e

Browse files
zandersozanderso
authored
Revert "Reland "add non-rendering operation culling to DisplayListBuilder" (flutter#41463)" (flutter#43721)
Reverts flutter#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.
1 parent c96f995 commit 886a14e

20 files changed

+311
-1267
lines changed

‎display_list/benchmarking/dl_complexity_unittests.cc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -423,7 +423,7 @@ TEST(DisplayListComplexity, DrawAtlas) {
423423
std::vector<SkRSXform> xforms;
424424
for (int i = 0; i < 10; i++) {
425425
rects.push_back(SkRect::MakeXYWH(0, 0, 10, 10));
426-
xforms.push_back(SkRSXform::Make(1, 0, 0, 0));
426+
xforms.push_back(SkRSXform::Make(0, 0, 0, 0));
427427
}
428428

429429
DisplayListBuilder builder;

‎display_list/display_list.cc‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -22,8 +22,7 @@ DisplayList::DisplayList()
2222
unique_id_(0),
2323
bounds_({0, 0, 0, 0}),
2424
can_apply_group_opacity_(true),
25-
is_ui_thread_safe_(true),
26-
modifies_transparent_black_(false) {}
25+
is_ui_thread_safe_(true) {}
2726

2827
DisplayList::DisplayList(DisplayListStorage&& storage,
2928
size_t byte_count,
@@ -33,7 +32,6 @@ DisplayList::DisplayList(DisplayListStorage&& storage,
3332
const SkRect& bounds,
3433
bool can_apply_group_opacity,
3534
bool is_ui_thread_safe,
36-
bool modifies_transparent_black,
3735
sk_sp<const DlRTree> rtree)
3836
: storage_(std::move(storage)),
3937
byte_count_(byte_count),
@@ -44,7 +42,6 @@ DisplayList::DisplayList(DisplayListStorage&& storage,
4442
bounds_(bounds),
4543
can_apply_group_opacity_(can_apply_group_opacity),
4644
is_ui_thread_safe_(is_ui_thread_safe),
47-
modifies_transparent_black_(modifies_transparent_black),
4845
rtree_(std::move(rtree)) {}
4946

5047
DisplayList::~DisplayList() {

‎display_list/display_list.h‎

Lines changed: 0 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -265,19 +265,6 @@ class DisplayList : public SkRefCnt {
265265
bool can_apply_group_opacity() const { return can_apply_group_opacity_; }
266266
bool isUIThreadSafe() const { return is_ui_thread_safe_; }
267267

268-
/// @brief Indicates if there are any rendering operations in this
269-
/// DisplayList that will modify a surface of transparent black
270-
/// pixels.
271-
///
272-
/// This condition can be used to determine whether to create a cleared
273-
/// surface, render a DisplayList into it, and then composite the
274-
/// result into a scene. It is not uncommon for code in the engine to
275-
/// come across such degenerate DisplayList objects when slicing up a
276-
/// frame between platform views.
277-
bool modifies_transparent_black() const {
278-
return modifies_transparent_black_;
279-
}
280-
281268
private:
282269
DisplayList(DisplayListStorage&& ptr,
283270
size_t byte_count,
@@ -287,7 +274,6 @@ class DisplayList : public SkRefCnt {
287274
const SkRect& bounds,
288275
bool can_apply_group_opacity,
289276
bool is_ui_thread_safe,
290-
bool modifies_transparent_black,
291277
sk_sp<const DlRTree> rtree);
292278

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

307293
const bool can_apply_group_opacity_;
308294
const bool is_ui_thread_safe_;
309-
const bool modifies_transparent_black_;
310-
311295
const sk_sp<const DlRTree> rtree_;
312296

313297
void Dispatch(DlOpReceiver& ctx,

‎display_list/display_list_unittests.cc‎

Lines changed: 0 additions & 160 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,6 @@
2424
#include "third_party/skia/include/core/SkBBHFactory.h"
2525
#include "third_party/skia/include/core/SkColorFilter.h"
2626
#include "third_party/skia/include/core/SkPictureRecorder.h"
27-
#include "third_party/skia/include/core/SkRSXform.h"
2827
#include "third_party/skia/include/core/SkSurface.h"
2928

3029
namespace flutter {
@@ -3019,164 +3018,5 @@ TEST_F(DisplayListTest, DrawUnorderedRoundRectPathCCW) {
30193018
check_inverted_bounds(renderer, "DrawRoundRectPath Counter-Clockwise");
30203019
}
30213020

3022-
TEST_F(DisplayListTest, NopOperationsOmittedFromRecords) {
3023-
auto run_tests = [](const std::string& name,
3024-
void init(DisplayListBuilder & builder, DlPaint & paint),
3025-
uint32_t expected_op_count = 0u) {
3026-
auto run_one_test =
3027-
[init](const std::string& name,
3028-
void build(DisplayListBuilder & builder, DlPaint & paint),
3029-
uint32_t expected_op_count = 0u) {
3030-
DisplayListBuilder builder;
3031-
DlPaint paint;
3032-
init(builder, paint);
3033-
build(builder, paint);
3034-
auto list = builder.Build();
3035-
if (list->op_count() != expected_op_count) {
3036-
FML_LOG(ERROR) << *list;
3037-
}
3038-
ASSERT_EQ(list->op_count(), expected_op_count) << name;
3039-
ASSERT_TRUE(list->bounds().isEmpty()) << name;
3040-
};
3041-
run_one_test(
3042-
name + " DrawColor",
3043-
[](DisplayListBuilder& builder, DlPaint& paint) {
3044-
builder.DrawColor(paint.getColor(), paint.getBlendMode());
3045-
},
3046-
expected_op_count);
3047-
run_one_test(
3048-
name + " DrawPaint",
3049-
[](DisplayListBuilder& builder, DlPaint& paint) {
3050-
builder.DrawPaint(paint);
3051-
},
3052-
expected_op_count);
3053-
run_one_test(
3054-
name + " DrawRect",
3055-
[](DisplayListBuilder& builder, DlPaint& paint) {
3056-
builder.DrawRect({10, 10, 20, 20}, paint);
3057-
},
3058-
expected_op_count);
3059-
run_one_test(
3060-
name + " Other Draw Ops",
3061-
[](DisplayListBuilder& builder, DlPaint& paint) {
3062-
builder.DrawLine({10, 10}, {20, 20}, paint);
3063-
builder.DrawOval({10, 10, 20, 20}, paint);
3064-
builder.DrawCircle({50, 50}, 20, paint);
3065-
builder.DrawRRect(SkRRect::MakeRectXY({10, 10, 20, 20}, 5, 5), paint);
3066-
builder.DrawDRRect(SkRRect::MakeRectXY({5, 5, 100, 100}, 5, 5),
3067-
SkRRect::MakeRectXY({10, 10, 20, 20}, 5, 5),
3068-
paint);
3069-
builder.DrawPath(kTestPath1, paint);
3070-
builder.DrawArc({10, 10, 20, 20}, 45, 90, true, paint);
3071-
SkPoint pts[] = {{10, 10}, {20, 20}};
3072-
builder.DrawPoints(PointMode::kLines, 2, pts, paint);
3073-
builder.DrawVertices(TestVertices1, DlBlendMode::kSrcOver, paint);
3074-
builder.DrawImage(TestImage1, {10, 10}, DlImageSampling::kLinear,
3075-
&paint);
3076-
builder.DrawImageRect(TestImage1, SkRect{0.0f, 0.0f, 10.0f, 10.0f},
3077-
SkRect{10.0f, 10.0f, 25.0f, 25.0f},
3078-
DlImageSampling::kLinear, &paint);
3079-
builder.DrawImageNine(TestImage1, {10, 10, 20, 20},
3080-
{10, 10, 100, 100}, DlFilterMode::kLinear,
3081-
&paint);
3082-
SkRSXform xforms[] = {{1, 0, 10, 10}, {0, 1, 10, 10}};
3083-
SkRect rects[] = {{10, 10, 20, 20}, {10, 20, 30, 20}};
3084-
builder.DrawAtlas(TestImage1, xforms, rects, nullptr, 2,
3085-
DlBlendMode::kSrcOver, DlImageSampling::kLinear,
3086-
nullptr, &paint);
3087-
builder.DrawTextBlob(TestBlob1, 10, 10, paint);
3088-
3089-
// Dst mode eliminates most rendering ops except for
3090-
// the following two, so we'll prune those manually...
3091-
if (paint.getBlendMode() != DlBlendMode::kDst) {
3092-
builder.DrawDisplayList(TestDisplayList1, paint.getOpacity());
3093-
builder.DrawShadow(kTestPath1, paint.getColor(), 1, true, 1);
3094-
}
3095-
},
3096-
expected_op_count);
3097-
run_one_test(
3098-
name + " SaveLayer",
3099-
[](DisplayListBuilder& builder, DlPaint& paint) {
3100-
builder.SaveLayer(nullptr, &paint, nullptr);
3101-
builder.DrawRect({10, 10, 20, 20}, DlPaint());
3102-
builder.Restore();
3103-
},
3104-
expected_op_count);
3105-
run_one_test(
3106-
name + " inside Save",
3107-
[](DisplayListBuilder& builder, DlPaint& paint) {
3108-
builder.Save();
3109-
builder.DrawRect({10, 10, 20, 20}, paint);
3110-
builder.Restore();
3111-
},
3112-
expected_op_count);
3113-
};
3114-
run_tests("transparent color", //
3115-
[](DisplayListBuilder& builder, DlPaint& paint) {
3116-
paint.setColor(DlColor::kTransparent());
3117-
});
3118-
run_tests("0 alpha", //
3119-
[](DisplayListBuilder& builder, DlPaint& paint) {
3120-
// The transparent test above already tested transparent
3121-
// black (all 0s), we set White color here so we can test
3122-
// the case of all 1s with a 0 alpha
3123-
paint.setColor(DlColor::kWhite());
3124-
paint.setAlpha(0);
3125-
});
3126-
run_tests("BlendMode::kDst", //
3127-
[](DisplayListBuilder& builder, DlPaint& paint) {
3128-
paint.setBlendMode(DlBlendMode::kDst);
3129-
});
3130-
run_tests("Empty rect clip", //
3131-
[](DisplayListBuilder& builder, DlPaint& paint) {
3132-
builder.ClipRect(SkRect::MakeEmpty(), ClipOp::kIntersect, false);
3133-
});
3134-
run_tests("Empty rrect clip", //
3135-
[](DisplayListBuilder& builder, DlPaint& paint) {
3136-
builder.ClipRRect(SkRRect::MakeEmpty(), ClipOp::kIntersect,
3137-
false);
3138-
});
3139-
run_tests("Empty path clip", //
3140-
[](DisplayListBuilder& builder, DlPaint& paint) {
3141-
builder.ClipPath(SkPath(), ClipOp::kIntersect, false);
3142-
});
3143-
run_tests("Transparent SaveLayer", //
3144-
[](DisplayListBuilder& builder, DlPaint& paint) {
3145-
DlPaint save_paint;
3146-
save_paint.setColor(DlColor::kTransparent());
3147-
builder.SaveLayer(nullptr, &save_paint);
3148-
});
3149-
run_tests("0 alpha SaveLayer", //
3150-
[](DisplayListBuilder& builder, DlPaint& paint) {
3151-
DlPaint save_paint;
3152-
// The transparent test above already tested transparent
3153-
// black (all 0s), we set White color here so we can test
3154-
// the case of all 1s with a 0 alpha
3155-
save_paint.setColor(DlColor::kWhite());
3156-
save_paint.setAlpha(0);
3157-
builder.SaveLayer(nullptr, &save_paint);
3158-
});
3159-
run_tests("Dst blended SaveLayer", //
3160-
[](DisplayListBuilder& builder, DlPaint& paint) {
3161-
DlPaint save_paint;
3162-
save_paint.setBlendMode(DlBlendMode::kDst);
3163-
builder.SaveLayer(nullptr, &save_paint);
3164-
});
3165-
run_tests(
3166-
"Nop inside SaveLayer",
3167-
[](DisplayListBuilder& builder, DlPaint& paint) {
3168-
builder.SaveLayer(nullptr, nullptr);
3169-
paint.setBlendMode(DlBlendMode::kDst);
3170-
},
3171-
2u);
3172-
run_tests("DrawImage inside Culled SaveLayer", //
3173-
[](DisplayListBuilder& builder, DlPaint& paint) {
3174-
DlPaint save_paint;
3175-
save_paint.setColor(DlColor::kTransparent());
3176-
builder.SaveLayer(nullptr, &save_paint);
3177-
builder.DrawImage(TestImage1, {10, 10}, DlImageSampling::kLinear);
3178-
});
3179-
}
3180-
31813021
} // namespace testing
31823022
} // namespace flutter

0 commit comments

Comments
 (0)