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

Changes DlColor to support wide gamut colors #54473

Merged
merged 35 commits into from
Aug 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
1071a2b
Adds DlColor wide gamut colors support.
gaaclarke Aug 9, 2024
624e4c6
fixed translation issue
gaaclarke Aug 9, 2024
190b22a
fixed memory crashes in the dl tests
gaaclarke Aug 9, 2024
bb6f2cc
fixed some test failures
gaaclarke Aug 9, 2024
6dfc01a
updated more tests
gaaclarke Aug 9, 2024
3802bf3
updated more literals
gaaclarke Aug 9, 2024
07d3a6a
++
gaaclarke Aug 10, 2024
468bc63
more tests
gaaclarke Aug 10, 2024
fe103df
tests
gaaclarke Aug 10, 2024
0c52081
++
gaaclarke Aug 12, 2024
fc37d07
++
gaaclarke Aug 12, 2024
3ce239c
++
gaaclarke Aug 12, 2024
ac388e5
++
gaaclarke Aug 12, 2024
edd3dcd
++
gaaclarke Aug 12, 2024
cbf1e99
updated equality
gaaclarke Aug 12, 2024
cf35445
added p3->srgb_xr transform
gaaclarke Aug 12, 2024
6ecf978
license update
gaaclarke Aug 15, 2024
961577c
fixed unsafe skia conversions
gaaclarke Aug 15, 2024
5cfa175
fixed vertices without color
gaaclarke Aug 15, 2024
9866b13
removed more reinterpret casts
gaaclarke Aug 16, 2024
4b276e9
fixed conversion loops
gaaclarke Aug 16, 2024
8753d85
jim feedback1
gaaclarke Aug 16, 2024
aa49207
jim feedback2
gaaclarke Aug 16, 2024
31623df
jim feedback3
gaaclarke Aug 16, 2024
93f5bd0
jim feedback4
gaaclarke Aug 16, 2024
1c50f9d
removed premultipliedargb()
gaaclarke Aug 19, 2024
7eca131
added class docstring
gaaclarke Aug 19, 2024
cf64760
added withColorSpace docstring
gaaclarke Aug 19, 2024
eeed2ed
added isclose docstring
gaaclarke Aug 19, 2024
35cbd31
removed comment
gaaclarke Aug 19, 2024
902e510
made store_colors faster
gaaclarke Aug 19, 2024
d46e140
added todo
gaaclarke Aug 19, 2024
10b1d6d
pulled out rvalue
gaaclarke Aug 19, 2024
b839e18
fixed typo
gaaclarke Aug 19, 2024
2be2649
added colorspace comment
gaaclarke Aug 19, 2024
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
2 changes: 2 additions & 0 deletions ci/licenses_golden/licenses_flutter
Original file line number Diff line number Diff line change
Expand Up @@ -41609,6 +41609,7 @@ ORIGIN: ../../../flutter/display_list/dl_builder.cc + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/display_list/dl_builder.h + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/display_list/dl_canvas.cc + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/display_list/dl_canvas.h + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/display_list/dl_color.cc + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/display_list/dl_color.h + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/display_list/dl_op_flags.cc + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/display_list/dl_op_flags.h + ../../../flutter/LICENSE
Expand Down Expand Up @@ -44491,6 +44492,7 @@ FILE: ../../../flutter/display_list/dl_builder.cc
FILE: ../../../flutter/display_list/dl_builder.h
FILE: ../../../flutter/display_list/dl_canvas.cc
FILE: ../../../flutter/display_list/dl_canvas.h
FILE: ../../../flutter/display_list/dl_color.cc
FILE: ../../../flutter/display_list/dl_color.h
FILE: ../../../flutter/display_list/dl_op_flags.cc
FILE: ../../../flutter/display_list/dl_op_flags.h
Expand Down
1 change: 1 addition & 0 deletions display_list/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ source_set("display_list") {
"dl_builder.h",
"dl_canvas.cc",
"dl_canvas.h",
"dl_color.cc",
"dl_color.h",
"dl_op_flags.cc",
"dl_op_flags.h",
Expand Down
2 changes: 1 addition & 1 deletion display_list/display_list_unittests.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1751,7 +1751,7 @@ TEST_F(DisplayListTest, FlutterSvgIssue661BoundsWereEmpty) {
// This is the more practical result. The bounds are "almost" 0,0,100x100
EXPECT_EQ(display_list->bounds().roundOut(), SkIRect::MakeWH(100, 100));
EXPECT_EQ(display_list->op_count(), 19u);
EXPECT_EQ(display_list->bytes(), sizeof(DisplayList) + 408u);
EXPECT_EQ(display_list->bytes(), sizeof(DisplayList) + 424u);
EXPECT_EQ(display_list->total_depth(), 3u);
}

Expand Down
2 changes: 1 addition & 1 deletion display_list/dl_builder.h
Original file line number Diff line number Diff line change
Expand Up @@ -805,7 +805,7 @@ class DisplayListBuilder final : public virtual DlCanvas,

// kAnyColor is a non-opaque and non-transparent color that will not
// trigger any short-circuit tests about the results of a blend.
static constexpr DlColor kAnyColor = DlColor::kMidGrey().withAlpha(0x80);
static constexpr DlColor kAnyColor = DlColor::kMidGrey().withAlphaF(0.5f);
static_assert(!kAnyColor.isOpaque());
static_assert(!kAnyColor.isTransparent());
static DlColor GetEffectiveColor(const DlPaint& paint,
Expand Down
75 changes: 75 additions & 0 deletions display_list/dl_color.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
// Copyright 2013 The Flutter Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#include "flutter/display_list/dl_color.h"

namespace flutter {

namespace {
const std::array<DlScalar, 12> kP3ToSrgb = {
1.306671048092539, -0.298061942172353,
0.213228303487995, -0.213580156254466, //
-0.117390025596251, 1.127722006101976,
0.109727644608938, -0.109450321455370, //
0.214813187718391, 0.054268702864647,
1.406898424029350, -0.364892765879631};

DlColor transform(const DlColor& color,
const std::array<DlScalar, 12>& matrix,
DlColorSpace color_space) {
return DlColor(color.getAlphaF(),
matrix[0] * color.getRedF() + //
matrix[1] * color.getGreenF() + //
matrix[2] * color.getBlueF() + //
matrix[3], //
matrix[4] * color.getRedF() + //
matrix[5] * color.getGreenF() + //
matrix[6] * color.getBlueF() + //
matrix[7], //
matrix[8] * color.getRedF() + //
matrix[9] * color.getGreenF() + //
matrix[10] * color.getBlueF() + //
matrix[11], //
color_space);
}
} // namespace

DlColor DlColor::withColorSpace(DlColorSpace color_space) const {
switch (color_space_) {
case DlColorSpace::kSRGB:
switch (color_space) {
case DlColorSpace::kSRGB:
return *this;
case DlColorSpace::kExtendedSRGB:
return DlColor(alpha_, red_, green_, blue_,
DlColorSpace::kExtendedSRGB);
case DlColorSpace::kDisplayP3:
FML_CHECK(false) << "not implemented";
Copy link
Contributor

Choose a reason for hiding this comment

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

How much do all the unimplemented checks in here hurt us? I guess we don't have actual use of the other color spaces yet?

Copy link
Member Author

Choose a reason for hiding this comment

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

I only implemented what was necessary in order for the integration test to pass. We can fill in other things as they come up. Since it isn't a publicly supported API we have a good idea of how this is used. In Dart I had to give it everything.

return *this;
}
case DlColorSpace::kExtendedSRGB:
switch (color_space) {
case DlColorSpace::kSRGB:
FML_CHECK(false) << "not implemented";
return *this;
case DlColorSpace::kExtendedSRGB:
return *this;
case DlColorSpace::kDisplayP3:
FML_CHECK(false) << "not implemented";
return *this;
}
case DlColorSpace::kDisplayP3:
switch (color_space) {
case DlColorSpace::kSRGB:
FML_CHECK(false) << "not implemented";
return *this;
case DlColorSpace::kExtendedSRGB:
return transform(*this, kP3ToSrgb, DlColorSpace::kExtendedSRGB);
case DlColorSpace::kDisplayP3:
return *this;
}
}
}

} // namespace flutter
152 changes: 113 additions & 39 deletions display_list/dl_color.h
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,39 @@

namespace flutter {

// These should match the enumerations defined in //lib/ui/painting.dart.
enum class DlColorSpace { kSRGB = 0, kExtendedSRGB = 1, kDisplayP3 = 2 };
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you have the = 0/1/2? And why a single-line definition? It might be nice to add a reference to the description of the CS as a doc comment on each one.

Copy link
Member Author

Choose a reason for hiding this comment

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

They have to match the one defined in painting.dart, I added a comment. The format is what clang format does.


/// A representation of a color.
///
/// The color belongs to a DlColorSpace. Using deprecated integer data accessors
/// on colors not in the kSRGB colorspace can lead to data loss. Using the
/// floating point accessors and constructors that were added for wide-gamut
/// support are preferred.
struct DlColor {
public:
constexpr DlColor() : argb_(0xFF000000) {}
constexpr explicit DlColor(uint32_t argb) : argb_(argb) {}
constexpr DlColor()
: alpha_(0.f),
red_(0.f),
green_(0.f),
blue_(0.f),
color_space_(DlColorSpace::kSRGB) {}
constexpr explicit DlColor(uint32_t argb)
: alpha_(toF((argb >> 24) & 0xff)),
red_(toF((argb >> 16) & 0xff)),
green_(toF((argb >> 8) & 0xff)),
blue_(toF((argb >> 0) & 0xff)),
color_space_(DlColorSpace::kSRGB) {}
constexpr DlColor(DlScalar alpha,
Copy link
Contributor

Choose a reason for hiding this comment

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

Now I remember. I didn't want to have a constructor that had an implicit ordering of the components so I created the ARGB and RGBA factories. If I'd my druthers I would also question the existence of the uint32_t constructor with its implicit byte order. How often was this constructor used and could we make it private and only used from the factories and the relatively small number of existing legacy constructors?

Copy link
Contributor

Choose a reason for hiding this comment

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

the uint32_t constructor has been around since the class was created.

The RGBA and ARGB constructors were only created last month for this reason.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably also make an ARGB(uint32_t) single argument factory and start replacing the raw constructor everywhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removing that constructor sounds good to me. I think that would be better suited for a different PR though since it wasn't introduced here.

DlScalar red,
DlScalar green,
DlScalar blue,
DlColorSpace colorspace)
: alpha_(alpha),
red_(red),
green_(green),
blue_(blue),
color_space_(colorspace) {}

/// @brief Construct a 32 bit color from floating point R, G, B, and A color
/// channels.
Expand All @@ -29,10 +58,7 @@ struct DlColor {
DlScalar r,
DlScalar g,
DlScalar b) {
return DlColor(toC(a) << 24 | //
toC(r) << 16 | //
toC(g) << 8 | //
toC(b));
return DlColor(a, r, g, b, DlColorSpace::kSRGB);
}

static constexpr uint8_t toAlpha(DlScalar opacity) { return toC(opacity); }
Expand All @@ -58,58 +84,106 @@ struct DlColor {
static constexpr DlColor kCornflowerBlue() {return DlColor(0xFF6495ED);};
// clang-format on

constexpr bool isOpaque() const { return getAlpha() == 0xFF; }
constexpr bool isTransparent() const { return getAlpha() == 0; }

constexpr int getAlpha() const { return argb_ >> 24; }
constexpr int getRed() const { return (argb_ >> 16) & 0xFF; }
constexpr int getGreen() const { return (argb_ >> 8) & 0xFF; }
constexpr int getBlue() const { return argb_ & 0xFF; }

constexpr DlScalar getAlphaF() const { return toF(getAlpha()); }
constexpr DlScalar getRedF() const { return toF(getRed()); }
constexpr DlScalar getGreenF() const { return toF(getGreen()); }
constexpr DlScalar getBlueF() const { return toF(getBlue()); }

constexpr uint32_t premultipliedArgb() const {
if (isOpaque()) {
return argb_;
}
DlScalar f = getAlphaF();
return (argb_ & 0xFF000000) | //
toC(getRedF() * f) << 16 | //
toC(getGreenF() * f) << 8 | //
toC(getBlueF() * f);
}
constexpr bool isOpaque() const { return alpha_ >= 1.f; }
constexpr bool isTransparent() const { return alpha_ <= 0.f; }

///\deprecated Use floating point accessors to avoid data loss when using wide
/// gamut colors.
constexpr int getAlpha() const { return toC(alpha_); }
///\deprecated Use floating point accessors to avoid data loss when using wide
/// gamut colors.
constexpr int getRed() const { return toC(red_); }
///\deprecated Use floating point accessors to avoid data loss when using wide
/// gamut colors.
constexpr int getGreen() const { return toC(green_); }
///\deprecated Use floating point accessors to avoid data loss when using wide
/// gamut colors.
constexpr int getBlue() const { return toC(blue_); }

constexpr DlScalar getAlphaF() const { return alpha_; }
constexpr DlScalar getRedF() const { return red_; }
constexpr DlScalar getGreenF() const { return green_; }
constexpr DlScalar getBlueF() const { return blue_; }

constexpr DlColorSpace getColorSpace() const { return color_space_; }

constexpr DlColor withAlpha(uint8_t alpha) const { //
return DlColor((argb_ & 0x00FFFFFF) | (alpha << 24));
return DlColor((argb() & 0x00FFFFFF) | (alpha << 24));
Copy link
Contributor

Choose a reason for hiding this comment

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

I would convert the alpha to float and then use the constructor that takes the raw r_, g_, b_ values (and colorspace) rather than convert to uint32, apply the alpha, and convert back?

Same comment for withR/G/B methods.

Copy link
Contributor

Choose a reason for hiding this comment

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

Basically return withFoo(ToF(arg))?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, modulateOpacity() could be more direct in this way. It's basically withAlphaF before withAlphaF was created in this patch.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think if people are using these old with* setters they want to have the old behavior, which is clamping the numbers to 255. modulateOpacity we should consider updating. I would want to do that in a follow up PR though to avoid anyone that is relying on this behavior.

}
constexpr DlColor withRed(uint8_t red) const { //
return DlColor((argb_ & 0xFF00FFFF) | (red << 16));
return DlColor((argb() & 0xFF00FFFF) | (red << 16));
}
constexpr DlColor withGreen(uint8_t green) const { //
return DlColor((argb_ & 0xFFFF00FF) | (green << 8));
return DlColor((argb() & 0xFFFF00FF) | (green << 8));
}
constexpr DlColor withBlue(uint8_t blue) const { //
return DlColor((argb_ & 0xFFFFFF00) | (blue << 0));
return DlColor((argb() & 0xFFFFFF00) | (blue << 0));
}
constexpr DlColor withAlphaF(float alpha) const { //
return DlColor(alpha, red_, green_, blue_, color_space_);
}
constexpr DlColor withRedF(float red) const { //
return DlColor(alpha_, red, green_, blue_, color_space_);
}
constexpr DlColor withGreenF(float green) const { //
return DlColor(alpha_, red_, green, blue_, color_space_);
}
constexpr DlColor withBlueF(float blue) const { //
return DlColor(alpha_, red_, green_, blue, color_space_);
}
/// Performs a colorspace transformation.
///
/// This isn't just a replacement of the color space field, the new color
/// components are calculated.
DlColor withColorSpace(DlColorSpace color_space) const;

constexpr DlColor modulateOpacity(DlScalar opacity) const {
return opacity <= 0 ? withAlpha(0)
: opacity >= 1 ? *this
: withAlpha(round(getAlpha() * opacity));
}

constexpr uint32_t argb() const { return argb_; }
///\deprecated Use floating point accessors to avoid data loss when using wide
/// gamut colors.
constexpr uint32_t argb() const {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this adjust to sRGB?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a fast path if it is already sRGB?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it still calculates sensible output. Wether it is what we want is asserted at the integration test level.

return toC(alpha_) << 24 | //
toC(red_) << 16 | //
toC(green_) << 8 | //
toC(blue_) << 0;
}

bool operator==(DlColor const& other) const { return argb_ == other.argb_; }
bool operator!=(DlColor const& other) const { return argb_ != other.argb_; }
bool operator==(uint32_t const& other) const { return argb_ == other; }
bool operator!=(uint32_t const& other) const { return argb_ != other; }
/// Checks that no difference in color components exceeds the delta.
///
/// This doesn't check against the actual distance between the colors in some
/// space.
bool isClose(DlColor const& other, DlScalar delta = 1.0f / 256.0f) {
return color_space_ == other.color_space_ &&
std::abs(alpha_ - other.alpha_) < delta &&
std::abs(red_ - other.red_) < delta &&
std::abs(green_ - other.green_) < delta &&
std::abs(blue_ - other.blue_) < delta;
}
bool operator==(DlColor const& other) const {
return alpha_ == other.alpha_ && red_ == other.red_ &&
green_ == other.green_ && blue_ == other.blue_ &&
color_space_ == other.color_space_;
}
bool operator!=(DlColor const& other) const {
return !this->operator==(other);
}
bool operator==(uint32_t const& other) const {
return argb() == other && color_space_ == DlColorSpace::kSRGB;
}
bool operator!=(uint32_t const& other) const {
return !this->operator==(other);
}

private:
uint32_t argb_;
DlScalar alpha_;
DlScalar red_;
DlScalar green_;
DlScalar blue_;
DlColorSpace color_space_;

static constexpr DlScalar toF(uint8_t comp) { return comp * (1.0f / 255); }
static constexpr uint8_t toC(DlScalar fComp) { return round(fComp * 255); }
Expand Down
Loading