From cb0e649e78368082b1b16ecabef0b5ba14b32ae3 Mon Sep 17 00:00:00 2001 From: Jim Graham Date: Thu, 3 Apr 2025 13:06:23 -0700 Subject: [PATCH] [Impeller] Render conics without conversion from Flutter apps (#166305) Now that Impeller performs high fidelity tessellation of Conic curves we will no longer convert Flutter app's conic curves into approximated quadratic curves. --- .../flutter/display_list/geometry/dl_path.cc | 9 +- .../geometry/dl_path_unittests.cc | 125 ++++++++++++++---- .../skia/dl_sk_conversions_unittests.cc | 46 ++----- .../src/flutter/display_list/testing/BUILD.gn | 1 + .../testing/dl_test_mock_path_receiver.h | 42 ++++++ .../impeller/geometry/path_component.cc | 4 +- .../flutter/testing/display_list_testing.cc | 84 ++++++++++++ .../flutter/testing/display_list_testing.h | 34 +++++ 8 files changed, 281 insertions(+), 64 deletions(-) create mode 100644 engine/src/flutter/display_list/testing/dl_test_mock_path_receiver.h diff --git a/engine/src/flutter/display_list/geometry/dl_path.cc b/engine/src/flutter/display_list/geometry/dl_path.cc index 98751b0d72..f18f154dea 100644 --- a/engine/src/flutter/display_list/geometry/dl_path.cc +++ b/engine/src/flutter/display_list/geometry/dl_path.cc @@ -5,8 +5,8 @@ #include "flutter/display_list/geometry/dl_path.h" #include "flutter/display_list/geometry/dl_geometry_types.h" +#include "flutter/impeller/geometry/path.h" #include "flutter/impeller/geometry/path_builder.h" -#include "impeller/geometry/path.h" namespace { inline constexpr flutter::DlPathFillType ToDlFillType(SkPathFillType sk_type) { @@ -452,9 +452,10 @@ class ImpellerPathReceiver final : public DlPathReceiver { void QuadTo(const DlPoint& cp, const DlPoint& p2) override { builder_.QuadraticCurveTo(cp, p2); } - // For legacy compatibility we do not override ConicTo to let the dispatcher - // convert conics to quads until we update Impeller for full support of - // rational quadratics + bool ConicTo(const DlPoint& cp, const DlPoint& p2, DlScalar weight) override { + builder_.ConicCurveTo(cp, p2, weight); + return true; + } void CubicTo(const DlPoint& cp1, const DlPoint& cp2, const DlPoint& p2) override { diff --git a/engine/src/flutter/display_list/geometry/dl_path_unittests.cc b/engine/src/flutter/display_list/geometry/dl_path_unittests.cc index 79b03cb52a..97d179cec5 100644 --- a/engine/src/flutter/display_list/geometry/dl_path_unittests.cc +++ b/engine/src/flutter/display_list/geometry/dl_path_unittests.cc @@ -4,9 +4,9 @@ #include "flutter/display_list/geometry/dl_path.h" -#include "gmock/gmock.h" #include "gtest/gtest.h" +#include "flutter/display_list/testing/dl_test_mock_path_receiver.h" #include "flutter/third_party/skia/include/core/SkRRect.h" namespace flutter { @@ -595,31 +595,6 @@ TEST(DisplayListPath, IsLineFromImpellerPath) { } namespace { -class DlPathReceiverMock : public DlPathReceiver { - public: - MOCK_METHOD(void, - RecommendSizes, - (size_t verb_count, size_t point_count), - (override)); - MOCK_METHOD(void, RecommendBounds, (const DlRect& bounds), (override)); - MOCK_METHOD(void, - SetPathInfo, - (DlPathFillType fill_type, bool is_convex), - (override)); - MOCK_METHOD(void, MoveTo, (const DlPoint& p2), (override)); - MOCK_METHOD(void, LineTo, (const DlPoint& p2), (override)); - MOCK_METHOD(void, QuadTo, (const DlPoint& cp, const DlPoint& p2), (override)); - MOCK_METHOD(bool, - ConicTo, - (const DlPoint& cp, const DlPoint& p2, DlScalar weight), - (override)); - MOCK_METHOD(void, - CubicTo, - (const DlPoint& cp1, const DlPoint& cp2, const DlPoint& p2), - (override)); - MOCK_METHOD(void, Close, (), (override)); -}; - using ::testing::AtMost; using ::testing::Return; } // namespace @@ -925,6 +900,104 @@ TEST(DisplayListPath, DispatchImpellerPathConvexSpecified) { path.Dispatch(mock_receiver); } +TEST(DisplayListPath, DispatchSkiaPathConicToQuads) { + // If we execute conicTo with a weight of exactly 1.0, SkPath will turn + // it into a quadTo, so we avoid that by using 0.999 + SkScalar weights[4] = { + 0.02f, + 0.5f, + SK_ScalarSqrt2 * 0.5f, + 1.0f - kEhCloseEnough, + }; + + for (SkScalar weight : weights) { + SkPath sk_path; + sk_path.moveTo(10, 10); + sk_path.conicTo(20, 10, 20, 20, weight); + + std::array i_points; + impeller::ConicPathComponent i_conic(DlPoint(10, 10), DlPoint(20, 10), + DlPoint(20, 20), weight); + i_conic.SubdivideToQuadraticPoints(i_points); + + ::testing::StrictMock mock_receiver; + + // Recommendations must happen before any of the path segments is dispatched + ::testing::ExpectationSet all_recommendations; + all_recommendations += // + EXPECT_CALL(mock_receiver, RecommendSizes(2u, 3u)) // + .Times(AtMost(1)); + all_recommendations += + EXPECT_CALL(mock_receiver, + RecommendBounds(DlRect::MakeLTRB(10, 10, 20, 20))) + .Times(AtMost(1)); + EXPECT_CALL(mock_receiver, SetPathInfo(DlPathFillType::kNonZero, true)); + + { + ::testing::InSequence sequence; + + EXPECT_CALL(mock_receiver, MoveTo(DlPoint(10, 10))) + .After(all_recommendations); + EXPECT_CALL(mock_receiver, + ConicTo(DlPoint(20, 10), DlPoint(20, 20), weight)) + .WillOnce(Return(false)); + EXPECT_CALL(mock_receiver, QuadTo(i_points[1], i_points[2])); + EXPECT_CALL(mock_receiver, QuadTo(i_points[3], i_points[4])); + } + + DlPath(sk_path).Dispatch(mock_receiver); + } +} + +TEST(DisplayListPath, DispatchImpellerPathConicToQuads) { + // If we execute conicTo with a weight of exactly 1.0, SkPath will turn + // it into a quadTo, so we avoid that by using 0.999 + DlScalar weights[4] = { + 0.02f, + 0.5f, + SK_ScalarSqrt2 * 0.5f, + 1.0f - kEhCloseEnough, + }; + + for (DlScalar weight : weights) { + DlPathBuilder path_builder; + path_builder.MoveTo(DlPoint(10, 10)); + path_builder.ConicCurveTo(DlPoint(20, 10), DlPoint(20, 20), weight); + + std::array i_points; + impeller::ConicPathComponent i_conic(DlPoint(10, 10), DlPoint(20, 10), + DlPoint(20, 20), weight); + i_conic.SubdivideToQuadraticPoints(i_points); + + ::testing::StrictMock mock_receiver; + + // Recommendations must happen before any of the path segments is dispatched + ::testing::ExpectationSet all_recommendations; + all_recommendations += // + EXPECT_CALL(mock_receiver, RecommendSizes(2u, 6u)) // + .Times(AtMost(1)); + all_recommendations += + EXPECT_CALL(mock_receiver, + RecommendBounds(DlRect::MakeLTRB(10, 10, 20, 20))) + .Times(AtMost(1)); + EXPECT_CALL(mock_receiver, SetPathInfo(DlPathFillType::kNonZero, false)); + + { + ::testing::InSequence sequence; + + EXPECT_CALL(mock_receiver, MoveTo(DlPoint(10, 10))) + .After(all_recommendations); + EXPECT_CALL(mock_receiver, + ConicTo(DlPoint(20, 10), DlPoint(20, 20), weight)) + .WillOnce(Return(false)); + EXPECT_CALL(mock_receiver, QuadTo(i_points[1], i_points[2])); + EXPECT_CALL(mock_receiver, QuadTo(i_points[3], i_points[4])); + } + + DlPath(path_builder).Dispatch(mock_receiver); + } +} + #ifndef NDEBUG // Tests that verify we don't try to use inverse path modes as they aren't // supported by either Flutter public APIs or Impeller diff --git a/engine/src/flutter/display_list/skia/dl_sk_conversions_unittests.cc b/engine/src/flutter/display_list/skia/dl_sk_conversions_unittests.cc index 61235d29b8..fb093baa5a 100644 --- a/engine/src/flutter/display_list/skia/dl_sk_conversions_unittests.cc +++ b/engine/src/flutter/display_list/skia/dl_sk_conversions_unittests.cc @@ -12,10 +12,11 @@ #include "flutter/display_list/effects/dl_image_filters.h" #include "flutter/display_list/skia/dl_sk_conversions.h" #include "flutter/impeller/geometry/path_component.h" +#include "flutter/third_party/skia/include/core/SkColorSpace.h" +#include "flutter/third_party/skia/include/core/SkSamplingOptions.h" +#include "flutter/third_party/skia/include/core/SkTileMode.h" + #include "gtest/gtest.h" -#include "third_party/skia/include/core/SkColorSpace.h" -#include "third_party/skia/include/core/SkSamplingOptions.h" -#include "third_party/skia/include/core/SkTileMode.h" namespace flutter { namespace testing { @@ -402,9 +403,7 @@ TEST(DisplayListSkConversions, ConicToQuads) { } } -// This tests the new conic subdivision code in the Impeller conic path -// component object vs the code we used to rely on inside Skia -TEST(DisplayListSkConversions, ConicPathToQuads) { +TEST(DisplayListSkConversions, ConicPathToImpeller) { // If we execute conicTo with a weight of exactly 1.0, SkPath will turn // it into a quadTo, so we avoid that by using 0.999 SkScalar weights[4] = { @@ -426,35 +425,16 @@ TEST(DisplayListSkConversions, ConicPathToQuads) { ASSERT_EQ(it.type(), impeller::Path::ComponentType::kContour); ++it; - ASSERT_EQ(it.type(), impeller::Path::ComponentType::kQuadratic); - auto quad1 = it.quadratic(); - ASSERT_NE(quad1, nullptr); + ASSERT_EQ(it.type(), impeller::Path::ComponentType::kConic); + auto conic = it.conic(); + ASSERT_NE(conic, nullptr); ++it; - ASSERT_EQ(it.type(), impeller::Path::ComponentType::kQuadratic); - auto quad2 = it.quadratic(); - ASSERT_NE(quad2, nullptr); - ++it; - - SkPoint sk_points[5]; - int ncurves = SkPath::ConvertConicToQuads( - SkPoint::Make(10, 10), SkPoint::Make(20, 10), SkPoint::Make(20, 20), - weight, sk_points, 1); - ASSERT_EQ(ncurves, 2); - - EXPECT_FLOAT_EQ(sk_points[0].fX, quad1->p1.x) << "weight: " << weight; - EXPECT_FLOAT_EQ(sk_points[0].fY, quad1->p1.y) << "weight: " << weight; - EXPECT_FLOAT_EQ(sk_points[1].fX, quad1->cp.x) << "weight: " << weight; - EXPECT_FLOAT_EQ(sk_points[1].fY, quad1->cp.y) << "weight: " << weight; - EXPECT_FLOAT_EQ(sk_points[2].fX, quad1->p2.x) << "weight: " << weight; - EXPECT_FLOAT_EQ(sk_points[2].fY, quad1->p2.y) << "weight: " << weight; - - EXPECT_FLOAT_EQ(sk_points[2].fX, quad2->p1.x) << "weight: " << weight; - EXPECT_FLOAT_EQ(sk_points[2].fY, quad2->p1.y) << "weight: " << weight; - EXPECT_FLOAT_EQ(sk_points[3].fX, quad2->cp.x) << "weight: " << weight; - EXPECT_FLOAT_EQ(sk_points[3].fY, quad2->cp.y) << "weight: " << weight; - EXPECT_FLOAT_EQ(sk_points[4].fX, quad2->p2.x) << "weight: " << weight; - EXPECT_FLOAT_EQ(sk_points[4].fY, quad2->p2.y) << "weight: " << weight; + EXPECT_EQ(conic->p1, DlPoint(10, 10)) << "weight: " << weight; + EXPECT_EQ(conic->cp, DlPoint(20, 10)) << "weight: " << weight; + EXPECT_EQ(conic->p2, DlPoint(20, 20)) << "weight: " << weight; + EXPECT_EQ(conic->weight.x, weight) << "weight: " << weight; + EXPECT_EQ(conic->weight.y, weight) << "weight: " << weight; } } diff --git a/engine/src/flutter/display_list/testing/BUILD.gn b/engine/src/flutter/display_list/testing/BUILD.gn index 93a9458da3..8e1a053612 100644 --- a/engine/src/flutter/display_list/testing/BUILD.gn +++ b/engine/src/flutter/display_list/testing/BUILD.gn @@ -10,6 +10,7 @@ source_set("display_list_testing") { sources = [ "dl_test_equality.h", + "dl_test_mock_path_receiver.h", "dl_test_snippets.cc", "dl_test_snippets.h", ] diff --git a/engine/src/flutter/display_list/testing/dl_test_mock_path_receiver.h b/engine/src/flutter/display_list/testing/dl_test_mock_path_receiver.h new file mode 100644 index 0000000000..bbef3f999d --- /dev/null +++ b/engine/src/flutter/display_list/testing/dl_test_mock_path_receiver.h @@ -0,0 +1,42 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef FLUTTER_DISPLAY_LIST_TESTING_DL_TEST_MOCK_PATH_RECEIVER_H_ +#define FLUTTER_DISPLAY_LIST_TESTING_DL_TEST_MOCK_PATH_RECEIVER_H_ + +#include "gmock/gmock.h" + +#include "flutter/display_list/geometry/dl_path.h" + +namespace flutter { +namespace testing { +class DlPathReceiverMock : public DlPathReceiver { + public: + MOCK_METHOD(void, + RecommendSizes, + (size_t verb_count, size_t point_count), + (override)); + MOCK_METHOD(void, RecommendBounds, (const DlRect& bounds), (override)); + MOCK_METHOD(void, + SetPathInfo, + (DlPathFillType fill_type, bool is_convex), + (override)); + MOCK_METHOD(void, MoveTo, (const DlPoint& p2), (override)); + MOCK_METHOD(void, LineTo, (const DlPoint& p2), (override)); + MOCK_METHOD(void, QuadTo, (const DlPoint& cp, const DlPoint& p2), (override)); + MOCK_METHOD(bool, + ConicTo, + (const DlPoint& cp, const DlPoint& p2, DlScalar weight), + (override)); + MOCK_METHOD(void, + CubicTo, + (const DlPoint& cp1, const DlPoint& cp2, const DlPoint& p2), + (override)); + MOCK_METHOD(void, Close, (), (override)); +}; + +} // namespace testing +} // namespace flutter + +#endif // FLUTTER_DISPLAY_LIST_TESTING_DL_TEST_MOCK_PATH_RECEIVER_H_ diff --git a/engine/src/flutter/impeller/geometry/path_component.cc b/engine/src/flutter/impeller/geometry/path_component.cc index 6d7d302c72..cba5b58bb9 100644 --- a/engine/src/flutter/impeller/geometry/path_component.cc +++ b/engine/src/flutter/impeller/geometry/path_component.cc @@ -344,7 +344,9 @@ void ConicPathComponent::AppendPolylinePoints( Scalar scale_factor, std::vector& points) const { ToLinearPathComponents(scale_factor, [&points](const Point& point) { - points.emplace_back(point); + if (point != points.back()) { + points.emplace_back(point); + } }); } diff --git a/engine/src/flutter/testing/display_list_testing.cc b/engine/src/flutter/testing/display_list_testing.cc index f921a68fe3..b93569cd14 100644 --- a/engine/src/flutter/testing/display_list_testing.cc +++ b/engine/src/flutter/testing/display_list_testing.cc @@ -59,6 +59,7 @@ using DlImageSampling = flutter::DlImageSampling; using SaveLayerOptions = flutter::SaveLayerOptions; using DisplayListOpType = flutter::DisplayListOpType; using DisplayListOpCategory = flutter::DisplayListOpCategory; +using DlPathFillType = flutter::DlPathFillType; using DlPath = flutter::DlPath; using DisplayListStreamDispatcher = flutter::testing::DisplayListStreamDispatcher; @@ -137,6 +138,14 @@ extern std::ostream& operator<<( return os << "DisplayListOpCategory::???"; } +extern std::ostream& operator<<( + std::ostream& os, const flutter::DlPathFillType& type) { + switch (type) { + DLT_OSTREAM_CASE(DlPathFillType, Odd); + DLT_OSTREAM_CASE(DlPathFillType, NonZero); + } +} + #undef DLT_OSTREAM_CASE std::ostream& operator<<(std::ostream& os, const SaveLayerOptions& options) { @@ -167,6 +176,12 @@ extern std::ostream& operator<<(std::ostream& os, const DlPath& path) { << ")"; } +extern std::ostream& operator<<(std::ostream& os, const flutter::testing::DlVerbosePath& path) { + DisplayListStreamDispatcher dispatcher(os, 0); + dispatcher.out(path); + return os; +} + std::ostream& operator<<(std::ostream& os, const flutter::DlClipOp& op) { switch (op) { case flutter::DlClipOp::kDifference: return os << "DlClipOp::kDifference"; @@ -625,6 +640,75 @@ void DisplayListStreamDispatcher::out(const DlImageFilter* filter) { outdent(1); } } +DisplayListStreamDispatcher::DlPathStreamer::~DlPathStreamer() { + if (done_with_info_) { + dispatcher_.outdent(2); + dispatcher_.startl() << "}" << std::endl; + } +} +void DisplayListStreamDispatcher::DlPathStreamer::RecommendSizes( + size_t verb_count, size_t point_count) { + FML_DCHECK(!done_with_info_); + dispatcher_.startl() << "sizes: " + << verb_count << " verbs, " << point_count << " points" << std::endl; +}; +void DisplayListStreamDispatcher::DlPathStreamer::RecommendBounds( + const DlRect& bounds) { + FML_DCHECK(!done_with_info_); + dispatcher_.startl() << "bounds: " << bounds << std::endl; +}; +void DisplayListStreamDispatcher::DlPathStreamer::SetPathInfo( + DlPathFillType fill_type, bool is_convex) { + FML_DCHECK(!done_with_info_); + dispatcher_.startl() << "info: " + << fill_type << ", convex: " << is_convex << std::endl; +} +void DisplayListStreamDispatcher::DlPathStreamer::MoveTo(const DlPoint& p2) { + if (!done_with_info_) { + done_with_info_ = true; + dispatcher_.startl() << "{" << std::endl; + dispatcher_.indent(2); + } + dispatcher_.startl() << "MoveTo(" << p2 << ")," << std::endl; +} +void DisplayListStreamDispatcher::DlPathStreamer::LineTo(const DlPoint& p2) { + FML_DCHECK(done_with_info_); + dispatcher_.startl() << "LineTo(" << p2 << ")," << std::endl; +} +void DisplayListStreamDispatcher::DlPathStreamer::QuadTo(const DlPoint& cp, + const DlPoint& p2) { + FML_DCHECK(done_with_info_); + dispatcher_.startl() << "QuadTo(" << cp << ", " << p2 << ")," << std::endl; +} +bool DisplayListStreamDispatcher::DlPathStreamer::ConicTo(const DlPoint& cp, + const DlPoint& p2, + DlScalar weight) { + FML_DCHECK(done_with_info_); + dispatcher_.startl() << "ConicTo(" << cp << ", " << p2 << ", " << weight + << ")," << std::endl; + return true; +} +void DisplayListStreamDispatcher::DlPathStreamer::CubicTo(const DlPoint& cp1, + const DlPoint& cp2, + const DlPoint& p2) { + FML_DCHECK(done_with_info_); + dispatcher_.startl() << "CubicTo(" << cp1 << ", " << cp2 << ", " << p2 << ", " + << p2 << ")," << std::endl; +} +void DisplayListStreamDispatcher::DlPathStreamer::Close() { + FML_DCHECK(done_with_info_); + dispatcher_.startl() << "Close()," << std::endl; +} +void DisplayListStreamDispatcher::out(const DlVerbosePath& path) { + os_ << "DlPath(" << std::endl; + indent(2); + { + DlPathStreamer streamer(*this); + path.path.Dispatch(streamer); + } + outdent(2); + os_ << ")"; +} void DisplayListStreamDispatcher::setImageFilter(const DlImageFilter* filter) { startl() << "setImageFilter("; indent(15); diff --git a/engine/src/flutter/testing/display_list_testing.h b/engine/src/flutter/testing/display_list_testing.h index 7b53354a57..354c6cf9ca 100644 --- a/engine/src/flutter/testing/display_list_testing.h +++ b/engine/src/flutter/testing/display_list_testing.h @@ -35,6 +35,12 @@ namespace flutter::testing { const sk_sp& b) { return DisplayListsNE_Verbose(a.get(), b.get()); } +class DlVerbosePath { + public: + explicit DlVerbosePath(const DlPath& path) : path(path) {} + + const DlPath& path; +}; } // namespace flutter::testing @@ -76,6 +82,10 @@ extern std::ostream& operator<<(std::ostream& os, extern std::ostream& operator<<(std::ostream& os, const flutter::DisplayListOpCategory& category); extern std::ostream& operator<<(std::ostream& os, const flutter::DlPath& path); +extern std::ostream& operator<<(std::ostream& os, + const flutter::testing::DlVerbosePath& path); +extern std::ostream& operator<<(std::ostream& os, + const flutter::DlPathFillType& type); extern std::ostream& operator<<(std::ostream& os, const flutter::DlImageFilter& type); extern std::ostream& operator<<(std::ostream& os, @@ -204,6 +214,7 @@ class DisplayListStreamDispatcher final : public DlOpReceiver { void out(const DlColorFilter* filter); void out(const DlImageFilter& filter); void out(const DlImageFilter* filter); + void out(const DlVerbosePath& path); private: std::ostream& os_; @@ -219,6 +230,29 @@ class DisplayListStreamDispatcher final : public DlOpReceiver { std::ostream& out_array(std::string name, int count, const T array[]); std::ostream& startl(); + + class DlPathStreamer : public DlPathReceiver { + public: + ~DlPathStreamer(); + + explicit DlPathStreamer(DisplayListStreamDispatcher& dispatcher) + : dispatcher_(dispatcher) {} + + void RecommendSizes(size_t verb_count, size_t point_count); + void RecommendBounds(const DlRect& bounds); + void SetPathInfo(DlPathFillType fill_type, bool is_convex); + void MoveTo(const DlPoint& p2); + void LineTo(const DlPoint& p2); + void QuadTo(const DlPoint& cp, const DlPoint& p2); + bool ConicTo(const DlPoint& cp, const DlPoint& p2, DlScalar weight); + void CubicTo(const DlPoint& cp1, const DlPoint& cp2, const DlPoint& p2); + void Close(); + + private: + DisplayListStreamDispatcher& dispatcher_; + bool done_with_info_ = false; + }; + friend class DlPathStreamer; }; class DisplayListGeneralReceiver : public DlOpReceiver {