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
auto-submit merged 12 commits into from
Oct 31, 2023
Merged

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

Show file tree
Hide file tree
Changes from 9 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

@flar flar Oct 30, 2023

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

@flar flar Oct 30, 2023

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
6 changes: 1 addition & 5 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 @@ -51,11 +51,7 @@ SkPaint ToSk(const DlPaint& paint, bool force_stroke) {
// 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.setDither(color_source->isGradient());
sk_paint.setShader(ToSk(color_source));
}

Expand Down
22 changes: 0 additions & 22 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,41 +286,19 @@ 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());
}

Expand Down
10 changes: 6 additions & 4 deletions display_list/skia/dl_sk_paint_dispatcher.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,6 @@ void DlSkPaintDispatchHelper::restore_opacity() {
void DlSkPaintDispatchHelper::setAntiAlias(bool aa) {
paint_.setAntiAlias(aa);
}
void DlSkPaintDispatchHelper::setDither(bool dither) {
dither_ = dither;
}
void DlSkPaintDispatchHelper::setInvertColors(bool invert) {
invert_colors_ = invert;
paint_.setColorFilter(makeColorFilter());
Expand Down Expand Up @@ -73,8 +70,13 @@ void DlSkPaintDispatchHelper::setBlendMode(DlBlendMode mode) {
paint_.setBlendMode(ToSk(mode));
}
void DlSkPaintDispatchHelper::setColorSource(const DlColorSource* source) {
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));
Expand Down
15 changes: 1 addition & 14 deletions display_list/skia/dl_sk_paint_dispatcher.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ class DlSkPaintDispatchHelper : public virtual DlOpReceiver {
}

void setAntiAlias(bool aa) override;
void setDither(bool dither) override;
void setDrawStyle(DlDrawStyle style) override;
void setColor(DlColor color) override;
void setStrokeWidth(SkScalar width) override;
Expand All @@ -38,17 +37,7 @@ class DlSkPaintDispatchHelper : public virtual DlOpReceiver {
void setMaskFilter(const DlMaskFilter* filter) override;
void setImageFilter(const DlImageFilter* filter) override;

const SkPaint& paint() {
// 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.
paint_.setDither(color_source_gradient_ && dither_);
return paint_;
}
const SkPaint& paint() { return paint_; }

/// Returns the current opacity attribute which is used to reduce
/// the alpha of all setColor calls encountered in the streeam
Expand All @@ -68,8 +57,6 @@ class DlSkPaintDispatchHelper : public virtual DlOpReceiver {

private:
SkPaint paint_;
bool color_source_gradient_ = false;
bool dither_ = false;
bool invert_colors_ = false;
sk_sp<SkColorFilter> sk_color_filter_;

Expand Down
Loading