Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(🧵): thread safety in RNSkJsiViewApi.h #2481

Merged
merged 4 commits into from
Jun 14, 2024
Merged

fix(🧵): thread safety in RNSkJsiViewApi.h #2481

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e1173f0
remove callJsiMethod
wcandillon Jun 14, 2024
5398d7e
fix(🧵): thread safety in RNSkJsiViewApi.h
wcandillon Jun 14, 2024
96e2217
Fix thread safety in RNSkJsiViewApi.h
wcandillon Jun 14, 2024
fd58f1a
Merge branch 'main' into JsiView
wcandillon Jun 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
92 changes: 25 additions & 67 deletions package/cpp/rnskia/RNSkJsiViewApi.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,10 +51,11 @@ class RNSkJsiViewApi : public RNJsi::JsiHostObject,

return jsi::Value::undefined();
}

auto nativeId = arguments[0].asNumber();
std::lock_guard<std::mutex> lock(_mutex);
auto info = getEnsuredViewInfo(nativeId);

std::lock_guard<std::mutex> lock(_mutex);
info->props.insert_or_assign(arguments[1].asString(runtime).utf8(runtime),
RNJsi::JsiValueWrapper(runtime, arguments[2]));

Expand All @@ -69,52 +70,6 @@ class RNSkJsiViewApi : public RNJsi::JsiHostObject,
return jsi::Value::undefined();
}

/**
Calls a custom command / method on a view by the view id.
*/
JSI_HOST_FUNCTION(callJsiMethod) {
if (count < 2) {
_platformContext->raiseError(
std::string("callCustomCommand: Expected at least 2 arguments, got " +
std::to_string(count) + "."));

return jsi::Value::undefined();
}

if (!arguments[0].isNumber()) {
_platformContext->raiseError(
"callCustomCommand: First argument must be a number");

return jsi::Value::undefined();
}

if (!arguments[1].isString()) {
_platformContext->raiseError("callCustomCommand: Second argument must be "
"the name of the action to call.");

return jsi::Value::undefined();
}

auto nativeId = arguments[0].asNumber();
auto action = arguments[1].asString(runtime).utf8(runtime);

auto info = getEnsuredViewInfo(nativeId);

if (info->view == nullptr) {
throw jsi::JSError(
runtime, std::string("callCustomCommand: Could not call action " +
action + " on view - view not ready.")
.c_str());

return jsi::Value::undefined();
}

// Get arguments
size_t paramsCount = count - 2;
const jsi::Value *params = paramsCount > 0 ? &arguments[2] : nullptr;
return info->view->callJsiMethod(runtime, action, params, paramsCount);
}

JSI_HOST_FUNCTION(requestRedraw) {
if (count != 1) {
_platformContext->raiseError(
Expand All @@ -133,7 +88,7 @@ class RNSkJsiViewApi : public RNJsi::JsiHostObject,

// find Skia View
int nativeId = arguments[0].asNumber();

std::lock_guard<std::mutex> lock(_mutex);
auto info = getEnsuredViewInfo(nativeId);
if (info->view != nullptr) {
info->view->requestRedraw();
Expand All @@ -158,13 +113,18 @@ class RNSkJsiViewApi : public RNJsi::JsiHostObject,
// find Skia view
int nativeId = arguments[0].asNumber();
sk_sp<SkImage> image;
auto info = getEnsuredViewInfo(nativeId);
if (info->view != nullptr) {
std::shared_ptr<RNSkView> view;
{
std::lock_guard<std::mutex> lock(_mutex);
auto info = getEnsuredViewInfo(nativeId);
view = info->view;
}
if (view != nullptr) {
if (count > 1 && !arguments[1].isUndefined() && !arguments[1].isNull()) {
auto rect = JsiSkRect::fromValue(runtime, arguments[1]);
image = info->view->makeImageSnapshot(rect.get());
image = view->makeImageSnapshot(rect.get());
} else {
image = info->view->makeImageSnapshot(nullptr);
image = view->makeImageSnapshot(nullptr);
}
if (image == nullptr) {
throw jsi::JSError(runtime,
Expand Down Expand Up @@ -194,20 +154,25 @@ class RNSkJsiViewApi : public RNJsi::JsiHostObject,

// find Skia view
int nativeId = arguments[0].asNumber();
auto info = getEnsuredViewInfo(nativeId);
std::shared_ptr<RNSkView> view;
{
std::lock_guard<std::mutex> lock(_mutex);
auto info = getEnsuredViewInfo(nativeId);
view = info->view;
}
auto context = _platformContext;
auto bounds =
count > 1 && !arguments[1].isUndefined() && !arguments[1].isNull()
? JsiSkRect::fromValue(runtime, arguments[1])
: nullptr;
return RNJsi::JsiPromises::createPromiseAsJSIValue(
runtime, [context = std::move(context), info, bounds](
runtime, [context = std::move(context), view, bounds](
jsi::Runtime &runtime,
std::shared_ptr<RNJsi::JsiPromises::Promise> promise) {
context->runOnMainThread([&runtime, info = std::move(info),
context->runOnMainThread([&runtime, view = std::move(view),
promise = std::move(promise),
context = std::move(context), bounds]() {
auto image = info->view->makeImageSnapshot(
auto image = view->makeImageSnapshot(
bounds == nullptr ? nullptr : bounds.get());
context->runOnJavascriptThread(
[&runtime, context = std::move(context),
Expand All @@ -225,7 +190,6 @@ class RNSkJsiViewApi : public RNJsi::JsiHostObject,
}

JSI_EXPORT_FUNCTIONS(JSI_EXPORT_FUNC(RNSkJsiViewApi, setJsiProperty),
JSI_EXPORT_FUNC(RNSkJsiViewApi, callJsiMethod),
JSI_EXPORT_FUNC(RNSkJsiViewApi, requestRedraw),
JSI_EXPORT_FUNC(RNSkJsiViewApi, makeImageSnapshotAsync),
JSI_EXPORT_FUNC(RNSkJsiViewApi, makeImageSnapshot))
Expand All @@ -237,21 +201,16 @@ class RNSkJsiViewApi : public RNJsi::JsiHostObject,
explicit RNSkJsiViewApi(std::shared_ptr<RNSkPlatformContext> platformContext)
: JsiHostObject(), _platformContext(platformContext) {}

/**
* Invalidates the Skia View Api object
*/
void invalidate() { unregisterAll(); }

/**
Call to remove all draw view infos
*/
void unregisterAll() {
std::lock_guard<std::mutex> lock(_mutex);
// Unregister all views
auto tempList = _viewInfos;
for (const auto &info : tempList) {
unregisterSkiaView(info.first);
}
std::lock_guard<std::mutex> lock(_mutex);
_viewInfos.clear();
}

Expand All @@ -261,8 +220,8 @@ class RNSkJsiViewApi : public RNJsi::JsiHostObject,
* @param view View to register
*/
void registerSkiaView(size_t nativeId, std::shared_ptr<RNSkView> view) {
auto info = getEnsuredViewInfo(nativeId);
std::lock_guard<std::mutex> lock(_mutex);
auto info = getEnsuredViewInfo(nativeId);
info->view = view;
info->view->setNativeId(nativeId);
info->view->setJsiProperties(info->props);
Expand All @@ -274,12 +233,12 @@ class RNSkJsiViewApi : public RNJsi::JsiHostObject,
* @param nativeId View id
*/
void unregisterSkiaView(size_t nativeId) {
std::lock_guard<std::mutex> lock(_mutex);
if (_viewInfos.count(nativeId) == 0) {
return;
}
auto info = getEnsuredViewInfo(nativeId);

std::lock_guard<std::mutex> lock(_mutex);
info->view = nullptr;
_viewInfos.erase(nativeId);
}
Expand All @@ -291,11 +250,11 @@ class RNSkJsiViewApi : public RNJsi::JsiHostObject,
or a valid view, effectively toggling the view's availability.
*/
void setSkiaView(size_t nativeId, std::shared_ptr<RNSkView> view) {
std::lock_guard<std::mutex> lock(_mutex);
if (_viewInfos.find(nativeId) == _viewInfos.end()) {
return;
}
auto info = getEnsuredViewInfo(nativeId);
std::lock_guard<std::mutex> lock(_mutex);
if (view != nullptr) {
info->view = view;
info->view->setNativeId(nativeId);
Expand All @@ -315,7 +274,6 @@ class RNSkJsiViewApi : public RNJsi::JsiHostObject,
RNSkViewInfo *getEnsuredViewInfo(size_t nativeId) {
if (_viewInfos.count(nativeId) == 0) {
RNSkViewInfo info;
std::lock_guard<std::mutex> lock(_mutex);
_viewInfos.emplace(nativeId, info);
}
return &_viewInfos.at(nativeId);
Expand Down
2 changes: 1 addition & 1 deletion package/cpp/rnskia/RNSkManager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ void RNSkManager::invalidate() {
_isInvalidated = true;

// Invalidate members
_viewApi->invalidate();
_viewApi->unregisterAll();
_platformContext->invalidate();
}

Expand Down
11 changes: 0 additions & 11 deletions package/cpp/rnskia/RNSkView.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,17 +173,6 @@ class RNSkView : public std::enable_shared_from_this<RNSkView> {
// Nothing here...
}

/**
Calls a custom action.
*/
virtual jsi::Value callJsiMethod(jsi::Runtime &runtime,
const std::string &name,
const jsi::Value *arguments, size_t count) {
throw std::runtime_error(
"The base Skia View does not support any commands. Command " + name +
" not found.");
}

/**
* Repaints the Skia view using the underlying context and the drawcallback.
* This method schedules a draw request that will be run on the correct
Expand Down
1 change: 0 additions & 1 deletion package/src/views/SkiaDomView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,6 @@ const assertSkiaViewApi = () => {
if (
SkiaViewApi === null ||
SkiaViewApi.setJsiProperty === null ||
SkiaViewApi.callJsiMethod === null ||
SkiaViewApi.requestRedraw === null ||
SkiaViewApi.makeImageSnapshot === null
) {
Expand Down
1 change: 0 additions & 1 deletion package/src/views/SkiaJSDomView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,6 @@ const assertSkiaViewApi = () => {
if (
SkiaViewApi === null ||
SkiaViewApi.setJsiProperty === null ||
SkiaViewApi.callJsiMethod === null ||
SkiaViewApi.requestRedraw === null ||
SkiaViewApi.makeImageSnapshot === null
) {
Expand Down
1 change: 0 additions & 1 deletion package/src/views/SkiaPictureView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,6 @@ const assertSkiaViewApi = () => {
if (
SkiaViewApi === null ||
SkiaViewApi.setJsiProperty === null ||
SkiaViewApi.callJsiMethod === null ||
SkiaViewApi.requestRedraw === null ||
SkiaViewApi.makeImageSnapshot === null
) {
Expand Down
13 changes: 0 additions & 13 deletions package/src/views/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,21 +48,8 @@ export type TouchHandlers = {

export type TouchHandler = (touchInfo: Array<Array<TouchInfo>>) => void;

/**
* Listener interface for value changes
*/
export interface ValueListener {
addListener: (callback: () => void) => number;
removeListener: (id: number) => void;
}

export interface ISkiaViewApi {
setJsiProperty: <T>(nativeId: number, name: string, value: T) => void;
callJsiMethod: <T extends Array<unknown>>(
nativeId: number,
name: string,
...args: T
) => void;
requestRedraw: (nativeId: number) => void;
makeImageSnapshot: (nativeId: number, rect?: SkRect) => SkImage;
makeImageSnapshotAsync: (nativeId: number, rect?: SkRect) => Promise<SkImage>;
Expand Down
Loading