[Skia] Only respect ui.Paint.dither when the colorSource is a gradient (

flutter#44730) In the Impeller backend, we **only** support dithering of _gradients_. In addition, it will be the default (and only option). In the [process of enabling dithering by default](flutter#44705), i.e. ```diff class Paint { - static bool enableDithering = false; + static bool enableDithering = true; } ``` ... we realized with internal Google testing this will now apply dithering on more than just gradients, i.e. images in the Skia backend. Since we won't support dithering of images in the Impeller backend, this PR gives a "hint" on whether the `colorSource` (if one is set) can be dithered by the contrived rules we've created.
gaaclarke · Aug 30, 2023 · e14b3c0 · e14b3c0
1 parent cf8eaa6
commit e14b3c0
Show file tree

Hide file tree

Showing 6 changed files with 109 additions and 100 deletions.
diff --git a/display_list/effects/dl_color_source.h b/display_list/effects/dl_color_source.h
@@ -119,6 +119,16 @@ class DlColorSource : public DlAttribute<DlColorSource, DlColorSourceType> {
   ///
   virtual bool isUIThreadSafe() const = 0;
 
+  //----------------------------------------------------------------------------
+  /// @brief      If the underlying platform data represents a gradient.
+  ///
+  ///             TODO(matanl): Remove this flag when the Skia backend is
+  ///             removed, https://github.com/flutter/flutter/issues/112498.
+  ///
+  /// @return     True if the class represents the output of a gradient.
+  ///
+  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; }
@@ -287,6 +297,8 @@ class DlGradientColorSourceBase : public DlMatrixColorSourceBase {
     return true;
   }
 
+  bool isGradient() const override { return true; }
+
   DlTileMode tile_mode() const { return mode_; }
   int stop_count() const { return stop_count_; }
   const DlColor* colors() const {

diff --git a/display_list/skia/dl_sk_canvas.cc b/display_list/skia/dl_sk_canvas.cc
@@ -14,47 +14,6 @@
 
 namespace flutter {
 
-// clang-format off
-constexpr float kInvertColorMatrix[20] = {
-  -1.0,    0,    0, 1.0, 0,
-     0, -1.0,    0, 1.0, 0,
-     0,    0, -1.0, 1.0, 0,
-   1.0,  1.0,  1.0, 1.0, 0
-};
-// clang-format on
-
-static SkPaint ToSk(const DlPaint& paint, bool force_stroke = false) {
-  SkPaint sk_paint;
-
-  sk_paint.setAntiAlias(paint.isAntiAlias());
-  sk_paint.setDither(paint.isDither());
-
-  sk_paint.setColor(paint.getColor());
-  sk_paint.setBlendMode(ToSk(paint.getBlendMode()));
-  sk_paint.setStyle(force_stroke ? SkPaint::kStroke_Style
-                                 : ToSk(paint.getDrawStyle()));
-  sk_paint.setStrokeWidth(paint.getStrokeWidth());
-  sk_paint.setStrokeMiter(paint.getStrokeMiter());
-  sk_paint.setStrokeCap(ToSk(paint.getStrokeCap()));
-  sk_paint.setStrokeJoin(ToSk(paint.getStrokeJoin()));
-
-  sk_paint.setShader(ToSk(paint.getColorSourcePtr()));
-  sk_paint.setImageFilter(ToSk(paint.getImageFilterPtr()));
-  auto color_filter = ToSk(paint.getColorFilterPtr());
-  if (paint.isInvertColors()) {
-    auto invert_filter = SkColorFilters::Matrix(kInvertColorMatrix);
-    if (color_filter) {
-      invert_filter = invert_filter->makeComposed(color_filter);
-    }
-    color_filter = invert_filter;
-  }
-  sk_paint.setColorFilter(color_filter);
-  sk_paint.setMaskFilter(ToSk(paint.getMaskFilterPtr()));
-  sk_paint.setPathEffect(ToSk(paint.getPathEffectPtr()));
-
-  return sk_paint;
-}
-
 class SkOptionalPaint {
  public:
   explicit SkOptionalPaint(const DlPaint* dl_paint) {

diff --git a/display_list/skia/dl_sk_conversions.cc b/display_list/skia/dl_sk_conversions.cc
@@ -10,6 +10,61 @@
 
 namespace flutter {
 
+// clang-format off
+constexpr float kInvertColorMatrix[20] = {
+  -1.0,    0,    0, 1.0, 0,
+     0, -1.0,    0, 1.0, 0,
+     0,    0, -1.0, 1.0, 0,
+   1.0,  1.0,  1.0, 1.0, 0
+};
+// clang-format on
+
+SkPaint ToSk(const DlPaint& paint, bool force_stroke) {
+  SkPaint sk_paint;
+
+  sk_paint.setAntiAlias(paint.isAntiAlias());
+  sk_paint.setColor(paint.getColor());
+  sk_paint.setBlendMode(ToSk(paint.getBlendMode()));
+  sk_paint.setStyle(force_stroke ? SkPaint::kStroke_Style
+                                 : ToSk(paint.getDrawStyle()));
+  sk_paint.setStrokeWidth(paint.getStrokeWidth());
+  sk_paint.setStrokeMiter(paint.getStrokeMiter());
+  sk_paint.setStrokeCap(ToSk(paint.getStrokeCap()));
+  sk_paint.setStrokeJoin(ToSk(paint.getStrokeJoin()));
+  sk_paint.setImageFilter(ToSk(paint.getImageFilterPtr()));
+  auto color_filter = ToSk(paint.getColorFilterPtr());
+  if (paint.isInvertColors()) {
+    auto invert_filter = SkColorFilters::Matrix(kInvertColorMatrix);
+    if (color_filter) {
+      invert_filter = invert_filter->makeComposed(color_filter);
+    }
+    color_filter = invert_filter;
+  }
+  sk_paint.setColorFilter(color_filter);
+
+  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.
+    if (color_source->isGradient()) {
+      // Originates from `dart:ui#Paint.enableDithering`.
+      auto user_specified_dither = paint.isDither();
+      sk_paint.setDither(user_specified_dither);
+    }
+    sk_paint.setShader(ToSk(color_source));
+  }
+
+  sk_paint.setMaskFilter(ToSk(paint.getMaskFilterPtr()));
+  sk_paint.setPathEffect(ToSk(paint.getPathEffectPtr()));
+
+  return sk_paint;
+}
+
 sk_sp<SkShader> 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
@@ -10,6 +10,8 @@
 
 namespace flutter {
 
+SkPaint ToSk(const DlPaint& paint, bool force_stroke = false);
+
 inline SkBlendMode ToSk(DlBlendMode mode) {
   return static_cast<SkBlendMode>(mode);
 }

diff --git a/display_list/skia/dl_sk_conversions_unittests.cc b/display_list/skia/dl_sk_conversions_unittests.cc
@@ -2,13 +2,13 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "flutter/display_list/skia/dl_sk_conversions.h"
-
 #include "flutter/display_list/dl_blend_mode.h"
 #include "flutter/display_list/dl_paint.h"
 #include "flutter/display_list/dl_sampling_options.h"
 #include "flutter/display_list/dl_tile_mode.h"
 #include "flutter/display_list/dl_vertices.h"
+#include "flutter/display_list/effects/dl_color_source.h"
+#include "flutter/display_list/skia/dl_sk_conversions.h"
 #include "gtest/gtest.h"
 #include "third_party/skia/include/core/SkSamplingOptions.h"
 #include "third_party/skia/include/core/SkTileMode.h"
@@ -270,5 +270,43 @@ 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),
+                                                    SkPoint::Make(100, 100), 0,
+                                                    0, 0, DlTileMode::kClamp));
+
+  SkPaint sk_paint = ToSk(dl_paint);
+
+  EXPECT_TRUE(sk_paint.isDither());
+}
+
 }  // namespace testing
 }  // namespace flutter
diff --git a/display_list/testing/dl_rendering_unittests.cc b/display_list/testing/dl_rendering_unittests.cc
@@ -1247,63 +1247,6 @@ class CanvasCompareTester {
               [=](DlCanvas* cv, DlPaint& p) { dl_aa_setup(cv, p, false); }));
     }
 
-    {
-      // The CPU renderer does not always dither for solid colors and we
-      // need to use a non-default color (default is black) on an opaque
-      // surface, so we use a shader instead of a color. 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 std::set<std::string> warnings_sent;
-        std::string name = dither_env.backend_name();
-        if (warnings_sent.find(name) == warnings_sent.end()) {
-          warnings_sent.insert(name);
-          FML_LOG(INFO) << "Skipping Dithering tests on " << name;
-        }
-      } else {
-        DlColor dither_bg = DlColor::kBlack();
-        SkSetup sk_dither_setup = [=](SkCanvas*, SkPaint& p) {
-          p.setShader(kTestSkImageColorSource);
-          p.setAlpha(0xf0);
-          p.setStrokeWidth(5.0);
-        };
-        DlSetup dl_dither_setup = [=](DlCanvas*, DlPaint& p) {
-          p.setColorSource(&kTestDlImageColorSource);
-          p.setAlpha(0xf0);
-          p.setStrokeWidth(5.0);
-        };
-        dither_env.init_ref(sk_dither_setup, testP.sk_renderer(),
-                            dl_dither_setup, testP.dl_renderer(), dither_bg);
-        quickCompareToReference(dither_env, "dither");
-        RenderWith(testP, dither_env, tolerance,
-                   CaseParameters(
-                       "Dither == True",
-                       [=](SkCanvas* cv, SkPaint& p) {
-                         sk_dither_setup(cv, p);
-                         p.setDither(true);
-                       },
-                       [=](DlCanvas* cv, DlPaint& p) {
-                         dl_dither_setup(cv, p);
-                         p.setDither(true);
-                       })
-                       .with_bg(dither_bg));
-        RenderWith(testP, dither_env, tolerance,
-                   CaseParameters(
-                       "Dither = False",
-                       [=](SkCanvas* cv, SkPaint& p) {
-                         sk_dither_setup(cv, p);
-                         p.setDither(false);
-                       },
-                       [=](DlCanvas* cv, DlPaint& p) {
-                         dl_dither_setup(cv, p);
-                         p.setDither(false);
-                       })
-                       .with_bg(dither_bg));
-      }
-    }
-
     RenderWith(
         testP, env, tolerance,
         CaseParameters(