Add Support for Wide Gamut (DisplayP3) Colors to React Native #1

ryanlntn · 2023-12-21T18:52:51Z

Summary:

Changelog:

Test Plan:

cipolleschi

Amazing starting point. I left some suggestions here and there on how we can improve the situation.

For the global flag, I'll proceed as follow:

in the RCTConvert.h file, I'll create a couple of method like these

RCTColorSpace RCTGetDefaultColorSpace(void);
void RCTSetDefaultColorSpace(RCTColorSpace colorSpace);

in the RCTConvert.m I'll implement them like this

static BOOL defaultColorSpace = RCTColorSpaceSRGB;
RCTColorSpace RCTGetDefaultColorSpace(void)
{
     return defaultColorSpace;
}
void RCTSetDefaultColorSpace(RCTColorSpace colorSpace) 
{
     defaultColorSpace = colorSpace;
}

A user can set it up in the AppDelegate (needs to be documented properly)
We can read the default space in any iOS file

WDYT?

cipolleschi · 2023-12-27T15:22:49Z

packages/normalize-color/index.js.flow

+  +'display-p3'?: true,
+  srgb?: true,


this is not good. These two should be mutually exclusive, the color space can be both srgb and display-p3.

Flow supports enums.
Can we create an enum ColorSpace?

Great point! I'll update this to use an enum.

cipolleschi · 2023-12-27T15:28:01Z

packages/react-native/React/Base/RCTConvert.m

+    if ((value = [dictionary objectForKey:@"display-p3"]) ||
+        (value = [dictionary objectForKey:@"srgb"])) {


if we change the js representation, this could be something like

NSString * colorSpace = [dictionary objectForKey: @"space"]; if ([@[@"display-p3", @"srgb"] containsObject:colorSpace]) { RCTColorSpace space = [RCTColorSpace fromString:colorSpace]; //... }

cipolleschi · 2023-12-27T15:28:21Z

packages/react-native/React/Base/RCTConvert.m

+      CGFloat g = [[dictionary objectForKey:@"g"] floatValue];
+      CGFloat b = [[dictionary objectForKey:@"b"] floatValue];
+      CGFloat a = [[dictionary objectForKey:@"a"] floatValue];
+      ColorSpace colorSpace = [dictionary objectForKey:@"display-p3"] ? displayP3 : sRGB;


if we change the default, this would have to change.

cipolleschi · 2023-12-27T15:31:26Z

packages/react-native/ReactCommon/react/renderer/graphics/ColorComponents.h

 struct ColorComponents {
  float red{0};
  float green{0};
  float blue{0};
  float alpha{0};
+  ColorSpace colorSpace{ColorSpace::sRGB};


we should be able to create a static defaultColorSpace that we initialize based on the default color space identified by the global flag, and use that static value to initialize the structure, right?

I wasn't able to figure out importing RCTConvert here to initialize it but I was able to do it the other way around. I'm wondering if it'd make more sense to move the global flag handling over here as well to continue to consolidate this.

that's an option. If we can manipulate the C++ flag from Java/Kotlin (through JNI) and objectiveC and read that flag in a similar way, it is a great alternative.

C++ ColorSpace would be the source of truth and iOS/Android would just read from it.

cipolleschi · 2023-12-27T15:33:03Z

...xtlayoutmanager/platform/ios/react/renderer/textlayoutmanager/RCTTextPrimitivesConversions.h

+  if (components.colorSpace == facebook::react::ColorSpace::DisplayP3) {
+    return [UIColor colorWithDisplayP3Red:components.red green:components.green blue:components.blue alpha:components.alpha];
+  }
  return [UIColor colorWithRed:components.red green:components.green blue:components.blue alpha:components.alpha];


We should reuse the factory method from the RCTConvert.h

cipolleschi

Very nice work! 👏
I left some other comments to improve it even further!

cipolleschi · 2024-01-03T15:49:26Z

packages/react-native/React/Base/RCTConvert.m

+}
+void RCTSetDefaultColorSpace(RCTColorSpace colorSpace) 
+{
+  defaultColorSpace = colorSpace;


Implemented as it is, this allow for changes at runtime, meaning that we can start with a sRGB as default and, with the press of a button, switch to a DisplayP3 as default.
Perhaps it is something we are ok with. If we want to lock it down after startup, you can have a look at how it is implemented here

I actually think that could be a neat feature if someone wanted to put together a demo where they toggle between sRGB and DisplayP3.

cipolleschi · 2024-01-03T15:50:32Z

packages/react-native/React/Base/RCTConvert.m

+ (UIColor *)createColorFrom:(CGFloat)r green:(CGFloat)g blue:(CGFloat)b alpha:(CGFloat)a
+{
+  RCTColorSpace space = RCTGetDefaultColorSpace();
+  return [self createColorFrom:r green:g blue:b alpha:a andColorSpace:space];
+}
+ (UIColor *)createColorFrom:(CGFloat)red green:(CGFloat)green blue:(CGFloat)blue alpha:(CGFloat)alpha andColorSpace:(RCTColorSpace)colorSpace
+{
+  if (colorSpace == RCTColorSpaceDisplayP3) {
+    return [UIColor colorWithDisplayP3Red:red green:green blue:blue alpha:alpha];
+  }
+  return [UIColor colorWithRed:red green:green blue:blue alpha:alpha];
+}


Creating an category of UIColor * is probably more iOS idiomatic, but I don't have a strong opinion about it, for now.

cipolleschi · 2024-01-03T15:57:50Z

packages/react-native/React/Base/RCTConvert.m

+      RCTColorSpace colorSpace = [rawColorSpace isEqual: @"display-p3"]
+        ? RCTColorSpaceDisplayP3
+        : [rawColorSpace isEqual: @"srgb"] 
+        ? RCTColorSpaceSRGB
+        : RCTGetDefaultColorSpace();


I'm not a fan of nested ternary operators.
we might create a function

+ (RCTColorSpace) colorSpaceFromString:(NSString *)colorSpace { if ([colorSpace isEqualTo:@"display-p3"]) { return RCTColorSpaceDisplayP3; } else if ([colorSpace isEqualTo:@"srgb"]) { return RCTColorSpaceSRGB; } return RCTGetDefaultColorSpace(); }

and then call it here like:

RCTColorSpace colorSpace = [self colorSpaceFromString:rawColorSpace];

WDYT?

I'm the oddball nested ternary fan but that sounds good to me. 😄

cipolleschi · 2024-01-03T15:58:17Z

packages/react-native/React/Fabric/RCTConversions.h

@@ -7,6 +7,7 @@

 #import <UIKit/UIKit.h>

+#import <React/RCTLog.h>


we can remove this, right?

cipolleschi · 2024-01-03T15:59:23Z

packages/react-native/ReactCommon/react/renderer/core/graphicsConversions.h

+          ? ColorSpace::DisplayP3
+          : ColorSpace::sRGB;


this should be updated, by considering the default color space, right?

cipolleschi

Amazing work! 🙌
just let a nit, but it looks good to me.

Do you feel there is something else missing for iOS?

cipolleschi · 2024-01-04T10:30:38Z

packages/react-native/React/Base/RCTConvert.mm

@@ -439,7 +441,7 @@ + (UIKeyboardType)UIKeyboardType:(id)json RCT_DYNAMIC
    mapping = temporaryMapping;
  });

-  UIKeyboardType type = RCTConvertEnumValue("UIKeyboardType", mapping, @(UIKeyboardTypeDefault), json).integerValue;
+  UIKeyboardType type = (UIKeyboardType)RCTConvertEnumValue("UIKeyboardType", mapping, @(UIKeyboardTypeDefault), json).integerValue;


This seems unrelated. I appreaciate the effort of improving the current codebase, but I'd rather keep the work here focused on wide gamut.
Could you put this line update in a different PR? 🙏

Yeah I was trying to avoid this as well but after converting this file to ObjC++ these were required to get this compiling again. They aren't required with just ObjC. Should I include converting this to ObjC++ as well in that separate PR?

No, let's keep it here in this case.

ryanlntn · 2024-01-04T16:23:24Z

Do you feel there is something else missing for iOS?

Not that I'm aware of. I'm going to double check shadows and animations to make sure those are good but I suspect they should be.

ryanlntn · 2024-01-05T16:08:10Z

packages/rn-tester/js/examples/Animated/ColorStylesExample.js

@@ -42,7 +44,8 @@ function AnimatedView({useNativeDriver}: {useNativeDriver: boolean}) {
  const interpolationAnimatedStyle = {
    backgroundColor: animatedBaseValue.interpolate({
      inputRange: [0, 1],
-      outputRange: ['blue', 'red'],
+      // outputRange: ['blue', 'red'],
+      outputRange: ['color(display-p3 0 0 1)', 'color(display-p3 1 0 0)'],


@cipolleschi I have interpolate working without the native driver but if useNativeDriver is true the output ends in black after interpolating from blue to red. I'm a bit confused because AFAICT the output range is derived from the UIColor objects that we get from RCTConvert. I was thinking with the extended gamut that we might be getting values greater than 1 for the maximums there but when logging that doesn't appear to be the case. Any insights here?

Actually, yes, that's the case. Not only greater than 1, but even negative.

For example, if you try this code:

UIColor *c = [UIColor colorWithDisplayP3Red:1 green:0.75 blue:0.25 alpha:1]; CGFloat red, green, blue, alpha; [c getRed:&red green:&green blue:&blue alpha:&alpha];

And then you debug it, you'll see that

red => 1.045830249786377 green => 0.73694425821304321 blue => -0.059313245117664337

So, I think that there is some code that is clamping the values OR the interpolation formula does not work properly.

The other problem is that RCTFromColorComponent is not aware of the color space... So we need to update RCTInterpolateColorInRange to take the color space into consideration: if a displayP3 color is passed as input, it should provide a displayP3 color as output.
I suspect that, as of now, it is returning a sRGB color.

cipolleschi · 2024-01-08T16:01:55Z

packages/react-native/Libraries/NativeAnimation/Nodes/RCTColorAnimatedNode.mm

@@ -21,7 +21,9 @@ - (void)performUpdate
  RCTValueAnimatedNode *bNode = (RCTValueAnimatedNode *)[self.parentNodes objectForKey:self.config[@"b"]];
  RCTValueAnimatedNode *aNode = (RCTValueAnimatedNode *)[self.parentNodes objectForKey:self.config[@"a"]];

-  _color = RCTColorFromComponents(rNode.value, gNode.value, bNode.value, aNode.value);
+  _color = self.config[@"space"] != nil
+    ? RCTColorFromComponents(rNode.value * 255, gNode.value * 255, bNode.value * 255, aNode.value) 


we should pass the space, no?

@cipolleschi Yeah I believe that's the better solution. Is there any value in converting back and forth from components to integer at all anymore? Or should we just create the UIColor from the color components directly?

No, I think that at this point we can keep the UIColors. I have no idea why we used to use ints for colors, but it looks a very bad API to me.

cipolleschi · 2024-01-08T16:03:18Z

packages/react-native/React/Base/RCTConvert.mm

@@ -16,6 +16,8 @@
 #import "RCTParserUtils.h"
 #import "RCTUtils.h"

+#import <react/renderer/graphics/ColorComponents.h>


Note for us: this might require us to update the podspec to include react-graphics, if that's not like this already. Not a problem, but just mentioning not to forget about it if we see failures in CI due to Dynamic Frameworks.

cipolleschi · 2024-01-08T16:03:32Z

packages/react-native/React/Base/RCTConvert.mm

@@ -439,7 +441,7 @@ + (UIKeyboardType)UIKeyboardType:(id)json RCT_DYNAMIC
    mapping = temporaryMapping;
  });

-  UIKeyboardType type = RCTConvertEnumValue("UIKeyboardType", mapping, @(UIKeyboardTypeDefault), json).integerValue;
+  UIKeyboardType type = (UIKeyboardType)RCTConvertEnumValue("UIKeyboardType", mapping, @(UIKeyboardTypeDefault), json).integerValue;


No, let's keep it here in this case.

cipolleschi · 2024-01-08T16:06:29Z

Also, can you invite me to your fork of react-native? It will be easier to help out if needed! 😄

This reverts commit 53aba9b.

ryanlntn · 2024-01-09T04:16:39Z

packages/react-native/Libraries/NativeAnimation/RCTAnimationUtils.mm

+
+//  return [UIColor colorWithRed:r green:g blue:b alpha:a];
+  return @{
+    @"space": @"srgb",


@cipolleschi I'm still struggling with how to handle the unclamped values here. If I understand Apple's docs correctly we'd need to pass those to colorWithRed:green:blue:alpha: so we'd want to stay in extended srgb color space. But this doesn't appear to work.

Also I'm not sure I understand why I can't just use UIColor. When I try to return the UIColor directly here or from RCTColorAnimatedNode I don't get the color to come through at all.

I'm not sure I completely understand your questions, but:

If I understand Apple's docs correctly we'd need to pass those to colorWithRed:green:blue:alpha: so we'd want to stay in extended srgb color space. But this doesn't appear to work.

This is true if the source space is sRGB.
If we are in displayP3, values that are < 0 and > 1 will not be properly represented. 🤔

I think we need to create the final UIColor by using colorWithDisplayP3Red:green:blue:alpha:.

Also I'm not sure I understand why I can't just use UIColor. When I try to return the UIColor directly here or from RCTColorAnimatedNode I don't get the color to come through at all

If we change the return type from int32_t to something else, we have to update the callsites to understand the new return type, no?

I'm not super familiar with this part of the codebase, I need to dig deeper here.

If this is the only blocker, I think we can move forward for the time being and consider this a known-issue: for the first implementation interpolateColor don't work with displayP3.

Even with this limitation, this would already be a massive improvement! I think that animating colors is not something every app actually require, so it is an edge case that it is ok to point out as "not working" for version 1 of the implementation.

Using colorWithDisplayP3Red:green:blue:alpha: gives me the same behavior as before including the color space and I believe it's because per Apple:

The red component of the color object, specified as a value from 0.0 to 1.0.

and

Values below 0.0 are interpreted as 0.0, and values above 1.0 are interpreted as 1.0.

Meanwhile with colorWithRed:green:blue:alpha:

The red value of the color object. On applications linked for iOS 10 or later, the color is specified in an extended color space, and the input value is never clamped. On earlier versions of iOS, red values below 0.0 are interpreted as 0.0, and values above 1.0 are interpreted as 1.0.

So I believe if we have unclamped values we may actually need to use colorWithRed:green:blue:alpha: to have them interpreted correctly. But then again I couldn't get it to work completely either way.

As far as returning UIColor directly goes I updated the call sites for RCTInterpolateColorInRange and RCTColorAnimatedNode but I was having trouble following what happens to the values after performUpdate is called. Essentially I'd like to know how the animated values ultimately get applied back to the original views.

If this is the only blocker, I think we can move forward for the time being and consider this a known-issue: for the first implementation interpolateColor don't work with displayP3.

Even with this limitation, this would already be a massive improvement! I think that animating colors is not something every app actually require, so it is an edge case that it is ok to point out as "not working" for version 1 of the implementation.

Makes sense. I'll move on to Android today.

To explore how the colors change based on the rgb values, I created this small project: https://github.com/cipolleschi/P3Colors.

feel free to clone it and play with it.

The top-left view is sRGB
The to-right view is displayP3
The bottom-left view is sRGB when we passed in the rgb values obtained by getRed:green:blue:alpha method on the displayP3 color.
The bottom-right view is displayP3 when we passed in the rgb values obtained by getRed:green:blue:alpha method on the displayP3 color.

Use a retina display to look at it! 😄

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-01-10.at.15.49.28.mp4

Interesting. It looks like my interpretation of Apple's docs is correct then since passing the DisplayP3 colors to colorWithRed:green:blue:alpha: gives the same color as the original DisplayP3 color while passing to colorWithDisplayP3Red:green:blue:alpha: is too red up until you hit values greater than 1 at which point it's the same.

The behavior right now in RCTInterpolateColorInRange is even more bizarre then since it's currently cycling through yellow and blue as well. 🤔

yeah... it hink that the behavior of interpolate is wrong in the context of display P3 because the color space is non euclidean. So the usual interpolation formula (which are linear) would not work properly there... 😓
Unfortunately, I don't have a clear solution...
One idea we can try to:

preprocess: interpolate the starting color from displayP3 to sRGB representation (e.g. red in P3 to red in sRGB)

interpolation: interpolate the colors in sRGB, from the source to the destination (e.g from red to blue) reusing the sRGB formula

postprocess: interpolate the final color from sRGB to displayP3 (e.g. blue in P3 to blue in sRGB).

In that way we minimize the weirdness paying the tradeoff that interpolation is in sRGB... but if that happens mainly in animation, they should be "fast" enough so that the user won't notice that they are not in displayP3 space...

What do you think?

This reverts commit 9f30b94.

…gets Summary: Changelog: [Internal] # This diff 1. Provides all Targets with an `executorFromThis()` method, which can be used from within a Target to access a *`this`-scoped main thread executor* = a `std::function` that will execute a callback asynchronously iff the current Target isn't destroyed first. 2. Refactors how (all) Target objects are constructed and retained, from a plain constructor to `static shared_ptr create()`. This is because `executorFromThis()` relies internally on `enable_shared_from_this` plus two-phase construction to populate the executor. 3. Creates utilities for deriving scoped executors from other executors and `shared_ptr`s. The concept is very much like `RuntimeExecutor` in reverse: the #1 use case is moving from the JS thread back to the main thread - where "main thread" is defined loosely as "anywhere it's legal to call methods on Target/Agent objects, access session state, etc". The actual dispatching mechanism is left up to the owner of each `PageTarget` object; for now we only have an iOS integration, where we use `RCTExecuteOnMainQueue`. Coupling the ownership/lifetime semantics with task scheduling is helpful, because it avoids the footgun of accidentally/nondeterministically moving `shared_ptr`s (and destructors!) to a different thread/queue . # This stack I'm refactoring the way the Runtime concept works in the modern CDP backend to bring it in line with the Page/Instance concepts. Overall, this will let us: * Integrate with engines that require us to instantiate a shared Target-like object (e.g. Hermes AsyncDebuggingAPI) in addition to an per-session Agent-like object. * Access JSI in a CDP context (both at target setup/teardown time and during a CDP session) to implement our own engine-agnostic functionality (`console` interception, `Runtime.addBinding`, etc). * Manage CDP execution contexts natively in RN, and (down the line) enable first-class debugging support for multiple Runtimes in an Instance. The core diffs in this stack: * ~~Introduce a `RuntimeTarget` class similar to `{Page,Instance}Target`. ~~ * ~~Make runtime registration explicit (`InstanceTarget::registerRuntime` similar to `PageTarget::registerInstance`). ~~ * ~~Rename the existing `RuntimeAgent` interface to `RuntimeAgentDelegate`.~~ * ~~Create a new concrete `RuntimeAgent` class similar to `{Page,Instance}Agent`.~~ * ~~Provide `RuntimeTarget` and `RuntimeAgent` with primitives for safe JSI access, namely a `RuntimeExecutor` for scheduling work on the JS thread.~~ * Provide RuntimeTarget with mechanism for scheduling work on the "main" thread from the JS thread, for when we need to do more than just send a CDP message (which we can already do with the thread-safe `FrontendChannel`) in response to a JS event. *← This diff* ## Architecture diagrams Before this stack: https://pxl.cl/4h7m0 After this stack: https://pxl.cl/4h7m7 Reviewed By: hoxyq Differential Revision: D53356953 fbshipit-source-id: 152c784eb64e9b217fc2966743b33f61bd8fd97e

cipolleschi reviewed Dec 27, 2023

View reviewed changes

cipolleschi reviewed Jan 3, 2024

View reviewed changes

cipolleschi approved these changes Jan 4, 2024

View reviewed changes

ryanlntn commented Jan 5, 2024

View reviewed changes

ryanlntn added 15 commits January 5, 2024 11:03

update normalizeColor and processColor to handle color()

134b675

initial DisplayP3 support for iOS

396d05e

fix testIDs

f4539c6

update js to use enum for color space

ee6d760

add global flag and reuse createColorFrom

12e3c59

add static defaultColorSpace to ColorComponents

88b16da

add colorSpaceFromString remove nested ternary

284653c

fix RCTConversions and graphicsConversions

51414ea

missed a cast

4c81a65

fix ColorComponents defaultColorSpace by providing getter

c5eab07

update graphicsConversions to use getDefaultColorSpace

c9bddc8

fix animated interpolation without native driver

53090da

handle color space in AnimatedColor

a36c9fc

mostly fix animation with native driver

7c5eeb9

temp include example animation

4b6a0e8

ryanlntn force-pushed the feat/wide-gamut-color branch from 00b803e to 4b6a0e8 Compare January 5, 2024 18:17

cipolleschi reviewed Jan 8, 2024

View reviewed changes

ryanlntn added 5 commits January 8, 2024 09:25

split out ColorComponents implementation to fix Android build

53aba9b

Revert "split out ColorComponents implementation to fix Android build"

9f30b94

This reverts commit 53aba9b.

fix RCTGetDefaultColorSpace and RCTSetDefaultColorSpace

f303df1

refactor away from RCTColorFromComponents in RCTColorAnimatedNode

f975b6b

refactor RCTInterpolateColorInRange

1b41b02

ryanlntn commented Jan 9, 2024

View reviewed changes

Reapply "split out ColorComponents implementation to fix Android build"

6a99355

This reverts commit 9f30b94.

ryanlntn added 19 commits January 9, 2024 12:59

initial Android handling of color function values (always sRGB for now)

2083fa0

fix crash in ColorAnimatedNode

f396fc3

fix color interpolation with native driver by reprocessing as srgb

e4b0949

set ReactActivity window color mode to COLOR_MODE_WIDE_COLOR_GAMUT

506d1ab

get p3 backgrounds working

96ade88

get display-p3 borders working in Android

d3895f9

fix issues with bad defaults

a8d00a4

update remaining color annotated props to long

e2884ad

update MapBuffer to support long values

f21a2a5

change toAndroidRepr return type to int64_t

f0e1f69

update toAndroidRepr to support DisplayP3

d65366e

update TextAttributesProps to support long colors

5352b6d

update ReactForegroundColorSpan so P3 text works

b5431ee

fix issues with basic FlatList example

366001c

update foreground and background color spans

6344460

pass longs through scroll view managers to scroll views

e8cee4d

update endFillColor to support long colors

d07c577

update switch

c6f8d99

clean up ReactViewBackgroundDrawable

b072f06

		if ((value = [dictionary objectForKey:@"display-p3"]) \|\|
		(value = [dictionary objectForKey:@"srgb"])) {

		@@ -7,6 +7,7 @@

		#import <UIKit/UIKit.h>

		#import <React/RCTLog.h>

Add Support for Wide Gamut (DisplayP3) Colors to React Native #1

Are you sure you want to change the base?

Add Support for Wide Gamut (DisplayP3) Colors to React Native #1

Conversation

ryanlntn commented Dec 21, 2023

Summary:

Changelog:

Test Plan:

cipolleschi left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cipolleschi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cipolleschi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanlntn commented Jan 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cipolleschi Jan 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cipolleschi commented Jan 8, 2024 • edited Loading

Choose a reason for hiding this comment

cipolleschi Jan 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cipolleschi left a comment •

edited

Loading

cipolleschi Jan 8, 2024 •

edited

Loading

cipolleschi commented Jan 8, 2024 •

edited

Loading

cipolleschi Jan 9, 2024 •

edited

Loading