From 3a5678b96f563e1894e8e876fe893919e77ae188 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Tue, 10 Oct 2023 17:45:33 -0700 Subject: [PATCH 01/11] WIP. --- .../benchmarking/dl_complexity_helper.h | 1 - display_list/display_list.h | 1 - display_list/display_list_unittests.cc | 2 - display_list/dl_builder.cc | 7 --- display_list/dl_builder.h | 7 --- display_list/dl_op_receiver.h | 1 - display_list/dl_op_records.h | 1 - display_list/dl_paint.h | 6 --- display_list/dl_paint_unittests.cc | 4 -- display_list/skia/dl_sk_conversions.cc | 8 ++-- .../skia/dl_sk_conversions_unittests.cc | 22 ---------- display_list/skia/dl_sk_paint_dispatcher.cc | 3 -- display_list/skia/dl_sk_paint_dispatcher.h | 9 ++-- .../skia/dl_sk_paint_dispatcher_unittests.cc | 44 +------------------ .../testing/dl_rendering_unittests.cc | 18 +------- display_list/testing/dl_test_snippets.cc | 5 --- display_list/utils/dl_receiver_utils.h | 1 - lib/ui/painting/paint.cc | 6 --- testing/display_list_testing.h | 1 - 19 files changed, 10 insertions(+), 137 deletions(-) diff --git a/display_list/benchmarking/dl_complexity_helper.h b/display_list/benchmarking/dl_complexity_helper.h index 871937a77a17c..49fe51004bc07 100644 --- a/display_list/benchmarking/dl_complexity_helper.h +++ b/display_list/benchmarking/dl_complexity_helper.h @@ -101,7 +101,6 @@ class ComplexityCalculatorHelper virtual ~ComplexityCalculatorHelper() = default; - void setDither(bool dither) override {} void setInvertColors(bool invert) override {} void setStrokeCap(DlStrokeCap cap) override {} void setStrokeJoin(DlStrokeJoin join) override {} diff --git a/display_list/display_list.h b/display_list/display_list.h index 621338e59fd1e..d7225865edb92 100644 --- a/display_list/display_list.h +++ b/display_list/display_list.h @@ -56,7 +56,6 @@ namespace flutter { #define FOR_EACH_DISPLAY_LIST_OP(V) \ V(SetAntiAlias) \ - V(SetDither) \ V(SetInvertColors) \ \ V(SetStrokeCap) \ diff --git a/display_list/display_list_unittests.cc b/display_list/display_list_unittests.cc index 78185657d8d43..2763d400bb853 100644 --- a/display_list/display_list_unittests.cc +++ b/display_list/display_list_unittests.cc @@ -101,7 +101,6 @@ class DisplayListTestBase : public BaseT { DlPaint defaults; EXPECT_EQ(builder_paint.isAntiAlias(), defaults.isAntiAlias()); - EXPECT_EQ(builder_paint.isDither(), defaults.isDither()); EXPECT_EQ(builder_paint.isInvertColors(), defaults.isInvertColors()); EXPECT_EQ(builder_paint.getColor(), defaults.getColor()); EXPECT_EQ(builder_paint.getBlendMode(), defaults.getBlendMode()); @@ -387,7 +386,6 @@ TEST_F(DisplayListTest, BuildRestoresAttributes) { builder.Build(); check_defaults(builder, cull_rect); - receiver.setDither(true); builder.Build(); check_defaults(builder, cull_rect); diff --git a/display_list/dl_builder.cc b/display_list/dl_builder.cc index 923d32230f632..dc69687293da2 100644 --- a/display_list/dl_builder.cc +++ b/display_list/dl_builder.cc @@ -123,10 +123,6 @@ void DisplayListBuilder::onSetAntiAlias(bool aa) { current_.setAntiAlias(aa); Push(0, 0, aa); } -void DisplayListBuilder::onSetDither(bool dither) { - current_.setDither(dither); - Push(0, 0, dither); -} void DisplayListBuilder::onSetInvertColors(bool invert) { current_.setInvertColors(invert); Push(0, 0, invert); @@ -347,9 +343,6 @@ void DisplayListBuilder::SetAttributesFromPaint( if (flags.applies_anti_alias()) { setAntiAlias(paint.isAntiAlias()); } - if (flags.applies_dither()) { - setDither(paint.isDither()); - } if (flags.applies_alpha_or_color()) { setColor(paint.getColor()); } diff --git a/display_list/dl_builder.h b/display_list/dl_builder.h index 3a65b8fad5328..b6400da3ed70a 100644 --- a/display_list/dl_builder.h +++ b/display_list/dl_builder.h @@ -270,12 +270,6 @@ class DisplayListBuilder final : public virtual DlCanvas, } } // |DlOpReceiver| - void setDither(bool dither) override { - if (current_.isDither() != dither) { - onSetDither(dither); - } - } - // |DlOpReceiver| void setInvertColors(bool invert) override { if (current_.isInvertColors() != invert) { onSetInvertColors(invert); @@ -673,7 +667,6 @@ class DisplayListBuilder final : public virtual DlCanvas, } void onSetAntiAlias(bool aa); - void onSetDither(bool dither); void onSetInvertColors(bool invert); void onSetStrokeCap(DlStrokeCap cap); void onSetStrokeJoin(DlStrokeJoin join); diff --git a/display_list/dl_op_receiver.h b/display_list/dl_op_receiver.h index d9bbc7b7f4f3f..3a13ce6935be6 100644 --- a/display_list/dl_op_receiver.h +++ b/display_list/dl_op_receiver.h @@ -55,7 +55,6 @@ class DlOpReceiver { // another method that changes the same attribute. The current set of // attributes is not affected by |save| and |restore|. virtual void setAntiAlias(bool aa) = 0; - virtual void setDither(bool dither) = 0; virtual void setDrawStyle(DlDrawStyle style) = 0; virtual void setColor(DlColor color) = 0; virtual void setStrokeWidth(float width) = 0; diff --git a/display_list/dl_op_records.h b/display_list/dl_op_records.h index 64c54f583cf70..fa9aeaafa0034 100644 --- a/display_list/dl_op_records.h +++ b/display_list/dl_op_records.h @@ -117,7 +117,6 @@ struct DLOp { } \ }; DEFINE_SET_BOOL_OP(AntiAlias) -DEFINE_SET_BOOL_OP(Dither) DEFINE_SET_BOOL_OP(InvertColors) #undef DEFINE_SET_BOOL_OP diff --git a/display_list/dl_paint.h b/display_list/dl_paint.h index 4d09a39ce67fa..44c8370649d0b 100644 --- a/display_list/dl_paint.h +++ b/display_list/dl_paint.h @@ -61,12 +61,6 @@ class DlPaint { return *this; } - bool isDither() const { return is_dither_; } - DlPaint& setDither(bool isDither) { - is_dither_ = isDither; - return *this; - } - bool isInvertColors() const { return is_invert_colors_; } DlPaint& setInvertColors(bool isInvertColors) { is_invert_colors_ = isInvertColors; diff --git a/display_list/dl_paint_unittests.cc b/display_list/dl_paint_unittests.cc index 2a34c466f60d1..e6a2b02e0308e 100644 --- a/display_list/dl_paint_unittests.cc +++ b/display_list/dl_paint_unittests.cc @@ -13,7 +13,6 @@ namespace testing { TEST(DisplayListPaint, ConstructorDefaults) { DlPaint paint; EXPECT_FALSE(paint.isAntiAlias()); - EXPECT_FALSE(paint.isDither()); EXPECT_FALSE(paint.isInvertColors()); EXPECT_EQ(paint.getColor(), DlPaint::kDefaultColor); EXPECT_EQ(paint.getAlpha(), 0xFF); @@ -45,7 +44,6 @@ TEST(DisplayListPaint, ConstructorDefaults) { EXPECT_EQ(paint, DlPaint(DlColor(0xFF000000))); EXPECT_NE(paint, DlPaint().setAntiAlias(true)); - EXPECT_NE(paint, DlPaint().setDither(true)); EXPECT_NE(paint, DlPaint().setInvertColors(true)); EXPECT_NE(paint, DlPaint().setColor(DlColor::kGreen())); EXPECT_NE(paint, DlPaint(DlColor::kGreen())); @@ -110,7 +108,6 @@ TEST(DisplayListPaint, ChainingConstructor) { DlPaint paint = DlPaint() // .setAntiAlias(true) // - .setDither(true) // .setInvertColors(true) // .setColor(DlColor::kGreen()) // .setAlpha(0x7F) // @@ -129,7 +126,6 @@ TEST(DisplayListPaint, ChainingConstructor) { .setMaskFilter(DlBlurMaskFilter(DlBlurStyle::kInner, 3.14).shared()) .setPathEffect(path_effect); EXPECT_TRUE(paint.isAntiAlias()); - EXPECT_TRUE(paint.isDither()); EXPECT_TRUE(paint.isInvertColors()); EXPECT_EQ(paint.getColor(), DlColor::kGreen().withAlpha(0x7F)); EXPECT_EQ(paint.getAlpha(), 0x7F); diff --git a/display_list/skia/dl_sk_conversions.cc b/display_list/skia/dl_sk_conversions.cc index 2e7a42f5f91a4..96c611f14c7fa 100644 --- a/display_list/skia/dl_sk_conversions.cc +++ b/display_list/skia/dl_sk_conversions.cc @@ -51,12 +51,10 @@ SkPaint ToSk(const DlPaint& paint, bool force_stroke) { // images, which is not supported in Impeller). // // See https://github.com/flutter/flutter/issues/112498. - if (color_source->isGradient()) { - // Originates from `dart:ui#Paint.enableDithering`. - auto user_specified_dither = paint.isDither(); - sk_paint.setDither(user_specified_dither); - } + sk_paint.setDither(color_source->isGradient()); sk_paint.setShader(ToSk(color_source)); + } else { + sk_paint.setDither(false); } sk_paint.setMaskFilter(ToSk(paint.getMaskFilterPtr())); diff --git a/display_list/skia/dl_sk_conversions_unittests.cc b/display_list/skia/dl_sk_conversions_unittests.cc index be782181d994e..7db166067ebd5 100644 --- a/display_list/skia/dl_sk_conversions_unittests.cc +++ b/display_list/skia/dl_sk_conversions_unittests.cc @@ -286,33 +286,12 @@ TEST(DisplayListSkConversions, MatrixColorFilterModifiesTransparency) { } } -TEST(DisplayListSkConversions, ToSkDitheringDisabledForGradients) { - // Test that when using the utility method "ToSk", the resulting SkPaint - // does not have "isDither" set to true, even if it's requested by the - // Flutter (dart:ui) paint, because it's not a supported feature in the - // Impeller backend. - - // Create a new DlPaint with isDither set to true. - // - // This mimics the behavior of ui.Paint.enableDithering = true. - DlPaint dl_paint; - dl_paint.setDither(true); - - SkPaint sk_paint = ToSk(dl_paint); - - EXPECT_FALSE(sk_paint.isDither()); -} - TEST(DisplayListSkConversions, ToSkDitheringEnabledForGradients) { // Test that when using the utility method "ToSk", the resulting SkPaint // has "isDither" set to true, if the paint is a gradient, because it's // a supported feature in the Impeller backend. - // Create a new DlPaint with isDither set to true. - // - // This mimics the behavior of ui.Paint.enableDithering = true. DlPaint dl_paint; - dl_paint.setDither(true); // Set the paint to be a gradient. dl_paint.setColorSource(DlColorSource::MakeLinear(SkPoint::Make(0, 0), @@ -320,7 +299,6 @@ TEST(DisplayListSkConversions, ToSkDitheringEnabledForGradients) { 0, 0, DlTileMode::kClamp)); SkPaint sk_paint = ToSk(dl_paint); - EXPECT_TRUE(sk_paint.isDither()); } diff --git a/display_list/skia/dl_sk_paint_dispatcher.cc b/display_list/skia/dl_sk_paint_dispatcher.cc index 43562405d4e23..0bd9c09b7641a 100644 --- a/display_list/skia/dl_sk_paint_dispatcher.cc +++ b/display_list/skia/dl_sk_paint_dispatcher.cc @@ -40,9 +40,6 @@ void DlSkPaintDispatchHelper::restore_opacity() { void DlSkPaintDispatchHelper::setAntiAlias(bool aa) { paint_.setAntiAlias(aa); } -void DlSkPaintDispatchHelper::setDither(bool dither) { - dither_ = dither; -} void DlSkPaintDispatchHelper::setInvertColors(bool invert) { invert_colors_ = invert; paint_.setColorFilter(makeColorFilter()); diff --git a/display_list/skia/dl_sk_paint_dispatcher.h b/display_list/skia/dl_sk_paint_dispatcher.h index dcbde11fba764..ce8dfb2c2c6ae 100644 --- a/display_list/skia/dl_sk_paint_dispatcher.h +++ b/display_list/skia/dl_sk_paint_dispatcher.h @@ -23,7 +23,6 @@ class DlSkPaintDispatchHelper : public virtual DlOpReceiver { } void setAntiAlias(bool aa) override; - void setDither(bool dither) override; void setDrawStyle(DlDrawStyle style) override; void setColor(DlColor color) override; void setStrokeWidth(SkScalar width) override; @@ -41,12 +40,12 @@ class DlSkPaintDispatchHelper : public virtual DlOpReceiver { const SkPaint& paint() { // On the Impeller backend, we will only support dithering of *gradients*, // and it will be enabled by default (without the option to disable it). - // Until Skia support is completely removed, we only want to respect the - // dither flag for gradients (otherwise it will also apply to, for - // example, images, which is not supported in Impeller). + // Until Skia support is completely removed, we only want to dither + // gradients (otherwise it will also apply to, for example, images, which is + // not supported in Impeller). // // See https://github.com/flutter/flutter/issues/112498. - paint_.setDither(color_source_gradient_ && dither_); + paint_.setDither(color_source_gradient_); return paint_; } diff --git a/display_list/skia/dl_sk_paint_dispatcher_unittests.cc b/display_list/skia/dl_sk_paint_dispatcher_unittests.cc index 2b638845757f9..a96dfee4699a3 100644 --- a/display_list/skia/dl_sk_paint_dispatcher_unittests.cc +++ b/display_list/skia/dl_sk_paint_dispatcher_unittests.cc @@ -45,60 +45,20 @@ TEST(DisplayListUtils, OverRestore) { } // https://github.com/flutter/flutter/issues/132860. -TEST(DisplayListUtils, SetDitherIgnoredIfColorSourceNotGradient) { +TEST(DisplayListUtils, SetColorSourceDithersIfGradient) { MockDispatchHelper helper; - helper.setDither(true); - EXPECT_FALSE(helper.paint().isDither()); -} -// https://github.com/flutter/flutter/issues/132860. -TEST(DisplayListUtils, SetColorSourceClearsDitherIfNotGradient) { - MockDispatchHelper helper; - helper.setDither(true); - helper.setColorSource(nullptr); - EXPECT_FALSE(helper.paint().isDither()); -} - -// https://github.com/flutter/flutter/issues/132860. -TEST(DisplayListUtils, SetDitherTrueThenSetColorSourceDithersIfGradient) { - MockDispatchHelper helper; - - // A naive implementation would ignore the dither flag here since the current - // color source is not a gradient. - helper.setDither(true); helper.setColorSource(kTestLinearGradient.get()); EXPECT_TRUE(helper.paint().isDither()); } // https://github.com/flutter/flutter/issues/132860. -TEST(DisplayListUtils, SetDitherTrueThenRenderNonGradientThenRenderGradient) { +TEST(DisplayListUtils, SetColorSourceDoesNotDitherIfNotGradient) { MockDispatchHelper helper; - // "Render" a dithered non-gradient. - helper.setDither(true); - EXPECT_FALSE(helper.paint().isDither()); - - // "Render" a gradient. helper.setColorSource(kTestLinearGradient.get()); - EXPECT_TRUE(helper.paint().isDither()); -} - -// https://github.com/flutter/flutter/issues/132860. -TEST(DisplayListUtils, SetDitherTrueThenGradientTHenNonGradientThenGradient) { - MockDispatchHelper helper; - - // "Render" a dithered gradient. - helper.setDither(true); - helper.setColorSource(kTestLinearGradient.get()); - EXPECT_TRUE(helper.paint().isDither()); - - // "Render" a non-gradient. helper.setColorSource(nullptr); EXPECT_FALSE(helper.paint().isDither()); - - // "Render" a gradient again. - helper.setColorSource(kTestLinearGradient.get()); - EXPECT_TRUE(helper.paint().isDither()); } } // namespace testing diff --git a/display_list/testing/dl_rendering_unittests.cc b/display_list/testing/dl_rendering_unittests.cc index 1704db6488361..4a9d88e1d613c 100644 --- a/display_list/testing/dl_rendering_unittests.cc +++ b/display_list/testing/dl_rendering_unittests.cc @@ -871,10 +871,6 @@ class TestParameters { return false; } } - if (flags_.applies_dither() && // - ref_attr.isDither() != attr.isDither()) { - return false; - } if (flags_.applies_color() && // ref_attr.getColor() != attr.getColor()) { return false; @@ -1819,19 +1815,7 @@ class CanvasCompareTester { }, [=](const DlSetupContext& ctx) { dl_dither_setup(ctx); - ctx.paint.setDither(true); - }) - .with_bg(dither_bg)); - RenderWith(testP, dither_env, tolerance, - CaseParameters( - "Dither = False", - [=](const SkSetupContext& ctx) { - sk_dither_setup(ctx); - ctx.paint.setDither(false); - }, - [=](const DlSetupContext& ctx) { - dl_dither_setup(ctx); - ctx.paint.setDither(false); + // Dithering is implicitly enabled for gradients. }) .with_bg(dither_bg)); } diff --git a/display_list/testing/dl_test_snippets.cc b/display_list/testing/dl_test_snippets.cc index 714da0cf732b6..3cbd39362b016 100644 --- a/display_list/testing/dl_test_snippets.cc +++ b/display_list/testing/dl_test_snippets.cc @@ -49,11 +49,6 @@ std::vector CreateAllAttributesOps() { {0, 8, 0, 0, [](DlOpReceiver& r) { r.setAntiAlias(true); }}, {0, 0, 0, 0, [](DlOpReceiver& r) { r.setAntiAlias(false); }}, }}, - {"SetDither", - { - {0, 8, 0, 0, [](DlOpReceiver& r) { r.setDither(true); }}, - {0, 0, 0, 0, [](DlOpReceiver& r) { r.setDither(false); }}, - }}, {"SetInvertColors", { {0, 8, 0, 0, [](DlOpReceiver& r) { r.setInvertColors(true); }}, diff --git a/display_list/utils/dl_receiver_utils.h b/display_list/utils/dl_receiver_utils.h index 2a1f9cb7efff4..4450cefdbe5ba 100644 --- a/display_list/utils/dl_receiver_utils.h +++ b/display_list/utils/dl_receiver_utils.h @@ -24,7 +24,6 @@ namespace flutter { class IgnoreAttributeDispatchHelper : public virtual DlOpReceiver { public: void setAntiAlias(bool aa) override {} - void setDither(bool dither) override {} void setInvertColors(bool invert) override {} void setStrokeCap(DlStrokeCap cap) override {} void setStrokeJoin(DlStrokeJoin join) override {} diff --git a/lib/ui/painting/paint.cc b/lib/ui/painting/paint.cc index 0614f3f966cea..ba91c67fe4016 100644 --- a/lib/ui/painting/paint.cc +++ b/lib/ui/painting/paint.cc @@ -169,10 +169,6 @@ const DlPaint* Paint::paint(DlPaint& paint, paint.setInvertColors(uint_data[kInvertColorIndex] != 0); } - if (flags.applies_dither()) { - paint.setDither(uint_data[kDitherIndex] != 0); - } - if (flags.applies_path_effect()) { // The paint API exposed to Dart does not support path effects. But other // operations such as text may set a path effect, which must be cleared. @@ -271,8 +267,6 @@ void Paint::toDlPaint(DlPaint& paint) const { paint.setInvertColors(uint_data[kInvertColorIndex] != 0); - paint.setDither(uint_data[kDitherIndex] != 0); - switch (uint_data[kMaskFilterIndex]) { case kNull: break; diff --git a/testing/display_list_testing.h b/testing/display_list_testing.h index 8604906753deb..7a7eac9c493d8 100644 --- a/testing/display_list_testing.h +++ b/testing/display_list_testing.h @@ -58,7 +58,6 @@ class DisplayListStreamDispatcher final : public DlOpReceiver { : os_(os), cur_indent_(cur_indent), indent_(indent) {} void setAntiAlias(bool aa) override; - void setDither(bool dither) override; void setDrawStyle(DlDrawStyle style) override; void setColor(DlColor color) override; void setStrokeWidth(SkScalar width) override; From b27c9df4902f6260445d1bae25aa44378cd468d4 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Tue, 10 Oct 2023 17:47:02 -0700 Subject: [PATCH 02/11] More WIP. --- impeller/display_list/dl_dispatcher.cc | 5 ----- impeller/display_list/dl_dispatcher.h | 3 --- 2 files changed, 8 deletions(-) diff --git a/impeller/display_list/dl_dispatcher.cc b/impeller/display_list/dl_dispatcher.cc index 7796efc26b8b6..cbc4ee6ff5461 100644 --- a/impeller/display_list/dl_dispatcher.cc +++ b/impeller/display_list/dl_dispatcher.cc @@ -179,11 +179,6 @@ void DlDispatcher::setAntiAlias(bool aa) { // Nothing to do because AA is implicit. } -// |flutter::DlOpReceiver| -void DlDispatcher::setDither(bool dither) { - paint_.dither = dither; -} - static Paint::Style ToStyle(flutter::DlDrawStyle style) { switch (style) { case flutter::DlDrawStyle::kFill: diff --git a/impeller/display_list/dl_dispatcher.h b/impeller/display_list/dl_dispatcher.h index 8f0136a020917..5d3dcb6cad212 100644 --- a/impeller/display_list/dl_dispatcher.h +++ b/impeller/display_list/dl_dispatcher.h @@ -26,9 +26,6 @@ class DlDispatcher final : public flutter::DlOpReceiver { // |flutter::DlOpReceiver| void setAntiAlias(bool aa) override; - // |flutter::DlOpReceiver| - void setDither(bool dither) override; - // |flutter::DlOpReceiver| void setDrawStyle(flutter::DlDrawStyle style) override; From 543b2aef44be160cbe51d0f71a8a6f74ef0d8493 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Tue, 10 Oct 2023 17:50:07 -0700 Subject: [PATCH 03/11] Finish removing. --- impeller/entity/contents/sweep_gradient_contents.h | 2 -- testing/display_list_testing.cc | 3 --- 2 files changed, 5 deletions(-) diff --git a/impeller/entity/contents/sweep_gradient_contents.h b/impeller/entity/contents/sweep_gradient_contents.h index 6d76baf39a69d..0351ce95491a9 100644 --- a/impeller/entity/contents/sweep_gradient_contents.h +++ b/impeller/entity/contents/sweep_gradient_contents.h @@ -45,8 +45,6 @@ class SweepGradientContents final : public ColorSourceContents { void SetTileMode(Entity::TileMode tile_mode); - void SetDither(bool dither); - const std::vector& GetColors() const; const std::vector& GetStops() const; diff --git a/testing/display_list_testing.cc b/testing/display_list_testing.cc index 1fa3561ddd45e..bad2d65c0bf8f 100644 --- a/testing/display_list_testing.cc +++ b/testing/display_list_testing.cc @@ -362,9 +362,6 @@ std::ostream& DisplayListStreamDispatcher::out_array(std::string name, // NOLIN void DisplayListStreamDispatcher::setAntiAlias(bool aa) { startl() << "setAntiAlias(" << aa << ");" << std::endl; } -void DisplayListStreamDispatcher::setDither(bool dither) { - startl() << "setDither(" << dither << ");" << std::endl; -} void DisplayListStreamDispatcher::setDrawStyle(DlDrawStyle style) { startl() << "setStyle(" << style << ");" << std::endl; } From 8f0a989161e5bf65ac17de811d2ca50a0a2b24b3 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Mon, 30 Oct 2023 13:42:40 -0700 Subject: [PATCH 04/11] Remove removed isDither call for << operator. --- testing/display_list_testing.cc | 3 --- 1 file changed, 3 deletions(-) diff --git a/testing/display_list_testing.cc b/testing/display_list_testing.cc index bad2d65c0bf8f..908afd9432755 100644 --- a/testing/display_list_testing.cc +++ b/testing/display_list_testing.cc @@ -70,9 +70,6 @@ std::ostream& operator<<(std::ostream& os, const DlPaint& paint) { if (paint.getMaskFilter()) { os << ", " << paint.getMaskFilter(); } - if (paint.isDither()) { - os << ", dither: " << paint.isDither(); - } if (paint.isInvertColors()) { os << ", invertColors: " << paint.isInvertColors(); } From e17ddfed224910b10c4ac18c141dbbc6d46d76f8 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Mon, 30 Oct 2023 13:58:13 -0700 Subject: [PATCH 05/11] Remove unused field. --- display_list/skia/dl_sk_paint_dispatcher.h | 1 - 1 file changed, 1 deletion(-) diff --git a/display_list/skia/dl_sk_paint_dispatcher.h b/display_list/skia/dl_sk_paint_dispatcher.h index ce8dfb2c2c6ae..7e0d2f06d5b8b 100644 --- a/display_list/skia/dl_sk_paint_dispatcher.h +++ b/display_list/skia/dl_sk_paint_dispatcher.h @@ -68,7 +68,6 @@ class DlSkPaintDispatchHelper : public virtual DlOpReceiver { private: SkPaint paint_; bool color_source_gradient_ = false; - bool dither_ = false; bool invert_colors_ = false; sk_sp sk_color_filter_; From 9a40ed46046d70497601f26828b1fcd946392563 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Mon, 30 Oct 2023 14:12:48 -0700 Subject: [PATCH 06/11] Finish cleanup. --- lib/ui/painting.dart | 15 +++------------ lib/ui/painting/paint.cc | 5 +++-- 2 files changed, 6 insertions(+), 14 deletions(-) diff --git a/lib/ui/painting.dart b/lib/ui/painting.dart index 1fbf1cae9fdd0..d77ecb363f27a 100644 --- a/lib/ui/painting.dart +++ b/lib/ui/painting.dart @@ -1090,10 +1090,7 @@ enum Clip { class Paint { /// Constructs an empty [Paint] object with all fields initialized to /// their defaults. - Paint() { - // TODO(matanlurey): Remove as part of https://github.com/flutter/flutter/issues/112498. - _enableDithering(); - } + Paint(); // Paint objects are encoded in two buffers: // @@ -1126,7 +1123,6 @@ class Paint { static const int _kMaskFilterBlurStyleIndex = 10; static const int _kMaskFilterSigmaIndex = 11; static const int _kInvertColorIndex = 12; - static const int _kDitherIndex = 13; static const int _kIsAntiAliasOffset = _kIsAntiAliasIndex << 2; static const int _kColorOffset = _kColorIndex << 2; @@ -1141,9 +1137,9 @@ class Paint { static const int _kMaskFilterBlurStyleOffset = _kMaskFilterBlurStyleIndex << 2; static const int _kMaskFilterSigmaOffset = _kMaskFilterSigmaIndex << 2; static const int _kInvertColorOffset = _kInvertColorIndex << 2; - static const int _kDitherOffset = _kDitherIndex << 2; + // If you add more fields, remember to update _kDataByteCount. - static const int _kDataByteCount = 56; + static const int _kDataByteCount = 52; // 4 * (last index + 1). // Binary format must match the deserialization code in paint.cc. // C++ unit tests access this. @@ -1478,11 +1474,6 @@ class Paint { _data.setInt32(_kInvertColorOffset, value ? 1 : 0, _kFakeHostEndian); } - // TODO(matanlurey): Remove as part of https://github.com/flutter/flutter/issues/112498. - void _enableDithering() { - _data.setInt32(_kDitherOffset, 1, _kFakeHostEndian); - } - @override String toString() { if (const bool.fromEnvironment('dart.vm.product')) { diff --git a/lib/ui/painting/paint.cc b/lib/ui/painting/paint.cc index ba91c67fe4016..6b3dbfec7c4ef 100644 --- a/lib/ui/painting/paint.cc +++ b/lib/ui/painting/paint.cc @@ -34,8 +34,9 @@ constexpr int kMaskFilterIndex = 9; constexpr int kMaskFilterBlurStyleIndex = 10; constexpr int kMaskFilterSigmaIndex = 11; constexpr int kInvertColorIndex = 12; -constexpr int kDitherIndex = 13; -constexpr size_t kDataByteCount = 56; // 4 * (last index + 1) +constexpr size_t kDataByteCount = 52; // 4 * (last index + 1) +static_assert(kDataByteCount == sizeof(uint32_t) * (kInvertColorIndex + 1), + "kDataByteCount must match the size of the data array."); // Indices for objects. constexpr int kShaderIndex = 0; From 8fc1180dc3729105d5288ef903c5ed728ab3baf3 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Mon, 30 Oct 2023 14:13:26 -0700 Subject: [PATCH 07/11] Format. --- display_list/display_list_unittests.cc | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/display_list/display_list_unittests.cc b/display_list/display_list_unittests.cc index 2763d400bb853..06a48cdf18003 100644 --- a/display_list/display_list_unittests.cc +++ b/display_list/display_list_unittests.cc @@ -1058,11 +1058,9 @@ TEST_F(DisplayListTest, SingleOpsMightSupportGroupOpacityBlendMode) { }; #define RUN_TESTS(body) \ - run_tests( \ - #body, [](DlOpReceiver& receiver) { body }, true, false) + run_tests(#body, [](DlOpReceiver& receiver) { body }, true, false) #define RUN_TESTS2(body, expect) \ - run_tests( \ - #body, [](DlOpReceiver& receiver) { body }, expect, expect) + run_tests(#body, [](DlOpReceiver& receiver) { body }, expect, expect) RUN_TESTS(receiver.drawPaint();); RUN_TESTS2(receiver.drawColor(DlColor(SK_ColorRED), DlBlendMode::kSrcOver); From 7a2bdf818dbd3e5cd2331223ae43f7074a7ed288 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Tue, 31 Oct 2023 10:20:35 -0700 Subject: [PATCH 08/11] Address comments. --- display_list/display_list_unittests.cc | 3 -- display_list/dl_op_flags.h | 28 +++++-------- display_list/dl_paint.cc | 2 - display_list/dl_paint.h | 1 - display_list/skia/dl_sk_conversions.cc | 2 - display_list/skia/dl_sk_paint_dispatcher.cc | 7 +++- display_list/skia/dl_sk_paint_dispatcher.h | 13 +----- .../testing/dl_rendering_unittests.cc | 42 ------------------- 8 files changed, 18 insertions(+), 80 deletions(-) diff --git a/display_list/display_list_unittests.cc b/display_list/display_list_unittests.cc index 06a48cdf18003..27dc400d46776 100644 --- a/display_list/display_list_unittests.cc +++ b/display_list/display_list_unittests.cc @@ -386,9 +386,6 @@ TEST_F(DisplayListTest, BuildRestoresAttributes) { builder.Build(); check_defaults(builder, cull_rect); - builder.Build(); - check_defaults(builder, cull_rect); - receiver.setInvertColors(true); builder.Build(); check_defaults(builder, cull_rect); diff --git a/display_list/dl_op_flags.h b/display_list/dl_op_flags.h index 2f6a4d936c192..ec9f7749e1692 100644 --- a/display_list/dl_op_flags.h +++ b/display_list/dl_op_flags.h @@ -81,15 +81,14 @@ class DisplayListFlags { // clang-format off static constexpr int kUsesAntiAlias = 1 << 10; - static constexpr int kUsesDither = 1 << 11; - static constexpr int kUsesAlpha = 1 << 12; - static constexpr int kUsesColor = 1 << 13; - static constexpr int kUsesBlend = 1 << 14; - static constexpr int kUsesShader = 1 << 15; - static constexpr int kUsesColorFilter = 1 << 16; - static constexpr int kUsesPathEffect = 1 << 17; - static constexpr int kUsesMaskFilter = 1 << 18; - static constexpr int kUsesImageFilter = 1 << 19; + static constexpr int kUsesAlpha = 1 << 11; + static constexpr int kUsesColor = 1 << 12; + static constexpr int kUsesBlend = 1 << 13; + static constexpr int kUsesShader = 1 << 14; + static constexpr int kUsesColorFilter = 1 << 15; + static constexpr int kUsesPathEffect = 1 << 16; + static constexpr int kUsesMaskFilter = 1 << 17; + static constexpr int kUsesImageFilter = 1 << 18; // Some ops have an optional paint argument. If the version // stored in the DisplayList ignores the paint, but there @@ -102,9 +101,8 @@ class DisplayListFlags { // clang-format on static constexpr int kAnyAttributeMask = // - kUsesAntiAlias | kUsesDither | kUsesAlpha | kUsesColor | kUsesBlend | - kUsesShader | kUsesColorFilter | kUsesPathEffect | kUsesMaskFilter | - kUsesImageFilter; + kUsesAntiAlias | kUsesAlpha | kUsesColor | kUsesBlend | kUsesShader | + kUsesColorFilter | kUsesPathEffect | kUsesMaskFilter | kUsesImageFilter; }; class DisplayListFlagsBase : protected DisplayListFlags { @@ -173,7 +171,6 @@ class DisplayListAttributeFlags : DisplayListFlagsBase { constexpr bool ignores_paint() const { return has_any(kIgnoresPaint); } constexpr bool applies_anti_alias() const { return has_any(kUsesAntiAlias); } - constexpr bool applies_dither() const { return has_any(kUsesDither); } constexpr bool applies_color() const { return has_any(kUsesColor); } constexpr bool applies_alpha() const { return has_any(kUsesAlpha); } constexpr bool applies_alpha_or_color() const { @@ -257,8 +254,7 @@ class DisplayListAttributeFlags : DisplayListFlagsBase { class DisplayListOpFlags : DisplayListFlags { private: // Flags common to all primitives that apply colors - static constexpr int kBasePaintFlags = (kUsesDither | // - kUsesColor | // + static constexpr int kBasePaintFlags = (kUsesColor | // kUsesAlpha | // kUsesBlend | // kUsesShader | // @@ -280,7 +276,6 @@ class DisplayListOpFlags : DisplayListFlags { // Flags common to primitives that render an image with paint attributes static constexpr int kBaseImageFlags = (kIsNonGeometric | // kUsesAlpha | // - kUsesDither | // kUsesBlend | // kUsesColorFilter | // kUsesImageFilter); @@ -376,7 +371,6 @@ class DisplayListOpFlags : DisplayListFlags { }; static constexpr DisplayListAttributeFlags kDrawVerticesFlags{ kIsNonGeometric | // - kUsesDither | // kUsesAlpha | // kUsesShader | // kUsesBlend | // diff --git a/display_list/dl_paint.cc b/display_list/dl_paint.cc index 2ce1ec763277a..81635a2a9f48d 100644 --- a/display_list/dl_paint.cc +++ b/display_list/dl_paint.cc @@ -12,7 +12,6 @@ DlPaint::DlPaint(DlColor color) stroke_cap_(static_cast(DlStrokeCap::kDefaultCap)), stroke_join_(static_cast(DlStrokeJoin::kDefaultJoin)), is_anti_alias_(false), - is_dither_(false), is_invert_colors_(false), color_(color), stroke_width_(kDefaultWidth), @@ -24,7 +23,6 @@ bool DlPaint::operator==(DlPaint const& other) const { stroke_cap_ == other.stroke_cap_ && // stroke_join_ == other.stroke_join_ && // is_anti_alias_ == other.is_anti_alias_ && // - is_dither_ == other.is_dither_ && // is_invert_colors_ == other.is_invert_colors_ && // color_ == other.color_ && // stroke_width_ == other.stroke_width_ && // diff --git a/display_list/dl_paint.h b/display_list/dl_paint.h index 44c8370649d0b..d83bb55c37f6c 100644 --- a/display_list/dl_paint.h +++ b/display_list/dl_paint.h @@ -216,7 +216,6 @@ class DlPaint { unsigned stroke_cap_ : kStrokeCapBits; unsigned stroke_join_ : kStrokeJoinBits; unsigned is_anti_alias_ : 1; - unsigned is_dither_ : 1; unsigned is_invert_colors_ : 1; }; }; diff --git a/display_list/skia/dl_sk_conversions.cc b/display_list/skia/dl_sk_conversions.cc index 96c611f14c7fa..5be8fac3828cb 100644 --- a/display_list/skia/dl_sk_conversions.cc +++ b/display_list/skia/dl_sk_conversions.cc @@ -53,8 +53,6 @@ SkPaint ToSk(const DlPaint& paint, bool force_stroke) { // See https://github.com/flutter/flutter/issues/112498. sk_paint.setDither(color_source->isGradient()); sk_paint.setShader(ToSk(color_source)); - } else { - sk_paint.setDither(false); } sk_paint.setMaskFilter(ToSk(paint.getMaskFilterPtr())); diff --git a/display_list/skia/dl_sk_paint_dispatcher.cc b/display_list/skia/dl_sk_paint_dispatcher.cc index 0bd9c09b7641a..edcf39bb5f4af 100644 --- a/display_list/skia/dl_sk_paint_dispatcher.cc +++ b/display_list/skia/dl_sk_paint_dispatcher.cc @@ -70,8 +70,13 @@ void DlSkPaintDispatchHelper::setBlendMode(DlBlendMode mode) { paint_.setBlendMode(ToSk(mode)); } void DlSkPaintDispatchHelper::setColorSource(const DlColorSource* source) { - color_source_gradient_ = source && source->isGradient(); paint_.setShader(ToSk(source)); + + // On the Impeller backend, we only support dithering of *gradients*, and so + // by default, so this will force Skia to match that behavior (i.e. turning + // dithering on for gradient paints, and off for everything else). + auto const is_gradient = source && source->isGradient(); + paint_.setDither(is_gradient); } void DlSkPaintDispatchHelper::setImageFilter(const DlImageFilter* filter) { paint_.setImageFilter(ToSk(filter)); diff --git a/display_list/skia/dl_sk_paint_dispatcher.h b/display_list/skia/dl_sk_paint_dispatcher.h index 7e0d2f06d5b8b..2c50d7668c747 100644 --- a/display_list/skia/dl_sk_paint_dispatcher.h +++ b/display_list/skia/dl_sk_paint_dispatcher.h @@ -37,17 +37,7 @@ class DlSkPaintDispatchHelper : public virtual DlOpReceiver { void setMaskFilter(const DlMaskFilter* filter) override; void setImageFilter(const DlImageFilter* filter) override; - const SkPaint& paint() { - // On the Impeller backend, we will only support dithering of *gradients*, - // and it will be enabled by default (without the option to disable it). - // Until Skia support is completely removed, we only want to dither - // gradients (otherwise it will also apply to, for example, images, which is - // not supported in Impeller). - // - // See https://github.com/flutter/flutter/issues/112498. - paint_.setDither(color_source_gradient_); - return paint_; - } + const SkPaint& paint() { return paint_; } /// Returns the current opacity attribute which is used to reduce /// the alpha of all setColor calls encountered in the streeam @@ -67,7 +57,6 @@ class DlSkPaintDispatchHelper : public virtual DlOpReceiver { private: SkPaint paint_; - bool color_source_gradient_ = false; bool invert_colors_ = false; sk_sp sk_color_filter_; diff --git a/display_list/testing/dl_rendering_unittests.cc b/display_list/testing/dl_rendering_unittests.cc index 9b68ad40fd04e..a2fc86fd50b0a 100644 --- a/display_list/testing/dl_rendering_unittests.cc +++ b/display_list/testing/dl_rendering_unittests.cc @@ -1781,48 +1781,6 @@ class CanvasCompareTester { ctx.paint.setColorSource(dl_gradient); })); } - - if (testP.uses_gradient()) { - // Dithering is only applied to gradients so we reuse the gradient - // created above in these setup methods. Also, thin stroked - // primitives (mainly drawLine and drawPoints) do not show much - // dithering so we use a non-trivial stroke width as well. - RenderEnvironment dither_env = - RenderEnvironment::Make565(env.provider()); - if (!dither_env.valid()) { - // Currently only happens on Metal backend - static OncePerBackendWarning warnings("Skipping Dithering tests"); - warnings.warn(dither_env.backend_name()); - } else { - DlColor dither_bg = DlColor::kBlack(); - SkSetup sk_dither_setup = [=](const SkSetupContext& ctx) { - ctx.paint.setShader(sk_gradient); - ctx.paint.setAlpha(0xf0); - ctx.paint.setStrokeWidth(5.0); - }; - DlSetup dl_dither_setup = [=](const DlSetupContext& ctx) { - ctx.paint.setColorSource(dl_gradient); - ctx.paint.setAlpha(0xf0); - ctx.paint.setStrokeWidth(5.0); - }; - dither_env.init_ref(sk_dither_setup, testP.sk_renderer(), - dl_dither_setup, testP.dl_renderer(), - testP.imp_renderer(), dither_bg); - quickCompareToReference(dither_env, "dither"); - RenderWith(testP, dither_env, tolerance, - CaseParameters( - "Dither == True", - [=](const SkSetupContext& ctx) { - sk_dither_setup(ctx); - ctx.paint.setDither(true); - }, - [=](const DlSetupContext& ctx) { - dl_dither_setup(ctx); - // Dithering is implicitly enabled for gradients. - }) - .with_bg(dither_bg)); - } - } } } From 9dc5f11d56ffd6f3a07403b3fd8978d8d3e36b3e Mon Sep 17 00:00:00 2001 From: Jim Graham Date: Tue, 31 Oct 2023 14:11:55 -0700 Subject: [PATCH 09/11] factor operation type into whether to set dither flag --- display_list/skia/dl_sk_canvas.cc | 7 ++- display_list/skia/dl_sk_conversions.cc | 17 +++++- display_list/skia/dl_sk_conversions.h | 4 +- .../skia/dl_sk_conversions_unittests.cc | 16 +++++- display_list/skia/dl_sk_dispatcher.cc | 5 +- display_list/skia/dl_sk_paint_dispatcher.cc | 15 +++-- display_list/skia/dl_sk_paint_dispatcher.h | 20 ++++++- .../skia/dl_sk_paint_dispatcher_unittests.cc | 55 ++++++++++++++++++- .../testing/dl_rendering_unittests.cc | 1 + 9 files changed, 120 insertions(+), 20 deletions(-) diff --git a/display_list/skia/dl_sk_canvas.cc b/display_list/skia/dl_sk_canvas.cc index 2699a46bd7078..bc40ef13fa3ae 100644 --- a/display_list/skia/dl_sk_canvas.cc +++ b/display_list/skia/dl_sk_canvas.cc @@ -16,9 +16,10 @@ namespace flutter { class SkOptionalPaint { public: + // SkOptionalPaint is only valid for ops that do not use the ColorSource explicit SkOptionalPaint(const DlPaint* dl_paint) { if (dl_paint && !dl_paint->isDefault()) { - sk_paint_ = ToSk(*dl_paint); + sk_paint_ = ToNonShaderSk(*dl_paint); ptr_ = &sk_paint_; } else { ptr_ = nullptr; @@ -193,7 +194,7 @@ void DlSkCanvasAdapter::DrawColor(DlColor color, DlBlendMode mode) { void DlSkCanvasAdapter::DrawLine(const SkPoint& p0, const SkPoint& p1, const DlPaint& paint) { - delegate_->drawLine(p0, p1, ToSk(paint, true)); + delegate_->drawLine(p0, p1, ToStrokedSk(paint)); } void DlSkCanvasAdapter::DrawRect(const SkRect& rect, const DlPaint& paint) { @@ -236,7 +237,7 @@ void DlSkCanvasAdapter::DrawPoints(PointMode mode, uint32_t count, const SkPoint pts[], const DlPaint& paint) { - delegate_->drawPoints(ToSk(mode), count, pts, ToSk(paint, true)); + delegate_->drawPoints(ToSk(mode), count, pts, ToStrokedSk(paint)); } void DlSkCanvasAdapter::DrawVertices(const DlVertices* vertices, diff --git a/display_list/skia/dl_sk_conversions.cc b/display_list/skia/dl_sk_conversions.cc index 5be8fac3828cb..98c81472a5a9d 100644 --- a/display_list/skia/dl_sk_conversions.cc +++ b/display_list/skia/dl_sk_conversions.cc @@ -19,14 +19,13 @@ constexpr float kInvertColorMatrix[20] = { }; // clang-format on -SkPaint ToSk(const DlPaint& paint, bool force_stroke) { +SkPaint ToSk(const DlPaint& paint) { SkPaint sk_paint; sk_paint.setAntiAlias(paint.isAntiAlias()); sk_paint.setColor(ToSk(paint.getColor())); sk_paint.setBlendMode(ToSk(paint.getBlendMode())); - sk_paint.setStyle(force_stroke ? SkPaint::kStroke_Style - : ToSk(paint.getDrawStyle())); + sk_paint.setStyle(ToSk(paint.getDrawStyle())); sk_paint.setStrokeWidth(paint.getStrokeWidth()); sk_paint.setStrokeMiter(paint.getStrokeMiter()); sk_paint.setStrokeCap(ToSk(paint.getStrokeCap())); @@ -61,6 +60,18 @@ SkPaint ToSk(const DlPaint& paint, bool force_stroke) { return sk_paint; } +SkPaint ToStrokedSk(const DlPaint& paint) { + DlPaint stroked_paint = paint; + stroked_paint.setDrawStyle(DlDrawStyle::kStroke); + return ToSk(stroked_paint); +} + +SkPaint ToNonShaderSk(const DlPaint& paint) { + DlPaint non_shader_paint = paint; + non_shader_paint.setColorSource(nullptr); + return ToSk(non_shader_paint); +} + sk_sp ToSk(const DlColorSource* source) { if (!source) { return nullptr; diff --git a/display_list/skia/dl_sk_conversions.h b/display_list/skia/dl_sk_conversions.h index a162f8e65d3a1..6ccfe7e714d5b 100644 --- a/display_list/skia/dl_sk_conversions.h +++ b/display_list/skia/dl_sk_conversions.h @@ -10,7 +10,9 @@ namespace flutter { -SkPaint ToSk(const DlPaint& paint, bool force_stroke = false); +SkPaint ToSk(const DlPaint& paint); +SkPaint ToStrokedSk(const DlPaint& paint); +SkPaint ToNonShaderSk(const DlPaint& paint); inline SkBlendMode ToSk(DlBlendMode mode) { return static_cast(mode); diff --git a/display_list/skia/dl_sk_conversions_unittests.cc b/display_list/skia/dl_sk_conversions_unittests.cc index 7db166067ebd5..f5ea4bce86ff7 100644 --- a/display_list/skia/dl_sk_conversions_unittests.cc +++ b/display_list/skia/dl_sk_conversions_unittests.cc @@ -298,8 +298,20 @@ TEST(DisplayListSkConversions, ToSkDitheringEnabledForGradients) { SkPoint::Make(100, 100), 0, 0, 0, DlTileMode::kClamp)); - SkPaint sk_paint = ToSk(dl_paint); - EXPECT_TRUE(sk_paint.isDither()); + { + SkPaint sk_paint = ToSk(dl_paint); + EXPECT_TRUE(sk_paint.isDither()); + } + + { + SkPaint sk_paint = ToStrokedSk(dl_paint); + EXPECT_TRUE(sk_paint.isDither()); + } + + { + SkPaint sk_paint = ToNonShaderSk(dl_paint); + EXPECT_FALSE(sk_paint.isDither()); + } } } // namespace testing diff --git a/display_list/skia/dl_sk_dispatcher.cc b/display_list/skia/dl_sk_dispatcher.cc index c05d23ec1d376..13449c38ee99d 100644 --- a/display_list/skia/dl_sk_dispatcher.cc +++ b/display_list/skia/dl_sk_dispatcher.cc @@ -17,7 +17,10 @@ const SkPaint* DlSkCanvasDispatcher::safe_paint(bool use_attributes) { if (use_attributes) { // The accumulated SkPaint object will already have incorporated // any attribute overrides. - return &paint(); + // Any rendering operation that uses an optional paint will ignore + // the shader in the paint so we inform that |paint()| method so + // that it can set the dither flag appropriately. + return &paint(false); } else if (has_opacity()) { temp_paint_.setAlphaf(opacity()); return &temp_paint_; diff --git a/display_list/skia/dl_sk_paint_dispatcher.cc b/display_list/skia/dl_sk_paint_dispatcher.cc index edcf39bb5f4af..0e05739daff01 100644 --- a/display_list/skia/dl_sk_paint_dispatcher.cc +++ b/display_list/skia/dl_sk_paint_dispatcher.cc @@ -70,13 +70,16 @@ void DlSkPaintDispatchHelper::setBlendMode(DlBlendMode mode) { paint_.setBlendMode(ToSk(mode)); } void DlSkPaintDispatchHelper::setColorSource(const DlColorSource* source) { + // On the Impeller backend, we only support dithering of *gradients*, and + // so we need to set the dither flag whenever we render a gradient. + // + // In this method we can determine whether or not the source is a gradient, + // but we don't have the other half of the information which is what + // rendering op is being performed. So, we simply record whether the + // source is a gradient here and let the |paint()| method figure out + // the rest (i.e. whether the color source will be used). + color_source_gradient_ = source && source->isGradient(); paint_.setShader(ToSk(source)); - - // On the Impeller backend, we only support dithering of *gradients*, and so - // by default, so this will force Skia to match that behavior (i.e. turning - // dithering on for gradient paints, and off for everything else). - auto const is_gradient = source && source->isGradient(); - paint_.setDither(is_gradient); } void DlSkPaintDispatchHelper::setImageFilter(const DlImageFilter* filter) { paint_.setImageFilter(ToSk(filter)); diff --git a/display_list/skia/dl_sk_paint_dispatcher.h b/display_list/skia/dl_sk_paint_dispatcher.h index 2c50d7668c747..edec66d4eec2f 100644 --- a/display_list/skia/dl_sk_paint_dispatcher.h +++ b/display_list/skia/dl_sk_paint_dispatcher.h @@ -37,8 +37,23 @@ class DlSkPaintDispatchHelper : public virtual DlOpReceiver { void setMaskFilter(const DlMaskFilter* filter) override; void setImageFilter(const DlImageFilter* filter) override; - const SkPaint& paint() { return paint_; } - + const SkPaint& paint(bool uses_shader = true) { + // On the Impeller backend, we will only support dithering of *gradients*, + // and it will be enabled by default (without the option to disable it). + // Until Skia support is completely removed, we only want to respect the + // dither flag for gradients (otherwise it will also apply to, for + // example, image ops and image sources, which are not supported in + // Impeller) and only on those operations that use the color source. + // + // The color_source_gradient_ flag lets us know if the color source is + // a gradient and then the uses_shader flag passed in to this method by + // the rendering op lets us know if the color source is used (and is + // therefore the primary source of colors for the op). + // + // See https://github.com/flutter/flutter/issues/112498. + paint_.setDither(uses_shader && color_source_gradient_); + return paint_; + } /// Returns the current opacity attribute which is used to reduce /// the alpha of all setColor calls encountered in the streeam SkScalar opacity() { return opacity_; } @@ -57,6 +72,7 @@ class DlSkPaintDispatchHelper : public virtual DlOpReceiver { private: SkPaint paint_; + bool color_source_gradient_ = false; bool invert_colors_ = false; sk_sp sk_color_filter_; diff --git a/display_list/skia/dl_sk_paint_dispatcher_unittests.cc b/display_list/skia/dl_sk_paint_dispatcher_unittests.cc index a96dfee4699a3..808204dd33db2 100644 --- a/display_list/skia/dl_sk_paint_dispatcher_unittests.cc +++ b/display_list/skia/dl_sk_paint_dispatcher_unittests.cc @@ -5,6 +5,8 @@ #include "display_list/effects/dl_color_source.h" #include "flutter/display_list/skia/dl_sk_paint_dispatcher.h" +#include "flutter/display_list/skia/dl_sk_dispatcher.h" +#include "flutter/display_list/testing/dl_test_snippets.h" #include "flutter/display_list/utils/dl_receiver_utils.h" #include "gtest/gtest.h" @@ -49,7 +51,8 @@ TEST(DisplayListUtils, SetColorSourceDithersIfGradient) { MockDispatchHelper helper; helper.setColorSource(kTestLinearGradient.get()); - EXPECT_TRUE(helper.paint().isDither()); + EXPECT_TRUE(helper.paint(true).isDither()); + EXPECT_FALSE(helper.paint(false).isDither()); } // https://github.com/flutter/flutter/issues/132860. @@ -58,7 +61,55 @@ TEST(DisplayListUtils, SetColorSourceDoesNotDitherIfNotGradient) { helper.setColorSource(kTestLinearGradient.get()); helper.setColorSource(nullptr); - EXPECT_FALSE(helper.paint().isDither()); + 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); + EXPECT_FALSE(helper.paint(true).isDither()); + EXPECT_FALSE(helper.paint(false).isDither()); +} + +// https://github.com/flutter/flutter/issues/132860. +TEST(DisplayListUtils, SkDispatcherSetColorSourceDithersIfGradient) { + SkCanvas canvas; + DlSkCanvasDispatcher dispatcher(&canvas); + + dispatcher.setColorSource(kTestLinearGradient.get()); + EXPECT_TRUE(dispatcher.paint(true).isDither()); + EXPECT_FALSE(dispatcher.paint(false).isDither()); + EXPECT_FALSE(dispatcher.safe_paint(true)->isDither()); + // Calling safe_paint(false) returns a nullptr +} + +// https://github.com/flutter/flutter/issues/132860. +TEST(DisplayListUtils, SkDispatcherSetColorSourceDoesNotDitherIfNotGradient) { + SkCanvas canvas; + DlSkCanvasDispatcher dispatcher(&canvas); + + dispatcher.setColorSource(kTestLinearGradient.get()); + dispatcher.setColorSource(nullptr); + 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 + + 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); + 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 } } // namespace testing diff --git a/display_list/testing/dl_rendering_unittests.cc b/display_list/testing/dl_rendering_unittests.cc index a2fc86fd50b0a..04caadfdb4a58 100644 --- a/display_list/testing/dl_rendering_unittests.cc +++ b/display_list/testing/dl_rendering_unittests.cc @@ -1776,6 +1776,7 @@ class CanvasCompareTester { "LinearGradient GYB", [=](const SkSetupContext& ctx) { ctx.paint.setShader(sk_gradient); + ctx.paint.setDither(testP.uses_gradient()); }, [=](const DlSetupContext& ctx) { ctx.paint.setColorSource(dl_gradient); From 0907ecd86e63d8f750429506aefb417230271867 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Tue, 31 Oct 2023 14:37:20 -0700 Subject: [PATCH 10/11] Tweak but still failing. --- display_list/skia/dl_sk_conversions.cc | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/display_list/skia/dl_sk_conversions.cc b/display_list/skia/dl_sk_conversions.cc index 98c81472a5a9d..ca30d2ad6f91e 100644 --- a/display_list/skia/dl_sk_conversions.cc +++ b/display_list/skia/dl_sk_conversions.cc @@ -43,13 +43,7 @@ SkPaint ToSk(const DlPaint& paint) { auto color_source = paint.getColorSourcePtr(); if (color_source) { - // On the Impeller backend, we will only support dithering of *gradients*, - // and it will be enabled by default (without the option to disable it). - // Until Skia support is completely removed, we only want to respect the - // dither flag for gradients (otherwise it will also apply to, for example, - // images, which is not supported in Impeller). - // - // See https://github.com/flutter/flutter/issues/112498. + // Unconditionally set dither to true for gradient shaders. sk_paint.setDither(color_source->isGradient()); sk_paint.setShader(ToSk(color_source)); } From 17031b9c70e631ca99bfe4feb0c3d71a4c10d711 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Tue, 31 Oct 2023 14:38:02 -0700 Subject: [PATCH 11/11] Tweak comment. --- display_list/skia/dl_sk_paint_dispatcher.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/display_list/skia/dl_sk_paint_dispatcher.h b/display_list/skia/dl_sk_paint_dispatcher.h index edec66d4eec2f..a027e7ee4777b 100644 --- a/display_list/skia/dl_sk_paint_dispatcher.h +++ b/display_list/skia/dl_sk_paint_dispatcher.h @@ -42,7 +42,7 @@ class DlSkPaintDispatchHelper : public virtual DlOpReceiver { // and it will be enabled by default (without the option to disable it). // Until Skia support is completely removed, we only want to respect the // dither flag for gradients (otherwise it will also apply to, for - // example, image ops and image sources, which are not supported in + // example, image ops and image sources, which are not dithered in // Impeller) and only on those operations that use the color source. // // The color_source_gradient_ flag lets us know if the color source is