WebRTC Client. Expose core dependancies. #5126
Conversation
WalkthroughExposes underlying native WebRTC peer connections via new public getNative() extensions on WebRtcPeerConnection for Android and iOS, adjusts peerConnection field visibility to internal, updates API signatures (capturer/ senders), and changes build configuration and dependency exposures for the webrtc client module. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 0
🧹 Nitpick comments (8)
ktor-client/ktor-client-webrtc/api/ktor-client-webrtc.klib.api (2)
1072-1073: Consider adding documentation and opt-in annotation.The
getNative()extension exposes platform-specific internals. Consider:
- Adding KDoc to explain the returned type and typical use cases
- Adding an
@ExperimentalAPIor similar opt-in annotation to signal that this API provides low-level accessExample documentation:
/** * Returns the native RTCPeerConnection instance used by this WebRTC peer connection. * * This provides direct access to the underlying WebRTC SDK's peer connection for advanced * use cases that require platform-specific APIs. Use with caution as this bypasses * Ktor's abstraction layer. * * @return The native RTCPeerConnection from the WebRTC SDK */As per coding guidelines
1076-1077: Add documentation for the factory function.The
defaultVideoCapturerFactory()function should document what default implementation it provides and when developers should use it versus creating custom factories.As per coding guidelines
ktor-client/ktor-client-webrtc/ios/src/io/ktor/client/webrtc/PeerConnection.kt (1)
259-265: Good documentation and proper annotations.The function is well-documented with an appropriate warning. The
@OptIn(ExperimentalForeignApi::class)annotation correctly reflects that this exposes platform-specific APIs.One minor suggestion: Consider adding a
@seeor reference to the WebRTC SDK documentation for developers unfamiliar withRTCPeerConnection.ktor-client/ktor-client-webrtc/android/src/io/ktor/client/webrtc/Engine.kt (1)
63-66: Add KDoc for the factory object.The new
AndroidWebRtcfactory object provides a clean API for creating WebRTC engines. Consider adding KDoc to explain its purpose and provide usage examples.Example:
/** * Factory for creating Android WebRTC client engines. * * Example usage: * ```kotlin * val client = WebRtcClient(AndroidWebRtc) { * context = applicationContext * } * ``` */ public object AndroidWebRtc : WebRtcClientEngineFactory<AndroidWebRtcEngineConfig> { // ... }As per coding guidelines
ktor-client/ktor-client-webrtc/android/src/io/ktor/client/webrtc/PeerConnection.kt (1)
223-228: Well-documented with room for enhancement.The function includes an appropriate warning. Consider enhancing the documentation to match the level of detail suggested for the iOS version:
/** * Returns the native PeerConnection instance used by this WebRTC peer connection. * * This provides direct access to the underlying WebRTC SDK's peer connection for advanced * use cases that require platform-specific APIs. Use with caution as this bypasses * Ktor's abstraction layer. * * @return The native PeerConnection from the org.webrtc library * @see <a href="https://webrtc.github.io/webrtc-org/native-code/native-apis/">WebRTC Native APIs</a> */ public fun WebRtcPeerConnection.getNative(): PeerConnection { return (this as AndroidWebRtcPeerConnection).peerConnection }ktor-client/ktor-client-webrtc/build.gradle.kts (3)
43-43: Exposing kotlin-wrappers as API: verify it’s requiredUsing api(kotlinWrappers.browser) leaks wrappers to consumers and pins their versions. If your public API does not expose wrapper types/symbols, prefer implementation to avoid constraining consumers.
Can you confirm whether any public APIs in this module surface browser wrapper types? If not, please keep it as implementation.
53-53: Android WebRTC dependency via api: confirm public surface and versioning impactapi(libs.stream.webrtc.android) makes this a transitive requirement for consumers. This is fine if public APIs surface types from that SDK; otherwise, prefer implementation to avoid version lock/conflicts in apps that already depend on the same SDK.
If exposure is required, consider documenting the expected version range and any known conflicts in the module’s README/release notes to aid consumers.
74-74: Static iOS framework: check interplay with WebRTC-SDK and doc the integrationisStatic = true can reduce transitive linking issues, but if WebRTC-SDK is distributed as (or requires) dynamic frameworks, consumers may still need to add the pod to avoid linker errors and could hit duplicate symbols if both sides provide the same objects.
- Please validate with a sample iOS app in both scenarios: with and without adding the WebRTC-SDK pod.
- Add a brief note to docs on when iOS consumers must add the pod explicitly and potential duplicate symbol pitfalls.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
ktor-client/ktor-client-webrtc/android/src/io/ktor/client/webrtc/Engine.kt(2 hunks)ktor-client/ktor-client-webrtc/android/src/io/ktor/client/webrtc/PeerConnection.kt(2 hunks)ktor-client/ktor-client-webrtc/api/android/ktor-client-webrtc.api(1 hunks)ktor-client/ktor-client-webrtc/api/ktor-client-webrtc.klib.api(4 hunks)ktor-client/ktor-client-webrtc/build.gradle.kts(4 hunks)ktor-client/ktor-client-webrtc/ios/src/io/ktor/client/webrtc/PeerConnection.kt(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{kt,kts}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{kt,kts}: Follow Kotlin official style guide for all Kotlin source and build scripts
Use star imports for io.ktor.* packages
Max line length is 120 characters
Indent with 4 spaces in Kotlin code
Include a copyright header in new Kotlin files
Files:
ktor-client/ktor-client-webrtc/ios/src/io/ktor/client/webrtc/PeerConnection.ktktor-client/ktor-client-webrtc/android/src/io/ktor/client/webrtc/Engine.ktktor-client/ktor-client-webrtc/android/src/io/ktor/client/webrtc/PeerConnection.ktktor-client/ktor-client-webrtc/build.gradle.kts
**/*.kt
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.kt: Document all public Kotlin APIs, including parameters, return types, and exceptions
Annotate internal APIs with @internalapi
Follow Kotlin error-handling conventions and use specific Ktor exceptions
Files:
ktor-client/ktor-client-webrtc/ios/src/io/ktor/client/webrtc/PeerConnection.ktktor-client/ktor-client-webrtc/android/src/io/ktor/client/webrtc/Engine.ktktor-client/ktor-client-webrtc/android/src/io/ktor/client/webrtc/PeerConnection.kt
**/api/**
📄 CodeRabbit inference engine (CLAUDE.md)
Track all public API changes in the /api/ directories
Files:
ktor-client/ktor-client-webrtc/api/android/ktor-client-webrtc.apiktor-client/ktor-client-webrtc/api/ktor-client-webrtc.klib.api
🧠 Learnings (3)
📚 Learning: 2025-08-26T06:07:28.352Z
Learnt from: CR
PR: ktorio/ktor#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-26T06:07:28.352Z
Learning: Project is Kotlin Multiplatform (JVM, JS, Native)
Applied to files:
ktor-client/ktor-client-webrtc/build.gradle.kts
📚 Learning: 2025-08-14T15:17:11.466Z
Learnt from: zibet27
PR: ktorio/ktor#5044
File: ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/build.gradle.kts:12-12
Timestamp: 2025-08-14T15:17:11.466Z
Learning: The Cargo plugin (dev.gobley.cargo) used in the Ktor WebRTC RS module depends on the kotlin("plugin.atomicfu") plugin, so atomicfu should not be removed even if there are no direct kotlinx.atomicfu imports in the module's source code.
Applied to files:
ktor-client/ktor-client-webrtc/build.gradle.kts
📚 Learning: 2025-08-14T15:17:11.466Z
Learnt from: zibet27
PR: ktorio/ktor#5044
File: ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/build.gradle.kts:12-12
Timestamp: 2025-08-14T15:17:11.466Z
Learning: The Gobley Cargo plugin (dev.gobley.cargo) used in the Ktor WebRTC RS module requires the kotlin("plugin.atomicfu") plugin as a dependency, so the atomicfu plugin should not be removed even if there are no direct kotlinx.atomicfu imports in the module's source code.
Applied to files:
ktor-client/ktor-client-webrtc/build.gradle.kts
🔇 Additional comments (7)
ktor-client/ktor-client-webrtc/api/ktor-client-webrtc.klib.api (1)
831-831: LGTM: Capturer now extends AutoCloseable.This change improves resource management by ensuring all video capturers can be properly closed. The implementing classes (
CameraVideoCapturerandSimulatorVideoCapturer) both provide the requiredclose()method.ktor-client/ktor-client-webrtc/ios/src/io/ktor/client/webrtc/PeerConnection.kt (1)
30-30: LGTM: Visibility change enables native access.Changing
peerConnectiontointernalis necessary to support the publicgetNative()extension function, aligning with the PR's objective to expose core dependencies.ktor-client/ktor-client-webrtc/android/src/io/ktor/client/webrtc/Engine.kt (1)
34-36: LGTM: Improved error handling.The refactored
localFactoryproperty provides clearer error messaging and properly handles the case where a customMediaTrackFactoryis provided without a correspondingrtcFactory.ktor-client/ktor-client-webrtc/android/src/io/ktor/client/webrtc/PeerConnection.kt (1)
21-21: LGTM: Consistent visibility change across platforms.The visibility change from
privatetointernalmirrors the iOS implementation and is necessary to support thegetNative()extension function.ktor-client/ktor-client-webrtc/api/android/ktor-client-webrtc.api (1)
163-165: KDoc Present for getNative Methods KDoc exists for bothgetNativemethods in PeerConnection.kt and Senders.kt; no further documentation required.ktor-client/ktor-client-webrtc/build.gradle.kts (2)
4-4: Opt-in annotations/imports look correctAnnotation placement and imports are valid for Wasm DSL and Gradle Plugin API usage. LGTM.
Also applies to: 7-8
69-69: CocoaPods cinterop flag (-fmodules): validate portabilityEnsure this flag is actually applied to cinterop (not just the Kotlin compiler) and that builds pass for:
- device + simulator (arm64 + x86_64)
- Xcode 15/16
If this was added to address a specific header/module parsing error, briefly document the rationale in a comment for future maintainers.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ktor-client/ktor-client-webrtc/android/src/io/ktor/client/webrtc/PeerConnection.kt(2 hunks)ktor-client/ktor-client-webrtc/ios/src/io/ktor/client/webrtc/PeerConnection.kt(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ktor-client/ktor-client-webrtc/android/src/io/ktor/client/webrtc/PeerConnection.kt
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{kt,kts}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{kt,kts}: Follow Kotlin official style guide for all Kotlin source and build scripts
Use star imports for io.ktor.* packages
Max line length is 120 characters
Indent with 4 spaces in Kotlin code
Include a copyright header in new Kotlin files
Files:
ktor-client/ktor-client-webrtc/ios/src/io/ktor/client/webrtc/PeerConnection.kt
**/*.kt
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.kt: Document all public Kotlin APIs, including parameters, return types, and exceptions
Annotate internal APIs with @internalapi
Follow Kotlin error-handling conventions and use specific Ktor exceptions
Files:
ktor-client/ktor-client-webrtc/ios/src/io/ktor/client/webrtc/PeerConnection.kt
🔇 Additional comments (1)
ktor-client/ktor-client-webrtc/ios/src/io/ktor/client/webrtc/PeerConnection.kt (1)
30-30: LGTM! Appropriate visibility for internal access.Changing to
internalallows thegetNative()extension function to access this property while keeping it unexposed outside the module.
| /** | ||
| * Returns implementation of the peer connection that is used under the hood. Use it with caution. | ||
| */ | ||
| @OptIn(ExperimentalForeignApi::class) | ||
| public fun WebRtcPeerConnection.getNative(): RTCPeerConnection { | ||
| return (this as IosWebRtcConnection).peerConnection | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Document exceptions and add initialization check.
The function has the following concerns:
-
Uninitialized property access risk: Accessing
peerConnectionwithout verifying initialization will throwUninitializedPropertyAccessExceptionifinitialize()hasn't been called. -
Insufficient documentation: Per coding guidelines, public Kotlin APIs must document parameters, return types, and exceptions. The current documentation doesn't mention:
- The precondition that
initialize()must be called first - Potential exceptions (
ClassCastException,UninitializedPropertyAccessException) - When and why developers should use this function
- The precondition that
As per coding guidelines
Apply this diff to improve safety and documentation:
/**
- * Returns implementation of the peer connection that is used under the hood. Use it with caution.
+ * Returns the native RTCPeerConnection implementation used under the hood.
+ *
+ * This function provides direct access to the platform-specific peer connection for advanced use cases
+ * that require native iOS WebRTC SDK APIs not exposed by the Ktor wrapper.
+ *
+ * @return the underlying [RTCPeerConnection] instance.
+ * @throws UninitializedPropertyAccessException if [IosWebRtcConnection.initialize] has not been called.
+ * @throws ClassCastException if called on a non-iOS WebRtcPeerConnection implementation.
*/
@OptIn(ExperimentalForeignApi::class)
public fun WebRtcPeerConnection.getNative(): RTCPeerConnection {
- return (this as IosWebRtcConnection).peerConnection
+ val connection = this as IosWebRtcConnection
+ check(connection::peerConnection.isInitialized) {
+ "Peer connection must be initialized before accessing native implementation. Call initialize() first."
+ }
+ return connection.peerConnection
}Alternatively, if you prefer to let it fail naturally without the check (for performance reasons), at minimum enhance the documentation to warn users about the precondition.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find IosWebRtcConnection class definition and peerConnection property
find . -type f -name "*.kt" | head -20
Length of output: 1355
🏁 Script executed:
#!/bin/bash
# Search for IosWebRtcConnection class
rg -n "class IosWebRtcConnection" --type=ktLength of output: 26
🏁 Script executed:
#!/bin/bash
# Search for peerConnection property declaration
rg -n "peerConnection" --type=kt -A 2 -B 2 | head -50Length of output: 26
🏁 Script executed:
#!/bin/bash
# Search for initialize function in WebRTC context
rg -n "fun initialize" --type=kt -C 3 | grep -A5 -B5 -i "webrtc\|peer"Length of output: 26
🏁 Script executed:
#!/bin/bash
# Search for calls to getNative()
rg -n "getNative\(\)" --type=kt -B 3 -A 3Length of output: 26
🏁 Script executed:
#!/bin/bash
# Look at the full PeerConnection.kt file structure
fd "PeerConnection.kt" --type=fLength of output: 340
🏁 Script executed:
#!/bin/bash
# Check if there's initialization guarantee before getNative access
rg -n "class.*WebRtcPeerConnection\|lateinit.*peerConnection\|fun initialize" --type=kt -C 2Length of output: 26
🏁 Script executed:
#!/bin/bash
# Read the iOS PeerConnection.kt file directly
cat ktor-client/ktor-client-webrtc/ios/src/io/ktor/client/webrtc/PeerConnection.ktLength of output: 10734
🏁 Script executed:
#!/bin/bash
# Search for IosWebRtcConnection without type restriction
rg -n "class IosWebRtcConnection" Length of output: 115
🏁 Script executed:
#!/bin/bash
# Search for peerConnection property
rg -n "lateinit.*peerConnection\|val peerConnection\|var peerConnection" Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for getNative usage
rg -n "getNative"Length of output: 14308
🏁 Script executed:
#!/bin/bash
# Search for initialize() function in WebRTC files
rg -n "fun initialize\(" ktor-client/ktor-client-webrtc/Length of output: 757
🏁 Script executed:
#!/bin/bash
# Read the common WebRtcPeerConnection interface
cat ktor-client/ktor-client-webrtc/common/src/io/ktor/client/webrtc/WebRtcPeerConnection.ktLength of output: 4153
Enhance documentation and add initialization check for better robustness and clarity.
The review identifies two valid concerns in the getNative() function:
-
Documentation gap: The current documentation violates Kotlin guidelines by not documenting exceptions or preconditions. The function should clarify that
initialize()must be called first and document potential exceptions (UninitializedPropertyAccessException). -
Initialization risk: The function directly accesses the
lateinitproperty without verification. This is inconsistent with the defensive pattern used elsewhere in the same class—for example,getStatistics()(line 33) checksif (!this::peerConnection.isInitialized)before access.
The suggested improvements are reasonable and align with Kotlin coding standards.
🤖 Prompt for AI Agents
In
ktor-client/ktor-client-webrtc/ios/src/io/ktor/client/webrtc/PeerConnection.kt
around lines 260 to 266, the getNative() function should be hardened and
documented: update the KDoc to state that initialize() must be called before use
and document that an UninitializedPropertyAccessException is thrown if the
underlying peerConnection is not initialized, then add a defensive runtime check
(e.g., if (!this::peerConnection.isInitialized) throw
UninitializedPropertyAccessException("peerConnection has not been initialized;
call initialize() first")) before returning peerConnection so behavior is
consistent with other methods like getStatistics().
There was a problem hiding this comment.
The user of the API should never see this Exception or call initialize().
There was a problem hiding this comment.
@zibet27, thank you for the clarification! That's an important design detail.
If initialize() is called internally by the framework and users never need to invoke it directly, then:
-
Should
initialize()beinternalinstead ofpublic? Currently it's a public method (line 48), which might confuse users about whether they need to call it. -
For the documentation: Instead of mentioning initialization requirements, we could simplify to focus on the actual usage:
/**
* Returns the native RTCPeerConnection implementation for advanced use cases
* requiring direct access to iOS WebRTC SDK APIs.
*
* @return the underlying [RTCPeerConnection] instance.
* @throws ClassCastException if called on a non-iOS WebRtcPeerConnection implementation.
*/
@OptIn(ExperimentalForeignApi::class)
public fun WebRtcPeerConnection.getNative(): RTCPeerConnection {
return (this as IosWebRtcConnection).peerConnection
}Could you clarify:
- Is
initialize()guaranteed to be called before users can obtain aWebRtcPeerConnectioninstance? - Should
initialize()visibility be changed tointernal?
This will help ensure the documentation accurately reflects the intended API usage pattern.
Subsystem
WebRTC Client.
Motivation
KTOR-8977 Expose Core dependencies in WebRTC Client
Solution
Use
apiinstead ofimplementationfor necessary dependencies.It works for Android and JS, but on iOS, it's not possible to make the Cocoapod dependency transitive.
The user needs to add the WebRTC-SDK Cocoapod dependency for the linker.
Minor:
WebRtcPeerConnection.getNativewas not available on Android and iOS.