From 6842d97557f7656d857fbf612e2bbd1135b372b4 Mon Sep 17 00:00:00 2001 From: Jim Graham Date: Sun, 13 Feb 2022 10:46:53 -0800 Subject: [PATCH] fix the bounds of nested save layers with a mix of clips and transforms (flutter/engine#31437) --- .../display_list/display_list_unittests.cc | 129 +++++++++++++++++- .../display_list/display_list_utils.cc | 88 ++++++++---- .../flutter/display_list/display_list_utils.h | 114 +++------------- 3 files changed, 210 insertions(+), 121 deletions(-) diff --git a/engine/src/flutter/display_list/display_list_unittests.cc b/engine/src/flutter/display_list/display_list_unittests.cc index 5a858e6631..6183470fb9 100644 --- a/engine/src/flutter/display_list/display_list_unittests.cc +++ b/engine/src/flutter/display_list/display_list_unittests.cc @@ -179,7 +179,8 @@ static constexpr SkIRect TestLatticeSrcRect = {1, 1, 39, 39}; static sk_sp MakeTestPicture(int w, int h, SkColor color) { SkPictureRecorder recorder; - SkCanvas* cv = recorder.beginRecording(TestBounds); + SkRTreeFactory rtree_factory; + SkCanvas* cv = recorder.beginRecording(TestBounds, &rtree_factory); SkPaint paint; paint.setColor(color); paint.setStyle(SkPaint::kFill_Style); @@ -1030,7 +1031,8 @@ TEST(DisplayList, DisplayListSaveLayerBoundsWithAlphaFilter) { // that modifies alpha channels (save layer bounds are meaningless // under those circumstances) SkPictureRecorder recorder; - SkCanvas* canvas = recorder.beginRecording(build_bounds); + SkRTreeFactory rtree_factory; + SkCanvas* canvas = recorder.beginRecording(build_bounds, &rtree_factory); SkPaint p1; p1.setColorFilter(alpha_color_filter); canvas->saveLayer(save_bounds, &p1); @@ -1826,5 +1828,128 @@ TEST(DisplayList, SaveLayerSrcBlendOnChildPreventsOpacityOptimization) { EXPECT_EQ(expector.save_layer_count(), 1); } +TEST(DisplayList, FlutterSvgIssue661BoundsWereEmpty) { + // See https://github.com/dnfield/flutter_svg/issues/661 + + SkPath path1; + path1.setFillType(SkPathFillType::kWinding); + path1.moveTo(25.54f, 37.52f); + path1.cubicTo(20.91f, 37.52f, 16.54f, 33.39f, 13.62f, 30.58f); + path1.lineTo(13, 30); + path1.lineTo(12.45f, 29.42f); + path1.cubicTo(8.39f, 25.15f, 1.61f, 18, 8.37f, 11.27f); + path1.cubicTo(10.18f, 9.46f, 12.37f, 9.58f, 14.49f, 11.58f); + path1.cubicTo(15.67f, 12.71f, 17.05f, 14.69f, 17.07f, 16.58f); + path1.cubicTo(17.0968f, 17.458f, 16.7603f, 18.3081f, 16.14f, 18.93f); + path1.cubicTo(15.8168f, 19.239f, 15.4653f, 19.5169f, 15.09f, 19.76f); + path1.cubicTo(14.27f, 20.33f, 14.21f, 20.44f, 14.27f, 20.62f); + path1.cubicTo(15.1672f, 22.3493f, 16.3239f, 23.9309f, 17.7f, 25.31f); + path1.cubicTo(19.0791f, 26.6861f, 20.6607f, 27.8428f, 22.39f, 28.74f); + path1.cubicTo(22.57f, 28.8f, 22.69f, 28.74f, 23.25f, 27.92f); + path1.cubicTo(23.5f, 27.566f, 23.778f, 27.231f, 24.08f, 26.92f); + path1.cubicTo(24.7045f, 26.3048f, 25.5538f, 25.9723f, 26.43f, 26); + path1.cubicTo(28.29f, 26, 30.27f, 27.4f, 31.43f, 28.58f); + path1.cubicTo(33.43f, 30.67f, 33.55f, 32.9f, 31.74f, 34.7f); + path1.cubicTo(30.1477f, 36.4508f, 27.906f, 37.4704f, 25.54f, 37.52f); + path1.close(); + path1.moveTo(11.17f, 12.23f); + path1.cubicTo(10.6946f, 12.2571f, 10.2522f, 12.4819f, 9.95f, 12.85f); + path1.cubicTo(5.12f, 17.67f, 8.95f, 22.5f, 14.05f, 27.85f); + path1.lineTo(14.62f, 28.45f); + path1.lineTo(15.16f, 28.96f); + path1.cubicTo(20.52f, 34.06f, 25.35f, 37.89f, 30.16f, 33.06f); + path1.cubicTo(30.83f, 32.39f, 31.25f, 31.56f, 29.81f, 30.06f); + path1.cubicTo(28.9247f, 29.07f, 27.7359f, 28.4018f, 26.43f, 28.16f); + path1.cubicTo(26.1476f, 28.1284f, 25.8676f, 28.2367f, 25.68f, 28.45f); + path1.cubicTo(25.4633f, 28.6774f, 25.269f, 28.9252f, 25.1f, 29.19f); + path1.cubicTo(24.53f, 30.01f, 23.47f, 31.54f, 21.54f, 30.79f); + path1.lineTo(21.41f, 30.72f); + path1.cubicTo(19.4601f, 29.7156f, 17.6787f, 28.4133f, 16.13f, 26.86f); + path1.cubicTo(14.5748f, 25.3106f, 13.2693f, 23.5295f, 12.26f, 21.58f); + path1.lineTo(12.2f, 21.44f); + path1.cubicTo(11.45f, 19.51f, 12.97f, 18.44f, 13.8f, 17.88f); + path1.cubicTo(14.061f, 17.706f, 14.308f, 17.512f, 14.54f, 17.3f); + path1.cubicTo(14.7379f, 17.1067f, 14.8404f, 16.8359f, 14.82f, 16.56f); + path1.cubicTo(14.5978f, 15.268f, 13.9585f, 14.0843f, 13, 13.19f); + path1.cubicTo(12.5398f, 12.642f, 11.8824f, 12.2971f, 11.17f, 12.23f); + path1.lineTo(11.17f, 12.23f); + path1.close(); + path1.moveTo(27, 19.34f); + path1.lineTo(24.74f, 19.34f); + path1.cubicTo(24.7319f, 18.758f, 24.262f, 18.2881f, 23.68f, 18.28f); + path1.lineTo(23.68f, 16.05f); + path1.lineTo(23.7f, 16.05f); + path1.cubicTo(25.5153f, 16.0582f, 26.9863f, 17.5248f, 27, 19.34f); + path1.lineTo(27, 19.34f); + path1.close(); + path1.moveTo(32.3f, 19.34f); + path1.lineTo(30.07f, 19.34f); + path1.cubicTo(30.037f, 15.859f, 27.171f, 13.011f, 23.69f, 13); + path1.lineTo(23.69f, 10.72f); + path1.cubicTo(28.415f, 10.725f, 32.3f, 14.615f, 32.3f, 19.34f); + path1.close(); + + SkPath path2; + path2.setFillType(SkPathFillType::kWinding); + path2.moveTo(37.5f, 19.33f); + path2.lineTo(35.27f, 19.33f); + path2.cubicTo(35.265f, 12.979f, 30.041f, 7.755f, 23.69f, 7.75f); + path2.lineTo(23.69f, 5.52f); + path2.cubicTo(31.264f, 5.525f, 37.495f, 11.756f, 37.5f, 19.33f); + path2.close(); + + DisplayListBuilder builder; + { + builder.save(); + builder.clipRect({0, 0, 100, 100}, SkClipOp::kIntersect, true); + { + builder.save(); + builder.transform2DAffine(2.17391, 0, -2547.83, // + 0, 2.04082, -500); + { + builder.save(); + builder.clipRect({1172, 245, 1218, 294}, SkClipOp::kIntersect, true); + { + builder.saveLayer(nullptr, SaveLayerOptions::kWithAttributes); + { + builder.save(); + builder.transform2DAffine(1.4375, 0, 1164.09, // + 0, 1.53125, 236.548); + builder.setAntiAlias(1); + builder.setColor(0xffffffff); + builder.drawPath(path1); + builder.restore(); + } + { + builder.save(); + builder.transform2DAffine(1.4375, 0, 1164.09, // + 0, 1.53125, 236.548); + builder.drawPath(path2); + builder.restore(); + } + builder.restore(); + } + builder.restore(); + } + builder.restore(); + } + builder.restore(); + } + sk_sp display_list = builder.Build(); + // Prior to the fix, the bounds were empty. + EXPECT_FALSE(display_list->bounds().isEmpty()); + // These are the expected bounds, but testing float values can be + // flaky wrt minor changes in the bounds calculations. If this + // line has to be revised too often as the DL implementation is + // improved and maintained, then we can eliminate this test and + // just rely on the "rounded out" bounds test that follows. + EXPECT_EQ(display_list->bounds(), + SkRect::MakeLTRB(0, 0.00189208984375, 99.9839630127, 100)); + // This is the more practical result. The bounds are "almost" 0,0,100x100 + EXPECT_EQ(display_list->bounds().roundOut(), SkIRect::MakeWH(100, 100)); + EXPECT_EQ(display_list->op_count(), 19u); + EXPECT_EQ(display_list->bytes(), sizeof(DisplayList) + 304u); +} + } // namespace testing } // namespace flutter diff --git a/engine/src/flutter/display_list/display_list_utils.cc b/engine/src/flutter/display_list/display_list_utils.cc index 9fac45b3e8..b219bb24bb 100644 --- a/engine/src/flutter/display_list/display_list_utils.cc +++ b/engine/src/flutter/display_list/display_list_utils.cc @@ -237,7 +237,7 @@ void ClipBoundsDispatchHelper::reset(const SkRect* cull_rect) { DisplayListBoundsCalculator::DisplayListBoundsCalculator( const SkRect* cull_rect) : ClipBoundsDispatchHelper(cull_rect) { - layer_infos_.emplace_back(std::make_unique()); + layer_infos_.emplace_back(std::make_unique(nullptr)); accumulator_ = layer_infos_.back()->layer_accumulator(); } void DisplayListBoundsCalculator::setStrokeCap(SkPaint::Cap cap) { @@ -284,7 +284,7 @@ void DisplayListBoundsCalculator::setMaskBlurFilter(SkBlurStyle style, void DisplayListBoundsCalculator::save() { SkMatrixDispatchHelper::save(); ClipBoundsDispatchHelper::save(); - layer_infos_.emplace_back(std::make_unique(accumulator_)); + layer_infos_.emplace_back(std::make_unique(accumulator_)); accumulator_ = layer_infos_.back()->layer_accumulator(); } void DisplayListBoundsCalculator::saveLayer(const SkRect* bounds, @@ -292,16 +292,27 @@ void DisplayListBoundsCalculator::saveLayer(const SkRect* bounds, SkMatrixDispatchHelper::save(); ClipBoundsDispatchHelper::save(); if (options.renders_with_attributes()) { - layer_infos_.emplace_back(std::make_unique( - accumulator_, image_filter_, paint_nops_on_transparency())); + // The actual flood of the outer layer clip will occur after the + // (eventual) corresponding restore is called, but rather than + // remember this information in the LayerInfo until the restore + // method is processed, we just mark the unbounded state up front. + if (!paint_nops_on_transparency()) { + // We will fill the clip of the outer layer when we restore + AccumulateUnbounded(); + } + + layer_infos_.emplace_back( + std::make_unique(accumulator_, image_filter_)); } else { layer_infos_.emplace_back( - std::make_unique(accumulator_, nullptr, true)); + std::make_unique(accumulator_, nullptr)); } + accumulator_ = layer_infos_.back()->layer_accumulator(); - // Accumulate the layer in its own coordinate system and then - // filter and transform its bounds on restore. - SkMatrixDispatchHelper::reset(); + + // 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 + // we set them as if a clip operation were performed. if (bounds) { clipRect(*bounds, SkClipOp::kIntersect, false); } @@ -310,24 +321,53 @@ void DisplayListBoundsCalculator::restore() { if (layer_infos_.size() > 1) { SkMatrixDispatchHelper::restore(); ClipBoundsDispatchHelper::restore(); - accumulator_ = layer_infos_.back()->restore_accumulator(); - SkRect layer_bounds = layer_infos_.back()->layer_bounds(); - // Must read unbounded state after layer_bounds - bool layer_unbounded = layer_infos_.back()->is_unbounded(); + + // Remember a few pieces of information from the current layer info + // for later processing. + LayerData* layer_info = layer_infos_.back().get(); + BoundsAccumulator* outer_accumulator = layer_info->restore_accumulator(); + bool is_unbounded = layer_info->is_unbounded(); + + // Before we pop_back we will get the current layer bounds from the + // current accumulator and adjust ot as required based on the filter. + SkRect layer_bounds = accumulator_->bounds(); + sk_sp filter = layer_info->filter(); + if (filter) { + if (filter->canComputeFastBounds()) { + SkIRect filter_bounds = + filter->filterBounds(layer_bounds.roundOut(), matrix(), + SkImageFilter::kForward_MapDirection); + layer_bounds.set(filter_bounds); + + // We could leave the clipping to the code below that will + // finally accumulate the layer bounds, but the bounds do + // not normally need clipping unless they were modified by + // entering this filtering code path. + if (has_clip() && !layer_bounds.intersect(clip_bounds())) { + layer_bounds.setEmpty(); + } + } else { + // If the filter cannot compute bounds then it might take an + // unbounded amount of space. This can sometimes happen if it + // modifies transparent black which means its affect will not + // be bounded by the transparent pixels outside of the layer + // drawable. + is_unbounded = true; + } + } + + // Restore the accumulator before popping the LayerInfo so that + // it nevers points to an out of scope instance. + accumulator_ = outer_accumulator; layer_infos_.pop_back(); - // We accumulate the bounds even if the layer was unbounded because - // the unbounded state may be contained at a higher level, so we at - // least accumulate our best estimate about what we have. - if (!layer_bounds.isEmpty()) { - // We do not use AccumulateOpBounds because the layer info already - // applied all bounds modifications based on the attributes that - // were in place when it was created. Modifying the bounds further - // based on the current attributes would mix attribute states. - // The bounds are still transformed and clipped by this method. - AccumulateBounds(layer_bounds); - } - if (layer_unbounded) { + // Finally accumulate the impact of the layer into the new scope. + // Note that the bounds were already accumulated in device pixels + // and clipped to any clips involved so we do not need to go + // through any transforms or clips to accuulate them into this + // layer. + accumulator_->accumulate(layer_bounds); + if (is_unbounded) { AccumulateUnbounded(); } } diff --git a/engine/src/flutter/display_list/display_list_utils.h b/engine/src/flutter/display_list/display_list_utils.h index e76b3d4b04..b88f9f7438 100644 --- a/engine/src/flutter/display_list/display_list_utils.h +++ b/engine/src/flutter/display_list/display_list_utils.h @@ -350,7 +350,7 @@ class BoundsAccumulator { } } void accumulate(const SkRect& r) { - if (r.fLeft <= r.fRight && r.fTop <= r.fBottom) { + if (r.fLeft < r.fRight && r.fTop < r.fBottom) { accumulate(r.fLeft, r.fTop); accumulate(r.fRight, r.fBottom); } @@ -495,41 +495,37 @@ class DisplayListBoundsCalculator final // current accumulator based on saveLayer history BoundsAccumulator* accumulator_; - // A class that abstracts the information kept for a single - // |save| or |saveLayer|, including the root information that - // is kept as a base set of information for the DisplayList - // at the initial conditions outside of any saveLayer. - // See |RootLayerData|, |SaveData| and |SaveLayerData|. + // A class that remembers the information kept for a single + // |save| or |saveLayer|. + // Each save or saveLayer will maintain its own bounds accumulator + // and then accumulate that back into the surrounding accumulator + // during restore. class LayerData { public: // Construct a LayerData to push on the save stack for a |save| // or |saveLayer| call. - // There does not tend to be an actual layer in the case of - // a |save| call, but in order to homogenize the handling - // of |restore| it adds a trivial implementation of this - // class to the stack of saves. // The |outer| parameter is the |BoundsAccumulator| that was // in use by the stream before this layer was pushed on the // stack and should be returned when this layer is popped off // the stack. - // Some layers may substitute their own accumulator to compute - // their own local bounds while they are on the stack. - explicit LayerData(BoundsAccumulator* outer) - : outer_(outer), is_unbounded_(false) {} - virtual ~LayerData() = default; + // Some saveLayer calls will process their bounds by an + // |SkImageFilter| when they are restored, but for most + // saveLayer (and all save) calls the filter will be null. + explicit LayerData(BoundsAccumulator* outer, + sk_sp filter = nullptr) + : outer_(outer), filter_(filter), is_unbounded_(false) {} + ~LayerData() = default; // The accumulator to use while this layer is put in play by // a |save| or |saveLayer| - virtual BoundsAccumulator* layer_accumulator() { return outer_; } + BoundsAccumulator* layer_accumulator() { return &layer_accumulator_; } // The accumulator to use after this layer is removed from play // via |restore| - virtual BoundsAccumulator* restore_accumulator() { return outer_; } + BoundsAccumulator* restore_accumulator() { return outer_; } - // The bounds of this layer. May be empty for cases like - // a non-layer |save| call which uses the |outer_| accumulator - // to accumulate draw calls inside of it - virtual SkRect layer_bounds() = 0; + // The filter to apply to the layer bounds when it is restored + sk_sp filter() { return filter_; } // is_unbounded should be set to true if we ever encounter an operation // on a layer that either is unrestricted (|drawColor| or |drawPaint|) @@ -562,86 +558,14 @@ class DisplayListBoundsCalculator final bool is_unbounded() const { return is_unbounded_; } private: + BoundsAccumulator layer_accumulator_; BoundsAccumulator* outer_; + sk_sp filter_; bool is_unbounded_; FML_DISALLOW_COPY_AND_ASSIGN(LayerData); }; - // An intermediate implementation class that handles keeping - // a local accumulator for the layer, used by both |RootLayerData| - // and |SaveLayerData|. - class AccumulatorLayerData : public LayerData { - public: - BoundsAccumulator* layer_accumulator() override { - return &layer_accumulator_; - } - - SkRect layer_bounds() override { - // Even though this layer might be unbounded, we still - // accumulate what bounds we have as the unbounded condition - // may be contained at a higher level and we at least want to - // account for the bounds that we do have. - return layer_accumulator_.bounds(); - } - - protected: - using LayerData::LayerData; - ~AccumulatorLayerData() = default; - - private: - BoundsAccumulator layer_accumulator_; - }; - - // Used as the initial default layer info for the Calculator. - class RootLayerData final : public AccumulatorLayerData { - public: - RootLayerData() : AccumulatorLayerData(nullptr) {} - ~RootLayerData() = default; - - private: - FML_DISALLOW_COPY_AND_ASSIGN(RootLayerData); - }; - - // Used for |save| - class SaveData final : public LayerData { - public: - using LayerData::LayerData; - ~SaveData() = default; - - SkRect layer_bounds() override { return SkRect::MakeEmpty(); } - - private: - FML_DISALLOW_COPY_AND_ASSIGN(SaveData); - }; - - // Used for |saveLayer| - class SaveLayerData final : public AccumulatorLayerData { - public: - SaveLayerData(BoundsAccumulator* outer, - sk_sp filter, - bool paint_nops_on_transparency) - : AccumulatorLayerData(outer), layer_filter_(std::move(filter)) { - if (!paint_nops_on_transparency) { - set_unbounded(); - } - } - ~SaveLayerData() = default; - - SkRect layer_bounds() override { - SkRect bounds = AccumulatorLayerData::layer_bounds(); - if (!ComputeFilteredBounds(bounds, layer_filter_.get())) { - set_unbounded(); - } - return bounds; - } - - private: - sk_sp layer_filter_; - - FML_DISALLOW_COPY_AND_ASSIGN(SaveLayerData); - }; - std::vector> layer_infos_; static constexpr SkScalar kMinStrokeWidth = 0.01;