From 0c245ce56b19440f5ef73bb02d1ebb2e9345fa6a Mon Sep 17 00:00:00 2001 From: Jim Graham Date: Tue, 26 Nov 2024 16:13:22 -0800 Subject: [PATCH] [DisplayList] Delete (publicly) unused DlColorColorSource (flutter/engine#56825) While recently updating the DlColorSource sources I noticed some questionably implementation choices in the Color variant of the color sources. I then realized that there was no public use of these classes (other than mostly their own unit tests) and so they should be deleted to focus on implementing the variants that are actually used by Flutter. --- .../ci/licenses_golden/licenses_flutter | 4 -- engine/src/flutter/display_list/BUILD.gn | 2 - engine/src/flutter/display_list/dl_builder.cc | 14 ++--- .../display_list/dl_paint_unittests.cc | 47 +++++++++++----- .../color_sources/dl_color_color_source.cc | 15 ------ .../color_sources/dl_color_color_source.h | 42 --------------- .../display_list/effects/dl_color_source.cc | 4 -- .../display_list/effects/dl_color_source.h | 8 --- .../effects/dl_color_source_unittests.cc | 54 +------------------ .../display_list/effects/dl_color_sources.h | 1 - .../display_list/skia/dl_sk_conversions.cc | 5 -- .../skia/dl_sk_paint_dispatcher_unittests.cc | 12 ----- .../testing/dl_rendering_unittests.cc | 2 +- .../flutter/impeller/display_list/canvas.cc | 10 ++-- .../impeller/display_list/dl_dispatcher.cc | 6 +-- .../impeller/display_list/dl_unittests.cc | 31 +++++++---- .../flutter/impeller/display_list/paint.cc | 5 -- engine/src/flutter/shell/common/dl_op_spy.cc | 10 ++-- engine/src/flutter/shell/common/dl_op_spy.h | 3 ++ .../shell/common/dl_op_spy_unittests.cc | 41 +++++++------- .../flutter/testing/display_list_testing.cc | 6 --- .../flutter/testing/display_list_testing.h | 1 - .../txt/src/skia/paragraph_skia.cc | 2 +- 23 files changed, 89 insertions(+), 236 deletions(-) delete mode 100644 engine/src/flutter/display_list/effects/color_sources/dl_color_color_source.cc delete mode 100644 engine/src/flutter/display_list/effects/color_sources/dl_color_color_source.h diff --git a/engine/src/flutter/ci/licenses_golden/licenses_flutter b/engine/src/flutter/ci/licenses_golden/licenses_flutter index 15bb63481a..34e34f9804 100644 --- a/engine/src/flutter/ci/licenses_golden/licenses_flutter +++ b/engine/src/flutter/ci/licenses_golden/licenses_flutter @@ -42540,8 +42540,6 @@ ORIGIN: ../../../flutter/display_list/effects/color_filters/dl_matrix_color_filt ORIGIN: ../../../flutter/display_list/effects/color_filters/dl_matrix_color_filter.h + ../../../flutter/LICENSE ORIGIN: ../../../flutter/display_list/effects/color_filters/dl_srgb_to_linear_gamma_color_filter.cc + ../../../flutter/LICENSE ORIGIN: ../../../flutter/display_list/effects/color_filters/dl_srgb_to_linear_gamma_color_filter.h + ../../../flutter/LICENSE -ORIGIN: ../../../flutter/display_list/effects/color_sources/dl_color_color_source.cc + ../../../flutter/LICENSE -ORIGIN: ../../../flutter/display_list/effects/color_sources/dl_color_color_source.h + ../../../flutter/LICENSE ORIGIN: ../../../flutter/display_list/effects/color_sources/dl_conical_gradient_color_source.cc + ../../../flutter/LICENSE ORIGIN: ../../../flutter/display_list/effects/color_sources/dl_conical_gradient_color_source.h + ../../../flutter/LICENSE ORIGIN: ../../../flutter/display_list/effects/color_sources/dl_gradient_color_source_base.h + ../../../flutter/LICENSE @@ -45467,8 +45465,6 @@ FILE: ../../../flutter/display_list/effects/color_filters/dl_matrix_color_filter FILE: ../../../flutter/display_list/effects/color_filters/dl_matrix_color_filter.h FILE: ../../../flutter/display_list/effects/color_filters/dl_srgb_to_linear_gamma_color_filter.cc FILE: ../../../flutter/display_list/effects/color_filters/dl_srgb_to_linear_gamma_color_filter.h -FILE: ../../../flutter/display_list/effects/color_sources/dl_color_color_source.cc -FILE: ../../../flutter/display_list/effects/color_sources/dl_color_color_source.h FILE: ../../../flutter/display_list/effects/color_sources/dl_conical_gradient_color_source.cc FILE: ../../../flutter/display_list/effects/color_sources/dl_conical_gradient_color_source.h FILE: ../../../flutter/display_list/effects/color_sources/dl_gradient_color_source_base.h diff --git a/engine/src/flutter/display_list/BUILD.gn b/engine/src/flutter/display_list/BUILD.gn index b8b863d81f..3ed4e721bf 100644 --- a/engine/src/flutter/display_list/BUILD.gn +++ b/engine/src/flutter/display_list/BUILD.gn @@ -55,8 +55,6 @@ source_set("display_list") { "effects/color_filters/dl_matrix_color_filter.h", "effects/color_filters/dl_srgb_to_linear_gamma_color_filter.cc", "effects/color_filters/dl_srgb_to_linear_gamma_color_filter.h", - "effects/color_sources/dl_color_color_source.cc", - "effects/color_sources/dl_color_color_source.h", "effects/color_sources/dl_conical_gradient_color_source.cc", "effects/color_sources/dl_conical_gradient_color_source.h", "effects/color_sources/dl_image_color_source.cc", diff --git a/engine/src/flutter/display_list/dl_builder.cc b/engine/src/flutter/display_list/dl_builder.cc index a6ff898c42..3e0e18ddd1 100644 --- a/engine/src/flutter/display_list/dl_builder.cc +++ b/engine/src/flutter/display_list/dl_builder.cc @@ -201,12 +201,6 @@ void DisplayListBuilder::onSetColorSource(const DlColorSource* source) { current_.setColorSource(source->shared()); is_ui_thread_safe_ = is_ui_thread_safe_ && source->isUIThreadSafe(); switch (source->type()) { - case DlColorSourceType::kColor: { - const DlColorColorSource* color_source = source->asColor(); - current_.setColorSource(nullptr); - setColor(color_source->color()); - break; - } case DlColorSourceType::kImage: { const DlImageColorSource* image_source = source->asImage(); FML_DCHECK(image_source); @@ -1953,11 +1947,9 @@ DlColor DisplayListBuilder::GetEffectiveColor(const DlPaint& paint, if (flags.applies_color()) { const DlColorSource* source = paint.getColorSourcePtr(); if (source) { - if (source->asColor()) { - color = source->asColor()->color(); - } else { - color = source->is_opaque() ? DlColor::kBlack() : kAnyColor; - } + // Suspecting that we need to modulate the ColorSource color by the + // color property, see https://github.com/flutter/flutter/issues/159507 + color = source->is_opaque() ? DlColor::kBlack() : kAnyColor; } else { color = paint.getColor(); } diff --git a/engine/src/flutter/display_list/dl_paint_unittests.cc b/engine/src/flutter/display_list/dl_paint_unittests.cc index 617a7b5bd0..e03f02ed24 100644 --- a/engine/src/flutter/display_list/dl_paint_unittests.cc +++ b/engine/src/flutter/display_list/dl_paint_unittests.cc @@ -56,7 +56,16 @@ TEST(DisplayListPaint, ConstructorDefaults) { EXPECT_NE(paint, DlPaint().setStrokeWidth(6)); EXPECT_NE(paint, DlPaint().setStrokeMiter(7)); - auto color_source = DlColorSource::MakeColor(DlColor::kMagenta()); + DlColor colors[2] = { + DlColor::kGreen(), + DlColor::kBlue(), + }; + float stops[2] = { + 0.0f, + 1.0f, + }; + auto color_source = DlColorSource::MakeLinear({0, 0}, {10, 10}, 2, colors, + stops, DlTileMode::kClamp); EXPECT_NE(paint, DlPaint().setColorSource(color_source)); auto color_filter = @@ -95,19 +104,28 @@ TEST(DisplayListPaint, NullSharedPointerSetGet) { } TEST(DisplayListPaint, ChainingConstructor) { + DlColor colors[2] = { + DlColor::kGreen(), + DlColor::kBlue(), + }; + float stops[2] = { + 0.0f, + 1.0f, + }; DlPaint paint = - DlPaint() // - .setAntiAlias(true) // - .setInvertColors(true) // - .setColor(DlColor::kGreen()) // - .setAlpha(0x7F) // - .setBlendMode(DlBlendMode::kLuminosity) // - .setDrawStyle(DlDrawStyle::kStrokeAndFill) // - .setStrokeCap(DlStrokeCap::kSquare) // - .setStrokeJoin(DlStrokeJoin::kBevel) // - .setStrokeWidth(42) // - .setStrokeMiter(1.5) // - .setColorSource(DlColorSource::MakeColor(DlColor::kMagenta())) // + DlPaint() // + .setAntiAlias(true) // + .setInvertColors(true) // + .setColor(DlColor::kGreen()) // + .setAlpha(0x7F) // + .setBlendMode(DlBlendMode::kLuminosity) // + .setDrawStyle(DlDrawStyle::kStrokeAndFill) // + .setStrokeCap(DlStrokeCap::kSquare) // + .setStrokeJoin(DlStrokeJoin::kBevel) // + .setStrokeWidth(42) // + .setStrokeMiter(1.5) // + .setColorSource(DlColorSource::MakeLinear( // + {0, 0}, {10, 10}, 2, colors, stops, DlTileMode::kClamp)) // .setColorFilter( DlColorFilter::MakeBlend(DlColor::kYellow(), DlBlendMode::kDstIn)) .setImageFilter(DlImageFilter::MakeBlur(1.3, 4.7, DlTileMode::kClamp)) @@ -123,7 +141,8 @@ TEST(DisplayListPaint, ChainingConstructor) { EXPECT_EQ(paint.getStrokeWidth(), 42); EXPECT_EQ(paint.getStrokeMiter(), 1.5); EXPECT_TRUE(Equals(paint.getColorSource(), - DlColorSource::MakeColor(DlColor::kMagenta()))); + DlColorSource::MakeLinear({0, 0}, {10, 10}, 2, colors, + stops, DlTileMode::kClamp))); EXPECT_TRUE(Equals( paint.getColorFilter(), DlColorFilter::MakeBlend(DlColor::kYellow(), DlBlendMode::kDstIn))); diff --git a/engine/src/flutter/display_list/effects/color_sources/dl_color_color_source.cc b/engine/src/flutter/display_list/effects/color_sources/dl_color_color_source.cc deleted file mode 100644 index e9f56f758c..0000000000 --- a/engine/src/flutter/display_list/effects/color_sources/dl_color_color_source.cc +++ /dev/null @@ -1,15 +0,0 @@ -// 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. - -#include "flutter/display_list/effects/color_sources/dl_color_color_source.h" - -namespace flutter { - -bool DlColorColorSource::equals_(DlColorSource const& other) const { - FML_DCHECK(other.type() == DlColorSourceType::kColor); - auto that = static_cast(&other); - return color_ == that->color_; -} - -} // namespace flutter diff --git a/engine/src/flutter/display_list/effects/color_sources/dl_color_color_source.h b/engine/src/flutter/display_list/effects/color_sources/dl_color_color_source.h deleted file mode 100644 index c873448ae4..0000000000 --- a/engine/src/flutter/display_list/effects/color_sources/dl_color_color_source.h +++ /dev/null @@ -1,42 +0,0 @@ -// 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_EFFECTS_COLOR_SOURCES_DL_COLOR_COLOR_SOURCE_H_ -#define FLUTTER_DISPLAY_LIST_EFFECTS_COLOR_SOURCES_DL_COLOR_COLOR_SOURCE_H_ - -#include "flutter/display_list/effects/dl_color_source.h" - -namespace flutter { - -class DlColorColorSource final : public DlColorSource { - public: - explicit DlColorColorSource(DlColor color) : color_(color) {} - - bool isUIThreadSafe() const override { return true; } - - std::shared_ptr shared() const override { - return std::make_shared(color_); - } - - const DlColorColorSource* asColor() const override { return this; } - - DlColorSourceType type() const override { return DlColorSourceType::kColor; } - size_t size() const override { return sizeof(*this); } - - bool is_opaque() const override { return color_.getAlpha() == 255; } - - DlColor color() const { return color_; } - - protected: - bool equals_(DlColorSource const& other) const override; - - private: - DlColor color_; - - FML_DISALLOW_COPY_ASSIGN_AND_MOVE(DlColorColorSource); -}; - -} // namespace flutter - -#endif // FLUTTER_DISPLAY_LIST_EFFECTS_COLOR_SOURCES_DL_COLOR_COLOR_SOURCE_H_ diff --git a/engine/src/flutter/display_list/effects/dl_color_source.cc b/engine/src/flutter/display_list/effects/dl_color_source.cc index 6009096b3d..5a5fa8b1fb 100644 --- a/engine/src/flutter/display_list/effects/dl_color_source.cc +++ b/engine/src/flutter/display_list/effects/dl_color_source.cc @@ -20,10 +20,6 @@ static void DlGradientDeleter(void* p) { ::operator delete(p); } -std::shared_ptr DlColorSource::MakeColor(DlColor color) { - return std::make_shared(color); -} - std::shared_ptr DlColorSource::MakeImage( const sk_sp& image, DlTileMode horizontal_tile_mode, diff --git a/engine/src/flutter/display_list/effects/dl_color_source.h b/engine/src/flutter/display_list/effects/dl_color_source.h index 8b25644248..05fe3ca585 100644 --- a/engine/src/flutter/display_list/effects/dl_color_source.h +++ b/engine/src/flutter/display_list/effects/dl_color_source.h @@ -15,7 +15,6 @@ namespace flutter { -class DlColorColorSource; class DlImageColorSource; class DlLinearGradientColorSource; class DlRadialGradientColorSource; @@ -34,7 +33,6 @@ class DlRuntimeEffectColorSource; // attributes, and the final blend with the pixels in the destination. enum class DlColorSourceType { - kColor, kImage, kLinearGradient, kRadialGradient, @@ -45,8 +43,6 @@ enum class DlColorSourceType { class DlColorSource : public DlAttribute { public: - static std::shared_ptr MakeColor(DlColor color); - static std::shared_ptr MakeImage( const sk_sp& image, DlTileMode horizontal_tile_mode, @@ -120,10 +116,6 @@ class DlColorSource : public DlAttribute { /// virtual bool isGradient() const { return false; } - // Return a DlColorColorSource pointer to this object iff it is an Color - // type of ColorSource, otherwise return nullptr. - virtual const DlColorColorSource* asColor() const { return nullptr; } - // Return a DlImageColorSource pointer to this object iff it is an Image // type of ColorSource, otherwise return nullptr. virtual const DlImageColorSource* asImage() const { return nullptr; } diff --git a/engine/src/flutter/display_list/effects/dl_color_source_unittests.cc b/engine/src/flutter/display_list/effects/dl_color_source_unittests.cc index 423c5c405b..944de9c994 100644 --- a/engine/src/flutter/display_list/effects/dl_color_source_unittests.cc +++ b/engine/src/flutter/display_list/effects/dl_color_source_unittests.cc @@ -88,53 +88,6 @@ static constexpr DlPoint kTestPoints2[2] = { DlPoint(107, 118), }; -TEST(DisplayListColorSource, ColorConstructor) { - DlColorColorSource source(DlColor::kRed()); -} - -TEST(DisplayListColorSource, ColorShared) { - DlColorColorSource source(DlColor::kRed()); - ASSERT_NE(source.shared().get(), &source); - ASSERT_EQ(*source.shared(), source); -} - -TEST(DisplayListColorSource, ColorAsColor) { - DlColorColorSource source(DlColor::kRed()); - ASSERT_NE(source.asColor(), nullptr); - ASSERT_EQ(source.asColor(), &source); - - ASSERT_EQ(source.asImage(), nullptr); - ASSERT_EQ(source.asLinearGradient(), nullptr); - ASSERT_EQ(source.asRadialGradient(), nullptr); - ASSERT_EQ(source.asConicalGradient(), nullptr); - ASSERT_EQ(source.asSweepGradient(), nullptr); - ASSERT_EQ(source.asRuntimeEffect(), nullptr); -} - -TEST(DisplayListColorSource, ColorContents) { - DlColorColorSource source(DlColor::kRed()); - ASSERT_EQ(source.color(), DlColor::kRed()); - ASSERT_EQ(source.is_opaque(), true); - for (int i = 0; i < 255; i++) { - SkColor alpha_color = SkColorSetA(SK_ColorRED, i); - auto const alpha_source = DlColorColorSource(DlColor(alpha_color)); - ASSERT_EQ(alpha_source.color(), alpha_color); - ASSERT_EQ(alpha_source.is_opaque(), false); - } -} - -TEST(DisplayListColorSource, ColorEquals) { - DlColorColorSource source1(DlColor::kRed()); - DlColorColorSource source2(DlColor::kRed()); - TestEquals(source1, source2); -} - -TEST(DisplayListColorSource, ColorNotEquals) { - DlColorColorSource source1(DlColor::kRed()); - DlColorColorSource source2(DlColor::kBlue()); - TestNotEquals(source1, source2, "Color differs"); -} - TEST(DisplayListColorSource, ImageConstructor) { DlImageColorSource source(kTestImage1, DlTileMode::kClamp, DlTileMode::kClamp, DlImageSampling::kLinear, &kTestMatrix1); @@ -153,11 +106,11 @@ TEST(DisplayListColorSource, ImageAsImage) { ASSERT_NE(source.asImage(), nullptr); ASSERT_EQ(source.asImage(), &source); - ASSERT_EQ(source.asColor(), nullptr); ASSERT_EQ(source.asLinearGradient(), nullptr); ASSERT_EQ(source.asRadialGradient(), nullptr); ASSERT_EQ(source.asConicalGradient(), nullptr); ASSERT_EQ(source.asSweepGradient(), nullptr); + ASSERT_EQ(source.asRuntimeEffect(), nullptr); } TEST(DisplayListColorSource, ImageContents) { @@ -251,7 +204,6 @@ TEST(DisplayListColorSource, LinearGradientAsLinear) { ASSERT_NE(source->asLinearGradient(), nullptr); ASSERT_EQ(source->asLinearGradient(), source.get()); - ASSERT_EQ(source->asColor(), nullptr); ASSERT_EQ(source->asImage(), nullptr); ASSERT_EQ(source->asRadialGradient(), nullptr); ASSERT_EQ(source->asConicalGradient(), nullptr); @@ -370,7 +322,6 @@ TEST(DisplayListColorSource, RadialGradientAsRadial) { ASSERT_NE(source->asRadialGradient(), nullptr); ASSERT_EQ(source->asRadialGradient(), source.get()); - ASSERT_EQ(source->asColor(), nullptr); ASSERT_EQ(source->asImage(), nullptr); ASSERT_EQ(source->asLinearGradient(), nullptr); ASSERT_EQ(source->asConicalGradient(), nullptr); @@ -489,7 +440,6 @@ TEST(DisplayListColorSource, ConicalGradientAsConical) { ASSERT_NE(source->asConicalGradient(), nullptr); ASSERT_EQ(source->asConicalGradient(), source.get()); - ASSERT_EQ(source->asColor(), nullptr); ASSERT_EQ(source->asImage(), nullptr); ASSERT_EQ(source->asLinearGradient(), nullptr); ASSERT_EQ(source->asRadialGradient(), nullptr); @@ -624,7 +574,6 @@ TEST(DisplayListColorSource, SweepGradientAsSweep) { ASSERT_NE(source->asSweepGradient(), nullptr); ASSERT_EQ(source->asSweepGradient(), source.get()); - ASSERT_EQ(source->asColor(), nullptr); ASSERT_EQ(source->asImage(), nullptr); ASSERT_EQ(source->asLinearGradient(), nullptr); ASSERT_EQ(source->asRadialGradient(), nullptr); @@ -743,7 +692,6 @@ TEST(DisplayListColorSource, RuntimeEffect) { ASSERT_NE(source2->asRuntimeEffect(), source1.get()); ASSERT_EQ(source1->asImage(), nullptr); - ASSERT_EQ(source1->asColor(), nullptr); ASSERT_EQ(source1->asLinearGradient(), nullptr); ASSERT_EQ(source1->asRadialGradient(), nullptr); ASSERT_EQ(source1->asConicalGradient(), nullptr); diff --git a/engine/src/flutter/display_list/effects/dl_color_sources.h b/engine/src/flutter/display_list/effects/dl_color_sources.h index ce5d802d04..23e810adc4 100644 --- a/engine/src/flutter/display_list/effects/dl_color_sources.h +++ b/engine/src/flutter/display_list/effects/dl_color_sources.h @@ -5,7 +5,6 @@ #ifndef FLUTTER_DISPLAY_LIST_EFFECTS_DL_COLOR_SOURCES_H_ #define FLUTTER_DISPLAY_LIST_EFFECTS_DL_COLOR_SOURCES_H_ -#include "flutter/display_list/effects/color_sources/dl_color_color_source.h" #include "flutter/display_list/effects/color_sources/dl_conical_gradient_color_source.h" #include "flutter/display_list/effects/color_sources/dl_image_color_source.h" #include "flutter/display_list/effects/color_sources/dl_linear_gradient_color_source.h" diff --git a/engine/src/flutter/display_list/skia/dl_sk_conversions.cc b/engine/src/flutter/display_list/skia/dl_sk_conversions.cc index 67fc7b3fbc..16b3455f9d 100644 --- a/engine/src/flutter/display_list/skia/dl_sk_conversions.cc +++ b/engine/src/flutter/display_list/skia/dl_sk_conversions.cc @@ -83,11 +83,6 @@ sk_sp ToSk(const DlColorSource* source) { return sk_colors; }; switch (source->type()) { - case DlColorSourceType::kColor: { - const DlColorColorSource* color_source = source->asColor(); - FML_DCHECK(color_source != nullptr); - return SkShaders::Color(ToSk(color_source->color())); - } case DlColorSourceType::kImage: { const DlImageColorSource* image_source = source->asImage(); FML_DCHECK(image_source != nullptr); diff --git a/engine/src/flutter/display_list/skia/dl_sk_paint_dispatcher_unittests.cc b/engine/src/flutter/display_list/skia/dl_sk_paint_dispatcher_unittests.cc index e515537ba2..f0bd925c1c 100644 --- a/engine/src/flutter/display_list/skia/dl_sk_paint_dispatcher_unittests.cc +++ b/engine/src/flutter/display_list/skia/dl_sk_paint_dispatcher_unittests.cc @@ -64,11 +64,6 @@ TEST(DisplayListUtils, SetColorSourceDoesNotDitherIfNotGradient) { EXPECT_FALSE(helper.paint(true).isDither()); EXPECT_FALSE(helper.paint(false).isDither()); - DlColorColorSource color_color_source(DlColor::kBlue()); - helper.setColorSource(&color_color_source); - EXPECT_FALSE(helper.paint(true).isDither()); - EXPECT_FALSE(helper.paint(false).isDither()); - helper.setColorSource(kTestSource1.get()); EXPECT_FALSE(helper.paint(true).isDither()); EXPECT_FALSE(helper.paint(false).isDither()); @@ -98,13 +93,6 @@ TEST(DisplayListUtils, SkDispatcherSetColorSourceDoesNotDitherIfNotGradient) { EXPECT_FALSE(dispatcher.safe_paint(true)->isDither()); // Calling safe_paint(false) returns a nullptr - DlColorColorSource color_color_source(DlColor::kBlue()); - dispatcher.setColorSource(&color_color_source); - EXPECT_FALSE(dispatcher.paint(true).isDither()); - EXPECT_FALSE(dispatcher.paint(false).isDither()); - EXPECT_FALSE(dispatcher.safe_paint(true)->isDither()); - // Calling safe_paint(false) returns a nullptr - dispatcher.setColorSource(kTestSource1.get()); EXPECT_FALSE(dispatcher.paint(true).isDither()); EXPECT_FALSE(dispatcher.paint(false).isDither()); diff --git a/engine/src/flutter/display_list/testing/dl_rendering_unittests.cc b/engine/src/flutter/display_list/testing/dl_rendering_unittests.cc index 399e51c8b1..bf752442fe 100644 --- a/engine/src/flutter/display_list/testing/dl_rendering_unittests.cc +++ b/engine/src/flutter/display_list/testing/dl_rendering_unittests.cc @@ -863,7 +863,7 @@ class TestParameters { bool impeller_compatible(const DlPaint& paint) const { if (is_draw_text_blob()) { // Non-color text is rendered as paths - if (paint.getColorSourcePtr() && !paint.getColorSourcePtr()->asColor()) { + if (paint.getColorSourcePtr()) { return false; } // Non-filled text (stroke or stroke and fill) is rendered as paths diff --git a/engine/src/flutter/impeller/display_list/canvas.cc b/engine/src/flutter/impeller/display_list/canvas.cc index 7b26f02892..4e5a4dcba0 100644 --- a/engine/src/flutter/impeller/display_list/canvas.cc +++ b/engine/src/flutter/impeller/display_list/canvas.cc @@ -54,9 +54,7 @@ static bool UseColorSourceContents( if (vertices->HasVertexColors()) { return false; } - if (vertices->HasTextureCoordinates() && - (!paint.color_source || - paint.color_source->type() == flutter::DlColorSourceType::kColor)) { + if (vertices->HasTextureCoordinates() && !paint.color_source) { return true; } return !vertices->HasTextureCoordinates(); @@ -314,8 +312,7 @@ bool Canvas::AttemptDrawBlurredRRect(const Rect& rect, return false; } - if (paint.color_source && - paint.color_source->type() != flutter::DlColorSourceType::kColor) { + if (paint.color_source) { return false; } @@ -736,8 +733,7 @@ void Canvas::DrawVertices(const std::shared_ptr& vertices, // Override the blend mode with kDestination in order to match the behavior // of Skia's SK_LEGACY_IGNORE_DRAW_VERTICES_BLEND_WITH_NO_SHADER flag, which // is enabled when the Flutter engine builds Skia. - if (!paint.color_source || - paint.color_source->type() == flutter::DlColorSourceType::kColor) { + if (!paint.color_source) { blend_mode = BlendMode::kDestination; } diff --git a/engine/src/flutter/impeller/display_list/dl_dispatcher.cc b/engine/src/flutter/impeller/display_list/dl_dispatcher.cc index b1ba5b7e35..3fb650d36a 100644 --- a/engine/src/flutter/impeller/display_list/dl_dispatcher.cc +++ b/engine/src/flutter/impeller/display_list/dl_dispatcher.cc @@ -222,11 +222,7 @@ void DlDispatcherBase::setStrokeJoin(flutter::DlStrokeJoin join) { void DlDispatcherBase::setColorSource(const flutter::DlColorSource* source) { AUTO_DEPTH_WATCHER(0u); - if (!source || source->type() == flutter::DlColorSourceType::kColor) { - paint_.color_source = nullptr; - } else { - paint_.color_source = source; - } + paint_.color_source = source; } // |flutter::DlOpReceiver| diff --git a/engine/src/flutter/impeller/display_list/dl_unittests.cc b/engine/src/flutter/impeller/display_list/dl_unittests.cc index 7ff0fa8405..832515a720 100644 --- a/engine/src/flutter/impeller/display_list/dl_unittests.cc +++ b/engine/src/flutter/impeller/display_list/dl_unittests.cc @@ -1256,30 +1256,41 @@ TEST_P(DisplayListTest, MaskBlursApplyCorrectlyToColorSources) { std::array colors = {flutter::DlColor::kBlue(), flutter::DlColor::kGreen()}; std::array stops = {0, 1}; + auto texture = CreateTextureForFixture("airplane.jpg"); + auto matrix = flutter::DlMatrix::MakeTranslation({-300, -110}); std::array, 2> color_sources = { - flutter::DlColorSource::MakeColor(flutter::DlColor::kWhite()), + flutter::DlColorSource::MakeImage( + DlImageImpeller::Make(texture), flutter::DlTileMode::kRepeat, + flutter::DlTileMode::kRepeat, flutter::DlImageSampling::kLinear, + &matrix), flutter::DlColorSource::MakeLinear( flutter::DlPoint(0, 0), flutter::DlPoint(100, 50), 2, colors.data(), - stops.data(), flutter::DlTileMode::kClamp)}; + stops.data(), flutter::DlTileMode::kClamp), + }; - int offset = 100; + builder.Save(); + builder.Translate(0, 100); for (const auto& color_source : color_sources) { flutter::DlPaint paint; paint.setColorSource(color_source); paint.setMaskFilter(blur_filter); + builder.Save(); + builder.Translate(100, 0); paint.setDrawStyle(flutter::DlDrawStyle::kFill); - builder.DrawRRect( - SkRRect::MakeRectXY(SkRect::MakeXYWH(100, offset, 100, 50), 30, 30), - paint); + builder.DrawRRect(SkRRect::MakeRectXY(SkRect::MakeWH(100, 50), 30, 30), + paint); + paint.setDrawStyle(flutter::DlDrawStyle::kStroke); paint.setStrokeWidth(10); - builder.DrawRRect( - SkRRect::MakeRectXY(SkRect::MakeXYWH(300, offset, 100, 50), 30, 30), - paint); + builder.Translate(200, 0); + builder.DrawRRect(SkRRect::MakeRectXY(SkRect::MakeWH(100, 50), 30, 30), + paint); - offset += 100; + builder.Restore(); + builder.Translate(0, 100); } + builder.Restore(); ASSERT_TRUE(OpenPlaygroundHere(builder.Build())); } diff --git a/engine/src/flutter/impeller/display_list/paint.cc b/engine/src/flutter/impeller/display_list/paint.cc index 6bbdc1a694..0305a4a88e 100644 --- a/engine/src/flutter/impeller/display_list/paint.cc +++ b/engine/src/flutter/impeller/display_list/paint.cc @@ -239,11 +239,6 @@ std::shared_ptr Paint::CreateContents() const { contents->SetTextureInputs(std::move(texture_inputs)); return contents; } - case flutter::DlColorSourceType::kColor: { - auto contents = std::make_shared(); - contents->SetColor(color); - return contents; - } } FML_UNREACHABLE(); } diff --git a/engine/src/flutter/shell/common/dl_op_spy.cc b/engine/src/flutter/shell/common/dl_op_spy.cc index 5c109bee31..f9671ccab4 100644 --- a/engine/src/flutter/shell/common/dl_op_spy.cc +++ b/engine/src/flutter/shell/common/dl_op_spy.cc @@ -4,8 +4,6 @@ #include "flutter/shell/common/dl_op_spy.h" -#include "flutter/display_list/effects/color_sources/dl_color_color_source.h" - namespace flutter { bool DlOpSpy::did_draw() { @@ -13,6 +11,7 @@ bool DlOpSpy::did_draw() { } void DlOpSpy::setColor(DlColor color) { + color_ = color; if (color.isTransparent()) { will_draw_ = false; } else { @@ -21,11 +20,8 @@ void DlOpSpy::setColor(DlColor color) { } void DlOpSpy::setColorSource(const DlColorSource* source) { if (!source) { - return; - } - const DlColorColorSource* color_source = source->asColor(); - if (color_source && color_source->color().isTransparent()) { - will_draw_ = false; + // Restore settings based on previously set color + setColor(color_); return; } will_draw_ = true; diff --git a/engine/src/flutter/shell/common/dl_op_spy.h b/engine/src/flutter/shell/common/dl_op_spy.h index 3c2c63d75f..27fe39554f 100644 --- a/engine/src/flutter/shell/common/dl_op_spy.h +++ b/engine/src/flutter/shell/common/dl_op_spy.h @@ -106,6 +106,9 @@ class DlOpSpy final : public virtual DlOpReceiver, bool transparent_occluder, DlScalar dpr) override; + // Most recently set color, used when color_source goes to null + DlColor color_; + // Indicates if the attributes are set to values that will modify the // destination. For now, the test only checks if there is a non-transparent // color set. diff --git a/engine/src/flutter/shell/common/dl_op_spy_unittests.cc b/engine/src/flutter/shell/common/dl_op_spy_unittests.cc index b04c6d6f17..8dc48734da 100644 --- a/engine/src/flutter/shell/common/dl_op_spy_unittests.cc +++ b/engine/src/flutter/shell/common/dl_op_spy_unittests.cc @@ -84,29 +84,26 @@ TEST(DlOpSpy, SetColorSource) { dl->Dispatch(dl_op_spy); ASSERT_DID_DRAW(dl_op_spy, dl); } - { // Set transparent color. - DisplayListBuilder builder; - DlPaint paint; - auto color = DlColor::kTransparent(); - DlColorColorSource color_source_transparent(color); - paint.setColorSource(color_source_transparent.shared()); - builder.DrawRect(SkRect::MakeWH(5, 5), paint); - sk_sp dl = builder.Build(); + { // setColorSource(null) restores previous color visibility DlOpSpy dl_op_spy; - dl->Dispatch(dl_op_spy); - ASSERT_NO_DRAW(dl_op_spy, dl); - } - { // Set black color. - DisplayListBuilder builder; - DlPaint paint; - auto color = DlColor::kBlack(); - DlColorColorSource color_source_transparent(color); - paint.setColorSource(color_source_transparent.shared()); - builder.DrawRect(SkRect::MakeWH(5, 5), paint); - sk_sp dl = builder.Build(); - DlOpSpy dl_op_spy; - dl->Dispatch(dl_op_spy); - ASSERT_DID_DRAW(dl_op_spy, dl); + DlOpReceiver* receiver = &dl_op_spy; + receiver->setColor(DlColor::kTransparent()); + receiver->drawRect(DlRect::MakeWH(5, 5)); + ASSERT_FALSE(dl_op_spy.did_draw()); + DlColor colors[2] = { + DlColor::kGreen(), + DlColor::kBlue(), + }; + float stops[2] = { + 0.0f, + 1.0f, + }; + auto color_source = DlColorSource::MakeLinear({0, 0}, {10, 10}, 2, colors, + stops, DlTileMode::kClamp); + receiver->setColorSource(color_source.get()); + receiver->setColorSource(nullptr); + receiver->drawRect(DlRect::MakeWH(5, 5)); + ASSERT_FALSE(dl_op_spy.did_draw()); } } diff --git a/engine/src/flutter/testing/display_list_testing.cc b/engine/src/flutter/testing/display_list_testing.cc index b969daa41c..b3c6fe65ba 100644 --- a/engine/src/flutter/testing/display_list_testing.cc +++ b/engine/src/flutter/testing/display_list_testing.cc @@ -444,12 +444,6 @@ void DisplayListStreamDispatcher::setColorSource(const DlColorSource* source) { } startl() << "setColorSource("; switch (source->type()) { - case DlColorSourceType::kColor: { - const DlColorColorSource* color_src = source->asColor(); - FML_DCHECK(color_src); - os_ << "DlColorColorSource(" << color_src->color() << ")"; - break; - } case DlColorSourceType::kImage: { const DlImageColorSource* image_src = source->asImage(); FML_DCHECK(image_src); diff --git a/engine/src/flutter/testing/display_list_testing.h b/engine/src/flutter/testing/display_list_testing.h index 9a707bb862..17cf01d023 100644 --- a/engine/src/flutter/testing/display_list_testing.h +++ b/engine/src/flutter/testing/display_list_testing.h @@ -263,7 +263,6 @@ class DisplayListGeneralReceiver : public DlOpReceiver { case DlColorSourceType::kRuntimeEffect: RecordByType(DisplayListOpType::kSetRuntimeEffectColorSource); break; - case DlColorSourceType::kColor: case DlColorSourceType::kLinearGradient: case DlColorSourceType::kRadialGradient: case DlColorSourceType::kConicalGradient: diff --git a/engine/src/flutter/third_party/txt/src/skia/paragraph_skia.cc b/engine/src/flutter/third_party/txt/src/skia/paragraph_skia.cc index b3521dd529..38065e9c59 100644 --- a/engine/src/flutter/third_party/txt/src/skia/paragraph_skia.cc +++ b/engine/src/flutter/third_party/txt/src/skia/paragraph_skia.cc @@ -191,7 +191,7 @@ class DisplayListParagraphPainter : public skt::ParagraphPainter { // rendering will be faster as it avoids software rasterization. A stroke // width of four was chosen by eyeballing the point at which the path // text looks good enough, with some room for error. - return (paint.getColorSource() && !paint.getColorSource()->asColor()) || + return paint.getColorSource() || (paint.getDrawStyle() == DlDrawStyle::kStroke && paint.getStrokeWidth() > 4); }