From e372d6536d56e1762e658325b6fe65b984a32fa6 Mon Sep 17 00:00:00 2001 From: Jonah Williams Date: Wed, 29 May 2024 09:45:22 -0700 Subject: [PATCH 1/2] [Impeller] relax conditions for SkRRect.isSimple conversion to impeller::RRect. (#53083) The flickering in https://github.com/flutter/flutter/issues/148412 is caused by us switching between the RRect fast path and a gaussian blur. The reason is that the SkRect.isSimple check doesn't handle fp precision very well. On one of the frames the difference was : ``` D/skia (18362): SkRect::MakeLTRB(74, 179.666672f, 374, 479.666656f); D/skia (18362): const SkPoint corners[] = { D/skia (18362): { 150, 149.999969f }, D/skia (18362): { 150, 150 }, D/skia (18362): { 150, 149.999969f }, D/skia (18362): { 150, 150 }, D/skia (18362): }; ``` So lets used a relaxed check for RRect.isSimple instead. Fixes https://github.com/flutter/flutter/issues/148412 --- impeller/display_list/dl_dispatcher.cc | 4 ++-- impeller/display_list/skia_conversions.cc | 14 ++++++++++++++ impeller/display_list/skia_conversions.h | 7 +++++++ .../display_list/skia_conversions_unittests.cc | 10 ++++++++++ 4 files changed, 33 insertions(+), 2 deletions(-) diff --git a/impeller/display_list/dl_dispatcher.cc b/impeller/display_list/dl_dispatcher.cc index 8be3b9daf39bf..71fbdc52019c5 100644 --- a/impeller/display_list/dl_dispatcher.cc +++ b/impeller/display_list/dl_dispatcher.cc @@ -814,8 +814,8 @@ void DlDispatcher::drawCircle(const SkPoint& center, SkScalar radius) { } // |flutter::DlOpReceiver| -void DlDispatcher::drawRRect(const SkRRect& rrect) { - if (rrect.isSimple()) { +void DlDispatcherBase::drawRRect(const SkRRect& rrect) { + if (skia_conversions::IsNearlySimpleRRect(rrect)) { canvas_.DrawRRect(skia_conversions::ToRect(rrect.rect()), skia_conversions::ToSize(rrect.getSimpleRadii()), paint_); } else { diff --git a/impeller/display_list/skia_conversions.cc b/impeller/display_list/skia_conversions.cc index f356e9973a57a..8ac11c81a2803 100644 --- a/impeller/display_list/skia_conversions.cc +++ b/impeller/display_list/skia_conversions.cc @@ -9,6 +9,20 @@ namespace impeller { namespace skia_conversions { +bool IsNearlySimpleRRect(const SkRRect& rr) { + auto [a, b] = rr.radii(SkRRect::kUpperLeft_Corner); + auto [c, d] = rr.radii(SkRRect::kLowerLeft_Corner); + auto [e, f] = rr.radii(SkRRect::kUpperRight_Corner); + auto [g, h] = rr.radii(SkRRect::kLowerRight_Corner); + return SkScalarNearlyEqual(a, b, kEhCloseEnough) && + SkScalarNearlyEqual(a, c, kEhCloseEnough) && + SkScalarNearlyEqual(a, d, kEhCloseEnough) && + SkScalarNearlyEqual(a, e, kEhCloseEnough) && + SkScalarNearlyEqual(a, f, kEhCloseEnough) && + SkScalarNearlyEqual(a, g, kEhCloseEnough) && + SkScalarNearlyEqual(a, h, kEhCloseEnough); +} + Rect ToRect(const SkRect& rect) { return Rect::MakeLTRB(rect.fLeft, rect.fTop, rect.fRight, rect.fBottom); } diff --git a/impeller/display_list/skia_conversions.h b/impeller/display_list/skia_conversions.h index a0fe839d29b87..a52f31236eec9 100644 --- a/impeller/display_list/skia_conversions.h +++ b/impeller/display_list/skia_conversions.h @@ -24,6 +24,13 @@ namespace impeller { namespace skia_conversions { +/// @brief Like SkRRect.isSimple, but allows the corners to differ by +/// kEhCloseEnough. +/// +/// An RRect is simple if all corner radii are approximately +/// equal. +bool IsNearlySimpleRRect(const SkRRect& rr); + Rect ToRect(const SkRect& rect); std::optional ToRect(const SkRect* rect); diff --git a/impeller/display_list/skia_conversions_unittests.cc b/impeller/display_list/skia_conversions_unittests.cc index e31005e322c9f..257f2039d8ea1 100644 --- a/impeller/display_list/skia_conversions_unittests.cc +++ b/impeller/display_list/skia_conversions_unittests.cc @@ -7,6 +7,7 @@ #include "flutter/testing/testing.h" #include "impeller/display_list/skia_conversions.h" #include "impeller/geometry/scalar.h" +#include "include/core/SkRRect.h" namespace impeller { namespace testing { @@ -174,5 +175,14 @@ TEST(SkiaConversionsTest, GradientConversionNonMonotonic) { ASSERT_TRUE(ScalarNearlyEqual(converted_stops[3], 1.0f)); } +TEST(SkiaConversionsTest, IsNearlySimpleRRect) { + EXPECT_TRUE(skia_conversions::IsNearlySimpleRRect( + SkRRect::MakeRectXY(SkRect::MakeLTRB(0, 0, 10, 10), 10, 10))); + EXPECT_TRUE(skia_conversions::IsNearlySimpleRRect( + SkRRect::MakeRectXY(SkRect::MakeLTRB(0, 0, 10, 10), 10, 9.999))); + EXPECT_FALSE(skia_conversions::IsNearlySimpleRRect( + SkRRect::MakeRectXY(SkRect::MakeLTRB(0, 0, 10, 10), 10, 9))); +} + } // namespace testing } // namespace impeller From f8e6d199c50caeed37185956fa80b2bef093d9d5 Mon Sep 17 00:00:00 2001 From: Jonah Williams Date: Tue, 4 Jun 2024 18:55:25 -0700 Subject: [PATCH 2/2] Update dl_dispatcher.cc --- impeller/display_list/dl_dispatcher.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/impeller/display_list/dl_dispatcher.cc b/impeller/display_list/dl_dispatcher.cc index 71fbdc52019c5..afafad4d3a1fb 100644 --- a/impeller/display_list/dl_dispatcher.cc +++ b/impeller/display_list/dl_dispatcher.cc @@ -814,7 +814,7 @@ void DlDispatcher::drawCircle(const SkPoint& center, SkScalar radius) { } // |flutter::DlOpReceiver| -void DlDispatcherBase::drawRRect(const SkRRect& rrect) { +void DlDispatcher::drawRRect(const SkRRect& rrect) { if (skia_conversions::IsNearlySimpleRRect(rrect)) { canvas_.DrawRRect(skia_conversions::ToRect(rrect.rect()), skia_conversions::ToSize(rrect.getSimpleRadii()), paint_);