Conversation
|
Warning Rate limit exceeded@gmemmy has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 17 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (26)
WalkthroughReplaces the legacy VisionRtc TurboModule with a new VisionRTC implementation across iOS (Swift + ObjC++ bridge) and Android (Kotlin); adds WebRTC dependencies and build config changes; implements synthetic camera sources, per-track lifecycle and stats APIs; updates TypeScript bridge, package/build configs, example app, and removes a local MCP config file. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor JS as React Native JS
participant Bridge as TurboModule / NativeVisionRTC
participant Native as iOS Swift / Android Kotlin
participant WebRTC as WebRTC factory/capturer
JS->>Bridge: createVisionCameraSource(viewTag)
Bridge->>Native: createVisionCameraSource(viewTag)
Native-->>Bridge: { __nativeSourceId }
Bridge-->>JS: { __nativeSourceId }
JS->>Bridge: createTrack(source, opts)
Bridge->>Native: createTrack(source, opts)
Native->>WebRTC: init factories & create capturer/video source
WebRTC-->>Native: video track ready
Native-->>Bridge: { trackId }
Bridge-->>JS: { trackId }
rect rgba(220,245,230,0.5)
note over Native,WebRTC: Scheduled synthetic frames delivered to track at target FPS
WebRTC->>Native: deliver frame
end
JS->>Bridge: getStats()
Bridge->>Native: getStats()
Native-->>Bridge: { fps, droppedFrames }
Bridge-->>JS: stats
JS->>Bridge: disposeTrack(trackId)
Bridge->>Native: disposeTrack(trackId)
Native->>WebRTC: stop & release resources
Native-->>Bridge: ack
Bridge-->>JS: done
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
android/src/main/java/com/visionrtc/VisionRtcPackage.kt (1)
22-29: Fix ReactModuleInfo: use fully qualified class name and 6-arg constructor
Replace the second argument withVisionRTCModule::class.java.nameand call the 6-parameter constructor (thehasConstantsoverload was removed in RN ≥ 0.74).- moduleInfos[VisionRTCModule.NAME] = ReactModuleInfo( - VisionRTCModule.NAME, - VisionRTCModule.NAME, - false, // canOverrideExistingModule - false, // needsEagerInit - false, // isCxxModule - true // isTurboModule - ) + moduleInfos[VisionRTCModule.NAME] = ReactModuleInfo( + VisionRTCModule.NAME, + VisionRTCModule::class.java.name, + false, // canOverrideExistingModule + false, // needsEagerInit + false, // isCxxModule + true // isTurboModule + )
🧹 Nitpick comments (20)
example/ios/VisionRtcExample.xcodeproj/project.pbxproj (2)
14-14: Privacy manifest added; verify contents and Info.plist usage descriptions.Nice addition. Please confirm:
- PrivacyInfo.xcprivacy includes accurate data collection/use declarations for camera/microphone.
- Info.plist contains NSCameraUsageDescription and NSMicrophoneUsageDescription strings aligned with the example app behavior.
I can draft a minimal PrivacyInfo.xcprivacy and Info.plist usage strings if you want.
Also applies to: 163-164
276-276: Unquoted bundle identifier is fine; keep style consistent.No functional difference, but prefer consistent quoting across configs to minimize churn in future diffs.
Also applies to: 303-303
src/types.ts (3)
1-1: Resolution type looks good.Consider documenting that width/height are pixels and expected to be positive integers.
3-9: Clarify units and expected ranges in TrackOptions.Add concise comments for units/defaults; this avoids ambiguity (e.g., bitrate units).
Apply this diff:
export type TrackOptions = { - fps?: number; // default 30 - resolution?: Resolution; - bitrate?: number; - colorSpace?: 'auto' | 'sRGB' | 'BT.709' | 'BT.2020'; - orientationMode?: 'auto' | 'fixed-0' | 'fixed-90' | 'fixed-180' | 'fixed-270'; + fps?: number; // frames per second (default 30) + resolution?: Resolution; // pixels + bitrate?: number; // bits per second + colorSpace?: 'auto' | 'sRGB' | 'BT.709' | 'BT.2020'; // platform-mapped + orientationMode?: 'auto' | 'fixed-0' | 'fixed-90' | 'fixed-180' | 'fixed-270'; // degrees };
11-11: Make the native source id read-only to discourage mutation.Apply this diff:
-export type VisionCameraSource = {__nativeSourceId: string}; +export type VisionCameraSource = {readonly __nativeSourceId: string};android/src/main/java/com/visionrtc/VisionRtcModule.kt (2)
63-83: Optional: validate TrackOptions (bounds/clamps).Clamp fps to a sane range (e.g., 1–120) and validate resolution to prevent huge allocations.
Apply this diff:
- val fps = (opts?.getDouble("fps") ?: 30.0).roundToInt() + val fps = ((opts?.getDouble("fps") ?: 30.0).roundToInt()).coerceIn(1, 120)
56-61: Unimplemented/stubbed APIs and unused inputs—confirm intended scaffolding.
- createVisionCameraSource ignores viewTag (expected for scaffolding?).
- createTrack ignores the provided source for now.
- replaceSenderTrack is a no-op.
Want me to wire these through to actual camera capture and sender replacement next? I can draft the follow-up tasks.
Also applies to: 63-68, 85-87
android/src/main/java/com/visionrtc/VisionRtcPackage.kt (1)
10-10: File/class casing mismatch.The file is VisionRtcPackage.kt but the class is VisionRTCPackage. Consider renaming the file to match the class for consistency.
example/ios/Podfile (1)
28-28: Pin or avoid deprecated GoogleWebRTC pod; consider future-proofing.
- GoogleWebRTC is deprecated; last release is 1.1.32000 (Mar 2023). Pin it or switch to a maintained distribution (xcframework/SPM or alternative pod), and avoid duplicating if the library podspec already depends on it. CocoaPods trunk is moving to read-only in 2026; prefer vendoring/SPM long-term.
Option A (pin now):
- pod 'GoogleWebRTC' + pod 'GoogleWebRTC', '1.1.32000'Option B (rely on podspec transitive dep — remove here):
- pod 'GoogleWebRTC'Verify duplication:
#!/bin/bash # Check if the library podspec already declares GoogleWebRTC fd -a -t f -p 'podspec' | xargs -I{} rg -n "GoogleWebRTC" {}Refs: deprecation and last release; CocoaPods trunk plan. (cocoapods.org, blog.cocoapods.org)
example/src/App.tsx (4)
27-33: Make Stop idempotent and resilient.Always return if nothing to stop; ensure cleanup even if dispose fails.
const onStop = async () => { - if (trackId) { - await disposeTrack(trackId); - setTrackId(null); - setStats(null); - } + if (!trackId) return; + try { + await disposeTrack(trackId); + } catch (e) { + console.warn('Failed to stop track', e); + } finally { + setTrackId(null); + setStats(null); + } };
35-38: Handle optional getStats and catch errors.Spec defines getStats as optional; call safely.
const onGetStats = async () => { - const s = await getStats(); - if (s) setStats({fps: s.fps, droppedFrames: s.droppedFrames}); + try { + const s = await getStats?.(); + if (s) setStats({fps: s.fps, droppedFrames: s.droppedFrames}); + } catch (e) { + console.warn('Failed to get stats', e); + } };
41-47: Disable buttons based on state to prevent invalid actions.- <View style={{flex: 1}}> - <Button title="Start" onPress={onStart} /> - <Button title="Stop" onPress={onStop} /> - <Button title="Get Stats" onPress={onGetStats} /> + <View style={{flex: 1}}> + <Button title="Start" onPress={onStart} disabled={!!trackId} /> + <Button title="Stop" onPress={onStop} disabled={!trackId} /> + <Button title="Get Stats" onPress={onGetStats} disabled={!trackId} />
10-15: Dispose active track on unmount.Ensures no native resources are leaked when the screen unmounts.
const [stats, setStats] = React.useState<{ fps: number; droppedFrames: number; } | null>(null); + React.useEffect(() => { + return () => { + if (trackId) { + disposeTrack(trackId).catch(() => {}); + } + }; + }, [trackId]);src/native-vision-rtc.ts (1)
28-33: getStats is optional; ensure JS wrapper normalizes.Since README/example call getStats directly, consider making the wrapper always defined and returning null when native omits it, or update docs accordingly.
ios/VisionRTCModule.swift (4)
24-30: Confirm the source shape matches TS types;viewTagis unused.
- You return
["__nativeSourceId": id]. VerifyVisionCameraSourceexpects this exact key. If not, align naming.- If
viewTagis required for VisionCamera integration, consider validating it or documenting that it’s currently unused.
17-23: Clean up unused state and consider per-track settings.
lastTimestampNsis unused.targetFps/outWidth/outHeightare global; if multiple tracks are supported later, migrate to per-track config.
101-106: Minor: prefer constructing the display link with a weak proxy to avoid retain cycles.
CADisplayLinkretains its target. While you invalidate on stop, a weak proxy (or closure via block-based wrapper) can be safer if lifecycle gets more complex.
113-156: Performance: heavy per-frame allocations on main thread.You allocate a new
[UInt8]andCVPixelBufferevery tick on the main run loop. Consider:
- Reusing a preallocated buffer and
CVPixelBuffer(or aCVPixelBufferPool).- Moving gradient generation off the main thread (keeping delivery thread-safe).
src/index.ts (2)
31-36: Expose replaceTrack as feature-detectable; iOS currently rejects.Since iOS returns a rejection (or was a no-op), surface a clearer API or document platform support. Optionally wrap with a guard:
export function replaceTrack( senderId: string, nextTrackId: string ): Promise<void> { - return NativeVisionRTC.replaceSenderTrack(senderId, nextTrackId); + return NativeVisionRTC.replaceSenderTrack(senderId, nextTrackId); }And document that it may reject on platforms where unimplemented.
57-63: getStats: align type across platforms.JS type optionally includes
encoderQueueDepth. If Android/iOS diverge, consider always returning the key withundefinedfor consistency, or narrowing the union.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
example/ios/Podfile.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (24)
.cursor/mcp.json(0 hunks)README.md(1 hunks)VisionRtc.podspec(1 hunks)android/build.gradle(1 hunks)android/src/main/java/com/visionrtc/VisionRtcModule.kt(1 hunks)android/src/main/java/com/visionrtc/VisionRtcPackage.kt(2 hunks)example/babel.config.js(2 hunks)example/index.js(1 hunks)example/ios/Podfile(1 hunks)example/ios/VisionRtcExample.xcodeproj/project.pbxproj(6 hunks)example/ios/VisionRtcExample.xcworkspace/contents.xcworkspacedata(1 hunks)example/ios/VisionRtcExample/Info.plist(1 hunks)example/metro.config.js(1 hunks)example/src/App.tsx(1 hunks)ios/VisionRTCBridge.mm(1 hunks)ios/VisionRTCModule.swift(1 hunks)ios/VisionRtc.h(0 hunks)package.json(1 hunks)src/NativeVisionRtc.ts(0 hunks)src/index.ts(1 hunks)src/index.tsx(0 hunks)src/native-vision-rtc.ts(1 hunks)src/types.ts(1 hunks)tsconfig.json(2 hunks)
💤 Files with no reviewable changes (4)
- src/NativeVisionRtc.ts
- src/index.tsx
- .cursor/mcp.json
- ios/VisionRtc.h
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-ios
- GitHub Check: build-android
🔇 Additional comments (20)
package.json (1)
129-129: Prettier configuration updated to remove bracket spacing.This formatting change will affect all object destructuring and literals across the codebase, which aligns with the formatting changes visible in other files in this PR.
example/metro.config.js (1)
2-3: Consistent formatting with Prettier bracketSpacing configuration.The removal of spaces inside destructuring braces aligns with the
bracketSpacing: falsesetting added to the Prettier configuration in package.json.tsconfig.json (2)
9-10: Enhanced TypeScript configuration for React Native development.The additions improve the development experience by:
- Adding explicit type declarations for React, React Native, and Jest
- Including React Native-specific custom conditions for better module resolution
These changes align well with the new VisionRTC TypeScript bridge being introduced.
29-29: Minor formatting: Added trailing comma for consistency.This follows the
trailingComma: "es5"setting from the Prettier configuration.example/ios/VisionRtcExample.xcworkspace/contents.xcworkspacedata (1)
1-10: Standard Xcode workspace configuration for CocoaPods integration.The workspace file correctly includes both the main project and the Pods project, which is essential for the GoogleWebRTC dependency added in the Podfile to work properly.
VisionRtc.podspec (1)
19-19: ```shell
#!/bin/bash
curl -s https://trunk.cocoapods.org/api/v1/pods/GoogleWebRTC/versions | head -n20</blockquote></details> <details> <summary>example/ios/VisionRtcExample/Info.plist (1)</summary><blockquote> `36-37`: **React Native's New Architecture enabled.** Setting `RCTNewArchEnabled` to `true` enables React Native's new architecture (Fabric and TurboModules), which aligns with the TurboModule implementation being introduced in this PR. Ensure that all native modules in this project are compatible with the New Architecture. Since this PR introduces new native modules for VisionRTC, verify they properly implement the TurboModule spec. ```shell #!/bin/bash # Check for TurboModule implementations in the native code echo "=== Checking Android TurboModule implementation ===" rg -n "TurboModule" android/src/main/java/com/visionrtc/ || echo "No TurboModule references found in Android" echo -e "\n=== Checking iOS TurboModule implementation ===" rg -n "TurboModule" ios/ || echo "No TurboModule references found in iOS" echo -e "\n=== Checking TypeScript Spec definitions ===" rg -n "TurboModule\|Spec" src/ || echo "No TurboModule/Spec references found in src"example/babel.config.js (1)
2-2: Formatting updated to match Prettier bracketSpacing configuration.Both the destructuring assignment and object literal now follow the
bracketSpacing: falsesetting from the Prettier configuration, maintaining consistency across the codebase.Also applies to: 11-11
example/ios/VisionRtcExample.xcodeproj/project.pbxproj (3)
49-49: LGTM: File reference is correctly wired into the project group.
379-383: Hermes + RN script path config—double-check Podfile alignment.
- REACT_NATIVE_PATH looks correct for an example app. Ensure your Podfile reads USE_HERMES = true so Pods are generated accordingly.
- SWIFT_ACTIVE_COMPILATION_CONDITIONS only in Debug as expected.
447-450: Release config mirrors Debug for RN/Hermes—good. Verify CI caches and Pod install.Ensure CI runs pod install after toggling Hermes so the correct pods are installed for Release too.
src/types.ts (1)
21-21: LGTM: Track handle shape is minimal and clear.android/src/main/java/com/visionrtc/VisionRtcPackage.kt (1)
10-16: Confirm name rename ripple (VisionRtc → VisionRTC).Ensure JS/TS and iOS bridge use the new module name consistently; otherwise TurboModule lookup will fail at runtime.
#!/bin/bash # Find lingering old identifiers rg -nP --ignore-case -C2 '\bVisionRtc\b' -g '!**/build/**' # Ensure new name is referenced rg -nP '\bVisionRTC\b' -g '!**/build/**'example/index.js (1)
1-3: Formatting alignment with repo style looks good.src/native-vision-rtc.ts (2)
17-20: Align naming: replaceSenderTrack vs wrapper API.AI summary mentions a wrapper named replaceTrack. Please confirm wrapper-to-spec mapping or rename for consistency to avoid confusion.
35-35: LGTM: Module name matches iOS export.TurboModuleRegistry.getEnforcing('VisionRTC') aligns with the iOS RCT_EXPORT_MODULE(VisionRTC).
ios/VisionRTCBridge.mm (2)
9-13: LGTM: Codegen TurboModule bridge hooked up correctly.getTurboModule returns NativeVisionRtcSpecJSI as expected for codegen.
6-8: Ensure single module registration.Confirm the Swift implementation doesn’t also export VisionRTC (to avoid duplicate registrations).
Run this to verify:
#!/bin/bash rg -n -S 'RCT_EXPORT_MODULE\((VisionRTC|\"VisionRTC\")\)' rg -n --type=swift '@objc\s*\(\s*VisionRTC\s*\)'ios/VisionRTCModule.swift (1)
55-60: ```shell
#!/bin/bashInspect replaceSenderTrack implementation and similar promise methods in ios/VisionRTCModule.swift
rg -n -C3 "func replaceSenderTrack" ios/VisionRTCModule.swift
rg -n -C3 "resolver\(" ios/VisionRTCModule.swift
rg -n -C3 "rejecter\(" ios/VisionRTCModule.swift</blockquote></details> <details> <summary>src/index.ts (1)</summary><blockquote> `18-22`: **Verify the return shape of createVisionCameraSource matches VisionCameraSource.** Ensure the native iOS returns the same key(s) your TS types expect (e.g., `__nativeSourceId` vs `nativeSourceId`), otherwise track creation will fail at runtime. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
gmemmy
force-pushed
the
feat/native-skeleton
branch
3 times, most recently
from
d93612f to
edfe6cd
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package.json (1)
9-11: Fix export "source" path to match entry file rename.The repo now uses src/index.ts (tsx removed). Update exports["."].source to prevent toolchains (bob/Metro) from resolving a non-existent file.
"exports": { ".": { - "source": "./src/index.tsx", + "source": "./src/index.ts", "types": "./lib/typescript/src/index.d.ts", "default": "./lib/module/index.js" },
♻️ Duplicate comments (1)
android/src/main/java/com/visionrtc/VisionRtcModule.kt (1)
184-205: FPS changes don’t take effect; reschedule the task when setFps is called.The executor period is fixed at start(). setFps only mutates a field.
private val executor: ScheduledExecutorService = Executors.newSingleThreadScheduledExecutor() + private var scheduled: java.util.concurrent.ScheduledFuture<*>? = null @@ fun start() { if (running) return running = true val periodNs = (1_000_000_000L / fps.toLong()) - executor.scheduleAtFixedRate({ tick() }, 0L, periodNs, TimeUnit.NANOSECONDS) + scheduled = executor.scheduleAtFixedRate({ tick() }, 0L, periodNs, TimeUnit.NANOSECONDS) } @@ fun stop() { running = false - executor.shutdownNow() + scheduled?.cancel(true) + executor.shutdownNow() } @@ - fun setFps(next: Int) { fps = next } + fun setFps(next: Int) { + if (next <= 0 || next == fps) return + fps = next + if (running) { + scheduled?.cancel(false) + val periodNs = (1_000_000_000L / fps.toLong()) + scheduled = executor.scheduleAtFixedRate({ tick() }, 0L, periodNs, TimeUnit.NANOSECONDS) + } + }Optionally:
+import java.util.concurrent.ScheduledFuture
🧹 Nitpick comments (11)
package.json (3)
129-129: Prettier bracketSpacing change: confirm repo-wide formatting run.This will churn many files on first pass; ensure CI/lint/pre-commit hooks are aligned to avoid noisy PRs.
95-101: Jest ignore pattern: consider excluding example/android/.gradle and build caches.Keeps discovery fast in workspaces.
"modulePathIgnorePatterns": [ "<rootDir>/example/node_modules", - "<rootDir>/lib/" + "<rootDir>/lib/", + "<rootDir>/example/android/.gradle", + "<rootDir>/example/android/build" ]
61-86: React/React Native pinning vs peer ranges.devDependencies pin RN 0.81.1/React 19.1.0 while peerDependencies are "*". Consider setting minimal peer ranges to communicate compatibility.
"peerDependencies": { - "react": "*", - "react-native": "*" + "react": ">=18.2 || ^19.0.0", + "react-native": ">=0.75" },android/build.gradle (1)
48-50: Modernize deprecated lintOptions block under AGP 8.AGP 8 expects the lint { } block.
- lintOptions { - disable "GradleCompatible" - } + lint { + disable += ["GradleCompatible"] + }ios/VisionRTCModule.swift (4)
19-19: Remove unused lastTimestampNs.- private var lastTimestampNs: Int64 = 0
57-61: createVisionCameraSource ignores viewTag.If the source is expected to be tied to a view (e.g., VisionCamera), store the mapping or document that it’s a placeholder.
58-61: replaceSenderTrack is a no-op; reject to surface unsupported API.func replaceSenderTrack(senderId: NSString, newTrackId: NSString, resolver: RCTPromiseResolveBlock, rejecter: RCTPromiseRejectBlock) { - resolver(NSNull()) + rejecter("E_NOT_IMPLEMENTED", "replaceSenderTrack is not implemented on iOS yet.", nil) }
173-177: Avoid allocating a new RTCVideoCapturer in the fallback path.Creating a capturer per frame can leak/overhead. Prefer returning if capturer missing, or ensure it’s created once and stored.
- } else { - src.capturer(RTCVideoCapturer(delegate: src), didCapture: frame) - } + }android/src/main/java/com/visionrtc/VisionRtcModule.kt (2)
159-170: Release EGL resources on teardown.eglBase holds native resources; release in invalidate().
override fun invalidate() { super.invalidate() tracks.values.forEach { handle -> handle.capturer.stop() handle.track.setEnabled(false) handle.track.dispose() handle.source.dispose() } tracks.clear() + try { eglBase.release() } catch (_: Throwable) {} }
152-157: getStats: consider reporting targetFps and per-track stats.Current API returns module-wide fps with droppedFrames=0. Clarify semantics or extend shape when multiple tracks are active.
README.md (1)
16-38: Example looks good; add note about platform dependencies.Mention Pod/WebRTC Gradle deps requirement and that getStats shape may evolve.
async function demo(reactTag: number) { @@ return stats; } // Example invocation: // demo(findNodeHandle(cameraRef)); +// Note: Ensure iOS Pods (WebRTC-SDK) and Android dependency (org.webrtc:google-webrtc) +// are installed before running the example.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
example/ios/Podfile.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (26)
.cursor/mcp.json(0 hunks)README.md(1 hunks)VisionRtc.podspec(1 hunks)android/build.gradle(1 hunks)android/src/main/java/com/visionrtc/VisionRtcModule.kt(1 hunks)android/src/main/java/com/visionrtc/VisionRtcPackage.kt(2 hunks)example/android/settings-gradle.lockfile(1 hunks)example/babel.config.js(2 hunks)example/index.js(1 hunks)example/ios/Podfile(1 hunks)example/ios/VisionRtcExample.xcodeproj/project.pbxproj(6 hunks)example/ios/VisionRtcExample.xcworkspace/contents.xcworkspacedata(1 hunks)example/ios/VisionRtcExample/Info.plist(1 hunks)example/metro.config.js(1 hunks)example/src/App.tsx(1 hunks)ios/VisionRTCBridge.mm(1 hunks)ios/VisionRTCModule.swift(1 hunks)ios/VisionRtc.h(0 hunks)ios/VisionRtc.mm(0 hunks)package.json(1 hunks)src/NativeVisionRtc.ts(0 hunks)src/index.ts(1 hunks)src/index.tsx(0 hunks)src/native-vision-rtc.ts(1 hunks)src/types.ts(1 hunks)tsconfig.json(2 hunks)
💤 Files with no reviewable changes (5)
- src/NativeVisionRtc.ts
- ios/VisionRtc.h
- src/index.tsx
- .cursor/mcp.json
- ios/VisionRtc.mm
✅ Files skipped from review due to trivial changes (2)
- example/android/settings-gradle.lockfile
- example/metro.config.js
🚧 Files skipped from review as they are similar to previous changes (13)
- example/index.js
- example/ios/Podfile
- tsconfig.json
- src/index.ts
- example/ios/VisionRtcExample.xcworkspace/contents.xcworkspacedata
- src/types.ts
- android/src/main/java/com/visionrtc/VisionRtcPackage.kt
- example/babel.config.js
- src/native-vision-rtc.ts
- example/ios/VisionRtcExample/Info.plist
- example/src/App.tsx
- ios/VisionRTCBridge.mm
- example/ios/VisionRtcExample.xcodeproj/project.pbxproj
🔇 Additional comments (2)
android/build.gradle (1)
77-77: LGTM: pinned WebRTC dependency.Pinning org.webrtc:google-webrtc:1.0.25821 avoids wildcard drift.
VisionRtc.podspec (1)
19-19: Document WebRTC-SDK dependency detailsVisionRtc.podspec:19
- Confirmed that the CocoaPod
WebRTC-SDKis maintained by the webrtcsdk project and publishes Chromium-revisioned builds (e.g. 125.6422.06). Ensure the version in your Podspec matches your Android artifact baseline.- Note that
GoogleWebRTC(last official Google/tip-of-tree pod is v1.1.32000 and is deprecated) differs in source, binary size, and licensing. Document that you’re using the third-partyWebRTC-SDKxcframework and any transitive size impact.- Add a brief note or comment in VisionRtc.podspec (or your project README) specifying the pod source, version, and rationale for choosing
WebRTC-SDKoverGoogleWebRTC.
gmemmy
force-pushed
the
feat/native-skeleton
branch
3 times, most recently
from
481bdd1 to
5d83c5b
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
example/src/App.tsx (1)
18-42: LGTM on start guard and error handling (matches earlier feedback)Early-return + try/catch addresses duplicate-start leaks from the prior review.
android/src/main/java/com/visionrtc/VisionRtcModule.kt (1)
164-172: Disposing VideoTrack and VideoSource on remove looks solidThis addresses the prior leak concern.
🧹 Nitpick comments (13)
package.json (1)
130-130: Prettier style tweak acknowledgedbracketSpacing:false is fine; ensure repo-wide formatting is run to avoid noisy diffs, and that ESLint/prettier rules don’t conflict.
ios/VisionRTCBridge.mm (1)
8-15: Optional: declare requiresMainQueueSetup to suppress old-arch warningsNot functionally required here, but declaring it avoids console noise if the app flips old/new arch flags.
Add:
@implementation VisionRTCBridging RCT_EXPORT_MODULE(VisionRTC) ++ (BOOL)requiresMainQueueSetup +{ + return NO; +}example/src/App.tsx (2)
18-32: Reset stale stats when (re)startingEnsure stats shown correspond to the active track.
Apply:
const onStart = async () => { if (creating || trackId) return; setCreating(true); + setStats(null); let newId: string | null = null;
58-64: Disable buttons based on statePrevents racy interactions (e.g., pressing Stop during create).
Apply:
- <Button title="Start" onPress={onStart} /> - <Button title="Stop" onPress={onStop} /> - <Button title="Get Stats" onPress={onGetStats} /> + <Button title="Start" onPress={onStart} disabled={creating || !!trackId} /> + <Button title="Stop" onPress={onStop} disabled={!trackId || creating} /> + <Button title="Get Stats" onPress={onGetStats} disabled={!trackId} />src/NativeVisionRtc.ts (1)
3-14: Optional: tighten TS shapes with readonly to express immutabilityConveys intent and prevents accidental mutation on callers.
Apply:
-type VisionCameraSourceShape = {__nativeSourceId: string}; +type VisionCameraSourceShape = {readonly __nativeSourceId: string}; type NativePixelSourceShape = - | {platform: 'ios'; pixelBufferRef: unknown} + | {readonly platform: 'ios'; readonly pixelBufferRef: unknown} | { - platform: 'android'; - hardwareBufferRef?: unknown; - surfaceTextureId?: number; + readonly platform: 'android'; + readonly hardwareBufferRef?: unknown; + readonly surfaceTextureId?: number; }; type TrackOptionsShape = { - fps?: number; - resolution?: {width: number; height: number}; + readonly fps?: number; + readonly resolution?: {readonly width: number; readonly height: number}; };android/src/main/java/com/visionrtc/VisionRtcModule.kt (8)
56-62: Source arg is ignored; align Android with iOS/spec
createVisionCameraSourcereturns__nativeSourceId, butcreateTrack(source, ...)doesn’t use it. Either persist and validate the sourceId here or document that Android ignores it for now to keep cross‑platform parity.Also applies to: 63-64
111-119: Return error for unknown track and toggle track enabled on pause/resumeCurrently, unknown IDs no-op and still resolve. Also reflect pause/resume on the
VideoTrackenabled state.Apply this diff:
override fun pauseTrack(trackId: String, promise: Promise) { - tracks[trackId]?.capturer?.pause() - promise.resolve(null) + val handle = tracks[trackId] + if (handle == null) { + promise.reject("ERR_UNKNOWN_TRACK", "Unknown trackId: $trackId") + return + } + handle.capturer.pause() + handle.track.setEnabled(false) + promise.resolve(null) } override fun resumeTrack(trackId: String, promise: Promise) { - tracks[trackId]?.capturer?.resume() - promise.resolve(null) + val handle = tracks[trackId] + if (handle == null) { + promise.reject("ERR_UNKNOWN_TRACK", "Unknown trackId: $trackId") + return + } + handle.capturer.resume() + handle.track.setEnabled(true) + promise.resolve(null) }
107-109: Reject unimplemented replaceSenderTrackAvoid silent success; reject clearly until implemented.
Apply this diff:
override fun replaceSenderTrack(senderId: String?, newTrackId: String?, promise: Promise) { - promise.resolve(null) + promise.reject("E_NOT_IMPLEMENTED", "replaceSenderTrack is not implemented on Android yet") }
63-87: Clamp and normalize constraints (width/height even, sane fps bounds)Prevent invalid/hostile values from JS from causing heavy allocations or odd-sized I420 frames.
Apply this diff after parsing opts:
- if (opts != null) { + if (opts != null) { ... } - targetFps = fps + // Normalize: even dims for I420, sensible ranges + width = width.coerceIn(16, 4096).let { if (it and 1 != 0) it - 1 else it } + height = height.coerceIn(16, 4096).let { if (it and 1 != 0) it - 1 else it } + fps = fps.coerceIn(1, 120) + targetFps = fps
153-161: Mirror clamping in setTrackConstraintsKeep runtime updates consistent with create.
Apply this diff:
if (nextWidth != null && nextHeight != null) { - cap.setResolution(nextWidth, nextHeight) + val w = nextWidth!!.coerceIn(16, 4096).let { if (it and 1 != 0) it - 1 else it } + val h = nextHeight!!.coerceIn(16, 4096).let { if (it and 1 != 0) it - 1 else it } + cap.setResolution(w, h) } if (nextFps != null) { - cap.setFps(nextFps) + cap.setFps(nextFps!!.coerceIn(1, 120)) }
174-179: Report targetFps and compute droppedFramesExpose target vs delivered FPS and a simple dropped frame estimate.
Apply this diff:
override fun getStats(promise: Promise) { val result = com.facebook.react.bridge.Arguments.createMap() result.putInt("fps", lastReportedFps) - result.putInt("droppedFrames", 0) + result.putInt("targetFps", targetFps) + result.putInt("droppedFrames", maxOf(0, targetFps - lastReportedFps)) promise.resolve(result) }
206-208: Consider sharing a scheduler across tracksOne thread per track scales poorly. A shared ScheduledThreadPoolExecutor with per-track tasks would reduce thread count.
257-270: Micro-optimization: bulk fill instead of per-pixel putsPer-pixel absolute ByteBuffer writes are costly. Precompute one gradient row into a byte[] and copy per row, and use
yPlane.put(offset, row, 0, rowLen)in a loop.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
example/ios/Podfile.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (26)
.cursor/mcp.json(0 hunks)README.md(1 hunks)VisionRtc.podspec(1 hunks)android/build.gradle(1 hunks)android/src/main/java/com/visionrtc/VisionRtcModule.kt(1 hunks)android/src/main/java/com/visionrtc/VisionRtcPackage.kt(2 hunks)example/android/settings-gradle.lockfile(1 hunks)example/babel.config.js(2 hunks)example/index.js(1 hunks)example/ios/Podfile(1 hunks)example/ios/VisionRtcExample.xcodeproj/project.pbxproj(6 hunks)example/ios/VisionRtcExample.xcworkspace/contents.xcworkspacedata(1 hunks)example/ios/VisionRtcExample/Info.plist(1 hunks)example/metro.config.js(1 hunks)example/package.json(1 hunks)example/src/App.tsx(1 hunks)ios/VisionRTCBridge.mm(1 hunks)ios/VisionRTCModule.swift(1 hunks)ios/VisionRtc.h(0 hunks)ios/VisionRtc.mm(0 hunks)package.json(2 hunks)src/NativeVisionRtc.ts(1 hunks)src/index.ts(1 hunks)src/index.tsx(0 hunks)src/types.ts(1 hunks)tsconfig.json(2 hunks)
💤 Files with no reviewable changes (4)
- .cursor/mcp.json
- src/index.tsx
- ios/VisionRtc.h
- ios/VisionRtc.mm
✅ Files skipped from review due to trivial changes (3)
- example/index.js
- example/package.json
- example/android/settings-gradle.lockfile
🚧 Files skipped from review as they are similar to previous changes (14)
- android/build.gradle
- VisionRtc.podspec
- example/babel.config.js
- README.md
- example/ios/VisionRtcExample.xcworkspace/contents.xcworkspacedata
- android/src/main/java/com/visionrtc/VisionRtcPackage.kt
- tsconfig.json
- ios/VisionRTCModule.swift
- example/metro.config.js
- example/ios/VisionRtcExample/Info.plist
- src/types.ts
- example/ios/Podfile
- src/index.ts
- example/ios/VisionRtcExample.xcodeproj/project.pbxproj
🧰 Additional context used
🧬 Code graph analysis (3)
src/NativeVisionRtc.ts (1)
android/src/main/java/com/visionrtc/VisionRtcModule.kt (1)
source(43-47)
android/src/main/java/com/visionrtc/VisionRtcModule.kt (1)
ios/VisionRTCModule.swift (1)
tick(174-245)
example/src/App.tsx (3)
src/index.ts (3)
createWebRTCTrack(24-29)disposeTrack(53-55)getStats(57-63)android/src/main/java/com/visionrtc/VisionRtcModule.kt (2)
disposeTrack(164-172)getStats(174-179)ios/VisionRTCModule.swift (2)
disposeTrack(124-137)getStats(139-146)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-ios
- GitHub Check: build-android
🔇 Additional comments (7)
ios/VisionRTCBridge.mm (1)
9-15: Module name alignment looks correctExported name "VisionRTC" matches the JS registry key used in TurboModuleRegistry, avoiding lookup mismatches.
src/NativeVisionRtc.ts (2)
43-43: Name matches iOS bridgeTurboModule key "VisionRTC" aligns with the iOS exported name. Good.
24-34: Verified: All declared methods are implemented natively on iOS and Android
The follow-ups in ios/VisionRTCModule.swift and VisionRtcModule.kt confirm @objc and override implementations for replaceSenderTrack, pauseTrack, resumeTrack, setTrackConstraints, disposeTrack (and related methods).android/src/main/java/com/visionrtc/VisionRtcModule.kt (4)
121-162: Nice defensive ReadableMap guards and unknown track handlingGood job hardening against missing keys and rejecting unknown track IDs.
228-237: FPS rescheduling fix is correctCancel-and-reschedule on fps change prevents stale periods. Good use of a ScheduledFuture.
63-87: Parity with iOSAndroid generates I420 directly; iOS uses BGRA CVPixelBuffer -> RTCCVPixelBuffer. That’s fine for a demo, but document format differences so consumers don’t assume a single pixel format across platforms.
Also applies to: 121-162
32-41: No duplicate initialize() calls detected
Confirmed thatPeerConnectionFactory.initialize(...)is only invoked once within the lazy block—no redundant initializations found.
gmemmy
force-pushed
the
feat/native-skeleton
branch
from
5d83c5b to
fef520c
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
package.json (3)
8-13: Harden exports for RN/ESM/CJS and keep source for tooling.Add explicit conditions and keep “source” for tooling.
"exports": { ".": { - "source": "./src/index.ts", - "types": "./lib/typescript/src/index.d.ts", - "default": "./lib/module/index.js" + "types": "./lib/typescript/src/index.d.ts", + "react-native": "./lib/module/index.js", + "import": "./lib/module/index.js", + "require": "./lib/commonjs/index.js", + "source": "./src/index.ts", + "default": "./lib/module/index.js" }, "./package.json": "./package.json" },
135-151: Produce a CJS build if exposing "require" in exports.Add CommonJS target to bob so the require path is real.
"react-native-builder-bob": { "source": "src", "output": "lib", "targets": [ + ["commonjs", {}], [ "module", { "esm": true } ], [ "typescript", { "project": "tsconfig.build.json" } ] ] },
6-6: Point react-native entry to built JS, not TS source.Using TS here can break consumers that don’t transpile node_modules.
- "react-native": "./src/index.ts", + "react-native": "./lib/module/index.js",example/src/App.tsx (1)
44-50: Harden stop and ensure cleanup on unmount.Dispose failures shouldn’t block state reset; add try/finally and unmount cleanup.
const onStop = async () => { - if (trackId) { - await disposeTrack(trackId); - setTrackId(null); - setStats(null); - } + if (!trackId) return; + try { + await disposeTrack(trackId); + } catch (e) { + console.warn('Failed to dispose track', e); + } finally { + setTrackId(null); + setStats(null); + } }; + + React.useEffect(() => { + return () => { + if (trackId) { + disposeTrack(trackId).catch(() => {}); + } + }; + }, [trackId]);
🧹 Nitpick comments (10)
package.json (1)
5-5: If you add CJS, align "main" to it.Optional, but conventional when a CJS artifact exists.
- "main": "./lib/module/index.js", + "main": "./lib/commonjs/index.js",example/src/App.tsx (1)
59-61: Disable buttons based on state to prevent accidental actions.Minor UX safety.
- <Button title="Start" onPress={onStart} /> - <Button title="Stop" onPress={onStop} /> - <Button title="Get Stats" onPress={onGetStats} /> + <Button title="Start" onPress={onStart} disabled={!!trackId || creating} /> + <Button title="Stop" onPress={onStop} disabled={!trackId} /> + <Button title="Get Stats" onPress={onGetStats} disabled={!trackId} />ios/VisionRTCModule.swift (5)
138-145: getStats returns configured FPS, not delivered FPS. Align semantics with Android.Track and report actual delivered FPS.
@@ func getStats(resolver: RCTPromiseResolveBlock, rejecter: RCTPromiseRejectBlock) { - var fpsMax = 0 - stateQueue.sync { - fpsMax = self.activeTrackIds.compactMap { self.trackStates[$0]?.fps }.max() ?? 0 - } - resolver(["fps": fpsMax, "droppedFrames": 0]) + let fps = stateQueue.sync { self.lastReportedFps } + resolver(["fps": fps, "droppedFrames": 0]) } @@ @objc private func tick() { + var now = CACurrentMediaTime() + var framesInc = 0 var ids: [String] = [] stateQueue.sync { ids = Array(self.activeTrackIds) } if ids.isEmpty { return } @@ - for trackId in ids { + for trackId in ids { ... + framesInc += 1 } + stateQueue.async(flags: .barrier) { + if now - self.lastSecondTs >= 1.0 { + self.lastReportedFps = self.framesThisSecond + self.framesThisSecond = 0 + self.lastSecondTs = now + } else { + self.framesThisSecond += framesInc + } + } }Add properties:
class VisionRTC: NSObject { + private var framesThisSecond = 0 + private var lastSecondTs = CACurrentMediaTime() + private var lastReportedFps = 0Also applies to: 178-186
215-222: Add IOSurface property when creating CVPixelBuffer.Improves compatibility and performance with CoreVideo.
- let attrs = [ - kCVPixelBufferCGImageCompatibilityKey: kCFBooleanTrue!, - kCVPixelBufferCGBitmapContextCompatibilityKey: kCFBooleanTrue!, - ] as CFDictionary + let attrs = [ + kCVPixelBufferCGImageCompatibilityKey: kCFBooleanTrue!, + kCVPixelBufferCGBitmapContextCompatibilityKey: kCFBooleanTrue!, + kCVPixelBufferIOSurfacePropertiesKey: [:] as CFDictionary + ] as CFDictionary
129-135: Optionally disable the track on dispose.Not required for synthetic source, but keeps semantics consistent.
- tracks.removeValue(forKey: trackId as String) + if let t = tracks[trackId as String] { t.isEnabled = false } + tracks.removeValue(forKey: trackId as String)
174-176: Rename updateDisplayLinkFps → updateTimerFps (no display link anymore).Avoid confusion.
- private func updateDisplayLinkFps() { + private func updateTimerFps() { if captureTimer != nil { startNullCapturer() } }And:
- updateDisplayLinkFps() + updateTimerFps()Also applies to: 119-121
46-53: Clamp inputs for width/height/fps and reject invalid values.Guard against zero/negative sizes and extreme fps.
- if let res = opts?["resolution"] as? [String: Any], + if let res = opts?["resolution"] as? [String: Any], let w = res["width"] as? NSNumber, let h = res["height"] as? NSNumber { - width = w.int32Value; height = h.int32Value + width = max(16, min(w.int32Value, 4096)) + height = max(16, min(h.int32Value, 4096)) } - if let f = opts?["fps"] as? NSNumber { fps = f.intValue } + if let f = opts?["fps"] as? NSNumber { fps = max(1, min(f.intValue, 120)) } @@ - if let f = opts["fps"] as? NSNumber { nextFps = f.intValue } + if let f = opts["fps"] as? NSNumber { nextFps = max(1, min(f.intValue, 120)) }Also applies to: 95-106
ios/VisionRTC+Spec.mm (3)
1-5: Remove unused WebRTC includeThe file doesn’t reference WebRTC symbols. Drop the header to speed up builds and avoid unnecessary dependency coupling.
#import <Foundation/Foundation.h> #import <React/RCTBridgeModule.h> #import "VisionRtcSpec/VisionRtcSpec.h" #import <ReactCommon/RCTTurboModule.h> -#import <WebRTC/WebRTC.h>
7-34: Add nullability annotations to exported APIExplicit nullability clarifies Objective‑C/Swift interop and improves TS/Flow codegen. Wrap the file with NS_ASSUME_NONNULL and mark nullable parameters/returns as needed.
+#if !__has_feature(nullability) +#define NS_ASSUME_NONNULL_BEGIN +#define NS_ASSUME_NONNULL_END +#endif +NS_ASSUME_NONNULL_BEGIN ... @end +NS_ASSUME_NONNULL_ENDIf any params can be nil (e.g., opts), mark them as nullable (NSDictionary *_Nullable, NSString *_Nullable).
73-84: DRY: factor TrackOptions conversion into a helperThe opts-to-NSDictionary conversion is duplicated in createTrack and setTrackConstraints. Centralize to avoid drift.
Add helper (outside the selected ranges):
static inline NSMutableDictionary *VisionRtcTrackOptionsToDict(const JS::NativeVisionRtc::TrackOptionsShape &opts) { NSMutableDictionary *optsDict = [NSMutableDictionary dictionary]; if (auto fps = opts.fps()) { optsDict[@"fps"] = @(*fps); } if (auto res = opts.resolution()) { optsDict[@"resolution"] = @{ @"width": @((*res).width()), @"height": @((*res).height()) }; } return optsDict; }Apply within the selected ranges:
- NSMutableDictionary *optsDict = [NSMutableDictionary dictionary]; - if (auto fps = opts.fps()) { optsDict[@"fps"] = @(*fps); } - if (auto res = opts.resolution()) { - NSMutableDictionary *resDict = [NSMutableDictionary dictionary]; - resDict[@"width"] = @((*res).width()); - resDict[@"height"] = @((*res).height()); - optsDict[@"resolution"] = resDict; - } + NSMutableDictionary *optsDict = VisionRtcTrackOptionsToDict(opts);Also applies to: 113-124
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
example/Gemfile.lockis excluded by!**/*.lockexample/ios/Podfile.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (26)
.cursor/mcp.json(0 hunks)README.md(1 hunks)VisionRtc.podspec(1 hunks)android/build.gradle(1 hunks)android/src/main/java/com/visionrtc/VisionRtcModule.kt(1 hunks)android/src/main/java/com/visionrtc/VisionRtcPackage.kt(2 hunks)example/android/settings-gradle.lockfile(1 hunks)example/babel.config.js(2 hunks)example/index.js(1 hunks)example/ios/Podfile(1 hunks)example/ios/VisionRtcExample.xcodeproj/project.pbxproj(6 hunks)example/ios/VisionRtcExample.xcworkspace/contents.xcworkspacedata(1 hunks)example/ios/VisionRtcExample/Info.plist(1 hunks)example/metro.config.js(1 hunks)example/package.json(1 hunks)example/src/App.tsx(1 hunks)ios/VisionRTC+Spec.mm(1 hunks)ios/VisionRTCModule.swift(1 hunks)ios/VisionRtc.h(0 hunks)ios/VisionRtc.mm(0 hunks)package.json(3 hunks)src/NativeVisionRtc.ts(1 hunks)src/index.ts(1 hunks)src/index.tsx(0 hunks)src/types.ts(1 hunks)tsconfig.json(2 hunks)
💤 Files with no reviewable changes (4)
- .cursor/mcp.json
- src/index.tsx
- ios/VisionRtc.mm
- ios/VisionRtc.h
✅ Files skipped from review due to trivial changes (2)
- example/metro.config.js
- example/index.js
🚧 Files skipped from review as they are similar to previous changes (16)
- example/android/settings-gradle.lockfile
- example/ios/VisionRtcExample.xcworkspace/contents.xcworkspacedata
- example/babel.config.js
- example/package.json
- tsconfig.json
- example/ios/Podfile
- src/index.ts
- README.md
- android/src/main/java/com/visionrtc/VisionRtcPackage.kt
- example/ios/VisionRtcExample/Info.plist
- android/src/main/java/com/visionrtc/VisionRtcModule.kt
- src/NativeVisionRtc.ts
- android/build.gradle
- src/types.ts
- VisionRtc.podspec
- example/ios/VisionRtcExample.xcodeproj/project.pbxproj
🧰 Additional context used
🧬 Code graph analysis (2)
example/src/App.tsx (3)
src/index.ts (3)
createWebRTCTrack(24-29)disposeTrack(53-55)getStats(57-63)android/src/main/java/com/visionrtc/VisionRtcModule.kt (2)
disposeTrack(164-172)getStats(174-179)ios/VisionRTCModule.swift (2)
disposeTrack(123-136)getStats(138-145)
ios/VisionRTCModule.swift (2)
android/src/main/java/com/visionrtc/VisionRtcModule.kt (11)
createVisionCameraSource(56-61)createTrack(63-105)source(43-47)replaceSenderTrack(107-109)pauseTrack(111-114)resumeTrack(116-119)setTrackConstraints(121-162)disposeTrack(164-172)getStats(174-179)tick(239-277)resume(221-221)src/index.ts (6)
createVisionCameraSource(18-22)pauseTrack(38-40)resumeTrack(42-44)setTrackConstraints(46-51)disposeTrack(53-55)getStats(57-63)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-ios
- GitHub Check: build-android
🔇 Additional comments (3)
package.json (1)
153-165: iOS modulesProvider mapping is correctVisionRTCTurbo and VisionRtcSpec symbols (including @objc(VisionRTC)) match the package.json modulesProvider entry.
ios/VisionRTC+Spec.mm (2)
42-42: Verify module name matches codegen specCodegen namespace is JS::NativeVisionRtc (camel-case “Rtc”), but the module is exported as “VisionRTC”. Mismatch can confuse the legacy bridge and tooling. Consider aligning the export name or omitting the explicit name.
-RCT_EXPORT_MODULE(VisionRTC) +// Option A: match codegen +RCT_EXPORT_MODULE(VisionRtc) +// Option B: omit explicit name (uses class name); ensure it matches codegen expectations +// RCT_EXPORT_MODULE()
55-60: TurboModule wiring looks correctReturning NativeVisionRtcSpecJSI with the provided InitParams matches RN codegen patterns.
gmemmy
force-pushed
the
feat/native-skeleton
branch
2 times, most recently
from
4d31970 to
c0dd6cd
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (7)
android/src/main/java/com/visionrtc/VisionRtcModule.kt (2)
230-243: Notify WebRTC of capturer start/stopCapturerObserver supports start/stop; signal it for correctness with sinks.
fun start() { if (running) return running = true + try { observer.onCapturerStarted(true) } catch (_: Throwable) {} val periodNs = (1_000_000_000L / fps.toLong()) scheduled = executor.scheduleAtFixedRate({ tick() }, 0L, periodNs, TimeUnit.NANOSECONDS) } ... fun stop() { running = false scheduled?.cancel(true) executor.shutdownNow() + try { observer.onCapturerStopped() } catch (_: Throwable) {} }
179-185: Stats parity noteAndroid reports delivered FPS; iOS reports max configured FPS. Consider aligning semantics or documenting difference.
ios/VisionRTCModule.swift (3)
219-226: Add IOSurface backing to CVPixelBuffer for better performanceThis enables pool-backed, zero-copy surfaces on many paths.
- let attrs = [ - kCVPixelBufferCGImageCompatibilityKey: kCFBooleanTrue!, - kCVPixelBufferCGBitmapContextCompatibilityKey: kCFBooleanTrue!, - ] as CFDictionary + let attrs = [ + kCVPixelBufferCGImageCompatibilityKey: kCFBooleanTrue!, + kCVPixelBufferCGBitmapContextCompatibilityKey: kCFBooleanTrue!, + kCVPixelBufferIOSurfacePropertiesKey: [:] as CFDictionary + ] as CFDictionary
174-176: Rename for clarity: updateDisplayLinkFps → updateTimerFps- private func updateDisplayLinkFps() { + private func updateTimerFps() { if captureTimer != nil { startNullCapturer() } }Also update callers.
244-255: Avoid creating a new RTCVideoCapturer per frameFallback allocates a capturer on every frame when missing; store once.
- if let cap = capOpt { - src.capturer(cap, didCapture: frame) - } else { - src.capturer(RTCVideoCapturer(delegate: src), didCapture: frame) - } + if let cap = capOpt { + src.capturer(cap, didCapture: frame) + } else { + let newCap = RTCVideoCapturer(delegate: src) + stateQueue.async(flags: .barrier) { self.capturers[trackId] = newCap } + src.capturer(newCap, didCapture: frame) + }ios/VisionRTC+Spec.mm (2)
36-38: Conform to RCTTurboModule for clarity and tooling compatibility-@interface VisionRTCTurbo : NSObject <NativeVisionRtcSpec> +@interface VisionRTCTurbo : NSObject <NativeVisionRtcSpec, RCTTurboModule>
98-110: Parity note: replaceSenderTrack is a no-opBoth platforms currently no-op; document or implement before wiring into call sites.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
example/Gemfile.lockis excluded by!**/*.lockexample/ios/Podfile.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (26)
.cursor/mcp.json(0 hunks)README.md(1 hunks)VisionRtc.podspec(1 hunks)android/build.gradle(1 hunks)android/src/main/java/com/visionrtc/VisionRtcModule.kt(1 hunks)android/src/main/java/com/visionrtc/VisionRtcPackage.kt(2 hunks)example/android/settings-gradle.lockfile(1 hunks)example/babel.config.js(2 hunks)example/index.js(1 hunks)example/ios/Podfile(1 hunks)example/ios/VisionRtcExample.xcodeproj/project.pbxproj(6 hunks)example/ios/VisionRtcExample.xcworkspace/contents.xcworkspacedata(1 hunks)example/ios/VisionRtcExample/Info.plist(1 hunks)example/metro.config.js(1 hunks)example/package.json(1 hunks)example/src/App.tsx(1 hunks)ios/VisionRTC+Spec.mm(1 hunks)ios/VisionRTCModule.swift(1 hunks)ios/VisionRtc.h(0 hunks)ios/VisionRtc.mm(0 hunks)package.json(5 hunks)src/NativeVisionRtc.ts(1 hunks)src/index.ts(1 hunks)src/index.tsx(0 hunks)src/types.ts(1 hunks)tsconfig.json(2 hunks)
💤 Files with no reviewable changes (4)
- .cursor/mcp.json
- ios/VisionRtc.h
- ios/VisionRtc.mm
- src/index.tsx
✅ Files skipped from review due to trivial changes (2)
- example/metro.config.js
- example/babel.config.js
🚧 Files skipped from review as they are similar to previous changes (17)
- tsconfig.json
- example/index.js
- example/ios/VisionRtcExample/Info.plist
- example/ios/VisionRtcExample.xcworkspace/contents.xcworkspacedata
- example/android/settings-gradle.lockfile
- example/ios/Podfile
- VisionRtc.podspec
- README.md
- src/index.ts
- android/src/main/java/com/visionrtc/VisionRtcPackage.kt
- example/package.json
- src/NativeVisionRtc.ts
- android/build.gradle
- example/src/App.tsx
- package.json
- src/types.ts
- example/ios/VisionRtcExample.xcodeproj/project.pbxproj
🧰 Additional context used
🧬 Code graph analysis (2)
ios/VisionRTCModule.swift (2)
android/src/main/java/com/visionrtc/VisionRtcModule.kt (11)
createVisionCameraSource(62-67)createTrack(69-111)source(48-52)replaceSenderTrack(113-115)pauseTrack(117-120)resumeTrack(122-125)setTrackConstraints(127-168)disposeTrack(170-178)getStats(180-185)tick(256-294)resume(238-238)src/index.ts (6)
createVisionCameraSource(18-22)pauseTrack(38-40)resumeTrack(42-44)setTrackConstraints(46-51)disposeTrack(53-55)getStats(57-63)
android/src/main/java/com/visionrtc/VisionRtcModule.kt (1)
ios/VisionRTCModule.swift (1)
tick(178-257)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-android
gmemmy
force-pushed
the
feat/native-skeleton
branch
from
c0dd6cd to
892f20f
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
ios/VisionRTC+Spec.mm (1)
36-38: Protocol conformance looks correct nowUsing NativeVisionRtcSpec (which extends RCTTurboModule in the generated header) resolves the prior misuse of RCTModuleProvider.
🧹 Nitpick comments (12)
README.md (3)
17-22: ImportfindNodeHandlefor the example invocation.Readers may copy/paste; add the import to avoid confusion.
```ts -import { +import { createVisionCameraSource, createWebRTCTrack, disposeTrack, getStats, } from 'react-native-vision-rtc'; +import { findNodeHandle } from 'react-native';--- `24-34`: **Ensure cleanup with try/finally.** If `getStats` throws, `disposeTrack` won’t run; wrap with `finally`. If there’s a source-dispose API, call it there too. ```diff async function demo(reactTag: number) { const source = await createVisionCameraSource(reactTag); const { trackId } = await createWebRTCTrack(source, { fps: 30, resolution: { width: 1280, height: 720 }, }); - - const stats = (await getStats?.()) ?? null; - await disposeTrack(trackId); - return stats; + try { + const stats = (await getStats?.()) ?? null; + return stats; + } finally { + await disposeTrack(trackId); + // If available: + // await disposeSource?.(source); + } }
36-37: Guard the example invocation against null tags.
findNodeHandlecan return null; show a safe call to prevent runtime errors.-// demo(findNodeHandle(cameraRef)); +// const tag = findNodeHandle(cameraRef); +// if (tag != null) demo(tag);android/src/main/java/com/visionrtc/VisionRtcModule.kt (2)
186-191: Expose targetFps or drop the field.targetFps is updated but unused in outputs; surface it in getStats or remove the field to avoid drift.
Apply this diff (option A):
override fun getStats(promise: Promise) { val result = com.facebook.react.bridge.Arguments.createMap() result.putInt("fps", lastReportedFps) result.putInt("droppedFrames", 0) + result.putInt("targetFps", targetFps) promise.resolve(result) }Or remove targetFps if not needed.
272-301: Consider reducing per-frame allocations.JavaI420Buffer.allocate(w,h) every tick creates GC pressure at 30/60fps. A small buffer pool (2–3 buffers) or reusing planes with wrap() would help.
I can sketch a tiny ring-buffer pool if you want.
ios/VisionRTC+Spec.mm (7)
5-5: Remove unused WebRTC importThis header isn’t referenced in this file and adds compile overhead.
-#import <WebRTC/WebRTC.h>
2-2: Add RCTLog import for diagnosticsNeeded for the logging suggestion below.
#import <React/RCTBridgeModule.h> +#import <React/RCTLog.h>
44-53: Log once when Swift implementation isn’t foundYou already reject per-call; logging at init makes misconfiguration obvious at startup.
if (self = [super init]) { Class swiftClass = NSClassFromString(@"VisionRtc.VisionRTC"); if (swiftClass == nil) { swiftClass = NSClassFromString(@"VisionRTC"); } + if (swiftClass == nil) { + RCTLogError(@"VisionRTC Swift implementation not found. Expected @objc(VisionRTC) in Swift module 'VisionRtc'."); + } _swift = swiftClass ? [swiftClass new] : nil; }
61-72: Ensure main-thread for view-tag driven workcreateVisionCameraSource likely touches UIKit via viewTag. TurboModule calls can land on background threads; dispatch to main to avoid UI threading bugs.
if (!self.swift) { if (reject) reject(@"E_NO_SWIFT_IMPL", @"VisionRTC Swift implementation not found. Ensure @objc(VisionRTC) exists in module 'VisionRtc'.", nil); return; } - [self.swift createVisionCameraSource:@(viewTag) resolver:resolve rejecter:reject]; + dispatch_async(dispatch_get_main_queue(), ^{ + [self.swift createVisionCameraSource:@(viewTag) resolver:resolve rejecter:reject]; + });
65-71: De-duplicate error code/message“E_NO_SWIFT_IMPL” and the long message are copy-pasted in many places, which is error-prone. Define once and reuse.
Example (place near the top of the file):
static NSString *const kVisionRTCNoSwiftCode = @"E_NO_SWIFT_IMPL"; static NSString *const kVisionRTCNoSwiftMsg = @"VisionRTC Swift implementation not found. Ensure @objc(VisionRTC) exists in module 'VisionRtc'.";And replace occurrences:
if (reject) reject(kVisionRTCNoSwiftCode, kVisionRTCNoSwiftMsg, nil);Also applies to: 79-85, 103-109, 116-122, 129-135, 143-149, 166-172, 178-184
85-95: Factor out TrackOptions conversion (avoid duplication)Same conversion logic appears here and in setTrackConstraints.
- NSMutableDictionary *optsDict = [NSMutableDictionary dictionary]; - if (auto fps = opts.fps()) { - optsDict[@"fps"] = @(*fps); - } - if (auto res = opts.resolution()) { - NSMutableDictionary *resDict = [NSMutableDictionary dictionary]; - resDict[@"width"] = @((*res).width()); - resDict[@"height"] = @((*res).height()); - optsDict[@"resolution"] = resDict; - } + NSMutableDictionary *optsDict = VisionRTCBuildTrackOptsDict(opts);Helper to add once in this file:
static inline NSMutableDictionary *VisionRTCBuildTrackOptsDict(JS::NativeVisionRtc::TrackOptionsShape &opts) { NSMutableDictionary *optsDict = [NSMutableDictionary dictionary]; if (auto fps = opts.fps()) { optsDict[@"fps"] = @(*fps); } if (auto res = opts.resolution()) { NSMutableDictionary *resDict = [NSMutableDictionary dictionary]; resDict[@"width"] = @((*res).width()); resDict[@"height"] = @((*res).height()); optsDict[@"resolution"] = resDict; } return optsDict; }
149-158: Apply the same TrackOptions helper hereUse the shared converter to keep behavior consistent.
- NSMutableDictionary *optsDict = [NSMutableDictionary dictionary]; - if (auto fps = opts.fps()) { - optsDict[@"fps"] = @(*fps); - } - if (auto res = opts.resolution()) { - NSMutableDictionary *resDict = [NSMutableDictionary dictionary]; - resDict[@"width"] = @((*res).width()); - resDict[@"height"] = @((*res).height()); - optsDict[@"resolution"] = resDict; - } + NSMutableDictionary *optsDict = VisionRTCBuildTrackOptsDict(opts);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
example/Gemfile.lockis excluded by!**/*.lockexample/ios/Podfile.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (26)
.cursor/mcp.json(0 hunks)README.md(1 hunks)VisionRtc.podspec(1 hunks)android/build.gradle(1 hunks)android/src/main/java/com/visionrtc/VisionRtcModule.kt(1 hunks)android/src/main/java/com/visionrtc/VisionRtcPackage.kt(2 hunks)example/android/settings-gradle.lockfile(1 hunks)example/babel.config.js(2 hunks)example/index.js(1 hunks)example/ios/Podfile(1 hunks)example/ios/VisionRtcExample.xcodeproj/project.pbxproj(6 hunks)example/ios/VisionRtcExample.xcworkspace/contents.xcworkspacedata(1 hunks)example/ios/VisionRtcExample/Info.plist(1 hunks)example/metro.config.js(1 hunks)example/package.json(1 hunks)example/src/App.tsx(1 hunks)ios/VisionRTC+Spec.mm(1 hunks)ios/VisionRTCModule.swift(1 hunks)ios/VisionRtc.h(0 hunks)ios/VisionRtc.mm(0 hunks)package.json(5 hunks)src/NativeVisionRtc.ts(1 hunks)src/index.ts(1 hunks)src/index.tsx(0 hunks)src/types.ts(1 hunks)tsconfig.json(2 hunks)
💤 Files with no reviewable changes (4)
- ios/VisionRtc.h
- .cursor/mcp.json
- ios/VisionRtc.mm
- src/index.tsx
✅ Files skipped from review due to trivial changes (1)
- example/index.js
🚧 Files skipped from review as they are similar to previous changes (18)
- example/package.json
- example/babel.config.js
- src/types.ts
- example/metro.config.js
- tsconfig.json
- example/android/settings-gradle.lockfile
- VisionRtc.podspec
- example/ios/VisionRtcExample.xcworkspace/contents.xcworkspacedata
- example/src/App.tsx
- example/ios/VisionRtcExample/Info.plist
- example/ios/Podfile
- src/index.ts
- android/build.gradle
- ios/VisionRTCModule.swift
- android/src/main/java/com/visionrtc/VisionRtcPackage.kt
- src/NativeVisionRtc.ts
- example/ios/VisionRtcExample.xcodeproj/project.pbxproj
- package.json
🧰 Additional context used
🧬 Code graph analysis (1)
android/src/main/java/com/visionrtc/VisionRtcModule.kt (1)
ios/VisionRTCModule.swift (1)
tick(181-272)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-ios
- GitHub Check: build-android
🔇 Additional comments (9)
README.md (2)
16-38: Nice cleanup of the example; addresses prior feedback.Async wrapper, real source creation, and defensive
getStats?.()look good.
31-31:getStatsis defined as a global function without atrackIdparameter.
Ignore the suggestion to pass atrackId—there’s no per-trackgetStatsAPI.Likely an incorrect or invalid review comment.
android/src/main/java/com/visionrtc/VisionRtcModule.kt (5)
30-46: Factory/EGL lazy init looks solid.Initialization is idempotent and deferred; good separation of concerns.
62-67: Confirm source semantics; viewTag is ignored.createVisionCameraSource returns an id but doesn’t persist or associate it with viewTag. If TS/iOS expect binding to a source handle, this will surprise callers.
Would you like me to wire a simple Map<sourceId, metadata> and validate it in createTrack?
97-111: Track creation lifecycle looks good.Track/source/capturer are created and started; handle stored and id returned.
117-131: Pause/resume API guard is consistent.Rejects unknown trackIds; idempotent calls are fine.
195-215: Teardown is comprehensive.Stops tracks, disposes factory/EGL, and shuts down cleanupExecutor.
ios/VisionRTC+Spec.mm (2)
55-59: TurboModule provider wiring LGTMReturns the correct codegen JSI wrapper.
42-42: Verify codegen spec name matches RCT_EXPORT_MODULE
- Locate the generated
VisionRtcSpec.hand confirm the protocol name (NativeVisionRtcSpec) exactly matches your intended module name (case-sensitive).- Ensure your
RCT_EXPORT_MODULE(...)argument uses that exact name:
• If the spec is “VisionRtc”, change to• If you only rely on-RCT_EXPORT_MODULE(VisionRTC) +RCT_EXPORT_MODULE(VisionRtc)TurboModuleRegistry, remove the export:-RCT_EXPORT_MODULE(VisionRTC) +// TurboModule-only; no legacy export
| override fun createTrack(source: ReadableMap?, opts: ReadableMap?, promise: Promise) { | ||
| var width: Int = 1280 | ||
| var height: Int = 720 | ||
| var fps: Int = 30 | ||
|
|
||
| if (opts != null) { | ||
| if (opts.hasKey("resolution")) { | ||
| val resolution = opts.getMap("resolution") | ||
| if (resolution != null) { | ||
| if (resolution.hasKey("width")) { | ||
| val w = resolution.getDouble("width") | ||
| if (!w.isNaN()) width = w.roundToInt() | ||
| } | ||
| if (resolution.hasKey("height")) { | ||
| val h = resolution.getDouble("height") | ||
| if (!h.isNaN()) height = h.roundToInt() | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (opts.hasKey("fps")) { | ||
| val f = opts.getDouble("fps") | ||
| if (!f.isNaN()) fps = f.roundToInt() | ||
| } | ||
| } | ||
|
|
||
| targetFps = fps | ||
|
|
||
| val videoSource = factory.createVideoSource(false) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate constraints to prevent OOM/crashes.
Width/height/fps can be zero/negative/huge leading to allocation failures in JavaI420Buffer.
Apply this diff:
@@
override fun createTrack(source: ReadableMap?, opts: ReadableMap?, promise: Promise) {
@@
if (opts != null) {
@@
}
+ // Sanity-check to avoid invalid allocations and runaway CPU
+ if (width <= 0 || height <= 0 || fps <= 0 || width > 4096 || height > 4096 || fps > 240) {
+ promise.reject("E_INVALID_CONSTRAINTS", "Invalid constraints: width=$width height=$height fps=$fps")
+ return
+ }
+
targetFps = fps📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| override fun createTrack(source: ReadableMap?, opts: ReadableMap?, promise: Promise) { | |
| var width: Int = 1280 | |
| var height: Int = 720 | |
| var fps: Int = 30 | |
| if (opts != null) { | |
| if (opts.hasKey("resolution")) { | |
| val resolution = opts.getMap("resolution") | |
| if (resolution != null) { | |
| if (resolution.hasKey("width")) { | |
| val w = resolution.getDouble("width") | |
| if (!w.isNaN()) width = w.roundToInt() | |
| } | |
| if (resolution.hasKey("height")) { | |
| val h = resolution.getDouble("height") | |
| if (!h.isNaN()) height = h.roundToInt() | |
| } | |
| } | |
| } | |
| if (opts.hasKey("fps")) { | |
| val f = opts.getDouble("fps") | |
| if (!f.isNaN()) fps = f.roundToInt() | |
| } | |
| } | |
| targetFps = fps | |
| val videoSource = factory.createVideoSource(false) | |
| override fun createTrack(source: ReadableMap?, opts: ReadableMap?, promise: Promise) { | |
| var width: Int = 1280 | |
| var height: Int = 720 | |
| var fps: Int = 30 | |
| if (opts != null) { | |
| if (opts.hasKey("resolution")) { | |
| val resolution = opts.getMap("resolution") | |
| if (resolution != null) { | |
| if (resolution.hasKey("width")) { | |
| val w = resolution.getDouble("width") | |
| if (!w.isNaN()) width = w.roundToInt() | |
| } | |
| if (resolution.hasKey("height")) { | |
| val h = resolution.getDouble("height") | |
| if (!h.isNaN()) height = h.roundToInt() | |
| } | |
| } | |
| } | |
| if (opts.hasKey("fps")) { | |
| val f = opts.getDouble("fps") | |
| if (!f.isNaN()) fps = f.roundToInt() | |
| } | |
| } | |
| // Sanity-check to avoid invalid allocations and runaway CPU | |
| if (width <= 0 || height <= 0 || fps <= 0 || width > 4096 || height > 4096 || fps > 240) { | |
| promise.reject("E_INVALID_CONSTRAINTS", "Invalid constraints: width=$width height=$height fps=$fps") | |
| return | |
| } | |
| targetFps = fps | |
| val videoSource = factory.createVideoSource(false) | |
| // ... | |
| } |
🤖 Prompt for AI Agents
In android/src/main/java/com/visionrtc/VisionRtcModule.kt around lines 69 to 97,
the parsed width/height/fps from opts are not validated and can be
zero/negative/or absurdly large causing JavaI420Buffer allocation OOMs/crashes;
clamp or validate these values after parsing (e.g. enforce width/height within
sane bounds such as min 64, max 3840 and fps within min 1, max 60), replace
out-of-range values with safe defaults (or reject the promise with a clear
error) and then assign targetFps from the validated/clamped fps before creating
the video source; ensure all subsequent uses rely on the validated values.
| override fun replaceSenderTrack(senderId: String?, newTrackId: String?, promise: Promise) { | ||
| promise.resolve(null) | ||
| } | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Avoid silent no-op: reject unsupported replaceSenderTrack.
Resolving success misleads the caller and hides missing functionality.
Apply this diff:
override fun replaceSenderTrack(senderId: String?, newTrackId: String?, promise: Promise) {
- promise.resolve(null)
+ promise.reject("E_NOT_IMPLEMENTED", "replaceSenderTrack is not implemented on Android yet")
}🏁 Script executed:
#!/bin/bash
# Find JS/TS call sites that rely on replaceSenderTrack so we can gate or polyfill.
rg -nPI --type ts --type tsx --type js --type jsx -C2 '\breplaceSenderTrack\s*\('Length of output: 27
🏁 Script executed:
#!/bin/bash
# Search for any references to replaceSenderTrack across JS/TS files.
rg -nP 'replaceSenderTrack' -g '*.{js,jsx,ts,tsx}'Length of output: 142
🏁 Script executed:
#!/bin/bash
# Display the code around line 35 to inspect how replaceSenderTrack is used
sed -n '25,60p' src/index.tsLength of output: 979
Reject unsupported replaceSenderTrack
Resolving with null hides missing functionality and misleads callers; reject the promise instead:
override fun replaceSenderTrack(senderId: String?, newTrackId: String?, promise: Promise) {
- promise.resolve(null)
+ promise.reject("E_NOT_IMPLEMENTED", "replaceSenderTrack is not implemented on Android yet")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| override fun replaceSenderTrack(senderId: String?, newTrackId: String?, promise: Promise) { | |
| promise.resolve(null) | |
| } | |
| override fun replaceSenderTrack(senderId: String?, newTrackId: String?, promise: Promise) { | |
| promise.reject("E_NOT_IMPLEMENTED", "replaceSenderTrack is not implemented on Android yet") | |
| } |
🤖 Prompt for AI Agents
In android/src/main/java/com/visionrtc/VisionRtcModule.kt around lines 113 to
116, the override of replaceSenderTrack currently resolves the promise with null
which hides the missing implementation; instead reject the promise with a clear
error code and message indicating the operation is unsupported (include senderId
and newTrackId in the message for context) so callers receive an explicit
failure; use promise.reject(...) with an appropriate error code like
"UNSUPPORTED_OPERATION" and a descriptive message.
| override fun setTrackConstraints(trackId: String, opts: ReadableMap, promise: Promise) { | ||
| val handle = tracks[trackId] | ||
| if (handle == null) { | ||
| promise.reject("ERR_UNKNOWN_TRACK", "Unknown trackId: $trackId") | ||
| return | ||
| } | ||
|
|
||
| val cap = handle.capturer | ||
|
|
||
| var nextWidth: Int? = null | ||
| var nextHeight: Int? = null | ||
| var nextFps: Int? = null | ||
|
|
||
| if (opts.hasKey("resolution")) { | ||
| val resolution = opts.getMap("resolution") | ||
| if (resolution != null) { | ||
| if (resolution.hasKey("width")) { | ||
| val w = resolution.getDouble("width") | ||
| if (!w.isNaN()) nextWidth = w.roundToInt() | ||
| } | ||
| if (resolution.hasKey("height")) { | ||
| val h = resolution.getDouble("height") | ||
| if (!h.isNaN()) nextHeight = h.roundToInt() | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (opts.hasKey("fps")) { | ||
| val f = opts.getDouble("fps") | ||
| if (!f.isNaN()) nextFps = f.roundToInt() | ||
| } | ||
|
|
||
| if (nextWidth != null && nextHeight != null) { | ||
| cap.setResolution(nextWidth, nextHeight) | ||
| } | ||
| if (nextFps != null) { | ||
| cap.setFps(nextFps) | ||
| } | ||
|
|
||
| targetFps = cap.fps | ||
| promise.resolve(null) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard setTrackConstraints with range checks; handle paused-reschedule case.
Invalid dimensions/fps should be rejected; also consider rescheduling when paused (see capturer comment below).
Apply this diff:
override fun setTrackConstraints(trackId: String, opts: ReadableMap, promise: Promise) {
@@
- if (nextWidth != null && nextHeight != null) {
+ // Validate before applying
+ if (nextWidth != null && (nextWidth!! <= 0 || nextWidth!! > 4096)) {
+ promise.reject("E_INVALID_CONSTRAINTS", "Invalid width: $nextWidth"); return
+ }
+ if (nextHeight != null && (nextHeight!! <= 0 || nextHeight!! > 4096)) {
+ promise.reject("E_INVALID_CONSTRAINTS", "Invalid height: $nextHeight"); return
+ }
+ if (nextFps != null && (nextFps!! <= 0 || nextFps!! > 240)) {
+ promise.reject("E_INVALID_CONSTRAINTS", "Invalid fps: $nextFps"); return
+ }
+
+ if (nextWidth != null && nextHeight != null) {
cap.setResolution(nextWidth, nextHeight)
}
if (nextFps != null) {
cap.setFps(nextFps)
}
targetFps = cap.fps
promise.resolve(null)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| override fun setTrackConstraints(trackId: String, opts: ReadableMap, promise: Promise) { | |
| val handle = tracks[trackId] | |
| if (handle == null) { | |
| promise.reject("ERR_UNKNOWN_TRACK", "Unknown trackId: $trackId") | |
| return | |
| } | |
| val cap = handle.capturer | |
| var nextWidth: Int? = null | |
| var nextHeight: Int? = null | |
| var nextFps: Int? = null | |
| if (opts.hasKey("resolution")) { | |
| val resolution = opts.getMap("resolution") | |
| if (resolution != null) { | |
| if (resolution.hasKey("width")) { | |
| val w = resolution.getDouble("width") | |
| if (!w.isNaN()) nextWidth = w.roundToInt() | |
| } | |
| if (resolution.hasKey("height")) { | |
| val h = resolution.getDouble("height") | |
| if (!h.isNaN()) nextHeight = h.roundToInt() | |
| } | |
| } | |
| } | |
| if (opts.hasKey("fps")) { | |
| val f = opts.getDouble("fps") | |
| if (!f.isNaN()) nextFps = f.roundToInt() | |
| } | |
| if (nextWidth != null && nextHeight != null) { | |
| cap.setResolution(nextWidth, nextHeight) | |
| } | |
| if (nextFps != null) { | |
| cap.setFps(nextFps) | |
| } | |
| targetFps = cap.fps | |
| promise.resolve(null) | |
| } | |
| override fun setTrackConstraints(trackId: String, opts: ReadableMap, promise: Promise) { | |
| val handle = tracks[trackId] | |
| if (handle == null) { | |
| promise.reject("ERR_UNKNOWN_TRACK", "Unknown trackId: $trackId") | |
| return | |
| } | |
| val cap = handle.capturer | |
| var nextWidth: Int? = null | |
| var nextHeight: Int? = null | |
| var nextFps: Int? = null | |
| if (opts.hasKey("resolution")) { | |
| val resolution = opts.getMap("resolution") | |
| if (resolution != null) { | |
| if (resolution.hasKey("width")) { | |
| val w = resolution.getDouble("width") | |
| if (!w.isNaN()) nextWidth = w.roundToInt() | |
| } | |
| if (resolution.hasKey("height")) { | |
| val h = resolution.getDouble("height") | |
| if (!h.isNaN()) nextHeight = h.roundToInt() | |
| } | |
| } | |
| } | |
| if (opts.hasKey("fps")) { | |
| val f = opts.getDouble("fps") | |
| if (!f.isNaN()) nextFps = f.roundToInt() | |
| } | |
| // Validate before applying | |
| if (nextWidth != null && (nextWidth <= 0 || nextWidth > 4096)) { | |
| promise.reject("E_INVALID_CONSTRAINTS", "Invalid width: $nextWidth") | |
| return | |
| } | |
| if (nextHeight != null && (nextHeight <= 0 || nextHeight > 4096)) { | |
| promise.reject("E_INVALID_CONSTRAINTS", "Invalid height: $nextHeight") | |
| return | |
| } | |
| if (nextFps != null && (nextFps <= 0 || nextFps > 240)) { | |
| promise.reject("E_INVALID_CONSTRAINTS", "Invalid fps: $nextFps") | |
| return | |
| } | |
| if (nextWidth != null && nextHeight != null) { | |
| cap.setResolution(nextWidth, nextHeight) | |
| } | |
| if (nextFps != null) { | |
| cap.setFps(nextFps) | |
| } | |
| targetFps = cap.fps | |
| promise.resolve(null) | |
| } |
🤖 Prompt for AI Agents
android/src/main/java/com/visionrtc/VisionRtcModule.kt around lines 133-174:
validate incoming resolution and fps values and handle the case where the
capturer is paused by deferring the change. Ensure width/height are integers >0
and within reasonable limits (e.g., <=8192) and fps is integer >0 and <=120; if
any value is out of range or NaN, call promise.reject with a clear error
code/message and return. If the capturer is currently paused, do not call
cap.setResolution/setFps immediately — instead store the requested
nextWidth/nextHeight/nextFps on the track handle as pending constraints (e.g.,
handle.pendingConstraints = ...), resolve the promise, and ensure the capturer
applies pendingConstraints when it resumes; otherwise, apply
cap.setResolution/setFps as before and update targetFps before resolving.
| fun start() { | ||
| if (running) return | ||
| running = true | ||
| val periodNs = (1_000_000_000L / fps.toLong()) | ||
| scheduled = executor.scheduleAtFixedRate({ tick() }, 0L, periodNs, TimeUnit.NANOSECONDS) | ||
| } | ||
|
|
||
| fun pause() { running = false } | ||
| fun resume() { running = true } | ||
| fun stop() { | ||
| running = false | ||
| scheduled?.cancel(true) | ||
| executor.shutdownNow() | ||
| } | ||
|
|
||
| fun setResolution(w: Int, h: Int) { width = w; height = h } | ||
| fun setFps(next: Int) { | ||
| if (next <= 0 || next == fps) return | ||
| fps = next | ||
| if (running) { | ||
| scheduled?.cancel(false) | ||
| val periodNs = (1_000_000_000L / fps.toLong()) | ||
| scheduled = executor.scheduleAtFixedRate({ tick() }, 0L, periodNs, TimeUnit.NANOSECONDS) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make pause/resume truly suspend scheduling; signal capturer start/stop.
Currently pause leaves the scheduled task running (wasted wakeups), and fps changes while paused don’t take effect on resume.
Apply this diff:
fun start() {
if (running) return
running = true
val periodNs = (1_000_000_000L / fps.toLong())
+ observer.onCapturerStarted(true)
scheduled = executor.scheduleAtFixedRate({ tick() }, 0L, periodNs, TimeUnit.NANOSECONDS)
}
- fun pause() { running = false }
- fun resume() { running = true }
+ fun pause() {
+ running = false
+ scheduled?.cancel(false)
+ scheduled = null
+ }
+ fun resume() {
+ if (running) return
+ running = true
+ val periodNs = (1_000_000_000L / fps.toLong())
+ scheduled = executor.scheduleAtFixedRate({ tick() }, 0L, periodNs, TimeUnit.NANOSECONDS)
+ }
@@
fun stop() {
running = false
scheduled?.cancel(true)
+ observer.onCapturerStopped()
executor.shutdownNow()
}
@@
- fun setFps(next: Int) {
+ fun setFps(next: Int) {
if (next <= 0 || next == fps) return
fps = next
- if (running) {
- scheduled?.cancel(false)
- val periodNs = (1_000_000_000L / fps.toLong())
- scheduled = executor.scheduleAtFixedRate({ tick() }, 0L, periodNs, TimeUnit.NANOSECONDS)
- }
+ // Reschedule only when running; resume() will schedule with current fps.
+ if (running) {
+ scheduled?.cancel(false)
+ val periodNs = (1_000_000_000L / fps.toLong())
+ scheduled = executor.scheduleAtFixedRate({ tick() }, 0L, periodNs, TimeUnit.NANOSECONDS)
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fun start() { | |
| if (running) return | |
| running = true | |
| val periodNs = (1_000_000_000L / fps.toLong()) | |
| scheduled = executor.scheduleAtFixedRate({ tick() }, 0L, periodNs, TimeUnit.NANOSECONDS) | |
| } | |
| fun pause() { running = false } | |
| fun resume() { running = true } | |
| fun stop() { | |
| running = false | |
| scheduled?.cancel(true) | |
| executor.shutdownNow() | |
| } | |
| fun setResolution(w: Int, h: Int) { width = w; height = h } | |
| fun setFps(next: Int) { | |
| if (next <= 0 || next == fps) return | |
| fps = next | |
| if (running) { | |
| scheduled?.cancel(false) | |
| val periodNs = (1_000_000_000L / fps.toLong()) | |
| scheduled = executor.scheduleAtFixedRate({ tick() }, 0L, periodNs, TimeUnit.NANOSECONDS) | |
| } | |
| } | |
| fun start() { | |
| if (running) return | |
| running = true | |
| val periodNs = (1_000_000_000L / fps.toLong()) | |
| observer.onCapturerStarted(true) | |
| scheduled = executor.scheduleAtFixedRate({ tick() }, 0L, periodNs, TimeUnit.NANOSECONDS) | |
| } | |
| fun pause() { | |
| running = false | |
| scheduled?.cancel(false) | |
| scheduled = null | |
| } | |
| fun resume() { | |
| if (running) return | |
| running = true | |
| val periodNs = (1_000_000_000L / fps.toLong()) | |
| scheduled = executor.scheduleAtFixedRate({ tick() }, 0L, periodNs, TimeUnit.NANOSECONDS) | |
| } | |
| fun stop() { | |
| running = false | |
| scheduled?.cancel(true) | |
| observer.onCapturerStopped() | |
| executor.shutdownNow() | |
| } | |
| fun setResolution(w: Int, h: Int) { width = w; height = h } | |
| fun setFps(next: Int) { | |
| if (next <= 0 || next == fps) return | |
| fps = next | |
| // Reschedule only when running; resume() will schedule with current fps. | |
| if (running) { | |
| scheduled?.cancel(false) | |
| val periodNs = (1_000_000_000L / fps.toLong()) | |
| scheduled = executor.scheduleAtFixedRate({ tick() }, 0L, periodNs, TimeUnit.NANOSECONDS) | |
| } | |
| } |
gmemmy
force-pushed
the
feat/native-skeleton
branch
from
892f20f to
ca26c49
Compare
1 participant
Summary by CodeRabbit