Further filter/clear <SkPaint>.setDither(true), this time in `DlSkP…

…aintDispatchHelper` (#44912) Closes flutter/flutter#132860. - If `setDither(true)` is called, and an existing `setColorSource` is a gradient, it is ignored. - If `setColorSource(...)` is called, and it is a gradient, and dithering was previously set, it is cleared. I'm not sure this is fool proof.
flutter · Aug 22, 2023 · 0a3cb0c · 0a3cb0c
1 parent 97f1ed1
commit 0a3cb0c
Show file tree

Hide file tree

Showing 3 changed files with 84 additions and 2 deletions.
diff --git a/display_list/skia/dl_sk_paint_dispatcher.cc b/display_list/skia/dl_sk_paint_dispatcher.cc
@@ -41,7 +41,7 @@ void DlSkPaintDispatchHelper::setAntiAlias(bool aa) {
   paint_.setAntiAlias(aa);
 }
 void DlSkPaintDispatchHelper::setDither(bool dither) {
-  paint_.setDither(dither);
+  dither_ = dither;
 }
 void DlSkPaintDispatchHelper::setInvertColors(bool invert) {
   invert_colors_ = invert;
@@ -73,6 +73,7 @@ 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));
 }
 void DlSkPaintDispatchHelper::setImageFilter(const DlImageFilter* filter) {

diff --git a/display_list/skia/dl_sk_paint_dispatcher.h b/display_list/skia/dl_sk_paint_dispatcher.h
@@ -37,7 +37,17 @@ 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() {
+    // 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.
+    paint_.setDither(color_source_gradient_ && dither_);
+    return paint_;
+  }
 
   /// Returns the current opacity attribute which is used to reduce
   /// the alpha of all setColor calls encountered in the streeam
@@ -57,6 +67,8 @@ class DlSkPaintDispatchHelper : public virtual DlOpReceiver {
 
  private:
   SkPaint paint_;
+  bool color_source_gradient_ = false;
+  bool dither_ = false;
   bool invert_colors_ = false;
   sk_sp<SkColorFilter> 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
@@ -2,6 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "display_list/effects/dl_color_source.h"
 #include "flutter/display_list/skia/dl_sk_paint_dispatcher.h"
 
 #include "flutter/display_list/utils/dl_receiver_utils.h"
@@ -21,6 +22,17 @@ class MockDispatchHelper final : public virtual DlOpReceiver,
   void restore() override { DlSkPaintDispatchHelper::restore_opacity(); }
 };
 
+static const DlColor kTestColors[2] = {0xFF000000, 0xFFFFFFFF};
+static const float kTestStops[2] = {0.0f, 1.0f};
+static const auto kTestLinearGradient =
+    DlColorSource::MakeLinear(SkPoint::Make(0.0f, 0.0f),
+                              SkPoint::Make(100.0f, 100.0f),
+                              2,
+                              kTestColors,
+                              kTestStops,
+                              DlTileMode::kClamp,
+                              nullptr);
+
 // Regression test for https://github.com/flutter/flutter/issues/100176.
 TEST(DisplayListUtils, OverRestore) {
   MockDispatchHelper helper;
@@ -31,5 +43,62 @@ TEST(DisplayListUtils, OverRestore) {
   helper.restore();
 }
 
+// https://github.com/flutter/flutter/issues/132860.
+TEST(DisplayListUtils, SetDitherIgnoredIfColorSourceNotGradient) {
+  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) {
+  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
 }  // namespace flutter