fix(🖼️): fix thread safety in SkImage and viewRef.makeImageSnapshot (#…

…2761) Add thread boundaries on images that could be used in a different thread than the one they were created on. The solution is to use the graphic context available in the thread instead of using the "saved" within the image. Skia now has a `SK_IMAGE_READ_PIXELS_DISABLE_LEGACY_API` that helps with catching these (however APIs like `makeImageSnapshot(GrDirectContext* gr = nullptr)` are not caught by the flag).
Shopify · Nov 22, 2024 · f4db5f5 · f4db5f5
1 parent d314745
commit f4db5f5
Show file tree

Hide file tree

Showing 15 changed files with 60 additions and 81 deletions.
diff --git a/apps/paper/ios/Podfile.lock b/apps/paper/ios/Podfile.lock
@@ -1935,7 +1935,7 @@ SPEC CHECKSUMS:
   React-Mapbuffer: 1c08607305558666fd16678b85ef135e455d5c96
   React-microtasksnativemodule: 87b8de96f937faefece8afd2cb3a518321b2ef99
   react-native-safe-area-context: ab8f4a3d8180913bd78ae75dd599c94cce3d5e9a
-  react-native-skia: 1f36fe252564881d77ad424f84a8986098cc73b7
+  react-native-skia: 1549ee5068efc5a004b84b2e0ba109c6234e2fde
   react-native-slider: 97ce0bd921f40de79cead9754546d5e4e7ba44f8
   react-native-wgpu: 8d0437a304318e0e3d6ccbfed2a39880f8eae4dd
   React-nativeconfig: 57781b79e11d5af7573e6f77cbf1143b71802a6d

diff --git a/packages/skia/android/CMakeLists.txt b/packages/skia/android/CMakeLists.txt
@@ -4,7 +4,7 @@ cmake_minimum_required(VERSION 3.4.1)
 set (CMAKE_VERBOSE_MAKEFILE ON)
 set (CMAKE_CXX_STANDARD 17)
 set(SK_GRAPHITE OFF)
-set (CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -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_BUILD_FOR_ANDROID -DSK_IMAGE_READ_PIXELS_DISABLE_LEGACY_API -DFOLLY_NO_CONFIG=1 -DFOLLY_HAVE_CLOCK_GETTIME=1 -DFOLLY_HAVE_MEMRCHR=1 -DFOLLY_USE_LIBCPP=1 -DFOLLY_MOBILE=1 -DON_ANDROID -DONANDROID")
 set (PACKAGE_NAME "rnskia")
 set (SKIA_LIB "skia")
 set (SKIA_SVG_LIB "svg")

diff --git a/packages/skia/android/cpp/rnskia-android/OpenGLContext.h b/packages/skia/android/cpp/rnskia-android/OpenGLContext.h
@@ -129,9 +129,11 @@ class OpenGLContext {
   std::unique_ptr<WindowContext> MakeWindow(ANativeWindow *window, int width,
                                             int height) {
     return std::make_unique<OpenGLWindowContext>(
-        _directContext, _glDisplay.get(), _glContext.get(), window);
+        _directContext.get(), _glDisplay.get(), _glContext.get(), window);
   }
 
+  GrDirectContext *getDirectContext() { return _directContext.get(); }
+
 private:
   EGLConfig _glConfig;
   std::unique_ptr<gl::Display> _glDisplay;

diff --git a/packages/skia/android/cpp/rnskia-android/OpenGLWindowContext.cpp b/packages/skia/android/cpp/rnskia-android/OpenGLWindowContext.cpp
@@ -45,7 +45,7 @@ sk_sp<SkSurface> OpenGLWindowContext::getSurface() {
     sk_sp<SkColorSpace> colorSpace(nullptr);
     SkSurfaceProps surfaceProps(0, kRGB_H_SkPixelGeometry);
     _skSurface = SkSurfaces::WrapBackendRenderTarget(
-        _directContext.get(), backendRT, kBottomLeft_GrSurfaceOrigin,
+        _directContext, backendRT, kBottomLeft_GrSurfaceOrigin,
         kRGBA_8888_SkColorType, colorSpace, &surfaceProps);
   }
   return _skSurface;

diff --git a/packages/skia/android/cpp/rnskia-android/OpenGLWindowContext.h b/packages/skia/android/cpp/rnskia-android/OpenGLWindowContext.h
@@ -33,9 +33,8 @@ namespace RNSkia {
 
 class OpenGLWindowContext : public WindowContext {
 public:
-  OpenGLWindowContext(sk_sp<GrDirectContext> directContext,
-                      gl::Display *display, gl::Context *glContext,
-                      ANativeWindow *window)
+  OpenGLWindowContext(GrDirectContext *directContext, gl::Display *display,
+                      gl::Context *glContext, ANativeWindow *window)
       : _directContext(directContext), _display(display), _glContext(glContext),
         _window(window) {
     ANativeWindow_acquire(_window);
@@ -60,11 +59,11 @@ class OpenGLWindowContext : public WindowContext {
   void resize(int width, int height) override { _skSurface = nullptr; }
 
 private:
-  sk_sp<GrDirectContext> _directContext;
+  GrDirectContext *_directContext;
   gl::Display *_display;
+  gl::Context *_glContext = nullptr;
   ANativeWindow *_window;
   sk_sp<SkSurface> _skSurface = nullptr;
-  gl::Context *_glContext = nullptr;
   std::unique_ptr<gl::Surface> _glSurface = nullptr;
 };
 

diff --git a/packages/skia/android/cpp/rnskia-android/RNSkAndroidPlatformContext.h b/packages/skia/android/cpp/rnskia-android/RNSkAndroidPlatformContext.h
@@ -152,6 +152,12 @@ class RNSkAndroidPlatformContext : public RNSkPlatformContext {
 #endif
   }
 
+#if !defined(SK_GRAPHITE)
+  GrDirectContext *getDirectContext() override {
+    return OpenGLContext::getInstance().getDirectContext();
+  }
+#endif
+
   sk_sp<SkFontMgr> createFontMgr() override {
     return SkFontMgr_New_Android(nullptr);
   }

diff --git a/packages/skia/cpp/api/JsiSkImage.h b/packages/skia/cpp/api/JsiSkImage.h
@@ -92,7 +92,11 @@ class JsiSkImage : public JsiSkWrappingSkPtrHostObject<SkImage> {
     image = DawnContext::getInstance().MakeRasterImage(image);
 #else
     if (image->isTextureBacked()) {
-      image = image->makeNonTextureImage();
+      auto grContext = getContext()->getDirectContext();
+      image = image->makeRasterImage(grContext);
+      if (!image) {
+        return nullptr;
+      }
     }
 #endif
     sk_sp<SkData> data;
@@ -121,6 +125,9 @@ class JsiSkImage : public JsiSkWrappingSkPtrHostObject<SkImage> {
 
   JSI_HOST_FUNCTION(encodeToBytes) {
     auto data = encodeImageData(arguments, count);
+    if (!data) {
+      return jsi::Value::null();
+    }
 
     auto arrayCtor =
         runtime.global().getPropertyAsFunction(runtime, "Uint8Array");
@@ -141,6 +148,9 @@ class JsiSkImage : public JsiSkWrappingSkPtrHostObject<SkImage> {
 
   JSI_HOST_FUNCTION(encodeToBase64) {
     auto data = encodeImageData(arguments, count);
+    if (!data) {
+      return jsi::Value::null();
+    }
 
     auto len = Base64::Encode(data->bytes(), data->size(), nullptr);
     auto buffer = std::string(len, 0);
@@ -182,18 +192,24 @@ class JsiSkImage : public JsiSkWrappingSkPtrHostObject<SkImage> {
             .asObject(runtime)
             .getArrayBuffer(runtime);
     auto bfrPtr = reinterpret_cast<void *>(buffer.data(runtime));
-
-    if (!getObject()->readPixels(info, bfrPtr, bytesPerRow, srcX, srcY)) {
+#if defined(SK_GRAPHITE)
+    throw std::runtime_error("Not implemented yet");
+#else
+    auto grContext = getContext()->getDirectContext();
+    if (!getObject()->readPixels(grContext, info, bfrPtr, bytesPerRow, srcX,
+                                 srcY)) {
       return jsi::Value::null();
     }
+#endif
     return dest;
   }
 
   JSI_HOST_FUNCTION(makeNonTextureImage) {
 #if defined(SK_GRAPHITE)
     auto rasterImage = DawnContext::getInstance().MakeRasterImage(getObject());
 #else
-    auto rasterImage = getObject()->makeNonTextureImage();
+    auto grContext = getContext()->getDirectContext();
+    auto rasterImage = getObject()->makeRasterImage(grContext);
 #endif
     return jsi::Object::createFromHostObject(
         runtime, std::make_shared<JsiSkImage>(getContext(), rasterImage));

diff --git a/packages/skia/cpp/rnskia/RNSkPlatformContext.h b/packages/skia/cpp/rnskia/RNSkPlatformContext.h
@@ -147,6 +147,10 @@ class RNSkPlatformContext {
    */
   virtual sk_sp<SkImage> makeImageFromNativeBuffer(void *buffer) = 0;
 
+#if !defined(SK_GRAPHITE)
+  virtual GrDirectContext *getDirectContext() = 0;
+#endif
+
   virtual void releaseNativeBuffer(uint64_t pointer) = 0;
 
   virtual uint64_t makeNativeBuffer(sk_sp<SkImage> image) = 0;

diff --git a/packages/skia/cpp/rnskia/RNSkView.h b/packages/skia/cpp/rnskia/RNSkView.h
@@ -90,7 +90,8 @@ class RNSkOffscreenCanvasProvider : public RNSkCanvasProvider {
   RNSkOffscreenCanvasProvider(std::shared_ptr<RNSkPlatformContext> context,
                               std::function<void()> requestRedraw, float width,
                               float height)
-      : RNSkCanvasProvider(requestRedraw), _width(width), _height(height) {
+      : RNSkCanvasProvider(requestRedraw), _context(context), _width(width),
+        _height(height) {
     _surface = context->makeOffscreenSurface(_width, _height);
     _pd = context->getPixelDensity();
   }
@@ -113,7 +114,8 @@ class RNSkOffscreenCanvasProvider : public RNSkCanvasProvider {
         _surface->recorder()->snap().get());
     return DawnContext::getInstance().MakeRasterImage(image);
 #else
-    return image->makeNonTextureImage();
+    auto grContext = _context->getDirectContext();
+    return image->makeRasterImage(grContext);
 #endif
   }
 
@@ -140,6 +142,7 @@ class RNSkOffscreenCanvasProvider : public RNSkCanvasProvider {
   float _height;
   float _pd = 1.0f;
   sk_sp<SkSurface> _surface;
+  std::shared_ptr<RNSkPlatformContext> _context;
 };
 
 enum RNSkDrawingMode { Default, Continuous };

diff --git a/packages/skia/ios/RNSkia-iOS/MetalContext.h b/packages/skia/ios/RNSkia-iOS/MetalContext.h
@@ -37,6 +37,8 @@ class MetalContext {
                                                 height);
   }
 
+  GrDirectContext *getDirectContext() { return _context.skContext.get(); }
+
 private:
   friend class RNSkia::RNSkiOSPlatformContext;
   id<MTLDevice> _device;

diff --git a/packages/skia/ios/RNSkia-iOS/RNSkiOSPlatformContext.h b/packages/skia/ios/RNSkia-iOS/RNSkiOSPlatformContext.h
@@ -76,6 +76,9 @@ class RNSkiOSPlatformContext : public RNSkPlatformContext {
 
   void raiseError(const std::exception &err) override;
   sk_sp<SkSurface> makeOffscreenSurface(int width, int height) override;
+#if !defined(SK_GRAPHITE)
+  GrDirectContext *getDirectContext() override;
+#endif
   sk_sp<SkFontMgr> createFontMgr() override;
 
   void willInvalidateModules() {

diff --git a/packages/skia/ios/RNSkia-iOS/RNSkiOSPlatformContext.mm b/packages/skia/ios/RNSkia-iOS/RNSkiOSPlatformContext.mm
@@ -192,6 +192,12 @@
 #endif
 }
 
+#if !defined(SK_GRAPHITE)
+GrDirectContext *RNSkiOSPlatformContext::getDirectContext() {
+  return MetalContext::getInstance().getDirectContext();
+}
+#endif
+
 sk_sp<SkFontMgr> RNSkiOSPlatformContext::createFontMgr() {
   return SkFontMgr_New_CoreText(nullptr);
 }

diff --git a/packages/skia/react-native-skia.podspec b/packages/skia/react-native-skia.podspec
@@ -9,8 +9,8 @@ use_graphite = ENV['SK_GRAPHITE'] == '1'
 
 # Set preprocessor definitions based on GRAPHITE flag
 preprocessor_defs = use_graphite ? 
-  '$(inherited) SK_GRAPHITE=1' : 
-  '$(inherited) SK_METAL=1 SK_GANESH=1'
+  '$(inherited) SK_GRAPHITE=1 SK_IMAGE_READ_PIXELS_DISABLE_LEGACY_API=1' : 
+  '$(inherited) SK_METAL=1 SK_GANESH=1 SK_IMAGE_READ_PIXELS_DISABLE_LEGACY_API=1'
 
 # Define base frameworks
 base_frameworks = ['libs/ios/libskia.xcframework', 

diff --git a/packages/skia/src/renderer/__tests__/e2e/Image.spec.tsx b/packages/skia/src/renderer/__tests__/e2e/Image.spec.tsx
@@ -43,51 +43,17 @@ describe("Image loading from bundles", () => {
       },
       {
         data: Array.from(
-          loadImage("skia/__tests__/assets/oslo.jpg").encodeToBytes()
+          loadImage("skia/__tests__/assets/oslo-mini.jpg").encodeToBytes()
         ),
       }
     );
     expect(pixels).toBeDefined();
     expect(pixels).toEqual([
-      170, 186, 199, 255, 170, 186, 199, 255, 170, 186, 199, 255, 170, 186, 199,
+      171, 188, 198, 255, 171, 188, 198, 255, 171, 188, 198, 255, 171, 188, 198,
       255,
     ]);
   });
 
-  // it("should read pixels from an image using a preallocated buffer", async () => {
-  //   const pixels = await surface.eval(
-  //     (Skia, { colorType, alphaType, data }) => {
-  //       const image = Skia.Image.MakeImageFromEncoded(
-  //         Skia.Data.fromBytes(new Uint8Array(data))
-  //       )!;
-  //       const result = new Uint8Array(16);
-  //       image.readPixels(
-  //         0,
-  //         0,
-  //         {
-  //           width: 2,
-  //           height: 2,
-  //           colorType,
-  //           alphaType,
-  //         },
-  //         result
-  //       );
-  //       return result;
-  //     },
-  //     {
-  //       colorType: ColorType.RGBA_8888,
-  //       alphaType: AlphaType.Unpremul,
-  //       data: Array.from(
-  //         loadImage("skia/__tests__/assets/oslo.jpg").encodeToBytes()
-  //       ),
-  //     }
-  //   );
-  //   expect(pixels).toBeDefined();
-  //   expect(Array.from(pixels!)).toEqual([
-  //     170, 186, 199, 255, 170, 186, 199, 255, 170, 186, 199, 255, 170, 186, 199,
-  //     255,
-  //   ]);
-  // });
   it("should read pixels from a canvas", async () => {
     const pixels = await surface.eval(
       (Skia, { colorType, alphaType }) => {
@@ -108,32 +74,4 @@ describe("Image loading from bundles", () => {
     expect(pixels).toBeDefined();
     expect(Array.from(pixels!)).toEqual([255, 0, 0, 255]);
   });
-  // it("should read pixels from a canvas using a preallocated buffer", async () => {
-  //   const pixels = await surface.eval(
-  //     (Skia, { colorType, alphaType }) => {
-  //       const offscreen = Skia.Surface.MakeOffscreen(10, 10)!;
-  //       const canvas = offscreen.getCanvas();
-  //       canvas.drawColor(Skia.Color("red"));
-  // const result = new Uint8Array(4);
-  //        canvas.readPixels(0, 0, {
-  //         width: 1,
-  //         height: 1,
-  //         colorType,
-  //         alphaType,
-  //       }, result);
-  //     },
-  //     { colorType: ColorType.RGBA_8888, alphaType: AlphaType.Unpremul }
-  //   );
-  //   expect(pixels).toBeDefined();
-  //   expect(Array.from(pixels!)).toEqual([255, 0, 0, 255]);
-  // });
-  // This test should only run on CI because it will trigger a redbox.
-  // While this is fine on CI, it is undesirable on local dev.
-  // it("should not crash with an invalid viewTag", async () => {
-  //   const result = await surface.eval((Skia) => {
-  //     Skia.Image.MakeImageFromViewTag(-1);
-  //     return true;
-  //   });
-  //   expect(result).toBe(true);
-  // });
 });
diff --git a/packages/skia/src/skia/__tests__/assets/oslo-mini.jpg b/packages/skia/src/skia/__tests__/assets/oslo-mini.jpg