Skip to content

Commit

Permalink
[Skia] Only respect ui.Paint.dither when the colorSource is a gradient (
Browse files Browse the repository at this point in the history 
#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](#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.
  • Loading branch information
@matanlurey
matanlurey authored Aug 16, 2023
1 parent 659cdfc commit 0922302
Show file tree
Hide file tree
Showing 6 changed files with 109 additions and 100 deletions.
12 changes: 12 additions & 0 deletions display_list/effects/dl_color_source.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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; }
Expand Down Expand Up @@ -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 {
Expand Down
41 changes: 0 additions & 41 deletions display_list/skia/dl_sk_canvas.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
55 changes: 55 additions & 0 deletions display_list/skia/dl_sk_conversions.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 2 additions & 0 deletions display_list/skia/dl_sk_conversions.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
42 changes: 40 additions & 2 deletions display_list/skia/dl_sk_conversions_unittests.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
57 changes: 0 additions & 57 deletions display_list/testing/dl_rendering_unittests.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down

0 comments on commit 0922302

Please sign in to comment.