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) #54648

Merged
auto-submit merged 5 commits into from
Aug 20, 2024
+446 −147
Merged

Changes DlColor to support wide gamut colors (#54473) #54648

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
107a9a2
Changes DlColor to support wide gamut colors (#54473)
gaaclarke Aug 19, 2024
055303c
Made sure dlcolor is opaque black by default
gaaclarke Aug 20, 2024
65d4573
fixed typo in test and added assert to avoid it
gaaclarke Aug 20, 2024
bbe7dd8
asserted colorspace
gaaclarke Aug 20, 2024
db4f334
removed asserts
gaaclarke Aug 20, 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
5 changes: 4 additions & 1 deletion display_list/dl_color.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,10 @@ struct DlColor {
red_(red),
green_(green),
blue_(blue),
color_space_(colorspace) {}
color_space_(colorspace) {
FML_DCHECK(alpha >= 0.0);
FML_DCHECK(alpha <= 1.0);

@flar flar Aug 20, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we dcheck or clamp? This is currently, in this PR, helping to catch a problem in a test conversion that we crossed paths with, but that bug-in-a-test would have normally been caught by the test creator. If we dcheck then we might annoy someone who calculated the alpha and ended up with a barely-out-of-range value. They could just have easily have provided 255 for one of the color values and we'd take it as a potential non-normalized Color Space float...?

@jonahwilliams jonahwilliams Aug 20, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Rather than dcheck I think we should clamp right? Certain blend modes (plus) can lead to alpha greater than one without a clamp

@gaaclarke gaaclarke Aug 20, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Rather than dcheck I think we should clamp right? Certain blend modes (plus) can lead to alpha greater than one without a clamp

My concern with that is that it will mask problems like we had where someone is sending in 255 to create a color. The assert forces people creating colors to have a clamp if they are doing arithmetic that will get it out of bounds. I didn't have to change any code as a result of all our tests, so that's a good sign. I think this leaves us in a better position considering we control all the locations where DlColors are created. A public API would be a different story.

@jonahwilliams jonahwilliams Aug 20, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You should clamp.

@gaaclarke gaaclarke Aug 20, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I removed the asserts to duplicate the behavior of impeller::Color. There may be transitory colors that are not valid, clamping or the asserts could mess with that. I don't think we blend with DlColor today, it's more of a transmission format, but I think we plan to use it more broadly in the future.

@jonahwilliams jonahwilliams Aug 20, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We will be CPU blending once we merge DlColor and impeller::Color. This will happen sooner than you'd think

}

/// @brief Construct a 32 bit color from floating point R, G, B, and A color
/// channels.
Expand Down
2 changes: 1 addition & 1 deletion impeller/display_list/aiks_dl_blend_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,7 +392,7 @@ static sk_sp<DisplayList> BlendModeTest(Vector2 content_scale,
// Draw grid behind the images.
{
DlPaint paint;
paint.setColor(DlColor::RGBA(41 / 255.0, 41 / 255.0, 41 / 255.0, 255));
paint.setColor(DlColor::RGBA(41 / 255.0, 41 / 255.0, 41 / 255.0, 1));
builder.DrawRect(SkRect::MakeLTRB(0, 0, 800, 400), paint);
}

Expand Down