From a9301c36d416b99e9c5740033e93b09db87299a1 Mon Sep 17 00:00:00 2001 From: chrfalch Date: Wed, 27 Apr 2022 14:41:34 +0200 Subject: [PATCH 01/15] Fix crash in release mode The problem (that was especially visible in Release mode) was a crash in the destructor of the RNSkDrawViewImpl on iOS where the CAMetalLayer was destructed twice in certain scenarios. The solution was to rewrite ownership of the RNSkDrawView so that we could make sure none of these objects were destroyed before all threads were done drawing etc. Each RNSkDrawView is now stored in a shared_ptr and captures are kept using weak_ptrs to ensure the lifetime of these objects. - Removed all std::bind that did not capture ownership correctly - Removed setNativeDrawFunc and converted to virtual function - Changed all references to RNSkDrawView to be shared_ptrs - Changed captures of RNSkDrawView in lambdas to use weak_ptrs - Changed caputers of RNSk***Value in lambdas to use weak_ptrs - Updated RNSkTimingInfo to handle skipping of frames correctly It now displays the time of the frame that made us skip the next frame. - Updated Android source code - Moved non-jni code to directory rnskia-android (alignment with iOS) - Fixed cleanup/dtor issues in Android - Added RNSkDrawView descendant RNSkDrawViewImpl to be able to store reference as std::shared_ptr (alignment with iOS) - iOS - Fixed cleanup issues, made sure CAMetalLayer is destroyed on the main UI Thread --- package/android/CMakeLists.txt | 4 +- package/android/cpp/jni/JniSkiaDrawView.cpp | 80 +------- package/android/cpp/jni/JniSkiaManager.cpp | 2 +- .../android/cpp/jni/include/JniSkiaDrawView.h | 38 ++-- .../cpp/rnskia-android/RNSkDrawViewImpl.cpp | 67 ++++++ .../cpp/rnskia-android/RNSkDrawViewImpl.h | 45 ++++ .../SkiaOpenGLRenderer.cpp | 125 ++++-------- .../SkiaOpenGLRenderer.h | 53 +---- .../reactnative/skia/RNSkiaViewManager.java | 1 - .../reactnative/skia/SkiaDrawView.java | 20 +- package/cpp/rnskia/RNSkDrawView.cpp | 193 +++++++----------- package/cpp/rnskia/RNSkDrawView.h | 40 +--- package/cpp/rnskia/RNSkJsiViewApi.h | 6 +- package/cpp/rnskia/RNSkManager.cpp | 4 +- package/cpp/rnskia/RNSkManager.h | 4 +- package/cpp/rnskia/RNSkPlatformContext.h | 2 +- package/cpp/rnskia/values/RNSkClockValue.h | 24 ++- package/cpp/rnskia/values/RNSkReadonlyValue.h | 16 +- package/cpp/utils/RNSkTimingInfo.h | 14 +- package/ios/RNSkia-iOS/RNSkDrawViewImpl.h | 12 +- package/ios/RNSkia-iOS/RNSkDrawViewImpl.mm | 35 +++- package/ios/RNSkia-iOS/SkiaDrawView.mm | 18 +- 22 files changed, 364 insertions(+), 439 deletions(-) create mode 100644 package/android/cpp/rnskia-android/RNSkDrawViewImpl.cpp create mode 100644 package/android/cpp/rnskia-android/RNSkDrawViewImpl.h rename package/android/cpp/{jni => rnskia-android}/SkiaOpenGLRenderer.cpp (64%) rename package/android/cpp/{jni/include => rnskia-android}/SkiaOpenGLRenderer.h (67%) diff --git a/package/android/CMakeLists.txt b/package/android/CMakeLists.txt index 3a0d711fe5..dc6ff5fb8c 100644 --- a/package/android/CMakeLists.txt +++ b/package/android/CMakeLists.txt @@ -31,7 +31,8 @@ add_library( "${PROJECT_SOURCE_DIR}/cpp/jni/JniSkiaManager.cpp" "${PROJECT_SOURCE_DIR}/cpp/jni/JniSkiaDrawView.cpp" "${PROJECT_SOURCE_DIR}/cpp/jni/JniPlatformContext.cpp" - "${PROJECT_SOURCE_DIR}/cpp/jni/SkiaOpenGLRenderer.cpp" + "${PROJECT_SOURCE_DIR}/cpp/rnskia-android/RNSkDrawViewImpl.cpp" + "${PROJECT_SOURCE_DIR}/cpp/rnskia-android/SkiaOpenGLRenderer.cpp" "${PROJECT_SOURCE_DIR}/cpp/jsi/JsiHostObject.cpp" @@ -64,6 +65,7 @@ target_include_directories( cpp/api cpp/jsi cpp/jni/include + cpp/rnskia-android cpp/rnskia cpp/rnskia/values cpp/utils diff --git a/package/android/cpp/jni/JniSkiaDrawView.cpp b/package/android/cpp/jni/JniSkiaDrawView.cpp index 8aea2fb9b7..c5ca50643e 100644 --- a/package/android/cpp/jni/JniSkiaDrawView.cpp +++ b/package/android/cpp/jni/JniSkiaDrawView.cpp @@ -39,8 +39,8 @@ namespace RNSkia /**** JNI ****/ TSelf JniSkiaDrawView::initHybrid( - alias_ref jThis, - JavaSkiaManager skiaManager) + alias_ref jThis, + JavaSkiaManager skiaManager) { return makeCxxInstance(jThis, skiaManager); } @@ -60,17 +60,17 @@ namespace RNSkia { if (mode.compare("continuous") == 0) { - setDrawingMode(RNSkDrawingMode::Continuous); + _drawView->setDrawingMode(RNSkDrawingMode::Continuous); } else { - setDrawingMode(RNSkDrawingMode::Default); + _drawView->setDrawingMode(RNSkDrawingMode::Default); } } void JniSkiaDrawView::setDebugMode(bool show) { - setShowDebugOverlays(show); + _drawView->setShowDebugOverlays(show); } void JniSkiaDrawView::updateTouchPoints(jni::JArrayDouble touches) @@ -78,7 +78,7 @@ namespace RNSkia // Create touch points std::vector points; auto pin = touches.pin(); - auto scale = getPlatformContext()->getPixelDensity(); + auto scale = _drawView->getPixelDensity(); points.reserve(pin.size() / 4); for (size_t i = 0; i < pin.size(); i += 4) { @@ -89,81 +89,21 @@ namespace RNSkia point.type = (RNSkia::RNSkTouchType)pin[i + 3]; points.push_back(point); } - updateTouchState(std::move(points)); + _drawView->updateTouchState(std::move(points)); } void JniSkiaDrawView::surfaceAvailable(jobject surface, int width, int height) { -#if LOG_ALL_DRAWING - RNSkLogger::logToConsole("JniSkiaDrawView::surfaceAvailable %i", getNativeId()); -#endif - - _width = width; - _height = height; - - if (_renderer == nullptr) - { - // Create renderer! - _renderer = new SkiaOpenGLRenderer( - ANativeWindow_fromSurface(Environment::current(), surface), getNativeId()); - - // Set the draw function - setNativeDrawFunc(std::bind(&JniSkiaDrawView::drawFrame, this, std::placeholders::_1)); - - // Redraw - requestRedraw(); - } + _drawView->surfaceAvailable(ANativeWindow_fromSurface(Environment::current(), surface), width, height); } void JniSkiaDrawView::surfaceSizeChanged(int width, int height) { -#if LOG_ALL_DRAWING - RNSkLogger::logToConsole("JniSkiaDrawView::surfaceSizeChanged %i", getNativeId()); -#endif - - _width = width; - _height = height; - - // Redraw after size change - requestRedraw(); + _drawView->surfaceSizeChanged(width, height); } void JniSkiaDrawView::surfaceDestroyed() { -#if LOG_ALL_DRAWING - RNSkLogger::logToConsole("JniSkiaDrawView::surfaceDestroyed %i", getNativeId()); -#endif - if (_renderer != nullptr) - { - // Turn off drawing - setNativeDrawFunc(nullptr); - - // Start teardown - _renderer->teardown(); - - // Ask for a redraw to tear down the render pipeline. This - // needs to be done on the render thread since OpenGL demands - // same thread access for OpenGL contexts. - getPlatformContext()->runOnRenderThread([this]() - { - if(_renderer != nullptr) { - _renderer->run(nullptr, 0, 0); - } }); - - // Wait until the above render has finished. - _renderer->waitForTeardown(); - - // Delete renderer. All resources should be released during teardown. - delete _renderer; - _renderer = nullptr; - } - } - - /**** Render method ****/ - - void JniSkiaDrawView::drawFrame(const sk_sp picture) - { - // No need to check if the renderer is nullptr since we only get here if it is not. - _renderer->run(picture, _width, _height); + _drawView->surfaceDestroyed(); } } // namespace RNSkia diff --git a/package/android/cpp/jni/JniSkiaManager.cpp b/package/android/cpp/jni/JniSkiaManager.cpp index c9de050ceb..e2b590d5be 100644 --- a/package/android/cpp/jni/JniSkiaManager.cpp +++ b/package/android/cpp/jni/JniSkiaManager.cpp @@ -47,7 +47,7 @@ void JniSkiaManager::initializeRuntime() { } void JniSkiaManager::registerSkiaView(int viewTag, JniSkiaDrawView *skiaView) { - _skManager->registerSkiaDrawView(viewTag, skiaView); + _skManager->registerSkiaDrawView(viewTag, skiaView->getDrawViewImpl()); } void JniSkiaManager::unregisterSkiaView(int viewTag) { diff --git a/package/android/cpp/jni/include/JniSkiaDrawView.h b/package/android/cpp/jni/include/JniSkiaDrawView.h index f461d523d4..8cacab8294 100644 --- a/package/android/cpp/jni/include/JniSkiaDrawView.h +++ b/package/android/cpp/jni/include/JniSkiaDrawView.h @@ -5,17 +5,16 @@ #include #include -#include -#include #include #include #include #include #include -#include "JniSkiaManager.h" -#include "JniSkiaDrawView.h" -#include "SkiaOpenGLRenderer.h" +#include +#include + +#include #include #include @@ -31,16 +30,15 @@ namespace RNSkia using JavaSkiaManager = jni::alias_ref; - class JniSkiaDrawView : public jni::HybridClass, - public RNSkDrawView + class JniSkiaDrawView : public jni::HybridClass { public: static auto constexpr kJavaDescriptor = "Lcom/shopify/reactnative/skia/SkiaDrawView;"; static auto constexpr TAG = "ReactNativeSkia"; static jni::local_ref initHybrid( - jni::alias_ref, - JavaSkiaManager); + jni::alias_ref, + JavaSkiaManager); static void registerNatives(); @@ -52,30 +50,26 @@ namespace RNSkia ~JniSkiaDrawView(); - protected: - int getWidth() override { return _width; } - int getHeight() override { return _height; } + std::shared_ptr getDrawViewImpl() { + return _drawView; + } + protected: void setMode(std::string mode); void setDebugMode(bool show); private: friend HybridBase; - void drawFrame(const sk_sp picture); - - int _width = 0; - int _height = 0; - - SkiaOpenGLRenderer* _renderer = nullptr; + std::shared_ptr _drawView; jni::global_ref javaPart_; explicit JniSkiaDrawView( - jni::alias_ref jThis, - JavaSkiaManager skiaManager) - : javaPart_(jni::make_global(jThis)), - RNSkDrawView(skiaManager->cthis()->getPlatformContext()) { + jni::alias_ref jThis, + JavaSkiaManager skiaManager) + : javaPart_(jni::make_global(jThis)), + _drawView(std::make_shared(skiaManager->cthis()->getPlatformContext())) { } }; diff --git a/package/android/cpp/rnskia-android/RNSkDrawViewImpl.cpp b/package/android/cpp/rnskia-android/RNSkDrawViewImpl.cpp new file mode 100644 index 0000000000..4a8d28733f --- /dev/null +++ b/package/android/cpp/rnskia-android/RNSkDrawViewImpl.cpp @@ -0,0 +1,67 @@ +#include + +#pragma clang diagnostic push +#pragma clang diagnostic ignored "-Wdocumentation" + +#include +#include + +#pragma clang diagnostic pop + +#include + +namespace RNSkia { + RNSkDrawViewImpl::RNSkDrawViewImpl(std::shared_ptr context) : + RNSkia::RNSkDrawView(context) { + } + + void RNSkDrawViewImpl::surfaceAvailable(ANativeWindow* surface, int width, int height) { + _width = width; + _height = height; + + if (_renderer == nullptr) + { + // Create renderer! + _renderer = std::make_shared(surface, getNativeId()); + + // Redraw + requestRedraw(); + } + } + + void RNSkDrawViewImpl::surfaceDestroyed() { + if (_renderer != nullptr) + { + // Start teardown + _renderer->teardown(); + + // Teardown renderer on the render thread since OpenGL demands + // same thread access for OpenGL contexts. + getPlatformContext()->runOnRenderThread([weakSelf = weak_from_this()]() { + auto self = weakSelf.lock(); + if(self) { + auto drawViewImpl = std::dynamic_pointer_cast(self); + if(drawViewImpl->_renderer != nullptr) { + drawViewImpl->_renderer->run(nullptr, 0, 0); + } + // Remove renderer + drawViewImpl->_renderer = nullptr; + } + }); + } + } + + void RNSkDrawViewImpl::surfaceSizeChanged(int width, int height) { + _width = width; + _height = height; + + // Redraw after size change + requestRedraw(); + } + + void RNSkDrawViewImpl::drawPicture(const sk_sp picture) { + if(_renderer != nullptr) { + _renderer->run(picture, _width, _height); + } + } +} diff --git a/package/android/cpp/rnskia-android/RNSkDrawViewImpl.h b/package/android/cpp/rnskia-android/RNSkDrawViewImpl.h new file mode 100644 index 0000000000..b841e64e99 --- /dev/null +++ b/package/android/cpp/rnskia-android/RNSkDrawViewImpl.h @@ -0,0 +1,45 @@ +#pragma once + +#include + +#include +#include + +#pragma clang diagnostic push +#pragma clang diagnostic ignored "-Wdocumentation" + +#include +#include + +#pragma clang diagnostic pop + +namespace RNSkia { + class RNSkDrawViewImpl : public RNSkia::RNSkDrawView { + public: + RNSkDrawViewImpl(std::shared_ptr context); + + void surfaceAvailable(ANativeWindow* surface, int, int); + void surfaceDestroyed(); + void surfaceSizeChanged(int, int); + + float getPixelDensity() { + return getPlatformContext()->getPixelDensity(); + } + + protected: + int getWidth() override { return _width * getPlatformContext()->getPixelDensity(); }; + + int getHeight() override { return _height * getPlatformContext()->getPixelDensity(); }; + + void drawPicture(const sk_sp picture) override; + + private: + bool createSkiaSurface(); + + std::shared_ptr _renderer = nullptr; + + int _nativeId; + int _width = -1; + int _height = -1; + }; +} diff --git a/package/android/cpp/jni/SkiaOpenGLRenderer.cpp b/package/android/cpp/rnskia-android/SkiaOpenGLRenderer.cpp similarity index 64% rename from package/android/cpp/jni/SkiaOpenGLRenderer.cpp rename to package/android/cpp/rnskia-android/SkiaOpenGLRenderer.cpp index a5fac939ce..307c1b1473 100644 --- a/package/android/cpp/jni/SkiaOpenGLRenderer.cpp +++ b/package/android/cpp/rnskia-android/SkiaOpenGLRenderer.cpp @@ -1,23 +1,17 @@ #include "SkiaOpenGLRenderer.h" #include + namespace RNSkia { - - /** Static members */ - std::shared_ptr SkiaOpenGLRenderer::getThreadDrawingContext() - { - auto threadId = std::this_thread::get_id(); - if (threadContexts.count(threadId) == 0) - { - auto drawingContext = std::make_shared(); - drawingContext->glContext = EGL_NO_CONTEXT; - drawingContext->glDisplay = EGL_NO_DISPLAY; - drawingContext->glConfig = 0; - drawingContext->skContext = nullptr; - threadContexts.emplace(threadId, drawingContext); - } - return threadContexts.at(threadId); + EGLContext SkiaOpenGLRenderer::glContext = EGL_NO_CONTEXT; + EGLDisplay SkiaOpenGLRenderer::glDisplay = EGL_NO_DISPLAY; + EGLConfig SkiaOpenGLRenderer::glConfig = 0; + sk_sp SkiaOpenGLRenderer::skContext = nullptr; + + SkiaOpenGLRenderer::SkiaOpenGLRenderer(ANativeWindow *surface, size_t renderId): + _surfaceTexture(surface), + _renderId(renderId) { } void SkiaOpenGLRenderer::run(const sk_sp picture, int width, int height) @@ -50,7 +44,7 @@ namespace RNSkia { // Reset Skia Context since it might be modified by another Skia View during // rendering. - getThreadDrawingContext()->skContext->resetContext(); + skContext->resetContext(); // Clear with transparent glClearColor(0.0f, 0.0f, 0.0f, 0.0f); @@ -61,9 +55,9 @@ namespace RNSkia // Flush _skSurface->getCanvas()->flush(); - getThreadDrawingContext()->skContext->flush(); + skContext->flush(); - if (!eglSwapBuffers(getThreadDrawingContext()->glDisplay, _glSurface)) + if (!eglSwapBuffers(glDisplay, _glSurface)) { RNSkLogger::logToConsole( "eglSwapBuffers failed: %d\n", eglGetError()); @@ -73,7 +67,16 @@ namespace RNSkia } case RenderState::Finishing: { - finish(); + _renderState = RenderState::Done; + + if (_glSurface != EGL_NO_SURFACE && glDisplay != EGL_NO_DISPLAY) + { + eglDestroySurface(glDisplay, _glSurface); + } + + _skSurface = nullptr; + _surfaceTexture = nullptr; + break; } case RenderState::Done: @@ -107,67 +110,20 @@ namespace RNSkia return true; } - void SkiaOpenGLRenderer::finish() - { - std::lock_guard lock(_lock); - - if (_renderState != RenderState::Finishing) - { - _cv.notify_all(); - return; - } - - finishGL(); - finishSkiaSurface(); - - _renderState = RenderState::Done; - - _cv.notify_one(); - } - - void SkiaOpenGLRenderer::finishGL() - { - if (_glSurface != EGL_NO_SURFACE && getThreadDrawingContext()->glDisplay != EGL_NO_DISPLAY) - { - eglDestroySurface(getThreadDrawingContext()->glDisplay, _glSurface); - } - } - - void SkiaOpenGLRenderer::finishSkiaSurface() - { - if (_skSurface != nullptr) - { - _skSurface = nullptr; - } - - if (_nativeWindow != nullptr) - { - ANativeWindow_release(_nativeWindow); - _nativeWindow = nullptr; - } - } - void SkiaOpenGLRenderer::teardown() { _renderState = RenderState::Finishing; } - void SkiaOpenGLRenderer::waitForTeardown() - { - std::unique_lock lock(_lock); - _cv.wait(lock, [this] - { return (_renderState == RenderState::Done); }); - } - bool SkiaOpenGLRenderer::initStaticGLContext() { - if (getThreadDrawingContext()->glContext != EGL_NO_CONTEXT) + if (glContext != EGL_NO_CONTEXT) { return true; } - getThreadDrawingContext()->glDisplay = eglGetDisplay(EGL_DEFAULT_DISPLAY); - if (getThreadDrawingContext()->glDisplay == EGL_NO_DISPLAY) + glDisplay = eglGetDisplay(EGL_DEFAULT_DISPLAY); + if (glDisplay == EGL_NO_DISPLAY) { RNSkLogger::logToConsole("eglGetdisplay failed : %i", glGetError()); return false; @@ -175,7 +131,7 @@ namespace RNSkia EGLint major; EGLint minor; - if (!eglInitialize(getThreadDrawingContext()->glDisplay, &major, &minor)) + if (!eglInitialize(glDisplay, &major, &minor)) { RNSkLogger::logToConsole("eglInitialize failed : %i", glGetError()); return false; @@ -201,8 +157,8 @@ namespace RNSkia EGL_NONE}; EGLint numConfigs; - getThreadDrawingContext()->glConfig = 0; - if (!eglChooseConfig(getThreadDrawingContext()->glDisplay, att, &getThreadDrawingContext()->glConfig, 1, &numConfigs) || + glConfig = 0; + if (!eglChooseConfig(glDisplay, att, &glConfig, 1, &numConfigs) || numConfigs == 0) { RNSkLogger::logToConsole( @@ -212,8 +168,9 @@ namespace RNSkia EGLint contextAttribs[] = {EGL_CONTEXT_CLIENT_VERSION, 2, EGL_NONE}; - getThreadDrawingContext()->glContext = eglCreateContext(getThreadDrawingContext()->glDisplay, getThreadDrawingContext()->glConfig, NULL, contextAttribs); - if (getThreadDrawingContext()->glContext == EGL_NO_CONTEXT) + glContext = eglCreateContext(glDisplay, glConfig, NULL, contextAttribs); + + if (glContext == EGL_NO_CONTEXT) { RNSkLogger::logToConsole( "eglCreateContext failed: %d\n", eglGetError()); @@ -225,15 +182,15 @@ namespace RNSkia bool SkiaOpenGLRenderer::initStaticSkiaContext() { - if (getThreadDrawingContext()->skContext != nullptr) + if (skContext != nullptr) { return true; } // Create the Skia backend context auto backendInterface = GrGLMakeNativeInterface(); - getThreadDrawingContext()->skContext = GrDirectContext::MakeGL(backendInterface); - if (getThreadDrawingContext()->skContext == nullptr) + skContext = GrDirectContext::MakeGL(backendInterface); + if (skContext == nullptr) { RNSkLogger::logToConsole("GrDirectContext::MakeGL failed"); return false; @@ -244,14 +201,14 @@ namespace RNSkia bool SkiaOpenGLRenderer::initGLSurface() { - if (_nativeWindow == nullptr) + if (_surfaceTexture == nullptr) { return false; } if (_glSurface != EGL_NO_SURFACE) { - if (!eglMakeCurrent(getThreadDrawingContext()->glDisplay, _glSurface, _glSurface, getThreadDrawingContext()->glContext)) + if (!eglMakeCurrent(glDisplay, _glSurface, _glSurface, glContext)) { RNSkLogger::logToConsole( "eglMakeCurrent failed: %d\n", eglGetError()); @@ -262,7 +219,7 @@ namespace RNSkia // Create the opengl surface _glSurface = - eglCreateWindowSurface(getThreadDrawingContext()->glDisplay, getThreadDrawingContext()->glConfig, _nativeWindow, nullptr); + eglCreateWindowSurface(glDisplay, glConfig, _surfaceTexture, nullptr); if (_glSurface == EGL_NO_SURFACE) { RNSkLogger::logToConsole( @@ -270,7 +227,7 @@ namespace RNSkia return false; } - if (!eglMakeCurrent(getThreadDrawingContext()->glDisplay, _glSurface, _glSurface, getThreadDrawingContext()->glContext)) + if (!eglMakeCurrent(glDisplay, _glSurface, _glSurface, glContext)) { RNSkLogger::logToConsole("eglMakeCurrent failed: %d\n", eglGetError()); return false; @@ -281,7 +238,7 @@ namespace RNSkia bool SkiaOpenGLRenderer::ensureSkiaSurface(int width, int height) { - if (getThreadDrawingContext()->skContext == nullptr) + if (skContext == nullptr) { return false; } @@ -305,7 +262,7 @@ namespace RNSkia GLint samples; glGetIntegerv(GL_SAMPLES, &samples); - auto maxSamples = getThreadDrawingContext()->skContext->maxSurfaceSampleCountForColorType( + auto maxSamples = skContext->maxSurfaceSampleCountForColorType( kRGBA_8888_SkColorType); if (samples > maxSamples) @@ -319,7 +276,7 @@ namespace RNSkia GrBackendRenderTarget(width, height, samples, stencil, fbInfo); _skSurface = SkSurface::MakeFromBackendRenderTarget( - getThreadDrawingContext()->skContext.get(), + skContext.get(), _skRenderTarget, kBottomLeft_GrSurfaceOrigin, kRGBA_8888_SkColorType, diff --git a/package/android/cpp/jni/include/SkiaOpenGLRenderer.h b/package/android/cpp/rnskia-android/SkiaOpenGLRenderer.h similarity index 67% rename from package/android/cpp/jni/include/SkiaOpenGLRenderer.h rename to package/android/cpp/rnskia-android/SkiaOpenGLRenderer.h index 4cda88a3a3..898ec01377 100644 --- a/package/android/cpp/jni/include/SkiaOpenGLRenderer.h +++ b/package/android/cpp/rnskia-android/SkiaOpenGLRenderer.h @@ -7,7 +7,6 @@ #include "GLES2/gl2.h" #include -#include #include #include @@ -25,16 +24,6 @@ namespace RNSkia { - using DrawingContext = struct - { - EGLContext glContext; - EGLDisplay glDisplay; - EGLConfig glConfig; - sk_sp skContext; - }; - - static std::unordered_map> threadContexts; - enum RenderState : int { Initializing, Rendering, @@ -45,9 +34,7 @@ namespace RNSkia class SkiaOpenGLRenderer { public: - SkiaOpenGLRenderer(ANativeWindow *nativeWindow, size_t renderId) : - _nativeWindow(nativeWindow), - _renderId(renderId) { } + SkiaOpenGLRenderer(ANativeWindow *surface, size_t renderId); /** * Initializes, renders and tears down the render pipeline depending on the state of the @@ -71,14 +58,6 @@ namespace RNSkia */ void teardown(); - /** - * Wait for teardown to finish. This means that we'll wait until the next - * render which will handle releasing all OpenGL and Skia resources used for - * this renderer. After tearing down the render will do nothing if the render - * method is called again. - */ - void waitForTeardown(); - private: /** * Initializes all required OpenGL and Skia objects @@ -117,31 +96,14 @@ namespace RNSkia */ bool ensureSkiaSurface(int width, int height); - /** - * Finalizes and releases all resources used by this renderer - */ - void finish(); - - /** - * Destroys the underlying OpenGL surface used for this renderer - */ - void finishGL(); - - /** - * Destroys the underlying Skia surface used for this renderer - */ - void finishSkiaSurface(); - - /** - * To be able to use static contexts (and avoid reloading the skia context for each - * new view, we track the OpenGL and Skia drawing context per thread. - * @return The drawing context for the current thread - */ - static std::shared_ptr getThreadDrawingContext(); + static EGLContext glContext; + static EGLDisplay glDisplay; + static EGLConfig glConfig; + static sk_sp skContext; EGLSurface _glSurface = EGL_NO_SURFACE; - ANativeWindow *_nativeWindow = nullptr; + ANativeWindow *_surfaceTexture = nullptr; GrBackendRenderTarget _skRenderTarget; sk_sp _skSurface; @@ -150,9 +112,6 @@ namespace RNSkia size_t _renderId; - std::mutex _lock; - std::condition_variable _cv; - std::atomic _renderState = { RenderState::Initializing }; }; diff --git a/package/android/src/main/java/com/shopify/reactnative/skia/RNSkiaViewManager.java b/package/android/src/main/java/com/shopify/reactnative/skia/RNSkiaViewManager.java index 6ed1bd76ea..cd290a6248 100644 --- a/package/android/src/main/java/com/shopify/reactnative/skia/RNSkiaViewManager.java +++ b/package/android/src/main/java/com/shopify/reactnative/skia/RNSkiaViewManager.java @@ -61,7 +61,6 @@ public void onDropViewInstance(@NonNull SkiaDrawView view) { Integer nativeId = mViewMapping.get(view); skiaModule.getSkiaManager().unregister(nativeId); mViewMapping.remove(view); - view.onRemoved(); } @NonNull diff --git a/package/android/src/main/java/com/shopify/reactnative/skia/SkiaDrawView.java b/package/android/src/main/java/com/shopify/reactnative/skia/SkiaDrawView.java index 7c57944b4e..3389f66e9b 100644 --- a/package/android/src/main/java/com/shopify/reactnative/skia/SkiaDrawView.java +++ b/package/android/src/main/java/com/shopify/reactnative/skia/SkiaDrawView.java @@ -39,18 +39,12 @@ public void setBackgroundColor(int color) { @Override protected void finalize() throws Throwable { mHybridData.resetNative(); + if(mSurface != null) { + mSurface.release(); + } super.finalize(); } - - public void onRemoved() { - // We'll mark the view removed since we reset the native part. - // This means that none of the native methods should be called after - // this point. - mIsRemoved = true; - mHybridData.resetNative(); - } - @Override public boolean onTouchEvent(MotionEvent ev) { if(mIsRemoved) { @@ -109,9 +103,11 @@ public boolean onSurfaceTextureDestroyed(SurfaceTexture surface) { return true; } surfaceDestroyed(); - mSurface.release(); - mSurface = null; - return true; + // https://developer.android.com/reference/android/view/TextureView.SurfaceTextureListener#onSurfaceTextureDestroyed(android.graphics.SurfaceTexture) + // Invoked when the specified SurfaceTexture is about to be destroyed. If returns true, + // no rendering should happen inside the surface texture after this method is invoked. + // If returns false, the client needs to call SurfaceTexture#release(). + return false; } @Override diff --git a/package/cpp/rnskia/RNSkDrawView.cpp b/package/cpp/rnskia/RNSkDrawView.cpp index a1f5c68691..3e04fa69ea 100644 --- a/package/cpp/rnskia/RNSkDrawView.cpp +++ b/package/cpp/rnskia/RNSkDrawView.cpp @@ -40,31 +40,13 @@ RNSkDrawView::RNSkDrawView(std::shared_ptr context) _platformContext(std::move(context)), _infoObject(std::make_shared()), _jsDrawingLock(std::make_shared()), - _gpuDrawingLock(std::make_shared()) + _gpuDrawingLock(std::make_shared()), + _jsTimingInfo("SKIA/JS"), + _gpuTimingInfo("SKIA/GPU") {} RNSkDrawView::~RNSkDrawView() { -#if LOG_ALL_DRAWING - RNSkLogger::logToConsole("RNSkDrawView::~RNSkDrawView - %i", getNativeId()); -#endif - - _isInvalidated = true; - endDrawingLoop(); - - // Wait for the drawing locks - if(!_jsDrawingLock->try_lock_for(1250ms)) { - RNSkLogger::logToConsole("Warning: JS drawing is still locked for native view with id %i", _nativeId); - } - - if(!_gpuDrawingLock->try_lock_for(1250ms)) { - RNSkLogger::logToConsole("Warning: SKIA drawing is still locked for native view with id %i", _nativeId); - } - - onInvalidated(); - - _jsDrawingLock = nullptr; - _gpuDrawingLock = nullptr; } void RNSkDrawView::setNativeId(size_t nativeId) { @@ -84,7 +66,6 @@ void RNSkDrawView::setDrawCallback(std::shared_ptr callback) { // Reset timing info _jsTimingInfo.reset(); _gpuTimingInfo.reset(); - _vsyncTimingInfo.reset(); // Set up debug font/paints auto font = SkFont(); @@ -94,53 +75,57 @@ void RNSkDrawView::setDrawCallback(std::shared_ptr callback) { // Create draw drawCallback wrapper _drawCallback = std::make_shared( - [this, paint = std::move(paint), font = std::move(font), callback = std::move(callback)](std::shared_ptr canvas, int width, - int height, double timestamp, - std::shared_ptr context) { - - auto runtime = context->getJsRuntime(); - - // Update info parameter - _infoObject->beginDrawOperation(width, height, timestamp); - - // Set up arguments array - std::vector args(2); - args[0] = jsi::Object::createFromHostObject(*runtime, canvas); - args[1] = jsi::Object::createFromHostObject(*runtime, _infoObject); - - // To be able to call the drawing function we'll wrap it once again - callback->call(*runtime, - static_cast(args.data()), - (size_t)2); - - // Reset touches - _infoObject->endDrawOperation(); - - // Draw debug overlays - if (_showDebugOverlay) { - - // Display average rendering timer - auto jsAvg = _jsTimingInfo.getAverage(); - //auto jsFps = _jsTimingInfo.getFps(); - - auto gpuAvg = _gpuTimingInfo.getAverage(); - //auto gpuFps = _gpuTimingInfo.getFps(); - - auto total = jsAvg + gpuAvg; - - //auto vsyncFps = _vsyncTimingInfo.getFps(); - - // Build string - std::ostringstream stream; - stream << "js: " << jsAvg << "ms gpu: " << gpuAvg << "ms " << " total: " << total << "ms"; -// stream << "js:" << jsAvg << "ms/" << jsFps << "fps " << "gpu:" << gpuAvg << "ms/" << -// gpuFps << "fps" << " total:" << total << "ms/" << vsyncFps << "fps"; - - std::string debugString = stream.str(); - - canvas->getCanvas()->drawSimpleText( - debugString.c_str(), debugString.size(), SkTextEncoding::kUTF8, 8, - 18, font, paint); + [weakSelf = weak_from_this(), + paint = std::move(paint), + font = std::move(font), + callback = std::move(callback)](std::shared_ptr canvas, + int width, + int height, + double timestamp, + std::shared_ptr context) { + + auto self = weakSelf.lock(); + if(self) { + auto runtime = context->getJsRuntime(); + + // Update info parameter + self->_infoObject->beginDrawOperation(width, height, timestamp); + + // Set up arguments array + std::vector args(2); + args[0] = jsi::Object::createFromHostObject(*runtime, canvas); + args[1] = jsi::Object::createFromHostObject(*runtime, self->_infoObject); + + // To be able to call the drawing function we'll wrap it once again + callback->call(*runtime, + static_cast(args.data()), + (size_t)2); + + // Reset touches + self->_infoObject->endDrawOperation(); + + // Draw debug overlays + if (self->_showDebugOverlay) { + + // Display average rendering timer + auto jsAvg = self->_jsTimingInfo.getAverage(); + //auto jsFps = _jsTimingInfo.getFps(); + + auto gpuAvg = self->_gpuTimingInfo.getAverage(); + //auto gpuFps = _gpuTimingInfo.getFps(); + + auto total = jsAvg + gpuAvg; + + // Build string + std::ostringstream stream; + stream << "js: " << jsAvg << "ms gpu: " << gpuAvg << "ms " << " total: " << total << "ms"; + + std::string debugString = stream.str(); + + canvas->getCanvas()->drawSimpleText( + debugString.c_str(), debugString.size(), SkTextEncoding::kUTF8, 8, + 18, font, paint); + } } }); @@ -196,10 +181,6 @@ void RNSkDrawView::updateTouchState(std::vector&& points) { } void RNSkDrawView::performDraw() { -#if LOG_ALL_DRAWING - RNSkLogger::logToConsole("RNSkDrawView::performDraw - %i", getNativeId()); -#endif - // Start timing _jsTimingInfo.beginTiming(); @@ -234,37 +215,22 @@ void RNSkDrawView::performDraw() { // Post drawing message to the render thread where the picture recorded // will be sent to the GPU/backend for rendering to screen. auto gpuLock = _gpuDrawingLock; - getPlatformContext()->runOnRenderThread([this, p = std::move(p), gpuLock]() { - - if(isInvalidated()) { - gpuLock->unlock(); - return; + _platformContext->runOnRenderThread([weakSelf = weak_from_this(), p = std::move(p), gpuLock]() { + auto self = weakSelf.lock(); + if (self) { + // Draw the picture recorded on the real GPU canvas + self->_gpuTimingInfo.beginTiming(); + self->drawPicture(p); + self->_gpuTimingInfo.stopTiming(); } - - _gpuTimingInfo.beginTiming(); - - // Draw the picture recorded on the real GPU canvas - if(_nativeDrawFunc != nullptr) { -#if LOG_ALL_DRAWING - RNSkLogger::logToConsole("RNSkDrawView::drawFrame - %i", getNativeId()); -#endif - _nativeDrawFunc(p); - } else { -#if LOG_ALL_DRAWING - RNSkLogger::logToConsole("RNSkDrawView::drawFrame - %i SKIPPING, draw func is null", getNativeId()); -#endif - } - - _gpuTimingInfo.stopTiming(); - // Unlock GPU drawing gpuLock->unlock(); }); } else { #ifdef DEBUG - static size_t framesSkipped = 0; - printf("SKIA/GPU: Skipped frames: %lu\n", ++framesSkipped); + _gpuTimingInfo.markSkipped(); #endif + // Request a new redraw since the last frame was skipped. requestRedraw(); } @@ -280,39 +246,34 @@ void RNSkDrawView::beginDrawingLoop() { if (_drawingLoopId != 0 || _nativeId == 0) { return; } - // Set to zero to avoid calling beginDrawLoop before we return - _drawingLoopId = _platformContext->beginDrawLoop( - _nativeId, std::bind(&RNSkDrawView::drawLoopCallback, this, std::placeholders::_1)); + _drawingLoopId = _platformContext->beginDrawLoop(_nativeId, + [weakSelf = weak_from_this()](bool invalidated) { + auto self = weakSelf.lock(); + if(self) { + self->drawLoopCallback(invalidated); + } + }); } void RNSkDrawView::drawLoopCallback(bool invalidated) { - if(invalidated) { - _isInvalidated = true; - onInvalidated(); -#if LOG_ALL_DRAWING - RNSkLogger::logToConsole("RNSkDrawView::onInvalidated - %i", getNativeId()); -#endif - return; - } - if(_redrawRequestCounter > 0 || _drawingMode == RNSkDrawingMode::Continuous) { _redrawRequestCounter = 0; - _vsyncTimingInfo.beginTiming(); - // We render on the javascript thread. if(_jsDrawingLock->try_lock()) { - _platformContext->runOnJavascriptThread(std::bind(&RNSkDrawView::performDraw, this)); + _platformContext->runOnJavascriptThread([weakSelf = weak_from_this()](){ + auto self = weakSelf.lock(); + if(self) { + self->performDraw(); + } + }); } else { #ifdef DEBUG - static size_t framesSkipped = 0; - printf("SKIA/JS: Skipped frames: %lu\n", ++framesSkipped); + _jsTimingInfo.markSkipped(); #endif requestRedraw(); } - - _vsyncTimingInfo.stopTiming(); } } diff --git a/package/cpp/rnskia/RNSkDrawView.h b/package/cpp/rnskia/RNSkDrawView.h index 3a7a10be7b..f1d6a0ffd6 100644 --- a/package/cpp/rnskia/RNSkDrawView.h +++ b/package/cpp/rnskia/RNSkDrawView.h @@ -20,8 +20,6 @@ #pragma clang diagnostic pop -#define LOG_ALL_DRAWING 0 - class SkPicture; class SkRect; class SkImage; @@ -35,7 +33,7 @@ using RNSkDrawCallback = enum RNSkDrawingMode { Default, Continuous }; -class RNSkDrawView { +class RNSkDrawView: public std::enable_shared_from_this { public: /** * Constructor @@ -45,7 +43,7 @@ class RNSkDrawView { /** Destructor */ - ~RNSkDrawView(); + virtual ~RNSkDrawView(); /** * Repaints the Skia view using the underlying context and the drawcallback. @@ -96,14 +94,6 @@ class RNSkDrawView { sk_sp makeImageSnapshot(std::shared_ptr bounds); protected: - void setNativeDrawFunc(std::function)> drawFunc) { - if(!_gpuDrawingLock->try_lock_for(250ms)) { - RNSkLogger::logToConsole("Could not lock drawing when clearing drawing function - %i", _nativeId); - } - _nativeDrawFunc = drawFunc; - _gpuDrawingLock->unlock(); - } - /** Returns the scaled width of the view */ @@ -115,14 +105,9 @@ class RNSkDrawView { virtual int getHeight() { return -1; }; /** - Returns true if the view is invalidated - */ - volatile bool isInvalidated() { return _isInvalidated; } - - /** - Override to be notified on invalidation + Override to render picture to GPU */ - virtual void onInvalidated() {} + virtual void drawPicture(const sk_sp picture) = 0; /** * @return The platformcontext @@ -210,12 +195,7 @@ class RNSkDrawView { /** Timing information for GPU rendering */ - RNSkTimingInfo _gpuTimingInfo; - - /** - Measures vsync framerate - */ - RNSkTimingInfo _vsyncTimingInfo; + RNSkTimingInfo _gpuTimingInfo; /** Redraw queue counter @@ -227,16 +207,6 @@ class RNSkDrawView { */ size_t _nativeId; - /** - Invalidation flag - */ - std::atomic _isInvalidated = { false }; - - /** - Native draw handler - */ - std::function)> _nativeDrawFunc; - }; } // namespace RNSkia diff --git a/package/cpp/rnskia/RNSkJsiViewApi.h b/package/cpp/rnskia/RNSkJsiViewApi.h index e85c4a2eca..506c6ced4f 100644 --- a/package/cpp/rnskia/RNSkJsiViewApi.h +++ b/package/cpp/rnskia/RNSkJsiViewApi.h @@ -20,7 +20,7 @@ using CallbackInfo = struct CallbackInfo { view = nullptr; } std::shared_ptr drawCallback; - RNSkDrawView *view; + std::shared_ptr view; }; class RNSkJsiViewApi : public JsiHostObject { @@ -226,7 +226,7 @@ class RNSkJsiViewApi : public JsiHostObject { * @param nativeId Id of view to register * @param view View to register */ - void registerSkiaDrawView(size_t nativeId, RNSkDrawView *view) { + void registerSkiaDrawView(size_t nativeId, std::shared_ptr view) { auto info = getEnsuredCallbackInfo(nativeId); info->view = view; if (info->drawCallback != nullptr) { @@ -260,7 +260,7 @@ class RNSkJsiViewApi : public JsiHostObject { a valid view is set, the setDrawCallback method is called on the view (if a valid callback exists). */ - void setSkiaDrawView(size_t nativeId, RNSkDrawView *view) { + void setSkiaDrawView(size_t nativeId, std::shared_ptr view) { if (_callbackInfos.find(nativeId) == _callbackInfos.end()) { return; } diff --git a/package/cpp/rnskia/RNSkManager.cpp b/package/cpp/rnskia/RNSkManager.cpp index ad2aa043e5..b273e05438 100644 --- a/package/cpp/rnskia/RNSkManager.cpp +++ b/package/cpp/rnskia/RNSkManager.cpp @@ -45,7 +45,7 @@ void RNSkManager::invalidate() { _platformContext->invalidate(); } -void RNSkManager::registerSkiaDrawView(size_t nativeId, RNSkDrawView *view) { +void RNSkManager::registerSkiaDrawView(size_t nativeId, std::shared_ptr view) { if (!_isInvalidated && _viewApi != nullptr) _viewApi->registerSkiaDrawView(nativeId, view); } @@ -55,7 +55,7 @@ void RNSkManager::unregisterSkiaDrawView(size_t nativeId) { _viewApi->unregisterSkiaDrawView(nativeId); } -void RNSkManager::setSkiaDrawView(size_t nativeId, RNSkDrawView *view) { +void RNSkManager::setSkiaDrawView(size_t nativeId, std::shared_ptr view) { if (!_isInvalidated && _viewApi != nullptr) _viewApi->setSkiaDrawView(nativeId, view); } diff --git a/package/cpp/rnskia/RNSkManager.h b/package/cpp/rnskia/RNSkManager.h index f033fb258b..8e86eeb65f 100644 --- a/package/cpp/rnskia/RNSkManager.h +++ b/package/cpp/rnskia/RNSkManager.h @@ -41,7 +41,7 @@ class RNSkManager { * @param nativeId Native view id * @param view View to register */ - void registerSkiaDrawView(size_t nativeId, RNSkDrawView *view); + void registerSkiaDrawView(size_t nativeId, std::shared_ptr view); /** * Unregisters the RNSkDrawView from the list of registered views @@ -54,7 +54,7 @@ class RNSkManager { Used when we want to remove a view without unregistering it - this happens typically on iOS. */ - void setSkiaDrawView(size_t nativeId, RNSkDrawView *view); + void setSkiaDrawView(size_t nativeId, std::shared_ptr view); /** * @return The platform context diff --git a/package/cpp/rnskia/RNSkPlatformContext.h b/package/cpp/rnskia/RNSkPlatformContext.h index d9d775a80b..c8b61df6b0 100644 --- a/package/cpp/rnskia/RNSkPlatformContext.h +++ b/package/cpp/rnskia/RNSkPlatformContext.h @@ -43,7 +43,7 @@ class RNSkPlatformContext { /** * Destructor */ - ~RNSkPlatformContext() { + virtual ~RNSkPlatformContext() { invalidate(); } diff --git a/package/cpp/rnskia/values/RNSkClockValue.h b/package/cpp/rnskia/values/RNSkClockValue.h index 3a1ca44688..3ed738f5ec 100644 --- a/package/cpp/rnskia/values/RNSkClockValue.h +++ b/package/cpp/rnskia/values/RNSkClockValue.h @@ -72,8 +72,12 @@ enum RNSkClockState { _start += timeSinceStop; _state = RNSkClockState::Running; - auto dispatch = std::bind(&RNSkClockValue::notifyUpdate, this, std::placeholders::_1); - getContext()->beginDrawLoop(_identifier, dispatch); + getContext()->beginDrawLoop(_identifier, [weakSelf = weak_from_this()](bool invalidated){ + auto self = weakSelf.lock(); + if(self) { + std::dynamic_pointer_cast(self)->notifyUpdate(invalidated); + } + }); } virtual void stopClock() { @@ -103,12 +107,16 @@ enum RNSkClockState { getContext()->runOnJavascriptThread( // To ensure that this shared_ptr instance is not deallocated before we are done // running the update lambda we pass a shared from this to the lambda scope. - [self = shared_from_this(), this]() { - if(_state == RNSkClockState::Running) { - auto now = std::chrono::high_resolution_clock::now(); - auto deltaFromStart = std::chrono::duration_cast(now - _start).count(); - tick(_runtime, static_cast(deltaFromStart)); - } + [weakSelf = weak_from_this()]() { + auto self = weakSelf.lock(); + if(self) { + auto selfClockValue = std::dynamic_pointer_cast(self); + if(selfClockValue->getState() == RNSkClockState::Running) { + auto now = std::chrono::high_resolution_clock::now(); + auto deltaFromStart = std::chrono::duration_cast(now - selfClockValue->_start).count(); + selfClockValue->tick(selfClockValue->_runtime, static_cast(deltaFromStart)); + } + } }); } diff --git a/package/cpp/rnskia/values/RNSkReadonlyValue.h b/package/cpp/rnskia/values/RNSkReadonlyValue.h index 80779814af..502bd27d32 100644 --- a/package/cpp/rnskia/values/RNSkReadonlyValue.h +++ b/package/cpp/rnskia/values/RNSkReadonlyValue.h @@ -48,10 +48,13 @@ class RNSkReadonlyValue : public JsiSkHostObject, } auto callback = std::make_shared(arguments[0].asObject(runtime).asFunction(runtime)); - auto unsubscribe = addListener([self = shared_from_this(), - this, + auto unsubscribe = addListener([weakSelf = weak_from_this(), callback = std::move(callback)](jsi::Runtime& runtime){ - callback->call(runtime, get_current(runtime)); + auto self = weakSelf.lock(); + if(self) { + auto selfReadonlyValue = std::dynamic_pointer_cast(self); + callback->call(runtime, selfReadonlyValue->get_current(runtime)); + } }); return jsi::Function::createFromHostFunction(runtime, @@ -74,8 +77,11 @@ class RNSkReadonlyValue : public JsiSkHostObject, std::lock_guard lock(_mutex); auto listenerId = _listenerId++; _listeners.emplace(listenerId, cb); - return [self = shared_from_this(), this, listenerId]() { - removeListener(listenerId); + return [weakSelf = weak_from_this(), listenerId]() { + auto self = weakSelf.lock(); + if(self) { + self->removeListener(listenerId); + } }; } diff --git a/package/cpp/utils/RNSkTimingInfo.h b/package/cpp/utils/RNSkTimingInfo.h index e36d86413a..ebd0a37ffb 100644 --- a/package/cpp/utils/RNSkTimingInfo.h +++ b/package/cpp/utils/RNSkTimingInfo.h @@ -1,5 +1,6 @@ #pragma once +#include #include #define NUMBER_OF_DURATION_SAMPLES 10 @@ -12,7 +13,7 @@ using ms = duration; class RNSkTimingInfo { public: - RNSkTimingInfo() { + RNSkTimingInfo(const std::string &name): _name(std::move(name)) { reset(); } @@ -25,6 +26,7 @@ class RNSkTimingInfo { _prevFpsTimer = -1; _frameCount = 0; _lastFrameCount = -1; + _didSkip = false; } void beginTiming() { @@ -35,6 +37,14 @@ class RNSkTimingInfo { time_point stop = high_resolution_clock::now(); addLastDuration(duration_cast(stop - _start).count()); tick(stop); + if(_didSkip) { + _didSkip = false; + RNSkLogger::logToConsole("%s: Skipped frame. Previous frame time: %lldms", _name.c_str(), _lastDuration); + } + } + + void markSkipped() { + _didSkip = true; } long getAverage() { return static_cast(_average); } @@ -86,6 +96,8 @@ class RNSkTimingInfo { long _prevFpsTimer; double _frameCount; double _lastFrameCount; + double _didSkip; + std::string _name; }; } // namespace RNSkia diff --git a/package/ios/RNSkia-iOS/RNSkDrawViewImpl.h b/package/ios/RNSkia-iOS/RNSkDrawViewImpl.h index 9a4aeed2d6..46179cde24 100644 --- a/package/ios/RNSkia-iOS/RNSkDrawViewImpl.h +++ b/package/ios/RNSkia-iOS/RNSkDrawViewImpl.h @@ -21,20 +21,18 @@ class RNSkDrawViewImpl : public RNSkia::RNSkDrawView { public: RNSkDrawViewImpl(std::shared_ptr context); + ~RNSkDrawViewImpl(); - void setSize(int width, int height); + CALayer* getLayer() { return _layer; } - CALayer* getLayer() { return _layer; }; + void setSize(int width, int height); protected: int getWidth() override { return _width * _context->getPixelDensity(); }; - int getHeight() override { return _height * _context->getPixelDensity(); }; - void onInvalidated() override { - setNativeDrawFunc(nullptr); - }; + int getHeight() override { return _height * _context->getPixelDensity(); }; private: - void drawFrame(const sk_sp picture); + void drawPicture(const sk_sp picture) override; bool createSkiaSurface(); int _nativeId; diff --git a/package/ios/RNSkia-iOS/RNSkDrawViewImpl.mm b/package/ios/RNSkia-iOS/RNSkDrawViewImpl.mm index 55774779d3..e88aa17aff 100644 --- a/package/ios/RNSkia-iOS/RNSkDrawViewImpl.mm +++ b/package/ios/RNSkia-iOS/RNSkDrawViewImpl.mm @@ -19,19 +19,34 @@ RNSkDrawViewImpl::RNSkDrawViewImpl(std::shared_ptr context): _context(context), RNSkia::RNSkDrawView(context) { - #pragma clang diagnostic push #pragma clang diagnostic ignored "-Wunguarded-availability-new" - _layer = [CAMetalLayer layer]; + _layer = [CAMetalLayer layer]; #pragma clang diagnostic pop - _layer.framebufferOnly = NO; - _layer.device = _device; - _layer.opaque = false; - _layer.contentsScale = _context->getPixelDensity(); - _layer.pixelFormat = MTLPixelFormatBGRA8Unorm; - - setNativeDrawFunc(std::bind(&RNSkDrawViewImpl::drawFrame, this, std::placeholders::_1)); + _layer.framebufferOnly = NO; + _layer.device = _device; + _layer.opaque = false; + _layer.contentsScale = _context->getPixelDensity(); + _layer.pixelFormat = MTLPixelFormatBGRA8Unorm; +} + +RNSkDrawViewImpl::~RNSkDrawViewImpl() { + if([[NSThread currentThread] isMainThread]) { + _layer = NULL; + } else { + __block auto tempLayer = _layer; + dispatch_async(dispatch_get_main_queue(), ^{ + // By using the tempLayer variable in the block we capture it and it will be + // released after the block has finished. This way the CAMetalLayer dealloc will + // only be called on the main thread. Problem: this destructor might be called from + // releasing the RNSkDrawViewImpl from a thread capture (after dtor has started), + // which would cause the CAMetalLayer dealloc to be called on another thread which + // causes a crash. + // https://github.com/Shopify/react-native-skia/issues/398 + tempLayer = tempLayer; + }); + } } void RNSkDrawViewImpl::setSize(int width, int height) { @@ -44,7 +59,7 @@ requestRedraw(); } -void RNSkDrawViewImpl::drawFrame(const sk_sp picture) { +void RNSkDrawViewImpl::drawPicture(const sk_sp picture) { if(_width == -1 && _height == -1) { return; } diff --git a/package/ios/RNSkia-iOS/SkiaDrawView.mm b/package/ios/RNSkia-iOS/SkiaDrawView.mm index 00aa0adf1a..da963132ab 100644 --- a/package/ios/RNSkia-iOS/SkiaDrawView.mm +++ b/package/ios/RNSkia-iOS/SkiaDrawView.mm @@ -39,12 +39,6 @@ - (instancetype) initWithManager: (RNSkia::RNSkManager*)manager; return self; } -- (void)dealloc { - if(_manager != nullptr) { - _manager->unregisterSkiaDrawView(_nativeId); - } -} - #pragma mark Lifecycle - (void) willMoveToWindow:(UIWindow *)newWindow { @@ -53,19 +47,21 @@ - (void) willMoveToWindow:(UIWindow *)newWindow { if (newWindow == NULL) { // Remove implementation view when the parent view is not set if(_impl != nullptr) { + [_impl->getLayer() removeFromSuperlayer]; + if(_nativeId != 0 && _manager != nullptr) { - _manager->setSkiaDrawView(_nativeId, nullptr); + _manager->unregisterSkiaDrawView(_nativeId); } - [_impl->getLayer() removeFromSuperlayer]; + _impl = nullptr; } } else { // Create implementation view when the parent view is set if(_impl == nullptr && _manager != nullptr) { _impl = std::make_shared(_manager->getPlatformContext()); - [self.layer addSublayer:_impl->getLayer()]; + [self.layer addSublayer: _impl->getLayer()]; if(_nativeId != 0) { - _manager->setSkiaDrawView(_nativeId, _impl.get()); + _manager->setSkiaDrawView(_nativeId, _impl); } _impl->setDrawingMode(_drawingMode); _impl->setShowDebugOverlays(_debugMode); @@ -103,7 +99,7 @@ - (void) setNativeId:(size_t) nativeId { _nativeId = nativeId; if(_impl != nullptr) { - _manager->registerSkiaDrawView(nativeId, _impl.get()); + _manager->registerSkiaDrawView(nativeId, _impl); } } From a1d9e9bd0f72c5bd8e3d0bf2518b47a7bb8af5f0 Mon Sep 17 00:00:00 2001 From: chrfalch Date: Wed, 27 Apr 2022 18:31:17 +0200 Subject: [PATCH 02/15] Resolved issue with removing the view in view api correctly Previously we just removed the view, now we make sure to remove the drawCallback in the view to deallocate the view. --- package/cpp/rnskia/RNSkJsiViewApi.h | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/package/cpp/rnskia/RNSkJsiViewApi.h b/package/cpp/rnskia/RNSkJsiViewApi.h index 506c6ced4f..ba2df310df 100644 --- a/package/cpp/rnskia/RNSkJsiViewApi.h +++ b/package/cpp/rnskia/RNSkJsiViewApi.h @@ -265,10 +265,13 @@ class RNSkJsiViewApi : public JsiHostObject { return; } auto info = getEnsuredCallbackInfo(nativeId); - info->view = view; if (view != nullptr && info->drawCallback != nullptr) { + info->view = view; info->view->setNativeId(nativeId); info->view->setDrawCallback(info->drawCallback); + } else if(view == nullptr && info->drawCallback != nullptr) { + info->view->setDrawCallback(nullptr); + info->view = view; } } From fe8ee664ec87e322a65f015d368d2171195a94be Mon Sep 17 00:00:00 2001 From: chrfalch Date: Wed, 27 Apr 2022 18:32:42 +0200 Subject: [PATCH 03/15] Fixed issue with mount/unmount Skia views. A fix was added to the PR that unregistered the SkiaView when it was unmounted on navigation. We now go back to just set it to nil to free the draw view. We also use a weakPtr to subscribe to notifications, and I also readded back dealloc with unregistration of the skia view. Also added unsubscribing to the notification in dealloc. --- package/ios/RNSkia-iOS/SkiaDrawView.mm | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/package/ios/RNSkia-iOS/SkiaDrawView.mm b/package/ios/RNSkia-iOS/SkiaDrawView.mm index da963132ab..1aba02bf99 100644 --- a/package/ios/RNSkia-iOS/SkiaDrawView.mm +++ b/package/ios/RNSkia-iOS/SkiaDrawView.mm @@ -26,14 +26,18 @@ - (instancetype) initWithManager: (RNSkia::RNSkManager*)manager; _nativeId = 0; _debugMode = false; _drawingMode = RNSkia::RNSkDrawingMode::Default; + // Listen to notifications about module invalidation + __unsafe_unretained SkiaDrawView *weakSelf = self; auto nc = [NSNotificationCenter defaultCenter]; [nc addObserverForName:RCTBridgeWillInvalidateModulesNotification object:nil queue:nil usingBlock:^(NSNotification *notification){ // Remove local variables - self->_manager = nullptr; + if(weakSelf != nullptr) { + weakSelf->_manager = nullptr; + } }]; } return self; @@ -41,16 +45,14 @@ - (instancetype) initWithManager: (RNSkia::RNSkManager*)manager; #pragma mark Lifecycle -- (void) willMoveToWindow:(UIWindow *)newWindow { - [super willMoveToWindow: newWindow]; - +- (void) willMoveToSuperview:(UIView *)newWindow { if (newWindow == NULL) { // Remove implementation view when the parent view is not set if(_impl != nullptr) { [_impl->getLayer() removeFromSuperlayer]; if(_nativeId != 0 && _manager != nullptr) { - _manager->unregisterSkiaDrawView(_nativeId); + _manager->setSkiaDrawView(_nativeId, nullptr); } _impl = nullptr; @@ -64,11 +66,19 @@ - (void) willMoveToWindow:(UIWindow *)newWindow { _manager->setSkiaDrawView(_nativeId, _impl); } _impl->setDrawingMode(_drawingMode); - _impl->setShowDebugOverlays(_debugMode); + _impl->setShowDebugOverlays(_debugMode); } } } +- (void) dealloc { + auto nc = [NSNotificationCenter defaultCenter]; + [nc removeObserver:self name:RCTBridgeWillInvalidateModulesNotification object:nil]; + if(_manager != nullptr && _nativeId != 0) { + _manager->unregisterSkiaDrawView(_nativeId); + } +} + #pragma mark Layout - (void) layoutSubviews { From 65a9f7e1312386ac4bcaad3bc457fbb9abfb0777 Mon Sep 17 00:00:00 2001 From: chrfalch Date: Wed, 27 Apr 2022 20:38:09 +0200 Subject: [PATCH 04/15] Reverted removal of onRemove to do cleanup. --- .../com/shopify/reactnative/skia/RNSkiaViewManager.java | 1 + .../java/com/shopify/reactnative/skia/SkiaDrawView.java | 8 ++++---- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/package/android/src/main/java/com/shopify/reactnative/skia/RNSkiaViewManager.java b/package/android/src/main/java/com/shopify/reactnative/skia/RNSkiaViewManager.java index cd290a6248..6ed1bd76ea 100644 --- a/package/android/src/main/java/com/shopify/reactnative/skia/RNSkiaViewManager.java +++ b/package/android/src/main/java/com/shopify/reactnative/skia/RNSkiaViewManager.java @@ -61,6 +61,7 @@ public void onDropViewInstance(@NonNull SkiaDrawView view) { Integer nativeId = mViewMapping.get(view); skiaModule.getSkiaManager().unregister(nativeId); mViewMapping.remove(view); + view.onRemoved(); } @NonNull diff --git a/package/android/src/main/java/com/shopify/reactnative/skia/SkiaDrawView.java b/package/android/src/main/java/com/shopify/reactnative/skia/SkiaDrawView.java index 3389f66e9b..6d192980b9 100644 --- a/package/android/src/main/java/com/shopify/reactnative/skia/SkiaDrawView.java +++ b/package/android/src/main/java/com/shopify/reactnative/skia/SkiaDrawView.java @@ -23,6 +23,7 @@ public class SkiaDrawView extends TextureView implements TextureView.SurfaceText @DoNotStrip private boolean mIsRemoved = false; + public SkiaDrawView(Context ctx) { super(ctx); RNSkiaModule skiaModule = ((ReactContext)ctx).getNativeModule(RNSkiaModule.class); @@ -36,13 +37,12 @@ public void setBackgroundColor(int color) { // Texture view does not support setting the background color. } - @Override - protected void finalize() throws Throwable { + public void onRemoved() { + mIsRemoved = true; mHybridData.resetNative(); if(mSurface != null) { mSurface.release(); } - super.finalize(); } @Override @@ -100,7 +100,7 @@ public void onSurfaceTextureSizeChanged(SurfaceTexture surface, int width, int h @Override public boolean onSurfaceTextureDestroyed(SurfaceTexture surface) { if(mIsRemoved) { - return true; + return false; } surfaceDestroyed(); // https://developer.android.com/reference/android/view/TextureView.SurfaceTextureListener#onSurfaceTextureDestroyed(android.graphics.SurfaceTexture) From c143c9b0b61232e92ebae824486be72aea20aa05 Mon Sep 17 00:00:00 2001 From: chrfalch Date: Wed, 27 Apr 2022 20:38:55 +0200 Subject: [PATCH 05/15] Reverted removal of storing drawing contexts pr thread This change made the OpenGL context not be thread safe when reloading in debug mode. --- .../cpp/rnskia-android/SkiaOpenGLRenderer.cpp | 80 +++++++++++++------ .../cpp/rnskia-android/SkiaOpenGLRenderer.h | 20 ++++- 2 files changed, 70 insertions(+), 30 deletions(-) diff --git a/package/android/cpp/rnskia-android/SkiaOpenGLRenderer.cpp b/package/android/cpp/rnskia-android/SkiaOpenGLRenderer.cpp index 307c1b1473..97d148ecd4 100644 --- a/package/android/cpp/rnskia-android/SkiaOpenGLRenderer.cpp +++ b/package/android/cpp/rnskia-android/SkiaOpenGLRenderer.cpp @@ -4,10 +4,21 @@ namespace RNSkia { - EGLContext SkiaOpenGLRenderer::glContext = EGL_NO_CONTEXT; - EGLDisplay SkiaOpenGLRenderer::glDisplay = EGL_NO_DISPLAY; - EGLConfig SkiaOpenGLRenderer::glConfig = 0; - sk_sp SkiaOpenGLRenderer::skContext = nullptr; + /** Static members */ + std::shared_ptr SkiaOpenGLRenderer::getThreadDrawingContext() + { + auto threadId = std::this_thread::get_id(); + if (threadContexts.count(threadId) == 0) + { + auto drawingContext = std::make_shared(); + drawingContext->glContext = EGL_NO_CONTEXT; + drawingContext->glDisplay = EGL_NO_DISPLAY; + drawingContext->glConfig = 0; + drawingContext->skContext = nullptr; + threadContexts.emplace(threadId, drawingContext); + } + return threadContexts.at(threadId); + } SkiaOpenGLRenderer::SkiaOpenGLRenderer(ANativeWindow *surface, size_t renderId): _surfaceTexture(surface), @@ -44,7 +55,7 @@ namespace RNSkia { // Reset Skia Context since it might be modified by another Skia View during // rendering. - skContext->resetContext(); + getThreadDrawingContext()->skContext->resetContext(); // Clear with transparent glClearColor(0.0f, 0.0f, 0.0f, 0.0f); @@ -55,9 +66,9 @@ namespace RNSkia // Flush _skSurface->getCanvas()->flush(); - skContext->flush(); + getThreadDrawingContext()->skContext->flush(); - if (!eglSwapBuffers(glDisplay, _glSurface)) + if (!eglSwapBuffers(getThreadDrawingContext()->glDisplay, _glSurface)) { RNSkLogger::logToConsole( "eglSwapBuffers failed: %d\n", eglGetError()); @@ -69,9 +80,9 @@ namespace RNSkia { _renderState = RenderState::Done; - if (_glSurface != EGL_NO_SURFACE && glDisplay != EGL_NO_DISPLAY) + if (_glSurface != EGL_NO_SURFACE && getThreadDrawingContext()->glDisplay != EGL_NO_DISPLAY) { - eglDestroySurface(glDisplay, _glSurface); + eglDestroySurface(getThreadDrawingContext()->glDisplay, _glSurface); } _skSurface = nullptr; @@ -117,13 +128,13 @@ namespace RNSkia bool SkiaOpenGLRenderer::initStaticGLContext() { - if (glContext != EGL_NO_CONTEXT) + if (getThreadDrawingContext()->glContext != EGL_NO_CONTEXT) { return true; } - glDisplay = eglGetDisplay(EGL_DEFAULT_DISPLAY); - if (glDisplay == EGL_NO_DISPLAY) + getThreadDrawingContext()->glDisplay = eglGetDisplay(EGL_DEFAULT_DISPLAY); + if (getThreadDrawingContext()->glDisplay == EGL_NO_DISPLAY) { RNSkLogger::logToConsole("eglGetdisplay failed : %i", glGetError()); return false; @@ -131,7 +142,7 @@ namespace RNSkia EGLint major; EGLint minor; - if (!eglInitialize(glDisplay, &major, &minor)) + if (!eglInitialize(getThreadDrawingContext()->glDisplay, &major, &minor)) { RNSkLogger::logToConsole("eglInitialize failed : %i", glGetError()); return false; @@ -157,8 +168,8 @@ namespace RNSkia EGL_NONE}; EGLint numConfigs; - glConfig = 0; - if (!eglChooseConfig(glDisplay, att, &glConfig, 1, &numConfigs) || + getThreadDrawingContext()->glConfig = 0; + if (!eglChooseConfig(getThreadDrawingContext()->glDisplay, att, &getThreadDrawingContext()->glConfig, 1, &numConfigs) || numConfigs == 0) { RNSkLogger::logToConsole( @@ -168,9 +179,13 @@ namespace RNSkia EGLint contextAttribs[] = {EGL_CONTEXT_CLIENT_VERSION, 2, EGL_NONE}; - glContext = eglCreateContext(glDisplay, glConfig, NULL, contextAttribs); + getThreadDrawingContext()->glContext = eglCreateContext( + getThreadDrawingContext()->glDisplay, + getThreadDrawingContext()->glConfig, + NULL, + contextAttribs); - if (glContext == EGL_NO_CONTEXT) + if (getThreadDrawingContext()->glContext == EGL_NO_CONTEXT) { RNSkLogger::logToConsole( "eglCreateContext failed: %d\n", eglGetError()); @@ -182,15 +197,15 @@ namespace RNSkia bool SkiaOpenGLRenderer::initStaticSkiaContext() { - if (skContext != nullptr) + if (getThreadDrawingContext()->skContext != nullptr) { return true; } // Create the Skia backend context auto backendInterface = GrGLMakeNativeInterface(); - skContext = GrDirectContext::MakeGL(backendInterface); - if (skContext == nullptr) + getThreadDrawingContext()->skContext = GrDirectContext::MakeGL(backendInterface); + if (getThreadDrawingContext()->skContext == nullptr) { RNSkLogger::logToConsole("GrDirectContext::MakeGL failed"); return false; @@ -208,7 +223,11 @@ namespace RNSkia if (_glSurface != EGL_NO_SURFACE) { - if (!eglMakeCurrent(glDisplay, _glSurface, _glSurface, glContext)) + if (!eglMakeCurrent( + getThreadDrawingContext()->glDisplay, + _glSurface, + _glSurface, + getThreadDrawingContext()->glContext)) { RNSkLogger::logToConsole( "eglMakeCurrent failed: %d\n", eglGetError()); @@ -219,7 +238,12 @@ namespace RNSkia // Create the opengl surface _glSurface = - eglCreateWindowSurface(glDisplay, glConfig, _surfaceTexture, nullptr); + eglCreateWindowSurface( + getThreadDrawingContext()->glDisplay, + getThreadDrawingContext()->glConfig, + _surfaceTexture, + nullptr); + if (_glSurface == EGL_NO_SURFACE) { RNSkLogger::logToConsole( @@ -227,7 +251,11 @@ namespace RNSkia return false; } - if (!eglMakeCurrent(glDisplay, _glSurface, _glSurface, glContext)) + if (!eglMakeCurrent( + getThreadDrawingContext()->glDisplay, + _glSurface, + _glSurface, + getThreadDrawingContext()->glContext)) { RNSkLogger::logToConsole("eglMakeCurrent failed: %d\n", eglGetError()); return false; @@ -238,7 +266,7 @@ namespace RNSkia bool SkiaOpenGLRenderer::ensureSkiaSurface(int width, int height) { - if (skContext == nullptr) + if (getThreadDrawingContext()->skContext == nullptr) { return false; } @@ -262,7 +290,7 @@ namespace RNSkia GLint samples; glGetIntegerv(GL_SAMPLES, &samples); - auto maxSamples = skContext->maxSurfaceSampleCountForColorType( + auto maxSamples = getThreadDrawingContext()->skContext->maxSurfaceSampleCountForColorType( kRGBA_8888_SkColorType); if (samples > maxSamples) @@ -276,7 +304,7 @@ namespace RNSkia GrBackendRenderTarget(width, height, samples, stencil, fbInfo); _skSurface = SkSurface::MakeFromBackendRenderTarget( - skContext.get(), + getThreadDrawingContext()->skContext.get(), _skRenderTarget, kBottomLeft_GrSurfaceOrigin, kRGBA_8888_SkColorType, diff --git a/package/android/cpp/rnskia-android/SkiaOpenGLRenderer.h b/package/android/cpp/rnskia-android/SkiaOpenGLRenderer.h index 898ec01377..e4fbe4bd0d 100644 --- a/package/android/cpp/rnskia-android/SkiaOpenGLRenderer.h +++ b/package/android/cpp/rnskia-android/SkiaOpenGLRenderer.h @@ -24,6 +24,16 @@ namespace RNSkia { + using DrawingContext = struct + { + EGLContext glContext; + EGLDisplay glDisplay; + EGLConfig glConfig; + sk_sp skContext; + }; + + static std::unordered_map> threadContexts; + enum RenderState : int { Initializing, Rendering, @@ -96,10 +106,12 @@ namespace RNSkia */ bool ensureSkiaSurface(int width, int height); - static EGLContext glContext; - static EGLDisplay glDisplay; - static EGLConfig glConfig; - static sk_sp skContext; + /** + * To be able to use static contexts (and avoid reloading the skia context for each + * new view, we track the OpenGL and Skia drawing context per thread. + * @return The drawing context for the current thread + */ + static std::shared_ptr getThreadDrawingContext(); EGLSurface _glSurface = EGL_NO_SURFACE; From aad82034f95d40d109657bc10355981db8fb42fd Mon Sep 17 00:00:00 2001 From: chrfalch Date: Wed, 27 Apr 2022 21:13:27 +0200 Subject: [PATCH 06/15] Changed the renderer member to unique_ptr --- package/android/cpp/rnskia-android/RNSkDrawViewImpl.cpp | 2 +- package/android/cpp/rnskia-android/RNSkDrawViewImpl.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/package/android/cpp/rnskia-android/RNSkDrawViewImpl.cpp b/package/android/cpp/rnskia-android/RNSkDrawViewImpl.cpp index 4a8d28733f..31b2257dfc 100644 --- a/package/android/cpp/rnskia-android/RNSkDrawViewImpl.cpp +++ b/package/android/cpp/rnskia-android/RNSkDrawViewImpl.cpp @@ -22,7 +22,7 @@ namespace RNSkia { if (_renderer == nullptr) { // Create renderer! - _renderer = std::make_shared(surface, getNativeId()); + _renderer = std::make_unique(surface, getNativeId()); // Redraw requestRedraw(); diff --git a/package/android/cpp/rnskia-android/RNSkDrawViewImpl.h b/package/android/cpp/rnskia-android/RNSkDrawViewImpl.h index b841e64e99..c0eb1c1aaa 100644 --- a/package/android/cpp/rnskia-android/RNSkDrawViewImpl.h +++ b/package/android/cpp/rnskia-android/RNSkDrawViewImpl.h @@ -36,7 +36,7 @@ namespace RNSkia { private: bool createSkiaSurface(); - std::shared_ptr _renderer = nullptr; + std::unique_ptr _renderer = nullptr; int _nativeId; int _width = -1; From 726380389045cbaeaa5483a096922ea779ea41d3 Mon Sep 17 00:00:00 2001 From: chrfalch Date: Wed, 27 Apr 2022 14:41:34 +0200 Subject: [PATCH 07/15] Fix crash in release mode The problem (that was especially visible in Release mode) was a crash in the destructor of the RNSkDrawViewImpl on iOS where the CAMetalLayer was destructed twice in certain scenarios. The solution was to rewrite ownership of the RNSkDrawView so that we could make sure none of these objects were destroyed before all threads were done drawing etc. Each RNSkDrawView is now stored in a shared_ptr and captures are kept using weak_ptrs to ensure the lifetime of these objects. - Removed all std::bind that did not capture ownership correctly - Removed setNativeDrawFunc and converted to virtual function - Changed all references to RNSkDrawView to be shared_ptrs - Changed captures of RNSkDrawView in lambdas to use weak_ptrs - Changed caputers of RNSk***Value in lambdas to use weak_ptrs - Updated RNSkTimingInfo to handle skipping of frames correctly It now displays the time of the frame that made us skip the next frame. - Updated Android source code - Moved non-jni code to directory rnskia-android (alignment with iOS) - Fixed cleanup/dtor issues in Android - Added RNSkDrawView descendant RNSkDrawViewImpl to be able to store reference as std::shared_ptr (alignment with iOS) - iOS - Fixed cleanup issues, made sure CAMetalLayer is destroyed on the main UI Thread --- package/android/CMakeLists.txt | 4 +- package/android/cpp/jni/JniSkiaDrawView.cpp | 80 +------- package/android/cpp/jni/JniSkiaManager.cpp | 2 +- .../android/cpp/jni/include/JniSkiaDrawView.h | 38 ++-- .../cpp/rnskia-android/RNSkDrawViewImpl.cpp | 67 ++++++ .../cpp/rnskia-android/RNSkDrawViewImpl.h | 45 ++++ .../SkiaOpenGLRenderer.cpp | 125 ++++-------- .../SkiaOpenGLRenderer.h | 53 +---- .../reactnative/skia/RNSkiaViewManager.java | 1 - .../reactnative/skia/SkiaDrawView.java | 20 +- package/cpp/rnskia/RNSkDrawView.cpp | 193 +++++++----------- package/cpp/rnskia/RNSkDrawView.h | 40 +--- package/cpp/rnskia/RNSkJsiViewApi.h | 6 +- package/cpp/rnskia/RNSkManager.cpp | 4 +- package/cpp/rnskia/RNSkManager.h | 4 +- package/cpp/rnskia/RNSkPlatformContext.h | 2 +- package/cpp/rnskia/values/RNSkClockValue.h | 24 ++- package/cpp/rnskia/values/RNSkReadonlyValue.h | 16 +- package/cpp/utils/RNSkTimingInfo.h | 14 +- package/ios/RNSkia-iOS/RNSkDrawViewImpl.h | 12 +- package/ios/RNSkia-iOS/RNSkDrawViewImpl.mm | 35 +++- package/ios/RNSkia-iOS/SkiaDrawView.mm | 18 +- 22 files changed, 364 insertions(+), 439 deletions(-) create mode 100644 package/android/cpp/rnskia-android/RNSkDrawViewImpl.cpp create mode 100644 package/android/cpp/rnskia-android/RNSkDrawViewImpl.h rename package/android/cpp/{jni => rnskia-android}/SkiaOpenGLRenderer.cpp (64%) rename package/android/cpp/{jni/include => rnskia-android}/SkiaOpenGLRenderer.h (67%) diff --git a/package/android/CMakeLists.txt b/package/android/CMakeLists.txt index 3a0d711fe5..dc6ff5fb8c 100644 --- a/package/android/CMakeLists.txt +++ b/package/android/CMakeLists.txt @@ -31,7 +31,8 @@ add_library( "${PROJECT_SOURCE_DIR}/cpp/jni/JniSkiaManager.cpp" "${PROJECT_SOURCE_DIR}/cpp/jni/JniSkiaDrawView.cpp" "${PROJECT_SOURCE_DIR}/cpp/jni/JniPlatformContext.cpp" - "${PROJECT_SOURCE_DIR}/cpp/jni/SkiaOpenGLRenderer.cpp" + "${PROJECT_SOURCE_DIR}/cpp/rnskia-android/RNSkDrawViewImpl.cpp" + "${PROJECT_SOURCE_DIR}/cpp/rnskia-android/SkiaOpenGLRenderer.cpp" "${PROJECT_SOURCE_DIR}/cpp/jsi/JsiHostObject.cpp" @@ -64,6 +65,7 @@ target_include_directories( cpp/api cpp/jsi cpp/jni/include + cpp/rnskia-android cpp/rnskia cpp/rnskia/values cpp/utils diff --git a/package/android/cpp/jni/JniSkiaDrawView.cpp b/package/android/cpp/jni/JniSkiaDrawView.cpp index 8aea2fb9b7..c5ca50643e 100644 --- a/package/android/cpp/jni/JniSkiaDrawView.cpp +++ b/package/android/cpp/jni/JniSkiaDrawView.cpp @@ -39,8 +39,8 @@ namespace RNSkia /**** JNI ****/ TSelf JniSkiaDrawView::initHybrid( - alias_ref jThis, - JavaSkiaManager skiaManager) + alias_ref jThis, + JavaSkiaManager skiaManager) { return makeCxxInstance(jThis, skiaManager); } @@ -60,17 +60,17 @@ namespace RNSkia { if (mode.compare("continuous") == 0) { - setDrawingMode(RNSkDrawingMode::Continuous); + _drawView->setDrawingMode(RNSkDrawingMode::Continuous); } else { - setDrawingMode(RNSkDrawingMode::Default); + _drawView->setDrawingMode(RNSkDrawingMode::Default); } } void JniSkiaDrawView::setDebugMode(bool show) { - setShowDebugOverlays(show); + _drawView->setShowDebugOverlays(show); } void JniSkiaDrawView::updateTouchPoints(jni::JArrayDouble touches) @@ -78,7 +78,7 @@ namespace RNSkia // Create touch points std::vector points; auto pin = touches.pin(); - auto scale = getPlatformContext()->getPixelDensity(); + auto scale = _drawView->getPixelDensity(); points.reserve(pin.size() / 4); for (size_t i = 0; i < pin.size(); i += 4) { @@ -89,81 +89,21 @@ namespace RNSkia point.type = (RNSkia::RNSkTouchType)pin[i + 3]; points.push_back(point); } - updateTouchState(std::move(points)); + _drawView->updateTouchState(std::move(points)); } void JniSkiaDrawView::surfaceAvailable(jobject surface, int width, int height) { -#if LOG_ALL_DRAWING - RNSkLogger::logToConsole("JniSkiaDrawView::surfaceAvailable %i", getNativeId()); -#endif - - _width = width; - _height = height; - - if (_renderer == nullptr) - { - // Create renderer! - _renderer = new SkiaOpenGLRenderer( - ANativeWindow_fromSurface(Environment::current(), surface), getNativeId()); - - // Set the draw function - setNativeDrawFunc(std::bind(&JniSkiaDrawView::drawFrame, this, std::placeholders::_1)); - - // Redraw - requestRedraw(); - } + _drawView->surfaceAvailable(ANativeWindow_fromSurface(Environment::current(), surface), width, height); } void JniSkiaDrawView::surfaceSizeChanged(int width, int height) { -#if LOG_ALL_DRAWING - RNSkLogger::logToConsole("JniSkiaDrawView::surfaceSizeChanged %i", getNativeId()); -#endif - - _width = width; - _height = height; - - // Redraw after size change - requestRedraw(); + _drawView->surfaceSizeChanged(width, height); } void JniSkiaDrawView::surfaceDestroyed() { -#if LOG_ALL_DRAWING - RNSkLogger::logToConsole("JniSkiaDrawView::surfaceDestroyed %i", getNativeId()); -#endif - if (_renderer != nullptr) - { - // Turn off drawing - setNativeDrawFunc(nullptr); - - // Start teardown - _renderer->teardown(); - - // Ask for a redraw to tear down the render pipeline. This - // needs to be done on the render thread since OpenGL demands - // same thread access for OpenGL contexts. - getPlatformContext()->runOnRenderThread([this]() - { - if(_renderer != nullptr) { - _renderer->run(nullptr, 0, 0); - } }); - - // Wait until the above render has finished. - _renderer->waitForTeardown(); - - // Delete renderer. All resources should be released during teardown. - delete _renderer; - _renderer = nullptr; - } - } - - /**** Render method ****/ - - void JniSkiaDrawView::drawFrame(const sk_sp picture) - { - // No need to check if the renderer is nullptr since we only get here if it is not. - _renderer->run(picture, _width, _height); + _drawView->surfaceDestroyed(); } } // namespace RNSkia diff --git a/package/android/cpp/jni/JniSkiaManager.cpp b/package/android/cpp/jni/JniSkiaManager.cpp index c9de050ceb..e2b590d5be 100644 --- a/package/android/cpp/jni/JniSkiaManager.cpp +++ b/package/android/cpp/jni/JniSkiaManager.cpp @@ -47,7 +47,7 @@ void JniSkiaManager::initializeRuntime() { } void JniSkiaManager::registerSkiaView(int viewTag, JniSkiaDrawView *skiaView) { - _skManager->registerSkiaDrawView(viewTag, skiaView); + _skManager->registerSkiaDrawView(viewTag, skiaView->getDrawViewImpl()); } void JniSkiaManager::unregisterSkiaView(int viewTag) { diff --git a/package/android/cpp/jni/include/JniSkiaDrawView.h b/package/android/cpp/jni/include/JniSkiaDrawView.h index f461d523d4..8cacab8294 100644 --- a/package/android/cpp/jni/include/JniSkiaDrawView.h +++ b/package/android/cpp/jni/include/JniSkiaDrawView.h @@ -5,17 +5,16 @@ #include #include -#include -#include #include #include #include #include #include -#include "JniSkiaManager.h" -#include "JniSkiaDrawView.h" -#include "SkiaOpenGLRenderer.h" +#include +#include + +#include #include #include @@ -31,16 +30,15 @@ namespace RNSkia using JavaSkiaManager = jni::alias_ref; - class JniSkiaDrawView : public jni::HybridClass, - public RNSkDrawView + class JniSkiaDrawView : public jni::HybridClass { public: static auto constexpr kJavaDescriptor = "Lcom/shopify/reactnative/skia/SkiaDrawView;"; static auto constexpr TAG = "ReactNativeSkia"; static jni::local_ref initHybrid( - jni::alias_ref, - JavaSkiaManager); + jni::alias_ref, + JavaSkiaManager); static void registerNatives(); @@ -52,30 +50,26 @@ namespace RNSkia ~JniSkiaDrawView(); - protected: - int getWidth() override { return _width; } - int getHeight() override { return _height; } + std::shared_ptr getDrawViewImpl() { + return _drawView; + } + protected: void setMode(std::string mode); void setDebugMode(bool show); private: friend HybridBase; - void drawFrame(const sk_sp picture); - - int _width = 0; - int _height = 0; - - SkiaOpenGLRenderer* _renderer = nullptr; + std::shared_ptr _drawView; jni::global_ref javaPart_; explicit JniSkiaDrawView( - jni::alias_ref jThis, - JavaSkiaManager skiaManager) - : javaPart_(jni::make_global(jThis)), - RNSkDrawView(skiaManager->cthis()->getPlatformContext()) { + jni::alias_ref jThis, + JavaSkiaManager skiaManager) + : javaPart_(jni::make_global(jThis)), + _drawView(std::make_shared(skiaManager->cthis()->getPlatformContext())) { } }; diff --git a/package/android/cpp/rnskia-android/RNSkDrawViewImpl.cpp b/package/android/cpp/rnskia-android/RNSkDrawViewImpl.cpp new file mode 100644 index 0000000000..4a8d28733f --- /dev/null +++ b/package/android/cpp/rnskia-android/RNSkDrawViewImpl.cpp @@ -0,0 +1,67 @@ +#include + +#pragma clang diagnostic push +#pragma clang diagnostic ignored "-Wdocumentation" + +#include +#include + +#pragma clang diagnostic pop + +#include + +namespace RNSkia { + RNSkDrawViewImpl::RNSkDrawViewImpl(std::shared_ptr context) : + RNSkia::RNSkDrawView(context) { + } + + void RNSkDrawViewImpl::surfaceAvailable(ANativeWindow* surface, int width, int height) { + _width = width; + _height = height; + + if (_renderer == nullptr) + { + // Create renderer! + _renderer = std::make_shared(surface, getNativeId()); + + // Redraw + requestRedraw(); + } + } + + void RNSkDrawViewImpl::surfaceDestroyed() { + if (_renderer != nullptr) + { + // Start teardown + _renderer->teardown(); + + // Teardown renderer on the render thread since OpenGL demands + // same thread access for OpenGL contexts. + getPlatformContext()->runOnRenderThread([weakSelf = weak_from_this()]() { + auto self = weakSelf.lock(); + if(self) { + auto drawViewImpl = std::dynamic_pointer_cast(self); + if(drawViewImpl->_renderer != nullptr) { + drawViewImpl->_renderer->run(nullptr, 0, 0); + } + // Remove renderer + drawViewImpl->_renderer = nullptr; + } + }); + } + } + + void RNSkDrawViewImpl::surfaceSizeChanged(int width, int height) { + _width = width; + _height = height; + + // Redraw after size change + requestRedraw(); + } + + void RNSkDrawViewImpl::drawPicture(const sk_sp picture) { + if(_renderer != nullptr) { + _renderer->run(picture, _width, _height); + } + } +} diff --git a/package/android/cpp/rnskia-android/RNSkDrawViewImpl.h b/package/android/cpp/rnskia-android/RNSkDrawViewImpl.h new file mode 100644 index 0000000000..b841e64e99 --- /dev/null +++ b/package/android/cpp/rnskia-android/RNSkDrawViewImpl.h @@ -0,0 +1,45 @@ +#pragma once + +#include + +#include +#include + +#pragma clang diagnostic push +#pragma clang diagnostic ignored "-Wdocumentation" + +#include +#include + +#pragma clang diagnostic pop + +namespace RNSkia { + class RNSkDrawViewImpl : public RNSkia::RNSkDrawView { + public: + RNSkDrawViewImpl(std::shared_ptr context); + + void surfaceAvailable(ANativeWindow* surface, int, int); + void surfaceDestroyed(); + void surfaceSizeChanged(int, int); + + float getPixelDensity() { + return getPlatformContext()->getPixelDensity(); + } + + protected: + int getWidth() override { return _width * getPlatformContext()->getPixelDensity(); }; + + int getHeight() override { return _height * getPlatformContext()->getPixelDensity(); }; + + void drawPicture(const sk_sp picture) override; + + private: + bool createSkiaSurface(); + + std::shared_ptr _renderer = nullptr; + + int _nativeId; + int _width = -1; + int _height = -1; + }; +} diff --git a/package/android/cpp/jni/SkiaOpenGLRenderer.cpp b/package/android/cpp/rnskia-android/SkiaOpenGLRenderer.cpp similarity index 64% rename from package/android/cpp/jni/SkiaOpenGLRenderer.cpp rename to package/android/cpp/rnskia-android/SkiaOpenGLRenderer.cpp index a5fac939ce..307c1b1473 100644 --- a/package/android/cpp/jni/SkiaOpenGLRenderer.cpp +++ b/package/android/cpp/rnskia-android/SkiaOpenGLRenderer.cpp @@ -1,23 +1,17 @@ #include "SkiaOpenGLRenderer.h" #include + namespace RNSkia { - - /** Static members */ - std::shared_ptr SkiaOpenGLRenderer::getThreadDrawingContext() - { - auto threadId = std::this_thread::get_id(); - if (threadContexts.count(threadId) == 0) - { - auto drawingContext = std::make_shared(); - drawingContext->glContext = EGL_NO_CONTEXT; - drawingContext->glDisplay = EGL_NO_DISPLAY; - drawingContext->glConfig = 0; - drawingContext->skContext = nullptr; - threadContexts.emplace(threadId, drawingContext); - } - return threadContexts.at(threadId); + EGLContext SkiaOpenGLRenderer::glContext = EGL_NO_CONTEXT; + EGLDisplay SkiaOpenGLRenderer::glDisplay = EGL_NO_DISPLAY; + EGLConfig SkiaOpenGLRenderer::glConfig = 0; + sk_sp SkiaOpenGLRenderer::skContext = nullptr; + + SkiaOpenGLRenderer::SkiaOpenGLRenderer(ANativeWindow *surface, size_t renderId): + _surfaceTexture(surface), + _renderId(renderId) { } void SkiaOpenGLRenderer::run(const sk_sp picture, int width, int height) @@ -50,7 +44,7 @@ namespace RNSkia { // Reset Skia Context since it might be modified by another Skia View during // rendering. - getThreadDrawingContext()->skContext->resetContext(); + skContext->resetContext(); // Clear with transparent glClearColor(0.0f, 0.0f, 0.0f, 0.0f); @@ -61,9 +55,9 @@ namespace RNSkia // Flush _skSurface->getCanvas()->flush(); - getThreadDrawingContext()->skContext->flush(); + skContext->flush(); - if (!eglSwapBuffers(getThreadDrawingContext()->glDisplay, _glSurface)) + if (!eglSwapBuffers(glDisplay, _glSurface)) { RNSkLogger::logToConsole( "eglSwapBuffers failed: %d\n", eglGetError()); @@ -73,7 +67,16 @@ namespace RNSkia } case RenderState::Finishing: { - finish(); + _renderState = RenderState::Done; + + if (_glSurface != EGL_NO_SURFACE && glDisplay != EGL_NO_DISPLAY) + { + eglDestroySurface(glDisplay, _glSurface); + } + + _skSurface = nullptr; + _surfaceTexture = nullptr; + break; } case RenderState::Done: @@ -107,67 +110,20 @@ namespace RNSkia return true; } - void SkiaOpenGLRenderer::finish() - { - std::lock_guard lock(_lock); - - if (_renderState != RenderState::Finishing) - { - _cv.notify_all(); - return; - } - - finishGL(); - finishSkiaSurface(); - - _renderState = RenderState::Done; - - _cv.notify_one(); - } - - void SkiaOpenGLRenderer::finishGL() - { - if (_glSurface != EGL_NO_SURFACE && getThreadDrawingContext()->glDisplay != EGL_NO_DISPLAY) - { - eglDestroySurface(getThreadDrawingContext()->glDisplay, _glSurface); - } - } - - void SkiaOpenGLRenderer::finishSkiaSurface() - { - if (_skSurface != nullptr) - { - _skSurface = nullptr; - } - - if (_nativeWindow != nullptr) - { - ANativeWindow_release(_nativeWindow); - _nativeWindow = nullptr; - } - } - void SkiaOpenGLRenderer::teardown() { _renderState = RenderState::Finishing; } - void SkiaOpenGLRenderer::waitForTeardown() - { - std::unique_lock lock(_lock); - _cv.wait(lock, [this] - { return (_renderState == RenderState::Done); }); - } - bool SkiaOpenGLRenderer::initStaticGLContext() { - if (getThreadDrawingContext()->glContext != EGL_NO_CONTEXT) + if (glContext != EGL_NO_CONTEXT) { return true; } - getThreadDrawingContext()->glDisplay = eglGetDisplay(EGL_DEFAULT_DISPLAY); - if (getThreadDrawingContext()->glDisplay == EGL_NO_DISPLAY) + glDisplay = eglGetDisplay(EGL_DEFAULT_DISPLAY); + if (glDisplay == EGL_NO_DISPLAY) { RNSkLogger::logToConsole("eglGetdisplay failed : %i", glGetError()); return false; @@ -175,7 +131,7 @@ namespace RNSkia EGLint major; EGLint minor; - if (!eglInitialize(getThreadDrawingContext()->glDisplay, &major, &minor)) + if (!eglInitialize(glDisplay, &major, &minor)) { RNSkLogger::logToConsole("eglInitialize failed : %i", glGetError()); return false; @@ -201,8 +157,8 @@ namespace RNSkia EGL_NONE}; EGLint numConfigs; - getThreadDrawingContext()->glConfig = 0; - if (!eglChooseConfig(getThreadDrawingContext()->glDisplay, att, &getThreadDrawingContext()->glConfig, 1, &numConfigs) || + glConfig = 0; + if (!eglChooseConfig(glDisplay, att, &glConfig, 1, &numConfigs) || numConfigs == 0) { RNSkLogger::logToConsole( @@ -212,8 +168,9 @@ namespace RNSkia EGLint contextAttribs[] = {EGL_CONTEXT_CLIENT_VERSION, 2, EGL_NONE}; - getThreadDrawingContext()->glContext = eglCreateContext(getThreadDrawingContext()->glDisplay, getThreadDrawingContext()->glConfig, NULL, contextAttribs); - if (getThreadDrawingContext()->glContext == EGL_NO_CONTEXT) + glContext = eglCreateContext(glDisplay, glConfig, NULL, contextAttribs); + + if (glContext == EGL_NO_CONTEXT) { RNSkLogger::logToConsole( "eglCreateContext failed: %d\n", eglGetError()); @@ -225,15 +182,15 @@ namespace RNSkia bool SkiaOpenGLRenderer::initStaticSkiaContext() { - if (getThreadDrawingContext()->skContext != nullptr) + if (skContext != nullptr) { return true; } // Create the Skia backend context auto backendInterface = GrGLMakeNativeInterface(); - getThreadDrawingContext()->skContext = GrDirectContext::MakeGL(backendInterface); - if (getThreadDrawingContext()->skContext == nullptr) + skContext = GrDirectContext::MakeGL(backendInterface); + if (skContext == nullptr) { RNSkLogger::logToConsole("GrDirectContext::MakeGL failed"); return false; @@ -244,14 +201,14 @@ namespace RNSkia bool SkiaOpenGLRenderer::initGLSurface() { - if (_nativeWindow == nullptr) + if (_surfaceTexture == nullptr) { return false; } if (_glSurface != EGL_NO_SURFACE) { - if (!eglMakeCurrent(getThreadDrawingContext()->glDisplay, _glSurface, _glSurface, getThreadDrawingContext()->glContext)) + if (!eglMakeCurrent(glDisplay, _glSurface, _glSurface, glContext)) { RNSkLogger::logToConsole( "eglMakeCurrent failed: %d\n", eglGetError()); @@ -262,7 +219,7 @@ namespace RNSkia // Create the opengl surface _glSurface = - eglCreateWindowSurface(getThreadDrawingContext()->glDisplay, getThreadDrawingContext()->glConfig, _nativeWindow, nullptr); + eglCreateWindowSurface(glDisplay, glConfig, _surfaceTexture, nullptr); if (_glSurface == EGL_NO_SURFACE) { RNSkLogger::logToConsole( @@ -270,7 +227,7 @@ namespace RNSkia return false; } - if (!eglMakeCurrent(getThreadDrawingContext()->glDisplay, _glSurface, _glSurface, getThreadDrawingContext()->glContext)) + if (!eglMakeCurrent(glDisplay, _glSurface, _glSurface, glContext)) { RNSkLogger::logToConsole("eglMakeCurrent failed: %d\n", eglGetError()); return false; @@ -281,7 +238,7 @@ namespace RNSkia bool SkiaOpenGLRenderer::ensureSkiaSurface(int width, int height) { - if (getThreadDrawingContext()->skContext == nullptr) + if (skContext == nullptr) { return false; } @@ -305,7 +262,7 @@ namespace RNSkia GLint samples; glGetIntegerv(GL_SAMPLES, &samples); - auto maxSamples = getThreadDrawingContext()->skContext->maxSurfaceSampleCountForColorType( + auto maxSamples = skContext->maxSurfaceSampleCountForColorType( kRGBA_8888_SkColorType); if (samples > maxSamples) @@ -319,7 +276,7 @@ namespace RNSkia GrBackendRenderTarget(width, height, samples, stencil, fbInfo); _skSurface = SkSurface::MakeFromBackendRenderTarget( - getThreadDrawingContext()->skContext.get(), + skContext.get(), _skRenderTarget, kBottomLeft_GrSurfaceOrigin, kRGBA_8888_SkColorType, diff --git a/package/android/cpp/jni/include/SkiaOpenGLRenderer.h b/package/android/cpp/rnskia-android/SkiaOpenGLRenderer.h similarity index 67% rename from package/android/cpp/jni/include/SkiaOpenGLRenderer.h rename to package/android/cpp/rnskia-android/SkiaOpenGLRenderer.h index 4cda88a3a3..898ec01377 100644 --- a/package/android/cpp/jni/include/SkiaOpenGLRenderer.h +++ b/package/android/cpp/rnskia-android/SkiaOpenGLRenderer.h @@ -7,7 +7,6 @@ #include "GLES2/gl2.h" #include -#include #include #include @@ -25,16 +24,6 @@ namespace RNSkia { - using DrawingContext = struct - { - EGLContext glContext; - EGLDisplay glDisplay; - EGLConfig glConfig; - sk_sp skContext; - }; - - static std::unordered_map> threadContexts; - enum RenderState : int { Initializing, Rendering, @@ -45,9 +34,7 @@ namespace RNSkia class SkiaOpenGLRenderer { public: - SkiaOpenGLRenderer(ANativeWindow *nativeWindow, size_t renderId) : - _nativeWindow(nativeWindow), - _renderId(renderId) { } + SkiaOpenGLRenderer(ANativeWindow *surface, size_t renderId); /** * Initializes, renders and tears down the render pipeline depending on the state of the @@ -71,14 +58,6 @@ namespace RNSkia */ void teardown(); - /** - * Wait for teardown to finish. This means that we'll wait until the next - * render which will handle releasing all OpenGL and Skia resources used for - * this renderer. After tearing down the render will do nothing if the render - * method is called again. - */ - void waitForTeardown(); - private: /** * Initializes all required OpenGL and Skia objects @@ -117,31 +96,14 @@ namespace RNSkia */ bool ensureSkiaSurface(int width, int height); - /** - * Finalizes and releases all resources used by this renderer - */ - void finish(); - - /** - * Destroys the underlying OpenGL surface used for this renderer - */ - void finishGL(); - - /** - * Destroys the underlying Skia surface used for this renderer - */ - void finishSkiaSurface(); - - /** - * To be able to use static contexts (and avoid reloading the skia context for each - * new view, we track the OpenGL and Skia drawing context per thread. - * @return The drawing context for the current thread - */ - static std::shared_ptr getThreadDrawingContext(); + static EGLContext glContext; + static EGLDisplay glDisplay; + static EGLConfig glConfig; + static sk_sp skContext; EGLSurface _glSurface = EGL_NO_SURFACE; - ANativeWindow *_nativeWindow = nullptr; + ANativeWindow *_surfaceTexture = nullptr; GrBackendRenderTarget _skRenderTarget; sk_sp _skSurface; @@ -150,9 +112,6 @@ namespace RNSkia size_t _renderId; - std::mutex _lock; - std::condition_variable _cv; - std::atomic _renderState = { RenderState::Initializing }; }; diff --git a/package/android/src/main/java/com/shopify/reactnative/skia/RNSkiaViewManager.java b/package/android/src/main/java/com/shopify/reactnative/skia/RNSkiaViewManager.java index 6ed1bd76ea..cd290a6248 100644 --- a/package/android/src/main/java/com/shopify/reactnative/skia/RNSkiaViewManager.java +++ b/package/android/src/main/java/com/shopify/reactnative/skia/RNSkiaViewManager.java @@ -61,7 +61,6 @@ public void onDropViewInstance(@NonNull SkiaDrawView view) { Integer nativeId = mViewMapping.get(view); skiaModule.getSkiaManager().unregister(nativeId); mViewMapping.remove(view); - view.onRemoved(); } @NonNull diff --git a/package/android/src/main/java/com/shopify/reactnative/skia/SkiaDrawView.java b/package/android/src/main/java/com/shopify/reactnative/skia/SkiaDrawView.java index 7c57944b4e..3389f66e9b 100644 --- a/package/android/src/main/java/com/shopify/reactnative/skia/SkiaDrawView.java +++ b/package/android/src/main/java/com/shopify/reactnative/skia/SkiaDrawView.java @@ -39,18 +39,12 @@ public void setBackgroundColor(int color) { @Override protected void finalize() throws Throwable { mHybridData.resetNative(); + if(mSurface != null) { + mSurface.release(); + } super.finalize(); } - - public void onRemoved() { - // We'll mark the view removed since we reset the native part. - // This means that none of the native methods should be called after - // this point. - mIsRemoved = true; - mHybridData.resetNative(); - } - @Override public boolean onTouchEvent(MotionEvent ev) { if(mIsRemoved) { @@ -109,9 +103,11 @@ public boolean onSurfaceTextureDestroyed(SurfaceTexture surface) { return true; } surfaceDestroyed(); - mSurface.release(); - mSurface = null; - return true; + // https://developer.android.com/reference/android/view/TextureView.SurfaceTextureListener#onSurfaceTextureDestroyed(android.graphics.SurfaceTexture) + // Invoked when the specified SurfaceTexture is about to be destroyed. If returns true, + // no rendering should happen inside the surface texture after this method is invoked. + // If returns false, the client needs to call SurfaceTexture#release(). + return false; } @Override diff --git a/package/cpp/rnskia/RNSkDrawView.cpp b/package/cpp/rnskia/RNSkDrawView.cpp index a1f5c68691..3e04fa69ea 100644 --- a/package/cpp/rnskia/RNSkDrawView.cpp +++ b/package/cpp/rnskia/RNSkDrawView.cpp @@ -40,31 +40,13 @@ RNSkDrawView::RNSkDrawView(std::shared_ptr context) _platformContext(std::move(context)), _infoObject(std::make_shared()), _jsDrawingLock(std::make_shared()), - _gpuDrawingLock(std::make_shared()) + _gpuDrawingLock(std::make_shared()), + _jsTimingInfo("SKIA/JS"), + _gpuTimingInfo("SKIA/GPU") {} RNSkDrawView::~RNSkDrawView() { -#if LOG_ALL_DRAWING - RNSkLogger::logToConsole("RNSkDrawView::~RNSkDrawView - %i", getNativeId()); -#endif - - _isInvalidated = true; - endDrawingLoop(); - - // Wait for the drawing locks - if(!_jsDrawingLock->try_lock_for(1250ms)) { - RNSkLogger::logToConsole("Warning: JS drawing is still locked for native view with id %i", _nativeId); - } - - if(!_gpuDrawingLock->try_lock_for(1250ms)) { - RNSkLogger::logToConsole("Warning: SKIA drawing is still locked for native view with id %i", _nativeId); - } - - onInvalidated(); - - _jsDrawingLock = nullptr; - _gpuDrawingLock = nullptr; } void RNSkDrawView::setNativeId(size_t nativeId) { @@ -84,7 +66,6 @@ void RNSkDrawView::setDrawCallback(std::shared_ptr callback) { // Reset timing info _jsTimingInfo.reset(); _gpuTimingInfo.reset(); - _vsyncTimingInfo.reset(); // Set up debug font/paints auto font = SkFont(); @@ -94,53 +75,57 @@ void RNSkDrawView::setDrawCallback(std::shared_ptr callback) { // Create draw drawCallback wrapper _drawCallback = std::make_shared( - [this, paint = std::move(paint), font = std::move(font), callback = std::move(callback)](std::shared_ptr canvas, int width, - int height, double timestamp, - std::shared_ptr context) { - - auto runtime = context->getJsRuntime(); - - // Update info parameter - _infoObject->beginDrawOperation(width, height, timestamp); - - // Set up arguments array - std::vector args(2); - args[0] = jsi::Object::createFromHostObject(*runtime, canvas); - args[1] = jsi::Object::createFromHostObject(*runtime, _infoObject); - - // To be able to call the drawing function we'll wrap it once again - callback->call(*runtime, - static_cast(args.data()), - (size_t)2); - - // Reset touches - _infoObject->endDrawOperation(); - - // Draw debug overlays - if (_showDebugOverlay) { - - // Display average rendering timer - auto jsAvg = _jsTimingInfo.getAverage(); - //auto jsFps = _jsTimingInfo.getFps(); - - auto gpuAvg = _gpuTimingInfo.getAverage(); - //auto gpuFps = _gpuTimingInfo.getFps(); - - auto total = jsAvg + gpuAvg; - - //auto vsyncFps = _vsyncTimingInfo.getFps(); - - // Build string - std::ostringstream stream; - stream << "js: " << jsAvg << "ms gpu: " << gpuAvg << "ms " << " total: " << total << "ms"; -// stream << "js:" << jsAvg << "ms/" << jsFps << "fps " << "gpu:" << gpuAvg << "ms/" << -// gpuFps << "fps" << " total:" << total << "ms/" << vsyncFps << "fps"; - - std::string debugString = stream.str(); - - canvas->getCanvas()->drawSimpleText( - debugString.c_str(), debugString.size(), SkTextEncoding::kUTF8, 8, - 18, font, paint); + [weakSelf = weak_from_this(), + paint = std::move(paint), + font = std::move(font), + callback = std::move(callback)](std::shared_ptr canvas, + int width, + int height, + double timestamp, + std::shared_ptr context) { + + auto self = weakSelf.lock(); + if(self) { + auto runtime = context->getJsRuntime(); + + // Update info parameter + self->_infoObject->beginDrawOperation(width, height, timestamp); + + // Set up arguments array + std::vector args(2); + args[0] = jsi::Object::createFromHostObject(*runtime, canvas); + args[1] = jsi::Object::createFromHostObject(*runtime, self->_infoObject); + + // To be able to call the drawing function we'll wrap it once again + callback->call(*runtime, + static_cast(args.data()), + (size_t)2); + + // Reset touches + self->_infoObject->endDrawOperation(); + + // Draw debug overlays + if (self->_showDebugOverlay) { + + // Display average rendering timer + auto jsAvg = self->_jsTimingInfo.getAverage(); + //auto jsFps = _jsTimingInfo.getFps(); + + auto gpuAvg = self->_gpuTimingInfo.getAverage(); + //auto gpuFps = _gpuTimingInfo.getFps(); + + auto total = jsAvg + gpuAvg; + + // Build string + std::ostringstream stream; + stream << "js: " << jsAvg << "ms gpu: " << gpuAvg << "ms " << " total: " << total << "ms"; + + std::string debugString = stream.str(); + + canvas->getCanvas()->drawSimpleText( + debugString.c_str(), debugString.size(), SkTextEncoding::kUTF8, 8, + 18, font, paint); + } } }); @@ -196,10 +181,6 @@ void RNSkDrawView::updateTouchState(std::vector&& points) { } void RNSkDrawView::performDraw() { -#if LOG_ALL_DRAWING - RNSkLogger::logToConsole("RNSkDrawView::performDraw - %i", getNativeId()); -#endif - // Start timing _jsTimingInfo.beginTiming(); @@ -234,37 +215,22 @@ void RNSkDrawView::performDraw() { // Post drawing message to the render thread where the picture recorded // will be sent to the GPU/backend for rendering to screen. auto gpuLock = _gpuDrawingLock; - getPlatformContext()->runOnRenderThread([this, p = std::move(p), gpuLock]() { - - if(isInvalidated()) { - gpuLock->unlock(); - return; + _platformContext->runOnRenderThread([weakSelf = weak_from_this(), p = std::move(p), gpuLock]() { + auto self = weakSelf.lock(); + if (self) { + // Draw the picture recorded on the real GPU canvas + self->_gpuTimingInfo.beginTiming(); + self->drawPicture(p); + self->_gpuTimingInfo.stopTiming(); } - - _gpuTimingInfo.beginTiming(); - - // Draw the picture recorded on the real GPU canvas - if(_nativeDrawFunc != nullptr) { -#if LOG_ALL_DRAWING - RNSkLogger::logToConsole("RNSkDrawView::drawFrame - %i", getNativeId()); -#endif - _nativeDrawFunc(p); - } else { -#if LOG_ALL_DRAWING - RNSkLogger::logToConsole("RNSkDrawView::drawFrame - %i SKIPPING, draw func is null", getNativeId()); -#endif - } - - _gpuTimingInfo.stopTiming(); - // Unlock GPU drawing gpuLock->unlock(); }); } else { #ifdef DEBUG - static size_t framesSkipped = 0; - printf("SKIA/GPU: Skipped frames: %lu\n", ++framesSkipped); + _gpuTimingInfo.markSkipped(); #endif + // Request a new redraw since the last frame was skipped. requestRedraw(); } @@ -280,39 +246,34 @@ void RNSkDrawView::beginDrawingLoop() { if (_drawingLoopId != 0 || _nativeId == 0) { return; } - // Set to zero to avoid calling beginDrawLoop before we return - _drawingLoopId = _platformContext->beginDrawLoop( - _nativeId, std::bind(&RNSkDrawView::drawLoopCallback, this, std::placeholders::_1)); + _drawingLoopId = _platformContext->beginDrawLoop(_nativeId, + [weakSelf = weak_from_this()](bool invalidated) { + auto self = weakSelf.lock(); + if(self) { + self->drawLoopCallback(invalidated); + } + }); } void RNSkDrawView::drawLoopCallback(bool invalidated) { - if(invalidated) { - _isInvalidated = true; - onInvalidated(); -#if LOG_ALL_DRAWING - RNSkLogger::logToConsole("RNSkDrawView::onInvalidated - %i", getNativeId()); -#endif - return; - } - if(_redrawRequestCounter > 0 || _drawingMode == RNSkDrawingMode::Continuous) { _redrawRequestCounter = 0; - _vsyncTimingInfo.beginTiming(); - // We render on the javascript thread. if(_jsDrawingLock->try_lock()) { - _platformContext->runOnJavascriptThread(std::bind(&RNSkDrawView::performDraw, this)); + _platformContext->runOnJavascriptThread([weakSelf = weak_from_this()](){ + auto self = weakSelf.lock(); + if(self) { + self->performDraw(); + } + }); } else { #ifdef DEBUG - static size_t framesSkipped = 0; - printf("SKIA/JS: Skipped frames: %lu\n", ++framesSkipped); + _jsTimingInfo.markSkipped(); #endif requestRedraw(); } - - _vsyncTimingInfo.stopTiming(); } } diff --git a/package/cpp/rnskia/RNSkDrawView.h b/package/cpp/rnskia/RNSkDrawView.h index 3a7a10be7b..f1d6a0ffd6 100644 --- a/package/cpp/rnskia/RNSkDrawView.h +++ b/package/cpp/rnskia/RNSkDrawView.h @@ -20,8 +20,6 @@ #pragma clang diagnostic pop -#define LOG_ALL_DRAWING 0 - class SkPicture; class SkRect; class SkImage; @@ -35,7 +33,7 @@ using RNSkDrawCallback = enum RNSkDrawingMode { Default, Continuous }; -class RNSkDrawView { +class RNSkDrawView: public std::enable_shared_from_this { public: /** * Constructor @@ -45,7 +43,7 @@ class RNSkDrawView { /** Destructor */ - ~RNSkDrawView(); + virtual ~RNSkDrawView(); /** * Repaints the Skia view using the underlying context and the drawcallback. @@ -96,14 +94,6 @@ class RNSkDrawView { sk_sp makeImageSnapshot(std::shared_ptr bounds); protected: - void setNativeDrawFunc(std::function)> drawFunc) { - if(!_gpuDrawingLock->try_lock_for(250ms)) { - RNSkLogger::logToConsole("Could not lock drawing when clearing drawing function - %i", _nativeId); - } - _nativeDrawFunc = drawFunc; - _gpuDrawingLock->unlock(); - } - /** Returns the scaled width of the view */ @@ -115,14 +105,9 @@ class RNSkDrawView { virtual int getHeight() { return -1; }; /** - Returns true if the view is invalidated - */ - volatile bool isInvalidated() { return _isInvalidated; } - - /** - Override to be notified on invalidation + Override to render picture to GPU */ - virtual void onInvalidated() {} + virtual void drawPicture(const sk_sp picture) = 0; /** * @return The platformcontext @@ -210,12 +195,7 @@ class RNSkDrawView { /** Timing information for GPU rendering */ - RNSkTimingInfo _gpuTimingInfo; - - /** - Measures vsync framerate - */ - RNSkTimingInfo _vsyncTimingInfo; + RNSkTimingInfo _gpuTimingInfo; /** Redraw queue counter @@ -227,16 +207,6 @@ class RNSkDrawView { */ size_t _nativeId; - /** - Invalidation flag - */ - std::atomic _isInvalidated = { false }; - - /** - Native draw handler - */ - std::function)> _nativeDrawFunc; - }; } // namespace RNSkia diff --git a/package/cpp/rnskia/RNSkJsiViewApi.h b/package/cpp/rnskia/RNSkJsiViewApi.h index 7531e3cf86..474629cdc0 100644 --- a/package/cpp/rnskia/RNSkJsiViewApi.h +++ b/package/cpp/rnskia/RNSkJsiViewApi.h @@ -20,7 +20,7 @@ using CallbackInfo = struct CallbackInfo { view = nullptr; } std::shared_ptr drawCallback; - RNSkDrawView *view; + std::shared_ptr view; }; class RNSkJsiViewApi : public JsiHostObject { @@ -226,7 +226,7 @@ class RNSkJsiViewApi : public JsiHostObject { * @param nativeId Id of view to register * @param view View to register */ - void registerSkiaDrawView(size_t nativeId, RNSkDrawView *view) { + void registerSkiaDrawView(size_t nativeId, std::shared_ptr view) { auto info = getEnsuredCallbackInfo(nativeId); info->view = view; if (info->drawCallback != nullptr) { @@ -260,7 +260,7 @@ class RNSkJsiViewApi : public JsiHostObject { a valid view is set, the setDrawCallback method is called on the view (if a valid callback exists). */ - void setSkiaDrawView(size_t nativeId, RNSkDrawView *view) { + void setSkiaDrawView(size_t nativeId, std::shared_ptr view) { if (_callbackInfos.find(nativeId) == _callbackInfos.end()) { return; } diff --git a/package/cpp/rnskia/RNSkManager.cpp b/package/cpp/rnskia/RNSkManager.cpp index ad2aa043e5..b273e05438 100644 --- a/package/cpp/rnskia/RNSkManager.cpp +++ b/package/cpp/rnskia/RNSkManager.cpp @@ -45,7 +45,7 @@ void RNSkManager::invalidate() { _platformContext->invalidate(); } -void RNSkManager::registerSkiaDrawView(size_t nativeId, RNSkDrawView *view) { +void RNSkManager::registerSkiaDrawView(size_t nativeId, std::shared_ptr view) { if (!_isInvalidated && _viewApi != nullptr) _viewApi->registerSkiaDrawView(nativeId, view); } @@ -55,7 +55,7 @@ void RNSkManager::unregisterSkiaDrawView(size_t nativeId) { _viewApi->unregisterSkiaDrawView(nativeId); } -void RNSkManager::setSkiaDrawView(size_t nativeId, RNSkDrawView *view) { +void RNSkManager::setSkiaDrawView(size_t nativeId, std::shared_ptr view) { if (!_isInvalidated && _viewApi != nullptr) _viewApi->setSkiaDrawView(nativeId, view); } diff --git a/package/cpp/rnskia/RNSkManager.h b/package/cpp/rnskia/RNSkManager.h index f033fb258b..8e86eeb65f 100644 --- a/package/cpp/rnskia/RNSkManager.h +++ b/package/cpp/rnskia/RNSkManager.h @@ -41,7 +41,7 @@ class RNSkManager { * @param nativeId Native view id * @param view View to register */ - void registerSkiaDrawView(size_t nativeId, RNSkDrawView *view); + void registerSkiaDrawView(size_t nativeId, std::shared_ptr view); /** * Unregisters the RNSkDrawView from the list of registered views @@ -54,7 +54,7 @@ class RNSkManager { Used when we want to remove a view without unregistering it - this happens typically on iOS. */ - void setSkiaDrawView(size_t nativeId, RNSkDrawView *view); + void setSkiaDrawView(size_t nativeId, std::shared_ptr view); /** * @return The platform context diff --git a/package/cpp/rnskia/RNSkPlatformContext.h b/package/cpp/rnskia/RNSkPlatformContext.h index d9d775a80b..c8b61df6b0 100644 --- a/package/cpp/rnskia/RNSkPlatformContext.h +++ b/package/cpp/rnskia/RNSkPlatformContext.h @@ -43,7 +43,7 @@ class RNSkPlatformContext { /** * Destructor */ - ~RNSkPlatformContext() { + virtual ~RNSkPlatformContext() { invalidate(); } diff --git a/package/cpp/rnskia/values/RNSkClockValue.h b/package/cpp/rnskia/values/RNSkClockValue.h index 3a1ca44688..3ed738f5ec 100644 --- a/package/cpp/rnskia/values/RNSkClockValue.h +++ b/package/cpp/rnskia/values/RNSkClockValue.h @@ -72,8 +72,12 @@ enum RNSkClockState { _start += timeSinceStop; _state = RNSkClockState::Running; - auto dispatch = std::bind(&RNSkClockValue::notifyUpdate, this, std::placeholders::_1); - getContext()->beginDrawLoop(_identifier, dispatch); + getContext()->beginDrawLoop(_identifier, [weakSelf = weak_from_this()](bool invalidated){ + auto self = weakSelf.lock(); + if(self) { + std::dynamic_pointer_cast(self)->notifyUpdate(invalidated); + } + }); } virtual void stopClock() { @@ -103,12 +107,16 @@ enum RNSkClockState { getContext()->runOnJavascriptThread( // To ensure that this shared_ptr instance is not deallocated before we are done // running the update lambda we pass a shared from this to the lambda scope. - [self = shared_from_this(), this]() { - if(_state == RNSkClockState::Running) { - auto now = std::chrono::high_resolution_clock::now(); - auto deltaFromStart = std::chrono::duration_cast(now - _start).count(); - tick(_runtime, static_cast(deltaFromStart)); - } + [weakSelf = weak_from_this()]() { + auto self = weakSelf.lock(); + if(self) { + auto selfClockValue = std::dynamic_pointer_cast(self); + if(selfClockValue->getState() == RNSkClockState::Running) { + auto now = std::chrono::high_resolution_clock::now(); + auto deltaFromStart = std::chrono::duration_cast(now - selfClockValue->_start).count(); + selfClockValue->tick(selfClockValue->_runtime, static_cast(deltaFromStart)); + } + } }); } diff --git a/package/cpp/rnskia/values/RNSkReadonlyValue.h b/package/cpp/rnskia/values/RNSkReadonlyValue.h index 80779814af..502bd27d32 100644 --- a/package/cpp/rnskia/values/RNSkReadonlyValue.h +++ b/package/cpp/rnskia/values/RNSkReadonlyValue.h @@ -48,10 +48,13 @@ class RNSkReadonlyValue : public JsiSkHostObject, } auto callback = std::make_shared(arguments[0].asObject(runtime).asFunction(runtime)); - auto unsubscribe = addListener([self = shared_from_this(), - this, + auto unsubscribe = addListener([weakSelf = weak_from_this(), callback = std::move(callback)](jsi::Runtime& runtime){ - callback->call(runtime, get_current(runtime)); + auto self = weakSelf.lock(); + if(self) { + auto selfReadonlyValue = std::dynamic_pointer_cast(self); + callback->call(runtime, selfReadonlyValue->get_current(runtime)); + } }); return jsi::Function::createFromHostFunction(runtime, @@ -74,8 +77,11 @@ class RNSkReadonlyValue : public JsiSkHostObject, std::lock_guard lock(_mutex); auto listenerId = _listenerId++; _listeners.emplace(listenerId, cb); - return [self = shared_from_this(), this, listenerId]() { - removeListener(listenerId); + return [weakSelf = weak_from_this(), listenerId]() { + auto self = weakSelf.lock(); + if(self) { + self->removeListener(listenerId); + } }; } diff --git a/package/cpp/utils/RNSkTimingInfo.h b/package/cpp/utils/RNSkTimingInfo.h index e36d86413a..ebd0a37ffb 100644 --- a/package/cpp/utils/RNSkTimingInfo.h +++ b/package/cpp/utils/RNSkTimingInfo.h @@ -1,5 +1,6 @@ #pragma once +#include #include #define NUMBER_OF_DURATION_SAMPLES 10 @@ -12,7 +13,7 @@ using ms = duration; class RNSkTimingInfo { public: - RNSkTimingInfo() { + RNSkTimingInfo(const std::string &name): _name(std::move(name)) { reset(); } @@ -25,6 +26,7 @@ class RNSkTimingInfo { _prevFpsTimer = -1; _frameCount = 0; _lastFrameCount = -1; + _didSkip = false; } void beginTiming() { @@ -35,6 +37,14 @@ class RNSkTimingInfo { time_point stop = high_resolution_clock::now(); addLastDuration(duration_cast(stop - _start).count()); tick(stop); + if(_didSkip) { + _didSkip = false; + RNSkLogger::logToConsole("%s: Skipped frame. Previous frame time: %lldms", _name.c_str(), _lastDuration); + } + } + + void markSkipped() { + _didSkip = true; } long getAverage() { return static_cast(_average); } @@ -86,6 +96,8 @@ class RNSkTimingInfo { long _prevFpsTimer; double _frameCount; double _lastFrameCount; + double _didSkip; + std::string _name; }; } // namespace RNSkia diff --git a/package/ios/RNSkia-iOS/RNSkDrawViewImpl.h b/package/ios/RNSkia-iOS/RNSkDrawViewImpl.h index 9a4aeed2d6..46179cde24 100644 --- a/package/ios/RNSkia-iOS/RNSkDrawViewImpl.h +++ b/package/ios/RNSkia-iOS/RNSkDrawViewImpl.h @@ -21,20 +21,18 @@ class RNSkDrawViewImpl : public RNSkia::RNSkDrawView { public: RNSkDrawViewImpl(std::shared_ptr context); + ~RNSkDrawViewImpl(); - void setSize(int width, int height); + CALayer* getLayer() { return _layer; } - CALayer* getLayer() { return _layer; }; + void setSize(int width, int height); protected: int getWidth() override { return _width * _context->getPixelDensity(); }; - int getHeight() override { return _height * _context->getPixelDensity(); }; - void onInvalidated() override { - setNativeDrawFunc(nullptr); - }; + int getHeight() override { return _height * _context->getPixelDensity(); }; private: - void drawFrame(const sk_sp picture); + void drawPicture(const sk_sp picture) override; bool createSkiaSurface(); int _nativeId; diff --git a/package/ios/RNSkia-iOS/RNSkDrawViewImpl.mm b/package/ios/RNSkia-iOS/RNSkDrawViewImpl.mm index 55774779d3..e88aa17aff 100644 --- a/package/ios/RNSkia-iOS/RNSkDrawViewImpl.mm +++ b/package/ios/RNSkia-iOS/RNSkDrawViewImpl.mm @@ -19,19 +19,34 @@ RNSkDrawViewImpl::RNSkDrawViewImpl(std::shared_ptr context): _context(context), RNSkia::RNSkDrawView(context) { - #pragma clang diagnostic push #pragma clang diagnostic ignored "-Wunguarded-availability-new" - _layer = [CAMetalLayer layer]; + _layer = [CAMetalLayer layer]; #pragma clang diagnostic pop - _layer.framebufferOnly = NO; - _layer.device = _device; - _layer.opaque = false; - _layer.contentsScale = _context->getPixelDensity(); - _layer.pixelFormat = MTLPixelFormatBGRA8Unorm; - - setNativeDrawFunc(std::bind(&RNSkDrawViewImpl::drawFrame, this, std::placeholders::_1)); + _layer.framebufferOnly = NO; + _layer.device = _device; + _layer.opaque = false; + _layer.contentsScale = _context->getPixelDensity(); + _layer.pixelFormat = MTLPixelFormatBGRA8Unorm; +} + +RNSkDrawViewImpl::~RNSkDrawViewImpl() { + if([[NSThread currentThread] isMainThread]) { + _layer = NULL; + } else { + __block auto tempLayer = _layer; + dispatch_async(dispatch_get_main_queue(), ^{ + // By using the tempLayer variable in the block we capture it and it will be + // released after the block has finished. This way the CAMetalLayer dealloc will + // only be called on the main thread. Problem: this destructor might be called from + // releasing the RNSkDrawViewImpl from a thread capture (after dtor has started), + // which would cause the CAMetalLayer dealloc to be called on another thread which + // causes a crash. + // https://github.com/Shopify/react-native-skia/issues/398 + tempLayer = tempLayer; + }); + } } void RNSkDrawViewImpl::setSize(int width, int height) { @@ -44,7 +59,7 @@ requestRedraw(); } -void RNSkDrawViewImpl::drawFrame(const sk_sp picture) { +void RNSkDrawViewImpl::drawPicture(const sk_sp picture) { if(_width == -1 && _height == -1) { return; } diff --git a/package/ios/RNSkia-iOS/SkiaDrawView.mm b/package/ios/RNSkia-iOS/SkiaDrawView.mm index 00aa0adf1a..da963132ab 100644 --- a/package/ios/RNSkia-iOS/SkiaDrawView.mm +++ b/package/ios/RNSkia-iOS/SkiaDrawView.mm @@ -39,12 +39,6 @@ - (instancetype) initWithManager: (RNSkia::RNSkManager*)manager; return self; } -- (void)dealloc { - if(_manager != nullptr) { - _manager->unregisterSkiaDrawView(_nativeId); - } -} - #pragma mark Lifecycle - (void) willMoveToWindow:(UIWindow *)newWindow { @@ -53,19 +47,21 @@ - (void) willMoveToWindow:(UIWindow *)newWindow { if (newWindow == NULL) { // Remove implementation view when the parent view is not set if(_impl != nullptr) { + [_impl->getLayer() removeFromSuperlayer]; + if(_nativeId != 0 && _manager != nullptr) { - _manager->setSkiaDrawView(_nativeId, nullptr); + _manager->unregisterSkiaDrawView(_nativeId); } - [_impl->getLayer() removeFromSuperlayer]; + _impl = nullptr; } } else { // Create implementation view when the parent view is set if(_impl == nullptr && _manager != nullptr) { _impl = std::make_shared(_manager->getPlatformContext()); - [self.layer addSublayer:_impl->getLayer()]; + [self.layer addSublayer: _impl->getLayer()]; if(_nativeId != 0) { - _manager->setSkiaDrawView(_nativeId, _impl.get()); + _manager->setSkiaDrawView(_nativeId, _impl); } _impl->setDrawingMode(_drawingMode); _impl->setShowDebugOverlays(_debugMode); @@ -103,7 +99,7 @@ - (void) setNativeId:(size_t) nativeId { _nativeId = nativeId; if(_impl != nullptr) { - _manager->registerSkiaDrawView(nativeId, _impl.get()); + _manager->registerSkiaDrawView(nativeId, _impl); } } From 21c7fe1ff73912c872aa6c81bd5f08279cc35c02 Mon Sep 17 00:00:00 2001 From: chrfalch Date: Wed, 27 Apr 2022 18:31:17 +0200 Subject: [PATCH 08/15] Resolved issue with removing the view in view api correctly Previously we just removed the view, now we make sure to remove the drawCallback in the view to deallocate the view. --- package/cpp/rnskia/RNSkJsiViewApi.h | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/package/cpp/rnskia/RNSkJsiViewApi.h b/package/cpp/rnskia/RNSkJsiViewApi.h index 474629cdc0..8c59ad34b2 100644 --- a/package/cpp/rnskia/RNSkJsiViewApi.h +++ b/package/cpp/rnskia/RNSkJsiViewApi.h @@ -265,10 +265,13 @@ class RNSkJsiViewApi : public JsiHostObject { return; } auto info = getEnsuredCallbackInfo(nativeId); - info->view = view; if (view != nullptr && info->drawCallback != nullptr) { + info->view = view; info->view->setNativeId(nativeId); info->view->setDrawCallback(info->drawCallback); + } else if(view == nullptr && info->drawCallback != nullptr) { + info->view->setDrawCallback(nullptr); + info->view = view; } } From 7f960f92e2e13e4bcfd1bf13b99a7c6eda899ca7 Mon Sep 17 00:00:00 2001 From: chrfalch Date: Wed, 27 Apr 2022 18:32:42 +0200 Subject: [PATCH 09/15] Fixed issue with mount/unmount Skia views. A fix was added to the PR that unregistered the SkiaView when it was unmounted on navigation. We now go back to just set it to nil to free the draw view. We also use a weakPtr to subscribe to notifications, and I also readded back dealloc with unregistration of the skia view. Also added unsubscribing to the notification in dealloc. --- package/ios/RNSkia-iOS/SkiaDrawView.mm | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/package/ios/RNSkia-iOS/SkiaDrawView.mm b/package/ios/RNSkia-iOS/SkiaDrawView.mm index da963132ab..1aba02bf99 100644 --- a/package/ios/RNSkia-iOS/SkiaDrawView.mm +++ b/package/ios/RNSkia-iOS/SkiaDrawView.mm @@ -26,14 +26,18 @@ - (instancetype) initWithManager: (RNSkia::RNSkManager*)manager; _nativeId = 0; _debugMode = false; _drawingMode = RNSkia::RNSkDrawingMode::Default; + // Listen to notifications about module invalidation + __unsafe_unretained SkiaDrawView *weakSelf = self; auto nc = [NSNotificationCenter defaultCenter]; [nc addObserverForName:RCTBridgeWillInvalidateModulesNotification object:nil queue:nil usingBlock:^(NSNotification *notification){ // Remove local variables - self->_manager = nullptr; + if(weakSelf != nullptr) { + weakSelf->_manager = nullptr; + } }]; } return self; @@ -41,16 +45,14 @@ - (instancetype) initWithManager: (RNSkia::RNSkManager*)manager; #pragma mark Lifecycle -- (void) willMoveToWindow:(UIWindow *)newWindow { - [super willMoveToWindow: newWindow]; - +- (void) willMoveToSuperview:(UIView *)newWindow { if (newWindow == NULL) { // Remove implementation view when the parent view is not set if(_impl != nullptr) { [_impl->getLayer() removeFromSuperlayer]; if(_nativeId != 0 && _manager != nullptr) { - _manager->unregisterSkiaDrawView(_nativeId); + _manager->setSkiaDrawView(_nativeId, nullptr); } _impl = nullptr; @@ -64,11 +66,19 @@ - (void) willMoveToWindow:(UIWindow *)newWindow { _manager->setSkiaDrawView(_nativeId, _impl); } _impl->setDrawingMode(_drawingMode); - _impl->setShowDebugOverlays(_debugMode); + _impl->setShowDebugOverlays(_debugMode); } } } +- (void) dealloc { + auto nc = [NSNotificationCenter defaultCenter]; + [nc removeObserver:self name:RCTBridgeWillInvalidateModulesNotification object:nil]; + if(_manager != nullptr && _nativeId != 0) { + _manager->unregisterSkiaDrawView(_nativeId); + } +} + #pragma mark Layout - (void) layoutSubviews { From da7b0a4b253bd704b59764ce65461b1bf64f1f74 Mon Sep 17 00:00:00 2001 From: chrfalch Date: Wed, 27 Apr 2022 20:38:09 +0200 Subject: [PATCH 10/15] Reverted removal of onRemove to do cleanup. --- .../com/shopify/reactnative/skia/RNSkiaViewManager.java | 1 + .../java/com/shopify/reactnative/skia/SkiaDrawView.java | 8 ++++---- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/package/android/src/main/java/com/shopify/reactnative/skia/RNSkiaViewManager.java b/package/android/src/main/java/com/shopify/reactnative/skia/RNSkiaViewManager.java index cd290a6248..6ed1bd76ea 100644 --- a/package/android/src/main/java/com/shopify/reactnative/skia/RNSkiaViewManager.java +++ b/package/android/src/main/java/com/shopify/reactnative/skia/RNSkiaViewManager.java @@ -61,6 +61,7 @@ public void onDropViewInstance(@NonNull SkiaDrawView view) { Integer nativeId = mViewMapping.get(view); skiaModule.getSkiaManager().unregister(nativeId); mViewMapping.remove(view); + view.onRemoved(); } @NonNull diff --git a/package/android/src/main/java/com/shopify/reactnative/skia/SkiaDrawView.java b/package/android/src/main/java/com/shopify/reactnative/skia/SkiaDrawView.java index 3389f66e9b..6d192980b9 100644 --- a/package/android/src/main/java/com/shopify/reactnative/skia/SkiaDrawView.java +++ b/package/android/src/main/java/com/shopify/reactnative/skia/SkiaDrawView.java @@ -23,6 +23,7 @@ public class SkiaDrawView extends TextureView implements TextureView.SurfaceText @DoNotStrip private boolean mIsRemoved = false; + public SkiaDrawView(Context ctx) { super(ctx); RNSkiaModule skiaModule = ((ReactContext)ctx).getNativeModule(RNSkiaModule.class); @@ -36,13 +37,12 @@ public void setBackgroundColor(int color) { // Texture view does not support setting the background color. } - @Override - protected void finalize() throws Throwable { + public void onRemoved() { + mIsRemoved = true; mHybridData.resetNative(); if(mSurface != null) { mSurface.release(); } - super.finalize(); } @Override @@ -100,7 +100,7 @@ public void onSurfaceTextureSizeChanged(SurfaceTexture surface, int width, int h @Override public boolean onSurfaceTextureDestroyed(SurfaceTexture surface) { if(mIsRemoved) { - return true; + return false; } surfaceDestroyed(); // https://developer.android.com/reference/android/view/TextureView.SurfaceTextureListener#onSurfaceTextureDestroyed(android.graphics.SurfaceTexture) From 4bc94d88914957f4306269f8fbb554636a0043fb Mon Sep 17 00:00:00 2001 From: chrfalch Date: Wed, 27 Apr 2022 20:38:55 +0200 Subject: [PATCH 11/15] Reverted removal of storing drawing contexts pr thread This change made the OpenGL context not be thread safe when reloading in debug mode. --- .../cpp/rnskia-android/SkiaOpenGLRenderer.cpp | 80 +++++++++++++------ .../cpp/rnskia-android/SkiaOpenGLRenderer.h | 20 ++++- 2 files changed, 70 insertions(+), 30 deletions(-) diff --git a/package/android/cpp/rnskia-android/SkiaOpenGLRenderer.cpp b/package/android/cpp/rnskia-android/SkiaOpenGLRenderer.cpp index 307c1b1473..97d148ecd4 100644 --- a/package/android/cpp/rnskia-android/SkiaOpenGLRenderer.cpp +++ b/package/android/cpp/rnskia-android/SkiaOpenGLRenderer.cpp @@ -4,10 +4,21 @@ namespace RNSkia { - EGLContext SkiaOpenGLRenderer::glContext = EGL_NO_CONTEXT; - EGLDisplay SkiaOpenGLRenderer::glDisplay = EGL_NO_DISPLAY; - EGLConfig SkiaOpenGLRenderer::glConfig = 0; - sk_sp SkiaOpenGLRenderer::skContext = nullptr; + /** Static members */ + std::shared_ptr SkiaOpenGLRenderer::getThreadDrawingContext() + { + auto threadId = std::this_thread::get_id(); + if (threadContexts.count(threadId) == 0) + { + auto drawingContext = std::make_shared(); + drawingContext->glContext = EGL_NO_CONTEXT; + drawingContext->glDisplay = EGL_NO_DISPLAY; + drawingContext->glConfig = 0; + drawingContext->skContext = nullptr; + threadContexts.emplace(threadId, drawingContext); + } + return threadContexts.at(threadId); + } SkiaOpenGLRenderer::SkiaOpenGLRenderer(ANativeWindow *surface, size_t renderId): _surfaceTexture(surface), @@ -44,7 +55,7 @@ namespace RNSkia { // Reset Skia Context since it might be modified by another Skia View during // rendering. - skContext->resetContext(); + getThreadDrawingContext()->skContext->resetContext(); // Clear with transparent glClearColor(0.0f, 0.0f, 0.0f, 0.0f); @@ -55,9 +66,9 @@ namespace RNSkia // Flush _skSurface->getCanvas()->flush(); - skContext->flush(); + getThreadDrawingContext()->skContext->flush(); - if (!eglSwapBuffers(glDisplay, _glSurface)) + if (!eglSwapBuffers(getThreadDrawingContext()->glDisplay, _glSurface)) { RNSkLogger::logToConsole( "eglSwapBuffers failed: %d\n", eglGetError()); @@ -69,9 +80,9 @@ namespace RNSkia { _renderState = RenderState::Done; - if (_glSurface != EGL_NO_SURFACE && glDisplay != EGL_NO_DISPLAY) + if (_glSurface != EGL_NO_SURFACE && getThreadDrawingContext()->glDisplay != EGL_NO_DISPLAY) { - eglDestroySurface(glDisplay, _glSurface); + eglDestroySurface(getThreadDrawingContext()->glDisplay, _glSurface); } _skSurface = nullptr; @@ -117,13 +128,13 @@ namespace RNSkia bool SkiaOpenGLRenderer::initStaticGLContext() { - if (glContext != EGL_NO_CONTEXT) + if (getThreadDrawingContext()->glContext != EGL_NO_CONTEXT) { return true; } - glDisplay = eglGetDisplay(EGL_DEFAULT_DISPLAY); - if (glDisplay == EGL_NO_DISPLAY) + getThreadDrawingContext()->glDisplay = eglGetDisplay(EGL_DEFAULT_DISPLAY); + if (getThreadDrawingContext()->glDisplay == EGL_NO_DISPLAY) { RNSkLogger::logToConsole("eglGetdisplay failed : %i", glGetError()); return false; @@ -131,7 +142,7 @@ namespace RNSkia EGLint major; EGLint minor; - if (!eglInitialize(glDisplay, &major, &minor)) + if (!eglInitialize(getThreadDrawingContext()->glDisplay, &major, &minor)) { RNSkLogger::logToConsole("eglInitialize failed : %i", glGetError()); return false; @@ -157,8 +168,8 @@ namespace RNSkia EGL_NONE}; EGLint numConfigs; - glConfig = 0; - if (!eglChooseConfig(glDisplay, att, &glConfig, 1, &numConfigs) || + getThreadDrawingContext()->glConfig = 0; + if (!eglChooseConfig(getThreadDrawingContext()->glDisplay, att, &getThreadDrawingContext()->glConfig, 1, &numConfigs) || numConfigs == 0) { RNSkLogger::logToConsole( @@ -168,9 +179,13 @@ namespace RNSkia EGLint contextAttribs[] = {EGL_CONTEXT_CLIENT_VERSION, 2, EGL_NONE}; - glContext = eglCreateContext(glDisplay, glConfig, NULL, contextAttribs); + getThreadDrawingContext()->glContext = eglCreateContext( + getThreadDrawingContext()->glDisplay, + getThreadDrawingContext()->glConfig, + NULL, + contextAttribs); - if (glContext == EGL_NO_CONTEXT) + if (getThreadDrawingContext()->glContext == EGL_NO_CONTEXT) { RNSkLogger::logToConsole( "eglCreateContext failed: %d\n", eglGetError()); @@ -182,15 +197,15 @@ namespace RNSkia bool SkiaOpenGLRenderer::initStaticSkiaContext() { - if (skContext != nullptr) + if (getThreadDrawingContext()->skContext != nullptr) { return true; } // Create the Skia backend context auto backendInterface = GrGLMakeNativeInterface(); - skContext = GrDirectContext::MakeGL(backendInterface); - if (skContext == nullptr) + getThreadDrawingContext()->skContext = GrDirectContext::MakeGL(backendInterface); + if (getThreadDrawingContext()->skContext == nullptr) { RNSkLogger::logToConsole("GrDirectContext::MakeGL failed"); return false; @@ -208,7 +223,11 @@ namespace RNSkia if (_glSurface != EGL_NO_SURFACE) { - if (!eglMakeCurrent(glDisplay, _glSurface, _glSurface, glContext)) + if (!eglMakeCurrent( + getThreadDrawingContext()->glDisplay, + _glSurface, + _glSurface, + getThreadDrawingContext()->glContext)) { RNSkLogger::logToConsole( "eglMakeCurrent failed: %d\n", eglGetError()); @@ -219,7 +238,12 @@ namespace RNSkia // Create the opengl surface _glSurface = - eglCreateWindowSurface(glDisplay, glConfig, _surfaceTexture, nullptr); + eglCreateWindowSurface( + getThreadDrawingContext()->glDisplay, + getThreadDrawingContext()->glConfig, + _surfaceTexture, + nullptr); + if (_glSurface == EGL_NO_SURFACE) { RNSkLogger::logToConsole( @@ -227,7 +251,11 @@ namespace RNSkia return false; } - if (!eglMakeCurrent(glDisplay, _glSurface, _glSurface, glContext)) + if (!eglMakeCurrent( + getThreadDrawingContext()->glDisplay, + _glSurface, + _glSurface, + getThreadDrawingContext()->glContext)) { RNSkLogger::logToConsole("eglMakeCurrent failed: %d\n", eglGetError()); return false; @@ -238,7 +266,7 @@ namespace RNSkia bool SkiaOpenGLRenderer::ensureSkiaSurface(int width, int height) { - if (skContext == nullptr) + if (getThreadDrawingContext()->skContext == nullptr) { return false; } @@ -262,7 +290,7 @@ namespace RNSkia GLint samples; glGetIntegerv(GL_SAMPLES, &samples); - auto maxSamples = skContext->maxSurfaceSampleCountForColorType( + auto maxSamples = getThreadDrawingContext()->skContext->maxSurfaceSampleCountForColorType( kRGBA_8888_SkColorType); if (samples > maxSamples) @@ -276,7 +304,7 @@ namespace RNSkia GrBackendRenderTarget(width, height, samples, stencil, fbInfo); _skSurface = SkSurface::MakeFromBackendRenderTarget( - skContext.get(), + getThreadDrawingContext()->skContext.get(), _skRenderTarget, kBottomLeft_GrSurfaceOrigin, kRGBA_8888_SkColorType, diff --git a/package/android/cpp/rnskia-android/SkiaOpenGLRenderer.h b/package/android/cpp/rnskia-android/SkiaOpenGLRenderer.h index 898ec01377..e4fbe4bd0d 100644 --- a/package/android/cpp/rnskia-android/SkiaOpenGLRenderer.h +++ b/package/android/cpp/rnskia-android/SkiaOpenGLRenderer.h @@ -24,6 +24,16 @@ namespace RNSkia { + using DrawingContext = struct + { + EGLContext glContext; + EGLDisplay glDisplay; + EGLConfig glConfig; + sk_sp skContext; + }; + + static std::unordered_map> threadContexts; + enum RenderState : int { Initializing, Rendering, @@ -96,10 +106,12 @@ namespace RNSkia */ bool ensureSkiaSurface(int width, int height); - static EGLContext glContext; - static EGLDisplay glDisplay; - static EGLConfig glConfig; - static sk_sp skContext; + /** + * To be able to use static contexts (and avoid reloading the skia context for each + * new view, we track the OpenGL and Skia drawing context per thread. + * @return The drawing context for the current thread + */ + static std::shared_ptr getThreadDrawingContext(); EGLSurface _glSurface = EGL_NO_SURFACE; From 4c737c8e44088211af06fe8e19ed56ece7f6fcae Mon Sep 17 00:00:00 2001 From: chrfalch Date: Wed, 27 Apr 2022 21:13:27 +0200 Subject: [PATCH 12/15] Changed the renderer member to unique_ptr --- package/android/cpp/rnskia-android/RNSkDrawViewImpl.cpp | 2 +- package/android/cpp/rnskia-android/RNSkDrawViewImpl.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/package/android/cpp/rnskia-android/RNSkDrawViewImpl.cpp b/package/android/cpp/rnskia-android/RNSkDrawViewImpl.cpp index 4a8d28733f..31b2257dfc 100644 --- a/package/android/cpp/rnskia-android/RNSkDrawViewImpl.cpp +++ b/package/android/cpp/rnskia-android/RNSkDrawViewImpl.cpp @@ -22,7 +22,7 @@ namespace RNSkia { if (_renderer == nullptr) { // Create renderer! - _renderer = std::make_shared(surface, getNativeId()); + _renderer = std::make_unique(surface, getNativeId()); // Redraw requestRedraw(); diff --git a/package/android/cpp/rnskia-android/RNSkDrawViewImpl.h b/package/android/cpp/rnskia-android/RNSkDrawViewImpl.h index b841e64e99..c0eb1c1aaa 100644 --- a/package/android/cpp/rnskia-android/RNSkDrawViewImpl.h +++ b/package/android/cpp/rnskia-android/RNSkDrawViewImpl.h @@ -36,7 +36,7 @@ namespace RNSkia { private: bool createSkiaSurface(); - std::shared_ptr _renderer = nullptr; + std::unique_ptr _renderer = nullptr; int _nativeId; int _width = -1; From 02f3ea168f0dd12a06c6a9cd6324db340d332288 Mon Sep 17 00:00:00 2001 From: chrfalch Date: Thu, 28 Apr 2022 12:01:44 +0200 Subject: [PATCH 13/15] Changes to releasing the underlying surface after CodeReview Added a callback to release the surface from the Android TextureView at the right time. --- package/android/cpp/jni/JniSkiaDrawView.cpp | 9 ++++-- .../android/cpp/jni/include/JniSkiaDrawView.h | 10 +++--- .../cpp/rnskia-android/RNSkDrawViewImpl.cpp | 7 +++-- .../cpp/rnskia-android/RNSkDrawViewImpl.h | 5 ++- .../reactnative/skia/RNSkiaViewManager.java | 1 - .../reactnative/skia/SkiaDrawView.java | 31 +++++++------------ 6 files changed, 32 insertions(+), 31 deletions(-) diff --git a/package/android/cpp/jni/JniSkiaDrawView.cpp b/package/android/cpp/jni/JniSkiaDrawView.cpp index c5ca50643e..45938d6013 100644 --- a/package/android/cpp/jni/JniSkiaDrawView.cpp +++ b/package/android/cpp/jni/JniSkiaDrawView.cpp @@ -31,9 +31,6 @@ namespace RNSkia /**** DTOR ***/ JniSkiaDrawView::~JniSkiaDrawView() { -#if LOG_ALL_DRAWING - RNSkLogger::logToConsole("JniSkiaDrawView::~JniSkiaDrawView %i", getNativeId()); -#endif } /**** JNI ****/ @@ -106,4 +103,10 @@ namespace RNSkia { _drawView->surfaceDestroyed(); } + + void JniSkiaDrawView::releaseSurface() { + jni::ThreadScope ts; + static auto method = javaPart_->getClass()->getMethod("releaseSurface"); + method(javaPart_.get()); + } } // namespace RNSkia diff --git a/package/android/cpp/jni/include/JniSkiaDrawView.h b/package/android/cpp/jni/include/JniSkiaDrawView.h index 8cacab8294..5a04661732 100644 --- a/package/android/cpp/jni/include/JniSkiaDrawView.h +++ b/package/android/cpp/jni/include/JniSkiaDrawView.h @@ -50,9 +50,9 @@ namespace RNSkia ~JniSkiaDrawView(); - std::shared_ptr getDrawViewImpl() { - return _drawView; - } + std::shared_ptr getDrawViewImpl() { return _drawView; } + + void releaseSurface(); protected: void setMode(std::string mode); @@ -69,7 +69,9 @@ namespace RNSkia jni::alias_ref jThis, JavaSkiaManager skiaManager) : javaPart_(jni::make_global(jThis)), - _drawView(std::make_shared(skiaManager->cthis()->getPlatformContext())) { + _drawView(std::make_shared(skiaManager->cthis()->getPlatformContext(), [this]() { + releaseSurface(); + })) { } }; diff --git a/package/android/cpp/rnskia-android/RNSkDrawViewImpl.cpp b/package/android/cpp/rnskia-android/RNSkDrawViewImpl.cpp index 31b2257dfc..1ffc6c139e 100644 --- a/package/android/cpp/rnskia-android/RNSkDrawViewImpl.cpp +++ b/package/android/cpp/rnskia-android/RNSkDrawViewImpl.cpp @@ -11,9 +11,9 @@ #include namespace RNSkia { - RNSkDrawViewImpl::RNSkDrawViewImpl(std::shared_ptr context) : - RNSkia::RNSkDrawView(context) { - } + RNSkDrawViewImpl::RNSkDrawViewImpl(std::shared_ptr context, std::function releaseSurfaceCallback) : + RNSkia::RNSkDrawView(context), + _releaseSurfaceCallback(std::move(releaseSurfaceCallback)) {} void RNSkDrawViewImpl::surfaceAvailable(ANativeWindow* surface, int width, int height) { _width = width; @@ -46,6 +46,7 @@ namespace RNSkia { } // Remove renderer drawViewImpl->_renderer = nullptr; + drawViewImpl->_releaseSurfaceCallback(); } }); } diff --git a/package/android/cpp/rnskia-android/RNSkDrawViewImpl.h b/package/android/cpp/rnskia-android/RNSkDrawViewImpl.h index c0eb1c1aaa..1044974d29 100644 --- a/package/android/cpp/rnskia-android/RNSkDrawViewImpl.h +++ b/package/android/cpp/rnskia-android/RNSkDrawViewImpl.h @@ -16,7 +16,8 @@ namespace RNSkia { class RNSkDrawViewImpl : public RNSkia::RNSkDrawView { public: - RNSkDrawViewImpl(std::shared_ptr context); + RNSkDrawViewImpl(std::shared_ptr context, + std::function releaseSurfaceCallback); void surfaceAvailable(ANativeWindow* surface, int, int); void surfaceDestroyed(); @@ -41,5 +42,7 @@ namespace RNSkia { int _nativeId; int _width = -1; int _height = -1; + + std::function _releaseSurfaceCallback; }; } diff --git a/package/android/src/main/java/com/shopify/reactnative/skia/RNSkiaViewManager.java b/package/android/src/main/java/com/shopify/reactnative/skia/RNSkiaViewManager.java index 6ed1bd76ea..cd290a6248 100644 --- a/package/android/src/main/java/com/shopify/reactnative/skia/RNSkiaViewManager.java +++ b/package/android/src/main/java/com/shopify/reactnative/skia/RNSkiaViewManager.java @@ -61,7 +61,6 @@ public void onDropViewInstance(@NonNull SkiaDrawView view) { Integer nativeId = mViewMapping.get(view); skiaModule.getSkiaManager().unregister(nativeId); mViewMapping.remove(view); - view.onRemoved(); } @NonNull diff --git a/package/android/src/main/java/com/shopify/reactnative/skia/SkiaDrawView.java b/package/android/src/main/java/com/shopify/reactnative/skia/SkiaDrawView.java index 6d192980b9..f6fa47cc65 100644 --- a/package/android/src/main/java/com/shopify/reactnative/skia/SkiaDrawView.java +++ b/package/android/src/main/java/com/shopify/reactnative/skia/SkiaDrawView.java @@ -18,11 +18,8 @@ public class SkiaDrawView extends TextureView implements TextureView.SurfaceText @DoNotStrip private HybridData mHybridData; - private Surface mSurface; - @DoNotStrip - private boolean mIsRemoved = false; - + private Surface mSurface; public SkiaDrawView(Context ctx) { super(ctx); @@ -37,19 +34,24 @@ public void setBackgroundColor(int color) { // Texture view does not support setting the background color. } - public void onRemoved() { - mIsRemoved = true; - mHybridData.resetNative(); + public void releaseSurface() { if(mSurface != null) { mSurface.release(); + mSurface = null; } + // This shouldn't need to be here... TODO: if here it releases underlying native views and finalize is called, but remounting a view crashes. + // If in finalize: Everything works but finalize is never called and underlying native views are never freed. FIX, you stupid man. + mHybridData.resetNative(); + } + + @Override + protected void finalize() throws Throwable { + releaseSurface(); + super.finalize(); } @Override public boolean onTouchEvent(MotionEvent ev) { - if(mIsRemoved) { - return false; - } int action = ev.getAction(); int count = ev.getPointerCount(); MotionEvent.PointerCoords r = new MotionEvent.PointerCoords(); @@ -82,26 +84,17 @@ public boolean onTouchEvent(MotionEvent ev) { @Override public void onSurfaceTextureAvailable(SurfaceTexture surface, int width, int height) { - if(mIsRemoved) { - return; - } mSurface = new Surface(surface); surfaceAvailable(mSurface, width, height); } @Override public void onSurfaceTextureSizeChanged(SurfaceTexture surface, int width, int height) { - if(mIsRemoved) { - return; - } surfaceSizeChanged(width, height); } @Override public boolean onSurfaceTextureDestroyed(SurfaceTexture surface) { - if(mIsRemoved) { - return false; - } surfaceDestroyed(); // https://developer.android.com/reference/android/view/TextureView.SurfaceTextureListener#onSurfaceTextureDestroyed(android.graphics.SurfaceTexture) // Invoked when the specified SurfaceTexture is about to be destroyed. If returns true, From d86bc287bdba8b76a6d0a76f6b31c35ba654831f Mon Sep 17 00:00:00 2001 From: chrfalch Date: Thu, 28 Apr 2022 15:56:19 +0200 Subject: [PATCH 14/15] Moved Platform context implenentation on Android Moved to rnsk-android folder to make sure jni folder only contains jni classes. --- package/android/cpp/jni/include/JniSkiaManager.h | 8 ++++---- .../RNSkPlatformContextImpl.h} | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) rename package/android/cpp/{jni/include/JniPlatformContextWrapper.h => rnskia-android/RNSkPlatformContextImpl.h} (80%) diff --git a/package/android/cpp/jni/include/JniSkiaManager.h b/package/android/cpp/jni/include/JniSkiaManager.h index e35e4c2eb0..7624686774 100644 --- a/package/android/cpp/jni/include/JniSkiaManager.h +++ b/package/android/cpp/jni/include/JniSkiaManager.h @@ -7,7 +7,7 @@ #include #include -#include +#include #include @@ -48,14 +48,14 @@ class JniSkiaManager : public jni::HybridClass { : _javaPart(jni::make_global(jThis)), _jsRuntime(runtime), _jsCallInvoker(jsCallInvoker), - _context(std::make_shared(platformContext, runtime, jsCallInvoker)) { + _context(std::make_shared(platformContext, runtime, jsCallInvoker)) { } void registerSkiaView(int viewTag, JniSkiaDrawView *skiaView); void unregisterSkiaView(int viewTag); - std::shared_ptr getPlatformContext() { return _context; } + std::shared_ptr getPlatformContext() { return _context; } void invalidate() { _context->stopDrawLoop(); @@ -73,7 +73,7 @@ class JniSkiaManager : public jni::HybridClass { jsi::Runtime *_jsRuntime; std::shared_ptr _jsCallInvoker; - std::shared_ptr _context; + std::shared_ptr _context; void initializeRuntime(); }; diff --git a/package/android/cpp/jni/include/JniPlatformContextWrapper.h b/package/android/cpp/rnskia-android/RNSkPlatformContextImpl.h similarity index 80% rename from package/android/cpp/jni/include/JniPlatformContextWrapper.h rename to package/android/cpp/rnskia-android/RNSkPlatformContextImpl.h index dee92d5e82..4ab7e12668 100644 --- a/package/android/cpp/jni/include/JniPlatformContextWrapper.h +++ b/package/android/cpp/rnskia-android/RNSkPlatformContextImpl.h @@ -11,11 +11,11 @@ namespace RNSkia { using namespace facebook; - class JniPlatformContextWrapper: public RNSkPlatformContext { + class RNSkPlatformContextImpl: public RNSkPlatformContext { public: - JniPlatformContextWrapper(JniPlatformContext* jniPlatformContext, - jsi::Runtime *runtime, - std::shared_ptr jsCallInvoker) : + RNSkPlatformContextImpl(JniPlatformContext* jniPlatformContext, + jsi::Runtime *runtime, + std::shared_ptr jsCallInvoker) : RNSkPlatformContext(runtime, jsCallInvoker, jniPlatformContext->getPixelDensity()), From eb425cdaf66307313e10de92f6874b181135cc33 Mon Sep 17 00:00:00 2001 From: chrfalch Date: Thu, 28 Apr 2022 16:15:42 +0200 Subject: [PATCH 15/15] Fixed issue with releasing the native part of the RNSkDrawView implementation in a determenistic way. --- .../reactnative/skia/RNSkiaViewManager.java | 1 + .../shopify/reactnative/skia/SkiaDrawView.java | 18 +++++++++++------- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/package/android/src/main/java/com/shopify/reactnative/skia/RNSkiaViewManager.java b/package/android/src/main/java/com/shopify/reactnative/skia/RNSkiaViewManager.java index cd290a6248..11f37c51a5 100644 --- a/package/android/src/main/java/com/shopify/reactnative/skia/RNSkiaViewManager.java +++ b/package/android/src/main/java/com/shopify/reactnative/skia/RNSkiaViewManager.java @@ -61,6 +61,7 @@ public void onDropViewInstance(@NonNull SkiaDrawView view) { Integer nativeId = mViewMapping.get(view); skiaModule.getSkiaManager().unregister(nativeId); mViewMapping.remove(view); + view.onViewRemoved(); } @NonNull diff --git a/package/android/src/main/java/com/shopify/reactnative/skia/SkiaDrawView.java b/package/android/src/main/java/com/shopify/reactnative/skia/SkiaDrawView.java index f6fa47cc65..d097cf0e28 100644 --- a/package/android/src/main/java/com/shopify/reactnative/skia/SkiaDrawView.java +++ b/package/android/src/main/java/com/shopify/reactnative/skia/SkiaDrawView.java @@ -18,6 +18,9 @@ public class SkiaDrawView extends TextureView implements TextureView.SurfaceText @DoNotStrip private HybridData mHybridData; + @DoNotStrip + private boolean mViewRemoved; + @DoNotStrip private Surface mSurface; @@ -39,15 +42,16 @@ public void releaseSurface() { mSurface.release(); mSurface = null; } - // This shouldn't need to be here... TODO: if here it releases underlying native views and finalize is called, but remounting a view crashes. - // If in finalize: Everything works but finalize is never called and underlying native views are never freed. FIX, you stupid man. - mHybridData.resetNative(); + // We can only reset the native side when the view was removed from screen. + // releasing the surface can also be done when the view is hidden and then + // we should only release the surface - and keep the native part around. + if(mViewRemoved) { + mHybridData.resetNative(); + } } - @Override - protected void finalize() throws Throwable { - releaseSurface(); - super.finalize(); + void onViewRemoved() { + mViewRemoved = true; } @Override