Update to m114 #1604

wcandillon · 2023-05-22T06:04:12Z

Refresh list of private headers that we need copy (this should be to done on every upgrade)
Add SK_GANESH to the list of preprocessor directives (m113)
Add duplicatesStrategy = 'include' to the header copy on Android due to headers which have moved location but a redirect at the old location is kept for backward compatibility. An example of such header would be: https://github.com/google/skia/blob/main/include/core/SkEncodedImageFormat.h
All SkImage factories have been moved to SkImages (m114)
image->encodeToData doesn't exists anymore. (m114)
SkPaint::getFillPath has been replaced with skpathutils::FillPathWithPaint (m110)
SkParsePath::ToSVGString now returns the string (m110)

chrfalch

Great work!

I'd like us to have a list with explanations of all the build changes you've made and why they were made - This is important so that we can go back and use it for reference later. The code changes are self explanatory, but the lint-, CMakeLists, podspec etc needs to be documented at least in this PRs description.

wcandillon · 2023-05-23T09:38:24Z

scripts/skia-configuration.ts

@@ -165,7 +165,6 @@ export const configurations: Configuration = {
    },
    args: [
      ["skia_use_metal", true],
-      ["skia_use_gl", true],


this was just removed because superfluous.

Agree - but we need a description of why it was superfluous - I know you removed SK_GL, but SK_GL was previously necessary to build on iOS with Metal - a description in the PR needs to contain the information of why this was now removed (probl. because it is no longer needed to make it build) and the tests you've made to verify this. It is also important that the PR's main description contains this information - it is important reference for later. As I said the code changes in C++ is self explanatory, so they only needs one line in the description.

So please add a list of main changes and why they were made on the top :) (not as comments :) :) )

I think I just assumed that it was superfluous and removed it and it works. Also the skia repository contains a list of build recipes and skia_use_metal=true skia_use_gl=true is not a recipe that exists. Since when this has changed, I don't know.

wcandillon · 2023-05-23T09:39:38Z

package/react-native-skia.podspec

@@ -22,7 +22,7 @@ Pod::Spec.new do |s|

  s.requires_arc = true
  s.pod_target_xcconfig = {
-    'GCC_PREPROCESSOR_DEFINITIONS' => '$(inherited) SK_GL=1 SK_METAL=1',
+    'GCC_PREPROCESSOR_DEFINITIONS' => '$(inherited) SK_METAL=1 SK_GANESH=1',


found from https://skia.googlesource.com/skia/+/refs/heads/main/RELEASE_NOTES.md#milestone-113
SK_GL was superfluous

wcandillon · 2023-05-23T09:39:44Z

package/ios/RNSkia-iOS/ViewScreenshotService.mm

@@ -85,7 +85,7 @@ - (instancetype)initWithUiManager:(RCTUIManager *)uiManager {
                        kBGRA_8888_SkColorType, kPremul_SkAlphaType);

  // ... and then create the SkImage itself!
-  return SkImage::MakeRasterData(info, skData, bytesPerRow);
+  return SkImages::RasterFromData(info, skData, bytesPerRow);


https://skia.googlesource.com/skia/+/refs/heads/main/RELEASE_NOTES.md#milestone-114

wcandillon · 2023-05-23T09:40:10Z

package/cpp/rnskia/dom/nodes/JsiPathNode.h

-          if (!strokePaint.getFillPath(*_path.get(), p.get(), nullptr,
-                                       precision)) {
+          if (!skpathutils::FillPathWithPaint(*_path.get(), strokePaint,
+                                              p.get(), nullptr, precision)) {


https://skia.googlesource.com/skia/+/refs/heads/main/RELEASE_NOTES.md#milestone-110

wcandillon · 2023-05-23T09:40:22Z

package/cpp/api/JsiSkPath.h

@@ -320,8 +323,7 @@ class JsiSkPath : public JsiSkWrappingSharedPtrHostObject<SkPath> {

  JSI_HOST_FUNCTION(toSVGString) {
    SkPath path = *getObject();
-    SkString s;
-    SkParsePath::ToSVGString(path, &s);
+    auto s = SkParsePath::ToSVGString(path);


https://skia.googlesource.com/skia/+/refs/heads/main/RELEASE_NOTES.md#milestone-110

wcandillon · 2023-05-23T09:40:33Z

package/cpp/api/JsiSkPath.h

+        skpathutils::FillPathWithPaint(path, p, &path, nullptr, precision);
+    if (result) {
+      getObject()->swap(path);
+    }


https://skia.googlesource.com/skia/+/refs/heads/main/RELEASE_NOTES.md#milestone-110

wcandillon · 2023-05-23T09:40:39Z

package/cpp/api/JsiSkImageFactory.h

@@ -30,7 +30,7 @@ class JsiSkImageFactory : public JsiSkHostObject {
    auto imageInfo = JsiSkImageInfo::fromValue(runtime, arguments[0]);
    auto pixelData = JsiSkData::fromValue(runtime, arguments[1]);
    auto bytesPerRow = arguments[2].asNumber();
-    auto image = SkImage::MakeRasterData(*imageInfo, pixelData, bytesPerRow);
+    auto image = SkImages::RasterFromData(*imageInfo, pixelData, bytesPerRow);


https://skia.googlesource.com/skia/+/refs/heads/main/RELEASE_NOTES.md#milestone-114

wcandillon · 2023-05-23T09:40:43Z

package/cpp/api/JsiSkImageFactory.h

@@ -18,7 +18,7 @@ class JsiSkImageFactory : public JsiSkHostObject {
 public:
  JSI_HOST_FUNCTION(MakeImageFromEncoded) {
    auto data = JsiSkData::fromValue(runtime, arguments[0]);
-    auto image = SkImage::MakeFromEncoded(data);
+    auto image = SkImages::DeferredFromEncodedData(data);


https://skia.googlesource.com/skia/+/refs/heads/main/RELEASE_NOTES.md#milestone-114

wcandillon · 2023-05-23T09:40:58Z

package/cpp/api/JsiSkImage.h

+    } else {
+      SkPngEncoder::Options options;
+      data = SkPngEncoder::Encode(nullptr, image.get(), options);
+    }


https://skia.googlesource.com/skia/+/refs/heads/main/RELEASE_NOTES.md#milestone-114

wcandillon · 2023-05-23T09:41:03Z

package/cpp/api/JsiSkImage.h

+    } else {
+      SkPngEncoder::Options options;
+      data = SkPngEncoder::Encode(nullptr, getObject().get(), options);
+    }


https://skia.googlesource.com/skia/+/refs/heads/main/RELEASE_NOTES.md#milestone-114

wcandillon · 2023-05-23T09:41:08Z

package/android/cpp/jni/JniPlatformContext.cpp

@@ -101,7 +101,7 @@ sk_sp<SkImage> JniPlatformContext::takeScreenshotFromViewTag(size_t tag) {
  // Create pixmap from pixels and make a copy of it so that
  // the SkImage owns its own pixels
  SkPixmap pm(skInfo, pixels, bmi.stride);
-  auto skImage = SkImage::MakeRasterCopy(pm);
+  auto skImage = SkImages::RasterFromPixmapCopy(pm);


https://skia.googlesource.com/skia/+/refs/heads/main/RELEASE_NOTES.md#milestone-114

wcandillon · 2023-05-23T09:42:12Z

package/android/build.gradle

@@ -278,6 +278,7 @@ if (ENABLE_PREFAB) {
        into "${project.buildDir}/headers/rnskia/"
        includeEmptyDirs = false
        include "**/*.h"
+        duplicatesStrategy = 'include'


https://skia.googlesource.com/skia/+/refs/heads/main/RELEASE_NOTES.md#milestone-114 (the duplicated files link directly to the moved files and will be removed eventually from Skia).

wcandillon · 2023-05-23T09:42:26Z

package/android/CMakeLists.txt

@@ -3,7 +3,7 @@ cmake_minimum_required(VERSION 3.4.1)

 set (CMAKE_VERBOSE_MAKEFILE ON)
 set (CMAKE_CXX_STANDARD 17)
-set (CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -DSK_GL -DSK_BUILD_FOR_ANDROID -DFOLLY_NO_CONFIG=1 -DFOLLY_HAVE_CLOCK_GETTIME=1 -DFOLLY_HAVE_MEMRCHR=1 -DFOLLY_USE_LIBCPP=1 -DFOLLY_MOBILE=1 -DON_ANDROID -DONANDROID")
+set (CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -DSK_GL -DSK_GANESH -DSK_BUILD_FOR_ANDROID -DFOLLY_NO_CONFIG=1 -DFOLLY_HAVE_CLOCK_GETTIME=1 -DFOLLY_HAVE_MEMRCHR=1 -DFOLLY_USE_LIBCPP=1 -DFOLLY_MOBILE=1 -DON_ANDROID -DONANDROID")


https://skia.googlesource.com/skia/+/refs/heads/main/RELEASE_NOTES.md#milestone-113

wcandillon · 2023-05-23T09:43:19Z

I linked to the relevant milestone (couldn't link directly to the item within the milestone) but we can discuss the changes offline as well if needed.

wcandillon · 2023-05-23T09:46:50Z

scripts/copy-skia-module-headers.ts

@@ -13,8 +13,7 @@ const copyModule = (module: string) => [
  `cp -a ./externals/skia/modules/skcms/. ./package/cpp/skia/modules/skcms`,
  `mkdir -p ./package/cpp/skia/src/`,
  `mkdir -p ./package/cpp/skia/src/core/`,
-  `cp -a ./externals/skia/src/core/SkLRUCache.h ./package/cpp/skia/src/core/.`,
-  `cp -a ./externals/skia/src/core/SkTInternalLList.h ./package/cpp/skia/src/core/.`,
+  `cp -a ./externals/skia/src/core/SkTHash.h ./package/cpp/skia/src/core/.`,


here I first removed the SkLRUCache.h SkTInternalLList.h and then added the ones which were needed. This is sensible to do this for every upgrade since these private API will be changing a lot everytime.

wcandillon · 2023-05-23T09:49:15Z

The way I approach the upgrade was:
To remove the private headers from the copy-header function to only have the ones that are needed for the current version.
For every build break, I was able to find the solution in the milestone notes (linked to it in the comments).
The duplicate header issue on android felt obivous when looking at the duplicates and the milestone notes to refer to the header change (with the previous location kept to be backward compatible).

chrfalch

Add a throughout explanation in the PR’s description about the changes (not each code change, they are kind of self explanatory) - but at least about each of the build file changes and why they were made.

Create a simple list of changes like this:

Changed "reanimtead/js-function-in-worklet" to off because ......
Added define SK_GANESH in Android build because...
Added duplicatesStrategy "include" in Android build.gradle because...
Removed define SK_GL since we can now build for Metal without this define on iOS
Copied the file SkTHash.h in our build script since...

…-skia into chrome/m114

wcandillon · 2023-05-23T11:26:57Z

I upgraded the description accordingly

chrfalch

Looking good, thanks for documenting the changes - it'll help us down the road.

Next step is to merge to main, start the Skia build and then create a new release!! :) :)

wcandillon · 2023-05-23T11:41:58Z

let's do it finger crossed 🤞🏻

# Why update @shopify/react-native-skia vendoring module for sdk 49 # How - [tools] update patch - `et uvm -m @shopify/react-native-skia -c 0.1.193` - backport skia chrome/m114 to sdk47 and sdk48 code. reference changes from Shopify/react-native-skia#1604 # Test Plan - versioned expo go + unversioned ncl skia - versioned expo go + sdk 48 ncl skia

⬆️

ef6dc62

wcandillon requested a review from chrfalch May 22, 2023 06:04

wcandillon added 2 commits May 22, 2023 08:49

Fix CI

ea7e4bb

Fix CI

58e378a

wcandillon self-assigned this May 23, 2023

chrfalch reviewed May 23, 2023

View reviewed changes

Merge branch 'main' into chrome/m114

86afcd0

wcandillon commented May 23, 2023

View reviewed changes

wcandillon requested a review from chrfalch May 23, 2023 09:43

wcandillon commented May 23, 2023

View reviewed changes

chrfalch reviewed May 23, 2023

View reviewed changes

wcandillon added 2 commits May 23, 2023 13:15

Remove bogus change

7fbd22d

Merge branch 'chrome/m114' of https://github.com/Shopify/react-native…

4404343

…-skia into chrome/m114

wcandillon requested a review from chrfalch May 23, 2023 11:26

chrfalch approved these changes May 23, 2023

View reviewed changes

wcandillon merged commit 6c2af98 into main May 23, 2023
5 of 7 checks passed

wcandillon deleted the chrome/m114 branch May 24, 2023 08:05

Kudo mentioned this pull request Jun 15, 2023

[go] update @shopify/react-native-skia to 0.1.193 expo/expo#22900

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update to m114 #1604

Update to m114 #1604

wcandillon commented May 22, 2023 •

edited

chrfalch left a comment

wcandillon May 23, 2023

chrfalch May 23, 2023

wcandillon May 23, 2023

wcandillon May 23, 2023

wcandillon May 23, 2023

wcandillon May 23, 2023

wcandillon May 23, 2023

wcandillon May 23, 2023

wcandillon May 23, 2023

wcandillon May 23, 2023

wcandillon May 23, 2023

wcandillon May 23, 2023

wcandillon May 23, 2023

wcandillon May 23, 2023

wcandillon May 23, 2023

wcandillon commented May 23, 2023

wcandillon May 23, 2023

wcandillon commented May 23, 2023

chrfalch left a comment

wcandillon commented May 23, 2023

chrfalch left a comment

wcandillon commented May 23, 2023

Update to m114 #1604

Update to m114 #1604

Conversation

wcandillon commented May 22, 2023 • edited

chrfalch 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

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

wcandillon commented May 23, 2023

Choose a reason for hiding this comment

wcandillon commented May 23, 2023

chrfalch left a comment

Choose a reason for hiding this comment

wcandillon commented May 23, 2023

chrfalch left a comment

Choose a reason for hiding this comment

wcandillon commented May 23, 2023

wcandillon commented May 22, 2023 •

edited