From 4492be6d6debfabdee697d29c53c7ccd53d34bce Mon Sep 17 00:00:00 2001 From: gaaclarke <30870216+gaaclarke@users.noreply.github.com> Date: Wed, 11 Dec 2024 09:32:04 -0800 Subject: [PATCH] Removed linear gradient heap allocation for color conversions between dart and display list (#57108) issue: https://github.com/flutter/flutter/issues/154650 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I added new tests to check the change I am making or feature I am adding, or the PR is [test-exempt]. See [testing the engine] for instructions on writing and running engine tests. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I signed the [CLA]. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. [Contributor Guide]: https://github.com/flutter/flutter/blob/master/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/master/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/master/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/master/docs/contributing/Style-guide-for-Flutter-repo.md [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style [testing the engine]: https://github.com/flutter/engine/blob/main/docs/testing/Testing-the-engine.md [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/master/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/master/docs/contributing/Chat.md --- .../dl_gradient_color_source_base.h | 4 + .../dl_linear_gradient_color_source.cc | 14 +++ .../dl_linear_gradient_color_source.h | 8 ++ display_list/effects/dl_color_source.cc | 95 ++++++++++++++++--- display_list/effects/dl_color_source.h | 11 +++ .../effects/dl_color_source_unittests.cc | 26 +++++ .../skia/dl_sk_conversions_unittests.cc | 3 +- lib/ui/painting/gradient.cc | 12 +-- 8 files changed, 150 insertions(+), 23 deletions(-) diff --git a/display_list/effects/color_sources/dl_gradient_color_source_base.h b/display_list/effects/color_sources/dl_gradient_color_source_base.h index f7a8620373e5b..573bffe69a489 100644 --- a/display_list/effects/color_sources/dl_gradient_color_source_base.h +++ b/display_list/effects/color_sources/dl_gradient_color_source_base.h @@ -42,6 +42,10 @@ class DlGradientColorSourceBase : public DlMatrixColorSourceBase { const DlColor* color_data, const float* stop_data); + void store_color_stops(void* pod, + const DlScalar* color_data_argb, + const float* stop_data); + private: DlTileMode mode_; uint32_t stop_count_; diff --git a/display_list/effects/color_sources/dl_linear_gradient_color_source.cc b/display_list/effects/color_sources/dl_linear_gradient_color_source.cc index 380dce96f95bd..6c9fa3b19a1ea 100644 --- a/display_list/effects/color_sources/dl_linear_gradient_color_source.cc +++ b/display_list/effects/color_sources/dl_linear_gradient_color_source.cc @@ -20,6 +20,20 @@ DlLinearGradientColorSource::DlLinearGradientColorSource( store_color_stops(this + 1, colors, stops); } +DlLinearGradientColorSource::DlLinearGradientColorSource( + const DlPoint start_point, + const DlPoint end_point, + uint32_t stop_count, + const DlScalar* colors, + const float* stops, + DlTileMode tile_mode, + const DlMatrix* matrix) + : DlGradientColorSourceBase(stop_count, tile_mode, matrix), + start_point_(start_point), + end_point_(end_point) { + store_color_stops(this + 1, colors, stops); +} + DlLinearGradientColorSource::DlLinearGradientColorSource( const DlLinearGradientColorSource* source) : DlGradientColorSourceBase(source->stop_count(), diff --git a/display_list/effects/color_sources/dl_linear_gradient_color_source.h b/display_list/effects/color_sources/dl_linear_gradient_color_source.h index 886d4ceb8811a..a229fbbd233c6 100644 --- a/display_list/effects/color_sources/dl_linear_gradient_color_source.h +++ b/display_list/effects/color_sources/dl_linear_gradient_color_source.h @@ -41,6 +41,14 @@ class DlLinearGradientColorSource final : public DlGradientColorSourceBase { DlTileMode tile_mode, const DlMatrix* matrix = nullptr); + DlLinearGradientColorSource(const DlPoint start_point, + const DlPoint end_point, + uint32_t stop_count, + const DlScalar* colors, + const float* stops, + DlTileMode tile_mode, + const DlMatrix* matrix = nullptr); + explicit DlLinearGradientColorSource( const DlLinearGradientColorSource* source); diff --git a/display_list/effects/dl_color_source.cc b/display_list/effects/dl_color_source.cc index 5a5fa8b1fbf19..6d027ef822d59 100644 --- a/display_list/effects/dl_color_source.cc +++ b/display_list/effects/dl_color_source.cc @@ -30,6 +30,13 @@ std::shared_ptr DlColorSource::MakeImage( image, horizontal_tile_mode, vertical_tile_mode, sampling, matrix); } +namespace { +size_t CalculateLinearGradientSize(uint32_t stop_count) { + return sizeof(DlLinearGradientColorSource) + + (stop_count * (sizeof(DlColor) + sizeof(float))); +} +} // namespace + std::shared_ptr DlColorSource::MakeLinear( const DlPoint start_point, const DlPoint end_point, @@ -38,9 +45,7 @@ std::shared_ptr DlColorSource::MakeLinear( const float* stops, DlTileMode tile_mode, const DlMatrix* matrix) { - size_t needed = sizeof(DlLinearGradientColorSource) + - (stop_count * (sizeof(DlColor) + sizeof(float))); - + size_t needed = CalculateLinearGradientSize(stop_count); void* storage = ::operator new(needed); std::shared_ptr ret; @@ -51,6 +56,25 @@ std::shared_ptr DlColorSource::MakeLinear( return ret; } +std::shared_ptr DlColorSource::MakeLinear( + const DlPoint start_point, + const DlPoint end_point, + uint32_t stop_count, + const DlScalar* colors_argb, + const float* stops, + DlTileMode tile_mode, + const DlMatrix* matrix) { + size_t needed = CalculateLinearGradientSize(stop_count); + void* storage = ::operator new(needed); + + std::shared_ptr ret; + ret.reset(new (storage) DlLinearGradientColorSource(start_point, end_point, + stop_count, colors_argb, + stops, tile_mode, matrix), + DlGradientDeleter); + return ret; +} + std::shared_ptr DlColorSource::MakeRadial( DlPoint center, DlScalar radius, @@ -157,23 +181,72 @@ bool DlGradientColorSourceBase::base_equals_( stop_count_ * sizeof(stops()[0])) == 0); } -void DlGradientColorSourceBase::store_color_stops(void* pod, - const DlColor* color_data, - const float* stop_data) { +namespace { +template +void do_store_color_stops(void* pod, + DlColorIt color_data_argb_begin, + DlColorIt color_data_argb_end, + const float* stop_data) { DlColor* color_storage = reinterpret_cast(pod); - memcpy(color_storage, color_data, stop_count_ * sizeof(*color_data)); - float* stop_storage = reinterpret_cast(color_storage + stop_count_); + uint32_t stop_count = 0; + while (color_data_argb_begin < color_data_argb_end) { + *color_storage++ = *color_data_argb_begin++; + stop_count += 1; + } + float* stop_storage = reinterpret_cast(color_storage); if (stop_data) { - memcpy(stop_storage, stop_data, stop_count_ * sizeof(*stop_data)); + memcpy(stop_storage, stop_data, stop_count * sizeof(*stop_data)); } else { - float div = stop_count_ - 1; + float div = stop_count - 1; if (div <= 0) { div = 1; } - for (uint32_t i = 0; i < stop_count_; i++) { + for (uint32_t i = 0; i < stop_count; i++) { stop_storage[i] = i / div; } } } +class DlScalerToDlColorIterator { + public: + explicit DlScalerToDlColorIterator(const DlScalar* ptr) : ptr_(ptr) {} + DlScalerToDlColorIterator(DlScalerToDlColorIterator&&) = default; + DlScalerToDlColorIterator(const DlScalerToDlColorIterator&) = delete; + DlScalerToDlColorIterator& operator=(const DlScalerToDlColorIterator&) = + delete; + + DlColor operator*() { + return DlColor(ptr_[0], ptr_[1], ptr_[2], ptr_[3], + DlColorSpace::kExtendedSRGB); + } + DlScalerToDlColorIterator operator++(int) { + auto result = DlScalerToDlColorIterator(ptr_); + ptr_ += 4; + return result; + } + bool operator<(const DlScalerToDlColorIterator& that) const { + return ptr_ < that.ptr_; + } + + private: + const DlScalar* ptr_; +}; + +} // namespace + +void DlGradientColorSourceBase::store_color_stops(void* pod, + const DlColor* color_data, + const float* stop_data) { + do_store_color_stops(pod, color_data, color_data + stop_count_, stop_data); +} + +void DlGradientColorSourceBase::store_color_stops( + void* pod, + const DlScalar* color_data_argb, + const float* stop_data) { + do_store_color_stops( + pod, DlScalerToDlColorIterator(color_data_argb), + DlScalerToDlColorIterator(color_data_argb + stop_count_ * 4), stop_data); +} + } // namespace flutter diff --git a/display_list/effects/dl_color_source.h b/display_list/effects/dl_color_source.h index 05fe3ca5853d2..44aa88ef38924 100644 --- a/display_list/effects/dl_color_source.h +++ b/display_list/effects/dl_color_source.h @@ -59,6 +59,17 @@ class DlColorSource : public DlAttribute { DlTileMode tile_mode, const DlMatrix* matrix = nullptr); + /// @brief Make a linear gradient. + /// @param colors_argb Array of DlScalars that represents colors in the ARGB. + static std::shared_ptr MakeLinear( + const DlPoint start_point, + const DlPoint end_point, + uint32_t stop_count, + const DlScalar* colors_argb, + const float* stops, + DlTileMode tile_mode, + const DlMatrix* matrix = nullptr); + static std::shared_ptr MakeRadial( DlPoint center, DlScalar radius, diff --git a/display_list/effects/dl_color_source_unittests.cc b/display_list/effects/dl_color_source_unittests.cc index 944de9c994836..de2a9b97011b2 100644 --- a/display_list/effects/dl_color_source_unittests.cc +++ b/display_list/effects/dl_color_source_unittests.cc @@ -189,6 +189,32 @@ TEST(DisplayListColorSource, LinearGradientConstructor) { DlTileMode::kClamp, &kTestMatrix1); } +TEST(DisplayListColorSource, LinearGradientARGBConstructor) { + std::array colors; + for (int i = 0; i < kTestStopCount; ++i) { + colors[i * 4 + 0] = kTestColors[i].getAlphaF(); // + colors[i * 4 + 1] = kTestColors[i].getRedF(); // + colors[i * 4 + 2] = kTestColors[i].getGreenF(); // + colors[i * 4 + 3] = kTestColors[i].getBlueF(); + } + std::shared_ptr source = DlColorSource::MakeLinear( + kTestPoints[0], kTestPoints[1], kTestStopCount, colors.data(), kTestStops, + DlTileMode::kClamp, &kTestMatrix1); + ASSERT_TRUE(source); + ASSERT_TRUE(source->asLinearGradient()); + EXPECT_EQ(source->asLinearGradient()->start_point(), kTestPoints[0]); + EXPECT_EQ(source->asLinearGradient()->end_point(), kTestPoints[1]); + EXPECT_EQ(source->asLinearGradient()->stop_count(), kTestStopCount); + for (int i = 0; i < kTestStopCount; i++) { + EXPECT_EQ(source->asLinearGradient()->colors()[i], + kTestColors[i].withColorSpace(DlColorSpace::kExtendedSRGB)); + EXPECT_EQ(source->asLinearGradient()->stops()[i], kTestStops[i]); + } + EXPECT_EQ(source->asLinearGradient()->tile_mode(), DlTileMode::kClamp); + EXPECT_EQ(source->asLinearGradient()->matrix(), kTestMatrix1); + EXPECT_EQ(source->is_opaque(), true); +} + TEST(DisplayListColorSource, LinearGradientShared) { std::shared_ptr source = DlColorSource::MakeLinear( kTestPoints[0], kTestPoints[1], kTestStopCount, kTestColors, kTestStops, diff --git a/display_list/skia/dl_sk_conversions_unittests.cc b/display_list/skia/dl_sk_conversions_unittests.cc index 81606246e76c4..ab536ba51903e 100644 --- a/display_list/skia/dl_sk_conversions_unittests.cc +++ b/display_list/skia/dl_sk_conversions_unittests.cc @@ -302,7 +302,8 @@ TEST(DisplayListSkConversions, ToSkDitheringEnabledForGradients) { // Set the paint to be a gradient. dl_paint.setColorSource(DlColorSource::MakeLinear( - DlPoint(0, 0), DlPoint(100, 100), 0, 0, 0, DlTileMode::kClamp)); + DlPoint(0, 0), DlPoint(100, 100), 0, + std::array{DlColor(0)}.data(), 0, DlTileMode::kClamp)); { SkPaint sk_paint = ToSk(dl_paint); diff --git a/lib/ui/painting/gradient.cc b/lib/ui/painting/gradient.cc index 1b87943e51162..a7ecf3be6cfdb 100644 --- a/lib/ui/painting/gradient.cc +++ b/lib/ui/painting/gradient.cc @@ -44,17 +44,7 @@ void CanvasGradient::initLinear(const tonic::Float32List& end_points, DlPoint p0 = DlPoint(end_points[0], end_points[1]); DlPoint p1 = DlPoint(end_points[2], end_points[3]); - std::vector dl_colors; - dl_colors.reserve(num_colors); - for (int i = 0; i < colors.num_elements(); i += 4) { - DlScalar a = colors[i + 0]; - DlScalar r = colors[i + 1]; - DlScalar g = colors[i + 2]; - DlScalar b = colors[i + 3]; - dl_colors.emplace_back(DlColor(a, r, g, b, DlColorSpace::kExtendedSRGB)); - } - - dl_shader_ = DlColorSource::MakeLinear(p0, p1, num_colors, dl_colors.data(), + dl_shader_ = DlColorSource::MakeLinear(p0, p1, num_colors, colors.data(), color_stops.data(), tile_mode, has_matrix ? &dl_matrix : nullptr); // Just a sanity check, all gradient shaders should be thread-safe