Conversation
| @@ -26,7 +27,12 @@ | |||
| "backgroundColor": "#ffffff" | |||
| }, | |||
| "package": "com.anonymous.computervision", | |||
| "permissions": ["android.permission.CAMERA"] | |||
| "permissions": [ | |||
| "android.permission.CAMERA", | |||
| "android.permission.INTERNET", | |||
| "android.permission.RECORD_AUDIO", | |||
| "android.permission.ACCESS_NETWORK_STATE" | |||
| ] | |||
There was a problem hiding this comment.
Can't remember if all of those are needed, but I had some issues with webrtc without those.
There was a problem hiding this comment.
Wowow, wait a minute, what computer vision app.json file has to do with the webrtc, completely separate package?
msluszniak
left a comment
There was a problem hiding this comment.
Remaining items:
- EMA state race + process-global model state — line on
FrameProcessorBridge.cpp. - iOS sibling races + singleton design — line on
ExecutorchFrameProcessor.mm. - Monorepo header path — line on the podspec.
Also — should we add a section about this package to the documentation?
|
|
||
| // Mask post-processing state (EMA temporal smoothing). Touched only on the | ||
| // capture thread; unload resets it. | ||
| cv::Mat g_previousMask; |
There was a problem hiding this comment.
The LoadedModel+mutex+shared_ptr snapshot doesn't cover g_previousMask / g_hasHistory. Capture thread reads/writes them at L158–165; unloadModel resets them at L216–217 from a different thread — cv::Mat isn't thread-safe for concurrent release/assignment, so an in-flight frame races with release(). The L41–42 "Touched only on the capture thread" comment contradicts itself.
Fix: hoist to a per-instance native handle (loadModel returns a jlong, others take it back) with the EMA state inside LoadedModel so the existing snapshot covers it.
| }); | ||
| } | ||
|
|
||
| - (void)setBlurRadius:(float)blurRadius { |
There was a problem hiding this comment.
Several cross-thread accesses without synchronization, all rooted in +sharedInstance:
_blurRadius(L122 vs L242): plain ivar across JS/capture threads.std::atomic<float>is the one-liner. Android handles this via@Volatile pendingBlurRadius._isProcessing(L165–177): plain BOOL, safe only because WebRTC happens to deliver frames serially._lastProcessedFrame: raw ivar — concurrent ARC stores from capture queue (L181) vs unload nil-out (L136) can over-release._outputPool: released in unload (L139) while capture queue may be reading it. UAF window._previousMask— see Android comment.
All collapse when state moves off +sharedInstance onto per-instance ivars with a serial queue (or os_unfair_lock). The singleton also blocks multi-track / camera-switch.
| # react-native-executorch exposes rnexecutorch/* headers via its header_dir. | ||
| # However, executorch SDK headers and internal headers don't propagate to | ||
| # dependent pods, so we need to add them here. | ||
| rne_path = '${PODS_ROOT}/../../node_modules/react-native-executorch' |
There was a problem hiding this comment.
${PODS_ROOT}/../../node_modules/react-native-executorch assumes a flat layout. In yarn/pnpm workspaces node_modules hoists to the workspace root, so the path resolves to nowhere and the executorch SDK headers aren't found — breaks any workspace consumer, including this repo's own apps.
Two options:
rne_path = ['../node_modules/react-native-executorch',
'../../node_modules/react-native-executorch',
'../../../node_modules/react-native-executorch']
.map { |p| File.expand_path(p, __dir__) }
.find { |p| File.exist?(p) }Or better, expose third-party/include/ and common/ from react-native-executorch.podspec as public_header_files, removing the need for any HEADER_SEARCH_PATHS workaround here. The L18–20 comment already admits this is the design issue.
|
Also please resolve conflicts. I tested this PR on Android and it worked well. I will be glad if someone test it on iOS as well. |
Description
Adds react-native-executorch-webrtc package for real-time background blur in Fishjam WebRTC video calls using on-device ExecuTorch segmentation models.
Key features:
Architecture:
Introduces a breaking change?
Type of change
Tested on
Testing instructions
You'll need to setup your fishjam account, and verify this example works properly:
Screenshots
Related issues
Checklist
Additional notes