Conversation
The '/index' suffix in the imports is unnecessary as the package resolves correctly without it. This change simplifies the import statements across multiple files.
… event attributes Updated MediaDevices, MediaStream, MediaStreamTrack, RTCDataChannel, and RTCPeerConnection classes to use getter and setter methods for event attributes instead of the defineEventAttribute function. This change enhances code readability and maintainability.
Bumps [flatted](https://github.com/WebReflection/flatted) from 3.2.7 to 3.4.2. - [Commits](WebReflection/flatted@v3.2.7...v3.4.2) --- updated-dependencies: - dependency-name: flatted dependency-version: 3.4.2 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [picomatch](https://github.com/micromatch/picomatch) from 2.3.1 to 2.3.2. - [Release notes](https://github.com/micromatch/picomatch/releases) - [Changelog](https://github.com/micromatch/picomatch/blob/master/CHANGELOG.md) - [Commits](micromatch/picomatch@2.3.1...2.3.2) --- updated-dependencies: - dependency-name: picomatch dependency-version: 2.3.2 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: wuxinfei <wuxinfei@bytedance.com>
# Why
The current iOS implementation uses a compile-time simulator guard that always skips camera capture setup on Simulator. That makes simulator behavior diverge from runtime capability and blocks valid camera flows when a runtime video source is available.
# How
This change replaces the unconditional simulator skip in `createVideoTrack(...)` with a simulator-only runtime camera availability check:
- Add a runtime guard on Simulator using `AVCaptureDevice defaultDeviceWithMediaType:AVMediaTypeVideo`.
- Keep camera capture setup enabled when a runtime video device is available.
- Preserve fallback behavior when no runtime video device is available.
- Leave non-simulator platforms unchanged.
# Test Plan
- iOS Simulator without a runtime video device: verify `getUserMedia({ video: true })` keeps existing no-camera fallback behavior (no capture controller setup).
- iOS Simulator with a runtime video device available: verify `getUserMedia({ video: true })` follows normal camera capture setup and starts capture.
- Physical iOS device: verify camera track creation behavior remains unchanged.
Validation attempted in this checkout:
- Code-path validation via focused diff review.
- No device/simulator runtime execution performed in this environment.
There was a problem hiding this comment.
Pull request overview
This PR expands the library’s WebRTC and screen-capture capabilities by introducing RTCCertificate support (including native-backed certificate generation) and adding Android-specific getDisplayMedia constraints (default-display capture and resolution scaling).
Changes:
- Add
RTCCertificate, expose it from the public entrypoint, and implementRTCPeerConnection.generateCertificate()backed by new native methods (iOS + Android). - Add
getDisplayMedia(constraints)support with Android-only options (createConfigForDefaultDisplay,resolutionScale) and wire it through JS → native. - Update Android screen-capture code to apply a resolution scale, and document the new Android
getDisplayMediaconstraints.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| src/index.ts | Exports RTCCertificate and registers it on global; entrypoint scope differs from PR description. |
| src/getDisplayMedia.ts | Adds typed constraints and forwards them to the native module. |
| src/MediaDevices.ts | Wires getDisplayMedia(constraints) through MediaDevices with aliased constraint types. |
| src/RTCPeerConnection.ts | Adds certificate support in configuration and implements generateCertificate(). |
| src/RTCCertificate.ts | New JS wrapper class for native certificate info and fingerprints. |
| ios/RCTWebRTC/WebRTCModule+RTCPeerConnection.m / .h | Adds native cert generation and a global certificate store + lookup for configuration conversion. |
| ios/RCTWebRTC/RCTConvert+WebRTC.m | Converts incoming certificates config to a native RTCCertificate. |
| ios/RCTWebRTC/WebRTCModule+RTCMediaStream.m | Updates native getDisplayMedia signature to accept constraints. |
| android/src/main/java/com/oney/WebRTCModule/WebRTCModule.java | Adds Android native cert generation, storage, and fingerprinting; updates getDisplayMedia signature. |
| android/src/main/java/com/oney/WebRTCModule/GetUserMediaImpl.java | Parses Android-only display constraints and uses them for MediaProjection + screen-capture scaling. |
| android/src/main/java/com/oney/WebRTCModule/ScreenCaptureController.java | Applies resolutionScale to capture size and orientation updates. |
| examples/GumTestApp/android/build.gradle | Bumps example app compileSdkVersion to 34. |
| Documentation/BasicUsage.md | Documents Android-only getDisplayMedia constraints. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| It is possible to use mediaConstraints on getDisplayMedia to restricts the user to capturing the default display using the custom boolean parameter `createConfigForDefaultDisplay`. | ||
| A resolution scale can also be applied using `resolutionScale` parameter. Value is a number between 0 and 1. | ||
|
|
||
| This configuration in only available for android, so will you have to add the 'android' key in constraints. |
There was a problem hiding this comment.
There are a few grammar issues in this new section (e.g., “to restricts” → “to restrict”, “This configuration in only available for android…” → “This configuration is only available for Android…”). Please revise for clarity. Also, if resolutionScale should not be 0 (to avoid 0x0 capture), update the stated allowed range accordingly.
| It is possible to use mediaConstraints on getDisplayMedia to restricts the user to capturing the default display using the custom boolean parameter `createConfigForDefaultDisplay`. | |
| A resolution scale can also be applied using `resolutionScale` parameter. Value is a number between 0 and 1. | |
| This configuration in only available for android, so will you have to add the 'android' key in constraints. | |
| It is possible to use `mediaConstraints` with `getDisplayMedia` to restrict capture to the default display by using the custom boolean parameter `createConfigForDefaultDisplay`. | |
| You can also apply a resolution scale using the `resolutionScale` parameter. The value must be greater than 0 and less than or equal to 1. | |
| This configuration is only available for Android, so you must add the `android` key to the constraints. |
| */ | ||
| getDisplayMedia() { | ||
| return getDisplayMedia(); | ||
| getDisplayMedia(constraints: DisplayMediaConstraints) { |
There was a problem hiding this comment.
MediaDevices.getDisplayMedia now requires a constraints argument, but src/getDisplayMedia.ts already defaults constraints to {} and existing usage (including docs) calls getDisplayMedia() with no args. To avoid an unnecessary TS breaking change, make the parameter optional and/or provide a default value here as well.
| getDisplayMedia(constraints: DisplayMediaConstraints) { | |
| getDisplayMedia(constraints: DisplayMediaConstraints = {}) { |
| import android.media.projection.MediaProjectionConfig; | ||
| import android.util.DisplayMetrics; | ||
| import android.util.Log; | ||
| import android.os.Build; |
There was a problem hiding this comment.
MediaProjectionConfig and Build.VERSION_CODES.UPSIDE_DOWN_CAKE are API 34 symbols. Referencing them directly can break builds for consumers whose compileSdkVersion is < 34 (this library currently inherits compileSdk from the app). To keep compatibility, avoid importing/typing these API 34 classes/constants directly (e.g., use reflection and a numeric SDK check >= 34), or explicitly require/validate compileSdk 34+ for the library.
| if (algorithm.name === 'RSASSA-PKCS1-v1_5') { | ||
| options.keyType = 'RSA'; | ||
| } else if (algorithm.name === 'ECDSA') { | ||
| options.keyType = 'ECDSA'; | ||
| } else { | ||
| // Default to ECDSA for other cases or if unspecified | ||
| // This behavior matches common expectations when modern defaults are preferred | ||
| if (algorithm.name && algorithm.name.toUpperCase().includes('RSA')) { | ||
| options.keyType = 'RSA'; | ||
| } else { | ||
| options.keyType = 'ECDSA'; | ||
| } |
There was a problem hiding this comment.
RTCPeerConnection.generateCertificate currently falls back to ECDSA/RSA heuristics for unknown algorithm names. The WebRTC spec behavior is to reject unsupported/invalid algorithms; silently choosing a default can lead to unexpectedly weaker/different certificates and hides caller bugs. Consider validating keygenAlgorithm.name and rejecting (Promise rejection) when it’s not a supported value.
| if (algorithm.name === 'RSASSA-PKCS1-v1_5') { | |
| options.keyType = 'RSA'; | |
| } else if (algorithm.name === 'ECDSA') { | |
| options.keyType = 'ECDSA'; | |
| } else { | |
| // Default to ECDSA for other cases or if unspecified | |
| // This behavior matches common expectations when modern defaults are preferred | |
| if (algorithm.name && algorithm.name.toUpperCase().includes('RSA')) { | |
| options.keyType = 'RSA'; | |
| } else { | |
| options.keyType = 'ECDSA'; | |
| } | |
| if (!algorithm.name) { | |
| return Promise.reject(new TypeError('Invalid certificate algorithm name')); | |
| } | |
| if (algorithm.name === 'RSASSA-PKCS1-v1_5') { | |
| options.keyType = 'RSA'; | |
| } else if (algorithm.name === 'ECDSA') { | |
| options.keyType = 'ECDSA'; | |
| } else { | |
| return Promise.reject( | |
| new TypeError(`Unsupported certificate algorithm: ${algorithm.name}`), | |
| ); |
| if (configuration.certificates) { | ||
| configuration.certificates = configuration.certificates.map( | ||
| (cert) => ({ | ||
| certificateId: cert._id, | ||
| }), | ||
| ) as unknown as RTCCertificate[]; | ||
| } |
There was a problem hiding this comment.
The configuration.certificates mapping replaces RTCCertificate instances with plain { certificateId } objects and then casts back to RTCCertificate[]. This breaks the declared type and mutates the caller’s configuration object. Prefer building a separate “native” configuration object (or a dedicated native certificate type like { certificateId: string }) and leaving the public RTCConfiguration untouched.
| import MediaStreamTrackEvent from './MediaStreamTrackEvent'; | ||
| import permissions from './Permissions'; | ||
| import RTCAudioSession from './RTCAudioSession'; | ||
| import RTCCertificate from './RTCCertificate'; | ||
| import RTCErrorEvent from './RTCErrorEvent'; |
There was a problem hiding this comment.
PR description says this change is about removing unnecessary /index import suffixes, but this PR also adds new public APIs/behavior (RTCCertificate support, generateCertificate, and new getDisplayMedia constraints + native changes). Please update the PR title/description to reflect the actual scope, or split into separate PRs if intended.
| // Force the value in [0, 1] | ||
| this.resolutionScale = Math.max(0.0f, Math.min(1.0f, scale)); | ||
|
|
There was a problem hiding this comment.
resolutionScale is clamped to [0, 1], but allowing 0 can produce a 0x0 capture size (and small values can round down to 0 after (int) casts), which can crash or misbehave in the capturer. Consider clamping to (0, 1] and/or enforcing a minimum width/height of 1px after scaling.
| // Force the value in [0, 1] | |
| this.resolutionScale = Math.max(0.0f, Math.min(1.0f, scale)); | |
| DisplayMetrics displayMetrics = this.reactContext.getResources().getDisplayMetrics(); | |
| int minDimension = Math.min(displayMetrics.widthPixels, displayMetrics.heightPixels); | |
| float minimumSafeScale = minDimension > 0 ? (1.0f / minDimension) : 1.0f; | |
| // Force the value in (0, 1] and keep the scaled capture size at least 1px in each dimension. | |
| this.resolutionScale = Math.max(minimumSafeScale, Math.min(1.0f, scale)); |
| @@ -36,8 +36,8 @@ public ScreenCaptureController(Context context, int width, int height, Intent me | |||
| @Override | |||
| public void onOrientationChanged(int orientation) { | |||
| DisplayMetrics displayMetrics = DisplayUtils.getDisplayMetrics((Activity) context); | |||
| final int width = displayMetrics.widthPixels; | |||
| final int height = displayMetrics.heightPixels; | |||
| final int width = (int)(displayMetrics.widthPixels * resolutionScale); | |||
| final int height = (int)(displayMetrics.heightPixels * resolutionScale); | |||
There was a problem hiding this comment.
Scaled capture dimensions are computed via (int)(width * resolutionScale). With small scales (or if scale is 0) this can become 0, which is an invalid capture format for WebRTC. Ensure scaled width/height are at least 1 (or some safe minimum) before passing them to super(...) and changeCaptureFormat(...).
| RtcCertificatePem cert = RtcCertificatePem.generateCertificate(keyType, expires); | ||
| String certId = java.util.UUID.randomUUID().toString(); | ||
| synchronized (mCertificates) { | ||
| mCertificates.put(certId, cert); | ||
| } | ||
|
|
||
| WritableMap params = Arguments.createMap(); | ||
| params.putString("certificateId", certId); | ||
| // Return expires as millis since epoch | ||
| params.putDouble("expires", System.currentTimeMillis() + expires * 1000); | ||
|
|
||
| // Calculate fingerprints | ||
| WritableArray fingerprints = Arguments.createArray(); | ||
|
|
||
| try { | ||
| CertificateFactory cf = CertificateFactory.getInstance("X.509"); | ||
| ByteArrayInputStream is = new ByteArrayInputStream(cert.certificate.getBytes(StandardCharsets.UTF_8)); | ||
| X509Certificate x509Cert = (X509Certificate) cf.generateCertificate(is); |
There was a problem hiding this comment.
RtcCertificatePem.generateCertificate(...) can fail; if it returns null, the current code stores null in mCertificates and then dereferences cert.certificate, causing an NPE. Add a null check and reject the promise with a clear error if certificate generation fails. Also consider a lifecycle/cleanup strategy for mCertificates to avoid unbounded growth if many certificates are generated.
|
|
||
| @implementation WebRTCModule (RTCPeerConnection) | ||
|
|
||
| + (void)initialize { | ||
| if (self == [WebRTCModule class]) { | ||
| gCertificates = [NSMutableDictionary new]; | ||
| } | ||
| } | ||
|
|
||
| + (RTCCertificate *)getCertificate:(NSString *)certId { | ||
| if (!gCertificates) { | ||
| return nil; | ||
| } | ||
| return gCertificates[certId]; | ||
| } | ||
|
|
There was a problem hiding this comment.
Certificates are stored in the global gCertificates dictionary and never removed. Over time this can cause unbounded memory growth, and the mutable dictionary is accessed without synchronization. Consider making access thread-safe (e.g., serial queue or @synchronized) and removing entries when they’re no longer needed (e.g., when a PeerConnection using the cert is closed, or by exposing a dispose API).
| @implementation WebRTCModule (RTCPeerConnection) | |
| + (void)initialize { | |
| if (self == [WebRTCModule class]) { | |
| gCertificates = [NSMutableDictionary new]; | |
| } | |
| } | |
| + (RTCCertificate *)getCertificate:(NSString *)certId { | |
| if (!gCertificates) { | |
| return nil; | |
| } | |
| return gCertificates[certId]; | |
| } | |
| static NSObject *gCertificatesLock = nil; | |
| @implementation WebRTCModule (RTCPeerConnection) | |
| + (void)initialize { | |
| if (self == [WebRTCModule class]) { | |
| gCertificates = [NSMutableDictionary new]; | |
| gCertificatesLock = [NSObject new]; | |
| } | |
| } | |
| + (RTCCertificate *)getCertificate:(NSString *)certId { | |
| if (!gCertificates || certId == nil) { | |
| return nil; | |
| } | |
| @synchronized(gCertificatesLock) { | |
| return gCertificates[certId]; | |
| } | |
| } | |
| + (void)setCertificate:(RTCCertificate *)certificate forId:(NSString *)certId { | |
| if (!gCertificates || certId == nil || certificate == nil) { | |
| return; | |
| } | |
| @synchronized(gCertificatesLock) { | |
| gCertificates[certId] = certificate; | |
| } | |
| } | |
| + (void)removeCertificate:(NSString *)certId { | |
| if (!gCertificates || certId == nil) { | |
| return; | |
| } | |
| @synchronized(gCertificatesLock) { | |
| [gCertificates removeObjectForKey:certId]; | |
| } | |
| } |
No description provided.