Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Surgically remove .*dither.* from the Engine #46750

Merged
merged 12 commits into from
Oct 31, 2023
Merged

Surgically remove .*dither.* from the Engine #46750

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
3a5678b
WIP.
matanlurey Oct 11, 2023
b27c9df
More WIP.
matanlurey Oct 11, 2023
543b2ae
Finish removing.
matanlurey Oct 11, 2023
879c6a1
Merge branch 'main' into engine-removeSetDither
matanlurey Oct 30, 2023
8f0a989
Remove removed isDither call for << operator.
matanlurey Oct 30, 2023
e17ddfe
Remove unused field.
matanlurey Oct 30, 2023
9a40ed4
Finish cleanup.
matanlurey Oct 30, 2023
8fc1180
Format.
matanlurey Oct 30, 2023
7a2bdf8
Address comments.
matanlurey Oct 31, 2023
9dc5f11
factor operation type into whether to set dither flag
flar Oct 31, 2023
0907ecd
Tweak but still failing.
matanlurey Oct 31, 2023
17031b9
Tweak comment.
matanlurey Oct 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion display_list/benchmarking/dl_complexity_helper.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,6 @@ class ComplexityCalculatorHelper

virtual ~ComplexityCalculatorHelper() = default;

void setDither(bool dither) override {}
void setInvertColors(bool invert) override {}
void setStrokeCap(DlStrokeCap cap) override {}
void setStrokeJoin(DlStrokeJoin join) override {}
Expand Down
1 change: 0 additions & 1 deletion display_list/display_list.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,6 @@ namespace flutter {

#define FOR_EACH_DISPLAY_LIST_OP(V) \
V(SetAntiAlias) \
V(SetDither) \
V(SetInvertColors) \
\
V(SetStrokeCap) \
Expand Down
11 changes: 2 additions & 9 deletions display_list/display_list_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,6 @@ class DisplayListTestBase : public BaseT {
DlPaint defaults;

EXPECT_EQ(builder_paint.isAntiAlias(), defaults.isAntiAlias());
EXPECT_EQ(builder_paint.isDither(), defaults.isDither());
EXPECT_EQ(builder_paint.isInvertColors(), defaults.isInvertColors());
EXPECT_EQ(builder_paint.getColor(), defaults.getColor());
EXPECT_EQ(builder_paint.getBlendMode(), defaults.getBlendMode());
Expand Down Expand Up @@ -387,10 +386,6 @@ TEST_F(DisplayListTest, BuildRestoresAttributes) {
builder.Build();
check_defaults(builder, cull_rect);

receiver.setDither(true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The two lines following that should also be removed. They have nothing to check if the receiver wasn't modified.

builder.Build();
check_defaults(builder, cull_rect);

receiver.setInvertColors(true);
builder.Build();
check_defaults(builder, cull_rect);
Expand Down Expand Up @@ -1060,11 +1055,9 @@ TEST_F(DisplayListTest, SingleOpsMightSupportGroupOpacityBlendMode) {
};

#define RUN_TESTS(body) \
run_tests( \
#body, [](DlOpReceiver& receiver) { body }, true, false)
run_tests(#body, [](DlOpReceiver& receiver) { body }, true, false)
#define RUN_TESTS2(body, expect) \
run_tests( \
#body, [](DlOpReceiver& receiver) { body }, expect, expect)
run_tests(#body, [](DlOpReceiver& receiver) { body }, expect, expect)

RUN_TESTS(receiver.drawPaint(););
RUN_TESTS2(receiver.drawColor(DlColor(SK_ColorRED), DlBlendMode::kSrcOver);
Expand Down
7 changes: 0 additions & 7 deletions display_list/dl_builder.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,10 +123,6 @@ void DisplayListBuilder::onSetAntiAlias(bool aa) {
current_.setAntiAlias(aa);
Push<SetAntiAliasOp>(0, 0, aa);
}
void DisplayListBuilder::onSetDither(bool dither) {
current_.setDither(dither);
Push<SetDitherOp>(0, 0, dither);
}
void DisplayListBuilder::onSetInvertColors(bool invert) {
current_.setInvertColors(invert);
Push<SetInvertColorsOp>(0, 0, invert);
Expand Down Expand Up @@ -347,9 +343,6 @@ void DisplayListBuilder::SetAttributesFromPaint(
if (flags.applies_anti_alias()) {
setAntiAlias(paint.isAntiAlias());
}
if (flags.applies_dither()) {
setDither(paint.isDither());
}
if (flags.applies_alpha_or_color()) {
setColor(paint.getColor());
}
Expand Down
7 changes: 0 additions & 7 deletions display_list/dl_builder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,12 +270,6 @@ class DisplayListBuilder final : public virtual DlCanvas,
}
}
// |DlOpReceiver|
void setDither(bool dither) override {
if (current_.isDither() != dither) {
onSetDither(dither);
}
}
// |DlOpReceiver|
void setInvertColors(bool invert) override {
if (current_.isInvertColors() != invert) {
onSetInvertColors(invert);
Expand Down Expand Up @@ -673,7 +667,6 @@ class DisplayListBuilder final : public virtual DlCanvas,
}

void onSetAntiAlias(bool aa);
void onSetDither(bool dither);
void onSetInvertColors(bool invert);
void onSetStrokeCap(DlStrokeCap cap);
void onSetStrokeJoin(DlStrokeJoin join);
Expand Down
28 changes: 11 additions & 17 deletions display_list/dl_op_flags.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,15 +81,14 @@ class DisplayListFlags {

// clang-format off
static constexpr int kUsesAntiAlias = 1 << 10;
static constexpr int kUsesDither = 1 << 11;
static constexpr int kUsesAlpha = 1 << 12;
static constexpr int kUsesColor = 1 << 13;
static constexpr int kUsesBlend = 1 << 14;
static constexpr int kUsesShader = 1 << 15;
static constexpr int kUsesColorFilter = 1 << 16;
static constexpr int kUsesPathEffect = 1 << 17;
static constexpr int kUsesMaskFilter = 1 << 18;
static constexpr int kUsesImageFilter = 1 << 19;
static constexpr int kUsesAlpha = 1 << 11;
static constexpr int kUsesColor = 1 << 12;
static constexpr int kUsesBlend = 1 << 13;
static constexpr int kUsesShader = 1 << 14;
static constexpr int kUsesColorFilter = 1 << 15;
static constexpr int kUsesPathEffect = 1 << 16;
static constexpr int kUsesMaskFilter = 1 << 17;
static constexpr int kUsesImageFilter = 1 << 18;

// Some ops have an optional paint argument. If the version
// stored in the DisplayList ignores the paint, but there
Expand All @@ -102,9 +101,8 @@ class DisplayListFlags {
// clang-format on

static constexpr int kAnyAttributeMask = //
kUsesAntiAlias | kUsesDither | kUsesAlpha | kUsesColor | kUsesBlend |
kUsesShader | kUsesColorFilter | kUsesPathEffect | kUsesMaskFilter |
kUsesImageFilter;
kUsesAntiAlias | kUsesAlpha | kUsesColor | kUsesBlend | kUsesShader |
kUsesColorFilter | kUsesPathEffect | kUsesMaskFilter | kUsesImageFilter;
};

class DisplayListFlagsBase : protected DisplayListFlags {
Expand Down Expand Up @@ -173,7 +171,6 @@ class DisplayListAttributeFlags : DisplayListFlagsBase {
constexpr bool ignores_paint() const { return has_any(kIgnoresPaint); }

constexpr bool applies_anti_alias() const { return has_any(kUsesAntiAlias); }
constexpr bool applies_dither() const { return has_any(kUsesDither); }
constexpr bool applies_color() const { return has_any(kUsesColor); }
constexpr bool applies_alpha() const { return has_any(kUsesAlpha); }
constexpr bool applies_alpha_or_color() const {
Expand Down Expand Up @@ -257,8 +254,7 @@ class DisplayListAttributeFlags : DisplayListFlagsBase {
class DisplayListOpFlags : DisplayListFlags {
private:
// Flags common to all primitives that apply colors
static constexpr int kBasePaintFlags = (kUsesDither | //
kUsesColor | //
static constexpr int kBasePaintFlags = (kUsesColor | //
kUsesAlpha | //
kUsesBlend | //
kUsesShader | //
Expand All @@ -280,7 +276,6 @@ class DisplayListOpFlags : DisplayListFlags {
// Flags common to primitives that render an image with paint attributes
static constexpr int kBaseImageFlags = (kIsNonGeometric | //
kUsesAlpha | //
kUsesDither | //
kUsesBlend | //
kUsesColorFilter | //
kUsesImageFilter);
Expand Down Expand Up @@ -376,7 +371,6 @@ class DisplayListOpFlags : DisplayListFlags {
};
static constexpr DisplayListAttributeFlags kDrawVerticesFlags{
kIsNonGeometric | //
kUsesDither | //
kUsesAlpha | //
kUsesShader | //
kUsesBlend | //
Expand Down
1 change: 0 additions & 1 deletion display_list/dl_op_receiver.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,6 @@ class DlOpReceiver {
// another method that changes the same attribute. The current set of
// attributes is not affected by |save| and |restore|.
virtual void setAntiAlias(bool aa) = 0;
virtual void setDither(bool dither) = 0;
virtual void setDrawStyle(DlDrawStyle style) = 0;
virtual void setColor(DlColor color) = 0;
virtual void setStrokeWidth(float width) = 0;
Expand Down
1 change: 0 additions & 1 deletion display_list/dl_op_records.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,6 @@ struct DLOp {
} \
};
DEFINE_SET_BOOL_OP(AntiAlias)
DEFINE_SET_BOOL_OP(Dither)
DEFINE_SET_BOOL_OP(InvertColors)
#undef DEFINE_SET_BOOL_OP

Expand Down
2 changes: 0 additions & 2 deletions display_list/dl_paint.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ DlPaint::DlPaint(DlColor color)
stroke_cap_(static_cast<unsigned>(DlStrokeCap::kDefaultCap)),
stroke_join_(static_cast<unsigned>(DlStrokeJoin::kDefaultJoin)),
is_anti_alias_(false),
is_dither_(false),
is_invert_colors_(false),
color_(color),
stroke_width_(kDefaultWidth),
Expand All @@ -24,7 +23,6 @@ bool DlPaint::operator==(DlPaint const& other) const {
stroke_cap_ == other.stroke_cap_ && //
stroke_join_ == other.stroke_join_ && //
is_anti_alias_ == other.is_anti_alias_ && //
is_dither_ == other.is_dither_ && //
is_invert_colors_ == other.is_invert_colors_ && //
color_ == other.color_ && //
stroke_width_ == other.stroke_width_ && //
Expand Down
7 changes: 0 additions & 7 deletions display_list/dl_paint.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,12 +61,6 @@ class DlPaint {
return *this;
}

bool isDither() const { return is_dither_; }
DlPaint& setDither(bool isDither) {
is_dither_ = isDither;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The field can be deleted as well.

return *this;
}

bool isInvertColors() const { return is_invert_colors_; }
DlPaint& setInvertColors(bool isInvertColors) {
is_invert_colors_ = isInvertColors;
Expand Down Expand Up @@ -222,7 +216,6 @@ class DlPaint {
unsigned stroke_cap_ : kStrokeCapBits;
unsigned stroke_join_ : kStrokeJoinBits;
unsigned is_anti_alias_ : 1;
unsigned is_dither_ : 1;
unsigned is_invert_colors_ : 1;
};
};
Expand Down
4 changes: 0 additions & 4 deletions display_list/dl_paint_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ namespace testing {
TEST(DisplayListPaint, ConstructorDefaults) {
DlPaint paint;
EXPECT_FALSE(paint.isAntiAlias());
EXPECT_FALSE(paint.isDither());
EXPECT_FALSE(paint.isInvertColors());
EXPECT_EQ(paint.getColor(), DlPaint::kDefaultColor);
EXPECT_EQ(paint.getAlpha(), 0xFF);
Expand Down Expand Up @@ -45,7 +44,6 @@ TEST(DisplayListPaint, ConstructorDefaults) {
EXPECT_EQ(paint, DlPaint(DlColor(0xFF000000)));

EXPECT_NE(paint, DlPaint().setAntiAlias(true));
EXPECT_NE(paint, DlPaint().setDither(true));
EXPECT_NE(paint, DlPaint().setInvertColors(true));
EXPECT_NE(paint, DlPaint().setColor(DlColor::kGreen()));
EXPECT_NE(paint, DlPaint(DlColor::kGreen()));
Expand Down Expand Up @@ -110,7 +108,6 @@ TEST(DisplayListPaint, ChainingConstructor) {
DlPaint paint =
DlPaint() //
.setAntiAlias(true) //
.setDither(true) //
.setInvertColors(true) //
.setColor(DlColor::kGreen()) //
.setAlpha(0x7F) //
Expand All @@ -129,7 +126,6 @@ TEST(DisplayListPaint, ChainingConstructor) {
.setMaskFilter(DlBlurMaskFilter(DlBlurStyle::kInner, 3.14).shared())
.setPathEffect(path_effect);
EXPECT_TRUE(paint.isAntiAlias());
EXPECT_TRUE(paint.isDither());
EXPECT_TRUE(paint.isInvertColors());
EXPECT_EQ(paint.getColor(), DlColor::kGreen().withAlpha(0x7F));
EXPECT_EQ(paint.getAlpha(), 0x7F);
Expand Down
7 changes: 4 additions & 3 deletions display_list/skia/dl_sk_canvas.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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,
Expand Down
31 changes: 16 additions & 15 deletions display_list/skia/dl_sk_conversions.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()));
Expand All @@ -44,18 +43,8 @@ SkPaint ToSk(const DlPaint& paint, bool force_stroke) {

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);
}
// Unconditionally set dither to true for gradient shaders.
sk_paint.setDither(color_source->isGradient());
sk_paint.setShader(ToSk(color_source));
}

Expand All @@ -65,6 +54,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;
Expand Down
4 changes: 3 additions & 1 deletion display_list/skia/dl_sk_conversions.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
36 changes: 13 additions & 23 deletions display_list/skia/dl_sk_conversions_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,42 +286,32 @@ 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);
{
SkPaint sk_paint = ToSk(dl_paint);
EXPECT_TRUE(sk_paint.isDither());
}

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
Expand Down
Loading