From e14b3c091947d9a69a842909747f48277ae52c28 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Tue, 15 Aug 2023 18:07:39 -0700 Subject: [PATCH] [Skia] Only respect ui.Paint.dither when the colorSource is a gradient (#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](https://github.com/flutter/engine/pull/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. --- display_list/effects/dl_color_source.h | 12 ++++ display_list/skia/dl_sk_canvas.cc | 41 ------------- display_list/skia/dl_sk_conversions.cc | 55 ++++++++++++++++++ display_list/skia/dl_sk_conversions.h | 2 + .../skia/dl_sk_conversions_unittests.cc | 42 +++++++++++++- .../testing/dl_rendering_unittests.cc | 57 ------------------- 6 files changed, 109 insertions(+), 100 deletions(-) diff --git a/display_list/effects/dl_color_source.h b/display_list/effects/dl_color_source.h index 394fe092af0fe..11a141da9c905 100644 --- a/display_list/effects/dl_color_source.h +++ b/display_list/effects/dl_color_source.h @@ -119,6 +119,16 @@ class DlColorSource : public DlAttribute { /// 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 index 842f4593035ba..9299c9a6ff9d7 100644 --- 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 index 4a4264269fd1f..16f5fc710a343 100644 --- 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 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 2291b4afc4ddf..a1a9f6ce838b8 100644 --- 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(mode); } diff --git a/display_list/skia/dl_sk_conversions_unittests.cc b/display_list/skia/dl_sk_conversions_unittests.cc index dae99690c91d5..1bae18a3ca1dd 100644 --- 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 index 491c82a0f9ffb..23a80c2f13d92 100644 --- 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 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(