From e4877ed9859f6d556d8ec1ce288a3ebaaab12ff2 Mon Sep 17 00:00:00 2001 From: Valentin Shergin Date: Thu, 17 Sep 2020 11:09:42 -0700 Subject: [PATCH] Fabric: Using `optional` instead of `CGColorRef` on iOS Summary: Finally, this diff changes the internal implementation of SharedColor to be `optional`. Initially, when we started working on the new renderer, it seemed like a good idea to allocated CGColor objects ahead of time and store them with Props. Now, this idea does not look so good, especially because: * Having `SharedColor` as a `shared_ptr` is a quite big perf overhead for copying this thing. And the size of the object is not small. * Having `SharedColor` as a `shared_ptr` creates huge interconnectedness among pieces of the core and rendering. E.g. improper releasing a pointer in some component view can cause a crash somewhere in the core (because both places might use the same shared `blackColor`. On Android, we already use simple `int` as a representation of a color, and this works great. And this diff implements something very similar to Android, but a bit differently: here we use `optional` instead of custom class with a single `int` field and some magic value that represents "empty value". This approach should fix T75836417 because now we don't have allocation and deallocation when we simply assign color values. If this solution works fine on iOS, I will do unify all implementations among all platforms. Changelog: [Internal] Fabric-specific internal change. Reviewed By: JoshuaGross Differential Revision: D23753507 fbshipit-source-id: 42fd6cee6bf7b39c92c88536da06ba9e8cf4d4db --- ReactCommon/react/renderer/graphics/BUCK | 2 ++ .../renderer/graphics/platform/ios/Color.cpp | 33 ++++++++----------- .../renderer/graphics/platform/ios/Color.h | 8 ++--- 3 files changed, 19 insertions(+), 24 deletions(-) diff --git a/ReactCommon/react/renderer/graphics/BUCK b/ReactCommon/react/renderer/graphics/BUCK index 9b7fd05c2d0..87064ce89c0 100644 --- a/ReactCommon/react/renderer/graphics/BUCK +++ b/ReactCommon/react/renderer/graphics/BUCK @@ -7,6 +7,7 @@ load( "fb_xplat_cxx_test", "get_apple_compiler_flags", "get_apple_inspector_flags", + "react_native_xplat_target", "rn_xplat_cxx_library", "subdir_glob", ) @@ -95,6 +96,7 @@ rn_xplat_cxx_library( tests = [":tests"], visibility = ["PUBLIC"], deps = [ + react_native_xplat_target("better:better"), "//third-party/glog:glog", "//xplat/fbsystrace:fbsystrace", "//xplat/folly:headers_only", diff --git a/ReactCommon/react/renderer/graphics/platform/ios/Color.cpp b/ReactCommon/react/renderer/graphics/platform/ios/Color.cpp index 8cbdeee7fdc..0b114759cb1 100644 --- a/ReactCommon/react/renderer/graphics/platform/ios/Color.cpp +++ b/ReactCommon/react/renderer/graphics/platform/ios/Color.cpp @@ -12,28 +12,21 @@ namespace facebook { namespace react { SharedColor colorFromComponents(ColorComponents components) { - const CGFloat componentsArray[] = { - components.red, components.green, components.blue, components.alpha}; - - auto color = CGColorCreate(CGColorSpaceCreateDeviceRGB(), componentsArray); - - return SharedColor(color, CGColorRelease); + float ratio = 255.9999; + return SharedColor( + ((int)(components.alpha * ratio) & 0xff) << 24 | + ((int)(components.red * ratio) & 0xff) << 16 | + ((int)(components.green * ratio) & 0xff) << 8 | + ((int)(components.blue * ratio) & 0xff)); } -ColorComponents colorComponentsFromColor(SharedColor color) { - if (!color) { - // Empty color object can be considered as `clear` (black, fully - // transparent) color. - return ColorComponents{0, 0, 0, 0}; - } - - auto numberOfComponents __unused = CGColorGetNumberOfComponents(color.get()); - assert(numberOfComponents == 4); - const CGFloat *components = CGColorGetComponents(color.get()); - return ColorComponents{(float)components[0], - (float)components[1], - (float)components[2], - (float)components[3]}; +ColorComponents colorComponentsFromColor(SharedColor sharedColor) { + float ratio = 256; + Color color = *sharedColor; + return ColorComponents{(float)((color >> 16) & 0xff) / ratio, + (float)((color >> 8) & 0xff) / ratio, + (float)((color >> 0) & 0xff) / ratio, + (float)((color >> 24) & 0xff) / ratio}; } SharedColor clearColor() { diff --git a/ReactCommon/react/renderer/graphics/platform/ios/Color.h b/ReactCommon/react/renderer/graphics/platform/ios/Color.h index abaf98f4245..0b8ab406b40 100644 --- a/ReactCommon/react/renderer/graphics/platform/ios/Color.h +++ b/ReactCommon/react/renderer/graphics/platform/ios/Color.h @@ -7,17 +7,17 @@ #pragma once -#include +#include -#include #include #include namespace facebook { namespace react { -using Color = CGColor; -using SharedColor = std::shared_ptr; +using Color = int32_t; + +using SharedColor = better::optional; SharedColor colorFromComponents(ColorComponents components); ColorComponents colorComponentsFromColor(SharedColor color);