Surgically remove .*dither.* from the Engine

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,

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<SkShader> ToSk(const DlColorSource* source) {
   if (!source) {
     return nullptr;

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<SkBlendMode>(mode);

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

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_;

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));

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<SkColorFilter> sk_color_filter_;
 

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

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);