Skip to content

Support wide gamut colors when applying a DlColor to an SkPaint #170613

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
auto-submit merged 2 commits into from
Jun 24, 2025
Merged

Support wide gamut colors when applying a DlColor to an SkPaint #170613

auto-submit merged 2 commits into from
Jun 24, 2025

Conversation

@jason-simmons
Copy link
Member

@jason-simmons jason-simmons commented Jun 13, 2025

Fixes #170203

@jason-simmons jason-simmons requested a review from flar June 13, 2025 20:52
@github-actions github-actions bot added the engine flutter/engine related. See also e: labels. label Jun 13, 2025
@ksokolovskyi ksokolovskyi mentioned this pull request Jun 13, 2025
Closed
@flutter-dashboard
Copy link

flutter-dashboard bot commented Jun 13, 2025

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #170613 at sha b5babef

@flutter-dashboard flutter-dashboard bot added the will affect goldens Changes to golden files label Jun 13, 2025
@flar
Copy link
Contributor

flar commented Jun 13, 2025

Skia supports color spaces, we should communicate the intended color space to them rather than just switching to Color4f and hoping that the range issues won't affect them.

@jason-simmons
Copy link
Member Author

jason-simmons commented Jun 13, 2025

SkPaint::setColor interprets a default/null color space as sRGB (see https://skia.googlesource.com/skia/+/refs/heads/main/include/core/SkPaint.h#243).

The documentation mentions support for extended range (e.g. https://skia.googlesource.com/skia/+/refs/heads/main/include/core/SkPaint.h#227).

Does that cover this use case? Or does this change need to do something more specific?

@flar
Copy link
Contributor

flar commented Jun 13, 2025

The version of setColor that takes a Color4f also takes an optional ColorSpace. I don't know if that reference to extended SRGB covers this without manually specifying a CS. The floats can express extended range, but how is that interpreted with no CS?

@flar
Copy link
Contributor

flar commented Jun 14, 2025

After reviewing, their docs seems to imply that they will just clamp. I asked the team for clarification, but I'll proceed with the review as if that is the case.

flar
flar requested changes Jun 14, 2025
View reviewed changes
Copy link
Contributor

@flar flar left a comment

Choose a reason for hiding this comment

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

LGTM with suggestions, but we should fix the ToSkColor conversion.

engine/src/flutter/display_list/skia/dl_sk_conversions.h Outdated

inline SkColor ToSk(DlColor color) {
// Returns an SkColor4f representing this color in the sRGB color space.
inline SkColor4f ToSk(DlColor color) {
Copy link
Contributor

@flar flar Jun 14, 2025

Choose a reason for hiding this comment

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

How much is that used. Rather than reuse the name for a new type of conversion, it might be better to have both type-named as ToSkColor4f and ToSkColor.

Copy link
Member Author

@jason-simmons jason-simmons Jun 16, 2025

Choose a reason for hiding this comment

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

done

engine/src/flutter/display_list/skia/dl_sk_conversions_unittests.cc Outdated
TEST(DisplayListSkConversions, ToSkColor) {
// Red
ASSERT_EQ(ToSk(DlColor::kRed()), SK_ColorRED);
ASSERT_EQ(ToSk(DlColor::kRed()), SkColors::kRed);
Copy link
Contributor

@flar flar Jun 14, 2025

Choose a reason for hiding this comment

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

Add tests so that we test both 4f and regular.

Copy link
Member Author

@jason-simmons jason-simmons Jun 16, 2025

Choose a reason for hiding this comment

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

done

engine/src/flutter/display_list/skia/dl_sk_conversions.h
}
}

inline SkColor ToSkColor(DlColor color) {
Copy link
Contributor

@flar flar Jun 14, 2025

Choose a reason for hiding this comment

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

This will fail in the cases that use it with wide gamut colors because out of range floats corrupt the value as they are not clamped in the argb() conversion. Try rendering with a shadow and a wide color and you should see the old broken behavior as that code still uses the SkColor conversion.

Copy link
Contributor

@flar flar Jun 14, 2025

Choose a reason for hiding this comment

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

Also drawAtlas has an array of colors that doesn't use these macros but just calls argb() directly which produces bad data...

Copy link
Contributor

@flar flar Jun 14, 2025

Choose a reason for hiding this comment

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

Suggestion:

DlColor::argb() {
  if (!cs == sRGB) {
    return withCS(sRGB).argb();
  }
  // mul, shift
}

Then have withCS() do:

case ExtendedSRGB:
  case sRGB:
    return DlColor(alpha, clamp, clamp, clamp, sRGB);
...
case P3:
  case sRGB:
    return transform(extended)->withCS(sRGB);

Copy link
Member Author

@jason-simmons jason-simmons Jun 16, 2025

Choose a reason for hiding this comment

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

done

flar
flar approved these changes Jun 17, 2025
View reviewed changes
Copy link
Contributor

@flar flar left a comment

Choose a reason for hiding this comment

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

LGTM

@flutter-dashboard
Copy link

flutter-dashboard bot commented Jun 24, 2025

Golden file changes are available for triage from new commit, Click here to view.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #170613 at sha c875d92

@jason-simmons jason-simmons added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 24, 2025
@auto-submit auto-submit bot added this pull request to the merge queue Jun 24, 2025
Merged via the queue into flutter:master with commit 12e4a89 Jun 24, 2025
174 of 175 checks passed
@flutter-dashboard flutter-dashboard bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jun 24, 2025
@gaaclarke gaaclarke mentioned this pull request Jun 24, 2025
Closed
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jun 24, 2025
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 24, 2025
@engine-flutter-autoroll
12e4a898d Support wide gamut colors when applying a DlColor to an SkP…
531f0f7 
…aint (flutter/flutter#170613)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jun 24, 2025
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 24, 2025
@engine-flutter-autoroll
12e4a898d Support wide gamut colors when applying a DlColor to an SkP…
aed1f4e 
…aint (flutter/flutter#170613)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jun 25, 2025
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 25, 2025
@engine-flutter-autoroll
12e4a898d Support wide gamut colors when applying a DlColor to an SkP…
edfe747 
…aint (flutter/flutter#170613)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jun 25, 2025
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 25, 2025
@engine-flutter-autoroll
12e4a898d Support wide gamut colors when applying a DlColor to an SkP…
521afec 
…aint (flutter/flutter#170613)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jun 25, 2025
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 25, 2025
@engine-flutter-autoroll
12e4a898d Support wide gamut colors when applying a DlColor to an SkP…
874c242 
…aint (flutter/flutter#170613)
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Jun 26, 2025
@engine-flutter-autoroll
Roll Flutter from d733bea58c1a to 2773c0c8e15e (42 revisions) (#9495)
0f80260 
Roll Flutter from d733bea58c1a to 2773c0c8e15e (42 revisions)

flutter/flutter@d733bea...2773c0c

2025-06-25 [email protected] Log stack traces from exceptions thrown by devicelab test tasks (flutter/flutter#171165)
2025-06-25 [email protected] Revert "Move `web_long_running_tests_{1,5}_5` to `bringup`." (flutter/flutter#171100)
2025-06-25 [email protected] Add missing M3 tests for InputDecoration.isDense (flutter/flutter#171058)
2025-06-25 [email protected] Add Android specific sub-step to validate the Android sdk path has no spaces (flutter/flutter#170829)
2025-06-24 [email protected] Update foundation library to export internal (flutter/flutter#170563)
2025-06-24 [email protected] Remove stale references to `Release-process.md` and `conductor` (flutter/flutter#171046)
2025-06-24 [email protected] License cpp jun23 (flutter/flutter#171047)
2025-06-24 [email protected] Add android-reviewers to CODEOWNERS (flutter/flutter#170157)
2025-06-24 [email protected] Update tool/README.md regarding locally-built engine (flutter/flutter#171102)
2025-06-24 [email protected] [web] Align the PR triage process with the ecosystem's triage flow (flutter/flutter#171086)
2025-06-24 [email protected] [flutter_tool] Migrate DAP off `ProcessUtils.writelnToStdinUnsafe` (flutter/flutter#171081)
2025-06-24 [email protected] [web] More granular configuration of the test environment (flutter/flutter#168767)
2025-06-24 [email protected] Clean up Devfs_Web into separate files (flutter/flutter#170769)
2025-06-24 [email protected] Add RawMenuAnchor animation callbacks (flutter/flutter#167806)
2025-06-24 [email protected] Support wide gamut colors when applying a DlColor to an SkPaint (flutter/flutter#170613)
2025-06-24 [email protected] Remove temporary workaround for web testing (flutter/flutter#170949)
2025-06-24 [email protected] Roll Packages from 02770da to d9d3191 (6 revisions) (flutter/flutter#171075)
2025-06-24 [email protected] Add LLDB warnings (flutter/flutter#170827)
2025-06-24 [email protected] Update FormField.initialValue documentation (flutter/flutter#171061)
2025-06-24 [email protected] Roll Skia from 132cb2052565 to a462e701b493 (2 revisions) (flutter/flutter#171063)
2025-06-24 [email protected] Roll Skia from f88706e3a863 to 132cb2052565 (4 revisions) (flutter/flutter#171057)
2025-06-24 [email protected] When maintainHintSize is false, hint is centered and aligned, it is different from the original one (flutter/flutter#168654)
2025-06-24 [email protected] Deprecate DropdownButtonFormField "value" parameter in favor of "initialValue" (flutter/flutter#170805)
2025-06-24 [email protected] Roll Skia from af6feb799ea6 to f88706e3a863 (2 revisions) (flutter/flutter#171056)
2025-06-24 [email protected] Roll Dart SDK from aebd78999b1a to d9edd9e7a634 (1 revision) (flutter/flutter#171053)
2025-06-24 [email protected] Roll Skia from ae517eba0170 to af6feb799ea6 (1 revision) (flutter/flutter#171052)
2025-06-24 [email protected] Roll Skia from a7735d517e6a to ae517eba0170 (9 revisions) (flutter/flutter#171049)
2025-06-24 [email protected] Enable interpretation fallback when unable to JIT on iOS. (flutter/flutter#170835)
2025-06-24 [email protected] Flutter test cleanup (flutter/flutter#170891)
2025-06-24 [email protected] Move `packages_autoroller` out of the carcass of `conductor`, delete `conductor` (flutter/flutter#171029)
2025-06-23 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Don't strip symbols from `libapp.so` on android by default (#162464)" (flutter/flutter#171044)
2025-06-23 [email protected] Roll Dart SDK from a09de0d3556c to aebd78999b1a (2 revisions) (flutter/flutter#171039)
2025-06-23 [email protected] Don't strip symbols from `libapp.so` on android by default (flutter/flutter#162464)
2025-06-23 [email protected] Roll Skia from 0311837abe86 to a7735d517e6a (12 revisions) (flutter/flutter#171037)
2025-06-23 [email protected] Pass font scanner to font mgr that need it (flutter/flutter#170701)
2025-06-23 [email protected] Make service worker tests more lenient. (flutter/flutter#170939)
2025-06-23 [email protected] Remove update CHANGELOG step from stable cherry pick process (flutter/flutter#171017)
2025-06-23 [email protected] Include dev_dependencies in all builds for iOS and macOS (flutter/flutter#171015)
2025-06-23 [email protected] Move `web_long_running_tests_{1,5}_5` to `bringup`. (flutter/flutter#171026)
2025-06-23 [email protected] rename from announce to supportsAnnounce on engine (flutter/flutter#170618)
2025-06-23 [email protected] Roll pub packages (flutter/flutter#171016)
2025-06-23 [email protected] Enhance Text Contrast for WCAG AAA Compliance (flutter/flutter#170758)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
...
mboetger pushed a commit to mboetger/flutter that referenced this pull request Jul 21, 2025
@jason-simmons @mboetger
Support wide gamut colors when applying a DlColor to an SkPaint (flut…
956dcd5 
…ter#170613)

Fixes flutter#170203
vashworth pushed a commit to vashworth/packages that referenced this pull request Jul 30, 2025
@engine-flutter-autoroll @vashworth
Roll Flutter from d733bea58c1a to 2773c0c8e15e (42 revisions) (flutte…
ae14bed 
…r#9495)

Roll Flutter from d733bea58c1a to 2773c0c8e15e (42 revisions)

flutter/flutter@d733bea...2773c0c

2025-06-25 [email protected] Log stack traces from exceptions thrown by devicelab test tasks (flutter/flutter#171165)
2025-06-25 [email protected] Revert "Move `web_long_running_tests_{1,5}_5` to `bringup`." (flutter/flutter#171100)
2025-06-25 [email protected] Add missing M3 tests for InputDecoration.isDense (flutter/flutter#171058)
2025-06-25 [email protected] Add Android specific sub-step to validate the Android sdk path has no spaces (flutter/flutter#170829)
2025-06-24 [email protected] Update foundation library to export internal (flutter/flutter#170563)
2025-06-24 [email protected] Remove stale references to `Release-process.md` and `conductor` (flutter/flutter#171046)
2025-06-24 [email protected] License cpp jun23 (flutter/flutter#171047)
2025-06-24 [email protected] Add android-reviewers to CODEOWNERS (flutter/flutter#170157)
2025-06-24 [email protected] Update tool/README.md regarding locally-built engine (flutter/flutter#171102)
2025-06-24 [email protected] [web] Align the PR triage process with the ecosystem's triage flow (flutter/flutter#171086)
2025-06-24 [email protected] [flutter_tool] Migrate DAP off `ProcessUtils.writelnToStdinUnsafe` (flutter/flutter#171081)
2025-06-24 [email protected] [web] More granular configuration of the test environment (flutter/flutter#168767)
2025-06-24 [email protected] Clean up Devfs_Web into separate files (flutter/flutter#170769)
2025-06-24 [email protected] Add RawMenuAnchor animation callbacks (flutter/flutter#167806)
2025-06-24 [email protected] Support wide gamut colors when applying a DlColor to an SkPaint (flutter/flutter#170613)
2025-06-24 [email protected] Remove temporary workaround for web testing (flutter/flutter#170949)
2025-06-24 [email protected] Roll Packages from 02770da to d9d3191 (6 revisions) (flutter/flutter#171075)
2025-06-24 [email protected] Add LLDB warnings (flutter/flutter#170827)
2025-06-24 [email protected] Update FormField.initialValue documentation (flutter/flutter#171061)
2025-06-24 [email protected] Roll Skia from 132cb2052565 to a462e701b493 (2 revisions) (flutter/flutter#171063)
2025-06-24 [email protected] Roll Skia from f88706e3a863 to 132cb2052565 (4 revisions) (flutter/flutter#171057)
2025-06-24 [email protected] When maintainHintSize is false, hint is centered and aligned, it is different from the original one (flutter/flutter#168654)
2025-06-24 [email protected] Deprecate DropdownButtonFormField "value" parameter in favor of "initialValue" (flutter/flutter#170805)
2025-06-24 [email protected] Roll Skia from af6feb799ea6 to f88706e3a863 (2 revisions) (flutter/flutter#171056)
2025-06-24 [email protected] Roll Dart SDK from aebd78999b1a to d9edd9e7a634 (1 revision) (flutter/flutter#171053)
2025-06-24 [email protected] Roll Skia from ae517eba0170 to af6feb799ea6 (1 revision) (flutter/flutter#171052)
2025-06-24 [email protected] Roll Skia from a7735d517e6a to ae517eba0170 (9 revisions) (flutter/flutter#171049)
2025-06-24 [email protected] Enable interpretation fallback when unable to JIT on iOS. (flutter/flutter#170835)
2025-06-24 [email protected] Flutter test cleanup (flutter/flutter#170891)
2025-06-24 [email protected] Move `packages_autoroller` out of the carcass of `conductor`, delete `conductor` (flutter/flutter#171029)
2025-06-23 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Don't strip symbols from `libapp.so` on android by default (#162464)" (flutter/flutter#171044)
2025-06-23 [email protected] Roll Dart SDK from a09de0d3556c to aebd78999b1a (2 revisions) (flutter/flutter#171039)
2025-06-23 [email protected] Don't strip symbols from `libapp.so` on android by default (flutter/flutter#162464)
2025-06-23 [email protected] Roll Skia from 0311837abe86 to a7735d517e6a (12 revisions) (flutter/flutter#171037)
2025-06-23 [email protected] Pass font scanner to font mgr that need it (flutter/flutter#170701)
2025-06-23 [email protected] Make service worker tests more lenient. (flutter/flutter#170939)
2025-06-23 [email protected] Remove update CHANGELOG step from stable cherry pick process (flutter/flutter#171017)
2025-06-23 [email protected] Include dev_dependencies in all builds for iOS and macOS (flutter/flutter#171015)
2025-06-23 [email protected] Move `web_long_running_tests_{1,5}_5` to `bringup`. (flutter/flutter#171026)
2025-06-23 [email protected] rename from announce to supportsAnnounce on engine (flutter/flutter#170618)
2025-06-23 [email protected] Roll pub packages (flutter/flutter#171016)
2025-06-23 [email protected] Enhance Text Contrast for WCAG AAA Compliance (flutter/flutter#170758)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
...
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 14, 2025
@engine-flutter-autoroll
12e4a898d Support wide gamut colors when applying a DlColor to an SkP…
d8ff4b2 
…aint (flutter/flutter#170613)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 14, 2025
@engine-flutter-autoroll
12e4a898d Support wide gamut colors when applying a DlColor to an SkP…
cbe48b1 
…aint (flutter/flutter#170613)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 15, 2025
@engine-flutter-autoroll
12e4a898d Support wide gamut colors when applying a DlColor to an SkP…
fad0de6 
…aint (flutter/flutter#170613)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 15, 2025
@engine-flutter-autoroll
12e4a898d Support wide gamut colors when applying a DlColor to an SkP…
15bdd3b 
…aint (flutter/flutter#170613)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 16, 2025
@engine-flutter-autoroll
12e4a898d Support wide gamut colors when applying a DlColor to an SkP…
7715d56 
…aint (flutter/flutter#170613)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@flar flar flar approved these changes

Assignees

No one assigned

Labels

engine flutter/engine related. See also e: labels. will affect goldens Changes to golden files

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Impeller] Colors with ColorSpace.displayP3 are displayed incorrectly on Mac in release mode.

2 participants

@jason-simmons @flar