From 615bccc579893524c43c2453995bd5ad92384316 Mon Sep 17 00:00:00 2001 From: Jim Graham Date: Thu, 2 Sep 2021 16:47:23 -0700 Subject: [PATCH 1/6] improve display list bounds calculations --- flow/display_list_canvas_unittests.cc | 3 ++ flow/display_list_utils.cc | 63 ++++++++++++++++----------- flow/display_list_utils.h | 23 +++++++++- 3 files changed, 63 insertions(+), 26 deletions(-) diff --git a/flow/display_list_canvas_unittests.cc b/flow/display_list_canvas_unittests.cc index 24e821ef1a918..13714db26b70b 100644 --- a/flow/display_list_canvas_unittests.cc +++ b/flow/display_list_canvas_unittests.cc @@ -723,6 +723,9 @@ class CanvasCompareTester { if (!dl_bounds.contains(ref_bounds)) { FML_LOG(ERROR) << "DisplayList bounds are too small!"; } + if (!ref_bounds.roundOut().contains(dl_bounds.roundOut())) { + FML_LOG(ERROR) << "###### DisplayList bounds are larger than reference!"; + } } #endif // DISPLAY_LIST_BOUNDS_ACCURACY_CHECKING diff --git a/flow/display_list_utils.cc b/flow/display_list_utils.cc index be2d47cff1820..9f6b6dd5a3215 100644 --- a/flow/display_list_utils.cc +++ b/flow/display_list_utils.cc @@ -210,25 +210,27 @@ void DisplayListBoundsCalculator::drawColor(SkColor color, SkBlendMode mode) { void DisplayListBoundsCalculator::drawLine(const SkPoint& p0, const SkPoint& p1) { SkRect bounds = SkRect::MakeLTRB(p0.fX, p0.fY, p1.fX, p1.fY).makeSorted(); - accumulateRect(bounds, true); + accumulateRect(bounds, kForceStroke | kIgnoreJoin); } void DisplayListBoundsCalculator::drawRect(const SkRect& rect) { - accumulateRect(rect); + // Right angle paths can ignore miter joins. + accumulateRect(rect, kIgnoreJoin | kIgnoreCap); } void DisplayListBoundsCalculator::drawOval(const SkRect& bounds) { - accumulateRect(bounds); + accumulateRect(bounds, kIgnoreJoin | kIgnoreCap); } void DisplayListBoundsCalculator::drawCircle(const SkPoint& center, SkScalar radius) { accumulateRect(SkRect::MakeLTRB(center.fX - radius, center.fY - radius, - center.fX + radius, center.fY + radius)); + center.fX + radius, center.fY + radius), + kIgnoreJoin | kIgnoreCap); } void DisplayListBoundsCalculator::drawRRect(const SkRRect& rrect) { - accumulateRect(rrect.getBounds()); + accumulateRect(rrect.getBounds(), kIgnoreJoin | kIgnoreCap); } void DisplayListBoundsCalculator::drawDRRect(const SkRRect& outer, const SkRRect& inner) { - accumulateRect(outer.getBounds()); + accumulateRect(outer.getBounds(), kIgnoreJoin | kIgnoreCap); } void DisplayListBoundsCalculator::drawPath(const SkPath& path) { accumulateRect(path.getBounds()); @@ -250,19 +252,23 @@ void DisplayListBoundsCalculator::drawPoints(SkCanvas::PointMode mode, for (size_t i = 0; i < count; i++) { ptBounds.accumulate(pts[i]); } - accumulateRect(ptBounds.getBounds(), true); + int flags = kForceStroke; + if (mode != SkCanvas::PointMode::kPolygon_PointMode) { + flags |= kIgnoreJoin; + } + accumulateRect(ptBounds.getBounds(), flags); } } void DisplayListBoundsCalculator::drawVertices(const sk_sp vertices, SkBlendMode mode) { - accumulateRect(vertices->bounds()); + accumulateRect(vertices->bounds(), kIgnoreStroke); } void DisplayListBoundsCalculator::drawImage(const sk_sp image, const SkPoint point, const SkSamplingOptions& sampling) { SkRect bounds = SkRect::Make(image->bounds()); bounds.offset(point); - accumulateRect(bounds); + accumulateRect(bounds, kIgnoreStroke); } void DisplayListBoundsCalculator::drawImageRect( const sk_sp image, @@ -270,13 +276,13 @@ void DisplayListBoundsCalculator::drawImageRect( const SkRect& dst, const SkSamplingOptions& sampling, SkCanvas::SrcRectConstraint constraint) { - accumulateRect(dst); + accumulateRect(dst, kIgnoreStroke); } void DisplayListBoundsCalculator::drawImageNine(const sk_sp image, const SkIRect& center, const SkRect& dst, SkFilterMode filter) { - accumulateRect(dst); + accumulateRect(dst, kIgnoreStroke); } void DisplayListBoundsCalculator::drawImageLattice( const sk_sp image, @@ -304,7 +310,7 @@ void DisplayListBoundsCalculator::drawAtlas(const sk_sp atlas, } } if (atlasBounds.isNotEmpty()) { - accumulateRect(atlasBounds.getBounds()); + accumulateRect(atlasBounds.getBounds(), kIgnoreStroke); } } void DisplayListBoundsCalculator::drawPicture(const sk_sp picture, @@ -318,7 +324,7 @@ void DisplayListBoundsCalculator::drawPicture(const sk_sp picture, pic_matrix->mapRect(&bounds); } if (with_save_layer) { - accumulateRect(bounds); + accumulateRect(bounds, kIgnoreStroke); } else { matrix().mapRect(&bounds); accumulator_->accumulate(bounds); @@ -326,7 +332,7 @@ void DisplayListBoundsCalculator::drawPicture(const sk_sp picture, } void DisplayListBoundsCalculator::drawDisplayList( const sk_sp display_list) { - accumulateRect(display_list->bounds()); + accumulateRect(display_list->bounds(), kIgnoreStroke); } void DisplayListBoundsCalculator::drawTextBlob(const sk_sp blob, SkScalar x, @@ -339,18 +345,28 @@ void DisplayListBoundsCalculator::drawShadow(const SkPath& path, bool occludes, SkScalar dpr) { accumulateRect( - PhysicalShapeLayer::ComputeShadowBounds(path, elevation, dpr, matrix())); + PhysicalShapeLayer::ComputeShadowBounds(path, elevation, dpr, matrix()), + kIgnoreStroke); } void DisplayListBoundsCalculator::accumulateRect(const SkRect& rect, - bool forceStroke) { + int flags) { SkRect dstRect = rect; - const SkPaint& p = paint(); - if (forceStroke) { - if (p.getStyle() == SkPaint::kFill_Style) { - setDrawStyle(SkPaint::kStroke_Style); - } else { - forceStroke = false; + SkPaint p = paint(); + if ((flags & kIgnoreStroke) != 0) { + FML_DCHECK((flags & kForceStroke) == 0); + p.setStyle(SkPaint::kFill_Style); + } else { + if ((flags & kForceStroke) != 0) { + p.setStyle(SkPaint::kStroke_Style); + } + if (p.getStyle() != SkPaint::kFill_Style) { + if ((flags & kIgnoreCap) != 0) { + p.setStrokeCap(SkPaint::kRound_Cap); + } + if ((flags & kIgnoreJoin) != 0) { + p.setStrokeJoin(SkPaint::kRound_Join); + } } } if (p.canComputeFastBounds()) { @@ -360,9 +376,6 @@ void DisplayListBoundsCalculator::accumulateRect(const SkRect& rect, } else { root_accumulator_.accumulate(bounds_cull_); } - if (forceStroke) { - setDrawStyle(SkPaint::kFill_Style); - } } DisplayListBoundsCalculator::SaveInfo::SaveInfo(BoundsAccumulator* accumulator) diff --git a/flow/display_list_utils.h b/flow/display_list_utils.h index af169604b2f14..54273343083d5 100644 --- a/flow/display_list_utils.h +++ b/flow/display_list_utils.h @@ -387,7 +387,28 @@ class DisplayListBoundsCalculator final std::vector> saved_infos_; - void accumulateRect(const SkRect& rect, bool force_stroke = false); + // Some operations are defined as stroking some geometry and + // need to always consider stroke attributes regardless of the + // setting of the drawing style in the paint. + static constexpr int kForceStroke = 1; + + // Some operations are defined as filling some geometry and + // should never consider stroke attributes regardless of the + // setting of the drawing style in the paint. + static constexpr int kIgnoreStroke = 2; + + // Some operations are defined as closed paths and will never + // have any segment end points which might be decorated with + // a square cap that expands the geometry. + static constexpr int kIgnoreCap = 4; + + // Some operations are defined as smooth paths or paths that + // never exceed 90 degrees at the corners. Such paths will + // never have a corner that extends outside of the box defined + // by (path_bounds + stroke_width/2). + static constexpr int kIgnoreJoin = 8; + + void accumulateRect(const SkRect& rect, int flags = 0); }; } // namespace flutter From 94d0ed28150d5066ade4ed4166e9c60ad161b568 Mon Sep 17 00:00:00 2001 From: Jim Graham Date: Wed, 8 Sep 2021 17:13:29 -0700 Subject: [PATCH 2/6] rework bounds calculations for better layer bounds management --- flow/display_list.cc | 2 +- flow/display_list.h | 4 +- flow/display_list_canvas_unittests.cc | 19 +- flow/display_list_unittests.cc | 37 ++- flow/display_list_utils.cc | 392 ++++++++++++++++---------- flow/display_list_utils.h | 291 ++++++++++++++----- 6 files changed, 511 insertions(+), 234 deletions(-) diff --git a/flow/display_list.cc b/flow/display_list.cc index 21f55692d326f..d1e06528bf734 100644 --- a/flow/display_list.cc +++ b/flow/display_list.cc @@ -823,7 +823,7 @@ DEFINE_DRAW_SHADOW_OP(ShadowOccludes, true) #pragma pack(pop, DLOp_Alignment) void DisplayList::ComputeBounds() { - DisplayListBoundsCalculator calculator(bounds_cull_); + DisplayListBoundsCalculator calculator(&bounds_cull_); Dispatch(calculator); bounds_ = calculator.getBounds(); } diff --git a/flow/display_list.h b/flow/display_list.h index d9d793c378abd..affd05b043b1a 100644 --- a/flow/display_list.h +++ b/flow/display_list.h @@ -245,8 +245,8 @@ class Dispatcher { virtual void setMaskBlurFilter(SkBlurStyle style, SkScalar sigma) = 0; virtual void save() = 0; - virtual void restore() = 0; virtual void saveLayer(const SkRect* bounds, bool restoreWithPaint) = 0; + virtual void restore() = 0; virtual void translate(SkScalar tx, SkScalar ty) = 0; virtual void scale(SkScalar sx, SkScalar sy) = 0; @@ -358,8 +358,8 @@ class DisplayListBuilder final : public virtual Dispatcher, public SkRefCnt { void setMaskBlurFilter(SkBlurStyle style, SkScalar sigma) override; void save() override; - void restore() override; void saveLayer(const SkRect* bounds, bool restoreWithPaint) override; + void restore() override; void translate(SkScalar tx, SkScalar ty) override; void scale(SkScalar sx, SkScalar sy) override; diff --git a/flow/display_list_canvas_unittests.cc b/flow/display_list_canvas_unittests.cc index 13714db26b70b..026302ec23d37 100644 --- a/flow/display_list_canvas_unittests.cc +++ b/flow/display_list_canvas_unittests.cc @@ -724,7 +724,7 @@ class CanvasCompareTester { FML_LOG(ERROR) << "DisplayList bounds are too small!"; } if (!ref_bounds.roundOut().contains(dl_bounds.roundOut())) { - FML_LOG(ERROR) << "###### DisplayList bounds are larger than reference!"; + FML_LOG(ERROR) << "###### DisplayList bounds larger than reference!"; } } #endif // DISPLAY_LIST_BOUNDS_ACCURACY_CHECKING @@ -806,12 +806,13 @@ class CanvasCompareTester { SkPMColor untouched = (bg) ? SkPreMultiplyColor(*bg) : 0; int pixels_touched = 0; int pixels_oob = 0; + SkIRect i_bounds = ref_bounds.roundOut(); for (int y = 0; y < TestHeight; y++) { const uint32_t* ref_row = ref_pixels->addr32(0, y); for (int x = 0; x < TestWidth; x++) { if (ref_row[x] != untouched) { pixels_touched++; - if (!ref_bounds.intersects(SkRect::MakeXYWH(x, y, 1, 1))) { + if (!i_bounds.contains(x, y)) { pixels_oob++; } } @@ -835,6 +836,8 @@ class CanvasCompareTester { ASSERT_EQ(test_pixels.width(), width) << info; ASSERT_EQ(test_pixels.height(), height) << info; ASSERT_EQ(test_pixels.info().bytesPerPixel(), 4) << info; + SkIRect i_bounds = + bounds ? bounds->roundOut() : SkIRect::MakeWH(width, height); int pixels_different = 0; int pixels_oob = 0; @@ -855,7 +858,7 @@ class CanvasCompareTester { maxX = x; if (maxY < y) maxY = y; - if (!bounds->intersects(SkRect::MakeXYWH(x, y, 1, 1))) { + if (!i_bounds.contains(x, y)) { pixels_oob++; } } @@ -869,6 +872,16 @@ class CanvasCompareTester { } } } + if (pixels_oob > 0) { + FML_LOG(ERROR) << "pix bounds[" // + << minX << ", " << minY << " => " << maxX << ", " << maxY + << "]"; + FML_LOG(ERROR) << "dl_bounds[" // + << bounds->fLeft << ", " << bounds->fTop // + << " => " // + << bounds->fRight << ", " << bounds->fBottom // + << "]"; + } #ifdef DISPLAY_LIST_BOUNDS_ACCURACY_CHECKING if (bounds && *bounds != SkRect::MakeLTRB(minX, minY, maxX + 1, maxY + 1)) { FML_LOG(ERROR) << "inaccurate bounds for " << info; diff --git a/flow/display_list_unittests.cc b/flow/display_list_unittests.cc index 0c56665c969bc..1572a866fa917 100644 --- a/flow/display_list_unittests.cc +++ b/flow/display_list_unittests.cc @@ -888,6 +888,7 @@ TEST(DisplayList, DisplayListSaveLayerBoundsWithAlphaFilter) { SkColorFilters::Matrix(alpha_matrix); { + // No tricky stuff, just verifying drawing a rect produces rect bounds DisplayListBuilder builder(build_bounds); builder.saveLayer(&save_bounds, true); builder.drawRect(rect); @@ -897,6 +898,7 @@ TEST(DisplayList, DisplayListSaveLayerBoundsWithAlphaFilter) { } { + // Now checking that a normal color filter still produces rect bounds DisplayListBuilder builder(build_bounds); builder.setColorFilter(base_color_filter); builder.saveLayer(&save_bounds, true); @@ -908,6 +910,26 @@ TEST(DisplayList, DisplayListSaveLayerBoundsWithAlphaFilter) { } { + // Now checking how SkPictureRecorder deals with a color filter + // that modifies alpha channels (save layer bounds are meaningless + // under those circumstances) + SkPictureRecorder recorder; + SkCanvas* canvas = recorder.beginRecording(build_bounds); + SkPaint p1; + p1.setColorFilter(alpha_color_filter); + canvas->saveLayer(save_bounds, &p1); + SkPaint p2; + canvas->drawRect(rect, p2); + canvas->restore(); + sk_sp picture = recorder.finishRecordingAsPicture(); + ASSERT_EQ(picture->cullRect(), build_bounds); + } + + { + // Now checking that DisplayList has the same behavior that we + // saw in the SkPictureRecorder example above - returning the + // cull rect of the DisplayListBuilder when it encounters a + // save layer that modifies an unbounded region DisplayListBuilder builder(build_bounds); builder.setColorFilter(alpha_color_filter); builder.saveLayer(&save_bounds, true); @@ -915,10 +937,12 @@ TEST(DisplayList, DisplayListSaveLayerBoundsWithAlphaFilter) { builder.drawRect(rect); builder.restore(); sk_sp display_list = builder.Build(); - ASSERT_EQ(display_list->bounds(), save_bounds); + ASSERT_EQ(display_list->bounds(), build_bounds); } { + // Verifying that the save layer bounds are not relevant + // to the behavior in the previous example DisplayListBuilder builder(build_bounds); builder.setColorFilter(alpha_color_filter); builder.saveLayer(nullptr, true); @@ -930,6 +954,8 @@ TEST(DisplayList, DisplayListSaveLayerBoundsWithAlphaFilter) { } { + // Making sure hiding a ColorFilter as an ImageFilter will + // generate the same behavior as setting it as a ColorFilter DisplayListBuilder builder(build_bounds); builder.setImageFilter( SkImageFilters::ColorFilter(base_color_filter, nullptr)); @@ -942,6 +968,8 @@ TEST(DisplayList, DisplayListSaveLayerBoundsWithAlphaFilter) { } { + // Making sure hiding a problematic ColorFilter as an ImageFilter + // will generate the same behavior as setting it as a ColorFilter DisplayListBuilder builder(build_bounds); builder.setImageFilter( SkImageFilters::ColorFilter(alpha_color_filter, nullptr)); @@ -950,10 +978,11 @@ TEST(DisplayList, DisplayListSaveLayerBoundsWithAlphaFilter) { builder.drawRect(rect); builder.restore(); sk_sp display_list = builder.Build(); - ASSERT_EQ(display_list->bounds(), save_bounds); + ASSERT_EQ(display_list->bounds(), build_bounds); } { + // Same as above (ImageFilter hiding ColorFilter) with no save bounds DisplayListBuilder builder(build_bounds); builder.setImageFilter( SkImageFilters::ColorFilter(alpha_color_filter, nullptr)); @@ -966,6 +995,7 @@ TEST(DisplayList, DisplayListSaveLayerBoundsWithAlphaFilter) { } { + // Testing behavior with an unboundable blend mode DisplayListBuilder builder(build_bounds); builder.setBlendMode(SkBlendMode::kClear); builder.saveLayer(&save_bounds, true); @@ -973,10 +1003,11 @@ TEST(DisplayList, DisplayListSaveLayerBoundsWithAlphaFilter) { builder.drawRect(rect); builder.restore(); sk_sp display_list = builder.Build(); - ASSERT_EQ(display_list->bounds(), save_bounds); + ASSERT_EQ(display_list->bounds(), build_bounds); } { + // Same as previous with no save bounds DisplayListBuilder builder(build_bounds); builder.setBlendMode(SkBlendMode::kClear); builder.saveLayer(nullptr, true); diff --git a/flow/display_list_utils.cc b/flow/display_list_utils.cc index 9f6b6dd5a3215..a9f6dc2bdae55 100644 --- a/flow/display_list_utils.cc +++ b/flow/display_list_utils.cc @@ -137,103 +137,204 @@ void SkMatrixDispatchHelper::reset() { } void ClipBoundsDispatchHelper::clipRect(const SkRect& rect, - bool isAA, + bool is_aa, SkClipOp clip_op) { if (clip_op == SkClipOp::kIntersect) { - intersect(rect); + intersect(rect, is_aa); } } void ClipBoundsDispatchHelper::clipRRect(const SkRRect& rrect, - bool isAA, + bool is_aa, SkClipOp clip_op) { if (clip_op == SkClipOp::kIntersect) { - intersect(rrect.getBounds()); + intersect(rrect.getBounds(), is_aa); } } void ClipBoundsDispatchHelper::clipPath(const SkPath& path, - bool isAA, + bool is_aa, SkClipOp clip_op) { if (clip_op == SkClipOp::kIntersect) { - intersect(path.getBounds()); + intersect(path.getBounds(), is_aa); } } -void ClipBoundsDispatchHelper::intersect(const SkRect& rect) { +void ClipBoundsDispatchHelper::intersect(const SkRect& rect, bool is_aa) { SkRect devClipBounds = matrix().mapRect(rect); - if (!bounds_.intersect(devClipBounds)) { - bounds_.setEmpty(); + if (is_aa) { + devClipBounds.roundOut(&devClipBounds); + } + if (has_clip_) { + if (!bounds_.intersect(devClipBounds)) { + bounds_.setEmpty(); + } + } else { + has_clip_ = true; + if (devClipBounds.isEmpty()) { + bounds_.setEmpty(); + } else { + bounds_ = devClipBounds; + } } } void ClipBoundsDispatchHelper::save() { - saved_.push_back(bounds_); + if (!has_clip_) { + saved_.push_back(SkRect::MakeLTRB(0, 0, -1, -1)); + } else if (bounds_.isEmpty()) { + saved_.push_back(SkRect::MakeEmpty()); + } else { + saved_.push_back(bounds_); + } } void ClipBoundsDispatchHelper::restore() { bounds_ = saved_.back(); saved_.pop_back(); + has_clip_ = (bounds_.fLeft <= bounds_.fRight && // + bounds_.fTop <= bounds_.fBottom); + if (!has_clip_) { + bounds_.setEmpty(); + } +} +void ClipBoundsDispatchHelper::reset(const SkRect* cull_rect) { + if ((has_clip_ = cull_rect)) { + bounds_ = *cull_rect; + } else { + bounds_.setEmpty(); + } } -void DisplayListBoundsCalculator::saveLayer(const SkRect* bounds, - bool with_paint) { +DisplayListBoundsCalculator::DisplayListBoundsCalculator( + const SkRect* cull_rect) + : ClipBoundsDispatchHelper(cull_rect) { + layer_infos_.emplace_back(std::make_unique()); + accumulator_ = layer_infos_.back()->accumulatorForLayer(); +} +void DisplayListBoundsCalculator::setCaps(SkPaint::Cap cap) { + cap_is_square_ = (cap == SkPaint::kSquare_Cap); +} +void DisplayListBoundsCalculator::setJoins(SkPaint::Join join) { + join_is_miter_ = (join == SkPaint::kMiter_Join); +} +void DisplayListBoundsCalculator::setDrawStyle(SkPaint::Style style) { + style_flag_ = (style == SkPaint::kFill_Style) ? kIsFilledGeometry // + : kIsStrokedGeometry; +} +void DisplayListBoundsCalculator::setStrokeWidth(SkScalar width) { + half_stroke_width_ = std::max(width * 0.5f, kMinStrokeWidth); +} +void DisplayListBoundsCalculator::setMiterLimit(SkScalar limit) { + miter_limit_ = std::max(limit, 1.0f); +} +void DisplayListBoundsCalculator::setBlendMode(SkBlendMode mode) { + blend_mode_ = mode; +} +void DisplayListBoundsCalculator::setBlender(sk_sp blender) { + SkPaint paint; + paint.setBlender(std::move(blender)); + blend_mode_ = paint.asBlendMode(); +} +void DisplayListBoundsCalculator::setImageFilter(sk_sp filter) { + image_filter_ = std::move(filter); +} +void DisplayListBoundsCalculator::setColorFilter(sk_sp filter) { + color_filter_ = std::move(filter); +} +void DisplayListBoundsCalculator::setPathEffect(sk_sp effect) { + path_effect_ = std::move(effect); +} +void DisplayListBoundsCalculator::setMaskFilter(sk_sp filter) { + mask_filter_ = std::move(filter); + mask_sigma_pad_ = 0.0f; +} +void DisplayListBoundsCalculator::setMaskBlurFilter(SkBlurStyle style, + SkScalar sigma) { + mask_sigma_pad_ = std::max(3.0f * sigma, 0.0f); + mask_filter_ = nullptr; +} +void DisplayListBoundsCalculator::save() { SkMatrixDispatchHelper::save(); ClipBoundsDispatchHelper::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(); + layer_infos_.emplace_back(std::make_unique(accumulator_)); + accumulator_ = layer_infos_.back()->accumulatorForLayer(); } -void DisplayListBoundsCalculator::save() { +void DisplayListBoundsCalculator::saveLayer(const SkRect* bounds, + bool with_paint) { SkMatrixDispatchHelper::save(); ClipBoundsDispatchHelper::save(); - saved_infos_.emplace_back(std::make_unique(accumulator_)); - accumulator_ = saved_infos_.back()->save(); + if (with_paint) { + layer_infos_.emplace_back(std::make_unique( + accumulator_, image_filter_, paintNopsOnTransparenBlack())); + } else { + layer_infos_.emplace_back( + std::make_unique(accumulator_, nullptr, true)); + } + accumulator_ = layer_infos_.back()->accumulatorForLayer(); + // Accumulate the layer in its own coordinate system and then + // filter and transform its bounds on restore. + SkMatrixDispatchHelper::reset(); } void DisplayListBoundsCalculator::restore() { - if (!saved_infos_.empty()) { + if (layer_infos_.size() > 1) { SkMatrixDispatchHelper::restore(); ClipBoundsDispatchHelper::restore(); - accumulator_ = saved_infos_.back()->restore(); - saved_infos_.pop_back(); + accumulator_ = layer_infos_.back()->accumulatorForRestore(); + SkRect layer_bounds = layer_infos_.back()->getLayerBounds(); + bool layer_flooded = layer_infos_.back()->is_flooded(); + layer_infos_.pop_back(); + + // We accumulate the bounds even if the layer was flooded because + // the flooding may become a NOP, so we at least accumulate what + // we have. + if (!layer_bounds.isEmpty()) { + accumulateRect(layer_bounds, kIsNonGeometric); + } + if (layer_flooded) { + accumulateFlood(); + } } } void DisplayListBoundsCalculator::drawPaint() { - if (!bounds_cull_.isEmpty()) { - root_accumulator_.accumulate(bounds_cull_); - } + accumulateFlood(); } void DisplayListBoundsCalculator::drawColor(SkColor color, SkBlendMode mode) { - if (!bounds_cull_.isEmpty()) { - root_accumulator_.accumulate(bounds_cull_); - } + accumulateFlood(); } void DisplayListBoundsCalculator::drawLine(const SkPoint& p0, const SkPoint& p1) { SkRect bounds = SkRect::MakeLTRB(p0.fX, p0.fY, p1.fX, p1.fY).makeSorted(); - accumulateRect(bounds, kForceStroke | kIgnoreJoin); + int cap_flag = kIsStrokedGeometry; + if (bounds.width() > 0.0f && bounds.height() > 0.0f) { + cap_flag |= kGeometryMayHaveDiagonalEndCaps; + } + accumulateRect(bounds, cap_flag); } void DisplayListBoundsCalculator::drawRect(const SkRect& rect) { - // Right angle paths can ignore miter joins. - accumulateRect(rect, kIgnoreJoin | kIgnoreCap); + accumulateRect(rect, kIsDrawnGeometry); } void DisplayListBoundsCalculator::drawOval(const SkRect& bounds) { - accumulateRect(bounds, kIgnoreJoin | kIgnoreCap); + accumulateRect(bounds, kIsDrawnGeometry); } void DisplayListBoundsCalculator::drawCircle(const SkPoint& center, SkScalar radius) { accumulateRect(SkRect::MakeLTRB(center.fX - radius, center.fY - radius, center.fX + radius, center.fY + radius), - kIgnoreJoin | kIgnoreCap); + kIsDrawnGeometry); } void DisplayListBoundsCalculator::drawRRect(const SkRRect& rrect) { - accumulateRect(rrect.getBounds(), kIgnoreJoin | kIgnoreCap); + accumulateRect(rrect.getBounds(), kIsDrawnGeometry); } void DisplayListBoundsCalculator::drawDRRect(const SkRRect& outer, const SkRRect& inner) { - accumulateRect(outer.getBounds(), kIgnoreJoin | kIgnoreCap); + accumulateRect(outer.getBounds(), kIsDrawnGeometry); } void DisplayListBoundsCalculator::drawPath(const SkPath& path) { - accumulateRect(path.getBounds()); + if (path.isInverseFillType()) { + accumulateFlood(); + } else { + accumulateRect(path.getBounds(), // + (kIsDrawnGeometry | // + kGeometryMayHaveDiagonalEndCaps | // + kGeometryMayHaveProblematicJoins)); + } } void DisplayListBoundsCalculator::drawArc(const SkRect& bounds, SkScalar start, @@ -242,7 +343,7 @@ void DisplayListBoundsCalculator::drawArc(const SkRect& bounds, // 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. - accumulateRect(bounds); + accumulateRect(bounds, kIsDrawnGeometry | kGeometryMayHaveDiagonalEndCaps); } void DisplayListBoundsCalculator::drawPoints(SkCanvas::PointMode mode, uint32_t count, @@ -252,23 +353,26 @@ void DisplayListBoundsCalculator::drawPoints(SkCanvas::PointMode mode, for (size_t i = 0; i < count; i++) { ptBounds.accumulate(pts[i]); } - int flags = kForceStroke; - if (mode != SkCanvas::PointMode::kPolygon_PointMode) { - flags |= kIgnoreJoin; + int flags = kIsStrokedGeometry; + if (mode != SkCanvas::kPoints_PointMode) { + flags |= kGeometryMayHaveDiagonalEndCaps; + if (mode == SkCanvas::PointMode::kPolygon_PointMode) { + flags |= kGeometryMayHaveProblematicJoins; + } } accumulateRect(ptBounds.getBounds(), flags); } } void DisplayListBoundsCalculator::drawVertices(const sk_sp vertices, SkBlendMode mode) { - accumulateRect(vertices->bounds(), kIgnoreStroke); + accumulateRect(vertices->bounds(), kIsFilledGeometry); } void DisplayListBoundsCalculator::drawImage(const sk_sp image, const SkPoint point, const SkSamplingOptions& sampling) { - SkRect bounds = SkRect::Make(image->bounds()); - bounds.offset(point); - accumulateRect(bounds, kIgnoreStroke); + SkRect bounds = SkRect::MakeXYWH(point.fX, point.fY, // + image->width(), image->height()); + accumulateRect(bounds, kIsNonGeometric | kApplyMaskFilter); } void DisplayListBoundsCalculator::drawImageRect( const sk_sp image, @@ -276,13 +380,13 @@ void DisplayListBoundsCalculator::drawImageRect( const SkRect& dst, const SkSamplingOptions& sampling, SkCanvas::SrcRectConstraint constraint) { - accumulateRect(dst, kIgnoreStroke); + accumulateRect(dst, kIsNonGeometric | kApplyMaskFilter); } void DisplayListBoundsCalculator::drawImageNine(const sk_sp image, const SkIRect& center, const SkRect& dst, SkFilterMode filter) { - accumulateRect(dst, kIgnoreStroke); + accumulateRect(dst, kIsNonGeometric); } void DisplayListBoundsCalculator::drawImageLattice( const sk_sp image, @@ -290,7 +394,7 @@ void DisplayListBoundsCalculator::drawImageLattice( const SkRect& dst, SkFilterMode filter, bool with_paint) { - accumulateRect(dst); + accumulateRect(dst, kIsNonGeometric | kApplyMaskFilter); } void DisplayListBoundsCalculator::drawAtlas(const sk_sp atlas, const SkRSXform xform[], @@ -310,7 +414,7 @@ void DisplayListBoundsCalculator::drawAtlas(const sk_sp atlas, } } if (atlasBounds.isNotEmpty()) { - accumulateRect(atlasBounds.getBounds(), kIgnoreStroke); + accumulateRect(atlasBounds.getBounds(), kIsNonGeometric); } } void DisplayListBoundsCalculator::drawPicture(const sk_sp picture, @@ -323,125 +427,133 @@ void DisplayListBoundsCalculator::drawPicture(const sk_sp picture, if (pic_matrix) { pic_matrix->mapRect(&bounds); } - if (with_save_layer) { - accumulateRect(bounds, kIgnoreStroke); - } else { - matrix().mapRect(&bounds); - accumulator_->accumulate(bounds); - } + accumulateRect(bounds, with_save_layer ? kIsFilledGeometry : kIsNonGeometric); } void DisplayListBoundsCalculator::drawDisplayList( const sk_sp display_list) { - accumulateRect(display_list->bounds(), kIgnoreStroke); + accumulateRect(display_list->bounds(), kIsUnfiltered); } void DisplayListBoundsCalculator::drawTextBlob(const sk_sp blob, SkScalar x, SkScalar y) { - accumulateRect(blob->bounds().makeOffset(x, y)); + accumulateRect(blob->bounds().makeOffset(x, y), kIsNonGeometric); } void DisplayListBoundsCalculator::drawShadow(const SkPath& path, const SkColor color, const SkScalar elevation, bool occludes, SkScalar dpr) { - accumulateRect( - PhysicalShapeLayer::ComputeShadowBounds(path, elevation, dpr, matrix()), - kIgnoreStroke); + SkRect shadow_bounds = + PhysicalShapeLayer::ComputeShadowBounds(path, elevation, dpr, matrix()); + accumulateRect(shadow_bounds, kIsUnfiltered); } -void DisplayListBoundsCalculator::accumulateRect(const SkRect& rect, - int flags) { - SkRect dstRect = rect; - SkPaint p = paint(); - if ((flags & kIgnoreStroke) != 0) { - FML_DCHECK((flags & kForceStroke) == 0); - p.setStyle(SkPaint::kFill_Style); - } else { - if ((flags & kForceStroke) != 0) { - p.setStyle(SkPaint::kStroke_Style); +bool DisplayListBoundsCalculator::adjustBoundsForPaint(SkRect& bounds, + int flags) { + if ((flags & kIsUnfiltered) != 0) { + FML_DCHECK(flags == kIsUnfiltered); + return true; + } + + if ((flags & kIsAnyGeometryMask) != 0) { + // Path effect occurs before stroking... + if (path_effect_) { + SkPaint p; + p.setPathEffect(path_effect_); + if (!p.canComputeFastBounds()) { + return false; + } + bounds = p.computeFastBounds(bounds, &bounds); } - if (p.getStyle() != SkPaint::kFill_Style) { - if ((flags & kIgnoreCap) != 0) { - p.setStrokeCap(SkPaint::kRound_Cap); + + if ((flags & kIsDrawnGeometry) != 0) { + FML_DCHECK((flags & (kIsFilledGeometry | kIsStrokedGeometry)) == 0); + flags |= style_flag_; + } + if ((flags & kIsStrokedGeometry) != 0) { + FML_DCHECK((flags & kIsFilledGeometry) == 0); + // Determine the max multiplier to the stroke width first. + SkScalar pad = 1.0f; + if (join_is_miter_ && (flags & kGeometryMayHaveProblematicJoins) != 0) { + pad = std::max(pad, miter_limit_); } - if ((flags & kIgnoreJoin) != 0) { - p.setStrokeJoin(SkPaint::kRound_Join); + if (cap_is_square_ && (flags & kGeometryMayHaveDiagonalEndCaps) != 0) { + pad = std::max(pad, SK_ScalarSqrt2); } + pad *= half_stroke_width_; + bounds.outset(pad, pad); + } else { + FML_DCHECK((flags & kIsStrokedGeometry) == 0); } - } - if (p.canComputeFastBounds()) { - dstRect = p.computeFastBounds(rect, &dstRect); - matrix().mapRect(&dstRect); - accumulator_->accumulate(dstRect); + flags |= kApplyMaskFilter; } else { - root_accumulator_.accumulate(bounds_cull_); + FML_DCHECK((flags & (kGeometryMayHaveDiagonalEndCaps | + kGeometryMayHaveProblematicJoins)) == 0); } -} -DisplayListBoundsCalculator::SaveInfo::SaveInfo(BoundsAccumulator* accumulator) - : saved_accumulator_(accumulator) {} -BoundsAccumulator* DisplayListBoundsCalculator::SaveInfo::save() { - // No need to swap out the accumulator for a normal save - return saved_accumulator_; -} -BoundsAccumulator* DisplayListBoundsCalculator::SaveInfo::restore() { - return saved_accumulator_; -} + if ((flags & kApplyMaskFilter) != 0) { + if (mask_filter_) { + SkPaint p; + p.setMaskFilter(mask_filter_); + if (!p.canComputeFastBounds()) { + return false; + } + bounds = p.computeFastBounds(bounds, &bounds); + } + if (mask_sigma_pad_ > 0.0f) { + bounds.outset(mask_sigma_pad_, mask_sigma_pad_); + } + } + + if (image_filter_) { + if (!image_filter_->canComputeFastBounds()) { + FML_LOG(ERROR) << "ImageFilter cannot compute bounds"; + return false; + } + bounds = image_filter_->computeFastBounds(bounds); + } -DisplayListBoundsCalculator::SaveLayerInfo::SaveLayerInfo( - BoundsAccumulator* accumulator, - const SkMatrix& matrix) - : SaveInfo(accumulator), matrix_(matrix) {} -BoundsAccumulator* DisplayListBoundsCalculator::SaveLayerInfo::save() { - // Use the local layerAccumulator until restore is called and - // then transform (and adjust with paint if necessary) on restore() - return &layer_accumulator_; -} -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_; + return true; } -DisplayListBoundsCalculator::SaveLayerWithPaintInfo::SaveLayerWithPaintInfo( - DisplayListBoundsCalculator* calculator, - BoundsAccumulator* accumulator, - const SkMatrix& saveMatrix, - const SkRect* saveBounds, - const SkPaint& savePaint) - : SaveLayerInfo(accumulator, saveMatrix), - calculator_(calculator), - paint_(savePaint) { - if (saveBounds) { - bounds_.emplace(*saveBounds); +void DisplayListBoundsCalculator::accumulateFlood() { + if (has_clip()) { + accumulator_->accumulate(getClipBounds()); + } else { + layer_infos_.back()->set_flooded(); + } +} +void DisplayListBoundsCalculator::accumulateRect(SkRect& rect, int flags) { + if (adjustBoundsForPaint(rect, flags)) { + matrix().mapRect(&rect); + if (!has_clip() || rect.intersect(getClipBounds())) { + accumulator_->accumulate(rect); + } + } else { + accumulateFlood(); } } -static bool PaintNopsOnTransparenBlack(const SkPaint& paint) { - SkImageFilter* image_filter = paint.getImageFilter(); +bool DisplayListBoundsCalculator::paintNopsOnTransparenBlack() { // 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()) { + 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) { + if (color_filter_ && + color_filter_->filterColor(SK_ColorTRANSPARENT) != SK_ColorTRANSPARENT) { return false; } - const auto blend_mode = paint.asBlendMode(); - if (!blend_mode) { + if (!blend_mode_) { return false; // can we query other blenders for this? } // Unusual blendmodes require us to process a saved layer @@ -449,7 +561,7 @@ static bool PaintNopsOnTransparenBlack(const SkPaint& paint) { // 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()) { + 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 @@ -498,30 +610,4 @@ static bool PaintNopsOnTransparenBlack(const SkPaint& paint) { } } -BoundsAccumulator* -DisplayListBoundsCalculator::SaveLayerWithPaintInfo::restore() { - 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); - } 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_; -} - } // namespace flutter diff --git a/flow/display_list_utils.h b/flow/display_list_utils.h index 54273343083d5..7a378edd232ec 100644 --- a/flow/display_list_utils.h +++ b/flow/display_list_utils.h @@ -190,20 +190,32 @@ class SkMatrixDispatchHelper : public virtual Dispatcher, class ClipBoundsDispatchHelper : public virtual Dispatcher, private virtual SkMatrixSource { public: - void clipRect(const SkRect& rect, bool isAA, SkClipOp clip_op) override; - void clipRRect(const SkRRect& rrect, bool isAA, SkClipOp clip_op) override; - void clipPath(const SkPath& path, bool isAA, SkClipOp clip_op) override; + ClipBoundsDispatchHelper() : ClipBoundsDispatchHelper(nullptr) {} + + ClipBoundsDispatchHelper(const SkRect* cull_rect) + : has_clip_(cull_rect), + bounds_(cull_rect && !cull_rect->isEmpty() ? *cull_rect + : SkRect::MakeEmpty()) {} + + void clipRect(const SkRect& rect, bool is_aa, SkClipOp clip_op) override; + void clipRRect(const SkRRect& rrect, bool is_aa, SkClipOp clip_op) override; + void clipPath(const SkPath& path, bool is_aa, SkClipOp clip_op) override; void save() override; void restore() override; - const SkRect& getCullingBounds() const { return bounds_; } + bool has_clip() const { return has_clip_; } + const SkRect& getClipBounds() const { return bounds_; } + + protected: + void reset(const SkRect* cull_rect); private: + bool has_clip_; SkRect bounds_; std::vector saved_; - void intersect(const SkRect& clipBounds); + void intersect(const SkRect& clipBounds, bool is_aa); }; class BoundsAccumulator { @@ -246,19 +258,36 @@ class BoundsAccumulator { // bounds of the rendering operations. class DisplayListBoundsCalculator final : public virtual Dispatcher, - public virtual SkPaintDispatchHelper, + public virtual IgnoreAttributeDispatchHelper, public virtual SkMatrixDispatchHelper, public virtual ClipBoundsDispatchHelper { public: // Construct a Calculator to determine the bounds of a list of // DisplayList dispatcher method calls. Since 2 of the method calls - // have no intrinsic size because they render to the entire available, - // the |cullRect| provides a bounds for them to include. - DisplayListBoundsCalculator(const SkRect& cull_rect = SkRect::MakeEmpty()) - : accumulator_(&root_accumulator_), bounds_cull_(cull_rect) {} + // have no intrinsic size because they flood the entire clip/surface, + // the |cull_rect| provides a bounds for them to include. If cull_rect + // is not specified or is null, then the flooding calls will not + // affect the resulting bounds, but will set the is_flooded flag + // below which can be inspected if an alternate plan is available + // for such cases. + // The flag should never be set if a cull_rect is provided. + DisplayListBoundsCalculator(const SkRect* cull_rect = nullptr); + + void setCaps(SkPaint::Cap cap) override; + void setJoins(SkPaint::Join join) override; + void setDrawStyle(SkPaint::Style style) override; + void setStrokeWidth(SkScalar width) override; + void setMiterLimit(SkScalar limit) override; + void setBlendMode(SkBlendMode mode) override; + void setBlender(sk_sp blender) override; + void setImageFilter(sk_sp filter) override; + void setColorFilter(sk_sp filter) override; + void setPathEffect(sk_sp effect) override; + void setMaskFilter(sk_sp filter) override; + void setMaskBlurFilter(SkBlurStyle style, SkScalar sigma) override; - void saveLayer(const SkRect* bounds, bool with_paint) override; void save() override; + void saveLayer(const SkRect* bounds, bool with_paint) override; void restore() override; void drawPaint() override; @@ -317,98 +346,216 @@ class DisplayListBoundsCalculator final bool occludes, SkScalar dpr) override; + bool isFlooded() { + FML_DCHECK(layer_infos_.size() == 1); + return layer_infos_.front()->is_flooded(); + } + SkRect getBounds() { - FML_DCHECK(accumulator_ == &root_accumulator_); - return root_accumulator_.getBounds(); + FML_DCHECK(layer_infos_.size() == 1); + if (isFlooded()) { + FML_LOG(INFO) << "returning partial bounds for flooded DisplayList"; + } + return accumulator_->getBounds(); } private: // current accumulator based on saveLayer history BoundsAccumulator* accumulator_; - // Only used for drawColor and drawPaint and paint objects that - // cannot support fast bounds. - SkRect bounds_cull_; - BoundsAccumulator root_accumulator_; - - class SaveInfo { + // 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|. + class LayerData { public: - SaveInfo(BoundsAccumulator* accumulator); - virtual ~SaveInfo() = default; - - virtual BoundsAccumulator* save(); - virtual BoundsAccumulator* restore(); - - protected: - BoundsAccumulator* saved_accumulator_; + LayerData(BoundsAccumulator* outer) : outer_(outer), is_flooded_(false) {} + virtual ~LayerData() = default; + + virtual BoundsAccumulator* accumulatorForLayer() { return outer_; } + virtual BoundsAccumulator* accumulatorForRestore() { return outer_; } + virtual SkRect getLayerBounds() { return SkRect::MakeEmpty(); } + + // is_flooded is to true if we ever encounter an operation + // on a layer that is unbounded (|drawColor| or |drawPaint|) + // or cannot compute its bounds (some effects and filters) + // and there was no outstanding clip op at the time. + // When the layer is restored, the outer layer may then + // process this flooded state by accumulating its own + // clip or recording the flood state to pass to its own + // outer layer. + // Typically the DisplayList will have been constructed with + // a cull rect which will act as a default clip for the + // outermost layer and the flood state of the overall + // DisplayList will never be true. + // + // SkPicture treats these same conditions as a Nop (they + // accumulate the SkPicture cull rect, but if it was not + // specified then it is an empty Rect and so has no + // effect on the bounds). + // If the Calculator object accumulates this flag into the + // root layer, then at least we can make the caller aware + // of that exceptional condition. + // + // Flutter is unlikely to ever run into this as the Dart + // mechanisms all supply a cull rect for all Dart Picture + // objects, even if that cull rect is kGiantRect. + void set_flooded() { is_flooded_ = true; } + bool is_flooded() { return is_flooded_; } private: - FML_DISALLOW_COPY_AND_ASSIGN(SaveInfo); + BoundsAccumulator* outer_; + bool is_flooded_; + + FML_DISALLOW_COPY_AND_ASSIGN(LayerData); }; - class SaveLayerInfo : public SaveInfo { + // An intermediate implementation class that handles keeping + // a local accumulator for the layer, used by both |RootLayerData| + // and |SaveLayerData|. + class AccumulatorLayerData : public LayerData { public: - SaveLayerInfo(BoundsAccumulator* accumulator, const SkMatrix& matrix); - virtual ~SaveLayerInfo() = default; + BoundsAccumulator* accumulatorForLayer() override { + return &layer_accumulator_; + } - BoundsAccumulator* save() override; - BoundsAccumulator* restore() override; + SkRect getLayerBounds() override { + // Even though this layer might have been flooded, we still + // accumulate what bounds we have as the flooded condition + // may be a nop at a higher level and we at least want to + // account for the bounds that we do have. + return layer_accumulator_.getBounds(); + } protected: - BoundsAccumulator layer_accumulator_; - const SkMatrix matrix_; + using LayerData::LayerData; + ~AccumulatorLayerData() = default; private: - FML_DISALLOW_COPY_AND_ASSIGN(SaveLayerInfo); + BoundsAccumulator layer_accumulator_; }; - class SaveLayerWithPaintInfo : public SaveLayerInfo { + // Used as the initial default layer info for the Calculator. + class RootLayerData : public AccumulatorLayerData { public: - SaveLayerWithPaintInfo(DisplayListBoundsCalculator* calculator, - BoundsAccumulator* accumulator, - const SkMatrix& save_matrix, - const SkRect* bounds, - const SkPaint& save_paint); - virtual ~SaveLayerWithPaintInfo() = default; + RootLayerData() : AccumulatorLayerData(nullptr) {} + ~RootLayerData() = default; - BoundsAccumulator* restore() override; - - protected: - DisplayListBoundsCalculator* calculator_; + private: + FML_DISALLOW_COPY_AND_ASSIGN(RootLayerData); + }; - std::optional bounds_; - SkPaint paint_; + // Used for |save| + class SaveData : public LayerData { + public: + using LayerData::LayerData; + ~SaveData() = default; private: - static constexpr SkRect kMissingBounds = SkRect::MakeWH(-1, -1); - - FML_DISALLOW_COPY_AND_ASSIGN(SaveLayerWithPaintInfo); + FML_DISALLOW_COPY_AND_ASSIGN(SaveData); }; - std::vector> saved_infos_; - - // Some operations are defined as stroking some geometry and - // need to always consider stroke attributes regardless of the - // setting of the drawing style in the paint. - static constexpr int kForceStroke = 1; + // Used for |saveLayer| + class SaveLayerData : 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_flooded(); + } + } + ~SaveLayerData() = default; - // Some operations are defined as filling some geometry and - // should never consider stroke attributes regardless of the - // setting of the drawing style in the paint. - static constexpr int kIgnoreStroke = 2; + private: + sk_sp layer_filter_; - // Some operations are defined as closed paths and will never - // have any segment end points which might be decorated with - // a square cap that expands the geometry. - static constexpr int kIgnoreCap = 4; + FML_DISALLOW_COPY_AND_ASSIGN(SaveLayerData); + }; - // Some operations are defined as smooth paths or paths that - // never exceed 90 degrees at the corners. Such paths will - // never have a corner that extends outside of the box defined - // by (path_bounds + stroke_width/2). - static constexpr int kIgnoreJoin = 8; + std::vector> layer_infos_; + + // A drawing operation that is not geometric in nature (but which + // may still apply a MaskFilter - see |kApplyMaskFilter| below). + static constexpr int kIsNonGeometric = 0x00; + + // A geometric operation that is defined as a fill operation + // regardless of what the current paint Style is set to. + // This flag will automatically assume |kApplyMaskFilter|. + static constexpr int kIsFilledGeometry = 0x01; + + // A geometric operation that is defined as a stroke operation + // regardless of what the current paint Style is set to. + // This flag will automatically assume |kApplyMaskFilter|. + static constexpr int kIsStrokedGeometry = 0x02; + + // A geometric operation that may be a stroke or fill operation + // depending on the current state of the paint Style attribute. + // This flag will automatically assume |kApplyMaskFilter|. + static constexpr int kIsDrawnGeometry = 0x04; + + static constexpr int kIsAnyGeometryMask = // + kIsFilledGeometry | // + kIsStrokedGeometry | // + kIsDrawnGeometry; + + // A geometric operation which has a path that might have + // end caps that are not rectilinear which means that square + // end caps might project further than half the stroke width + // from the geometry bounds. + // A rectilinear path such as |drawRect| will not have + // diagonal end caps. |drawLine| might have diagonal end + // caps depending on the angle of the line, and more likely + // |drawPath| will often have such end caps. + static constexpr int kGeometryMayHaveDiagonalEndCaps = 0x08; + + // A geometric operation which has joined vertices that are + // not guaranteed to be smooth (angles of incoming and outgoing) + // segments at some joins may not have the same angle) or + // rectilinear (squares have right angles at the corners, but + // those corners will never extend past the bounding box of + // the geometry pre-transform). + // |drawRect|, |drawOval| and |drawRRect| all have well + // behaved joins, but |drawPath| might have joins that cause + // mitered extensions outside the pre-transformed bounding box. + static constexpr int kGeometryMayHaveProblematicJoins = 0x10; + + // Some operations are inherently non-geometric and yet have the + // mask filter applied anyway. + // |drawImage| variants behave this way. + static constexpr int kApplyMaskFilter = 0x20; + + // In very rare circumstances the ImageFilter is ignored. + // This is one of the few flags that turns off a step in + // estimating the bounds, rather than turning on any steps. + static constexpr int kIsUnfiltered = 0x40; + + static constexpr SkScalar kMinStrokeWidth = 0.01; + + skstd::optional blend_mode_ = SkBlendMode::kSrcOver; + sk_sp color_filter_; - void accumulateRect(const SkRect& rect, int flags = 0); + SkScalar half_stroke_width_ = kMinStrokeWidth; + SkScalar miter_limit_ = 4.0; + int style_flag_ = kIsFilledGeometry; + bool join_is_miter_ = true; + bool cap_is_square_ = false; + sk_sp image_filter_; + sk_sp path_effect_; + sk_sp mask_filter_; + SkScalar mask_sigma_pad_ = 0.0; + + bool paintNopsOnTransparenBlack(); + bool adjustBoundsForPaint(SkRect& bounds, int flags); + + void accumulateFlood(); + void accumulateRect(const SkRect& rect, int flags) { + SkRect bounds = rect; + accumulateRect(bounds, flags); + } + void accumulateRect(SkRect& rect, int flags); }; } // namespace flutter From 73916964c7268098cd9ad73c5188a5f9d3dce27b Mon Sep 17 00:00:00 2001 From: Jim Graham Date: Wed, 8 Sep 2021 19:18:00 -0700 Subject: [PATCH 3/6] add SaveLayer tests to DL_canvas test suite and fix discovered bugs --- flow/display_list_canvas_unittests.cc | 89 ++++++++++++++++++++++++--- flow/display_list_utils.cc | 32 ++++++---- flow/display_list_utils.h | 14 +++++ 3 files changed, 116 insertions(+), 19 deletions(-) diff --git a/flow/display_list_canvas_unittests.cc b/flow/display_list_canvas_unittests.cc index 026302ec23d37..98f7dcd682160 100644 --- a/flow/display_list_canvas_unittests.cc +++ b/flow/display_list_canvas_unittests.cc @@ -116,13 +116,27 @@ class CanvasCompareTester { cv_renderer, dl_renderer, "Base Test"); RenderWithTransforms(cv_renderer, dl_renderer); RenderWithClips(cv_renderer, dl_renderer); + RenderWithSaveRestore(cv_renderer, dl_renderer); } static void RenderWithSaveRestore(CvRenderer& cv_renderer, DlRenderer& dl_renderer) { SkRect clip = SkRect::MakeLTRB(0, 0, 10, 10); SkRect rect = SkRect::MakeLTRB(5, 5, 15, 15); - SkColor save_layer_color = SkColorSetARGB(0x7f, 0x00, 0xff, 0xff); + SkColor alpha_layer_color = SkColorSetARGB(0x7f, 0x00, 0xff, 0xff); + SkColor default_color = SkPaint().getColor(); + CvRenderer cv_restored = [=](SkCanvas* cv, SkPaint& p) { + // Draw more than one primitive to disable peephole optimizations + cv->drawRect(RenderBounds.makeOutset(5, 5), p); + cv_renderer(cv, p); + cv->restore(); + }; + DlRenderer dl_restored = [=](DisplayListBuilder& b) { + // Draw more than one primitive to disable peephole optimizations + b.drawRect(RenderBounds.makeOutset(5, 5)); + dl_renderer(b); + b.restore(); + }; RenderWith( [=](SkCanvas* cv, SkPaint& p) { cv->save(); @@ -137,20 +151,81 @@ class CanvasCompareTester { b.restore(); }, cv_renderer, dl_renderer, "With prior save/clip/restore"); + RenderWith( + [=](SkCanvas* cv, SkPaint& p) { // + cv->saveLayer(nullptr, nullptr); + }, + [=](DisplayListBuilder& b) { // + b.saveLayer(nullptr, false); + }, + cv_restored, dl_restored, "saveLayer no bounds, no paint"); + RenderWith( + [=](SkCanvas* cv, SkPaint& p) { // + cv->saveLayer(RenderBounds, nullptr); + }, + [=](DisplayListBuilder& b) { // + b.saveLayer(&RenderBounds, false); + }, + cv_restored, dl_restored, "saveLayer with bounds, no paint"); RenderWith( [=](SkCanvas* cv, SkPaint& p) { SkPaint save_p; - save_p.setColor(save_layer_color); + save_p.setColor(alpha_layer_color); cv->saveLayer(RenderBounds, &save_p); - cv->drawRect(rect, p); }, [=](DisplayListBuilder& b) { - b.setColor(save_layer_color); + b.setColor(alpha_layer_color); b.saveLayer(&RenderBounds, true); - b.setColor(SkPaint().getColor()); - b.drawRect(rect); + b.setColor(default_color); }, - cv_renderer, dl_renderer, "With saveLayer"); + cv_restored, dl_restored, "saveLayer no bounds, with alpha"); + RenderWith( + [=](SkCanvas* cv, SkPaint& p) { + SkPaint save_p; + save_p.setColor(alpha_layer_color); + cv->saveLayer(RenderBounds, &save_p); + }, + [=](DisplayListBuilder& b) { + b.setColor(alpha_layer_color); + b.saveLayer(&RenderBounds, true); + b.setColor(default_color); + }, + cv_restored, dl_restored, "saveLayer bounds and alpha"); + + { + sk_sp filter = + SkImageFilters::Blur(5.0, 5.0, SkTileMode::kDecal, nullptr, nullptr); + { + RenderWith( + [=](SkCanvas* cv, SkPaint& p) { + SkPaint save_p; + save_p.setImageFilter(filter); + cv->saveLayer(nullptr, &save_p); + }, + [=](DisplayListBuilder& b) { + b.setImageFilter(filter); + b.saveLayer(nullptr, true); + b.setImageFilter(nullptr); + }, + cv_restored, dl_restored, "saveLayer ImageFilter, no bounds"); + } + ASSERT_TRUE(filter->unique()) << "SL with IF no bounds Cleanup"; + { + RenderWith( + [=](SkCanvas* cv, SkPaint& p) { + SkPaint save_p; + save_p.setImageFilter(filter); + cv->saveLayer(RenderBounds, &save_p); + }, + [=](DisplayListBuilder& b) { + b.setImageFilter(filter); + b.saveLayer(&RenderBounds, true); + b.setImageFilter(nullptr); + }, + cv_restored, dl_restored, "saveLayer ImageFilter and bounds"); + } + ASSERT_TRUE(filter->unique()) << "SL with IF and bounds Cleanup"; + } } static void RenderWithAttributes(CvRenderer& cv_renderer, diff --git a/flow/display_list_utils.cc b/flow/display_list_utils.cc index a9f6dc2bdae55..8ad62fc315e80 100644 --- a/flow/display_list_utils.cc +++ b/flow/display_list_utils.cc @@ -277,14 +277,19 @@ void DisplayListBoundsCalculator::restore() { ClipBoundsDispatchHelper::restore(); accumulator_ = layer_infos_.back()->accumulatorForRestore(); SkRect layer_bounds = layer_infos_.back()->getLayerBounds(); + // Must read flooded state after layer_bounds bool layer_flooded = layer_infos_.back()->is_flooded(); layer_infos_.pop_back(); // We accumulate the bounds even if the layer was flooded because - // the flooding may become a NOP, so we at least accumulate what - // we have. + // the flooding may become a NOP, so we at least accumulate our + // best estimate about what we have. if (!layer_bounds.isEmpty()) { - accumulateRect(layer_bounds, kIsNonGeometric); + // kUnfiltered because the layer already applied all bounds + // modifications based on the attributes that were in place + // when it was instantiated. Modifying it further base on the + // current attributes would mix attribute states. + accumulateRect(layer_bounds, kIsUnfiltered); } if (layer_flooded) { accumulateFlood(); @@ -448,6 +453,17 @@ void DisplayListBoundsCalculator::drawShadow(const SkPath& path, accumulateRect(shadow_bounds, kIsUnfiltered); } +bool DisplayListBoundsCalculator::getFilteredBounds(SkRect& bounds, + SkImageFilter* filter) { + if (filter) { + if (!filter->canComputeFastBounds()) { + return false; + } + bounds = filter->computeFastBounds(bounds); + } + return true; +} + bool DisplayListBoundsCalculator::adjustBoundsForPaint(SkRect& bounds, int flags) { if ((flags & kIsUnfiltered) != 0) { @@ -505,15 +521,7 @@ bool DisplayListBoundsCalculator::adjustBoundsForPaint(SkRect& bounds, } } - if (image_filter_) { - if (!image_filter_->canComputeFastBounds()) { - FML_LOG(ERROR) << "ImageFilter cannot compute bounds"; - return false; - } - bounds = image_filter_->computeFastBounds(bounds); - } - - return true; + return getFilteredBounds(bounds, image_filter_.get()); } void DisplayListBoundsCalculator::accumulateFlood() { diff --git a/flow/display_list_utils.h b/flow/display_list_utils.h index 7a378edd232ec..fa4cba9946a8c 100644 --- a/flow/display_list_utils.h +++ b/flow/display_list_utils.h @@ -402,6 +402,10 @@ class DisplayListBoundsCalculator final // mechanisms all supply a cull rect for all Dart Picture // objects, even if that cull rect is kGiantRect. void set_flooded() { is_flooded_ = true; } + + // |is_flooded| should be called after |getLayerBounds| in case + // a problem was found during the computation of those bounds, + // the layer will have one last chance to flag a flooded state. bool is_flooded() { return is_flooded_; } private: @@ -469,6 +473,14 @@ class DisplayListBoundsCalculator final } ~SaveLayerData() = default; + SkRect getLayerBounds() override { + SkRect bounds = AccumulatorLayerData::getLayerBounds(); + if (!getFilteredBounds(bounds, layer_filter_.get())) { + set_flooded(); + } + return bounds; + } + private: sk_sp layer_filter_; @@ -548,6 +560,8 @@ class DisplayListBoundsCalculator final SkScalar mask_sigma_pad_ = 0.0; bool paintNopsOnTransparenBlack(); + + static bool getFilteredBounds(SkRect& rect, SkImageFilter* filter); bool adjustBoundsForPaint(SkRect& bounds, int flags); void accumulateFlood(); From 66cc99d3c4afb7fc0b9b3a6c4431948c77498bb7 Mon Sep 17 00:00:00 2001 From: Jim Graham Date: Mon, 13 Sep 2021 15:47:09 -0700 Subject: [PATCH 4/6] first pass at improving corner cases of bounds testing --- flow/display_list_canvas_unittests.cc | 444 +++++++++++++++++++++----- flow/display_list_utils.cc | 30 +- 2 files changed, 382 insertions(+), 92 deletions(-) diff --git a/flow/display_list_canvas_unittests.cc b/flow/display_list_canvas_unittests.cc index 98f7dcd682160..7bbf1c5437f31 100644 --- a/flow/display_list_canvas_unittests.cc +++ b/flow/display_list_canvas_unittests.cc @@ -31,6 +31,8 @@ constexpr int TestWidth = 200; constexpr int TestHeight = 200; constexpr int RenderWidth = 100; constexpr int RenderHeight = 100; +constexpr int RenderHalfWidth = 50; +constexpr int RenderHalfHeight = 50; constexpr int RenderLeft = (TestWidth - RenderWidth) / 2; constexpr int RenderTop = (TestHeight - RenderHeight) / 2; constexpr int RenderRight = RenderLeft + RenderWidth; @@ -45,6 +47,84 @@ constexpr SkRect TestBounds = SkRect::MakeWH(TestWidth, TestHeight); constexpr SkRect RenderBounds = SkRect::MakeLTRB(RenderLeft, RenderTop, RenderRight, RenderBottom); +// The tests try 3 miter limit values, 0.0, 4.0 (the default), and 10.0 +// These values will allow us to construct a diamond that spans the +// width or height of the render box and still show the miter for 4.0 +// and 10.0 + +// The X offsets which will be used for tall vertical diamonds are +// expressed in terms of the rendering height to obtain the proper angle +constexpr SkScalar MiterExtremeDiamondOffsetX = RenderHeight * 0.04; +constexpr SkScalar Miter10DiamondOffsetX = RenderHeight * 0.06; +constexpr SkScalar Miter4DiamondOffsetX = RenderHeight * 0.14; + +// The Y offsets which will be used for long horizontal diamonds are +// expressed in terms of the rendering width to obtain the proper angle +constexpr SkScalar MiterExtremeDiamondOffsetY = RenderWidth * 0.04; +constexpr SkScalar Miter10DiamondOffsetY = RenderWidth * 0.06; +constexpr SkScalar Miter4DiamondOffsetY = RenderWidth * 0.14; + +// Render 3 vertical and horizontal diamonds each +// designed to break at the tested miter limits +// 0.0, 4.0 and 10.0 +constexpr SkScalar x_off_0 = RenderCenterX; +constexpr SkScalar x_off_l1 = RenderCenterX - Miter4DiamondOffsetX; +constexpr SkScalar x_off_l2 = x_off_l1 - Miter10DiamondOffsetX; +constexpr SkScalar x_off_l3 = x_off_l2 - Miter10DiamondOffsetX; +constexpr SkScalar x_off_r1 = RenderCenterX + Miter4DiamondOffsetX; +constexpr SkScalar x_off_r2 = x_off_r1 + MiterExtremeDiamondOffsetX; +constexpr SkScalar x_off_r3 = x_off_r2 + MiterExtremeDiamondOffsetX; +constexpr SkPoint VerticalMiterDiamondPoints[] = { + // Vertical diamonds: + // M10 M4 Mextreme + // /\ /|\ /\ top of RenderBounds + // / \ / | \ / \ to + // <----X--+--X----> RenderCenter + // \ / \ | / \ / to + // \/ \|/ \/ bottom of RenderBounds + SkPoint::Make(x_off_l3, RenderCenterY), + SkPoint::Make(x_off_l2, RenderTop), + SkPoint::Make(x_off_l1, RenderCenterY), + SkPoint::Make(x_off_0, RenderTop), + SkPoint::Make(x_off_r1, RenderCenterY), + SkPoint::Make(x_off_r2, RenderTop), + SkPoint::Make(x_off_r3, RenderCenterY), + SkPoint::Make(x_off_r2, RenderBottom), + SkPoint::Make(x_off_r1, RenderCenterY), + SkPoint::Make(x_off_0, RenderBottom), + SkPoint::Make(x_off_l1, RenderCenterY), + SkPoint::Make(x_off_l2, RenderBottom), + SkPoint::Make(x_off_l3, RenderCenterY), +}; +const int VerticalMiterDiamondPointCount = sizeof(VerticalMiterDiamondPoints) / sizeof(VerticalMiterDiamondPoints[0]); + +constexpr SkScalar y_off_0 = RenderCenterY; +constexpr SkScalar y_off_u1 = RenderCenterY - Miter4DiamondOffsetY; +constexpr SkScalar y_off_u2 = y_off_u1 - Miter10DiamondOffsetY; +constexpr SkScalar y_off_u3 = y_off_u2 - Miter10DiamondOffsetY; +constexpr SkScalar y_off_d1 = RenderCenterY + Miter4DiamondOffsetY; +constexpr SkScalar y_off_d2 = y_off_d1 + MiterExtremeDiamondOffsetY; +constexpr SkScalar y_off_d3 = y_off_d2 + MiterExtremeDiamondOffsetY; +const SkPoint HorizontalMiterDiamondPoints[] = { + // Horizontal diamonds + // Same configuration as Vertical diamonds above but + // rotated 90 degrees + SkPoint::Make(RenderCenterX, y_off_u3), + SkPoint::Make(RenderLeft, y_off_u2), + SkPoint::Make(RenderCenterX, y_off_u1), + SkPoint::Make(RenderLeft, y_off_0), + SkPoint::Make(RenderCenterX, y_off_d1), + SkPoint::Make(RenderLeft, y_off_d2), + SkPoint::Make(RenderCenterX, y_off_d3), + SkPoint::Make(RenderRight, y_off_d2), + SkPoint::Make(RenderCenterX, y_off_d1), + SkPoint::Make(RenderRight, y_off_0), + SkPoint::Make(RenderCenterX, y_off_u1), + SkPoint::Make(RenderRight, y_off_u2), + SkPoint::Make(RenderCenterX, y_off_u3), +}; +const int HorizontalMiterDiamondPointCount = sizeof(HorizontalMiterDiamondPoints) / sizeof(HorizontalMiterDiamondPoints[0]); + class CanvasCompareTester { private: // If a test is using any shadow operations then we cannot currently @@ -62,6 +142,13 @@ class CanvasCompareTester { // See: https://bugs.chromium.org/p/skia/issues/detail?id=12199 static bool TestingDrawAtlas; + // Bounds will never be completely tight for most operations, but we + // can establish an estimate for the amount of expected conservative + // overflow and only report bounds overflows above that factor. + // Expressed as a ratio of RenderWidth or RenderHeight. + static SkScalar NegligibleBoundsOverflowFactorX; + static SkScalar NegligibleBoundsOverflowFactorY; + public: typedef const std::function CvRenderer; typedef const std::function DlRenderer; @@ -72,10 +159,12 @@ class CanvasCompareTester { // But there are a couple of conditions beyond our control which require // the use of one of the variant methods below (|RenderShadows|, // |RenderVertices|, |RenderAtlas|). - static void RenderAll(CvRenderer& cv_renderer, DlRenderer& dl_renderer) { + static void RenderAll(CvRenderer& cv_renderer, DlRenderer& dl_renderer, + SkScalar overflow_factor_x = 0.10, + SkScalar overflow_factor_y = 0.10) { + RenderNoAttributes(cv_renderer, dl_renderer, overflow_factor_x, + overflow_factor_y); RenderWithAttributes(cv_renderer, dl_renderer); - RenderWithTransforms(cv_renderer, dl_renderer); - RenderWithClips(cv_renderer, dl_renderer); } // Used by the tests that render shadows to deal with a condition where @@ -110,7 +199,11 @@ class CanvasCompareTester { // call. Those tests could use |RenderAll| but there would be a lot of // wasted test runs that prepare an SkPaint that is never used. static void RenderNoAttributes(CvRenderer& cv_renderer, - DlRenderer& dl_renderer) { + DlRenderer& dl_renderer, + SkScalar overflow_factor_x = 0.10, + SkScalar overflow_factor_y = 0.10) { + NegligibleBoundsOverflowFactorX = overflow_factor_x; + NegligibleBoundsOverflowFactorY = overflow_factor_y; RenderWith([=](SkCanvas*, SkPaint& p) {}, // [=](DisplayListBuilder& d) {}, // cv_renderer, dl_renderer, "Base Test"); @@ -158,7 +251,7 @@ class CanvasCompareTester { [=](DisplayListBuilder& b) { // b.saveLayer(nullptr, false); }, - cv_restored, dl_restored, "saveLayer no bounds, no paint"); + cv_restored, dl_restored, "saveLayer no paint, no bounds"); RenderWith( [=](SkCanvas* cv, SkPaint& p) { // cv->saveLayer(RenderBounds, nullptr); @@ -166,19 +259,19 @@ class CanvasCompareTester { [=](DisplayListBuilder& b) { // b.saveLayer(&RenderBounds, false); }, - cv_restored, dl_restored, "saveLayer with bounds, no paint"); + cv_restored, dl_restored, "saveLayer no paint, with bounds"); RenderWith( [=](SkCanvas* cv, SkPaint& p) { SkPaint save_p; save_p.setColor(alpha_layer_color); - cv->saveLayer(RenderBounds, &save_p); + cv->saveLayer(nullptr, &save_p); }, [=](DisplayListBuilder& b) { b.setColor(alpha_layer_color); - b.saveLayer(&RenderBounds, true); + b.saveLayer(nullptr, true); b.setColor(default_color); }, - cv_restored, dl_restored, "saveLayer no bounds, with alpha"); + cv_restored, dl_restored, "saveLayer with alpha, no bounds"); RenderWith( [=](SkCanvas* cv, SkPaint& p) { SkPaint save_p; @@ -190,7 +283,7 @@ class CanvasCompareTester { b.saveLayer(&RenderBounds, true); b.setColor(default_color); }, - cv_restored, dl_restored, "saveLayer bounds and alpha"); + cv_restored, dl_restored, "saveLayer with alpha and bounds"); { sk_sp filter = @@ -308,17 +401,35 @@ class CanvasCompareTester { sk_sp filter = SkImageFilters::Blur(5.0, 5.0, SkTileMode::kDecal, nullptr, nullptr); { - RenderWith([=](SkCanvas*, SkPaint& p) { p.setImageFilter(filter); }, - [=](DisplayListBuilder& b) { b.setImageFilter(filter); }, - cv_renderer, dl_renderer, "ImageFilter == Decal Blur 5"); + RenderWith( + [=](SkCanvas*, SkPaint& p) { + // Provide some non-trivial stroke size to get blurred + p.setStrokeWidth(3.0); + p.setImageFilter(filter); + }, + [=](DisplayListBuilder& b) { + // Provide some non-trivial stroke size to get blurred + b.setStrokeWidth(3.0); + b.setImageFilter(filter); + }, + cv_renderer, dl_renderer, "ImageFilter == Decal Blur 5"); } ASSERT_TRUE(filter->unique()) << "ImageFilter Cleanup"; filter = SkImageFilters::Blur(5.0, 5.0, SkTileMode::kClamp, nullptr, nullptr); { - RenderWith([=](SkCanvas*, SkPaint& p) { p.setImageFilter(filter); }, - [=](DisplayListBuilder& b) { b.setImageFilter(filter); }, - cv_renderer, dl_renderer, "ImageFilter == Clamp Blur 5"); + RenderWith( + [=](SkCanvas*, SkPaint& p) { + // Provide some non-trivial stroke size to get blurred + p.setStrokeWidth(3.0); + p.setImageFilter(filter); + }, + [=](DisplayListBuilder& b) { + // Provide some non-trivial stroke size to get blurred + b.setStrokeWidth(3.0); + b.setImageFilter(filter); + }, + cv_renderer, dl_renderer, "ImageFilter == Clamp Blur 5"); } ASSERT_TRUE(filter->unique()) << "ImageFilter Cleanup"; } @@ -371,8 +482,6 @@ class CanvasCompareTester { } { - // Discrete path effects need a stroke width for drawPointsAsPoints - // to do something realistic sk_sp effect = SkDiscretePathEffect::Make(3, 5); { // Discrete path effects need a stroke width for drawPointsAsPoints @@ -413,11 +522,17 @@ class CanvasCompareTester { [=](SkCanvas*, SkPaint& p) { // Provide some non-trivial stroke size to get blurred p.setStrokeWidth(3.0); + // MaskFilters do not always render with Square or Butt caps + // See https://bugs.chromium.org/p/skia/issues/detail?id=12435 + p.setStrokeCap(SkPaint::kRound_Cap); p.setMaskFilter(filter); }, [=](DisplayListBuilder& b) { // Provide some non-trivial stroke size to get blurred b.setStrokeWidth(3.0); + // MaskFilters do not always render with Square or Butt caps + // See https://bugs.chromium.org/p/skia/issues/detail?id=12435 + b.setCaps(SkPaint::kRound_Cap); b.setMaskFilter(filter); }, cv_renderer, dl_renderer, "MaskFilter == Blur 5"); @@ -428,11 +543,17 @@ class CanvasCompareTester { [=](SkCanvas*, SkPaint& p) { // Provide some non-trivial stroke size to get blurred p.setStrokeWidth(3.0); + // MaskFilters do not always render with Square or Butt caps + // See https://bugs.chromium.org/p/skia/issues/detail?id=12435 + p.setStrokeCap(SkPaint::kRound_Cap); p.setMaskFilter(filter); }, [=](DisplayListBuilder& b) { // Provide some non-trivial stroke size to get blurred b.setStrokeWidth(3.0); + // MaskFilters do not always render with Square or Butt caps + // See https://bugs.chromium.org/p/skia/issues/detail?id=12435 + b.setCaps(SkPaint::kRound_Cap); b.setMaskBlurFilter(kNormal_SkBlurStyle, 5.0); }, cv_renderer, dl_renderer, "MaskFilter == Blur(Normal, 5.0)"); @@ -563,16 +684,16 @@ class CanvasCompareTester { [=](SkCanvas*, SkPaint& p) { p.setStyle(SkPaint::kStroke_Style); p.setStrokeWidth(5.0); - p.setStrokeMiter(100.0); + p.setStrokeMiter(10.0); p.setStrokeJoin(SkPaint::kMiter_Join); }, [=](DisplayListBuilder& b) { b.setDrawStyle(SkPaint::kStroke_Style); b.setStrokeWidth(5.0); - b.setMiterLimit(100.0); + b.setMiterLimit(10.0); b.setJoins(SkPaint::kMiter_Join); }, - cv_renderer, dl_renderer, "Stroke Width 5, Miter 100"); + cv_renderer, dl_renderer, "Stroke Width 5, Miter 10"); RenderWith( [=](SkCanvas*, SkPaint& p) { @@ -596,13 +717,31 @@ class CanvasCompareTester { { RenderWith( [=](SkCanvas*, SkPaint& p) { + // Need stroke style to see dashing properly p.setStyle(SkPaint::kStroke_Style); + // Provide some non-trivial stroke size to get dashed p.setStrokeWidth(5.0); + // Bounds code must conservatively assume a PathEffect may + // induce miter joins in geometry and adds a fairly large + // pad to the bounds as a result. Dashing doesn't create + // any new joins so that pessimal assumption in the bounds + // code isn't necessary here. We set a round join so that + // our bounds efficiency tests don't see a ridiculous pad. + p.setStrokeJoin(SkPaint::kRound_Join); p.setPathEffect(effect); }, [=](DisplayListBuilder& b) { + // Need stroke style to see dashing properly b.setDrawStyle(SkPaint::kStroke_Style); + // Provide some non-trivial stroke size to get dashed b.setStrokeWidth(5.0); + // Bounds code must conservatively assume a PathEffect may + // induce miter joins in geometry and adds a fairly large + // pad to the bounds as a result. Dashing doesn't create + // any new joins so that pessimal assumption in the bounds + // code isn't necessary here. We set a round join so that + // our bounds efficiency tests don't see a ridiculous pad. + b.setJoins(SkPaint::kRound_Join); b.setPathEffect(effect); }, cv_renderer, dl_renderer, "PathEffect == Dash-4-2"); @@ -614,11 +753,13 @@ class CanvasCompareTester { [=](SkCanvas*, SkPaint& p) { p.setStyle(SkPaint::kStroke_Style); p.setStrokeWidth(5.0); + p.setStrokeJoin(SkPaint::kRound_Join); p.setPathEffect(effect); }, [=](DisplayListBuilder& b) { b.setDrawStyle(SkPaint::kStroke_Style); b.setStrokeWidth(5.0); + b.setJoins(SkPaint::kRound_Join); b.setPathEffect(effect); }, cv_renderer, dl_renderer, "PathEffect == Dash-1-1.5"); @@ -642,7 +783,9 @@ class CanvasCompareTester { [=](DisplayListBuilder& b) { b.skew(0.05, 0.05); }, // cv_renderer, dl_renderer, "Skew 5%"); { - SkMatrix tx = SkMatrix::MakeAll(1.1, 0.1, 1.05, 0.05, 1, 1, 0, 0, 1); + SkMatrix tx = SkMatrix::MakeAll(1.10, 0.10, 5, + 0.05, 1.05, 10, + 0, 0, 1); RenderWith([=](SkCanvas* c, SkPaint&) { c->concat(tx); }, // [=](DisplayListBuilder& b) { b.transform2x3(tx[0], tx[1], tx[2], // @@ -651,7 +794,9 @@ class CanvasCompareTester { cv_renderer, dl_renderer, "Transform 2x3"); } { - SkMatrix tx = SkMatrix::MakeAll(1.1, 0.1, 1.05, 0.05, 1, 1, 0, 0, 1.01); + SkMatrix tx = SkMatrix::MakeAll(1.10, 0.10, 5, + 0.05, 1.05, 10, + 0, 0, 1.01); RenderWith([=](SkCanvas* c, SkPaint&) { c->concat(tx); }, // [=](DisplayListBuilder& b) { b.transform3x3(tx[0], tx[1], tx[2], // @@ -929,10 +1074,10 @@ class CanvasCompareTester { minX = x; if (minY > y) minY = y; - if (maxX < x) - maxX = x; - if (maxY < y) - maxY = y; + if (maxX <= x) + maxX = x + 1; + if (maxY <= y) + maxY = y + 1; if (!i_bounds.contains(x, y)) { pixels_oob++; } @@ -956,6 +1101,8 @@ class CanvasCompareTester { << " => " // << bounds->fRight << ", " << bounds->fBottom // << "]"; + } else if (bounds) { + showBoundsOverflow(info, i_bounds, minX, minY, maxX, maxY); } #ifdef DISPLAY_LIST_BOUNDS_ACCURACY_CHECKING if (bounds && *bounds != SkRect::MakeLTRB(minX, minY, maxX + 1, maxY + 1)) { @@ -970,6 +1117,37 @@ class CanvasCompareTester { ASSERT_EQ(pixels_different, 0) << info; } + static void showBoundsOverflow(std::string info, SkIRect& bounds, + int pixLeft, int pixTop, int pixRight, int pixBottom) { + int pad_left = std::max(0, pixLeft - bounds.fLeft); + int pad_top = std::max(0, pixTop - bounds.fTop); + int pad_right = std::max(0, bounds.fRight - pixRight); + int pad_bottom = std::max(0, bounds.fBottom - pixBottom); + int pixWidth = pixRight - pixLeft; + int pixHeight = pixBottom - pixTop; + int worst_pad_x = std::max(pad_left, pad_right); + int worst_pad_y = std::max(pad_top, pad_bottom); + if (worst_pad_x > std::ceil(pixWidth * NegligibleBoundsOverflowFactorX) || + worst_pad_y > std::ceil(pixHeight * NegligibleBoundsOverflowFactorY)) { + FML_LOG(ERROR) << "Overflow for " << info; + FML_LOG(ERROR) << "pix bounds[" // + << pixLeft << ", " << pixTop << " => " << pixRight << ", " << pixBottom + << "]"; + FML_LOG(ERROR) << "dl_bounds[" // + << bounds.fLeft << ", " << bounds.fTop // + << " => " // + << bounds.fRight << ", " << bounds.fBottom // + << "]"; + FML_LOG(ERROR) << "Bounds overflowed by up to " << worst_pad_x << ", " << worst_pad_y + << " (" << (worst_pad_x * 100.0 / pixWidth) + << "%, " << (worst_pad_y * 100.0 / pixHeight) << "%)"; + int pix_area = pixWidth * pixHeight; + int dl_area = bounds.width() * bounds.height(); + FML_LOG(ERROR) << "Total overflow area: " << (dl_area - pix_area) + << " (+" << (dl_area * 100.0 / pix_area - 100.0) << "%)"; + } + } + static sk_sp makeSurface(const SkColor* bg, int width = TestWidth, int height = TestHeight) { @@ -1003,8 +1181,8 @@ class CanvasCompareTester { } static sk_sp MakeTextBlob(std::string string, - SkScalar height = RenderHeight) { - SkFont font(SkTypeface::MakeDefault(), height); + SkScalar font_height) { + SkFont font(SkTypeface::MakeFromName("ahem", SkFontStyle::Normal()), font_height); return SkTextBlob::MakeFromText(string.c_str(), string.size(), font, SkTextEncoding::kUTF8); } @@ -1013,6 +1191,8 @@ class CanvasCompareTester { bool CanvasCompareTester::TestingDrawShadows = false; bool CanvasCompareTester::TestingDrawVertices = false; bool CanvasCompareTester::TestingDrawAtlas = false; +SkScalar CanvasCompareTester::NegligibleBoundsOverflowFactorX = 0.0; +SkScalar CanvasCompareTester::NegligibleBoundsOverflowFactorY = 0.0; const sk_sp CanvasCompareTester::testImage = CanvasCompareTester::makeTestImage(); @@ -1037,20 +1217,59 @@ TEST(DisplayListCanvas, DrawColor) { }); } -TEST(DisplayListCanvas, DrawLine) { - SkRect rect = RenderBounds; - SkPoint p1 = SkPoint::Make(rect.fLeft, rect.fTop); - SkPoint p2 = SkPoint::Make(rect.fRight, rect.fBottom); +TEST(DisplayListCanvas, DrawDiagonalLine) { + SkPoint p1 = SkPoint::Make(RenderLeft, RenderTop); + SkPoint p2 = SkPoint::Make(RenderRight, RenderBottom); CanvasCompareTester::RenderAll( [=](SkCanvas* canvas, SkPaint& paint) { // - canvas->drawLine(p1, p2, paint); + SkPaint p = paint; + p.setStyle(SkPaint::kStroke_Style); + canvas->drawLine(p1, p2, p); }, [=](DisplayListBuilder& builder) { // builder.drawLine(p1, p2); }); } +TEST(DisplayListCanvas, DrawHorizontalLine) { + SkPoint p1 = SkPoint::Make(RenderLeft, RenderCenterY); + SkPoint p2 = SkPoint::Make(RenderRight, RenderCenterY); + + CanvasCompareTester::RenderAll( + [=](SkCanvas* canvas, SkPaint& paint) { // + SkPaint p = paint; + p.setStyle(SkPaint::kStroke_Style); + canvas->drawLine(p1, p2, p); + }, + [=](DisplayListBuilder& builder) { // + builder.drawLine(p1, p2); + }, + // Small disturbances in conservative bounds estimation can have + // a large proportional effect on the bounds height for a + // horizontal line so we relax the vertical compliance a bit. + 0.10f, 0.20f); +} + +TEST(DisplayListCanvas, DrawVerticalLine) { + SkPoint p1 = SkPoint::Make(RenderCenterX, RenderTop); + SkPoint p2 = SkPoint::Make(RenderCenterY, RenderBottom); + + CanvasCompareTester::RenderAll( + [=](SkCanvas* canvas, SkPaint& paint) { // + SkPaint p = paint; + p.setStyle(SkPaint::kStroke_Style); + canvas->drawLine(p1, p2, p); + }, + [=](DisplayListBuilder& builder) { // + builder.drawLine(p1, p2); + }, + // Small disturbances in conservative bounds estimation can have + // a large proportional effect on the bounds width for a + // vertical line so we relax the horizontal compliance a bit. + 0.20f, 0.10f); +} + TEST(DisplayListCanvas, DrawRect) { CanvasCompareTester::RenderAll( [=](SkCanvas* canvas, SkPaint& paint) { // @@ -1117,6 +1336,17 @@ TEST(DisplayListCanvas, DrawPath) { path.lineTo(RenderLeft, RenderCenterY); path.lineTo(RenderRight, RenderCenterY); path.lineTo(RenderLeft, RenderBottom); + path.close(); + path.moveTo(VerticalMiterDiamondPoints[0]); + for (int i = 1; i < VerticalMiterDiamondPointCount; i++) { + path.lineTo(VerticalMiterDiamondPoints[i]); + } + path.close(); + path.moveTo(HorizontalMiterDiamondPoints[0]); + for (int i = 1; i < HorizontalMiterDiamondPointCount; i++) { + path.lineTo(HorizontalMiterDiamondPoints[i]); + } + path.close(); CanvasCompareTester::RenderAll( [=](SkCanvas* canvas, SkPaint& paint) { // canvas->drawPath(path, paint); @@ -1129,54 +1359,62 @@ TEST(DisplayListCanvas, DrawPath) { TEST(DisplayListCanvas, DrawArc) { CanvasCompareTester::RenderAll( [=](SkCanvas* canvas, SkPaint& paint) { // - canvas->drawArc(RenderBounds, 30, 270, false, paint); + canvas->drawArc(RenderBounds, 60, 330, false, paint); }, [=](DisplayListBuilder& builder) { // - builder.drawArc(RenderBounds, 30, 270, false); + builder.drawArc(RenderBounds, 60, 330, false); }); } TEST(DisplayListCanvas, DrawArcCenter) { CanvasCompareTester::RenderAll( [=](SkCanvas* canvas, SkPaint& paint) { // - canvas->drawArc(RenderBounds, 30, 270, true, paint); + canvas->drawArc(RenderBounds, 60, 330, true, paint); }, [=](DisplayListBuilder& builder) { // - builder.drawArc(RenderBounds, 30, 270, true); + builder.drawArc(RenderBounds, 60, 330, true); }); } TEST(DisplayListCanvas, DrawPointsAsPoints) { + // The +/- 16 points are designed to fall just inside the clips + // that are tested against so we avoid lots of undrawn pixels + // in the accumulated bounds. const SkScalar x0 = RenderLeft; - const SkScalar x1 = (RenderLeft + RenderCenterX) * 0.5; - const SkScalar x2 = RenderCenterX; - const SkScalar x3 = (RenderRight + RenderCenterX) * 0.5; - const SkScalar x4 = RenderRight; + const SkScalar x1 = RenderLeft + 16; + const SkScalar x2 = (RenderLeft + RenderCenterX) * 0.5; + const SkScalar x3 = RenderCenterX; + const SkScalar x4 = (RenderRight + RenderCenterX) * 0.5; + const SkScalar x5 = RenderRight - 16; + const SkScalar x6 = RenderRight; const SkScalar y0 = RenderTop; - const SkScalar y1 = (RenderTop + RenderCenterY) * 0.5; - const SkScalar y2 = RenderCenterY; - const SkScalar y3 = (RenderBottom + RenderCenterY) * 0.5; - const SkScalar y4 = RenderBottom; + const SkScalar y1 = RenderTop + 16; + const SkScalar y2 = (RenderTop + RenderCenterY) * 0.5; + const SkScalar y3 = RenderCenterY; + const SkScalar y4 = (RenderBottom + RenderCenterY) * 0.5; + const SkScalar y5 = RenderBottom - 16; + const SkScalar y6 = RenderBottom; // clang-format off const SkPoint points[] = { - {x0, y0}, {x1, y0}, {x2, y0}, {x3, y0}, {x4, y0}, - {x0, y1}, {x1, y1}, {x2, y1}, {x3, y1}, {x4, y1}, - {x0, y2}, {x1, y2}, {x2, y2}, {x3, y2}, {x4, y2}, - {x0, y3}, {x1, y3}, {x2, y3}, {x3, y3}, {x4, y3}, - {x0, y4}, {x1, y4}, {x2, y4}, {x3, y4}, {x4, y4}, + {x0, y0}, {x1, y0}, {x2, y0}, {x3, y0}, {x4, y0}, {x5, y0}, {x6, y0}, + {x0, y1}, {x1, y1}, {x2, y1}, {x3, y1}, {x4, y1}, {x5, y1}, {x6, y1}, + {x0, y2}, {x1, y2}, {x2, y2}, {x3, y2}, {x4, y2}, {x5, y2}, {x6, y2}, + {x0, y3}, {x1, y3}, {x2, y3}, {x3, y3}, {x4, y3}, {x5, y3}, {x6, y3}, + {x0, y4}, {x1, y4}, {x2, y4}, {x3, y4}, {x4, y4}, {x5, y4}, {x6, y4}, + {x0, y5}, {x1, y5}, {x2, y5}, {x3, y5}, {x4, y5}, {x5, y5}, {x6, y5}, + {x0, y6}, {x1, y6}, {x2, y6}, {x3, y6}, {x4, y6}, {x5, y6}, {x6, y6}, }; // clang-format on - const int count = sizeof(points) / sizeof(points[0]); + CanvasCompareTester::RenderAll( [=](SkCanvas* canvas, SkPaint& paint) { // - SkPaint p = paint; - p.setStyle(SkPaint::kStroke_Style); - canvas->drawPoints(SkCanvas::kPoints_PointMode, count, points, p); + canvas->drawPoints(SkCanvas::kPoints_PointMode, count, points, paint); }, [=](DisplayListBuilder& builder) { // + // builder.setCaps(SkPaint::kSquare_Cap); builder.drawPoints(SkCanvas::kPoints_PointMode, count, points); }); } @@ -1213,9 +1451,7 @@ TEST(DisplayListCanvas, DrawPointsAsLines) { ASSERT_TRUE((count & 1) == 0); CanvasCompareTester::RenderAll( [=](SkCanvas* canvas, SkPaint& paint) { // - SkPaint p = paint; - p.setStyle(SkPaint::kStroke_Style); - canvas->drawPoints(SkCanvas::kLines_PointMode, count, points, p); + canvas->drawPoints(SkCanvas::kLines_PointMode, count, points, paint); }, [=](DisplayListBuilder& builder) { // builder.drawPoints(SkCanvas::kLines_PointMode, count, points); @@ -1223,33 +1459,50 @@ TEST(DisplayListCanvas, DrawPointsAsLines) { } TEST(DisplayListCanvas, DrawPointsAsPolygon) { - const SkPoint points[] = { + const SkPoint points1[] = { + // RenderCorner hourglass SkPoint::Make(RenderLeft, RenderTop), SkPoint::Make(RenderRight, RenderBottom), SkPoint::Make(RenderRight, RenderTop), SkPoint::Make(RenderLeft, RenderBottom), SkPoint::Make(RenderLeft, RenderTop), }; + const int count1 = sizeof(points1) / sizeof(points1[0]); + CanvasCompareTester::RenderAll( [=](SkCanvas* canvas, SkPaint& paint) { // - SkPaint p = paint; - p.setStyle(SkPaint::kStroke_Style); - canvas->drawPoints(SkCanvas::kPolygon_PointMode, 4, points, p); + paint.setStyle(SkPaint::kStroke_Style); + canvas->drawPoints(SkCanvas::kPolygon_PointMode, count1, points1, paint); + canvas->drawPoints(SkCanvas::kPolygon_PointMode, VerticalMiterDiamondPointCount, VerticalMiterDiamondPoints, paint); + canvas->drawPoints(SkCanvas::kPolygon_PointMode, HorizontalMiterDiamondPointCount, HorizontalMiterDiamondPoints, paint); }, [=](DisplayListBuilder& builder) { // - builder.drawPoints(SkCanvas::kPolygon_PointMode, 4, points); + builder.setDrawStyle(SkPaint::kStroke_Style); + builder.drawPoints(SkCanvas::kPolygon_PointMode, count1, points1); + builder.drawPoints(SkCanvas::kPolygon_PointMode, VerticalMiterDiamondPointCount, VerticalMiterDiamondPoints); + builder.drawPoints(SkCanvas::kPolygon_PointMode, HorizontalMiterDiamondPointCount, HorizontalMiterDiamondPoints); }); } TEST(DisplayListCanvas, DrawVerticesWithColors) { - const SkPoint pts[3] = { + const SkPoint pts[6] = { SkPoint::Make(RenderCenterX, RenderTop), SkPoint::Make(RenderLeft, RenderBottom), SkPoint::Make(RenderRight, RenderBottom), + SkPoint::Make(RenderCenterX, RenderBottom), + SkPoint::Make(RenderLeft, RenderTop), + SkPoint::Make(RenderRight, RenderTop), + }; + const SkColor colors[6] = { + SK_ColorRED, + SK_ColorBLUE, + SK_ColorGREEN, + SK_ColorCYAN, + SK_ColorYELLOW, + SK_ColorMAGENTA, }; - const SkColor colors[3] = {SK_ColorRED, SK_ColorBLUE, SK_ColorGREEN}; const sk_sp vertices = SkVertices::MakeCopy( - SkVertices::kTriangles_VertexMode, 3, pts, nullptr, colors); + SkVertices::kTriangles_VertexMode, 6, pts, nullptr, colors); CanvasCompareTester::RenderVertices( [=](SkCanvas* canvas, SkPaint& paint) { // canvas->drawVertices(vertices.get(), SkBlendMode::kSrcOver, paint); @@ -1261,18 +1514,24 @@ TEST(DisplayListCanvas, DrawVerticesWithColors) { } TEST(DisplayListCanvas, DrawVerticesWithImage) { - const SkPoint pts[3] = { + const SkPoint pts[6] = { SkPoint::Make(RenderCenterX, RenderTop), SkPoint::Make(RenderLeft, RenderBottom), SkPoint::Make(RenderRight, RenderBottom), + SkPoint::Make(RenderCenterX, RenderBottom), + SkPoint::Make(RenderLeft, RenderTop), + SkPoint::Make(RenderRight, RenderTop), }; - const SkPoint tex[3] = { + const SkPoint tex[6] = { SkPoint::Make(RenderWidth / 2.0, 0), SkPoint::Make(0, RenderHeight), SkPoint::Make(RenderWidth, RenderHeight), + SkPoint::Make(RenderWidth / 2, RenderHeight), + SkPoint::Make(0, 0), + SkPoint::Make(RenderWidth, 0), }; const sk_sp vertices = SkVertices::MakeCopy( - SkVertices::kTriangles_VertexMode, 3, pts, tex, nullptr); + SkVertices::kTriangles_VertexMode, 6, pts, tex, nullptr); const sk_sp shader = CanvasCompareTester::testImage->makeShader( SkTileMode::kRepeat, SkTileMode::kRepeat, SkSamplingOptions()); CanvasCompareTester::RenderVertices( @@ -1426,26 +1685,32 @@ TEST(DisplayListCanvas, DrawImageLatticeLinear) { TEST(DisplayListCanvas, DrawAtlasNearest) { const SkRSXform xform[] = { - {0.5, 0, RenderLeft, RenderRight}, - {0, 0.5, RenderCenterX, RenderCenterY}, + { 1.2f, 0.0f, RenderLeft, RenderTop}, + { 0.0f, 1.2f, RenderRight, RenderTop}, + {-1.2f, 0.0f, RenderRight, RenderBottom}, + { 0.0f, -1.2f, RenderLeft, RenderBottom}, }; const SkRect tex[] = { - {0, 0, RenderWidth * 0.5, RenderHeight * 0.5}, - {RenderWidth * 0.5, RenderHeight * 0.5, RenderWidth, RenderHeight}, + SkRect::MakeXYWH(0, 0, RenderHalfWidth, RenderHalfHeight), + SkRect::MakeXYWH(RenderHalfWidth, 0, RenderHalfWidth, RenderHalfHeight), + SkRect::MakeXYWH(RenderHalfWidth, RenderHalfHeight, RenderHalfWidth, RenderHalfHeight), + SkRect::MakeXYWH(0, RenderHalfHeight, RenderHalfWidth, RenderHalfHeight), }; const SkColor colors[] = { SK_ColorBLUE, SK_ColorGREEN, + SK_ColorYELLOW, + SK_ColorMAGENTA, }; const sk_sp image = CanvasCompareTester::testImage; CanvasCompareTester::RenderAtlas( [=](SkCanvas* canvas, SkPaint& paint) { - canvas->drawAtlas(image.get(), xform, tex, colors, 2, + canvas->drawAtlas(image.get(), xform, tex, colors, 4, SkBlendMode::kSrcOver, DisplayList::NearestSampling, nullptr, &paint); }, [=](DisplayListBuilder& builder) { - builder.drawAtlas(image, xform, tex, colors, 2, // + builder.drawAtlas(image, xform, tex, colors, 4, // SkBlendMode::kSrcOver, DisplayList::NearestSampling, nullptr); }); @@ -1453,16 +1718,22 @@ TEST(DisplayListCanvas, DrawAtlasNearest) { TEST(DisplayListCanvas, DrawAtlasLinear) { const SkRSXform xform[] = { - {0.5, 0, RenderLeft, RenderRight}, - {0, 0.5, RenderCenterX, RenderCenterY}, + { 1.2f, 0.0f, RenderLeft, RenderTop}, + { 0.0f, 1.2f, RenderRight, RenderTop}, + {-1.2f, 0.0f, RenderRight, RenderBottom}, + { 0.0f, -1.2f, RenderLeft, RenderBottom}, }; const SkRect tex[] = { - {0, 0, RenderWidth * 0.5, RenderHeight * 0.5}, - {RenderWidth * 0.5, RenderHeight * 0.5, RenderWidth, RenderHeight}, + SkRect::MakeXYWH(0, 0, RenderHalfWidth, RenderHalfHeight), + SkRect::MakeXYWH(RenderHalfWidth, 0, RenderHalfWidth, RenderHalfHeight), + SkRect::MakeXYWH(RenderHalfWidth, RenderHalfHeight, RenderHalfWidth, RenderHalfHeight), + SkRect::MakeXYWH(0, RenderHalfHeight, RenderHalfWidth, RenderHalfHeight), }; const SkColor colors[] = { SK_ColorBLUE, SK_ColorGREEN, + SK_ColorYELLOW, + SK_ColorMAGENTA, }; const sk_sp image = CanvasCompareTester::testImage; CanvasCompareTester::RenderAtlas( @@ -1574,14 +1845,25 @@ TEST(DisplayListCanvas, DrawTextBlob) { #if defined(OS_FUCHSIA) GTEST_SKIP() << "Rendering comparisons require a valid default font manager"; #endif // OS_FUCHSIA - sk_sp blob = CanvasCompareTester::MakeTextBlob("Test Blob"); + sk_sp blob = + CanvasCompareTester::MakeTextBlob("Test#Blob", RenderHeight * 0.33f); + FML_LOG(ERROR) << "text blob bounds: " + << blob->bounds().fLeft << ", " << blob->bounds().fTop << " => " + << blob->bounds().fRight << ", " << blob->bounds().fBottom; + SkScalar RenderY1_3 = RenderTop + RenderHeight * 0.33; + SkScalar RenderY2_3 = RenderTop + RenderHeight * 0.66; CanvasCompareTester::RenderNoAttributes( [=](SkCanvas* canvas, SkPaint& paint) { // + canvas->drawTextBlob(blob, RenderLeft, RenderY1_3, paint); + canvas->drawTextBlob(blob, RenderLeft, RenderY2_3, paint); canvas->drawTextBlob(blob, RenderLeft, RenderBottom, paint); }, [=](DisplayListBuilder& builder) { // + builder.drawTextBlob(blob, RenderLeft, RenderY1_3); + builder.drawTextBlob(blob, RenderLeft, RenderY2_3); builder.drawTextBlob(blob, RenderLeft, RenderBottom); - }); + }, + 0.25, 0.20); } TEST(DisplayListCanvas, DrawShadow) { diff --git a/flow/display_list_utils.cc b/flow/display_list_utils.cc index 8ad62fc315e80..ad90db7b40aa9 100644 --- a/flow/display_list_utils.cc +++ b/flow/display_list_utils.cc @@ -194,7 +194,7 @@ void ClipBoundsDispatchHelper::restore() { } } void ClipBoundsDispatchHelper::reset(const SkRect* cull_rect) { - if ((has_clip_ = cull_rect)) { + if ((has_clip_ = ((bool) cull_rect))) { bounds_ = *cull_rect; } else { bounds_.setEmpty(); @@ -270,6 +270,7 @@ void DisplayListBoundsCalculator::saveLayer(const SkRect* bounds, // Accumulate the layer in its own coordinate system and then // filter and transform its bounds on restore. SkMatrixDispatchHelper::reset(); + ClipBoundsDispatchHelper::reset(bounds); } void DisplayListBoundsCalculator::restore() { if (layer_infos_.size() > 1) { @@ -361,16 +362,14 @@ void DisplayListBoundsCalculator::drawPoints(SkCanvas::PointMode mode, int flags = kIsStrokedGeometry; if (mode != SkCanvas::kPoints_PointMode) { flags |= kGeometryMayHaveDiagonalEndCaps; - if (mode == SkCanvas::PointMode::kPolygon_PointMode) { - flags |= kGeometryMayHaveProblematicJoins; - } + // Even Polygon mode just draws (count-1) separate lines, no joins } accumulateRect(ptBounds.getBounds(), flags); } } void DisplayListBoundsCalculator::drawVertices(const sk_sp vertices, SkBlendMode mode) { - accumulateRect(vertices->bounds(), kIsFilledGeometry); + accumulateRect(vertices->bounds(), kIsNonGeometric); } void DisplayListBoundsCalculator::drawImage(const sk_sp image, const SkPoint point, @@ -411,13 +410,19 @@ void DisplayListBoundsCalculator::drawAtlas(const sk_sp atlas, const SkRect* cullRect) { SkPoint quad[4]; BoundsAccumulator atlasBounds; + // FML_LOG(ERROR) << "atlas quads: {"; for (int i = 0; i < count; i++) { const SkRect& src = tex[i]; + // FML_LOG(ERROR) << " rect " << (i + 1) << ": " << src.fLeft << ", " << src.fTop << " => " << src.fRight << ", " << src.fBottom; xform[i].toQuad(src.width(), src.height(), quad); + // FML_LOG(ERROR) << " quad " << (i + 1) << ": {"; for (int j = 0; j < 4; j++) { + // FML_LOG(ERROR) << " " << quad[j].fX << ", " << quad[j].fY; atlasBounds.accumulate(quad[j]); } + // FML_LOG(ERROR) << " }"; } + // FML_LOG(ERROR) << "}"; if (atlasBounds.isNotEmpty()) { accumulateRect(atlasBounds.getBounds(), kIsNonGeometric); } @@ -432,7 +437,7 @@ void DisplayListBoundsCalculator::drawPicture(const sk_sp picture, if (pic_matrix) { pic_matrix->mapRect(&bounds); } - accumulateRect(bounds, with_save_layer ? kIsFilledGeometry : kIsNonGeometric); + accumulateRect(bounds, with_save_layer ? kIsNonGeometric : kIsUnfiltered); } void DisplayListBoundsCalculator::drawDisplayList( const sk_sp display_list) { @@ -441,7 +446,7 @@ void DisplayListBoundsCalculator::drawDisplayList( void DisplayListBoundsCalculator::drawTextBlob(const sk_sp blob, SkScalar x, SkScalar y) { - accumulateRect(blob->bounds().makeOffset(x, y), kIsNonGeometric); + accumulateRect(blob->bounds().makeOffset(x, y), kIsFilledGeometry); } void DisplayListBoundsCalculator::drawShadow(const SkPath& path, const SkColor color, @@ -472,6 +477,11 @@ bool DisplayListBoundsCalculator::adjustBoundsForPaint(SkRect& bounds, } if ((flags & kIsAnyGeometryMask) != 0) { + if ((flags & kIsDrawnGeometry) != 0) { + FML_DCHECK((flags & (kIsFilledGeometry | kIsStrokedGeometry)) == 0); + flags |= style_flag_; + } + // Path effect occurs before stroking... if (path_effect_) { SkPaint p; @@ -480,12 +490,10 @@ bool DisplayListBoundsCalculator::adjustBoundsForPaint(SkRect& bounds, return false; } bounds = p.computeFastBounds(bounds, &bounds); + flags |= kGeometryMayHaveDiagonalEndCaps | + kGeometryMayHaveProblematicJoins; } - if ((flags & kIsDrawnGeometry) != 0) { - FML_DCHECK((flags & (kIsFilledGeometry | kIsStrokedGeometry)) == 0); - flags |= style_flag_; - } if ((flags & kIsStrokedGeometry) != 0) { FML_DCHECK((flags & kIsFilledGeometry) == 0); // Determine the max multiplier to the stroke width first. From 957e54369fc950e6ea4ff0c9d7644aee0c3d117b Mon Sep 17 00:00:00 2001 From: Jim Graham Date: Wed, 15 Sep 2021 21:40:11 -0700 Subject: [PATCH 5/6] major updates to bounds testing and a minor improvement in dashing bounds --- flow/display_list.cc | 44 +- flow/display_list.h | 6 +- flow/display_list_canvas.cc | 4 +- flow/display_list_canvas.h | 2 +- flow/display_list_canvas_unittests.cc | 886 ++++++++++++++++++-------- flow/display_list_utils.cc | 35 +- flow/display_list_utils.h | 2 +- 7 files changed, 661 insertions(+), 318 deletions(-) diff --git a/flow/display_list.cc b/flow/display_list.cc index d1e06528bf734..9c64d757855e0 100644 --- a/flow/display_list.cc +++ b/flow/display_list.cc @@ -797,27 +797,27 @@ struct DrawTextBlobOp final : DLOp { }; // 4 byte header + 28 byte payload packs evenly into 32 bytes -#define DEFINE_DRAW_SHADOW_OP(name, occludes) \ - struct Draw##name##Op final : DLOp { \ - static const auto kType = DisplayListOpType::kDraw##name; \ - \ - Draw##name##Op(const SkPath& path, \ - SkColor color, \ - SkScalar elevation, \ - SkScalar dpr) \ - : color(color), elevation(elevation), dpr(dpr), path(path) {} \ - \ - const SkColor color; \ - const SkScalar elevation; \ - const SkScalar dpr; \ - const SkPath path; \ - \ - void dispatch(Dispatcher& dispatcher) const { \ - dispatcher.drawShadow(path, color, elevation, occludes, dpr); \ - } \ +#define DEFINE_DRAW_SHADOW_OP(name, nonoccluding) \ + struct Draw##name##Op final : DLOp { \ + static const auto kType = DisplayListOpType::kDraw##name; \ + \ + Draw##name##Op(const SkPath& path, \ + SkColor color, \ + SkScalar elevation, \ + SkScalar dpr) \ + : color(color), elevation(elevation), dpr(dpr), path(path) {} \ + \ + const SkColor color; \ + const SkScalar elevation; \ + const SkScalar dpr; \ + const SkPath path; \ + \ + void dispatch(Dispatcher& dispatcher) const { \ + dispatcher.drawShadow(path, color, elevation, nonoccluding, dpr); \ + } \ }; DEFINE_DRAW_SHADOW_OP(Shadow, false) -DEFINE_DRAW_SHADOW_OP(ShadowOccludes, true) +DEFINE_DRAW_SHADOW_OP(NonOccludingShadow, true) #undef DEFINE_DRAW_SHADOW_OP #pragma pack(pop, DLOp_Alignment) @@ -1365,10 +1365,10 @@ void DisplayListBuilder::drawTextBlob(const sk_sp blob, void DisplayListBuilder::drawShadow(const SkPath& path, const SkColor color, const SkScalar elevation, - bool occludes, + bool transparentOccluder, SkScalar dpr) { - occludes // - ? Push(0, 1, path, color, elevation, dpr) + transparentOccluder // + ? Push(0, 1, path, color, elevation, dpr) : Push(0, 1, path, color, elevation, dpr); } diff --git a/flow/display_list.h b/flow/display_list.h index affd05b043b1a..727094acb27eb 100644 --- a/flow/display_list.h +++ b/flow/display_list.h @@ -148,7 +148,7 @@ namespace flutter { V(DrawTextBlob) \ \ V(DrawShadow) \ - V(DrawShadowOccludes) + V(DrawNonOccludingShadow) #define DL_OP_TO_ENUM_VALUE(name) k##name, enum class DisplayListOpType { FOR_EACH_DISPLAY_LIST_OP(DL_OP_TO_ENUM_VALUE) }; @@ -325,7 +325,7 @@ class Dispatcher { virtual void drawShadow(const SkPath& path, const SkColor color, const SkScalar elevation, - bool occludes, + bool transparentOccluder, SkScalar dpr) = 0; }; @@ -440,7 +440,7 @@ class DisplayListBuilder final : public virtual Dispatcher, public SkRefCnt { void drawShadow(const SkPath& path, const SkColor color, const SkScalar elevation, - bool occludes, + bool transparentOccluder, SkScalar dpr) override; sk_sp Build(); diff --git a/flow/display_list_canvas.cc b/flow/display_list_canvas.cc index 2da08e5406c0f..ba37740a37156 100644 --- a/flow/display_list_canvas.cc +++ b/flow/display_list_canvas.cc @@ -176,10 +176,10 @@ void DisplayListCanvasDispatcher::drawTextBlob(const sk_sp blob, void DisplayListCanvasDispatcher::drawShadow(const SkPath& path, const SkColor color, const SkScalar elevation, - bool occludes, + bool transparentOccluder, SkScalar dpr) { flutter::PhysicalShapeLayer::DrawShadow(canvas_, path, color, elevation, - occludes, dpr); + transparentOccluder, dpr); } DisplayListCanvasRecorder::DisplayListCanvasRecorder(const SkRect& bounds) diff --git a/flow/display_list_canvas.h b/flow/display_list_canvas.h index aa99ac8dc1238..b2d9361c452e3 100644 --- a/flow/display_list_canvas.h +++ b/flow/display_list_canvas.h @@ -110,7 +110,7 @@ class DisplayListCanvasDispatcher : public virtual Dispatcher, void drawShadow(const SkPath& path, const SkColor color, const SkScalar elevation, - bool occludes, + bool transparentOccluder, SkScalar dpr) override; private: diff --git a/flow/display_list_canvas_unittests.cc b/flow/display_list_canvas_unittests.cc index 7bbf1c5437f31..992b19ba19a42 100644 --- a/flow/display_list_canvas_unittests.cc +++ b/flow/display_list_canvas_unittests.cc @@ -55,13 +55,13 @@ constexpr SkRect RenderBounds = // The X offsets which will be used for tall vertical diamonds are // expressed in terms of the rendering height to obtain the proper angle constexpr SkScalar MiterExtremeDiamondOffsetX = RenderHeight * 0.04; -constexpr SkScalar Miter10DiamondOffsetX = RenderHeight * 0.06; +constexpr SkScalar Miter10DiamondOffsetX = RenderHeight * 0.051; constexpr SkScalar Miter4DiamondOffsetX = RenderHeight * 0.14; // The Y offsets which will be used for long horizontal diamonds are // expressed in terms of the rendering width to obtain the proper angle constexpr SkScalar MiterExtremeDiamondOffsetY = RenderWidth * 0.04; -constexpr SkScalar Miter10DiamondOffsetY = RenderWidth * 0.06; +constexpr SkScalar Miter10DiamondOffsetY = RenderWidth * 0.051; constexpr SkScalar Miter4DiamondOffsetY = RenderWidth * 0.14; // Render 3 vertical and horizontal diamonds each @@ -82,6 +82,7 @@ constexpr SkPoint VerticalMiterDiamondPoints[] = { // <----X--+--X----> RenderCenter // \ / \ | / \ / to // \/ \|/ \/ bottom of RenderBounds + // clang-format off SkPoint::Make(x_off_l3, RenderCenterY), SkPoint::Make(x_off_l2, RenderTop), SkPoint::Make(x_off_l1, RenderCenterY), @@ -95,8 +96,10 @@ constexpr SkPoint VerticalMiterDiamondPoints[] = { SkPoint::Make(x_off_l1, RenderCenterY), SkPoint::Make(x_off_l2, RenderBottom), SkPoint::Make(x_off_l3, RenderCenterY), + // clang-format on }; -const int VerticalMiterDiamondPointCount = sizeof(VerticalMiterDiamondPoints) / sizeof(VerticalMiterDiamondPoints[0]); +const int VerticalMiterDiamondPointCount = + sizeof(VerticalMiterDiamondPoints) / sizeof(VerticalMiterDiamondPoints[0]); constexpr SkScalar y_off_0 = RenderCenterY; constexpr SkScalar y_off_u1 = RenderCenterY - Miter4DiamondOffsetY; @@ -109,6 +112,7 @@ const SkPoint HorizontalMiterDiamondPoints[] = { // Horizontal diamonds // Same configuration as Vertical diamonds above but // rotated 90 degrees + // clang-format off SkPoint::Make(RenderCenterX, y_off_u3), SkPoint::Make(RenderLeft, y_off_u2), SkPoint::Make(RenderCenterX, y_off_u1), @@ -122,8 +126,120 @@ const SkPoint HorizontalMiterDiamondPoints[] = { SkPoint::Make(RenderCenterX, y_off_u1), SkPoint::Make(RenderRight, y_off_u2), SkPoint::Make(RenderCenterX, y_off_u3), + // clang-format on +}; +const int HorizontalMiterDiamondPointCount = + (sizeof(HorizontalMiterDiamondPoints) / + sizeof(HorizontalMiterDiamondPoints[0])); + +// A class to specify how much tolerance to allow in bounds estimates. +// For some attributes, the machinery must make some conservative +// assumptions as to the extent of the bounds, but some of our test +// parameters do not produce bounds that expand by the full conservative +// estimates. This class provides a number of tweaks to apply to the +// pixel bounds to account for the conservative factors. +// +// An instance is passed along through the methods and if any test adds +// a paint attribute or other modifier that will cause a more conservative +// estimate for bounds, it can modify the factors here to account for it. +// Ideally, all tests will be executed with geometry that will trigger +// the conservative cases anyway and all attributes will be combined with +// other attributes that make their output more predictable, but in those +// cases where a given test sequence cannot really provide attributes to +// demonstrate the worst case scenario, they can modify these factors to +// avoid false bounds overflow notifications. +class BoundsTolerance { + public: + BoundsTolerance() : BoundsTolerance(0, 0, 1, 1, 0, 0, 0) {} + BoundsTolerance(SkScalar bounds_pad_x, + SkScalar bounds_pad_y, + SkScalar scale_x, + SkScalar scale_y, + SkScalar absolute_pad_x, + SkScalar absolute_pad_y, + SkScalar discrete_offset) + : bounds_pad_x_(bounds_pad_x), + bounds_pad_y_(bounds_pad_y), + scale_x_(scale_x), + scale_y_(scale_y), + absolute_pad_x_(absolute_pad_x), + absolute_pad_y_(absolute_pad_y), + discrete_offset_(discrete_offset) {} + + BoundsTolerance addBoundsPadding(SkScalar bounds_pad_x, + SkScalar bounds_pad_y) const { + return {bounds_pad_x_ + bounds_pad_x, + bounds_pad_y_ + bounds_pad_y, + scale_x_, + scale_y_, + absolute_pad_x_, + absolute_pad_y_, + discrete_offset_}; + } + + BoundsTolerance addScale(SkScalar scale_x, SkScalar scale_y) const { + return {bounds_pad_x_, // + bounds_pad_y_, // + scale_x_ * scale_x, // + scale_y_ * scale_y, // + absolute_pad_x_, // + absolute_pad_y_, // + discrete_offset_}; + } + + BoundsTolerance addAbsolutePadding(SkScalar absolute_pad_x, + SkScalar absolute_pad_y) const { + return {bounds_pad_x_, + bounds_pad_y_, + scale_x_, + scale_y_, + absolute_pad_x_ + absolute_pad_x, + absolute_pad_y_ + absolute_pad_y, + discrete_offset_}; + } + + BoundsTolerance addDiscreteOffset(SkScalar discrete_offset) const { + return {bounds_pad_x_, + bounds_pad_y_, + scale_x_, + scale_y_, + absolute_pad_x_, + absolute_pad_y_, + discrete_offset_ + discrete_offset}; + } + + bool overflows(SkISize pix_size, + int worst_bounds_pad_x, + int worst_bounds_pad_y) const { + int scaled_bounds_pad_x = + std::ceil((pix_size.width() + bounds_pad_x_) * scale_x_); + int allowed_width = scaled_bounds_pad_x + absolute_pad_x_; + int scaled_bounds_pad_y = + std::ceil((pix_size.height() + bounds_pad_y_) * scale_y_); + int allowed_height = scaled_bounds_pad_y + absolute_pad_y_; + int allowed_pad_x = allowed_width - pix_size.width(); + int allowed_pad_y = allowed_height - pix_size.height(); + if (worst_bounds_pad_x > allowed_pad_x || + worst_bounds_pad_y > allowed_pad_y) { + FML_LOG(ERROR) << "allowed pad: " // + << allowed_pad_x << ", " << allowed_pad_y; + } + return (worst_bounds_pad_x > allowed_pad_x || + worst_bounds_pad_y > allowed_pad_y); + } + + SkScalar discrete_offset() const { return discrete_offset_; } + + private: + SkScalar bounds_pad_x_; + SkScalar bounds_pad_y_; + SkScalar scale_x_; + SkScalar scale_y_; + SkScalar absolute_pad_x_; + SkScalar absolute_pad_y_; + + SkScalar discrete_offset_; }; -const int HorizontalMiterDiamondPointCount = sizeof(HorizontalMiterDiamondPoints) / sizeof(HorizontalMiterDiamondPoints[0]); class CanvasCompareTester { private: @@ -142,16 +258,20 @@ class CanvasCompareTester { // See: https://bugs.chromium.org/p/skia/issues/detail?id=12199 static bool TestingDrawAtlas; - // Bounds will never be completely tight for most operations, but we - // can establish an estimate for the amount of expected conservative - // overflow and only report bounds overflows above that factor. - // Expressed as a ratio of RenderWidth or RenderHeight. - static SkScalar NegligibleBoundsOverflowFactorX; - static SkScalar NegligibleBoundsOverflowFactorY; - public: typedef const std::function CvRenderer; typedef const std::function DlRenderer; + typedef const std::function + ToleranceAdjuster; + + static BoundsTolerance DefaultTolerance; + static const BoundsTolerance DefaultAdjuster(const BoundsTolerance& tolerance, + const SkPaint& paint, + const SkMatrix& matrix) { + return tolerance; + } // All of the tests should eventually use this method except for the // tests that call |RenderNoAttributes| because they do not use the @@ -159,21 +279,25 @@ class CanvasCompareTester { // But there are a couple of conditions beyond our control which require // the use of one of the variant methods below (|RenderShadows|, // |RenderVertices|, |RenderAtlas|). - static void RenderAll(CvRenderer& cv_renderer, DlRenderer& dl_renderer, - SkScalar overflow_factor_x = 0.10, - SkScalar overflow_factor_y = 0.10) { - RenderNoAttributes(cv_renderer, dl_renderer, overflow_factor_x, - overflow_factor_y); - RenderWithAttributes(cv_renderer, dl_renderer); + static void RenderAll(CvRenderer& cv_renderer, + DlRenderer& dl_renderer, + ToleranceAdjuster& adjuster = DefaultAdjuster, + const BoundsTolerance& tolerance = DefaultTolerance) { + RenderNoAttributes(cv_renderer, dl_renderer, adjuster, tolerance); + RenderWithAttributes(cv_renderer, dl_renderer, adjuster, tolerance); } // Used by the tests that render shadows to deal with a condition where // we cannot recapture the shadow information from an SkCanvas stream // due to the DrawShadowRec used by Skia is not properly exported. // See: https://bugs.chromium.org/p/skia/issues/detail?id=12125 - static void RenderShadows(CvRenderer& cv_renderer, DlRenderer& dl_renderer) { + static void RenderShadows( + CvRenderer& cv_renderer, + DlRenderer& dl_renderer, + ToleranceAdjuster& adjuster = DefaultAdjuster, + const BoundsTolerance& tolerance = DefaultTolerance) { TestingDrawShadows = true; - RenderNoAttributes(cv_renderer, dl_renderer); + RenderNoAttributes(cv_renderer, dl_renderer, adjuster, tolerance); TestingDrawShadows = false; } @@ -198,22 +322,23 @@ class CanvasCompareTester { // Used by the tests that call a draw method that does not take a paint // call. Those tests could use |RenderAll| but there would be a lot of // wasted test runs that prepare an SkPaint that is never used. - static void RenderNoAttributes(CvRenderer& cv_renderer, - DlRenderer& dl_renderer, - SkScalar overflow_factor_x = 0.10, - SkScalar overflow_factor_y = 0.10) { - NegligibleBoundsOverflowFactorX = overflow_factor_x; - NegligibleBoundsOverflowFactorY = overflow_factor_y; + static void RenderNoAttributes( + CvRenderer& cv_renderer, + DlRenderer& dl_renderer, + ToleranceAdjuster& adjuster = DefaultAdjuster, + const BoundsTolerance& tolerance = DefaultTolerance) { RenderWith([=](SkCanvas*, SkPaint& p) {}, // [=](DisplayListBuilder& d) {}, // - cv_renderer, dl_renderer, "Base Test"); - RenderWithTransforms(cv_renderer, dl_renderer); - RenderWithClips(cv_renderer, dl_renderer); - RenderWithSaveRestore(cv_renderer, dl_renderer); + cv_renderer, dl_renderer, adjuster, tolerance, "Base Test"); + RenderWithTransforms(cv_renderer, dl_renderer, adjuster, tolerance); + RenderWithClips(cv_renderer, dl_renderer, adjuster, tolerance); + RenderWithSaveRestore(cv_renderer, dl_renderer, adjuster, tolerance); } static void RenderWithSaveRestore(CvRenderer& cv_renderer, - DlRenderer& dl_renderer) { + DlRenderer& dl_renderer, + ToleranceAdjuster& adjuster, + const BoundsTolerance& tolerance) { SkRect clip = SkRect::MakeLTRB(0, 0, 10, 10); SkRect rect = SkRect::MakeLTRB(5, 5, 15, 15); SkColor alpha_layer_color = SkColorSetARGB(0x7f, 0x00, 0xff, 0xff); @@ -243,7 +368,8 @@ class CanvasCompareTester { b.drawRect(rect); b.restore(); }, - cv_renderer, dl_renderer, "With prior save/clip/restore"); + cv_renderer, dl_renderer, adjuster, tolerance, + "With prior save/clip/restore"); RenderWith( [=](SkCanvas* cv, SkPaint& p) { // cv->saveLayer(nullptr, nullptr); @@ -251,7 +377,8 @@ class CanvasCompareTester { [=](DisplayListBuilder& b) { // b.saveLayer(nullptr, false); }, - cv_restored, dl_restored, "saveLayer no paint, no bounds"); + cv_restored, dl_restored, adjuster, tolerance, + "saveLayer no paint, no bounds"); RenderWith( [=](SkCanvas* cv, SkPaint& p) { // cv->saveLayer(RenderBounds, nullptr); @@ -259,7 +386,8 @@ class CanvasCompareTester { [=](DisplayListBuilder& b) { // b.saveLayer(&RenderBounds, false); }, - cv_restored, dl_restored, "saveLayer no paint, with bounds"); + cv_restored, dl_restored, adjuster, tolerance, + "saveLayer no paint, with bounds"); RenderWith( [=](SkCanvas* cv, SkPaint& p) { SkPaint save_p; @@ -271,7 +399,8 @@ class CanvasCompareTester { b.saveLayer(nullptr, true); b.setColor(default_color); }, - cv_restored, dl_restored, "saveLayer with alpha, no bounds"); + cv_restored, dl_restored, adjuster, tolerance, + "saveLayer with alpha, no bounds"); RenderWith( [=](SkCanvas* cv, SkPaint& p) { SkPaint save_p; @@ -283,24 +412,29 @@ class CanvasCompareTester { b.saveLayer(&RenderBounds, true); b.setColor(default_color); }, - cv_restored, dl_restored, "saveLayer with alpha and bounds"); + cv_restored, dl_restored, adjuster, tolerance, + "saveLayer with alpha and bounds"); { sk_sp filter = SkImageFilters::Blur(5.0, 5.0, SkTileMode::kDecal, nullptr, nullptr); + BoundsTolerance blur5Tolerance = tolerance.addBoundsPadding(4, 4); { RenderWith( [=](SkCanvas* cv, SkPaint& p) { SkPaint save_p; save_p.setImageFilter(filter); cv->saveLayer(nullptr, &save_p); + p.setStrokeWidth(5.0); }, [=](DisplayListBuilder& b) { b.setImageFilter(filter); b.saveLayer(nullptr, true); b.setImageFilter(nullptr); + b.setStrokeWidth(5.0); }, - cv_restored, dl_restored, "saveLayer ImageFilter, no bounds"); + cv_restored, dl_restored, adjuster, blur5Tolerance, + "saveLayer ImageFilter, no bounds"); } ASSERT_TRUE(filter->unique()) << "SL with IF no bounds Cleanup"; { @@ -309,46 +443,51 @@ class CanvasCompareTester { SkPaint save_p; save_p.setImageFilter(filter); cv->saveLayer(RenderBounds, &save_p); + p.setStrokeWidth(5.0); }, [=](DisplayListBuilder& b) { b.setImageFilter(filter); b.saveLayer(&RenderBounds, true); b.setImageFilter(nullptr); + b.setStrokeWidth(5.0); }, - cv_restored, dl_restored, "saveLayer ImageFilter and bounds"); + cv_restored, dl_restored, adjuster, blur5Tolerance, + "saveLayer ImageFilter and bounds"); } ASSERT_TRUE(filter->unique()) << "SL with IF and bounds Cleanup"; } } static void RenderWithAttributes(CvRenderer& cv_renderer, - DlRenderer& dl_renderer) { + DlRenderer& dl_renderer, + ToleranceAdjuster& adjuster, + const BoundsTolerance& tolerance) { RenderWith([=](SkCanvas*, SkPaint& p) {}, // [=](DisplayListBuilder& d) {}, // - cv_renderer, dl_renderer, "Base Test"); + cv_renderer, dl_renderer, adjuster, tolerance, "Base Test"); RenderWith([=](SkCanvas*, SkPaint& p) { p.setAntiAlias(true); }, // [=](DisplayListBuilder& b) { b.setAA(true); }, // - cv_renderer, dl_renderer, "AA == True"); + cv_renderer, dl_renderer, adjuster, tolerance, "AA == True"); RenderWith([=](SkCanvas*, SkPaint& p) { p.setAntiAlias(false); }, // [=](DisplayListBuilder& b) { b.setAA(false); }, // - cv_renderer, dl_renderer, "AA == False"); + cv_renderer, dl_renderer, adjuster, tolerance, "AA == False"); RenderWith([=](SkCanvas*, SkPaint& p) { p.setDither(true); }, // [=](DisplayListBuilder& b) { b.setDither(true); }, // - cv_renderer, dl_renderer, "Dither == True"); + cv_renderer, dl_renderer, adjuster, tolerance, "Dither == True"); RenderWith([=](SkCanvas*, SkPaint& p) { p.setDither(false); }, // [=](DisplayListBuilder& b) { b.setDither(false); }, // - cv_renderer, dl_renderer, "Dither = False"); + cv_renderer, dl_renderer, adjuster, tolerance, "Dither = False"); RenderWith([=](SkCanvas*, SkPaint& p) { p.setColor(SK_ColorBLUE); }, // [=](DisplayListBuilder& b) { b.setColor(SK_ColorBLUE); }, // - cv_renderer, dl_renderer, "Color == Blue"); + cv_renderer, dl_renderer, adjuster, tolerance, "Color == Blue"); RenderWith([=](SkCanvas*, SkPaint& p) { p.setColor(SK_ColorGREEN); }, // [=](DisplayListBuilder& b) { b.setColor(SK_ColorGREEN); }, // - cv_renderer, dl_renderer, "Color == Green"); + cv_renderer, dl_renderer, adjuster, tolerance, "Color == Green"); - RenderWithStrokes(cv_renderer, dl_renderer); + RenderWithStrokes(cv_renderer, dl_renderer, adjuster, tolerance); { // half opaque cyan @@ -364,7 +503,7 @@ class CanvasCompareTester { b.setBlendMode(SkBlendMode::kSrcIn); b.setColor(blendableColor); }, - cv_renderer, dl_renderer, "Blend == SrcIn", &bg); + cv_renderer, dl_renderer, adjuster, tolerance, "Blend == SrcIn", &bg); RenderWith( [=](SkCanvas*, SkPaint& p) { p.setBlendMode(SkBlendMode::kDstIn); @@ -374,7 +513,7 @@ class CanvasCompareTester { b.setBlendMode(SkBlendMode::kDstIn); b.setColor(blendableColor); }, - cv_renderer, dl_renderer, "Blend == DstIn", &bg); + cv_renderer, dl_renderer, adjuster, tolerance, "Blend == DstIn", &bg); } if (!(TestingDrawAtlas || TestingDrawVertices)) { @@ -383,7 +522,7 @@ class CanvasCompareTester { { RenderWith([=](SkCanvas*, SkPaint& p) { p.setBlender(blender); }, [=](DisplayListBuilder& b) { b.setBlender(blender); }, - cv_renderer, dl_renderer, + cv_renderer, dl_renderer, adjuster, tolerance, "ImageFilter == Blender Arithmetic 0.25-false"); } ASSERT_TRUE(blender->unique()) << "Blender Cleanup"; @@ -391,7 +530,7 @@ class CanvasCompareTester { { RenderWith([=](SkCanvas*, SkPaint& p) { p.setBlender(blender); }, [=](DisplayListBuilder& b) { b.setBlender(blender); }, - cv_renderer, dl_renderer, + cv_renderer, dl_renderer, adjuster, tolerance, "ImageFilter == Blender Arithmetic 0.25-true"); } ASSERT_TRUE(blender->unique()) << "Blender Cleanup"; @@ -400,19 +539,21 @@ class CanvasCompareTester { { sk_sp filter = SkImageFilters::Blur(5.0, 5.0, SkTileMode::kDecal, nullptr, nullptr); + BoundsTolerance blur5Tolerance = tolerance.addBoundsPadding(4, 4); { RenderWith( [=](SkCanvas*, SkPaint& p) { // Provide some non-trivial stroke size to get blurred - p.setStrokeWidth(3.0); + p.setStrokeWidth(5.0); p.setImageFilter(filter); }, [=](DisplayListBuilder& b) { // Provide some non-trivial stroke size to get blurred - b.setStrokeWidth(3.0); + b.setStrokeWidth(5.0); b.setImageFilter(filter); - }, - cv_renderer, dl_renderer, "ImageFilter == Decal Blur 5"); + }, + cv_renderer, dl_renderer, adjuster, blur5Tolerance, + "ImageFilter == Decal Blur 5"); } ASSERT_TRUE(filter->unique()) << "ImageFilter Cleanup"; filter = @@ -421,15 +562,16 @@ class CanvasCompareTester { RenderWith( [=](SkCanvas*, SkPaint& p) { // Provide some non-trivial stroke size to get blurred - p.setStrokeWidth(3.0); + p.setStrokeWidth(5.0); p.setImageFilter(filter); }, [=](DisplayListBuilder& b) { // Provide some non-trivial stroke size to get blurred - b.setStrokeWidth(3.0); + b.setStrokeWidth(5.0); b.setImageFilter(filter); }, - cv_renderer, dl_renderer, "ImageFilter == Clamp Blur 5"); + cv_renderer, dl_renderer, adjuster, blur5Tolerance, + "ImageFilter == Clamp Blur 5"); } ASSERT_TRUE(filter->unique()) << "ImageFilter Cleanup"; } @@ -461,7 +603,8 @@ class CanvasCompareTester { b.setColor(SK_ColorYELLOW); b.setColorFilter(filter); }, - cv_renderer, dl_renderer, "ColorFilter == RotateRGB", &bg); + cv_renderer, dl_renderer, adjuster, tolerance, + "ColorFilter == RotateRGB", &bg); } ASSERT_TRUE(filter->unique()) << "ColorFilter Cleanup"; filter = SkColorFilters::Matrix(invert_color_matrix); @@ -476,7 +619,8 @@ class CanvasCompareTester { b.setColor(SK_ColorYELLOW); b.setInvertColors(true); }, - cv_renderer, dl_renderer, "ColorFilter == Invert", &bg); + cv_renderer, dl_renderer, adjuster, tolerance, + "ColorFilter == Invert", &bg); } ASSERT_TRUE(filter->unique()) << "ColorFilter Cleanup"; } @@ -489,13 +633,26 @@ class CanvasCompareTester { RenderWith( [=](SkCanvas*, SkPaint& p) { p.setStrokeWidth(5.0); + // A Discrete(3, 5) effect produces miters that are near + // maximal for a miter limit of 3.0. + p.setStrokeMiter(3.0); p.setPathEffect(effect); }, [=](DisplayListBuilder& b) { b.setStrokeWidth(5.0); + // A Discrete(3, 5) effect produces miters that are near + // maximal for a miter limit of 3.0. + b.setMiterLimit(3.0); b.setPathEffect(effect); }, - cv_renderer, dl_renderer, "PathEffect == Discrete-3-5"); + cv_renderer, dl_renderer, adjuster, + tolerance + // register the discrete offset so adjusters can compensate + .addDiscreteOffset(5) + // the miters in the 3-5 discrete effect don't always fill + // their conservative bounds, so tolerate a couple of pixels + .addBoundsPadding(2, 2), + "PathEffect == Discrete-3-5"); } ASSERT_TRUE(effect->unique()) << "PathEffect Cleanup"; effect = SkDiscretePathEffect::Make(2, 3); @@ -503,13 +660,26 @@ class CanvasCompareTester { RenderWith( [=](SkCanvas*, SkPaint& p) { p.setStrokeWidth(5.0); + // A Discrete(2, 3) effect produces miters that are near + // maximal for a miter limit of 2.5. + p.setStrokeMiter(2.5); p.setPathEffect(effect); }, [=](DisplayListBuilder& b) { b.setStrokeWidth(5.0); + // A Discrete(2, 3) effect produces miters that are near + // maximal for a miter limit of 2.5. + b.setMiterLimit(2.5); b.setPathEffect(effect); }, - cv_renderer, dl_renderer, "PathEffect == Discrete-2-3"); + cv_renderer, dl_renderer, adjuster, + tolerance + // register the discrete offset so adjusters can compensate + .addDiscreteOffset(3) + // the miters in the 3-5 discrete effect don't always fill + // their conservative bounds, so tolerate a couple of pixels + .addBoundsPadding(2, 2), + "PathEffect == Discrete-2-3"); } ASSERT_TRUE(effect->unique()) << "PathEffect Cleanup"; } @@ -517,46 +687,37 @@ class CanvasCompareTester { { sk_sp filter = SkMaskFilter::MakeBlur(kNormal_SkBlurStyle, 5.0); + BoundsTolerance blur5Tolerance = tolerance.addBoundsPadding(4, 4); { RenderWith( [=](SkCanvas*, SkPaint& p) { // Provide some non-trivial stroke size to get blurred - p.setStrokeWidth(3.0); - // MaskFilters do not always render with Square or Butt caps - // See https://bugs.chromium.org/p/skia/issues/detail?id=12435 - p.setStrokeCap(SkPaint::kRound_Cap); + p.setStrokeWidth(5.0); p.setMaskFilter(filter); }, [=](DisplayListBuilder& b) { // Provide some non-trivial stroke size to get blurred - b.setStrokeWidth(3.0); - // MaskFilters do not always render with Square or Butt caps - // See https://bugs.chromium.org/p/skia/issues/detail?id=12435 - b.setCaps(SkPaint::kRound_Cap); + b.setStrokeWidth(5.0); b.setMaskFilter(filter); }, - cv_renderer, dl_renderer, "MaskFilter == Blur 5"); + cv_renderer, dl_renderer, adjuster, blur5Tolerance, + "MaskFilter == Blur 5"); } ASSERT_TRUE(filter->unique()) << "MaskFilter Cleanup"; { RenderWith( [=](SkCanvas*, SkPaint& p) { // Provide some non-trivial stroke size to get blurred - p.setStrokeWidth(3.0); - // MaskFilters do not always render with Square or Butt caps - // See https://bugs.chromium.org/p/skia/issues/detail?id=12435 - p.setStrokeCap(SkPaint::kRound_Cap); + p.setStrokeWidth(5.0); p.setMaskFilter(filter); }, [=](DisplayListBuilder& b) { // Provide some non-trivial stroke size to get blurred - b.setStrokeWidth(3.0); - // MaskFilters do not always render with Square or Butt caps - // See https://bugs.chromium.org/p/skia/issues/detail?id=12435 - b.setCaps(SkPaint::kRound_Cap); + b.setStrokeWidth(5.0); b.setMaskBlurFilter(kNormal_SkBlurStyle, 5.0); }, - cv_renderer, dl_renderer, "MaskFilter == Blur(Normal, 5.0)"); + cv_renderer, dl_renderer, adjuster, blur5Tolerance, + "MaskFilter == Blur(Normal, 5.0)"); } ASSERT_TRUE(filter->unique()) << "MaskFilter Cleanup"; } @@ -581,22 +742,30 @@ class CanvasCompareTester { { RenderWith([=](SkCanvas*, SkPaint& p) { p.setShader(shader); }, [=](DisplayListBuilder& b) { b.setShader(shader); }, - cv_renderer, dl_renderer, "LinearGradient GYB"); + cv_renderer, dl_renderer, adjuster, tolerance, + "LinearGradient GYB"); } ASSERT_TRUE(shader->unique()) << "Shader Cleanup"; } } static void RenderWithStrokes(CvRenderer& cv_renderer, - DlRenderer& dl_renderer) { + DlRenderer& dl_renderer, + ToleranceAdjuster& adjuster, + const BoundsTolerance& tolerance_in) { + // The test cases were generated with geometry that will try to fill + // out the various miter limits used for testing, but they can be off + // by a couple of pixels so we will relax bounds testing for strokes by + // a couple of pixels. + BoundsTolerance tolerance = tolerance_in.addBoundsPadding(2, 2); RenderWith( [=](SkCanvas*, SkPaint& p) { p.setStyle(SkPaint::kFill_Style); }, [=](DisplayListBuilder& b) { b.setDrawStyle(SkPaint::kFill_Style); }, - cv_renderer, dl_renderer, "Fill"); + cv_renderer, dl_renderer, adjuster, tolerance, "Fill"); RenderWith( [=](SkCanvas*, SkPaint& p) { p.setStyle(SkPaint::kStroke_Style); }, [=](DisplayListBuilder& b) { b.setDrawStyle(SkPaint::kStroke_Style); }, - cv_renderer, dl_renderer, "Stroke + defaults"); + cv_renderer, dl_renderer, adjuster, tolerance, "Stroke + defaults"); RenderWith( [=](SkCanvas*, SkPaint& p) { @@ -607,7 +776,8 @@ class CanvasCompareTester { b.setDrawStyle(SkPaint::kFill_Style); b.setStrokeWidth(10.0); }, - cv_renderer, dl_renderer, "Fill + unnecessary StrokeWidth 10"); + cv_renderer, dl_renderer, adjuster, tolerance, + "Fill + unnecessary StrokeWidth 10"); RenderWith( [=](SkCanvas*, SkPaint& p) { @@ -618,7 +788,7 @@ class CanvasCompareTester { b.setDrawStyle(SkPaint::kStroke_Style); b.setStrokeWidth(10.0); }, - cv_renderer, dl_renderer, "Stroke Width 10"); + cv_renderer, dl_renderer, adjuster, tolerance, "Stroke Width 10"); RenderWith( [=](SkCanvas*, SkPaint& p) { p.setStyle(SkPaint::kStroke_Style); @@ -628,7 +798,7 @@ class CanvasCompareTester { b.setDrawStyle(SkPaint::kStroke_Style); b.setStrokeWidth(5.0); }, - cv_renderer, dl_renderer, "Stroke Width 5"); + cv_renderer, dl_renderer, adjuster, tolerance, "Stroke Width 5"); RenderWith( [=](SkCanvas*, SkPaint& p) { @@ -641,7 +811,8 @@ class CanvasCompareTester { b.setStrokeWidth(5.0); b.setCaps(SkPaint::kButt_Cap); }, - cv_renderer, dl_renderer, "Stroke Width 5, Butt Cap"); + cv_renderer, dl_renderer, adjuster, tolerance, + "Stroke Width 5, Butt Cap"); RenderWith( [=](SkCanvas*, SkPaint& p) { p.setStyle(SkPaint::kStroke_Style); @@ -653,7 +824,8 @@ class CanvasCompareTester { b.setStrokeWidth(5.0); b.setCaps(SkPaint::kRound_Cap); }, - cv_renderer, dl_renderer, "Stroke Width 5, Round Cap"); + cv_renderer, dl_renderer, adjuster, tolerance, + "Stroke Width 5, Round Cap"); RenderWith( [=](SkCanvas*, SkPaint& p) { @@ -666,7 +838,8 @@ class CanvasCompareTester { b.setStrokeWidth(5.0); b.setJoins(SkPaint::kBevel_Join); }, - cv_renderer, dl_renderer, "Stroke Width 5, Bevel Join"); + cv_renderer, dl_renderer, adjuster, tolerance, + "Stroke Width 5, Bevel Join"); RenderWith( [=](SkCanvas*, SkPaint& p) { p.setStyle(SkPaint::kStroke_Style); @@ -678,7 +851,8 @@ class CanvasCompareTester { b.setStrokeWidth(5.0); b.setJoins(SkPaint::kRound_Join); }, - cv_renderer, dl_renderer, "Stroke Width 5, Round Join"); + cv_renderer, dl_renderer, adjuster, tolerance, + "Stroke Width 5, Round Join"); RenderWith( [=](SkCanvas*, SkPaint& p) { @@ -686,14 +860,21 @@ class CanvasCompareTester { p.setStrokeWidth(5.0); p.setStrokeMiter(10.0); p.setStrokeJoin(SkPaint::kMiter_Join); + // AA helps fill in the peaks of the really thin miters better + // for bounds accuracy testing + p.setAntiAlias(true); }, [=](DisplayListBuilder& b) { b.setDrawStyle(SkPaint::kStroke_Style); b.setStrokeWidth(5.0); b.setMiterLimit(10.0); b.setJoins(SkPaint::kMiter_Join); + // AA helps fill in the peaks of the really thin miters better + // for bounds accuracy testing + b.setAA(true); }, - cv_renderer, dl_renderer, "Stroke Width 5, Miter 10"); + cv_renderer, dl_renderer, adjuster, tolerance, + "Stroke Width 5, Miter 10"); RenderWith( [=](SkCanvas*, SkPaint& p) { @@ -708,11 +889,12 @@ class CanvasCompareTester { b.setMiterLimit(0.0); b.setJoins(SkPaint::kMiter_Join); }, - cv_renderer, dl_renderer, "Stroke Width 5, Miter 0"); + cv_renderer, dl_renderer, adjuster, tolerance, + "Stroke Width 5, Miter 0"); { - const SkScalar TestDashes1[] = {4.0, 2.0}; - const SkScalar TestDashes2[] = {1.0, 1.5}; + const SkScalar TestDashes1[] = {29.0, 2.0}; + const SkScalar TestDashes2[] = {17.0, 1.5}; sk_sp effect = SkDashPathEffect::Make(TestDashes1, 2, 0.0f); { RenderWith( @@ -721,13 +903,6 @@ class CanvasCompareTester { p.setStyle(SkPaint::kStroke_Style); // Provide some non-trivial stroke size to get dashed p.setStrokeWidth(5.0); - // Bounds code must conservatively assume a PathEffect may - // induce miter joins in geometry and adds a fairly large - // pad to the bounds as a result. Dashing doesn't create - // any new joins so that pessimal assumption in the bounds - // code isn't necessary here. We set a round join so that - // our bounds efficiency tests don't see a ridiculous pad. - p.setStrokeJoin(SkPaint::kRound_Join); p.setPathEffect(effect); }, [=](DisplayListBuilder& b) { @@ -735,67 +910,75 @@ class CanvasCompareTester { b.setDrawStyle(SkPaint::kStroke_Style); // Provide some non-trivial stroke size to get dashed b.setStrokeWidth(5.0); - // Bounds code must conservatively assume a PathEffect may - // induce miter joins in geometry and adds a fairly large - // pad to the bounds as a result. Dashing doesn't create - // any new joins so that pessimal assumption in the bounds - // code isn't necessary here. We set a round join so that - // our bounds efficiency tests don't see a ridiculous pad. - b.setJoins(SkPaint::kRound_Join); b.setPathEffect(effect); }, - cv_renderer, dl_renderer, "PathEffect == Dash-4-2"); + cv_renderer, dl_renderer, adjuster, tolerance, + "PathEffect == Dash-29-2"); } ASSERT_TRUE(effect->unique()) << "PathEffect Cleanup"; effect = SkDashPathEffect::Make(TestDashes2, 2, 0.0f); { RenderWith( [=](SkCanvas*, SkPaint& p) { + // Need stroke style to see dashing properly p.setStyle(SkPaint::kStroke_Style); + // Provide some non-trivial stroke size to get dashed p.setStrokeWidth(5.0); - p.setStrokeJoin(SkPaint::kRound_Join); p.setPathEffect(effect); }, [=](DisplayListBuilder& b) { + // Need stroke style to see dashing properly b.setDrawStyle(SkPaint::kStroke_Style); + // Provide some non-trivial stroke size to get dashed b.setStrokeWidth(5.0); - b.setJoins(SkPaint::kRound_Join); b.setPathEffect(effect); }, - cv_renderer, dl_renderer, "PathEffect == Dash-1-1.5"); + cv_renderer, dl_renderer, adjuster, tolerance, + "PathEffect == Dash-17-1.5"); } ASSERT_TRUE(effect->unique()) << "PathEffect Cleanup"; } } static void RenderWithTransforms(CvRenderer& cv_renderer, - DlRenderer& dl_renderer) { + DlRenderer& dl_renderer, + ToleranceAdjuster& adjuster, + const BoundsTolerance& tolerance) { + // If there is bounds padding for some conservative bounds overestimate + // then that padding will be even more pronounced in rotated or skewed + // coordinate systems so we scale the padding by about 5% to compensate. + BoundsTolerance skewed_tolerance = tolerance.addScale(1.05, 1.05); RenderWith([=](SkCanvas* c, SkPaint&) { c->translate(5, 10); }, // [=](DisplayListBuilder& b) { b.translate(5, 10); }, // - cv_renderer, dl_renderer, "Translate 5, 10"); + cv_renderer, dl_renderer, adjuster, tolerance, + "Translate 5, 10"); RenderWith([=](SkCanvas* c, SkPaint&) { c->scale(1.05, 1.05); }, // [=](DisplayListBuilder& b) { b.scale(1.05, 1.05); }, // - cv_renderer, dl_renderer, "Scale +5%"); + cv_renderer, dl_renderer, adjuster, tolerance, // + "Scale +5%"); RenderWith([=](SkCanvas* c, SkPaint&) { c->rotate(5); }, // [=](DisplayListBuilder& b) { b.rotate(5); }, // - cv_renderer, dl_renderer, "Rotate 5 degrees"); - RenderWith([=](SkCanvas* c, SkPaint&) { c->skew(0.05, 0.05); }, // - [=](DisplayListBuilder& b) { b.skew(0.05, 0.05); }, // - cv_renderer, dl_renderer, "Skew 5%"); + cv_renderer, dl_renderer, adjuster, skewed_tolerance, + "Rotate 5 degrees"); + RenderWith([=](SkCanvas* c, SkPaint&) { c->skew(0.05, 0.05); }, // + [=](DisplayListBuilder& b) { b.skew(0.05, 0.05); }, // + cv_renderer, dl_renderer, adjuster, skewed_tolerance, // + "Skew 5%"); { - SkMatrix tx = SkMatrix::MakeAll(1.10, 0.10, 5, - 0.05, 1.05, 10, + SkMatrix tx = SkMatrix::MakeAll(1.10, 0.10, 5, // + 0.05, 1.05, 10, // 0, 0, 1); RenderWith([=](SkCanvas* c, SkPaint&) { c->concat(tx); }, // [=](DisplayListBuilder& b) { b.transform2x3(tx[0], tx[1], tx[2], // tx[3], tx[4], tx[5]); }, // - cv_renderer, dl_renderer, "Transform 2x3"); + cv_renderer, dl_renderer, adjuster, skewed_tolerance, + "Transform 2x3"); } { - SkMatrix tx = SkMatrix::MakeAll(1.10, 0.10, 5, - 0.05, 1.05, 10, + SkMatrix tx = SkMatrix::MakeAll(1.10, 0.10, 5, // + 0.05, 1.05, 10, // 0, 0, 1.01); RenderWith([=](SkCanvas* c, SkPaint&) { c->concat(tx); }, // [=](DisplayListBuilder& b) { @@ -803,13 +986,19 @@ class CanvasCompareTester { tx[3], tx[4], tx[5], // tx[6], tx[7], tx[8]); }, // - cv_renderer, dl_renderer, "Transform 3x3"); + cv_renderer, dl_renderer, adjuster, skewed_tolerance, + "Transform 3x3"); } } static void RenderWithClips(CvRenderer& cv_renderer, - DlRenderer& dl_renderer) { + DlRenderer& dl_renderer, + ToleranceAdjuster& diff_adjuster, + const BoundsTolerance& diff_tolerance) { SkRect r_clip = RenderBounds.makeInset(15.5, 15.5); + // For kIntersect clips we can be really strict on tolerance + ToleranceAdjuster& intersect_adjuster = DefaultAdjuster; + BoundsTolerance& intersect_tolerance = DefaultTolerance; RenderWith( [=](SkCanvas* c, SkPaint&) { c->clipRect(r_clip, SkClipOp::kIntersect, false); @@ -817,7 +1006,8 @@ class CanvasCompareTester { [=](DisplayListBuilder& b) { b.clipRect(r_clip, false, SkClipOp::kIntersect); }, - cv_renderer, dl_renderer, "Hard ClipRect inset by 15.5"); + cv_renderer, dl_renderer, intersect_adjuster, intersect_tolerance, + "Hard ClipRect inset by 15.5"); RenderWith( [=](SkCanvas* c, SkPaint&) { c->clipRect(r_clip, SkClipOp::kIntersect, true); @@ -825,7 +1015,8 @@ class CanvasCompareTester { [=](DisplayListBuilder& b) { b.clipRect(r_clip, true, SkClipOp::kIntersect); }, - cv_renderer, dl_renderer, "AA ClipRect inset by 15.5"); + cv_renderer, dl_renderer, intersect_adjuster, intersect_tolerance, + "AA ClipRect inset by 15.5"); RenderWith( [=](SkCanvas* c, SkPaint&) { c->clipRect(r_clip, SkClipOp::kDifference, false); @@ -833,7 +1024,8 @@ class CanvasCompareTester { [=](DisplayListBuilder& b) { b.clipRect(r_clip, false, SkClipOp::kDifference); }, - cv_renderer, dl_renderer, "Hard ClipRect Diff, inset by 15.5"); + cv_renderer, dl_renderer, diff_adjuster, diff_tolerance, + "Hard ClipRect Diff, inset by 15.5"); SkRRect rr_clip = SkRRect::MakeRectXY(r_clip, 1.8, 2.7); RenderWith( [=](SkCanvas* c, SkPaint&) { @@ -842,7 +1034,8 @@ class CanvasCompareTester { [=](DisplayListBuilder& b) { b.clipRRect(rr_clip, false, SkClipOp::kIntersect); }, - cv_renderer, dl_renderer, "Hard ClipRRect inset by 15.5"); + cv_renderer, dl_renderer, intersect_adjuster, intersect_tolerance, + "Hard ClipRRect inset by 15.5"); RenderWith( [=](SkCanvas* c, SkPaint&) { c->clipRRect(rr_clip, SkClipOp::kIntersect, true); @@ -850,7 +1043,8 @@ class CanvasCompareTester { [=](DisplayListBuilder& b) { b.clipRRect(rr_clip, true, SkClipOp::kIntersect); }, - cv_renderer, dl_renderer, "AA ClipRRect inset by 15.5"); + cv_renderer, dl_renderer, intersect_adjuster, intersect_tolerance, + "AA ClipRRect inset by 15.5"); RenderWith( [=](SkCanvas* c, SkPaint&) { c->clipRRect(rr_clip, SkClipOp::kDifference, false); @@ -858,7 +1052,8 @@ class CanvasCompareTester { [=](DisplayListBuilder& b) { b.clipRRect(rr_clip, false, SkClipOp::kDifference); }, - cv_renderer, dl_renderer, "Hard ClipRRect Diff, inset by 15.5"); + cv_renderer, dl_renderer, diff_adjuster, diff_tolerance, + "Hard ClipRRect Diff, inset by 15.5"); SkPath path_clip = SkPath(); path_clip.setFillType(SkPathFillType::kEvenOdd); path_clip.addRect(r_clip); @@ -870,7 +1065,8 @@ class CanvasCompareTester { [=](DisplayListBuilder& b) { b.clipPath(path_clip, false, SkClipOp::kIntersect); }, - cv_renderer, dl_renderer, "Hard ClipPath inset by 15.5"); + cv_renderer, dl_renderer, intersect_adjuster, intersect_tolerance, + "Hard ClipPath inset by 15.5"); RenderWith( [=](SkCanvas* c, SkPaint&) { c->clipPath(path_clip, SkClipOp::kIntersect, true); @@ -878,7 +1074,8 @@ class CanvasCompareTester { [=](DisplayListBuilder& b) { b.clipPath(path_clip, true, SkClipOp::kIntersect); }, - cv_renderer, dl_renderer, "AA ClipPath inset by 15.5"); + cv_renderer, dl_renderer, intersect_adjuster, intersect_tolerance, + "AA ClipPath inset by 15.5"); RenderWith( [=](SkCanvas* c, SkPaint&) { c->clipPath(path_clip, SkClipOp::kDifference, false); @@ -886,7 +1083,8 @@ class CanvasCompareTester { [=](DisplayListBuilder& b) { b.clipPath(path_clip, false, SkClipOp::kDifference); }, - cv_renderer, dl_renderer, "Hard ClipPath Diff, inset by 15.5"); + cv_renderer, dl_renderer, diff_adjuster, diff_tolerance, + "Hard ClipPath Diff, inset by 15.5"); } static sk_sp getSkPicture(CvRenderer& cv_setup, @@ -904,6 +1102,8 @@ class CanvasCompareTester { DlRenderer& dl_setup, CvRenderer& cv_render, DlRenderer& dl_render, + ToleranceAdjuster& adjuster, + const BoundsTolerance& tolerance_in, const std::string info, const SkColor* bg = nullptr) { // surface1 is direct rendering via SkCanvas to SkSurface @@ -911,6 +1111,8 @@ class CanvasCompareTester { sk_sp ref_surface = makeSurface(bg); SkPaint paint1; cv_setup(ref_surface->getCanvas(), paint1); + const BoundsTolerance tolerance = adjuster( + tolerance_in, paint1, ref_surface->getCanvas()->getTotalMatrix()); cv_render(ref_surface->getCanvas(), paint1); sk_sp ref_picture = getSkPicture(cv_setup, cv_render); SkRect ref_bounds = ref_picture->cullRect(); @@ -931,15 +1133,14 @@ class CanvasCompareTester { dl_render(builder); sk_sp display_list = builder.Build(); SkRect dl_bounds = display_list->bounds(); -#ifdef DISPLAY_LIST_BOUNDS_ACCURACY_CHECKING - if (dl_bounds != ref_bounds) { + if (!ref_bounds.roundOut().contains(dl_bounds)) { FML_LOG(ERROR) << "For " << info; - FML_LOG(ERROR) << "ref: " << ref_bounds.fLeft << ", " << ref_bounds.fTop - << " => " << ref_bounds.fRight << ", " - << ref_bounds.fBottom; - FML_LOG(ERROR) << "dl: " << dl_bounds.fLeft << ", " << dl_bounds.fTop - << " => " << dl_bounds.fRight << ", " - << dl_bounds.fBottom; + FML_LOG(ERROR) << "ref: " // + << ref_bounds.fLeft << ", " << ref_bounds.fTop << " => " + << ref_bounds.fRight << ", " << ref_bounds.fBottom; + FML_LOG(ERROR) << "dl: " // + << dl_bounds.fLeft << ", " << dl_bounds.fTop << " => " + << dl_bounds.fRight << ", " << dl_bounds.fBottom; if (!dl_bounds.contains(ref_bounds)) { FML_LOG(ERROR) << "DisplayList bounds are too small!"; } @@ -947,7 +1148,6 @@ class CanvasCompareTester { FML_LOG(ERROR) << "###### DisplayList bounds larger than reference!"; } } -#endif // DISPLAY_LIST_BOUNDS_ACCURACY_CHECKING // This sometimes triggers, but when it triggers and I examine // the ref_bounds, they are always unnecessarily large and @@ -960,7 +1160,7 @@ class CanvasCompareTester { display_list->RenderTo(test_surface->getCanvas()); compareToReference(test_surface.get(), &ref_pixels, info + " (DL render)", - &dl_bounds, bg); + &dl_bounds, &tolerance, bg); } // This test cannot work if the rendering is using shadows until @@ -976,7 +1176,7 @@ class CanvasCompareTester { cv_render(&dl_recorder, test_paint); dl_recorder.builder()->Build()->RenderTo(test_surface->getCanvas()); compareToReference(test_surface.get(), &ref_pixels, - info + " (Sk->DL render)", nullptr, nullptr); + info + " (Sk->DL render)", nullptr, nullptr, nullptr); } { @@ -1015,7 +1215,7 @@ class CanvasCompareTester { display_list->RenderTo(test_canvas); compareToReference(test_surface.get(), &ref_pixels2, info + " (SKP/DL render scaled 2x)", nullptr, nullptr, - TestWidth2, TestHeight2, false); + nullptr, TestWidth2, TestHeight2, false); } } @@ -1046,6 +1246,7 @@ class CanvasCompareTester { SkPixmap* reference, const std::string info, SkRect* bounds, + const BoundsTolerance* tolerance, const SkColor* bg, int width = TestWidth, int height = TestHeight, @@ -1102,49 +1303,48 @@ class CanvasCompareTester { << bounds->fRight << ", " << bounds->fBottom // << "]"; } else if (bounds) { - showBoundsOverflow(info, i_bounds, minX, minY, maxX, maxY); + showBoundsOverflow(info, i_bounds, tolerance, minX, minY, maxX, maxY); } -#ifdef DISPLAY_LIST_BOUNDS_ACCURACY_CHECKING - if (bounds && *bounds != SkRect::MakeLTRB(minX, minY, maxX + 1, maxY + 1)) { - FML_LOG(ERROR) << "inaccurate bounds for " << info; - FML_LOG(ERROR) << "dl: " << bounds->fLeft << ", " << bounds->fTop - << " => " << bounds->fRight << ", " << bounds->fBottom; - FML_LOG(ERROR) << "pixels: " << minX << ", " << minY << " => " - << (maxX + 1) << ", " << (maxY + 1); - } -#endif // DISPLAY_LIST_BOUNDS_ACCURACY_CHECKING ASSERT_EQ(pixels_oob, 0) << info; ASSERT_EQ(pixels_different, 0) << info; } - static void showBoundsOverflow(std::string info, SkIRect& bounds, - int pixLeft, int pixTop, int pixRight, int pixBottom) { + static void showBoundsOverflow(std::string info, + SkIRect& bounds, + const BoundsTolerance* tolerance, + int pixLeft, + int pixTop, + int pixRight, + int pixBottom) { int pad_left = std::max(0, pixLeft - bounds.fLeft); int pad_top = std::max(0, pixTop - bounds.fTop); int pad_right = std::max(0, bounds.fRight - pixRight); int pad_bottom = std::max(0, bounds.fBottom - pixBottom); int pixWidth = pixRight - pixLeft; int pixHeight = pixBottom - pixTop; + SkISize pixSize = SkISize::Make(pixWidth, pixHeight); int worst_pad_x = std::max(pad_left, pad_right); int worst_pad_y = std::max(pad_top, pad_bottom); - if (worst_pad_x > std::ceil(pixWidth * NegligibleBoundsOverflowFactorX) || - worst_pad_y > std::ceil(pixHeight * NegligibleBoundsOverflowFactorY)) { + if (tolerance->overflows(pixSize, worst_pad_x, worst_pad_y)) { FML_LOG(ERROR) << "Overflow for " << info; - FML_LOG(ERROR) << "pix bounds[" // - << pixLeft << ", " << pixTop << " => " << pixRight << ", " << pixBottom + FML_LOG(ERROR) << "pix bounds[" // + << pixLeft << ", " << pixTop << " => " // + << pixRight << ", " << pixBottom // << "]"; FML_LOG(ERROR) << "dl_bounds[" // << bounds.fLeft << ", " << bounds.fTop // << " => " // << bounds.fRight << ", " << bounds.fBottom // << "]"; - FML_LOG(ERROR) << "Bounds overflowed by up to " << worst_pad_x << ", " << worst_pad_y - << " (" << (worst_pad_x * 100.0 / pixWidth) + FML_LOG(ERROR) << "Bounds overflowed by up to " // + << worst_pad_x << ", " << worst_pad_y // + << " (" << (worst_pad_x * 100.0 / pixWidth) // << "%, " << (worst_pad_y * 100.0 / pixHeight) << "%)"; - int pix_area = pixWidth * pixHeight; + int pix_area = pixSize.area(); int dl_area = bounds.width() * bounds.height(); - FML_LOG(ERROR) << "Total overflow area: " << (dl_area - pix_area) + FML_LOG(ERROR) << "Total overflow area: " << (dl_area - pix_area) // << " (+" << (dl_area * 100.0 / pix_area - 100.0) << "%)"; + FML_LOG(ERROR); } } @@ -1182,7 +1382,8 @@ class CanvasCompareTester { static sk_sp MakeTextBlob(std::string string, SkScalar font_height) { - SkFont font(SkTypeface::MakeFromName("ahem", SkFontStyle::Normal()), font_height); + SkFont font(SkTypeface::MakeFromName("ahem", SkFontStyle::Normal()), + font_height); return SkTextBlob::MakeFromText(string.c_str(), string.size(), font, SkTextEncoding::kUTF8); } @@ -1191,8 +1392,8 @@ class CanvasCompareTester { bool CanvasCompareTester::TestingDrawShadows = false; bool CanvasCompareTester::TestingDrawVertices = false; bool CanvasCompareTester::TestingDrawAtlas = false; -SkScalar CanvasCompareTester::NegligibleBoundsOverflowFactorX = 0.0; -SkScalar CanvasCompareTester::NegligibleBoundsOverflowFactorY = 0.0; +BoundsTolerance CanvasCompareTester::DefaultTolerance = + BoundsTolerance().addAbsolutePadding(1, 1); const sk_sp CanvasCompareTester::testImage = CanvasCompareTester::makeTestImage(); @@ -1217,19 +1418,99 @@ TEST(DisplayListCanvas, DrawColor) { }); } -TEST(DisplayListCanvas, DrawDiagonalLine) { +BoundsTolerance lineTolerance(const BoundsTolerance& tolerance, + const SkPaint& paint, + const SkMatrix& matrix, + bool is_horizontal, + bool is_vertical, + bool ignores_butt_cap) { + SkScalar adjust = 0.0; + SkScalar half_width = paint.getStrokeWidth() * 0.5f; + if (tolerance.discrete_offset() > 0) { + // When a discrete path effect is added, the bounds calculations must allow + // for miters in any direction, but a horizontal line will not have + // miters in the horizontal direction, similarly for vertical + // lines, and diagonal lines will have miters off at a "45 degree" angle + // that don't expand the bounds much at all. + // Also, the discrete offset will not move any points parallel with + // the line, so provide tolerance for both miters and offset. + adjust = half_width * paint.getStrokeMiter() + tolerance.discrete_offset(); + } + if (paint.getStrokeCap() == SkPaint::kButt_Cap && !ignores_butt_cap) { + adjust = std::max(adjust, half_width); + } + if (adjust == 0) { + return CanvasCompareTester::DefaultAdjuster(tolerance, paint, matrix); + } + SkScalar hTolerance; + SkScalar vTolerance; + if (is_horizontal) { + FML_DCHECK(!is_vertical); + hTolerance = adjust; + vTolerance = 0; + } else if (is_vertical) { + hTolerance = 0; + vTolerance = adjust; + } else { + // The perpendicular miters just do not impact the bounds of + // diagonal lines at all as they are aimed in the wrong direction + // to matter. So allow tolerance in both axes. + hTolerance = vTolerance = adjust; + } + BoundsTolerance new_tolerance = + tolerance.addBoundsPadding(hTolerance, vTolerance); + return CanvasCompareTester::DefaultAdjuster(new_tolerance, paint, matrix); +} + +// For drawing horizontal lines +BoundsTolerance hLineTolerance(const BoundsTolerance& tolerance, + const SkPaint& paint, + const SkMatrix& matrix) { + return lineTolerance(tolerance, paint, matrix, true, false, false); +} + +// For drawing vertical lines +BoundsTolerance vLineTolerance(const BoundsTolerance& tolerance, + const SkPaint& paint, + const SkMatrix& matrix) { + return lineTolerance(tolerance, paint, matrix, false, true, false); +} + +// For drawing diagonal lines +BoundsTolerance dLineTolerance(const BoundsTolerance& tolerance, + const SkPaint& paint, + const SkMatrix& matrix) { + return lineTolerance(tolerance, paint, matrix, false, false, false); +} + +// For drawing individual points (drawPoints(Point_Mode)) +BoundsTolerance pointsTolerance(const BoundsTolerance& tolerance, + const SkPaint& paint, + const SkMatrix& matrix) { + return lineTolerance(tolerance, paint, matrix, false, false, true); +} + +TEST(DisplayListCanvas, DrawDiagonalLines) { SkPoint p1 = SkPoint::Make(RenderLeft, RenderTop); SkPoint p2 = SkPoint::Make(RenderRight, RenderBottom); + SkPoint p3 = SkPoint::Make(RenderLeft, RenderBottom); + SkPoint p4 = SkPoint::Make(RenderRight, RenderTop); CanvasCompareTester::RenderAll( [=](SkCanvas* canvas, SkPaint& paint) { // + // Skia requires kStroke style on horizontal and vertical + // lines to get the bounds correct. + // See https://bugs.chromium.org/p/skia/issues/detail?id=12446 SkPaint p = paint; p.setStyle(SkPaint::kStroke_Style); canvas->drawLine(p1, p2, p); + canvas->drawLine(p3, p4, p); }, [=](DisplayListBuilder& builder) { // builder.drawLine(p1, p2); - }); + builder.drawLine(p3, p4); + }, + dLineTolerance); } TEST(DisplayListCanvas, DrawHorizontalLine) { @@ -1238,6 +1519,9 @@ TEST(DisplayListCanvas, DrawHorizontalLine) { CanvasCompareTester::RenderAll( [=](SkCanvas* canvas, SkPaint& paint) { // + // Skia requires kStroke style on horizontal and vertical + // lines to get the bounds correct. + // See https://bugs.chromium.org/p/skia/issues/detail?id=12446 SkPaint p = paint; p.setStyle(SkPaint::kStroke_Style); canvas->drawLine(p1, p2, p); @@ -1245,10 +1529,7 @@ TEST(DisplayListCanvas, DrawHorizontalLine) { [=](DisplayListBuilder& builder) { // builder.drawLine(p1, p2); }, - // Small disturbances in conservative bounds estimation can have - // a large proportional effect on the bounds height for a - // horizontal line so we relax the vertical compliance a bit. - 0.10f, 0.20f); + hLineTolerance); } TEST(DisplayListCanvas, DrawVerticalLine) { @@ -1257,6 +1538,9 @@ TEST(DisplayListCanvas, DrawVerticalLine) { CanvasCompareTester::RenderAll( [=](SkCanvas* canvas, SkPaint& paint) { // + // Skia requires kStroke style on horizontal and vertical + // lines to get the bounds correct. + // See https://bugs.chromium.org/p/skia/issues/detail?id=12446 SkPaint p = paint; p.setStyle(SkPaint::kStroke_Style); canvas->drawLine(p1, p2, p); @@ -1264,10 +1548,7 @@ TEST(DisplayListCanvas, DrawVerticalLine) { [=](DisplayListBuilder& builder) { // builder.drawLine(p1, p2); }, - // Small disturbances in conservative bounds estimation can have - // a large proportional effect on the bounds width for a - // vertical line so we relax the horizontal compliance a bit. - 0.20f, 0.10f); + vLineTolerance); } TEST(DisplayListCanvas, DrawRect) { @@ -1331,12 +1612,7 @@ TEST(DisplayListCanvas, DrawDRRect) { TEST(DisplayListCanvas, DrawPath) { SkPath path; - path.moveTo(RenderCenterX, RenderTop); - path.lineTo(RenderRight, RenderBottom); - path.lineTo(RenderLeft, RenderCenterY); - path.lineTo(RenderRight, RenderCenterY); - path.lineTo(RenderLeft, RenderBottom); - path.close(); + path.addRect(RenderBounds); path.moveTo(VerticalMiterDiamondPoints[0]); for (int i = 1; i < VerticalMiterDiamondPointCount; i++) { path.lineTo(VerticalMiterDiamondPoints[i]); @@ -1411,39 +1687,46 @@ TEST(DisplayListCanvas, DrawPointsAsPoints) { CanvasCompareTester::RenderAll( [=](SkCanvas* canvas, SkPaint& paint) { // - canvas->drawPoints(SkCanvas::kPoints_PointMode, count, points, paint); + // Skia requires kStroke style on horizontal and vertical + // lines to get the bounds correct. + // See https://bugs.chromium.org/p/skia/issues/detail?id=12446 + SkPaint p = paint; + p.setStyle(SkPaint::kStroke_Style); + canvas->drawPoints(SkCanvas::kPoints_PointMode, count, points, p); }, [=](DisplayListBuilder& builder) { // - // builder.setCaps(SkPaint::kSquare_Cap); builder.drawPoints(SkCanvas::kPoints_PointMode, count, points); - }); + }, + pointsTolerance); } TEST(DisplayListCanvas, DrawPointsAsLines) { - const SkScalar x0 = RenderLeft; - const SkScalar x1 = (RenderLeft + RenderCenterX) * 0.5; - const SkScalar x2 = RenderCenterX; - const SkScalar x3 = (RenderRight + RenderCenterX) * 0.5; - const SkScalar x4 = RenderRight; + const SkScalar x0 = RenderLeft + 1; + const SkScalar x1 = RenderLeft + 16; + const SkScalar x2 = RenderRight - 16; + const SkScalar x3 = RenderRight - 1; const SkScalar y0 = RenderTop; - const SkScalar y1 = (RenderTop + RenderCenterY) * 0.5; - const SkScalar y2 = RenderCenterY; - const SkScalar y3 = (RenderBottom + RenderCenterY) * 0.5; - const SkScalar y4 = RenderBottom; + const SkScalar y1 = RenderTop + 16; + const SkScalar y2 = RenderBottom - 16; + const SkScalar y3 = RenderBottom; // clang-format off const SkPoint points[] = { + // Outer box + {x0, y0}, {x3, y0}, + {x3, y0}, {x3, y3}, + {x3, y3}, {x0, y3}, + {x0, y3}, {x0, y0}, + // Diagonals - {x0, y0}, {x4, y4}, {x4, y0}, {x0, y4}, + {x0, y0}, {x3, y3}, {x3, y0}, {x0, y3}, + // Inner box - {x1, y1}, {x3, y1}, - {x3, y1}, {x3, y3}, - {x3, y3}, {x1, y3}, - {x1, y3}, {x1, y1}, - // Middle crosshair - {x2, y1}, {x2, y3}, - {x1, y2}, {x3, y3}, + {x1, y1}, {x2, y1}, + {x2, y1}, {x2, y2}, + {x2, y2}, {x1, y2}, + {x1, y2}, {x1, y1}, }; // clang-format on @@ -1451,7 +1734,12 @@ TEST(DisplayListCanvas, DrawPointsAsLines) { ASSERT_TRUE((count & 1) == 0); CanvasCompareTester::RenderAll( [=](SkCanvas* canvas, SkPaint& paint) { // - canvas->drawPoints(SkCanvas::kLines_PointMode, count, points, paint); + // Skia requires kStroke style on horizontal and vertical + // lines to get the bounds correct. + // See https://bugs.chromium.org/p/skia/issues/detail?id=12446 + SkPaint p = paint; + p.setStyle(SkPaint::kStroke_Style); + canvas->drawPoints(SkCanvas::kLines_PointMode, count, points, p); }, [=](DisplayListBuilder& builder) { // builder.drawPoints(SkCanvas::kLines_PointMode, count, points); @@ -1460,46 +1748,53 @@ TEST(DisplayListCanvas, DrawPointsAsLines) { TEST(DisplayListCanvas, DrawPointsAsPolygon) { const SkPoint points1[] = { - // RenderCorner hourglass + // RenderBounds box with a diagonal SkPoint::Make(RenderLeft, RenderTop), - SkPoint::Make(RenderRight, RenderBottom), SkPoint::Make(RenderRight, RenderTop), + SkPoint::Make(RenderRight, RenderBottom), SkPoint::Make(RenderLeft, RenderBottom), SkPoint::Make(RenderLeft, RenderTop), + SkPoint::Make(RenderRight, RenderBottom), }; const int count1 = sizeof(points1) / sizeof(points1[0]); CanvasCompareTester::RenderAll( [=](SkCanvas* canvas, SkPaint& paint) { // - paint.setStyle(SkPaint::kStroke_Style); - canvas->drawPoints(SkCanvas::kPolygon_PointMode, count1, points1, paint); - canvas->drawPoints(SkCanvas::kPolygon_PointMode, VerticalMiterDiamondPointCount, VerticalMiterDiamondPoints, paint); - canvas->drawPoints(SkCanvas::kPolygon_PointMode, HorizontalMiterDiamondPointCount, HorizontalMiterDiamondPoints, paint); + // Skia requires kStroke style on horizontal and vertical + // lines to get the bounds correct. + // See https://bugs.chromium.org/p/skia/issues/detail?id=12446 + SkPaint p = paint; + p.setStyle(SkPaint::kStroke_Style); + canvas->drawPoints(SkCanvas::kPolygon_PointMode, count1, points1, p); }, [=](DisplayListBuilder& builder) { // - builder.setDrawStyle(SkPaint::kStroke_Style); builder.drawPoints(SkCanvas::kPolygon_PointMode, count1, points1); - builder.drawPoints(SkCanvas::kPolygon_PointMode, VerticalMiterDiamondPointCount, VerticalMiterDiamondPoints); - builder.drawPoints(SkCanvas::kPolygon_PointMode, HorizontalMiterDiamondPointCount, HorizontalMiterDiamondPoints); }); } TEST(DisplayListCanvas, DrawVerticesWithColors) { + // Cover as many sides of the box with only 6 vertices: + // +----------+ + // |xxxxxxxxxx| + // | xxxxxx| + // | xxx| + // |xxx | + // |xxxxxx | + // |xxxxxxxxxx| + // +----------| const SkPoint pts[6] = { - SkPoint::Make(RenderCenterX, RenderTop), - SkPoint::Make(RenderLeft, RenderBottom), - SkPoint::Make(RenderRight, RenderBottom), - SkPoint::Make(RenderCenterX, RenderBottom), + // Upper-Right corner, full top, half right coverage SkPoint::Make(RenderLeft, RenderTop), SkPoint::Make(RenderRight, RenderTop), + SkPoint::Make(RenderRight, RenderCenterY), + // Lower-Left corner, full bottom, half left coverage + SkPoint::Make(RenderLeft, RenderBottom), + SkPoint::Make(RenderLeft, RenderCenterY), + SkPoint::Make(RenderRight, RenderBottom), }; const SkColor colors[6] = { - SK_ColorRED, - SK_ColorBLUE, - SK_ColorGREEN, - SK_ColorCYAN, - SK_ColorYELLOW, - SK_ColorMAGENTA, + SK_ColorRED, SK_ColorBLUE, SK_ColorGREEN, + SK_ColorCYAN, SK_ColorYELLOW, SK_ColorMAGENTA, }; const sk_sp vertices = SkVertices::MakeCopy( SkVertices::kTriangles_VertexMode, 6, pts, nullptr, colors); @@ -1514,13 +1809,24 @@ TEST(DisplayListCanvas, DrawVerticesWithColors) { } TEST(DisplayListCanvas, DrawVerticesWithImage) { + // Cover as many sides of the box with only 6 vertices: + // +----------+ + // |xxxxxxxxxx| + // | xxxxxx| + // | xxx| + // |xxx | + // |xxxxxx | + // |xxxxxxxxxx| + // +----------| const SkPoint pts[6] = { - SkPoint::Make(RenderCenterX, RenderTop), - SkPoint::Make(RenderLeft, RenderBottom), - SkPoint::Make(RenderRight, RenderBottom), - SkPoint::Make(RenderCenterX, RenderBottom), + // Upper-Right corner, full top, half right coverage SkPoint::Make(RenderLeft, RenderTop), SkPoint::Make(RenderRight, RenderTop), + SkPoint::Make(RenderRight, RenderCenterY), + // Lower-Left corner, full bottom, half left coverage + SkPoint::Make(RenderLeft, RenderBottom), + SkPoint::Make(RenderLeft, RenderCenterY), + SkPoint::Make(RenderRight, RenderBottom), }; const SkPoint tex[6] = { SkPoint::Make(RenderWidth / 2.0, 0), @@ -1685,16 +1991,20 @@ TEST(DisplayListCanvas, DrawImageLatticeLinear) { TEST(DisplayListCanvas, DrawAtlasNearest) { const SkRSXform xform[] = { - { 1.2f, 0.0f, RenderLeft, RenderTop}, + // clang-format off + { 1.2f, 0.0f, RenderLeft, RenderTop}, { 0.0f, 1.2f, RenderRight, RenderTop}, {-1.2f, 0.0f, RenderRight, RenderBottom}, - { 0.0f, -1.2f, RenderLeft, RenderBottom}, + { 0.0f, -1.2f, RenderLeft, RenderBottom}, + // clang-format on }; const SkRect tex[] = { - SkRect::MakeXYWH(0, 0, RenderHalfWidth, RenderHalfHeight), - SkRect::MakeXYWH(RenderHalfWidth, 0, RenderHalfWidth, RenderHalfHeight), - SkRect::MakeXYWH(RenderHalfWidth, RenderHalfHeight, RenderHalfWidth, RenderHalfHeight), - SkRect::MakeXYWH(0, RenderHalfHeight, RenderHalfWidth, RenderHalfHeight), + // clang-format off + {0, 0, RenderHalfWidth, RenderHalfHeight}, + {RenderHalfWidth, 0, RenderWidth, RenderHalfHeight}, + {RenderHalfWidth, RenderHalfHeight, RenderWidth, RenderHeight}, + {0, RenderHalfHeight, RenderHalfWidth, RenderHeight}, + // clang-format on }; const SkColor colors[] = { SK_ColorBLUE, @@ -1718,16 +2028,20 @@ TEST(DisplayListCanvas, DrawAtlasNearest) { TEST(DisplayListCanvas, DrawAtlasLinear) { const SkRSXform xform[] = { - { 1.2f, 0.0f, RenderLeft, RenderTop}, + // clang-format off + { 1.2f, 0.0f, RenderLeft, RenderTop}, { 0.0f, 1.2f, RenderRight, RenderTop}, {-1.2f, 0.0f, RenderRight, RenderBottom}, - { 0.0f, -1.2f, RenderLeft, RenderBottom}, + { 0.0f, -1.2f, RenderLeft, RenderBottom}, + // clang-format on }; const SkRect tex[] = { - SkRect::MakeXYWH(0, 0, RenderHalfWidth, RenderHalfHeight), - SkRect::MakeXYWH(RenderHalfWidth, 0, RenderHalfWidth, RenderHalfHeight), - SkRect::MakeXYWH(RenderHalfWidth, RenderHalfHeight, RenderHalfWidth, RenderHalfHeight), - SkRect::MakeXYWH(0, RenderHalfHeight, RenderHalfWidth, RenderHalfHeight), + // clang-format off + {0, 0, RenderHalfWidth, RenderHalfHeight}, + {RenderHalfWidth, 0, RenderWidth, RenderHalfHeight}, + {RenderHalfWidth, RenderHalfHeight, RenderWidth, RenderHeight}, + {0, RenderHalfHeight, RenderHalfWidth, RenderHeight}, + // clang-format on }; const SkColor colors[] = { SK_ColorBLUE, @@ -1846,10 +2160,7 @@ TEST(DisplayListCanvas, DrawTextBlob) { GTEST_SKIP() << "Rendering comparisons require a valid default font manager"; #endif // OS_FUCHSIA sk_sp blob = - CanvasCompareTester::MakeTextBlob("Test#Blob", RenderHeight * 0.33f); - FML_LOG(ERROR) << "text blob bounds: " - << blob->bounds().fLeft << ", " << blob->bounds().fTop << " => " - << blob->bounds().fRight << ", " << blob->bounds().fBottom; + CanvasCompareTester::MakeTextBlob("Testing", RenderHeight * 0.33f); SkScalar RenderY1_3 = RenderTop + RenderHeight * 0.33; SkScalar RenderY2_3 = RenderTop + RenderHeight * 0.66; CanvasCompareTester::RenderNoAttributes( @@ -1863,19 +2174,36 @@ TEST(DisplayListCanvas, DrawTextBlob) { builder.drawTextBlob(blob, RenderLeft, RenderY2_3); builder.drawTextBlob(blob, RenderLeft, RenderBottom); }, - 0.25, 0.20); + CanvasCompareTester::DefaultAdjuster, + // From examining the bounds differential for the "Default" case, the + // SkTextBlob adds a padding of ~31 on the left, ~30 on the right, + // ~12 on top and ~8 on the bottom, so we add 32h & 13v allowed + // padding to the tolerance + CanvasCompareTester::DefaultTolerance.addBoundsPadding(32, 13)); +} + +const BoundsTolerance shadowTolerance(const BoundsTolerance& tolerance, + const SkPaint& paint, + const SkMatrix& matrix) { + // Shadow primitives could use just a little more horizontal bounds + // tolerance when drawn with a perspective transform. + return CanvasCompareTester::DefaultAdjuster( + matrix.hasPerspective() ? tolerance.addScale(1.04, 1.0) : tolerance, + paint, matrix); } TEST(DisplayListCanvas, DrawShadow) { SkPath path; - path.moveTo(RenderCenterX, RenderTop); - path.lineTo(RenderRight, RenderBottom); - path.lineTo(RenderLeft, RenderCenterY); - path.lineTo(RenderRight, RenderCenterY); - path.lineTo(RenderLeft, RenderBottom); - path.close(); + path.addRoundRect( + { + RenderLeft + 10, + RenderTop, + RenderRight - 10, + RenderBottom - 20, + }, + RenderCornerRadius, RenderCornerRadius); const SkColor color = SK_ColorDKGRAY; - const SkScalar elevation = 10; + const SkScalar elevation = 5; CanvasCompareTester::RenderShadows( [=](SkCanvas* canvas, SkPaint& paint) { // @@ -1884,19 +2212,23 @@ TEST(DisplayListCanvas, DrawShadow) { }, [=](DisplayListBuilder& builder) { // builder.drawShadow(path, color, elevation, false, 1.0); - }); + }, + shadowTolerance, + CanvasCompareTester::DefaultTolerance.addBoundsPadding(3, 3)); } -TEST(DisplayListCanvas, DrawOccludingShadow) { +TEST(DisplayListCanvas, DrawNonOccludedShadow) { SkPath path; - path.moveTo(RenderCenterX, RenderTop); - path.lineTo(RenderRight, RenderBottom); - path.lineTo(RenderLeft, RenderCenterY); - path.lineTo(RenderRight, RenderCenterY); - path.lineTo(RenderLeft, RenderBottom); - path.close(); + path.addRoundRect( + { + RenderLeft + 10, + RenderTop, + RenderRight - 10, + RenderBottom - 20, + }, + RenderCornerRadius, RenderCornerRadius); const SkColor color = SK_ColorDKGRAY; - const SkScalar elevation = 10; + const SkScalar elevation = 5; CanvasCompareTester::RenderShadows( [=](SkCanvas* canvas, SkPaint& paint) { // @@ -1905,28 +2237,34 @@ TEST(DisplayListCanvas, DrawOccludingShadow) { }, [=](DisplayListBuilder& builder) { // builder.drawShadow(path, color, elevation, true, 1.0); - }); + }, + shadowTolerance, + CanvasCompareTester::DefaultTolerance.addBoundsPadding(3, 3)); } TEST(DisplayListCanvas, DrawShadowDpr) { SkPath path; - path.moveTo(RenderCenterX, RenderTop); - path.lineTo(RenderRight, RenderBottom); - path.lineTo(RenderLeft, RenderCenterY); - path.lineTo(RenderRight, RenderCenterY); - path.lineTo(RenderLeft, RenderBottom); - path.close(); + path.addRoundRect( + { + RenderLeft + 10, + RenderTop, + RenderRight - 10, + RenderBottom - 20, + }, + RenderCornerRadius, RenderCornerRadius); const SkColor color = SK_ColorDKGRAY; - const SkScalar elevation = 10; + const SkScalar elevation = 5; CanvasCompareTester::RenderShadows( [=](SkCanvas* canvas, SkPaint& paint) { // PhysicalShapeLayer::DrawShadow(canvas, path, color, elevation, false, - 2.5); + 1.5); }, [=](DisplayListBuilder& builder) { // - builder.drawShadow(path, color, elevation, false, 2.5); - }); + builder.drawShadow(path, color, elevation, false, 1.5); + }, + shadowTolerance, + CanvasCompareTester::DefaultTolerance.addBoundsPadding(3, 3)); } } // namespace testing diff --git a/flow/display_list_utils.cc b/flow/display_list_utils.cc index ad90db7b40aa9..58b6ab889047a 100644 --- a/flow/display_list_utils.cc +++ b/flow/display_list_utils.cc @@ -194,7 +194,7 @@ void ClipBoundsDispatchHelper::restore() { } } void ClipBoundsDispatchHelper::reset(const SkRect* cull_rect) { - if ((has_clip_ = ((bool) cull_rect))) { + if ((has_clip_ = ((bool)cull_rect)) && !cull_rect->isEmpty()) { bounds_ = *cull_rect; } else { bounds_.setEmpty(); @@ -410,19 +410,13 @@ void DisplayListBoundsCalculator::drawAtlas(const sk_sp atlas, const SkRect* cullRect) { SkPoint quad[4]; BoundsAccumulator atlasBounds; - // FML_LOG(ERROR) << "atlas quads: {"; for (int i = 0; i < count; i++) { const SkRect& src = tex[i]; - // FML_LOG(ERROR) << " rect " << (i + 1) << ": " << src.fLeft << ", " << src.fTop << " => " << src.fRight << ", " << src.fBottom; xform[i].toQuad(src.width(), src.height(), quad); - // FML_LOG(ERROR) << " quad " << (i + 1) << ": {"; for (int j = 0; j < 4; j++) { - // FML_LOG(ERROR) << " " << quad[j].fX << ", " << quad[j].fY; atlasBounds.accumulate(quad[j]); } - // FML_LOG(ERROR) << " }"; } - // FML_LOG(ERROR) << "}"; if (atlasBounds.isNotEmpty()) { accumulateRect(atlasBounds.getBounds(), kIsNonGeometric); } @@ -451,7 +445,7 @@ void DisplayListBoundsCalculator::drawTextBlob(const sk_sp blob, void DisplayListBoundsCalculator::drawShadow(const SkPath& path, const SkColor color, const SkScalar elevation, - bool occludes, + bool transparentOccluder, SkScalar dpr) { SkRect shadow_bounds = PhysicalShapeLayer::ComputeShadowBounds(path, elevation, dpr, matrix()); @@ -484,14 +478,25 @@ bool DisplayListBoundsCalculator::adjustBoundsForPaint(SkRect& bounds, // Path effect occurs before stroking... if (path_effect_) { - SkPaint p; - p.setPathEffect(path_effect_); - if (!p.canComputeFastBounds()) { - return false; + SkPathEffect::DashInfo info; + if (path_effect_->asADash(&info) == SkPathEffect::kDash_DashType) { + // A dash effect has a very simple impact. It cannot introduce any + // miter joins that weren't already present in the original path + // and it does not grow the bounds of the path, but it can add + // end caps to areas that might not have had them before so all + // we need to do is to indicate the potential for diagonal + // end caps and move on. + flags |= kGeometryMayHaveDiagonalEndCaps; + } else { + SkPaint p; + p.setPathEffect(path_effect_); + if (!p.canComputeFastBounds()) { + return false; + } + bounds = p.computeFastBounds(bounds, &bounds); + flags |= (kGeometryMayHaveDiagonalEndCaps | + kGeometryMayHaveProblematicJoins); } - bounds = p.computeFastBounds(bounds, &bounds); - flags |= kGeometryMayHaveDiagonalEndCaps | - kGeometryMayHaveProblematicJoins; } if ((flags & kIsStrokedGeometry) != 0) { diff --git a/flow/display_list_utils.h b/flow/display_list_utils.h index fa4cba9946a8c..4a0a7d6f32fa0 100644 --- a/flow/display_list_utils.h +++ b/flow/display_list_utils.h @@ -343,7 +343,7 @@ class DisplayListBoundsCalculator final void drawShadow(const SkPath& path, const SkColor color, const SkScalar elevation, - bool occludes, + bool transparentOccluder, SkScalar dpr) override; bool isFlooded() { From 7f54d566e5e1b778ec2952908ac67281c51ac117 Mon Sep 17 00:00:00 2001 From: Jim Graham Date: Thu, 16 Sep 2021 19:55:59 -0700 Subject: [PATCH 6/6] feedback from review, naming, final modifiers, abbreviations in strings --- flow/display_list_canvas_unittests.cc | 52 ++++++----- flow/display_list_utils.cc | 26 +++--- flow/display_list_utils.h | 123 ++++++++++++++++---------- 3 files changed, 121 insertions(+), 80 deletions(-) diff --git a/flow/display_list_canvas_unittests.cc b/flow/display_list_canvas_unittests.cc index 992b19ba19a42..2f95c55122cb6 100644 --- a/flow/display_list_canvas_unittests.cc +++ b/flow/display_list_canvas_unittests.cc @@ -50,7 +50,10 @@ constexpr SkRect RenderBounds = // The tests try 3 miter limit values, 0.0, 4.0 (the default), and 10.0 // These values will allow us to construct a diamond that spans the // width or height of the render box and still show the miter for 4.0 -// and 10.0 +// and 10.0. +// These values were discovered by drawing a diamond path in Skia fiddle +// and then playing with the cross-axis size until the miter was about +// as large as it could get before it got cut off. // The X offsets which will be used for tall vertical diamonds are // expressed in terms of the rendering height to obtain the proper angle @@ -436,7 +439,8 @@ class CanvasCompareTester { cv_restored, dl_restored, adjuster, blur5Tolerance, "saveLayer ImageFilter, no bounds"); } - ASSERT_TRUE(filter->unique()) << "SL with IF no bounds Cleanup"; + ASSERT_TRUE(filter->unique()) + << "saveLayer ImageFilter, no bounds Cleanup"; { RenderWith( [=](SkCanvas* cv, SkPaint& p) { @@ -454,7 +458,8 @@ class CanvasCompareTester { cv_restored, dl_restored, adjuster, blur5Tolerance, "saveLayer ImageFilter and bounds"); } - ASSERT_TRUE(filter->unique()) << "SL with IF and bounds Cleanup"; + ASSERT_TRUE(filter->unique()) + << "saveLayer ImageFilter and bounds Cleanup"; } } @@ -468,10 +473,12 @@ class CanvasCompareTester { RenderWith([=](SkCanvas*, SkPaint& p) { p.setAntiAlias(true); }, // [=](DisplayListBuilder& b) { b.setAA(true); }, // - cv_renderer, dl_renderer, adjuster, tolerance, "AA == True"); + cv_renderer, dl_renderer, adjuster, tolerance, + "AntiAlias == True"); RenderWith([=](SkCanvas*, SkPaint& p) { p.setAntiAlias(false); }, // [=](DisplayListBuilder& b) { b.setAA(false); }, // - cv_renderer, dl_renderer, adjuster, tolerance, "AA == False"); + cv_renderer, dl_renderer, adjuster, tolerance, + "AntiAlias == False"); RenderWith([=](SkCanvas*, SkPaint& p) { p.setDither(true); }, // [=](DisplayListBuilder& b) { b.setDither(true); }, // @@ -606,7 +613,7 @@ class CanvasCompareTester { cv_renderer, dl_renderer, adjuster, tolerance, "ColorFilter == RotateRGB", &bg); } - ASSERT_TRUE(filter->unique()) << "ColorFilter Cleanup"; + ASSERT_TRUE(filter->unique()) << "ColorFilter == RotateRGB Cleanup"; filter = SkColorFilters::Matrix(invert_color_matrix); { SkColor bg = SK_ColorWHITE; @@ -622,7 +629,7 @@ class CanvasCompareTester { cv_renderer, dl_renderer, adjuster, tolerance, "ColorFilter == Invert", &bg); } - ASSERT_TRUE(filter->unique()) << "ColorFilter Cleanup"; + ASSERT_TRUE(filter->unique()) << "ColorFilter == Invert Cleanup"; } { @@ -654,7 +661,7 @@ class CanvasCompareTester { .addBoundsPadding(2, 2), "PathEffect == Discrete-3-5"); } - ASSERT_TRUE(effect->unique()) << "PathEffect Cleanup"; + ASSERT_TRUE(effect->unique()) << "PathEffect == Discrete-3-5 Cleanup"; effect = SkDiscretePathEffect::Make(2, 3); { RenderWith( @@ -681,7 +688,7 @@ class CanvasCompareTester { .addBoundsPadding(2, 2), "PathEffect == Discrete-2-3"); } - ASSERT_TRUE(effect->unique()) << "PathEffect Cleanup"; + ASSERT_TRUE(effect->unique()) << "PathEffect == Discrete-2-3 Cleanup"; } { @@ -703,7 +710,7 @@ class CanvasCompareTester { cv_renderer, dl_renderer, adjuster, blur5Tolerance, "MaskFilter == Blur 5"); } - ASSERT_TRUE(filter->unique()) << "MaskFilter Cleanup"; + ASSERT_TRUE(filter->unique()) << "MaskFilter == Blur 5 Cleanup"; { RenderWith( [=](SkCanvas*, SkPaint& p) { @@ -719,7 +726,8 @@ class CanvasCompareTester { cv_renderer, dl_renderer, adjuster, blur5Tolerance, "MaskFilter == Blur(Normal, 5.0)"); } - ASSERT_TRUE(filter->unique()) << "MaskFilter Cleanup"; + ASSERT_TRUE(filter->unique()) + << "MaskFilter == Blur(Normal, 5.0) Cleanup"; } { @@ -745,7 +753,7 @@ class CanvasCompareTester { cv_renderer, dl_renderer, adjuster, tolerance, "LinearGradient GYB"); } - ASSERT_TRUE(shader->unique()) << "Shader Cleanup"; + ASSERT_TRUE(shader->unique()) << "LinearGradient GYB Cleanup"; } } @@ -915,7 +923,7 @@ class CanvasCompareTester { cv_renderer, dl_renderer, adjuster, tolerance, "PathEffect == Dash-29-2"); } - ASSERT_TRUE(effect->unique()) << "PathEffect Cleanup"; + ASSERT_TRUE(effect->unique()) << "PathEffect == Dash-29-2 Cleanup"; effect = SkDashPathEffect::Make(TestDashes2, 2, 0.0f); { RenderWith( @@ -936,7 +944,7 @@ class CanvasCompareTester { cv_renderer, dl_renderer, adjuster, tolerance, "PathEffect == Dash-17-1.5"); } - ASSERT_TRUE(effect->unique()) << "PathEffect Cleanup"; + ASSERT_TRUE(effect->unique()) << "PathEffect == Dash-17-1.5 Cleanup"; } } @@ -1016,7 +1024,7 @@ class CanvasCompareTester { b.clipRect(r_clip, true, SkClipOp::kIntersect); }, cv_renderer, dl_renderer, intersect_adjuster, intersect_tolerance, - "AA ClipRect inset by 15.5"); + "AntiAlias ClipRect inset by 15.5"); RenderWith( [=](SkCanvas* c, SkPaint&) { c->clipRect(r_clip, SkClipOp::kDifference, false); @@ -1044,7 +1052,7 @@ class CanvasCompareTester { b.clipRRect(rr_clip, true, SkClipOp::kIntersect); }, cv_renderer, dl_renderer, intersect_adjuster, intersect_tolerance, - "AA ClipRRect inset by 15.5"); + "AntiAlias ClipRRect inset by 15.5"); RenderWith( [=](SkCanvas* c, SkPaint&) { c->clipRRect(rr_clip, SkClipOp::kDifference, false); @@ -1075,7 +1083,7 @@ class CanvasCompareTester { b.clipPath(path_clip, true, SkClipOp::kIntersect); }, cv_renderer, dl_renderer, intersect_adjuster, intersect_tolerance, - "AA ClipPath inset by 15.5"); + "AntiAlias ClipPath inset by 15.5"); RenderWith( [=](SkCanvas* c, SkPaint&) { c->clipPath(path_clip, SkClipOp::kDifference, false); @@ -1121,7 +1129,7 @@ class CanvasCompareTester { ASSERT_EQ(ref_pixels.width(), TestWidth) << info; ASSERT_EQ(ref_pixels.height(), TestHeight) << info; ASSERT_EQ(ref_pixels.info().bytesPerPixel(), 4) << info; - checkPixels(&ref_pixels, ref_bounds, info, bg); + checkPixels(&ref_pixels, ref_bounds, info + " (Skia reference)", bg); { // This sequence plays the provided equivalently constructed @@ -1159,7 +1167,8 @@ class CanvasCompareTester { << info; display_list->RenderTo(test_surface->getCanvas()); - compareToReference(test_surface.get(), &ref_pixels, info + " (DL render)", + compareToReference(test_surface.get(), &ref_pixels, + info + " (DisplayList built directly -> surface)", &dl_bounds, &tolerance, bg); } @@ -1176,7 +1185,8 @@ class CanvasCompareTester { cv_render(&dl_recorder, test_paint); dl_recorder.builder()->Build()->RenderTo(test_surface->getCanvas()); compareToReference(test_surface.get(), &ref_pixels, - info + " (Sk->DL render)", nullptr, nullptr, nullptr); + info + " (Skia calls -> DisplayList -> surface)", + nullptr, nullptr, nullptr); } { @@ -1214,7 +1224,7 @@ class CanvasCompareTester { test_canvas->scale(TestScale, TestScale); display_list->RenderTo(test_canvas); compareToReference(test_surface.get(), &ref_pixels2, - info + " (SKP/DL render scaled 2x)", nullptr, nullptr, + info + " (Both rendered scaled 2x)", nullptr, nullptr, nullptr, TestWidth2, TestHeight2, false); } } diff --git a/flow/display_list_utils.cc b/flow/display_list_utils.cc index 58b6ab889047a..353a457f430b3 100644 --- a/flow/display_list_utils.cc +++ b/flow/display_list_utils.cc @@ -278,13 +278,13 @@ void DisplayListBoundsCalculator::restore() { ClipBoundsDispatchHelper::restore(); accumulator_ = layer_infos_.back()->accumulatorForRestore(); SkRect layer_bounds = layer_infos_.back()->getLayerBounds(); - // Must read flooded state after layer_bounds - bool layer_flooded = layer_infos_.back()->is_flooded(); + // Must read unbounded state after layer_bounds + bool layer_unbounded = layer_infos_.back()->is_unbounded(); layer_infos_.pop_back(); - // We accumulate the bounds even if the layer was flooded because - // the flooding may become a NOP, so we at least accumulate our - // best estimate about what we have. + // 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()) { // kUnfiltered because the layer already applied all bounds // modifications based on the attributes that were in place @@ -292,17 +292,17 @@ void DisplayListBoundsCalculator::restore() { // current attributes would mix attribute states. accumulateRect(layer_bounds, kIsUnfiltered); } - if (layer_flooded) { - accumulateFlood(); + if (layer_unbounded) { + accumulateUnbounded(); } } } void DisplayListBoundsCalculator::drawPaint() { - accumulateFlood(); + accumulateUnbounded(); } void DisplayListBoundsCalculator::drawColor(SkColor color, SkBlendMode mode) { - accumulateFlood(); + accumulateUnbounded(); } void DisplayListBoundsCalculator::drawLine(const SkPoint& p0, const SkPoint& p1) { @@ -334,7 +334,7 @@ void DisplayListBoundsCalculator::drawDRRect(const SkRRect& outer, } void DisplayListBoundsCalculator::drawPath(const SkPath& path) { if (path.isInverseFillType()) { - accumulateFlood(); + accumulateUnbounded(); } else { accumulateRect(path.getBounds(), // (kIsDrawnGeometry | // @@ -537,11 +537,11 @@ bool DisplayListBoundsCalculator::adjustBoundsForPaint(SkRect& bounds, return getFilteredBounds(bounds, image_filter_.get()); } -void DisplayListBoundsCalculator::accumulateFlood() { +void DisplayListBoundsCalculator::accumulateUnbounded() { if (has_clip()) { accumulator_->accumulate(getClipBounds()); } else { - layer_infos_.back()->set_flooded(); + layer_infos_.back()->set_unbounded(); } } void DisplayListBoundsCalculator::accumulateRect(SkRect& rect, int flags) { @@ -551,7 +551,7 @@ void DisplayListBoundsCalculator::accumulateRect(SkRect& rect, int flags) { accumulator_->accumulate(rect); } } else { - accumulateFlood(); + accumulateUnbounded(); } } diff --git a/flow/display_list_utils.h b/flow/display_list_utils.h index 4a0a7d6f32fa0..4973f83f5508f 100644 --- a/flow/display_list_utils.h +++ b/flow/display_list_utils.h @@ -266,9 +266,9 @@ class DisplayListBoundsCalculator final // DisplayList dispatcher method calls. Since 2 of the method calls // have no intrinsic size because they flood the entire clip/surface, // the |cull_rect| provides a bounds for them to include. If cull_rect - // is not specified or is null, then the flooding calls will not - // affect the resulting bounds, but will set the is_flooded flag - // below which can be inspected if an alternate plan is available + // is not specified or is null, then the unbounded calls will not + // affect the resulting bounds, but will set a flag that can be + // queried using |isUnbounded| if an alternate plan is available // for such cases. // The flag should never be set if a cull_rect is provided. DisplayListBoundsCalculator(const SkRect* cull_rect = nullptr); @@ -346,15 +346,24 @@ class DisplayListBoundsCalculator final bool transparentOccluder, SkScalar dpr) override; - bool isFlooded() { + // The DisplayList had an unbounded call with no cull rect or clip + // to contain it. Should only be called after the stream is fully + // dispatched. + // Unbounded operations are calls like |drawColor| which are defined + // to flood the entire surface, or calls that relied on a rendering + // attribute which is unable to compute bounds (should be rare). + // In those cases the bounds will represent only the accumulation + // of the bounded calls and this flag will be set to indicate that + // condition. + bool isUnbounded() const { FML_DCHECK(layer_infos_.size() == 1); - return layer_infos_.front()->is_flooded(); + return layer_infos_.front()->is_unbounded(); } - SkRect getBounds() { + SkRect getBounds() const { FML_DCHECK(layer_infos_.size() == 1); - if (isFlooded()) { - FML_LOG(INFO) << "returning partial bounds for flooded DisplayList"; + if (isUnbounded()) { + FML_LOG(INFO) << "returning partial bounds for unbounded DisplayList"; } return accumulator_->getBounds(); } @@ -370,47 +379,67 @@ class DisplayListBoundsCalculator final // See |RootLayerData|, |SaveData| and |SaveLayerData|. class LayerData { public: - LayerData(BoundsAccumulator* outer) : outer_(outer), is_flooded_(false) {} + // 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. + LayerData(BoundsAccumulator* outer) : outer_(outer), is_unbounded_(false) {} virtual ~LayerData() = default; + // The accumulator to use while this layer is put in play by + // a |save| or |saveLayer| virtual BoundsAccumulator* accumulatorForLayer() { return outer_; } + + // The accumulator to use after this layer is removed from play + // via |restore| virtual BoundsAccumulator* accumulatorForRestore() { return outer_; } - virtual SkRect getLayerBounds() { return SkRect::MakeEmpty(); } - - // is_flooded is to true if we ever encounter an operation - // on a layer that is unbounded (|drawColor| or |drawPaint|) - // or cannot compute its bounds (some effects and filters) - // and there was no outstanding clip op at the time. - // When the layer is restored, the outer layer may then - // process this flooded state by accumulating its own - // clip or recording the flood state to pass to its own - // outer layer. - // Typically the DisplayList will have been constructed with - // a cull rect which will act as a default clip for the - // outermost layer and the flood state of the overall + + // 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 getLayerBounds() = 0; + + // is_unbounded should be set to true if we ever encounter an operation + // on a layer that either is unrestricted (|drawColor| or |drawPaint|) + // or cannot compute its bounds (some effects and filters) and there + // was no outstanding clip op at the time. + // When the layer is restored, the outer layer may then process this + // unbounded state by accumulating its own clip or transferring the + // unbounded state to its own outer layer. + // Typically the DisplayList will have been constructed with a cull + // rect which will act as a default clip for the outermost layer and + // the unbounded state of all sub layers will eventually be caught by + // that cull rect so that the overall unbounded state of the entire // DisplayList will never be true. // - // SkPicture treats these same conditions as a Nop (they - // accumulate the SkPicture cull rect, but if it was not - // specified then it is an empty Rect and so has no - // effect on the bounds). - // If the Calculator object accumulates this flag into the - // root layer, then at least we can make the caller aware - // of that exceptional condition. + // SkPicture treats these same conditions as a Nop (they accumulate + // the SkPicture cull rect, but if it was not specified then it is an + // empty Rect and so has no effect on the bounds). + // If the Calculator object accumulates this flag into the root layer, + // then at least we can make the caller aware of that exceptional + // condition via the |DisplayListBoundsCalculator::isUnbounded| call. // - // Flutter is unlikely to ever run into this as the Dart - // mechanisms all supply a cull rect for all Dart Picture - // objects, even if that cull rect is kGiantRect. - void set_flooded() { is_flooded_ = true; } + // Flutter is unlikely to ever run into this as the Dart mechanisms + // all supply a non-null cull rect for all Dart Picture objects, + // even if that cull rect is kGiantRect. + void set_unbounded() { is_unbounded_ = true; } - // |is_flooded| should be called after |getLayerBounds| in case + // |is_unbounded| should be called after |getLayerBounds| in case // a problem was found during the computation of those bounds, - // the layer will have one last chance to flag a flooded state. - bool is_flooded() { return is_flooded_; } + // the layer will have one last chance to flag an unbounded state. + bool is_unbounded() const { return is_unbounded_; } private: BoundsAccumulator* outer_; - bool is_flooded_; + bool is_unbounded_; FML_DISALLOW_COPY_AND_ASSIGN(LayerData); }; @@ -425,9 +454,9 @@ class DisplayListBoundsCalculator final } SkRect getLayerBounds() override { - // Even though this layer might have been flooded, we still - // accumulate what bounds we have as the flooded condition - // may be a nop at a higher level and we at least want to + // 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_.getBounds(); } @@ -441,7 +470,7 @@ class DisplayListBoundsCalculator final }; // Used as the initial default layer info for the Calculator. - class RootLayerData : public AccumulatorLayerData { + class RootLayerData final : public AccumulatorLayerData { public: RootLayerData() : AccumulatorLayerData(nullptr) {} ~RootLayerData() = default; @@ -451,24 +480,26 @@ class DisplayListBoundsCalculator final }; // Used for |save| - class SaveData : public LayerData { + class SaveData final : public LayerData { public: using LayerData::LayerData; ~SaveData() = default; + SkRect getLayerBounds() override { return SkRect::MakeEmpty(); } + private: FML_DISALLOW_COPY_AND_ASSIGN(SaveData); }; // Used for |saveLayer| - class SaveLayerData : public AccumulatorLayerData { + 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_flooded(); + set_unbounded(); } } ~SaveLayerData() = default; @@ -476,7 +507,7 @@ class DisplayListBoundsCalculator final SkRect getLayerBounds() override { SkRect bounds = AccumulatorLayerData::getLayerBounds(); if (!getFilteredBounds(bounds, layer_filter_.get())) { - set_flooded(); + set_unbounded(); } return bounds; } @@ -564,7 +595,7 @@ class DisplayListBoundsCalculator final static bool getFilteredBounds(SkRect& rect, SkImageFilter* filter); bool adjustBoundsForPaint(SkRect& bounds, int flags); - void accumulateFlood(); + void accumulateUnbounded(); void accumulateRect(const SkRect& rect, int flags) { SkRect bounds = rect; accumulateRect(bounds, flags);