Added wide-gamut color support for `ui.Image.toByteData` and `ui.Image.colorSpace` #40031

gaaclarke · 2023-03-02T21:47:42Z

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

flutter-dashboard · 2023-03-03T17:35:55Z

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.

Changes reported for pull request #40031 at sha 992fb70

jonahwilliams · 2023-03-03T17:53:33Z

lib/ui/painting.dart

+/// The color space that an [Image] uses.
+enum ColorSpace {
+  /// The sRGB color gamut.
+  sRGB,
+  /// A color space that is backwards compatible with sRGB but can represent
+  /// colors outside of that gamut with values outside of [0..1];
+  extendedSRGB,
+}
+


I think this enum and values probably deserve a lot more explanation. I think @Hixie would probably be interesting in taking a pass over this.

FYI I'm finishing up a breaking change notice too that I planned on sharing with him for his review.

jonahwilliams · 2023-03-03T17:54:32Z

lib/ui/painting.dart

+  /// The sRGB color gamut.
+  sRGB,
+  /// A color space that is backwards compatible with sRGB but can represent
+  /// colors outside of that gamut with values outside of [0..1];


I thought that for "regular" images we gave colors channel values in the range of 0-255. Does the extended srb use 0-1?

Yep, you have to use toByteData(ImageByteFormat.rawExtendedRgba128) to get normalized color components. I can add a note here linking that.

(added a note about ImageByteFormat.rawExtendedRgba128)

lib/ui/painting.dart

yjbanov

Is this an impeller-only feature? Does Skia support this? I vaguely recall that it did. If so, we should add a proper CanvasKit implementation for this.

yjbanov · 2023-03-03T18:32:08Z

lib/web_ui/lib/painting.dart

@@ -351,6 +351,8 @@ abstract class Image {

  List<StackTrace>? debugGetOpenHandleStackTraces() => null;

+  ColorSpace get colorSpace => ColorSpace.sRGB;


Looks like all concrete subclasses are implementing this getter. I think this means that the getter in the base class can be abstract.

The reason all of the other places had to implement it themselves is because they are using implements instead of extends. I don't know if there was anyone that was using extends, but if they were, I wouldn't want them to have to implement this property.

gaaclarke · 2023-03-03T18:53:24Z

Is this an impeller-only feature? Does Skia support this? I vaguely recall that it did. If so, we should add a proper CanvasKit implementation for this.

Yep, it's on impeller + iOS currently (and behind a switch which I hope to remove). The skia backend could support it but there are no plans to invest in it. I'd love to have web support too. I don't know what that would entail but webkit can already render wide gamut images for what it's worth.

lib/ui/painting.dart

jonahwilliams · 2023-03-07T23:40:15Z

lib/ui/painting.dart

+/// This value can help you decide which [ImageByteFormat] to use with
+/// [Image.toByteData] and is the result of [Image.colorSpace].
+enum ColorSpace {
+  /// The sRGB color space, the defined standard color space for the web.


the defined standard color space for the web.

This is kind of an odd statement, I'm not sure how that helps me to know what it means for flutter. Perhaps describe this in terms of the values the colors can take and/or what encoding to use?

I don't think this value has any effect on what encoding you should use. The only way I could describe the values would be to list the frequencies in the gamut. Did you have a different idea on how we could specify it? It is kind of weird to list the web standard but it's the only thing I could grasp onto where people would have an existing familiarity with sRGB.

defined by whom? linking to the relevant standard would be useful for people new to this topic

Citations are provided in the wiki (but it was standardized by the International Electrotechnical Commission (IEC) as IEC 61966-2-1). Linking the wikipedia page.

The important part isn't whether or not this is the defined color space for the web. As a flutter developer, what does the web's color space have to do with my application? If we don't have a good answer, maybe just something like "this is the default color space for applications" or something like that

Okay, reworded it and added note about it being the colorspace of non-wide-gamut apps.

lib/ui/painting.dart

jonahwilliams · 2023-03-07T23:41:29Z

lib/ui/painting.dart

@@ -1591,6 +1605,21 @@ enum ImageByteFormat {
  /// image may use a single 8-bit channel for each pixel.
  rawUnmodified,

+  /// Raw extended range RGBA format.
+  ///
+  /// Unencoded bytes, in RGBA row-primary form with straight alpha, 32 bit


Is straight alpha premultiplied or un-premultiplied (post multiplied)?

It's un-premultiplied, "straight" is the nomenclature we use in the other fields.

jonahwilliams · 2023-03-07T23:41:44Z

lib/ui/painting.dart

@@ -1591,6 +1605,21 @@ enum ImageByteFormat {
  /// image may use a single 8-bit channel for each pixel.
  rawUnmodified,

+  /// Raw extended range RGBA format.
+  ///
+  /// Unencoded bytes, in RGBA row-primary form with straight alpha, 32 bit


I think this matches the encoding of the dart:ui Color class, right?

The dart:ui Color class is using integer values for components, this is float32.

lib/ui/painting.dart

…d `ui.Image.colorSpace` (flutter/engine#40031)

…`ui.Image.colorSpace` (#40031)" This reverts commit 7675de7.

…`ui.Image.colorSpace` (#40031)" (#40295) Revert "Added wide-gamut color support for `ui.Image.toByteData` and `ui.Image.colorSpace`"

…d `ui.Image.colorSpace` (flutter/engine#40031)

gaaclarke force-pushed the wide-gamut-ui-image branch from c019ff1 to 15743a8 Compare March 2, 2023 23:28

flutter-dashboard bot added the platform-web Code specifically for the web engine label Mar 3, 2023

gaaclarke marked this pull request as ready for review March 3, 2023 17:29

gaaclarke changed the title ~~Added wide-gamut color support for ui.Image.toByteData.~~ Added wide-gamut color support for ui.Image.toByteData and ui.Image.colorSpace Mar 3, 2023

gaaclarke changed the title ~~Added wide-gamut color support for ui.Image.toByteData and ui.Image.colorSpace~~ Added wide-gamut color support for ui.Image.toByteData and ui.Image.colorSpace Mar 3, 2023

flutter-dashboard bot added the will affect goldens label Mar 3, 2023

gaaclarke requested review from yjbanov and jonahwilliams March 3, 2023 17:50

jonahwilliams reviewed Mar 3, 2023

View reviewed changes

yjbanov reviewed Mar 3, 2023

View reviewed changes

gaaclarke requested a review from Hixie March 3, 2023 18:49

Hixie reviewed Mar 3, 2023

View reviewed changes