From 4f7f65eb99a4c032a3f0987e830d602aa9eda572 Mon Sep 17 00:00:00 2001 From: Jim Graham Date: Tue, 3 Aug 2021 00:10:02 -0700 Subject: [PATCH] fix a number of conditions for computing bounds from saveLayers (flutter/engine#27683) --- engine/src/flutter/flow/display_list.cc | 6 +- engine/src/flutter/flow/display_list.h | 6 +- .../flutter/flow/display_list_unittests.cc | 125 +++++++++++++++++ engine/src/flutter/flow/display_list_utils.cc | 126 +++++++++++++++--- engine/src/flutter/flow/display_list_utils.h | 24 +++- 5 files changed, 265 insertions(+), 22 deletions(-) diff --git a/engine/src/flutter/flow/display_list.cc b/engine/src/flutter/flow/display_list.cc index 7da9ec2b55d..6178eb2fed9 100644 --- a/engine/src/flutter/flow/display_list.cc +++ b/engine/src/flutter/flow/display_list.cc @@ -867,7 +867,7 @@ static void DisposeOps(uint8_t* ptr, uint8_t* end) { FOR_EACH_DISPLAY_LIST_OP(DL_OP_DISPOSE) -#undef DL_OP_DISPATCH +#undef DL_OP_DISPOSE default: FML_DCHECK(false); @@ -905,7 +905,7 @@ static bool CompareOps(uint8_t* ptrA, FOR_EACH_DISPLAY_LIST_OP(DL_OP_EQUALS) -#undef DL_OP_DISPATCH +#undef DL_OP_EQUALS default: FML_DCHECK(false); @@ -1246,7 +1246,7 @@ void DisplayListBuilder::drawPoints(SkCanvas::PointMode mode, uint32_t count, const SkPoint pts[]) { void* data_ptr; - FML_DCHECK(count < MaxDrawPointsCount); + FML_DCHECK(count < kMaxDrawPointsCount); int bytes = count * sizeof(SkPoint); switch (mode) { case SkCanvas::PointMode::kPoints_PointMode: diff --git a/engine/src/flutter/flow/display_list.h b/engine/src/flutter/flow/display_list.h index c1990eadeb0..a765cfbf6ce 100644 --- a/engine/src/flutter/flow/display_list.h +++ b/engine/src/flutter/flow/display_list.h @@ -222,7 +222,7 @@ class DisplayList : public SkRefCnt { class Dispatcher { public: // MaxDrawPointsCount * sizeof(SkPoint) must be less than 1 << 32 - static constexpr int MaxDrawPointsCount = ((1 << 29) - 1); + static constexpr int kMaxDrawPointsCount = ((1 << 29) - 1); virtual void setAA(bool aa) = 0; virtual void setDither(bool dither) = 0; @@ -334,7 +334,7 @@ class Dispatcher { // the DisplayListCanvasRecorder class. class DisplayListBuilder final : public virtual Dispatcher, public SkRefCnt { public: - DisplayListBuilder(const SkRect& cull = SkRect::MakeEmpty()); + DisplayListBuilder(const SkRect& cull = kMaxCull_); ~DisplayListBuilder(); void setAA(bool aa) override; @@ -451,6 +451,8 @@ class DisplayListBuilder final : public virtual Dispatcher, public SkRefCnt { int save_level_ = 0; SkRect cull_; + static constexpr SkRect kMaxCull_ = + SkRect::MakeLTRB(-1E9F, -1E9F, 1E9F, 1E9F); template void* Push(size_t extra, Args&&... args); diff --git a/engine/src/flutter/flow/display_list_unittests.cc b/engine/src/flutter/flow/display_list_unittests.cc index bdaf92b0e99..17447b7bd29 100644 --- a/engine/src/flutter/flow/display_list_unittests.cc +++ b/engine/src/flutter/flow/display_list_unittests.cc @@ -863,5 +863,130 @@ TEST(DisplayList, DisplayListsWithVaryingOpComparisons) { } } +TEST(DisplayList, DisplayListSaveLayerBoundsWithAlphaFilter) { + SkRect build_bounds = SkRect::MakeLTRB(-100, -100, 200, 200); + SkRect save_bounds = SkRect::MakeWH(100, 100); + SkRect rect = SkRect::MakeLTRB(30, 30, 70, 70); + // clang-format off + const float color_matrix[] = { + 0, 0, 0, 0, 0, + 0, 1, 0, 0, 0, + 0, 0, 1, 0, 0, + 0, 0, 0, 1, 0, + }; + // clang-format on + sk_sp base_color_filter = SkColorFilters::Matrix(color_matrix); + // clang-format off + const float alpha_matrix[] = { + 0, 0, 0, 0, 0, + 0, 1, 0, 0, 0, + 0, 0, 1, 0, 0, + 0, 0, 0, 0, 1, + }; + // clang-format on + sk_sp alpha_color_filter = + SkColorFilters::Matrix(alpha_matrix); + + { + DisplayListBuilder builder(build_bounds); + builder.saveLayer(&save_bounds, true); + builder.drawRect(rect); + builder.restore(); + sk_sp display_list = builder.Build(); + ASSERT_EQ(display_list->bounds(), rect); + } + + { + DisplayListBuilder builder(build_bounds); + builder.setColorFilter(base_color_filter); + builder.saveLayer(&save_bounds, true); + builder.setColorFilter(nullptr); + builder.drawRect(rect); + builder.restore(); + sk_sp display_list = builder.Build(); + ASSERT_EQ(display_list->bounds(), rect); + } + + { + DisplayListBuilder builder(build_bounds); + builder.setColorFilter(alpha_color_filter); + builder.saveLayer(&save_bounds, true); + builder.setColorFilter(nullptr); + builder.drawRect(rect); + builder.restore(); + sk_sp display_list = builder.Build(); + ASSERT_EQ(display_list->bounds(), save_bounds); + } + + { + DisplayListBuilder builder(build_bounds); + builder.setColorFilter(alpha_color_filter); + builder.saveLayer(nullptr, true); + builder.setColorFilter(nullptr); + builder.drawRect(rect); + builder.restore(); + sk_sp display_list = builder.Build(); + ASSERT_EQ(display_list->bounds(), build_bounds); + } + + { + DisplayListBuilder builder(build_bounds); + builder.setImageFilter( + SkImageFilters::ColorFilter(base_color_filter, nullptr)); + builder.saveLayer(&save_bounds, true); + builder.setImageFilter(nullptr); + builder.drawRect(rect); + builder.restore(); + sk_sp display_list = builder.Build(); + ASSERT_EQ(display_list->bounds(), rect); + } + + { + DisplayListBuilder builder(build_bounds); + builder.setImageFilter( + SkImageFilters::ColorFilter(alpha_color_filter, nullptr)); + builder.saveLayer(&save_bounds, true); + builder.setImageFilter(nullptr); + builder.drawRect(rect); + builder.restore(); + sk_sp display_list = builder.Build(); + ASSERT_EQ(display_list->bounds(), save_bounds); + } + + { + DisplayListBuilder builder(build_bounds); + builder.setImageFilter( + SkImageFilters::ColorFilter(alpha_color_filter, nullptr)); + builder.saveLayer(nullptr, true); + builder.setImageFilter(nullptr); + builder.drawRect(rect); + builder.restore(); + sk_sp display_list = builder.Build(); + ASSERT_EQ(display_list->bounds(), build_bounds); + } + + { + DisplayListBuilder builder(build_bounds); + builder.setBlendMode(SkBlendMode::kClear); + builder.saveLayer(&save_bounds, true); + builder.setBlendMode(SkBlendMode::kSrcOver); + builder.drawRect(rect); + builder.restore(); + sk_sp display_list = builder.Build(); + ASSERT_EQ(display_list->bounds(), save_bounds); + } + + { + DisplayListBuilder builder(build_bounds); + builder.setBlendMode(SkBlendMode::kClear); + builder.saveLayer(nullptr, true); + builder.setBlendMode(SkBlendMode::kSrcOver); + builder.drawRect(rect); + builder.restore(); + sk_sp display_list = builder.Build(); + ASSERT_EQ(display_list->bounds(), build_bounds); + } +} + } // namespace testing } // namespace flutter diff --git a/engine/src/flutter/flow/display_list_utils.cc b/engine/src/flutter/flow/display_list_utils.cc index 13480843984..be2d47cff18 100644 --- a/engine/src/flutter/flow/display_list_utils.cc +++ b/engine/src/flutter/flow/display_list_utils.cc @@ -175,27 +175,25 @@ void DisplayListBoundsCalculator::saveLayer(const SkRect* bounds, bool with_paint) { SkMatrixDispatchHelper::save(); ClipBoundsDispatchHelper::save(); - SaveInfo info = - with_paint ? SaveLayerWithPaintInfo(this, accumulator_, matrix(), paint()) - : SaveLayerInfo(accumulator_, matrix()); - saved_infos_.push_back(info); - accumulator_ = info.save(); + saved_infos_.emplace_back( + with_paint ? std::make_unique( + this, accumulator_, matrix(), bounds, paint()) + : std::make_unique(accumulator_, matrix())); + accumulator_ = saved_infos_.back()->save(); SkMatrixDispatchHelper::reset(); } void DisplayListBoundsCalculator::save() { SkMatrixDispatchHelper::save(); ClipBoundsDispatchHelper::save(); - SaveInfo info = SaveInfo(accumulator_); - saved_infos_.push_back(info); - accumulator_ = info.save(); + saved_infos_.emplace_back(std::make_unique(accumulator_)); + accumulator_ = saved_infos_.back()->save(); } void DisplayListBoundsCalculator::restore() { if (!saved_infos_.empty()) { SkMatrixDispatchHelper::restore(); ClipBoundsDispatchHelper::restore(); - SaveInfo info = saved_infos_.back(); + accumulator_ = saved_infos_.back()->restore(); saved_infos_.pop_back(); - accumulator_ = info.restore(); } } @@ -388,6 +386,7 @@ BoundsAccumulator* DisplayListBoundsCalculator::SaveLayerInfo::save() { } BoundsAccumulator* DisplayListBoundsCalculator::SaveLayerInfo::restore() { SkRect layer_bounds = layer_accumulator_.getBounds(); + layer_bounds.roundOut(&layer_bounds); matrix_.mapRect(&layer_bounds); saved_accumulator_->accumulate(layer_bounds); return saved_accumulator_; @@ -397,21 +396,118 @@ DisplayListBoundsCalculator::SaveLayerWithPaintInfo::SaveLayerWithPaintInfo( DisplayListBoundsCalculator* calculator, BoundsAccumulator* accumulator, const SkMatrix& saveMatrix, + const SkRect* saveBounds, const SkPaint& savePaint) : SaveLayerInfo(accumulator, saveMatrix), calculator_(calculator), - paint_(savePaint) {} + paint_(savePaint) { + if (saveBounds) { + bounds_.emplace(*saveBounds); + } +} + +static bool PaintNopsOnTransparenBlack(const SkPaint& paint) { + SkImageFilter* image_filter = paint.getImageFilter(); + // SkImageFilter::canComputeFastBounds tests for transparency behavior + // This test assumes that the blend mode checked down below will + // NOP on transparent black. + if (image_filter && !image_filter->canComputeFastBounds()) { + return false; + } + + SkColorFilter* color_filter = paint.getColorFilter(); + // We filter the transparent black that is used for the background of a + // saveLayer and make sure it returns transparent black. If it does, then + // the color filter will leave all area surrounding the contents of the + // 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 (color_filter && + color_filter->filterColor(SK_ColorTRANSPARENT) != SK_ColorTRANSPARENT) { + return false; + } + + const auto blend_mode = paint.asBlendMode(); + if (!blend_mode) { + return false; // can we query other blenders for this? + } + // Unusual blendmodes require us to process a saved layer + // even with operations outisde the clip. + // For example, DstIn is used by masking layers. + // https://code.google.com/p/skia/issues/detail?id=1291 + // https://crbug.com/401593 + switch (blend_mode.value()) { + // For each of the following transfer modes, if the source + // alpha is zero (our transparent black), the resulting + // blended pixel is not necessarily equal to the original + // destination pixel. + // Mathematically, any time in the following equations where + // the result is not d assuming source is 0 + case SkBlendMode::kClear: // r = 0 + case SkBlendMode::kSrc: // r = s + case SkBlendMode::kSrcIn: // r = s * da + case SkBlendMode::kDstIn: // r = d * sa + case SkBlendMode::kSrcOut: // r = s * (1-da) + case SkBlendMode::kDstATop: // r = d*sa + s*(1-da) + case SkBlendMode::kModulate: // r = s*d + return false; + break; + + // And in these equations, the result must be d if the + // source is 0 + case SkBlendMode::kDst: // r = d + case SkBlendMode::kSrcOver: // r = s + (1-sa)*d + case SkBlendMode::kDstOver: // r = d + (1-da)*s + case SkBlendMode::kDstOut: // r = d * (1-sa) + case SkBlendMode::kSrcATop: // r = s*da + d*(1-sa) + case SkBlendMode::kXor: // r = s*(1-da) + d*(1-sa) + case SkBlendMode::kPlus: // r = min(s + d, 1) + case SkBlendMode::kScreen: // r = s + d - s*d + case SkBlendMode::kOverlay: // multiply or screen, depending on dest + case SkBlendMode::kDarken: // rc = s + d - max(s*da, d*sa), + // ra = kSrcOver + case SkBlendMode::kLighten: // rc = s + d - min(s*da, d*sa), + // ra = kSrcOver + case SkBlendMode::kColorDodge: // brighten destination to reflect source + case SkBlendMode::kColorBurn: // darken destination to reflect source + case SkBlendMode::kHardLight: // multiply or screen, depending on source + case SkBlendMode::kSoftLight: // lighten or darken, depending on source + case SkBlendMode::kDifference: // rc = s + d - 2*(min(s*da, d*sa)), + // ra = kSrcOver + case SkBlendMode::kExclusion: // rc = s + d - two(s*d), ra = kSrcOver + case SkBlendMode::kMultiply: // r = s*(1-da) + d*(1-sa) + s*d + case SkBlendMode::kHue: // ra = kSrcOver + case SkBlendMode::kSaturation: // ra = kSrcOver + case SkBlendMode::kColor: // ra = kSrcOver + case SkBlendMode::kLuminosity: // ra = kSrcOver + return true; + break; + } +} BoundsAccumulator* DisplayListBoundsCalculator::SaveLayerWithPaintInfo::restore() { - SkRect layer_bounds = layer_accumulator_.getBounds(); - if (paint_.canComputeFastBounds()) { + SkRect layer_bounds; + if (paint_.canComputeFastBounds() && PaintNopsOnTransparenBlack(paint_)) { + // The ideal situation. The paint can compute the bounds AND the + // surrounding transparent pixels will not affect the destination. + layer_bounds = layer_accumulator_.getBounds(); layer_bounds = paint_.computeFastBounds(layer_bounds, &layer_bounds); - matrix_.mapRect(&layer_bounds); - saved_accumulator_->accumulate(layer_bounds); + } else if (bounds_.has_value()) { + // Bounds were provided by the save layer, the operation will affect + // all of those bounds. + layer_bounds = bounds_.value(); } else { + // Bounds were not provided for the save layer. We will fill to the + // cull bounds provided to the original DisplayList. calculator_->root_accumulator_.accumulate(calculator_->bounds_cull_); + // There is no need to process the layer bounds further as we just + // expanded bounds to the cull rect of the DisplayList. + return saved_accumulator_; } + layer_bounds.roundOut(&layer_bounds); + matrix_.mapRect(&layer_bounds); + saved_accumulator_->accumulate(layer_bounds); return saved_accumulator_; } diff --git a/engine/src/flutter/flow/display_list_utils.h b/engine/src/flutter/flow/display_list_utils.h index b950ebd00f9..af169604b2f 100644 --- a/engine/src/flutter/flow/display_list_utils.h +++ b/engine/src/flutter/flow/display_list_utils.h @@ -5,7 +5,11 @@ #ifndef FLUTTER_FLOW_DISPLAY_LIST_UTILS_H_ #define FLUTTER_FLOW_DISPLAY_LIST_UTILS_H_ +#include + #include "flutter/flow/display_list.h" +#include "flutter/fml/logging.h" +#include "flutter/fml/macros.h" #include "third_party/skia/include/core/SkMaskFilter.h" @@ -313,7 +317,10 @@ class DisplayListBoundsCalculator final bool occludes, SkScalar dpr) override; - SkRect getBounds() { return accumulator_->getBounds(); } + SkRect getBounds() { + FML_DCHECK(accumulator_ == &root_accumulator_); + return root_accumulator_.getBounds(); + } private: // current accumulator based on saveLayer history @@ -334,6 +341,9 @@ class DisplayListBoundsCalculator final protected: BoundsAccumulator* saved_accumulator_; + + private: + FML_DISALLOW_COPY_AND_ASSIGN(SaveInfo); }; class SaveLayerInfo : public SaveInfo { @@ -347,6 +357,9 @@ class DisplayListBoundsCalculator final protected: BoundsAccumulator layer_accumulator_; const SkMatrix matrix_; + + private: + FML_DISALLOW_COPY_AND_ASSIGN(SaveLayerInfo); }; class SaveLayerWithPaintInfo : public SaveLayerInfo { @@ -354,6 +367,7 @@ class DisplayListBoundsCalculator final SaveLayerWithPaintInfo(DisplayListBoundsCalculator* calculator, BoundsAccumulator* accumulator, const SkMatrix& save_matrix, + const SkRect* bounds, const SkPaint& save_paint); virtual ~SaveLayerWithPaintInfo() = default; @@ -362,10 +376,16 @@ class DisplayListBoundsCalculator final protected: DisplayListBoundsCalculator* calculator_; + std::optional bounds_; SkPaint paint_; + + private: + static constexpr SkRect kMissingBounds = SkRect::MakeWH(-1, -1); + + FML_DISALLOW_COPY_AND_ASSIGN(SaveLayerWithPaintInfo); }; - std::vector saved_infos_; + std::vector> saved_infos_; void accumulateRect(const SkRect& rect, bool force_stroke = false); };