From d3a8e2e4d81dcaf846a02a521a5d8e47cdc2667c Mon Sep 17 00:00:00 2001 From: Jim Graham Date: Mon, 24 Apr 2023 15:58:27 -0700 Subject: [PATCH 1/8] add non-rendering operation culling to DisplayListBuilder --- .../benchmarking/dl_complexity_unittests.cc | 2 +- display_list/display_list_unittests.cc | 159 ++++++ display_list/dl_builder.cc | 500 +++++++++++++----- display_list/dl_builder.h | 43 +- display_list/dl_color.h | 24 +- display_list/dl_paint.h | 1 + .../testing/dl_rendering_unittests.cc | 9 +- display_list/testing/dl_test_snippets.cc | 2 +- display_list/testing/dl_test_snippets.h | 6 +- display_list/utils/dl_matrix_clip_tracker.h | 4 +- flow/diff_context_unittests.cc | 2 +- flow/layers/container_layer_unittests.cc | 12 +- flow/layers/display_list_layer_unittests.cc | 19 +- flow/layers/opacity_layer_unittests.cc | 4 +- flow/testing/diff_context_test.cc | 2 +- flow/testing/diff_context_test.h | 3 +- shell/common/dl_op_spy.cc | 2 +- shell/common/dl_op_spy_unittests.cc | 170 +++--- 18 files changed, 707 insertions(+), 257 deletions(-) diff --git a/display_list/benchmarking/dl_complexity_unittests.cc b/display_list/benchmarking/dl_complexity_unittests.cc index 4ec6111485d88..eee77ac9bdf55 100644 --- a/display_list/benchmarking/dl_complexity_unittests.cc +++ b/display_list/benchmarking/dl_complexity_unittests.cc @@ -423,7 +423,7 @@ TEST(DisplayListComplexity, DrawAtlas) { std::vector 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; diff --git a/display_list/display_list_unittests.cc b/display_list/display_list_unittests.cc index f63435acdd1a5..b0e05d3fff120 100644 --- a/display_list/display_list_unittests.cc +++ b/display_list/display_list_unittests.cc @@ -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 { @@ -2636,5 +2637,163 @@ TEST_F(DisplayListTest, DrawSaveDrawCannotInheritOpacity) { ASSERT_FALSE(display_list->can_apply_group_opacity()); } +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, {0, 0, 10, 10}, {10, 10, 25, 25}, + 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 diff --git a/display_list/dl_builder.cc b/display_list/dl_builder.cc index 731ac6307bc9c..d45627b00e2f8 100644 --- a/display_list/dl_builder.cc +++ b/display_list/dl_builder.cc @@ -6,10 +6,12 @@ #include "flutter/display_list/display_list.h" #include "flutter/display_list/dl_blend_mode.h" +#include "flutter/display_list/dl_op_flags.h" #include "flutter/display_list/dl_op_records.h" #include "flutter/display_list/effects/dl_color_source.h" #include "flutter/display_list/utils/dl_bounds_accumulator.h" #include "fml/logging.h" +#include "third_party/skia/include/core/SkScalar.h" namespace flutter { @@ -382,9 +384,11 @@ void DisplayListBuilder::checkForDeferredSave() { } void DisplayListBuilder::Save() { + bool is_nop = current_layer_->is_nop_; layer_stack_.emplace_back(); current_layer_ = &layer_stack_.back(); current_layer_->has_deferred_save_op_ = true; + current_layer_->is_nop_ = is_nop; tracker_.save(); accumulator()->save(); } @@ -471,18 +475,14 @@ void DisplayListBuilder::saveLayer(const SkRect* bounds, const SaveLayerOptions in_options, const DlImageFilter* backdrop) { SaveLayerOptions options = in_options.without_optimizations(); - size_t save_layer_offset = used_; - if (backdrop) { - bounds // - ? Push(0, 1, options, *bounds, backdrop) - : Push(0, 1, options, backdrop); - } else { - bounds // - ? Push(0, 1, options, *bounds) - : Push(0, 1, options); + if (current_layer_->is_nop_ || + (options.renders_with_attributes() && + !paint_affects_dest(current_, kSaveLayerWithPaintFlags))) { + save(); + current_layer_->is_nop_ = true; + return; } - CheckLayerOpacityCompatibility(options.renders_with_attributes()); - + size_t save_layer_offset = used_; if (options.renders_with_attributes()) { // The actual flood of the outer layer clip will occur after the // (eventual) corresponding restore is called, but rather than @@ -496,14 +496,27 @@ void DisplayListBuilder::saveLayer(const SkRect* bounds, // We will fill the clip of the outer layer when we restore AccumulateUnbounded(); } + CheckLayerOpacityCompatibility(true); layer_stack_.emplace_back(save_layer_offset, true, current_.getImageFilter()); } else { + CheckLayerOpacityCompatibility(false); layer_stack_.emplace_back(save_layer_offset, true, nullptr); } tracker_.save(); accumulator()->save(); current_layer_ = &layer_stack_.back(); + + if (backdrop) { + bounds // + ? Push(0, 1, options, *bounds, backdrop) + : Push(0, 1, options, backdrop); + } else { + bounds // + ? Push(0, 1, options, *bounds) + : Push(0, 1, options); + } + if (options.renders_with_attributes()) { // |current_opacity_compatibility_| does not take an ImageFilter into // account because an individual primitive with an ImageFilter can apply @@ -647,6 +660,11 @@ void DisplayListBuilder::ClipRect(const SkRect& rect, if (!rect.isFinite()) { return; } + tracker_.clipRect(rect, clip_op, is_aa); + if (current_layer_->is_nop_ || tracker_.is_cull_rect_empty()) { + current_layer_->is_nop_ = true; + return; + } checkForDeferredSave(); switch (clip_op) { case ClipOp::kIntersect: @@ -656,7 +674,6 @@ void DisplayListBuilder::ClipRect(const SkRect& rect, Push(0, 1, rect, is_aa); break; } - tracker_.clipRect(rect, clip_op, is_aa); } void DisplayListBuilder::ClipRRect(const SkRRect& rrect, ClipOp clip_op, @@ -664,6 +681,11 @@ void DisplayListBuilder::ClipRRect(const SkRRect& rrect, if (rrect.isRect()) { clipRect(rrect.rect(), clip_op, is_aa); } else { + tracker_.clipRRect(rrect, clip_op, is_aa); + if (current_layer_->is_nop_ || tracker_.is_cull_rect_empty()) { + current_layer_->is_nop_ = true; + return; + } checkForDeferredSave(); switch (clip_op) { case ClipOp::kIntersect: @@ -673,7 +695,6 @@ void DisplayListBuilder::ClipRRect(const SkRRect& rrect, Push(0, 1, rrect, is_aa); break; } - tracker_.clipRRect(rrect, clip_op, is_aa); } } void DisplayListBuilder::ClipPath(const SkPath& path, @@ -696,6 +717,11 @@ void DisplayListBuilder::ClipPath(const SkPath& path, return; } } + tracker_.clipPath(path, clip_op, is_aa); + if (current_layer_->is_nop_ || tracker_.is_cull_rect_empty()) { + current_layer_->is_nop_ = true; + return; + } checkForDeferredSave(); switch (clip_op) { case ClipOp::kIntersect: @@ -705,7 +731,6 @@ void DisplayListBuilder::ClipPath(const SkPath& path, Push(0, 1, path, is_aa); break; } - tracker_.clipPath(path, clip_op, is_aa); } bool DisplayListBuilder::QuickReject(const SkRect& bounds) const { @@ -713,27 +738,33 @@ bool DisplayListBuilder::QuickReject(const SkRect& bounds) const { } void DisplayListBuilder::drawPaint() { - Push(0, 1); - CheckLayerOpacityCompatibility(); - AccumulateUnbounded(); + if (paint_affects_dest(current_, kDrawPaintFlags) && // + AccumulateUnbounded()) { + Push(0, 1); + CheckLayerOpacityCompatibility(); + } } void DisplayListBuilder::DrawPaint(const DlPaint& paint) { SetAttributesFromPaint(paint, DisplayListOpFlags::kDrawPaintFlags); drawPaint(); } void DisplayListBuilder::DrawColor(DlColor color, DlBlendMode mode) { - Push(0, 1, color, mode); - CheckLayerOpacityCompatibility(mode); - AccumulateUnbounded(); + if (paint_affects_dest(DlPaint(color).setBlendMode(mode)) && + AccumulateUnbounded()) { + Push(0, 1, color, mode); + CheckLayerOpacityCompatibility(mode); + } } void DisplayListBuilder::drawLine(const SkPoint& p0, const SkPoint& p1) { - Push(0, 1, p0, p1); - CheckLayerOpacityCompatibility(); SkRect bounds = SkRect::MakeLTRB(p0.fX, p0.fY, p1.fX, p1.fY).makeSorted(); DisplayListAttributeFlags flags = (bounds.width() > 0.0f && bounds.height() > 0.0f) ? kDrawLineFlags : kDrawHVLineFlags; - AccumulateOpBounds(bounds, flags); + if (paint_affects_dest(current_, flags) && + AccumulateOpBounds(bounds, flags)) { + Push(0, 1, p0, p1); + CheckLayerOpacityCompatibility(); + } } void DisplayListBuilder::DrawLine(const SkPoint& p0, const SkPoint& p1, @@ -742,29 +773,36 @@ void DisplayListBuilder::DrawLine(const SkPoint& p0, drawLine(p0, p1); } void DisplayListBuilder::drawRect(const SkRect& rect) { - Push(0, 1, rect); - CheckLayerOpacityCompatibility(); - AccumulateOpBounds(rect, kDrawRectFlags); + if (paint_affects_dest(current_, kDrawRectFlags) && + AccumulateOpBounds(rect, kDrawRectFlags)) { + Push(0, 1, rect); + CheckLayerOpacityCompatibility(); + } } void DisplayListBuilder::DrawRect(const SkRect& rect, const DlPaint& paint) { SetAttributesFromPaint(paint, DisplayListOpFlags::kDrawRectFlags); drawRect(rect); } void DisplayListBuilder::drawOval(const SkRect& bounds) { - Push(0, 1, bounds); - CheckLayerOpacityCompatibility(); - AccumulateOpBounds(bounds, kDrawOvalFlags); + if (paint_affects_dest(current_, kDrawOvalFlags) && + AccumulateOpBounds(bounds, kDrawOvalFlags)) { + Push(0, 1, bounds); + CheckLayerOpacityCompatibility(); + } } void DisplayListBuilder::DrawOval(const SkRect& bounds, const DlPaint& paint) { SetAttributesFromPaint(paint, DisplayListOpFlags::kDrawOvalFlags); drawOval(bounds); } void DisplayListBuilder::drawCircle(const SkPoint& center, SkScalar radius) { - Push(0, 1, center, radius); - CheckLayerOpacityCompatibility(); - AccumulateOpBounds(SkRect::MakeLTRB(center.fX - radius, center.fY - radius, - center.fX + radius, center.fY + radius), - kDrawCircleFlags); + if (paint_affects_dest(current_, kDrawCircleFlags)) { + SkRect bounds = SkRect::MakeLTRB(center.fX - radius, center.fY - radius, + center.fX + radius, center.fY + radius); + if (AccumulateOpBounds(bounds, kDrawCircleFlags)) { + Push(0, 1, center, radius); + CheckLayerOpacityCompatibility(); + } + } } void DisplayListBuilder::DrawCircle(const SkPoint& center, SkScalar radius, @@ -778,9 +816,11 @@ void DisplayListBuilder::drawRRect(const SkRRect& rrect) { } else if (rrect.isOval()) { drawOval(rrect.rect()); } else { - Push(0, 1, rrect); - CheckLayerOpacityCompatibility(); - AccumulateOpBounds(rrect.getBounds(), kDrawRRectFlags); + if (paint_affects_dest(current_, kDrawRRectFlags) && + AccumulateOpBounds(rrect.getBounds(), kDrawRRectFlags)) { + Push(0, 1, rrect); + CheckLayerOpacityCompatibility(); + } } } void DisplayListBuilder::DrawRRect(const SkRRect& rrect, const DlPaint& paint) { @@ -789,9 +829,11 @@ void DisplayListBuilder::DrawRRect(const SkRRect& rrect, const DlPaint& paint) { } void DisplayListBuilder::drawDRRect(const SkRRect& outer, const SkRRect& inner) { - Push(0, 1, outer, inner); - CheckLayerOpacityCompatibility(); - AccumulateOpBounds(outer.getBounds(), kDrawDRRectFlags); + if (paint_affects_dest(current_, kDrawDRRectFlags) && + AccumulateOpBounds(outer.getBounds(), kDrawDRRectFlags)) { + Push(0, 1, outer, inner); + CheckLayerOpacityCompatibility(); + } } void DisplayListBuilder::DrawDRRect(const SkRRect& outer, const SkRRect& inner, @@ -800,12 +842,15 @@ void DisplayListBuilder::DrawDRRect(const SkRRect& outer, drawDRRect(outer, inner); } void DisplayListBuilder::drawPath(const SkPath& path) { - Push(0, 1, path); - CheckLayerOpacityHairlineCompatibility(); - if (path.isInverseFillType()) { - AccumulateUnbounded(); - } else { - AccumulateOpBounds(path.getBounds(), kDrawPathFlags); + if (paint_affects_dest(current_, kDrawPathFlags)) { + bool is_visible = + path.isInverseFillType() + ? AccumulateUnbounded() + : AccumulateOpBounds(path.getBounds(), kDrawPathFlags); + if (is_visible) { + Push(0, 1, path); + CheckLayerOpacityHairlineCompatibility(); + } } } void DisplayListBuilder::DrawPath(const SkPath& path, const DlPaint& paint) { @@ -817,19 +862,22 @@ void DisplayListBuilder::drawArc(const SkRect& bounds, SkScalar start, SkScalar sweep, bool useCenter) { - Push(0, 1, bounds, start, sweep, useCenter); - if (useCenter) { - CheckLayerOpacityHairlineCompatibility(); - } else { - CheckLayerOpacityCompatibility(); - } + DisplayListAttributeFlags flags = // + useCenter // + ? kDrawArcWithCenterFlags + : kDrawArcNoCenterFlags; // This could be tighter if we compute where the start and end // angles are and then also consider the quadrants swept and // the center if specified. - AccumulateOpBounds(bounds, - useCenter // - ? kDrawArcWithCenterFlags - : kDrawArcNoCenterFlags); + if (paint_affects_dest(current_, flags) && + AccumulateOpBounds(bounds, flags)) { + Push(0, 1, bounds, start, sweep, useCenter); + if (useCenter) { + CheckLayerOpacityHairlineCompatibility(); + } else { + CheckLayerOpacityCompatibility(); + } + } } void DisplayListBuilder::DrawArc(const SkRect& bounds, SkScalar start, @@ -840,14 +888,30 @@ void DisplayListBuilder::DrawArc(const SkRect& bounds, paint, useCenter ? kDrawArcWithCenterFlags : kDrawArcNoCenterFlags); drawArc(bounds, start, sweep, useCenter); } +DisplayListAttributeFlags DisplayListBuilder::FlagsForPointMode( + PointMode mode) { + switch (mode) { + case DlCanvas::PointMode::kPoints: + return kDrawPointsAsPointsFlags; + case PointMode::kLines: + return kDrawPointsAsLinesFlags; + case PointMode::kPolygon: + return kDrawPointsAsPolygonFlags; + } + FML_DCHECK(false); +} + void DisplayListBuilder::drawPoints(PointMode mode, uint32_t count, const SkPoint pts[]) { if (count == 0) { return; } + DisplayListAttributeFlags flags = FlagsForPointMode(mode); + if (!paint_affects_dest(current_, flags)) { + return; + } - void* data_ptr; FML_DCHECK(count < DlOpReceiver::kMaxDrawPointsCount); int bytes = count * sizeof(SkPoint); RectBoundsAccumulator ptBounds; @@ -855,18 +919,20 @@ void DisplayListBuilder::drawPoints(PointMode mode, ptBounds.accumulate(pts[i]); } SkRect point_bounds = ptBounds.bounds(); + if (!AccumulateOpBounds(point_bounds, flags)) { + return; + } + + void* data_ptr; switch (mode) { case PointMode::kPoints: data_ptr = Push(bytes, 1, count); - AccumulateOpBounds(point_bounds, kDrawPointsAsPointsFlags); break; case PointMode::kLines: data_ptr = Push(bytes, 1, count); - AccumulateOpBounds(point_bounds, kDrawPointsAsLinesFlags); break; case PointMode::kPolygon: data_ptr = Push(bytes, 1, count); - AccumulateOpBounds(point_bounds, kDrawPointsAsPolygonFlags); break; default: FML_DCHECK(false); @@ -904,14 +970,16 @@ void DisplayListBuilder::DrawPoints(PointMode mode, } void DisplayListBuilder::drawVertices(const DlVertices* vertices, DlBlendMode mode) { - void* pod = Push(vertices->size(), 1, mode); - new (pod) DlVertices(vertices); - // DrawVertices applies its colors to the paint so we have no way - // of controlling opacity using the current paint attributes. - // Although, examination of the |mode| might find some predictable - // cases. - UpdateLayerOpacityCompatibility(false); - AccumulateOpBounds(vertices->bounds(), kDrawVerticesFlags); + if (paint_affects_dest(current_, kDrawVerticesFlags) && + AccumulateOpBounds(vertices->bounds(), kDrawVerticesFlags)) { + void* pod = Push(vertices->size(), 1, mode); + new (pod) DlVertices(vertices); + // DrawVertices applies its colors to the paint so we have no way + // of controlling opacity using the current paint attributes. + // Although, examination of the |mode| might find some predictable + // cases. + UpdateLayerOpacityCompatibility(false); + } } void DisplayListBuilder::DrawVertices(const DlVertices* vertices, DlBlendMode mode, @@ -924,17 +992,26 @@ void DisplayListBuilder::drawImage(const sk_sp image, const SkPoint point, DlImageSampling sampling, bool render_with_attributes) { - render_with_attributes - ? Push(0, 1, image, point, sampling) - : Push(0, 1, image, point, sampling); - CheckLayerOpacityCompatibility(render_with_attributes); - is_ui_thread_safe_ = is_ui_thread_safe_ && image->isUIThreadSafe(); - SkRect bounds = SkRect::MakeXYWH(point.fX, point.fY, // - image->width(), image->height()); DisplayListAttributeFlags flags = render_with_attributes // ? kDrawImageWithPaintFlags : kDrawImageFlags; - AccumulateOpBounds(bounds, flags); + if (render_with_attributes) { + // paint_affects_dest checks layer->is_nop + if (!paint_affects_dest(current_, flags)) { + return; + } + } else if (current_layer_->is_nop_) { + return; + } + SkRect bounds = SkRect::MakeXYWH(point.fX, point.fY, // + image->width(), image->height()); + if (AccumulateOpBounds(bounds, flags)) { + render_with_attributes + ? Push(0, 1, image, point, sampling) + : Push(0, 1, image, point, sampling); + CheckLayerOpacityCompatibility(render_with_attributes); + is_ui_thread_safe_ = is_ui_thread_safe_ && image->isUIThreadSafe(); + } } void DisplayListBuilder::DrawImage(const sk_sp& image, const SkPoint point, @@ -954,14 +1031,23 @@ void DisplayListBuilder::drawImageRect(const sk_sp image, DlImageSampling sampling, bool render_with_attributes, SrcRectConstraint constraint) { - Push(0, 1, image, src, dst, sampling, render_with_attributes, - constraint); - CheckLayerOpacityCompatibility(render_with_attributes); - is_ui_thread_safe_ = is_ui_thread_safe_ && image->isUIThreadSafe(); DisplayListAttributeFlags flags = render_with_attributes ? kDrawImageRectWithPaintFlags : kDrawImageRectFlags; - AccumulateOpBounds(dst, flags); + if (render_with_attributes) { + // paint_affects_dest checks layer->is_nop + if (!paint_affects_dest(current_, flags)) { + return; + } + } else if (current_layer_->is_nop_) { + return; + } + if (AccumulateOpBounds(dst, flags)) { + Push(0, 1, image, src, dst, sampling, + render_with_attributes, constraint); + CheckLayerOpacityCompatibility(render_with_attributes); + is_ui_thread_safe_ = is_ui_thread_safe_ && image->isUIThreadSafe(); + } } void DisplayListBuilder::DrawImageRect(const sk_sp& image, const SkRect& src, @@ -982,15 +1068,24 @@ void DisplayListBuilder::drawImageNine(const sk_sp image, const SkRect& dst, DlFilterMode filter, bool render_with_attributes) { - render_with_attributes - ? Push(0, 1, image, center, dst, filter) - : Push(0, 1, image, center, dst, filter); - CheckLayerOpacityCompatibility(render_with_attributes); - is_ui_thread_safe_ = is_ui_thread_safe_ && image->isUIThreadSafe(); DisplayListAttributeFlags flags = render_with_attributes ? kDrawImageNineWithPaintFlags : kDrawImageNineFlags; - AccumulateOpBounds(dst, flags); + if (render_with_attributes) { + // paint_affects_dest checks layer->is_nop + if (!paint_affects_dest(current_, flags)) { + return; + } + } else if (current_layer_->is_nop_) { + return; + } + if (AccumulateOpBounds(dst, flags)) { + render_with_attributes + ? Push(0, 1, image, center, dst, filter) + : Push(0, 1, image, center, dst, filter); + CheckLayerOpacityCompatibility(render_with_attributes); + is_ui_thread_safe_ = is_ui_thread_safe_ && image->isUIThreadSafe(); + } } void DisplayListBuilder::DrawImageNine(const sk_sp& image, const SkIRect& center, @@ -1014,6 +1109,31 @@ void DisplayListBuilder::drawAtlas(const sk_sp atlas, DlImageSampling sampling, const SkRect* cull_rect, bool render_with_attributes) { + DisplayListAttributeFlags flags = render_with_attributes // + ? kDrawAtlasWithPaintFlags + : kDrawAtlasFlags; + if (render_with_attributes) { + // paint_affects_dest checks layer->is_nop + if (!paint_affects_dest(current_, flags)) { + return; + } + } else if (current_layer_->is_nop_) { + return; + } + SkPoint quad[4]; + RectBoundsAccumulator atlasBounds; + for (int i = 0; i < count; i++) { + const SkRect& src = tex[i]; + xform[i].toQuad(src.width(), src.height(), quad); + for (int j = 0; j < 4; j++) { + atlasBounds.accumulate(quad[j]); + } + } + if (atlasBounds.is_empty() || + !AccumulateOpBounds(atlasBounds.bounds(), flags)) { + return; + } + int bytes = count * (sizeof(SkRSXform) + sizeof(SkRect)); void* data_ptr; if (colors != nullptr) { @@ -1043,22 +1163,6 @@ void DisplayListBuilder::drawAtlas(const sk_sp atlas, // of the transforms and texture rectangles. UpdateLayerOpacityCompatibility(false); is_ui_thread_safe_ = is_ui_thread_safe_ && atlas->isUIThreadSafe(); - - SkPoint quad[4]; - RectBoundsAccumulator atlasBounds; - for (int i = 0; i < count; i++) { - const SkRect& src = tex[i]; - xform[i].toQuad(src.width(), src.height(), quad); - for (int j = 0; j < 4; j++) { - atlasBounds.accumulate(quad[j]); - } - } - if (atlasBounds.is_not_empty()) { - DisplayListAttributeFlags flags = render_with_attributes // - ? kDrawAtlasWithPaintFlags - : kDrawAtlasFlags; - AccumulateOpBounds(atlasBounds.bounds(), flags); - } } void DisplayListBuilder::DrawAtlas(const sk_sp& atlas, const SkRSXform xform[], @@ -1082,34 +1186,48 @@ void DisplayListBuilder::DrawAtlas(const sk_sp& atlas, void DisplayListBuilder::DrawDisplayList(const sk_sp display_list, SkScalar opacity) { - DlPaint current_paint = current_; - Push(0, 1, display_list, opacity); - is_ui_thread_safe_ = is_ui_thread_safe_ && display_list->isUIThreadSafe(); - // Not really necessary if the developer is interacting with us via - // our attribute-state-less DlCanvas methods, but this avoids surprises - // for those who may have been using the stateful Dispatcher methods. - SetAttributesFromPaint(current_paint, - DisplayListOpFlags::kSaveLayerWithPaintFlags); - + if (!SkScalarIsFinite(opacity) || opacity <= SK_ScalarNearlyZero || + display_list->op_count() == 0 || display_list->bounds().isEmpty() || + current_layer_->is_nop_) { + return; + } const SkRect bounds = display_list->bounds(); + bool accumulated; switch (accumulator()->type()) { case BoundsAccumulatorType::kRect: - AccumulateOpBounds(bounds, kDrawDisplayListFlags); + accumulated = AccumulateOpBounds(bounds, kDrawDisplayListFlags); break; case BoundsAccumulatorType::kRTree: auto rtree = display_list->rtree(); if (rtree) { std::list rects = rtree->searchAndConsolidateRects(bounds); + accumulated = false; for (const SkRect& rect : rects) { // TODO (https://github.com/flutter/flutter/issues/114919): Attributes // are not necessarily `kDrawDisplayListFlags`. - AccumulateOpBounds(rect, kDrawDisplayListFlags); + if (AccumulateOpBounds(rect, kDrawDisplayListFlags)) { + accumulated = true; + } } } else { - AccumulateOpBounds(bounds, kDrawDisplayListFlags); + accumulated = AccumulateOpBounds(bounds, kDrawDisplayListFlags); } break; } + if (!accumulated) { + return; + } + + DlPaint current_paint = current_; + Push(0, 1, display_list, + opacity < SK_Scalar1 ? opacity : SK_Scalar1); + is_ui_thread_safe_ = is_ui_thread_safe_ && display_list->isUIThreadSafe(); + // Not really necessary if the developer is interacting with us via + // our attribute-state-less DlCanvas methods, but this avoids surprises + // for those who may have been using the stateful Dispatcher methods. + SetAttributesFromPaint(current_paint, + DisplayListOpFlags::kSaveLayerWithPaintFlags); + // The non-nested op count accumulated in the |Push| method will include // this call to |drawDisplayList| for non-nested op count metrics. // But, for nested op count metrics we want the |drawDisplayList| call itself @@ -1123,14 +1241,16 @@ void DisplayListBuilder::DrawDisplayList(const sk_sp display_list, void DisplayListBuilder::drawTextBlob(const sk_sp blob, SkScalar x, SkScalar y) { - Push(0, 1, blob, x, y); - AccumulateOpBounds(blob->bounds().makeOffset(x, y), kDrawTextBlobFlags); - // There is no way to query if the glyphs of a text blob overlap and - // there are no current guarantees from either Skia or Impeller that - // they will protect overlapping glyphs from the effects of overdraw - // so we must make the conservative assessment that this DL layer is - // not compatible with group opacity inheritance. - UpdateLayerOpacityCompatibility(false); + if (paint_affects_dest(current_, kDrawTextBlobFlags) && + AccumulateOpBounds(blob->bounds().makeOffset(x, y), kDrawTextBlobFlags)) { + Push(0, 1, blob, x, y); + // There is no way to query if the glyphs of a text blob overlap and + // there are no current guarantees from either Skia or Impeller that + // they will protect overlapping glyphs from the effects of overdraw + // so we must make the conservative assessment that this DL layer is + // not compatible with group opacity inheritance. + UpdateLayerOpacityCompatibility(false); + } } void DisplayListBuilder::DrawTextBlob(const sk_sp& blob, SkScalar x, @@ -1144,14 +1264,17 @@ void DisplayListBuilder::DrawShadow(const SkPath& path, const SkScalar elevation, bool transparent_occluder, SkScalar dpr) { - transparent_occluder // - ? Push(0, 1, path, color, elevation, dpr) - : Push(0, 1, path, color, elevation, dpr); - - SkRect shadow_bounds = - DlCanvas::ComputeShadowBounds(path, elevation, dpr, GetTransform()); - AccumulateOpBounds(shadow_bounds, kDrawShadowFlags); - UpdateLayerOpacityCompatibility(false); + if (paint_affects_dest(DlPaint(color))) { + SkRect shadow_bounds = + DlCanvas::ComputeShadowBounds(path, elevation, dpr, GetTransform()); + if (AccumulateOpBounds(shadow_bounds, kDrawShadowFlags)) { + transparent_occluder // + ? Push(0, 1, path, color, elevation, + dpr) + : Push(0, 1, path, color, elevation, dpr); + UpdateLayerOpacityCompatibility(false); + } + } } bool DisplayListBuilder::ComputeFilteredBounds(SkRect& bounds, @@ -1221,31 +1344,40 @@ bool DisplayListBuilder::AdjustBoundsForPaint(SkRect& bounds, return true; } -void DisplayListBuilder::AccumulateUnbounded() { - accumulator()->accumulate(tracker_.device_cull_rect(), op_index_ - 1); +bool DisplayListBuilder::AccumulateUnbounded() { + SkRect clip = tracker_.device_cull_rect(); + if (clip.isEmpty()) { + return false; + } + accumulator()->accumulate(clip, op_index_); + return true; } -void DisplayListBuilder::AccumulateOpBounds(SkRect& bounds, +bool DisplayListBuilder::AccumulateOpBounds(SkRect& bounds, DisplayListAttributeFlags flags) { if (AdjustBoundsForPaint(bounds, flags)) { - AccumulateBounds(bounds); + return AccumulateBounds(bounds); } else { - AccumulateUnbounded(); + return AccumulateUnbounded(); } } -void DisplayListBuilder::AccumulateBounds(SkRect& bounds) { - tracker_.mapRect(&bounds); - if (bounds.intersect(tracker_.device_cull_rect())) { - accumulator()->accumulate(bounds, op_index_ - 1); +bool DisplayListBuilder::AccumulateBounds(SkRect& bounds) { + if (!bounds.isEmpty()) { + tracker_.mapRect(&bounds); + if (bounds.intersect(tracker_.device_cull_rect())) { + accumulator()->accumulate(bounds, op_index_); + return true; + } } + return false; } bool DisplayListBuilder::paint_nops_on_transparency() { // SkImageFilter::canComputeFastBounds tests for transparency behavior // This test assumes that the blend mode checked down below will // NOP on transparent black. - if (current_.getImageFilter() && - current_.getImageFilter()->modifies_transparent_black()) { + if (current_.getImageFilterPtr() && + current_.getImageFilterPtr()->modifies_transparent_black()) { return false; } @@ -1255,8 +1387,8 @@ bool DisplayListBuilder::paint_nops_on_transparency() { // save layer untouched out to the edge of the output surface. // This test assumes that the blend mode checked down below will // NOP on transparent black. - if (current_.getColorFilter() && - current_.getColorFilter()->modifies_transparent_black()) { + if (current_.getColorFilterPtr() && + current_.getColorFilterPtr()->modifies_transparent_black()) { return false; } @@ -1313,4 +1445,84 @@ bool DisplayListBuilder::paint_nops_on_transparency() { break; } } + +DlColor DisplayListBuilder::GetEffectiveColor(const DlPaint& paint, + DisplayListAttributeFlags flags) { + if (flags.applies_image_filter() && paint.getImageFilterPtr()) { + return kAnyColor; + } + if (flags.applies_color_filter() && paint.getColorFilterPtr()) { + return kAnyColor; + } + if (flags.applies_alpha_or_color()) { + const DlColorSource* source = paint.getColorSourcePtr(); + if (source) { + if (source->asColor()) { + return source->asColor()->color(); + } + return source->is_opaque() ? DlColor::kBlack() : kAnyColor; + } + return paint.getColor(); + } + return kAnyColor; +} + +bool DisplayListBuilder::paint_affects_dest(const DlPaint& paint, + DisplayListAttributeFlags flags) { + if (current_layer_->is_nop_) { + return false; + } + if (flags.applies_blend()) { + switch (paint.getBlendMode()) { + // Nop blend mode (singular, there is only one) + case DlBlendMode::kDst: + return false; + + // Always destructive blend modes + // (Some answers might differ if dest is opaque, but that is unknown) + case DlBlendMode::kSrc: + case DlBlendMode::kClear: + case DlBlendMode::kSrcIn: + case DlBlendMode::kSrcOut: + case DlBlendMode::kDstATop: + case DlBlendMode::kModulate: + case DlBlendMode::kHue: + case DlBlendMode::kSaturation: + case DlBlendMode::kColor: + case DlBlendMode::kLuminosity: + return true; + + // The next group of blend modes modifies the destination unless the + // source color is opaque (or there is a color or image filter). + case DlBlendMode::kDstIn: + return !GetEffectiveColor(paint, flags).isOpaque(); + + // The next group of blend modes modifies the destination unless the + // source color is transparent (or there is a color or image filter). + case DlBlendMode::kSrcOver: + case DlBlendMode::kDstOver: + case DlBlendMode::kDstOut: + case DlBlendMode::kSrcATop: + case DlBlendMode::kXor: + case DlBlendMode::kPlus: + case DlBlendMode::kScreen: + case DlBlendMode::kMultiply: + case DlBlendMode::kOverlay: + case DlBlendMode::kDarken: + case DlBlendMode::kLighten: + case DlBlendMode::kColorDodge: + case DlBlendMode::kHardLight: + case DlBlendMode::kSoftLight: + case DlBlendMode::kDifference: + case DlBlendMode::kExclusion: + return !GetEffectiveColor(paint, flags).isTransparent(); + + // Color Burn only leaves the pixel alone when the source is white. + case DlBlendMode::kColorBurn: + return GetEffectiveColor(paint, flags) != DlColor::kWhite(); + } + } + return true; +} + } // namespace flutter diff --git a/display_list/dl_builder.h b/display_list/dl_builder.h index 78e7d8ade81cf..db9c04b489a29 100644 --- a/display_list/dl_builder.h +++ b/display_list/dl_builder.h @@ -502,15 +502,13 @@ class DisplayListBuilder final : public virtual DlCanvas, class LayerInfo { public: - explicit LayerInfo(size_t save_offset = 0, - bool has_layer = false, - std::shared_ptr filter = nullptr) + explicit LayerInfo( + size_t save_offset = 0, + bool has_layer = false, + const std::shared_ptr& filter = nullptr) : save_offset_(save_offset), has_layer_(has_layer), - cannot_inherit_opacity_(false), - has_compatible_op_(false), - filter_(filter), - is_unbounded_(false) {} + filter_(filter) {} // The offset into the memory buffer where the saveLayer DLOp record // for this saveLayer() call is placed. This may be needed if the @@ -579,11 +577,12 @@ class DisplayListBuilder final : public virtual DlCanvas, private: size_t save_offset_; bool has_layer_; - bool cannot_inherit_opacity_; - bool has_compatible_op_; + bool cannot_inherit_opacity_ = false; + bool has_compatible_op_ = false; std::shared_ptr filter_; - bool is_unbounded_; + bool is_unbounded_ = false; bool has_deferred_save_op_ = false; + bool is_nop_ = false; friend class DisplayListBuilder; }; @@ -697,9 +696,23 @@ class DisplayListBuilder final : public virtual DlCanvas, return accumulator_->rtree(); } + static DisplayListAttributeFlags FlagsForPointMode(PointMode mode); + bool paint_nops_on_transparency(); + bool paint_affects_dest(const DlPaint& paint, + DisplayListAttributeFlags flags = kDrawPaintFlags); + + // kAnyColor is a non-opaque and non-transparent color that will not + // trigger any short-circuit tests about the results of a blend. + static constexpr DlColor kAnyColor = DlColor(0x7fffffff); + static_assert(!kAnyColor.isOpaque()); + static_assert(!kAnyColor.isTransparent()); + static DlColor GetEffectiveColor(const DlPaint& paint, + DisplayListAttributeFlags flags); // Computes the bounds of an operation adjusted for a given ImageFilter + // and returns whether the computation was possible. If the method + // returns false then the caller should assume the worst about the bounds. static bool ComputeFilteredBounds(SkRect& bounds, const DlImageFilter* filter); @@ -709,24 +722,24 @@ class DisplayListBuilder final : public virtual DlCanvas, // Records the fact that we encountered an op that either could not // estimate its bounds or that fills all of the destination space. - void AccumulateUnbounded(); + bool AccumulateUnbounded(); // Records the bounds for an op after modifying them according to the // supplied attribute flags and transforming by the current matrix. - void AccumulateOpBounds(const SkRect& bounds, + bool AccumulateOpBounds(const SkRect& bounds, DisplayListAttributeFlags flags) { SkRect safe_bounds = bounds; - AccumulateOpBounds(safe_bounds, flags); + return AccumulateOpBounds(safe_bounds, flags); } // Records the bounds for an op after modifying them according to the // supplied attribute flags and transforming by the current matrix // and clipping against the current clip. - void AccumulateOpBounds(SkRect& bounds, DisplayListAttributeFlags flags); + bool AccumulateOpBounds(SkRect& bounds, DisplayListAttributeFlags flags); // Records the given bounds after transforming by the current matrix // and clipping against the current clip. - void AccumulateBounds(SkRect& bounds); + bool AccumulateBounds(SkRect& bounds); DlPaint current_; }; diff --git a/display_list/dl_color.h b/display_list/dl_color.h index d926e58c3b818..2b6d5e8360698 100644 --- a/display_list/dl_color.h +++ b/display_list/dl_color.h @@ -34,18 +34,18 @@ struct DlColor { uint32_t argb; - bool isOpaque() const { return getAlpha() == 0xFF; } - bool isTransparent() const { return getAlpha() == 0; } - - int getAlpha() const { return argb >> 24; } - int getRed() const { return (argb >> 16) & 0xFF; } - int getGreen() const { return (argb >> 8) & 0xFF; } - int getBlue() const { return argb & 0xFF; } - - float getAlphaF() const { return toF(getAlpha()); } - float getRedF() const { return toF(getRed()); } - float getGreenF() const { return toF(getGreen()); } - float getBlueF() const { return toF(getBlue()); } + constexpr bool isOpaque() const { return getAlpha() == 0xFF; } + constexpr bool isTransparent() const { return getAlpha() == 0; } + + constexpr int getAlpha() const { return argb >> 24; } + constexpr int getRed() const { return (argb >> 16) & 0xFF; } + constexpr int getGreen() const { return (argb >> 8) & 0xFF; } + constexpr int getBlue() const { return argb & 0xFF; } + + constexpr float getAlphaF() const { return toF(getAlpha()); } + constexpr float getRedF() const { return toF(getRed()); } + constexpr float getGreenF() const { return toF(getGreen()); } + constexpr float getBlueF() const { return toF(getBlue()); } uint32_t premultipliedArgb() const { if (isOpaque()) { diff --git a/display_list/dl_paint.h b/display_list/dl_paint.h index 77619a2567164..3d9220f57e3d6 100644 --- a/display_list/dl_paint.h +++ b/display_list/dl_paint.h @@ -83,6 +83,7 @@ class DlPaint { color_.argb = alpha << 24 | (color_.argb & 0x00FFFFFF); return *this; } + SkScalar getOpacity() const { return color_.getAlphaF(); } DlPaint& setOpacity(SkScalar opacity) { setAlpha(SkScalarRoundToInt(opacity * 0xff)); return *this; diff --git a/display_list/testing/dl_rendering_unittests.cc b/display_list/testing/dl_rendering_unittests.cc index 3070b8f016ff9..30a752c8608c9 100644 --- a/display_list/testing/dl_rendering_unittests.cc +++ b/display_list/testing/dl_rendering_unittests.cc @@ -907,7 +907,14 @@ class CanvasCompareTester { }; DlRenderer dl_safe_restore = [=](DlCanvas* cv, const DlPaint& p) { // Draw another primitive to disable peephole optimizations - cv->DrawRect(kRenderBounds.makeOffset(500, 500), DlPaint()); + // As the rendering op rejection in the DisplayList Builder + // gets smarter and smarter, this operation has had to get + // sneakier and sneakier about specifying an operation that + // won't practically show up in the output, but technically + // can't be culled. + cv->DrawRect( + SkRect::MakeXYWH(kRenderCenterX, kRenderCenterY, 0.0001, 0.0001), + DlPaint()); cv->Restore(); }; SkRenderer sk_opt_restore = [=](SkCanvas* cv, const SkPaint& p) { diff --git a/display_list/testing/dl_test_snippets.cc b/display_list/testing/dl_test_snippets.cc index e0066cc7c1b4d..13cc2ea4e61af 100644 --- a/display_list/testing/dl_test_snippets.cc +++ b/display_list/testing/dl_test_snippets.cc @@ -515,7 +515,7 @@ std::vector CreateAllRenderingOps() { }}, {1, 16, 1, 24, [](DlOpReceiver& r) { - r.drawColor(SK_ColorBLUE, DlBlendMode::kDstIn); + r.drawColor(SK_ColorBLUE, DlBlendMode::kDstOut); }}, {1, 16, 1, 24, [](DlOpReceiver& r) { diff --git a/display_list/testing/dl_test_snippets.h b/display_list/testing/dl_test_snippets.h index d60f6f45f011e..94be886d3e51d 100644 --- a/display_list/testing/dl_test_snippets.h +++ b/display_list/testing/dl_test_snippets.h @@ -150,9 +150,9 @@ static const DlBlurImageFilter kTestBlurImageFilter4(5.0, static const DlDilateImageFilter kTestDilateImageFilter1(5.0, 5.0); static const DlDilateImageFilter kTestDilateImageFilter2(6.0, 5.0); static const DlDilateImageFilter kTestDilateImageFilter3(5.0, 6.0); -static const DlErodeImageFilter kTestErodeImageFilter1(5.0, 5.0); -static const DlErodeImageFilter kTestErodeImageFilter2(6.0, 5.0); -static const DlErodeImageFilter kTestErodeImageFilter3(5.0, 6.0); +static const DlErodeImageFilter kTestErodeImageFilter1(4.0, 4.0); +static const DlErodeImageFilter kTestErodeImageFilter2(4.0, 3.0); +static const DlErodeImageFilter kTestErodeImageFilter3(3.0, 4.0); static const DlMatrixImageFilter kTestMatrixImageFilter1( SkMatrix::RotateDeg(45), kNearestSampling); diff --git a/display_list/utils/dl_matrix_clip_tracker.h b/display_list/utils/dl_matrix_clip_tracker.h index 7a15b690dc878..e62d90c1fa221 100644 --- a/display_list/utils/dl_matrix_clip_tracker.h +++ b/display_list/utils/dl_matrix_clip_tracker.h @@ -38,6 +38,7 @@ class DisplayListMatrixClipTracker { bool content_culled(const SkRect& content_bounds) const { return current_->content_culled(content_bounds); } + bool is_cull_rect_empty() const { return current_->is_cull_rect_empty(); } void save(); void restore(); @@ -81,9 +82,10 @@ class DisplayListMatrixClipTracker { virtual SkMatrix matrix_3x3() const = 0; virtual SkM44 matrix_4x4() const = 0; - virtual SkRect device_cull_rect() const { return cull_rect_; } + SkRect device_cull_rect() const { return cull_rect_; } virtual SkRect local_cull_rect() const = 0; virtual bool content_culled(const SkRect& content_bounds) const; + bool is_cull_rect_empty() const { return cull_rect_.isEmpty(); } virtual void translate(SkScalar tx, SkScalar ty) = 0; virtual void scale(SkScalar sx, SkScalar sy) = 0; diff --git a/flow/diff_context_unittests.cc b/flow/diff_context_unittests.cc index e981258ef1a03..d8b78a94070c0 100644 --- a/flow/diff_context_unittests.cc +++ b/flow/diff_context_unittests.cc @@ -10,7 +10,7 @@ namespace testing { TEST_F(DiffContextTest, ClipAlignment) { MockLayerTree t1; t1.root()->Add(CreateDisplayListLayer( - CreateDisplayList(SkRect::MakeLTRB(30, 30, 50, 50), 1))); + CreateDisplayList(SkRect::MakeLTRB(30, 30, 50, 50)))); auto damage = DiffLayerTree(t1, MockLayerTree(), SkIRect::MakeEmpty(), 0, 0); EXPECT_EQ(damage.frame_damage, SkIRect::MakeLTRB(30, 30, 50, 50)); EXPECT_EQ(damage.buffer_damage, SkIRect::MakeLTRB(30, 30, 50, 50)); diff --git a/flow/layers/container_layer_unittests.cc b/flow/layers/container_layer_unittests.cc index a4c671d05f39d..ff8cf9ceb5b4a 100644 --- a/flow/layers/container_layer_unittests.cc +++ b/flow/layers/container_layer_unittests.cc @@ -561,9 +561,9 @@ using ContainerLayerDiffTest = DiffContextTest; // Insert PictureLayer amongst container layers TEST_F(ContainerLayerDiffTest, PictureLayerInsertion) { - auto pic1 = CreateDisplayList(SkRect::MakeLTRB(0, 0, 50, 50), 1); - auto pic2 = CreateDisplayList(SkRect::MakeLTRB(100, 0, 150, 50), 1); - auto pic3 = CreateDisplayList(SkRect::MakeLTRB(200, 0, 250, 50), 1); + auto pic1 = CreateDisplayList(SkRect::MakeLTRB(0, 0, 50, 50)); + auto pic2 = CreateDisplayList(SkRect::MakeLTRB(100, 0, 150, 50)); + auto pic3 = CreateDisplayList(SkRect::MakeLTRB(200, 0, 250, 50)); MockLayerTree t1; @@ -613,9 +613,9 @@ TEST_F(ContainerLayerDiffTest, PictureLayerInsertion) { // Insert picture layer amongst other picture layers TEST_F(ContainerLayerDiffTest, PictureInsertion) { - auto pic1 = CreateDisplayList(SkRect::MakeLTRB(0, 0, 50, 50), 1); - auto pic2 = CreateDisplayList(SkRect::MakeLTRB(100, 0, 150, 50), 1); - auto pic3 = CreateDisplayList(SkRect::MakeLTRB(200, 0, 250, 50), 1); + auto pic1 = CreateDisplayList(SkRect::MakeLTRB(0, 0, 50, 50)); + auto pic2 = CreateDisplayList(SkRect::MakeLTRB(100, 0, 150, 50)); + auto pic3 = CreateDisplayList(SkRect::MakeLTRB(200, 0, 250, 50)); MockLayerTree t1; t1.root()->Add(CreateDisplayListLayer(pic1)); diff --git a/flow/layers/display_list_layer_unittests.cc b/flow/layers/display_list_layer_unittests.cc index 137b76d2472fb..01468aec96c1d 100644 --- a/flow/layers/display_list_layer_unittests.cc +++ b/flow/layers/display_list_layer_unittests.cc @@ -294,7 +294,7 @@ TEST_F(DisplayListLayerTest, CachedIncompatibleDisplayListOpacityInheritance) { using DisplayListLayerDiffTest = DiffContextTest; TEST_F(DisplayListLayerDiffTest, SimpleDisplayList) { - auto display_list = CreateDisplayList(SkRect::MakeLTRB(10, 10, 60, 60), 1); + auto display_list = CreateDisplayList(SkRect::MakeLTRB(10, 10, 60, 60)); MockLayerTree tree1; tree1.root()->Add(CreateDisplayListLayer(display_list)); @@ -314,7 +314,7 @@ TEST_F(DisplayListLayerDiffTest, SimpleDisplayList) { } TEST_F(DisplayListLayerDiffTest, FractionalTranslation) { - auto display_list = CreateDisplayList(SkRect::MakeLTRB(10, 10, 60, 60), 1); + auto display_list = CreateDisplayList(SkRect::MakeLTRB(10, 10, 60, 60)); MockLayerTree tree1; tree1.root()->Add( @@ -327,7 +327,7 @@ TEST_F(DisplayListLayerDiffTest, FractionalTranslation) { } TEST_F(DisplayListLayerDiffTest, FractionalTranslationWithRasterCache) { - auto display_list = CreateDisplayList(SkRect::MakeLTRB(10, 10, 60, 60), 1); + auto display_list = CreateDisplayList(SkRect::MakeLTRB(10, 10, 60, 60)); MockLayerTree tree1; tree1.root()->Add( @@ -341,21 +341,25 @@ TEST_F(DisplayListLayerDiffTest, FractionalTranslationWithRasterCache) { TEST_F(DisplayListLayerDiffTest, DisplayListCompare) { MockLayerTree tree1; - auto display_list1 = CreateDisplayList(SkRect::MakeLTRB(10, 10, 60, 60), 1); + auto display_list1 = + CreateDisplayList(SkRect::MakeLTRB(10, 10, 60, 60), DlColor::kGreen()); tree1.root()->Add(CreateDisplayListLayer(display_list1)); auto damage = DiffLayerTree(tree1, MockLayerTree()); EXPECT_EQ(damage.frame_damage, SkIRect::MakeLTRB(10, 10, 60, 60)); MockLayerTree tree2; - auto display_list2 = CreateDisplayList(SkRect::MakeLTRB(10, 10, 60, 60), 1); + // same DL, same offset + auto display_list2 = + CreateDisplayList(SkRect::MakeLTRB(10, 10, 60, 60), DlColor::kGreen()); tree2.root()->Add(CreateDisplayListLayer(display_list2)); damage = DiffLayerTree(tree2, tree1); EXPECT_EQ(damage.frame_damage, SkIRect::MakeEmpty()); MockLayerTree tree3; - auto display_list3 = CreateDisplayList(SkRect::MakeLTRB(10, 10, 60, 60), 1); + auto display_list3 = + CreateDisplayList(SkRect::MakeLTRB(10, 10, 60, 60), DlColor::kGreen()); // add offset tree3.root()->Add( CreateDisplayListLayer(display_list3, SkPoint::Make(10, 10))); @@ -365,7 +369,8 @@ TEST_F(DisplayListLayerDiffTest, DisplayListCompare) { MockLayerTree tree4; // different color - auto display_list4 = CreateDisplayList(SkRect::MakeLTRB(10, 10, 60, 60), 2); + auto display_list4 = + CreateDisplayList(SkRect::MakeLTRB(10, 10, 60, 60), DlColor::kRed()); tree4.root()->Add( CreateDisplayListLayer(display_list4, SkPoint::Make(10, 10))); diff --git a/flow/layers/opacity_layer_unittests.cc b/flow/layers/opacity_layer_unittests.cc index a2a119264bf8b..1f1562806d163 100644 --- a/flow/layers/opacity_layer_unittests.cc +++ b/flow/layers/opacity_layer_unittests.cc @@ -656,7 +656,7 @@ using OpacityLayerDiffTest = DiffContextTest; TEST_F(OpacityLayerDiffTest, FractionalTranslation) { auto picture = CreateDisplayListLayer( - CreateDisplayList(SkRect::MakeLTRB(10, 10, 60, 60), 1)); + CreateDisplayList(SkRect::MakeLTRB(10, 10, 60, 60))); auto layer = CreateOpacityLater({picture}, 128, SkPoint::Make(0.5, 0.5)); MockLayerTree tree1; @@ -669,7 +669,7 @@ TEST_F(OpacityLayerDiffTest, FractionalTranslation) { TEST_F(OpacityLayerDiffTest, FractionalTranslationWithRasterCache) { auto picture = CreateDisplayListLayer( - CreateDisplayList(SkRect::MakeLTRB(10, 10, 60, 60), 1)); + CreateDisplayList(SkRect::MakeLTRB(10, 10, 60, 60))); auto layer = CreateOpacityLater({picture}, 128, SkPoint::Make(0.5, 0.5)); MockLayerTree tree1; diff --git a/flow/testing/diff_context_test.cc b/flow/testing/diff_context_test.cc index 20153a0140d5c..c4c68bb7ab271 100644 --- a/flow/testing/diff_context_test.cc +++ b/flow/testing/diff_context_test.cc @@ -30,7 +30,7 @@ Damage DiffContextTest::DiffLayerTree(MockLayerTree& layer_tree, } sk_sp DiffContextTest::CreateDisplayList(const SkRect& bounds, - SkColor color) { + DlColor color) { DisplayListBuilder builder; builder.DrawRect(bounds, DlPaint().setColor(color)); return builder.Build(); diff --git a/flow/testing/diff_context_test.h b/flow/testing/diff_context_test.h index 69beb41d83470..f22561552fdd0 100644 --- a/flow/testing/diff_context_test.h +++ b/flow/testing/diff_context_test.h @@ -47,7 +47,8 @@ class DiffContextTest : public LayerTest { // Create display list consisting of filled rect with given color; Being able // to specify different color is useful to test deep comparison of pictures - sk_sp CreateDisplayList(const SkRect& bounds, uint32_t color); + sk_sp CreateDisplayList(const SkRect& bounds, + DlColor color = DlColor::kBlack()); std::shared_ptr CreateDisplayListLayer( const sk_sp& display_list, diff --git a/shell/common/dl_op_spy.cc b/shell/common/dl_op_spy.cc index 15910fc2e85de..0ef9553a52336 100644 --- a/shell/common/dl_op_spy.cc +++ b/shell/common/dl_op_spy.cc @@ -34,7 +34,7 @@ void DlOpSpy::saveLayer(const SkRect* bounds, const DlImageFilter* backdrop) {} void DlOpSpy::restore() {} void DlOpSpy::drawColor(DlColor color, DlBlendMode mode) { - did_draw_ |= !color.isTransparent(); + did_draw_ |= !(color.isTransparent() && mode == DlBlendMode::kSrcOver); } void DlOpSpy::drawPaint() { did_draw_ |= will_draw_; diff --git a/shell/common/dl_op_spy_unittests.cc b/shell/common/dl_op_spy_unittests.cc index dba02134238c8..6a82f1b16c251 100644 --- a/shell/common/dl_op_spy_unittests.cc +++ b/shell/common/dl_op_spy_unittests.cc @@ -7,10 +7,29 @@ #include "flutter/shell/common/dl_op_spy.h" #include "flutter/testing/testing.h" #include "third_party/skia/include/core/SkBitmap.h" +#include "third_party/skia/include/core/SkRSXform.h" namespace flutter { namespace testing { +// The following 2 macros demonstrate that the DlOpSpy class is equivalent +// to simpler tests on the DisplayList object itself now that the +// DisplayListBuilder implements operation culling. +// See https://github.com/flutter/flutter/issues/125403 +#define ASSERT_DID_DRAW(spy, dl) \ + do { \ + ASSERT_TRUE(spy.did_draw()); \ + ASSERT_GT(dl->op_count(), 0u); \ + ASSERT_FALSE(dl->bounds().isEmpty()); \ + } while (0) + +#define ASSERT_NO_DRAW(spy, dl) \ + do { \ + ASSERT_FALSE(spy.did_draw()); \ + ASSERT_EQ(dl->op_count(), 0u); \ + ASSERT_TRUE(dl->bounds().isEmpty()); \ + } while (0) + TEST(DlOpSpy, DidDrawIsFalseByDefault) { DlOpSpy dl_op_spy; ASSERT_FALSE(dl_op_spy.did_draw()); @@ -24,7 +43,7 @@ TEST(DlOpSpy, SetColor) { sk_sp dl = builder.Build(); DlOpSpy dl_op_spy; dl->Dispatch(dl_op_spy); - ASSERT_TRUE(dl_op_spy.did_draw()); + ASSERT_DID_DRAW(dl_op_spy, dl); } { // Set transparent color. DisplayListBuilder builder; @@ -33,7 +52,7 @@ TEST(DlOpSpy, SetColor) { sk_sp dl = builder.Build(); DlOpSpy dl_op_spy; dl->Dispatch(dl_op_spy); - ASSERT_FALSE(dl_op_spy.did_draw()); + ASSERT_NO_DRAW(dl_op_spy, dl); } { // Set black color. DisplayListBuilder builder; @@ -42,7 +61,7 @@ TEST(DlOpSpy, SetColor) { sk_sp dl = builder.Build(); DlOpSpy dl_op_spy; dl->Dispatch(dl_op_spy); - ASSERT_TRUE(dl_op_spy.did_draw()); + ASSERT_DID_DRAW(dl_op_spy, dl); } } @@ -55,7 +74,7 @@ TEST(DlOpSpy, SetColorSource) { sk_sp dl = builder.Build(); DlOpSpy dl_op_spy; dl->Dispatch(dl_op_spy); - ASSERT_TRUE(dl_op_spy.did_draw()); + ASSERT_DID_DRAW(dl_op_spy, dl); } { // Set transparent color. DisplayListBuilder builder; @@ -67,7 +86,7 @@ TEST(DlOpSpy, SetColorSource) { sk_sp dl = builder.Build(); DlOpSpy dl_op_spy; dl->Dispatch(dl_op_spy); - ASSERT_FALSE(dl_op_spy.did_draw()); + ASSERT_NO_DRAW(dl_op_spy, dl); } { // Set black color. DisplayListBuilder builder; @@ -79,7 +98,7 @@ TEST(DlOpSpy, SetColorSource) { sk_sp dl = builder.Build(); DlOpSpy dl_op_spy; dl->Dispatch(dl_op_spy); - ASSERT_TRUE(dl_op_spy.did_draw()); + ASSERT_DID_DRAW(dl_op_spy, dl); } } @@ -91,7 +110,7 @@ TEST(DlOpSpy, DrawColor) { sk_sp dl = builder.Build(); DlOpSpy dl_op_spy; dl->Dispatch(dl_op_spy); - ASSERT_TRUE(dl_op_spy.did_draw()); + ASSERT_DID_DRAW(dl_op_spy, dl); } { // Transparent color source. DisplayListBuilder builder; @@ -100,7 +119,16 @@ TEST(DlOpSpy, DrawColor) { sk_sp dl = builder.Build(); DlOpSpy dl_op_spy; dl->Dispatch(dl_op_spy); - ASSERT_FALSE(dl_op_spy.did_draw()); + ASSERT_DID_DRAW(dl_op_spy, dl); + } + { // Transparent color source-over. + DisplayListBuilder builder; + auto color = DlColor::kTransparent(); + builder.DrawColor(color, DlBlendMode::kSrcOver); + sk_sp dl = builder.Build(); + DlOpSpy dl_op_spy; + dl->Dispatch(dl_op_spy); + ASSERT_NO_DRAW(dl_op_spy, dl); } } @@ -112,7 +140,7 @@ TEST(DlOpSpy, DrawPaint) { sk_sp dl = builder.Build(); DlOpSpy dl_op_spy; dl->Dispatch(dl_op_spy); - ASSERT_FALSE(dl_op_spy.did_draw()); + ASSERT_NO_DRAW(dl_op_spy, dl); } { // black color in paint. DisplayListBuilder builder; @@ -121,7 +149,7 @@ TEST(DlOpSpy, DrawPaint) { sk_sp dl = builder.Build(); DlOpSpy dl_op_spy; dl->Dispatch(dl_op_spy); - ASSERT_TRUE(dl_op_spy.did_draw()); + ASSERT_DID_DRAW(dl_op_spy, dl); } } @@ -133,7 +161,7 @@ TEST(DlOpSpy, DrawLine) { sk_sp dl = builder.Build(); DlOpSpy dl_op_spy; dl->Dispatch(dl_op_spy); - ASSERT_TRUE(dl_op_spy.did_draw()); + ASSERT_DID_DRAW(dl_op_spy, dl); } { // transparent DisplayListBuilder builder; @@ -142,7 +170,7 @@ TEST(DlOpSpy, DrawLine) { sk_sp dl = builder.Build(); DlOpSpy dl_op_spy; dl->Dispatch(dl_op_spy); - ASSERT_FALSE(dl_op_spy.did_draw()); + ASSERT_NO_DRAW(dl_op_spy, dl); } } @@ -154,7 +182,7 @@ TEST(DlOpSpy, DrawRect) { sk_sp dl = builder.Build(); DlOpSpy dl_op_spy; dl->Dispatch(dl_op_spy); - ASSERT_TRUE(dl_op_spy.did_draw()); + ASSERT_DID_DRAW(dl_op_spy, dl); } { // transparent DisplayListBuilder builder; @@ -163,11 +191,11 @@ TEST(DlOpSpy, DrawRect) { sk_sp dl = builder.Build(); DlOpSpy dl_op_spy; dl->Dispatch(dl_op_spy); - ASSERT_FALSE(dl_op_spy.did_draw()); + ASSERT_NO_DRAW(dl_op_spy, dl); } } -TEST(DlOpSpy, drawOval) { +TEST(DlOpSpy, DrawOval) { { // black DisplayListBuilder builder; DlPaint paint(DlColor::kBlack()); @@ -175,7 +203,7 @@ TEST(DlOpSpy, drawOval) { sk_sp dl = builder.Build(); DlOpSpy dl_op_spy; dl->Dispatch(dl_op_spy); - ASSERT_TRUE(dl_op_spy.did_draw()); + ASSERT_DID_DRAW(dl_op_spy, dl); } { // transparent DisplayListBuilder builder; @@ -184,11 +212,11 @@ TEST(DlOpSpy, drawOval) { sk_sp dl = builder.Build(); DlOpSpy dl_op_spy; dl->Dispatch(dl_op_spy); - ASSERT_FALSE(dl_op_spy.did_draw()); + ASSERT_NO_DRAW(dl_op_spy, dl); } } -TEST(DlOpSpy, drawCircle) { +TEST(DlOpSpy, DrawCircle) { { // black DisplayListBuilder builder; DlPaint paint(DlColor::kBlack()); @@ -196,7 +224,7 @@ TEST(DlOpSpy, drawCircle) { sk_sp dl = builder.Build(); DlOpSpy dl_op_spy; dl->Dispatch(dl_op_spy); - ASSERT_TRUE(dl_op_spy.did_draw()); + ASSERT_DID_DRAW(dl_op_spy, dl); } { // transparent DisplayListBuilder builder; @@ -205,11 +233,11 @@ TEST(DlOpSpy, drawCircle) { sk_sp dl = builder.Build(); DlOpSpy dl_op_spy; dl->Dispatch(dl_op_spy); - ASSERT_FALSE(dl_op_spy.did_draw()); + ASSERT_NO_DRAW(dl_op_spy, dl); } } -TEST(DlOpSpy, drawRRect) { +TEST(DlOpSpy, DrawRRect) { { // black DisplayListBuilder builder; DlPaint paint(DlColor::kBlack()); @@ -217,7 +245,7 @@ TEST(DlOpSpy, drawRRect) { sk_sp dl = builder.Build(); DlOpSpy dl_op_spy; dl->Dispatch(dl_op_spy); - ASSERT_TRUE(dl_op_spy.did_draw()); + ASSERT_DID_DRAW(dl_op_spy, dl); } { // transparent DisplayListBuilder builder; @@ -226,34 +254,49 @@ TEST(DlOpSpy, drawRRect) { sk_sp dl = builder.Build(); DlOpSpy dl_op_spy; dl->Dispatch(dl_op_spy); - ASSERT_FALSE(dl_op_spy.did_draw()); + ASSERT_NO_DRAW(dl_op_spy, dl); } } -TEST(DlOpSpy, drawPath) { - { // black +TEST(DlOpSpy, DrawPath) { + { // black line DisplayListBuilder builder; DlPaint paint(DlColor::kBlack()); + paint.setDrawStyle(DlDrawStyle::kStroke); builder.DrawPath(SkPath::Line(SkPoint::Make(0, 1), SkPoint::Make(1, 1)), paint); sk_sp dl = builder.Build(); DlOpSpy dl_op_spy; dl->Dispatch(dl_op_spy); - ASSERT_TRUE(dl_op_spy.did_draw()); + ASSERT_DID_DRAW(dl_op_spy, dl); } - { // transparent + { // triangle + DisplayListBuilder builder; + DlPaint paint(DlColor::kBlack()); + SkPath path; + path.moveTo({0, 0}); + path.lineTo({1, 0}); + path.lineTo({0, 1}); + builder.DrawPath(path, paint); + sk_sp dl = builder.Build(); + DlOpSpy dl_op_spy; + dl->Dispatch(dl_op_spy); + ASSERT_DID_DRAW(dl_op_spy, dl); + } + { // transparent line DisplayListBuilder builder; DlPaint paint(DlColor::kTransparent()); + paint.setDrawStyle(DlDrawStyle::kStroke); builder.DrawPath(SkPath::Line(SkPoint::Make(0, 1), SkPoint::Make(1, 1)), paint); sk_sp dl = builder.Build(); DlOpSpy dl_op_spy; dl->Dispatch(dl_op_spy); - ASSERT_FALSE(dl_op_spy.did_draw()); + ASSERT_NO_DRAW(dl_op_spy, dl); } } -TEST(DlOpSpy, drawArc) { +TEST(DlOpSpy, DrawArc) { { // black DisplayListBuilder builder; DlPaint paint(DlColor::kBlack()); @@ -261,7 +304,7 @@ TEST(DlOpSpy, drawArc) { sk_sp dl = builder.Build(); DlOpSpy dl_op_spy; dl->Dispatch(dl_op_spy); - ASSERT_TRUE(dl_op_spy.did_draw()); + ASSERT_DID_DRAW(dl_op_spy, dl); } { // transparent DisplayListBuilder builder; @@ -270,11 +313,11 @@ TEST(DlOpSpy, drawArc) { sk_sp dl = builder.Build(); DlOpSpy dl_op_spy; dl->Dispatch(dl_op_spy); - ASSERT_FALSE(dl_op_spy.did_draw()); + ASSERT_NO_DRAW(dl_op_spy, dl); } } -TEST(DlOpSpy, drawPoints) { +TEST(DlOpSpy, DrawPoints) { { // black DisplayListBuilder builder; DlPaint paint(DlColor::kBlack()); @@ -283,7 +326,7 @@ TEST(DlOpSpy, drawPoints) { sk_sp dl = builder.Build(); DlOpSpy dl_op_spy; dl->Dispatch(dl_op_spy); - ASSERT_TRUE(dl_op_spy.did_draw()); + ASSERT_DID_DRAW(dl_op_spy, dl); } { // transparent DisplayListBuilder builder; @@ -293,38 +336,46 @@ TEST(DlOpSpy, drawPoints) { sk_sp dl = builder.Build(); DlOpSpy dl_op_spy; dl->Dispatch(dl_op_spy); - ASSERT_FALSE(dl_op_spy.did_draw()); + ASSERT_NO_DRAW(dl_op_spy, dl); } } -TEST(DlOpSpy, drawVertices) { +TEST(DlOpSpy, DrawVertices) { { // black DisplayListBuilder builder; DlPaint paint(DlColor::kBlack()); - const SkPoint vertices[] = {SkPoint::Make(5, 5)}; + const SkPoint vertices[] = { + SkPoint::Make(5, 5), + SkPoint::Make(5, 15), + SkPoint::Make(15, 5), + }; const SkPoint texture_coordinates[] = {SkPoint::Make(5, 5)}; const DlColor colors[] = {DlColor::kBlack()}; - auto dl_vertices = DlVertices::Make(DlVertexMode::kTriangles, 1, vertices, + auto dl_vertices = DlVertices::Make(DlVertexMode::kTriangles, 3, vertices, texture_coordinates, colors, 0); builder.DrawVertices(dl_vertices.get(), DlBlendMode::kSrc, paint); sk_sp dl = builder.Build(); DlOpSpy dl_op_spy; dl->Dispatch(dl_op_spy); - ASSERT_TRUE(dl_op_spy.did_draw()); + ASSERT_DID_DRAW(dl_op_spy, dl); } { // transparent DisplayListBuilder builder; DlPaint paint(DlColor::kTransparent()); - const SkPoint vertices[] = {SkPoint::Make(5, 5)}; + const SkPoint vertices[] = { + SkPoint::Make(5, 5), + SkPoint::Make(5, 15), + SkPoint::Make(15, 5), + }; const SkPoint texture_coordinates[] = {SkPoint::Make(5, 5)}; const DlColor colors[] = {DlColor::kBlack()}; - auto dl_vertices = DlVertices::Make(DlVertexMode::kTriangles, 1, vertices, + auto dl_vertices = DlVertices::Make(DlVertexMode::kTriangles, 3, vertices, texture_coordinates, colors, 0); builder.DrawVertices(dl_vertices.get(), DlBlendMode::kSrc, paint); sk_sp dl = builder.Build(); DlOpSpy dl_op_spy; dl->Dispatch(dl_op_spy); - ASSERT_FALSE(dl_op_spy.did_draw()); + ASSERT_NO_DRAW(dl_op_spy, dl); } } @@ -343,7 +394,7 @@ TEST(DlOpSpy, Images) { sk_sp dl = builder.Build(); DlOpSpy dl_op_spy; dl->Dispatch(dl_op_spy); - ASSERT_TRUE(dl_op_spy.did_draw()); + ASSERT_DID_DRAW(dl_op_spy, dl); } { // DrawImageRect DisplayListBuilder builder; @@ -359,7 +410,7 @@ TEST(DlOpSpy, Images) { sk_sp dl = builder.Build(); DlOpSpy dl_op_spy; dl->Dispatch(dl_op_spy); - ASSERT_TRUE(dl_op_spy.did_draw()); + ASSERT_DID_DRAW(dl_op_spy, dl); } { // DrawImageNine DisplayListBuilder builder; @@ -375,7 +426,7 @@ TEST(DlOpSpy, Images) { sk_sp dl = builder.Build(); DlOpSpy dl_op_spy; dl->Dispatch(dl_op_spy); - ASSERT_TRUE(dl_op_spy.did_draw()); + ASSERT_DID_DRAW(dl_op_spy, dl); } { // DrawAtlas DisplayListBuilder builder; @@ -386,20 +437,19 @@ TEST(DlOpSpy, Images) { SkBitmap bitmap; bitmap.allocPixels(info, 0); auto sk_image = SkImages::RasterFromBitmap(bitmap); - const SkRSXform xform[] = {}; - const SkRect tex[] = {}; - const DlColor colors[] = {}; + const SkRSXform xform[] = {SkRSXform::Make(1, 0, 0, 0)}; + const SkRect tex[] = {SkRect::MakeXYWH(10, 10, 10, 10)}; SkRect cull_rect = SkRect::MakeWH(5, 5); - builder.DrawAtlas(DlImage::Make(sk_image), xform, tex, colors, 0, + builder.DrawAtlas(DlImage::Make(sk_image), xform, tex, nullptr, 1, DlBlendMode::kSrc, DlImageSampling::kLinear, &cull_rect); sk_sp dl = builder.Build(); DlOpSpy dl_op_spy; dl->Dispatch(dl_op_spy); - ASSERT_TRUE(dl_op_spy.did_draw()); + ASSERT_DID_DRAW(dl_op_spy, dl); } } -TEST(DlOpSpy, drawDisplayList) { +TEST(DlOpSpy, DrawDisplayList) { { // Recursive Transparent DisplayList DisplayListBuilder builder; DlPaint paint(DlColor::kTransparent()); @@ -414,7 +464,7 @@ TEST(DlOpSpy, drawDisplayList) { DlOpSpy dl_op_spy; dl2->Dispatch(dl_op_spy); - ASSERT_FALSE(dl_op_spy.did_draw()); + ASSERT_NO_DRAW(dl_op_spy, dl2); } { // Sub non-transparent DisplayList, DisplayListBuilder builder; @@ -430,7 +480,7 @@ TEST(DlOpSpy, drawDisplayList) { DlOpSpy dl_op_spy; dl2->Dispatch(dl_op_spy); - ASSERT_TRUE(dl_op_spy.did_draw()); + ASSERT_DID_DRAW(dl_op_spy, dl2); } { // Sub non-transparent DisplayList, 0 opacity @@ -447,7 +497,7 @@ TEST(DlOpSpy, drawDisplayList) { DlOpSpy dl_op_spy; dl2->Dispatch(dl_op_spy); - ASSERT_FALSE(dl_op_spy.did_draw()); + ASSERT_NO_DRAW(dl_op_spy, dl2); } { // Parent non-transparent DisplayList @@ -464,11 +514,11 @@ TEST(DlOpSpy, drawDisplayList) { DlOpSpy dl_op_spy; dl2->Dispatch(dl_op_spy); - ASSERT_TRUE(dl_op_spy.did_draw()); + ASSERT_DID_DRAW(dl_op_spy, dl2); } } -TEST(DlOpSpy, drawTextBlob) { +TEST(DlOpSpy, DrawTextBlob) { { // Non-transparent color. DisplayListBuilder builder; DlPaint paint(DlColor::kBlack()); @@ -479,7 +529,7 @@ TEST(DlOpSpy, drawTextBlob) { sk_sp dl = builder.Build(); DlOpSpy dl_op_spy; dl->Dispatch(dl_op_spy); - ASSERT_TRUE(dl_op_spy.did_draw()); + ASSERT_DID_DRAW(dl_op_spy, dl); } { // transparent color. DisplayListBuilder builder; @@ -491,11 +541,11 @@ TEST(DlOpSpy, drawTextBlob) { sk_sp dl = builder.Build(); DlOpSpy dl_op_spy; dl->Dispatch(dl_op_spy); - ASSERT_FALSE(dl_op_spy.did_draw()); + ASSERT_NO_DRAW(dl_op_spy, dl); } } -TEST(DlOpSpy, drawShadow) { +TEST(DlOpSpy, DrawShadow) { { // valid shadow DisplayListBuilder builder; DlPaint paint; @@ -505,7 +555,7 @@ TEST(DlOpSpy, drawShadow) { sk_sp dl = builder.Build(); DlOpSpy dl_op_spy; dl->Dispatch(dl_op_spy); - ASSERT_TRUE(dl_op_spy.did_draw()); + ASSERT_DID_DRAW(dl_op_spy, dl); } { // transparent color DisplayListBuilder builder; @@ -516,7 +566,7 @@ TEST(DlOpSpy, drawShadow) { sk_sp dl = builder.Build(); DlOpSpy dl_op_spy; dl->Dispatch(dl_op_spy); - ASSERT_FALSE(dl_op_spy.did_draw()); + ASSERT_NO_DRAW(dl_op_spy, dl); } } From cef30abaf6b624e62bb250178fc41f3efd21ea31 Mon Sep 17 00:00:00 2001 From: Jim Graham Date: Wed, 26 Apr 2023 14:07:49 -0700 Subject: [PATCH 2/8] add detection of DLs that draw non-transparent pixels --- display_list/display_list.cc | 5 +- display_list/display_list.h | 6 + display_list/dl_builder.cc | 283 ++++++++++++++++------------ display_list/dl_builder.h | 31 ++- shell/common/dl_op_spy.cc | 2 +- shell/common/dl_op_spy_unittests.cc | 40 ++-- 6 files changed, 230 insertions(+), 137 deletions(-) diff --git a/display_list/display_list.cc b/display_list/display_list.cc index 378f86804f260..53707b4c272d9 100644 --- a/display_list/display_list.cc +++ b/display_list/display_list.cc @@ -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), + affects_transparent_surface_(false) {} DisplayList::DisplayList(DisplayListStorage&& storage, size_t byte_count, @@ -32,6 +33,7 @@ DisplayList::DisplayList(DisplayListStorage&& storage, const SkRect& bounds, bool can_apply_group_opacity, bool is_ui_thread_safe, + bool affects_transparent_surface, sk_sp rtree) : storage_(std::move(storage)), byte_count_(byte_count), @@ -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), + affects_transparent_surface_(affects_transparent_surface), rtree_(std::move(rtree)) {} DisplayList::~DisplayList() { diff --git a/display_list/display_list.h b/display_list/display_list.h index 4a6f339fdea0d..5e49d2bead06f 100644 --- a/display_list/display_list.h +++ b/display_list/display_list.h @@ -263,6 +263,9 @@ class DisplayList : public SkRefCnt { } bool can_apply_group_opacity() const { return can_apply_group_opacity_; } + bool affects_transparent_surface() const { + return affects_transparent_surface_; + } bool isUIThreadSafe() const { return is_ui_thread_safe_; } private: @@ -274,6 +277,7 @@ class DisplayList : public SkRefCnt { const SkRect& bounds, bool can_apply_group_opacity, bool is_ui_thread_safe, + bool affects_transparent_surface, sk_sp rtree); static uint32_t next_unique_id(); @@ -292,6 +296,8 @@ class DisplayList : public SkRefCnt { const bool can_apply_group_opacity_; const bool is_ui_thread_safe_; + const bool affects_transparent_surface_; + const sk_sp rtree_; void Dispatch(DlOpReceiver& ctx, diff --git a/display_list/dl_builder.cc b/display_list/dl_builder.cc index d45627b00e2f8..b6477099933ee 100644 --- a/display_list/dl_builder.cc +++ b/display_list/dl_builder.cc @@ -74,11 +74,12 @@ sk_sp DisplayListBuilder::Build() { used_ = allocated_ = render_op_count_ = op_index_ = 0; nested_bytes_ = nested_op_count_ = 0; storage_.realloc(bytes); - bool compatible = layer_stack_.back().is_group_opacity_compatible(); + bool compatible = current_layer_->is_group_opacity_compatible(); bool is_safe = is_ui_thread_safe_; - return sk_sp( - new DisplayList(std::move(storage_), bytes, count, nested_bytes, - nested_count, bounds(), compatible, is_safe, rtree())); + bool affects_transparency = current_layer_->affects_transparent_layer(); + return sk_sp(new DisplayList( + std::move(storage_), bytes, count, nested_bytes, nested_count, bounds(), + compatible, is_safe, affects_transparency, rtree())); } DisplayListBuilder::DisplayListBuilder(const SkRect& cull_rect, @@ -475,9 +476,11 @@ void DisplayListBuilder::saveLayer(const SkRect* bounds, const SaveLayerOptions in_options, const DlImageFilter* backdrop) { SaveLayerOptions options = in_options.without_optimizations(); - if (current_layer_->is_nop_ || - (options.renders_with_attributes() && - !paint_affects_dest(current_, kSaveLayerWithPaintFlags))) { + DisplayListAttributeFlags flags = options.renders_with_attributes() + ? kSaveLayerWithPaintFlags + : kSaveLayerFlags; + OpResult result = PaintResult(current_, flags); + if (result == OpResult::kNoEffect) { save(); current_layer_->is_nop_ = true; return; @@ -493,8 +496,12 @@ void DisplayListBuilder::saveLayer(const SkRect* bounds, // with its full bounds and the right op_index so that it doesn't // get culled during rendering. if (!paint_nops_on_transparency()) { - // We will fill the clip of the outer layer when we restore - AccumulateUnbounded(); + // We will fill the clip of the outer layer when we restore. + // Accumulate should always return true here because if the + // clip was empty then that would have been caught up above + // when we tested the PaintResult. + bool unclipped = AccumulateUnbounded(); + FML_DCHECK(unclipped); } CheckLayerOpacityCompatibility(true); layer_stack_.emplace_back(save_layer_offset, true, @@ -503,11 +510,18 @@ void DisplayListBuilder::saveLayer(const SkRect* bounds, CheckLayerOpacityCompatibility(false); layer_stack_.emplace_back(save_layer_offset, true, nullptr); } + current_layer_ = &layer_stack_.back(); + tracker_.save(); accumulator()->save(); - current_layer_ = &layer_stack_.back(); if (backdrop) { + // A backdrop will affect up to the entire surface, bounded by the clip + // Accumulate should always return true here because if the + // clip was empty then that would have been caught up above + // when we tested the PaintResult. + bool unclipped = AccumulateUnbounded(); + FML_DCHECK(unclipped); bounds // ? Push(0, 1, options, *bounds, backdrop) : Push(0, 1, options, backdrop); @@ -527,6 +541,7 @@ void DisplayListBuilder::saveLayer(const SkRect* bounds, UpdateLayerOpacityCompatibility(false); } } + UpdateLayerResult(result); // Even though Skia claims that the bounds are only a hint, they actually // use them as the temporary layer bounds during rendering the layer, so @@ -534,10 +549,6 @@ void DisplayListBuilder::saveLayer(const SkRect* bounds, if (bounds) { tracker_.clipRect(*bounds, ClipOp::kIntersect, false); } - if (backdrop) { - // A backdrop will affect up to the entire surface, bounded by the clip - AccumulateUnbounded(); - } } void DisplayListBuilder::SaveLayer(const SkRect* bounds, const DlPaint* paint, @@ -738,10 +749,11 @@ bool DisplayListBuilder::QuickReject(const SkRect& bounds) const { } void DisplayListBuilder::drawPaint() { - if (paint_affects_dest(current_, kDrawPaintFlags) && // - AccumulateUnbounded()) { + OpResult result = PaintResult(current_, kDrawPaintFlags); + if (result != OpResult::kNoEffect && AccumulateUnbounded()) { Push(0, 1); CheckLayerOpacityCompatibility(); + UpdateLayerResult(result); } } void DisplayListBuilder::DrawPaint(const DlPaint& paint) { @@ -749,10 +761,11 @@ void DisplayListBuilder::DrawPaint(const DlPaint& paint) { drawPaint(); } void DisplayListBuilder::DrawColor(DlColor color, DlBlendMode mode) { - if (paint_affects_dest(DlPaint(color).setBlendMode(mode)) && - AccumulateUnbounded()) { + OpResult result = PaintResult(DlPaint(color).setBlendMode(mode)); + if (result != OpResult::kNoEffect && AccumulateUnbounded()) { Push(0, 1, color, mode); CheckLayerOpacityCompatibility(mode); + UpdateLayerResult(result); } } void DisplayListBuilder::drawLine(const SkPoint& p0, const SkPoint& p1) { @@ -760,10 +773,11 @@ void DisplayListBuilder::drawLine(const SkPoint& p0, const SkPoint& p1) { DisplayListAttributeFlags flags = (bounds.width() > 0.0f && bounds.height() > 0.0f) ? kDrawLineFlags : kDrawHVLineFlags; - if (paint_affects_dest(current_, flags) && - AccumulateOpBounds(bounds, flags)) { + OpResult result = PaintResult(current_, flags); + if (result != OpResult::kNoEffect && AccumulateOpBounds(bounds, flags)) { Push(0, 1, p0, p1); CheckLayerOpacityCompatibility(); + UpdateLayerResult(result); } } void DisplayListBuilder::DrawLine(const SkPoint& p0, @@ -773,10 +787,12 @@ void DisplayListBuilder::DrawLine(const SkPoint& p0, drawLine(p0, p1); } void DisplayListBuilder::drawRect(const SkRect& rect) { - if (paint_affects_dest(current_, kDrawRectFlags) && - AccumulateOpBounds(rect, kDrawRectFlags)) { + DisplayListAttributeFlags flags = kDrawRectFlags; + OpResult result = PaintResult(current_, flags); + if (result != OpResult::kNoEffect && AccumulateOpBounds(rect, flags)) { Push(0, 1, rect); CheckLayerOpacityCompatibility(); + UpdateLayerResult(result); } } void DisplayListBuilder::DrawRect(const SkRect& rect, const DlPaint& paint) { @@ -784,10 +800,12 @@ void DisplayListBuilder::DrawRect(const SkRect& rect, const DlPaint& paint) { drawRect(rect); } void DisplayListBuilder::drawOval(const SkRect& bounds) { - if (paint_affects_dest(current_, kDrawOvalFlags) && - AccumulateOpBounds(bounds, kDrawOvalFlags)) { + DisplayListAttributeFlags flags = kDrawOvalFlags; + OpResult result = PaintResult(current_, flags); + if (result != OpResult::kNoEffect && AccumulateOpBounds(bounds, flags)) { Push(0, 1, bounds); CheckLayerOpacityCompatibility(); + UpdateLayerResult(result); } } void DisplayListBuilder::DrawOval(const SkRect& bounds, const DlPaint& paint) { @@ -795,12 +813,15 @@ void DisplayListBuilder::DrawOval(const SkRect& bounds, const DlPaint& paint) { drawOval(bounds); } void DisplayListBuilder::drawCircle(const SkPoint& center, SkScalar radius) { - if (paint_affects_dest(current_, kDrawCircleFlags)) { + DisplayListAttributeFlags flags = kDrawCircleFlags; + OpResult result = PaintResult(current_, flags); + if (result != OpResult::kNoEffect) { SkRect bounds = SkRect::MakeLTRB(center.fX - radius, center.fY - radius, center.fX + radius, center.fY + radius); - if (AccumulateOpBounds(bounds, kDrawCircleFlags)) { + if (AccumulateOpBounds(bounds, flags)) { Push(0, 1, center, radius); CheckLayerOpacityCompatibility(); + UpdateLayerResult(result); } } } @@ -816,10 +837,13 @@ void DisplayListBuilder::drawRRect(const SkRRect& rrect) { } else if (rrect.isOval()) { drawOval(rrect.rect()); } else { - if (paint_affects_dest(current_, kDrawRRectFlags) && - AccumulateOpBounds(rrect.getBounds(), kDrawRRectFlags)) { + DisplayListAttributeFlags flags = kDrawRRectFlags; + OpResult result = PaintResult(current_, flags); + if (result != OpResult::kNoEffect && + AccumulateOpBounds(rrect.getBounds(), flags)) { Push(0, 1, rrect); CheckLayerOpacityCompatibility(); + UpdateLayerResult(result); } } } @@ -829,10 +853,13 @@ void DisplayListBuilder::DrawRRect(const SkRRect& rrect, const DlPaint& paint) { } void DisplayListBuilder::drawDRRect(const SkRRect& outer, const SkRRect& inner) { - if (paint_affects_dest(current_, kDrawDRRectFlags) && - AccumulateOpBounds(outer.getBounds(), kDrawDRRectFlags)) { + DisplayListAttributeFlags flags = kDrawDRRectFlags; + OpResult result = PaintResult(current_, flags); + if (result != OpResult::kNoEffect && + AccumulateOpBounds(outer.getBounds(), flags)) { Push(0, 1, outer, inner); CheckLayerOpacityCompatibility(); + UpdateLayerResult(result); } } void DisplayListBuilder::DrawDRRect(const SkRRect& outer, @@ -842,14 +869,16 @@ void DisplayListBuilder::DrawDRRect(const SkRRect& outer, drawDRRect(outer, inner); } void DisplayListBuilder::drawPath(const SkPath& path) { - if (paint_affects_dest(current_, kDrawPathFlags)) { - bool is_visible = - path.isInverseFillType() - ? AccumulateUnbounded() - : AccumulateOpBounds(path.getBounds(), kDrawPathFlags); + DisplayListAttributeFlags flags = kDrawPathFlags; + OpResult result = PaintResult(current_, flags); + if (result != OpResult::kNoEffect) { + bool is_visible = path.isInverseFillType() + ? AccumulateUnbounded() + : AccumulateOpBounds(path.getBounds(), flags); if (is_visible) { Push(0, 1, path); CheckLayerOpacityHairlineCompatibility(); + UpdateLayerResult(result); } } } @@ -866,17 +895,18 @@ void DisplayListBuilder::drawArc(const SkRect& bounds, useCenter // ? kDrawArcWithCenterFlags : kDrawArcNoCenterFlags; + OpResult result = PaintResult(current_, flags); // This could be tighter if we compute where the start and end // angles are and then also consider the quadrants swept and // the center if specified. - if (paint_affects_dest(current_, flags) && - AccumulateOpBounds(bounds, flags)) { + if (result != OpResult::kNoEffect && AccumulateOpBounds(bounds, flags)) { Push(0, 1, bounds, start, sweep, useCenter); if (useCenter) { CheckLayerOpacityHairlineCompatibility(); } else { CheckLayerOpacityCompatibility(); } + UpdateLayerResult(result); } } void DisplayListBuilder::DrawArc(const SkRect& bounds, @@ -888,6 +918,7 @@ void DisplayListBuilder::DrawArc(const SkRect& bounds, paint, useCenter ? kDrawArcWithCenterFlags : kDrawArcNoCenterFlags); drawArc(bounds, start, sweep, useCenter); } + DisplayListAttributeFlags DisplayListBuilder::FlagsForPointMode( PointMode mode) { switch (mode) { @@ -900,7 +931,6 @@ DisplayListAttributeFlags DisplayListBuilder::FlagsForPointMode( } FML_DCHECK(false); } - void DisplayListBuilder::drawPoints(PointMode mode, uint32_t count, const SkPoint pts[]) { @@ -908,7 +938,8 @@ void DisplayListBuilder::drawPoints(PointMode mode, return; } DisplayListAttributeFlags flags = FlagsForPointMode(mode); - if (!paint_affects_dest(current_, flags)) { + OpResult result = PaintResult(current_, flags); + if (result == OpResult::kNoEffect) { return; } @@ -945,33 +976,21 @@ void DisplayListBuilder::drawPoints(PointMode mode, // bounds of every sub-primitive. // See: https://fiddle.skia.org/c/228459001d2de8db117ce25ef5cedb0c UpdateLayerOpacityCompatibility(false); + UpdateLayerResult(result); } void DisplayListBuilder::DrawPoints(PointMode mode, uint32_t count, const SkPoint pts[], const DlPaint& paint) { - const DisplayListAttributeFlags* flags; - switch (mode) { - case PointMode::kPoints: - flags = &DisplayListOpFlags::kDrawPointsAsPointsFlags; - break; - case PointMode::kLines: - flags = &DisplayListOpFlags::kDrawPointsAsLinesFlags; - break; - case PointMode::kPolygon: - flags = &DisplayListOpFlags::kDrawPointsAsPolygonFlags; - break; - default: - FML_DCHECK(false); - return; - } - SetAttributesFromPaint(paint, *flags); + SetAttributesFromPaint(paint, FlagsForPointMode(mode)); drawPoints(mode, count, pts); } void DisplayListBuilder::drawVertices(const DlVertices* vertices, DlBlendMode mode) { - if (paint_affects_dest(current_, kDrawVerticesFlags) && - AccumulateOpBounds(vertices->bounds(), kDrawVerticesFlags)) { + DisplayListAttributeFlags flags = kDrawVerticesFlags; + OpResult result = PaintResult(current_, flags); + if (result != OpResult::kNoEffect && + AccumulateOpBounds(vertices->bounds(), flags)) { void* pod = Push(vertices->size(), 1, mode); new (pod) DlVertices(vertices); // DrawVertices applies its colors to the paint so we have no way @@ -979,6 +998,7 @@ void DisplayListBuilder::drawVertices(const DlVertices* vertices, // Although, examination of the |mode| might find some predictable // cases. UpdateLayerOpacityCompatibility(false); + UpdateLayerResult(result); } } void DisplayListBuilder::DrawVertices(const DlVertices* vertices, @@ -995,12 +1015,8 @@ void DisplayListBuilder::drawImage(const sk_sp image, DisplayListAttributeFlags flags = render_with_attributes // ? kDrawImageWithPaintFlags : kDrawImageFlags; - if (render_with_attributes) { - // paint_affects_dest checks layer->is_nop - if (!paint_affects_dest(current_, flags)) { - return; - } - } else if (current_layer_->is_nop_) { + OpResult result = PaintResult(current_, flags); + if (result == OpResult::kNoEffect) { return; } SkRect bounds = SkRect::MakeXYWH(point.fX, point.fY, // @@ -1010,6 +1026,7 @@ void DisplayListBuilder::drawImage(const sk_sp image, ? Push(0, 1, image, point, sampling) : Push(0, 1, image, point, sampling); CheckLayerOpacityCompatibility(render_with_attributes); + UpdateLayerResult(result); is_ui_thread_safe_ = is_ui_thread_safe_ && image->isUIThreadSafe(); } } @@ -1034,18 +1051,12 @@ void DisplayListBuilder::drawImageRect(const sk_sp image, DisplayListAttributeFlags flags = render_with_attributes ? kDrawImageRectWithPaintFlags : kDrawImageRectFlags; - if (render_with_attributes) { - // paint_affects_dest checks layer->is_nop - if (!paint_affects_dest(current_, flags)) { - return; - } - } else if (current_layer_->is_nop_) { - return; - } - if (AccumulateOpBounds(dst, flags)) { + OpResult result = PaintResult(current_, flags); + if (result != OpResult::kNoEffect && AccumulateOpBounds(dst, flags)) { Push(0, 1, image, src, dst, sampling, render_with_attributes, constraint); CheckLayerOpacityCompatibility(render_with_attributes); + UpdateLayerResult(result); is_ui_thread_safe_ = is_ui_thread_safe_ && image->isUIThreadSafe(); } } @@ -1071,19 +1082,13 @@ void DisplayListBuilder::drawImageNine(const sk_sp image, DisplayListAttributeFlags flags = render_with_attributes ? kDrawImageNineWithPaintFlags : kDrawImageNineFlags; - if (render_with_attributes) { - // paint_affects_dest checks layer->is_nop - if (!paint_affects_dest(current_, flags)) { - return; - } - } else if (current_layer_->is_nop_) { - return; - } - if (AccumulateOpBounds(dst, flags)) { + OpResult result = PaintResult(current_, flags); + if (result != OpResult::kNoEffect && AccumulateOpBounds(dst, flags)) { render_with_attributes ? Push(0, 1, image, center, dst, filter) : Push(0, 1, image, center, dst, filter); CheckLayerOpacityCompatibility(render_with_attributes); + UpdateLayerResult(result); is_ui_thread_safe_ = is_ui_thread_safe_ && image->isUIThreadSafe(); } } @@ -1112,12 +1117,8 @@ void DisplayListBuilder::drawAtlas(const sk_sp atlas, DisplayListAttributeFlags flags = render_with_attributes // ? kDrawAtlasWithPaintFlags : kDrawAtlasFlags; - if (render_with_attributes) { - // paint_affects_dest checks layer->is_nop - if (!paint_affects_dest(current_, flags)) { - return; - } - } else if (current_layer_->is_nop_) { + OpResult result = PaintResult(current_, flags); + if (result == OpResult::kNoEffect) { return; } SkPoint quad[4]; @@ -1162,6 +1163,7 @@ void DisplayListBuilder::drawAtlas(const sk_sp atlas, // on it to distribute the opacity without overlap without checking all // of the transforms and texture rectangles. UpdateLayerOpacityCompatibility(false); + UpdateLayerResult(result); is_ui_thread_safe_ = is_ui_thread_safe_ && atlas->isUIThreadSafe(); } void DisplayListBuilder::DrawAtlas(const sk_sp& atlas, @@ -1237,12 +1239,17 @@ void DisplayListBuilder::DrawDisplayList(const sk_sp display_list, nested_op_count_ += display_list->op_count(true) - 1; nested_bytes_ += display_list->bytes(true); UpdateLayerOpacityCompatibility(display_list->can_apply_group_opacity()); + UpdateLayerResult(display_list->affects_transparent_surface() + ? OpResult::kDrawsPixels + : OpResult::kClearsPixels); } void DisplayListBuilder::drawTextBlob(const sk_sp blob, SkScalar x, SkScalar y) { - if (paint_affects_dest(current_, kDrawTextBlobFlags) && - AccumulateOpBounds(blob->bounds().makeOffset(x, y), kDrawTextBlobFlags)) { + DisplayListAttributeFlags flags = kDrawTextBlobFlags; + OpResult result = PaintResult(current_, flags); + if (result != OpResult::kNoEffect && + AccumulateOpBounds(blob->bounds().makeOffset(x, y), flags)) { Push(0, 1, blob, x, y); // There is no way to query if the glyphs of a text blob overlap and // there are no current guarantees from either Skia or Impeller that @@ -1250,6 +1257,7 @@ void DisplayListBuilder::drawTextBlob(const sk_sp blob, // so we must make the conservative assessment that this DL layer is // not compatible with group opacity inheritance. UpdateLayerOpacityCompatibility(false); + UpdateLayerResult(result); } } void DisplayListBuilder::DrawTextBlob(const sk_sp& blob, @@ -1264,7 +1272,8 @@ void DisplayListBuilder::DrawShadow(const SkPath& path, const SkScalar elevation, bool transparent_occluder, SkScalar dpr) { - if (paint_affects_dest(DlPaint(color))) { + OpResult result = PaintResult(DlPaint(color)); + if (result != OpResult::kNoEffect) { SkRect shadow_bounds = DlCanvas::ComputeShadowBounds(path, elevation, dpr, GetTransform()); if (AccumulateOpBounds(shadow_bounds, kDrawShadowFlags)) { @@ -1273,6 +1282,7 @@ void DisplayListBuilder::DrawShadow(const SkPath& path, dpr) : Push(0, 1, path, color, elevation, dpr); UpdateLayerOpacityCompatibility(false); + UpdateLayerResult(result); } } } @@ -1448,57 +1458,90 @@ bool DisplayListBuilder::paint_nops_on_transparency() { DlColor DisplayListBuilder::GetEffectiveColor(const DlPaint& paint, DisplayListAttributeFlags flags) { - if (flags.applies_image_filter() && paint.getImageFilterPtr()) { - return kAnyColor; - } - if (flags.applies_color_filter() && paint.getColorFilterPtr()) { - return kAnyColor; - } + DlColor color; if (flags.applies_alpha_or_color()) { const DlColorSource* source = paint.getColorSourcePtr(); if (source) { if (source->asColor()) { - return source->asColor()->color(); + color = source->asColor()->color(); + } else { + color = source->is_opaque() ? DlColor::kBlack() : kAnyColor; } - return source->is_opaque() ? DlColor::kBlack() : kAnyColor; + } else { + color = paint.getColor(); } - return paint.getColor(); } - return kAnyColor; + if (flags.applies_image_filter()) { + auto filter = paint.getImageFilterPtr(); + if (filter) { + if (!color.isTransparent() || filter->modifies_transparent_black()) { + color = kAnyColor; + } + } + } + if (flags.applies_color_filter()) { + auto filter = paint.getColorFilterPtr(); + if (filter) { + if (!color.isTransparent() || filter->modifies_transparent_black()) { + color = kAnyColor; + } + } + } + return color; } -bool DisplayListBuilder::paint_affects_dest(const DlPaint& paint, - DisplayListAttributeFlags flags) { +DisplayListBuilder::OpResult DisplayListBuilder::PaintResult( + const DlPaint& paint, + DisplayListAttributeFlags flags) { if (current_layer_->is_nop_) { - return false; + return OpResult::kNoEffect; } if (flags.applies_blend()) { switch (paint.getBlendMode()) { // Nop blend mode (singular, there is only one) case DlBlendMode::kDst: - return false; + return OpResult::kNoEffect; + + // Always clears pixels blend mode (singular, there is only one) + case DlBlendMode::kClear: + return OpResult::kClearsPixels; // Always destructive blend modes + // These modes ignore source alpha entirely + case DlBlendMode::kHue: + case DlBlendMode::kSaturation: + case DlBlendMode::kColor: + case DlBlendMode::kLuminosity: + return OpResult::kDrawsPixels; + + // Always destructive blend modes + // The ops will clear the destination if the source is transparent // (Some answers might differ if dest is opaque, but that is unknown) case DlBlendMode::kSrc: - case DlBlendMode::kClear: case DlBlendMode::kSrcIn: case DlBlendMode::kSrcOut: case DlBlendMode::kDstATop: case DlBlendMode::kModulate: - case DlBlendMode::kHue: - case DlBlendMode::kSaturation: - case DlBlendMode::kColor: - case DlBlendMode::kLuminosity: - return true; + return GetEffectiveColor(paint, flags).isTransparent() + ? OpResult::kClearsPixels + : OpResult::kDrawsPixels; - // The next group of blend modes modifies the destination unless the - // source color is opaque (or there is a color or image filter). - case DlBlendMode::kDstIn: - return !GetEffectiveColor(paint, flags).isOpaque(); + // The kDstIn blend mode modifies the destination unless the + // source color is opaque. Additionally, it will deterministically + // clear the destination if the source is transparent. + case DlBlendMode::kDstIn: { + DlColor color = GetEffectiveColor(paint, flags); + if (color.isOpaque()) { + return OpResult::kNoEffect; + } else if (color.isTransparent()) { + return OpResult::kClearsPixels; + } else { + return OpResult::kDrawsPixels; + } + } // The next group of blend modes modifies the destination unless the - // source color is transparent (or there is a color or image filter). + // source color is transparent. case DlBlendMode::kSrcOver: case DlBlendMode::kDstOver: case DlBlendMode::kDstOut: @@ -1515,14 +1558,18 @@ bool DisplayListBuilder::paint_affects_dest(const DlPaint& paint, case DlBlendMode::kSoftLight: case DlBlendMode::kDifference: case DlBlendMode::kExclusion: - return !GetEffectiveColor(paint, flags).isTransparent(); + return GetEffectiveColor(paint, flags).isTransparent() + ? OpResult::kNoEffect + : OpResult::kDrawsPixels; // Color Burn only leaves the pixel alone when the source is white. case DlBlendMode::kColorBurn: - return GetEffectiveColor(paint, flags) != DlColor::kWhite(); + return GetEffectiveColor(paint, flags) == DlColor::kWhite() + ? OpResult::kNoEffect + : OpResult::kDrawsPixels; } } - return true; + return OpResult::kDrawsPixels; } } // namespace flutter diff --git a/display_list/dl_builder.h b/display_list/dl_builder.h index db9c04b489a29..040f622284515 100644 --- a/display_list/dl_builder.h +++ b/display_list/dl_builder.h @@ -521,6 +521,9 @@ class DisplayListBuilder final : public virtual DlCanvas, bool has_layer() const { return has_layer_; } bool cannot_inherit_opacity() const { return cannot_inherit_opacity_; } bool has_compatible_op() const { return has_compatible_op_; } + bool affects_transparent_layer() const { + return affects_transparent_layer_; + } bool is_group_opacity_compatible() const { return !cannot_inherit_opacity_; @@ -543,6 +546,12 @@ class DisplayListBuilder final : public virtual DlCanvas, } } + // Records that the current layer contains an op that produces visible + // output on a transparent surface. + void add_visible_op() { + affects_transparent_layer_ = true; + } + // The filter to apply to the layer bounds when it is restored std::shared_ptr filter() { return filter_; } @@ -583,6 +592,7 @@ class DisplayListBuilder final : public virtual DlCanvas, bool is_unbounded_ = false; bool has_deferred_save_op_ = false; bool is_nop_ = false; + bool affects_transparent_layer_ = false; friend class DisplayListBuilder; }; @@ -698,9 +708,26 @@ class DisplayListBuilder final : public virtual DlCanvas, static DisplayListAttributeFlags FlagsForPointMode(PointMode mode); + enum class OpResult { + kNoEffect, + kClearsPixels, + kDrawsPixels, + }; + bool paint_nops_on_transparency(); - bool paint_affects_dest(const DlPaint& paint, - DisplayListAttributeFlags flags = kDrawPaintFlags); + OpResult PaintResult(const DlPaint& paint, + DisplayListAttributeFlags flags = kDrawPaintFlags); + + void UpdateLayerResult(OpResult result) { + switch (result) { + case OpResult::kNoEffect: + case OpResult::kClearsPixels: + break; + case OpResult::kDrawsPixels: + current_layer_->add_visible_op(); + break; + } + } // kAnyColor is a non-opaque and non-transparent color that will not // trigger any short-circuit tests about the results of a blend. diff --git a/shell/common/dl_op_spy.cc b/shell/common/dl_op_spy.cc index 0ef9553a52336..15910fc2e85de 100644 --- a/shell/common/dl_op_spy.cc +++ b/shell/common/dl_op_spy.cc @@ -34,7 +34,7 @@ void DlOpSpy::saveLayer(const SkRect* bounds, const DlImageFilter* backdrop) {} void DlOpSpy::restore() {} void DlOpSpy::drawColor(DlColor color, DlBlendMode mode) { - did_draw_ |= !(color.isTransparent() && mode == DlBlendMode::kSrcOver); + did_draw_ |= !color.isTransparent(); } void DlOpSpy::drawPaint() { did_draw_ |= will_draw_; diff --git a/shell/common/dl_op_spy_unittests.cc b/shell/common/dl_op_spy_unittests.cc index 6a82f1b16c251..3852204753612 100644 --- a/shell/common/dl_op_spy_unittests.cc +++ b/shell/common/dl_op_spy_unittests.cc @@ -13,21 +13,31 @@ namespace flutter { namespace testing { // The following 2 macros demonstrate that the DlOpSpy class is equivalent -// to simpler tests on the DisplayList object itself now that the -// DisplayListBuilder implements operation culling. +// to DisplayList::affects_transparent_surface() now that DisplayListBuilder +// implements operation culling. // See https://github.com/flutter/flutter/issues/125403 -#define ASSERT_DID_DRAW(spy, dl) \ - do { \ - ASSERT_TRUE(spy.did_draw()); \ - ASSERT_GT(dl->op_count(), 0u); \ - ASSERT_FALSE(dl->bounds().isEmpty()); \ +#define ASSERT_DID_DRAW(spy, dl) \ + do { \ + ASSERT_TRUE(spy.did_draw()); \ + ASSERT_TRUE(dl->affects_transparent_surface()); \ + ASSERT_GT(dl->op_count(), 0u); \ + ASSERT_FALSE(dl->bounds().isEmpty()); \ } while (0) -#define ASSERT_NO_DRAW(spy, dl) \ - do { \ - ASSERT_FALSE(spy.did_draw()); \ - ASSERT_EQ(dl->op_count(), 0u); \ - ASSERT_TRUE(dl->bounds().isEmpty()); \ +#define ASSERT_TRANSPARENT_DRAW(spy, dl) \ + do { \ + ASSERT_FALSE(spy.did_draw()); \ + ASSERT_FALSE(dl->affects_transparent_surface()); \ + ASSERT_GT(dl->op_count(), 0u); \ + ASSERT_FALSE(dl->bounds().isEmpty()); \ + } while (0) + +#define ASSERT_NO_DRAW(spy, dl) \ + do { \ + ASSERT_FALSE(spy.did_draw()); \ + ASSERT_FALSE(dl->affects_transparent_surface()); \ + ASSERT_EQ(dl->op_count(), 0u); \ + ASSERT_TRUE(dl->bounds().isEmpty()); \ } while (0) TEST(DlOpSpy, DidDrawIsFalseByDefault) { @@ -112,16 +122,16 @@ TEST(DlOpSpy, DrawColor) { dl->Dispatch(dl_op_spy); ASSERT_DID_DRAW(dl_op_spy, dl); } - { // Transparent color source. + { // Transparent color with kSrc. DisplayListBuilder builder; auto color = DlColor::kTransparent(); builder.DrawColor(color, DlBlendMode::kSrc); sk_sp dl = builder.Build(); DlOpSpy dl_op_spy; dl->Dispatch(dl_op_spy); - ASSERT_DID_DRAW(dl_op_spy, dl); + ASSERT_TRANSPARENT_DRAW(dl_op_spy, dl); } - { // Transparent color source-over. + { // Transparent color with kSrcOver. DisplayListBuilder builder; auto color = DlColor::kTransparent(); builder.DrawColor(color, DlBlendMode::kSrcOver); From abfc8b43c2eaee064acf936b771e24f6b6cd8d82 Mon Sep 17 00:00:00 2001 From: Jim Graham Date: Wed, 26 Apr 2023 18:06:34 -0700 Subject: [PATCH 3/8] adjust some FML assertions --- display_list/dl_builder.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/display_list/dl_builder.cc b/display_list/dl_builder.cc index b6477099933ee..fcef518ed5ae2 100644 --- a/display_list/dl_builder.cc +++ b/display_list/dl_builder.cc @@ -500,7 +500,7 @@ void DisplayListBuilder::saveLayer(const SkRect* bounds, // Accumulate should always return true here because if the // clip was empty then that would have been caught up above // when we tested the PaintResult. - bool unclipped = AccumulateUnbounded(); + [[maybe_unused]] bool unclipped = AccumulateUnbounded(); FML_DCHECK(unclipped); } CheckLayerOpacityCompatibility(true); @@ -520,7 +520,7 @@ void DisplayListBuilder::saveLayer(const SkRect* bounds, // Accumulate should always return true here because if the // clip was empty then that would have been caught up above // when we tested the PaintResult. - bool unclipped = AccumulateUnbounded(); + [[maybe_unused]] bool unclipped = AccumulateUnbounded(); FML_DCHECK(unclipped); bounds // ? Push(0, 1, options, *bounds, backdrop) @@ -929,7 +929,7 @@ DisplayListAttributeFlags DisplayListBuilder::FlagsForPointMode( case PointMode::kPolygon: return kDrawPointsAsPolygonFlags; } - FML_DCHECK(false); + FML_UNREACHABLE(); } void DisplayListBuilder::drawPoints(PointMode mode, uint32_t count, @@ -966,7 +966,7 @@ void DisplayListBuilder::drawPoints(PointMode mode, data_ptr = Push(bytes, 1, count); break; default: - FML_DCHECK(false); + FML_UNREACHABLE(); return; } CopyV(data_ptr, pts, count); From 95a18a069f6604396cd6aac259861afa26706728 Mon Sep 17 00:00:00 2001 From: Jim Graham Date: Wed, 26 Apr 2023 19:14:56 -0700 Subject: [PATCH 4/8] remove more avoid accidental DL record culling in unit tests --- impeller/display_list/dl_unittests.cc | 16 +++++----------- shell/common/dl_op_spy_unittests.cc | 24 ++++++++++++++++++++---- 2 files changed, 25 insertions(+), 15 deletions(-) diff --git a/impeller/display_list/dl_unittests.cc b/impeller/display_list/dl_unittests.cc index 9029d3ea7fcb0..f08087935b8ff 100644 --- a/impeller/display_list/dl_unittests.cc +++ b/impeller/display_list/dl_unittests.cc @@ -830,18 +830,12 @@ TEST_P(DisplayListTest, CanDrawShadow) { } TEST_P(DisplayListTest, TransparentShadowProducesCorrectColor) { - flutter::DisplayListBuilder builder; - { - builder.Save(); - builder.Scale(1.618, 1.618); - builder.DrawShadow(SkPath{}.addRect(SkRect::MakeXYWH(0, 0, 200, 100)), - SK_ColorTRANSPARENT, 15, false, 1); - builder.Restore(); - } - auto dl = builder.Build(); - DlDispatcher dispatcher; - dispatcher.drawDisplayList(dl, 1); + dispatcher.save(); + dispatcher.scale(1.618, 1.618); + dispatcher.drawShadow(SkPath{}.addRect(SkRect::MakeXYWH(0, 0, 200, 100)), + SK_ColorTRANSPARENT, 15, false, 1); + dispatcher.restore(); auto picture = dispatcher.EndRecordingAsPicture(); std::shared_ptr rrect_shadow; diff --git a/shell/common/dl_op_spy_unittests.cc b/shell/common/dl_op_spy_unittests.cc index 3852204753612..6e43c283ebd2e 100644 --- a/shell/common/dl_op_spy_unittests.cc +++ b/shell/common/dl_op_spy_unittests.cc @@ -359,8 +359,16 @@ TEST(DlOpSpy, DrawVertices) { SkPoint::Make(5, 15), SkPoint::Make(15, 5), }; - const SkPoint texture_coordinates[] = {SkPoint::Make(5, 5)}; - const DlColor colors[] = {DlColor::kBlack()}; + const SkPoint texture_coordinates[] = { + SkPoint::Make(5, 5), + SkPoint::Make(15, 5), + SkPoint::Make(5, 15), + }; + const DlColor colors[] = { + DlColor::kBlack(), + DlColor::kRed(), + DlColor::kGreen(), + }; auto dl_vertices = DlVertices::Make(DlVertexMode::kTriangles, 3, vertices, texture_coordinates, colors, 0); builder.DrawVertices(dl_vertices.get(), DlBlendMode::kSrc, paint); @@ -377,8 +385,16 @@ TEST(DlOpSpy, DrawVertices) { SkPoint::Make(5, 15), SkPoint::Make(15, 5), }; - const SkPoint texture_coordinates[] = {SkPoint::Make(5, 5)}; - const DlColor colors[] = {DlColor::kBlack()}; + const SkPoint texture_coordinates[] = { + SkPoint::Make(5, 5), + SkPoint::Make(15, 5), + SkPoint::Make(5, 15), + }; + const DlColor colors[] = { + DlColor::kBlack(), + DlColor::kRed(), + DlColor::kGreen(), + }; auto dl_vertices = DlVertices::Make(DlVertexMode::kTriangles, 3, vertices, texture_coordinates, colors, 0); builder.DrawVertices(dl_vertices.get(), DlBlendMode::kSrc, paint); From d1cce7e06b5297345ba0639541633ce73eb89ffd Mon Sep 17 00:00:00 2001 From: Jim Graham Date: Wed, 26 Apr 2023 21:31:17 -0700 Subject: [PATCH 5/8] work-around to prevent culling all text ops in Fuchsia testing --- display_list/dl_builder.cc | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/display_list/dl_builder.cc b/display_list/dl_builder.cc index fcef518ed5ae2..a6fd7842015de 100644 --- a/display_list/dl_builder.cc +++ b/display_list/dl_builder.cc @@ -1248,8 +1248,18 @@ void DisplayListBuilder::drawTextBlob(const sk_sp blob, SkScalar y) { DisplayListAttributeFlags flags = kDrawTextBlobFlags; OpResult result = PaintResult(current_, flags); - if (result != OpResult::kNoEffect && - AccumulateOpBounds(blob->bounds().makeOffset(x, y), flags)) { + if (result == OpResult::kNoEffect) { + return; + } + bool unclipped = AccumulateOpBounds(blob->bounds().makeOffset(x, y), flags); + // TODO(https://github.com/flutter/flutter/issues/82202): Remove once the + // unit tests can use Fuchsia's font manager instead of the empty default. + // Until then we might encounter empty bounds for otherwise valid text and + // thus we ignore the results from AccumulateOpBounds. +#if defined(OS_FUCHSIA) + unclipped = true; +#endif // OS_FUCHSIA + if (unclipped) { Push(0, 1, blob, x, y); // There is no way to query if the glyphs of a text blob overlap and // there are no current guarantees from either Skia or Impeller that From 648b737c5f753bb14622300cbf3de1b6c74354d4 Mon Sep 17 00:00:00 2001 From: Jim Graham Date: Wed, 26 Apr 2023 23:38:33 -0700 Subject: [PATCH 6/8] remove unrelated checks from DlOpSpy tests --- shell/common/dl_op_spy_unittests.cc | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/shell/common/dl_op_spy_unittests.cc b/shell/common/dl_op_spy_unittests.cc index 6e43c283ebd2e..08394afdd5ba1 100644 --- a/shell/common/dl_op_spy_unittests.cc +++ b/shell/common/dl_op_spy_unittests.cc @@ -12,7 +12,7 @@ namespace flutter { namespace testing { -// The following 2 macros demonstrate that the DlOpSpy class is equivalent +// The following macros demonstrate that the DlOpSpy class is equivalent // to DisplayList::affects_transparent_surface() now that DisplayListBuilder // implements operation culling. // See https://github.com/flutter/flutter/issues/125403 @@ -20,24 +20,12 @@ namespace testing { do { \ ASSERT_TRUE(spy.did_draw()); \ ASSERT_TRUE(dl->affects_transparent_surface()); \ - ASSERT_GT(dl->op_count(), 0u); \ - ASSERT_FALSE(dl->bounds().isEmpty()); \ - } while (0) - -#define ASSERT_TRANSPARENT_DRAW(spy, dl) \ - do { \ - ASSERT_FALSE(spy.did_draw()); \ - ASSERT_FALSE(dl->affects_transparent_surface()); \ - ASSERT_GT(dl->op_count(), 0u); \ - ASSERT_FALSE(dl->bounds().isEmpty()); \ } while (0) #define ASSERT_NO_DRAW(spy, dl) \ do { \ ASSERT_FALSE(spy.did_draw()); \ ASSERT_FALSE(dl->affects_transparent_surface()); \ - ASSERT_EQ(dl->op_count(), 0u); \ - ASSERT_TRUE(dl->bounds().isEmpty()); \ } while (0) TEST(DlOpSpy, DidDrawIsFalseByDefault) { @@ -45,6 +33,14 @@ TEST(DlOpSpy, DidDrawIsFalseByDefault) { ASSERT_FALSE(dl_op_spy.did_draw()); } +TEST(DlOpSpy, EmptyDisplayList) { + DisplayListBuilder builder; + sk_sp dl = builder.Build(); + DlOpSpy dl_op_spy; + dl->Dispatch(dl_op_spy); + ASSERT_NO_DRAW(dl_op_spy, dl); +} + TEST(DlOpSpy, SetColor) { { // No Color set. DisplayListBuilder builder; @@ -129,7 +125,7 @@ TEST(DlOpSpy, DrawColor) { sk_sp dl = builder.Build(); DlOpSpy dl_op_spy; dl->Dispatch(dl_op_spy); - ASSERT_TRANSPARENT_DRAW(dl_op_spy, dl); + ASSERT_NO_DRAW(dl_op_spy, dl); } { // Transparent color with kSrcOver. DisplayListBuilder builder; From a6c654d46a2dff7da33090c165ca6c08e4036288 Mon Sep 17 00:00:00 2001 From: Jim Graham Date: Fri, 5 May 2023 17:48:00 -0700 Subject: [PATCH 7/8] fix missing ColorBurn saveLayer contents --- display_list/dl_builder.cc | 6 +++++- display_list/dl_builder.h | 2 +- display_list/dl_color.h | 12 ++++++------ 3 files changed, 12 insertions(+), 8 deletions(-) diff --git a/display_list/dl_builder.cc b/display_list/dl_builder.cc index a6fd7842015de..c728b81aa6b5c 100644 --- a/display_list/dl_builder.cc +++ b/display_list/dl_builder.cc @@ -1469,7 +1469,7 @@ bool DisplayListBuilder::paint_nops_on_transparency() { DlColor DisplayListBuilder::GetEffectiveColor(const DlPaint& paint, DisplayListAttributeFlags flags) { DlColor color; - if (flags.applies_alpha_or_color()) { + if (flags.applies_color()) { const DlColorSource* source = paint.getColorSourcePtr(); if (source) { if (source->asColor()) { @@ -1480,6 +1480,10 @@ DlColor DisplayListBuilder::GetEffectiveColor(const DlPaint& paint, } else { color = paint.getColor(); } + } else if (flags.applies_alpha()) { + color = kAnyColor.withAlpha(paint.getAlpha()); + } else { + color = kAnyColor; } if (flags.applies_image_filter()) { auto filter = paint.getImageFilterPtr(); diff --git a/display_list/dl_builder.h b/display_list/dl_builder.h index 040f622284515..86fe261861288 100644 --- a/display_list/dl_builder.h +++ b/display_list/dl_builder.h @@ -731,7 +731,7 @@ class DisplayListBuilder final : public virtual DlCanvas, // kAnyColor is a non-opaque and non-transparent color that will not // trigger any short-circuit tests about the results of a blend. - static constexpr DlColor kAnyColor = DlColor(0x7fffffff); + static constexpr DlColor kAnyColor = DlColor::kMidGrey().withAlpha(0x7f); static_assert(!kAnyColor.isOpaque()); static_assert(!kAnyColor.isTransparent()); static DlColor GetEffectiveColor(const DlPaint& paint, diff --git a/display_list/dl_color.h b/display_list/dl_color.h index 2b6d5e8360698..92a39150d2f2e 100644 --- a/display_list/dl_color.h +++ b/display_list/dl_color.h @@ -47,7 +47,7 @@ struct DlColor { constexpr float getGreenF() const { return toF(getGreen()); } constexpr float getBlueF() const { return toF(getBlue()); } - uint32_t premultipliedArgb() const { + constexpr uint32_t premultipliedArgb() const { if (isOpaque()) { return argb; } @@ -58,20 +58,20 @@ struct DlColor { toC(getBlueF() * f); } - DlColor withAlpha(uint8_t alpha) const { // + constexpr DlColor withAlpha(uint8_t alpha) const { // return (argb & 0x00FFFFFF) | (alpha << 24); } - DlColor withRed(uint8_t red) const { // + constexpr DlColor withRed(uint8_t red) const { // return (argb & 0xFF00FFFF) | (red << 16); } - DlColor withGreen(uint8_t green) const { // + constexpr DlColor withGreen(uint8_t green) const { // return (argb & 0xFFFF00FF) | (green << 8); } - DlColor withBlue(uint8_t blue) const { // + constexpr DlColor withBlue(uint8_t blue) const { // return (argb & 0xFFFFFF00) | (blue << 0); } - DlColor modulateOpacity(float opacity) const { + constexpr DlColor modulateOpacity(float opacity) const { return opacity <= 0 ? withAlpha(0) : opacity >= 1 ? *this : withAlpha(round(getAlpha() * opacity)); From 8ceae569126d5a60099f2bd00c269e15a4d5505d Mon Sep 17 00:00:00 2001 From: Jim Graham Date: Thu, 11 May 2023 20:22:19 -0700 Subject: [PATCH 8/8] adjust to new formatting and overload resolution behaviors --- display_list/display_list_unittests.cc | 3 ++- display_list/testing/dl_test_snippets.h | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/display_list/display_list_unittests.cc b/display_list/display_list_unittests.cc index b0e05d3fff120..bfa2a9148b957 100644 --- a/display_list/display_list_unittests.cc +++ b/display_list/display_list_unittests.cc @@ -2691,7 +2691,8 @@ TEST_F(DisplayListTest, NopOperationsOmittedFromRecords) { builder.DrawVertices(TestVertices1, DlBlendMode::kSrcOver, paint); builder.DrawImage(TestImage1, {10, 10}, DlImageSampling::kLinear, &paint); - builder.DrawImageRect(TestImage1, {0, 0, 10, 10}, {10, 10, 25, 25}, + 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, diff --git a/display_list/testing/dl_test_snippets.h b/display_list/testing/dl_test_snippets.h index 94be886d3e51d..c53059b9931f9 100644 --- a/display_list/testing/dl_test_snippets.h +++ b/display_list/testing/dl_test_snippets.h @@ -90,7 +90,7 @@ static sk_sp MakeTestImage(int w, int h, int checker_size) { static auto TestImage1 = MakeTestImage(40, 40, 5); static auto TestImage2 = MakeTestImage(50, 50, 5); -static auto TestSkImage = MakeTestImage(30, 30, 5) -> skia_image(); +static auto TestSkImage = MakeTestImage(30, 30, 5)->skia_image(); static const DlImageColorSource kTestSource1(TestImage1, DlTileMode::kClamp,