ktor-client-webrtc module common API + tests #4902
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughA new multiplatform WebRTC client API is introduced for Kotlin under the Changes
Suggested reviewers
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (8)
ktor-server/ktor-server-core/api/ktor-server-core.api (1)
387-389: Interface declaration appears redundant.The
SerializableConfigValueinterface declaresgetAs(TypeInfo): Objectwhich is already declared in its parent interfaceApplicationConfigValue(line 299). This creates potential confusion about the interface's purpose.Consider either:
- Removing the redundant
getAsdeclaration if the interface serves only as a marker- Adding additional methods that differentiate this interface from
ApplicationConfigValue- Adding documentation to clarify the intended purpose
public abstract interface class io/ktor/server/config/SerializableConfigValue : io/ktor/server/config/ApplicationConfigValue { - public abstract fun getAs (Lio/ktor/util/reflect/TypeInfo;)Ljava/lang/Object; + // Remove redundant declaration or add specific serialization-related methods }ktor-client/ktor-client-webrtc/common/src/io/ktor/client/webrtc/WebRTCClient.kt (2)
10-10: Documentation could be more precise.The comment mentions "managing media tracks" but the
WebRTCClientclass doesn't directly manage tracks - it delegates all operations to the engine. Consider updating to reflect the delegation pattern.-A multiplatform asynchronous WebRTC client for establishing peer-to-peer connections, managing media tracks. +A multiplatform asynchronous WebRTC client that provides access to WebRTC engine functionality for establishing peer-to-peer connections and managing media tracks.
21-30: Fix variable name inconsistency in documentation example.The example creates a client named
rtcClientbut the closing example referencesclient.* val rtcClient = WebRTCClient(AndroidWebRTC) { * iceServers = listOf(WebRTC.IceServer(urls = "stun:stun.l.google.com:19302")) * statsRefreshRate = 10_000 * } * ``` * * # Closing the client * ```kotlin -* client.close() +* rtcClient.close() * ```ktor-client/ktor-client-webrtc/common/src/io/ktor/client/webrtc/WebRTCPeerConnection.kt (1)
170-172: Consider thread safety for callback assignment.Direct assignment of the callback might have thread safety issues if called from multiple coroutines. Consider using atomic operations or synchronization.
+import kotlinx.atomicfu.atomic +import kotlinx.atomicfu.AtomicRef -protected var negotiationNeededCallback: () -> Unit = {} +private val _negotiationNeededCallback: AtomicRef<() -> Unit> = atomic {} +protected var negotiationNeededCallback: () -> Unit + get() = _negotiationNeededCallback.value + set(value) { _negotiationNeededCallback.value = value }ktor-client/ktor-client-webrtc/common/src/io/ktor/client/webrtc/WebRTCMedia.kt (1)
147-156: Consider improving exception formatting consistency.The exception classes are well-designed, but there's inconsistent spacing in the
PermissionExceptionclass declaration.Apply this diff for consistent formatting:
- public class PermissionException(mediaType: String?) : - RuntimeException("You should grant $mediaType permission for this operation to work.") + public class PermissionException(mediaType: String?) : RuntimeException( + "You should grant $mediaType permission for this operation to work." + )ktor-client/ktor-client-webrtc/common/src/io/ktor/client/webrtc/WebRTC.kt (1)
220-228: Consider improving type safety for RTP parameters.The
RtpParametersinterface usesAnytype forcodecs,rtcp, andencodings. While this provides flexibility for platform-specific implementations, it reduces type safety.Consider defining more specific interfaces or using generic type parameters to improve type safety while maintaining flexibility:
public interface RtpCodec { val mimeType: String val clockRate: Int // other common codec properties } public interface RtpParameters { public val transactionId: String public val codecs: Iterable<RtpCodec> public val rtcp: RtcpParameters // ... }This would provide better compile-time type checking while still allowing platform-specific extensions.
ktor-client/ktor-client-webrtc/common/test/io/ktor/client/webrtc/WebRTCEngineTest.kt (1)
348-407: Thorough remote track management test!This test excellently covers:
- Remote track reception for both audio and video
- Track removal and renegotiation
- Replay behavior for late subscribers
- Proper cleanup of coroutine jobs
Consider extracting the track verification logic into a helper function to reduce duplication:
private fun verifyTracks(tracks: Array<Operation<WebRTCMedia.Track>>, expectedAudio: Int, expectedVideo: Int) { assertEquals(expectedAudio, tracks.filter { it.item.kind === WebRTCMedia.TrackType.AUDIO }.size) assertEquals(expectedVideo, tracks.filter { it.item.kind === WebRTCMedia.TrackType.VIDEO }.size) }ktor-client/ktor-client-webrtc/api/ktor-client-webrtc.klib.api (1)
586-586: Remove incomplete line at end of file.Line 586 appears to be incomplete with just the line number and no content. This should be removed to clean up the file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
ktor-client/ktor-client-webrtc/api/ktor-client-webrtc.klib.api(1 hunks)ktor-client/ktor-client-webrtc/build.gradle.kts(1 hunks)ktor-client/ktor-client-webrtc/common/src/io/ktor/client/webrtc/WebRTC.kt(1 hunks)ktor-client/ktor-client-webrtc/common/src/io/ktor/client/webrtc/WebRTCClient.kt(1 hunks)ktor-client/ktor-client-webrtc/common/src/io/ktor/client/webrtc/WebRTCEngine.kt(1 hunks)ktor-client/ktor-client-webrtc/common/src/io/ktor/client/webrtc/WebRTCMedia.kt(1 hunks)ktor-client/ktor-client-webrtc/common/src/io/ktor/client/webrtc/WebRTCPeerConnection.kt(1 hunks)ktor-client/ktor-client-webrtc/common/test/io/ktor/client/webrtc/WebRTCEngineTest.kt(1 hunks)ktor-client/ktor-client-webrtc/gradle.properties(1 hunks)ktor-client/ktor-client-webrtc/js/src/io/ktor/client/webrtc/WebRTCEngine.js.kt(1 hunks)ktor-client/ktor-client-webrtc/js/test/io/ktor/client/webrtc/WebRTCEngineTest.js.kt(1 hunks)ktor-server/ktor-server-core/api/ktor-server-core.api(1 hunks)settings.gradle.kts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
ktor-client/ktor-client-webrtc/common/src/io/ktor/client/webrtc/WebRTCEngine.kt (1)
ktor-client/ktor-client-webrtc/js/src/io/ktor/client/webrtc/WebRTCEngine.js.kt (1)
ioDispatcher(10-10)
🔇 Additional comments (20)
ktor-client/ktor-client-webrtc/gradle.properties (1)
4-7:❌ Incorrect review comment
Verify platform target configuration for WebRTC module.
Setting all platform targets to
falseseems counterintuitive for a WebRTC client module, especiallytarget.js.nodeJs=false. WebRTC is primarily a web technology that should support JavaScript/browser platforms. This configuration might prevent the module from being built for the intended platforms.
🏁 Script executed:
#!/bin/bash # Verify if other Ktor client modules have similar configurations fd -t f "gradle.properties" ktor-client/ --exec grep -H "target\."Length of output: 485
Gradle properties correctly constrain WebRTC module to browser JS
The
ktor-client-webrtc/gradle.propertiesfile intentionally disablestarget.js.nodeJs,target.wasmJs,target.jvm, andtarget.posixwhile leaving thejs.browsertarget enabled by default. This ensures the module only compiles to browser JavaScript, which is exactly what a WebRTC client needs. No changes are necessary unless you specifically want to add Node.js, WASM, JVM, or POSIX support (in which case you would re-enable those targets).Likely an incorrect or invalid review comment.
settings.gradle.kts (1)
129-129: LGTM! Proper integration of the new WebRTC module.The addition correctly follows the established pattern for including client modules and is positioned appropriately within the client projects section.
ktor-client/ktor-client-webrtc/js/src/io/ktor/client/webrtc/WebRTCEngine.js.kt (1)
10-10:✅ Verification successful
Consider dispatcher choice for WebRTC I/O operations.
Using
Dispatchers.Defaultfor WebRTC I/O operations might not be optimal. WebRTC typically involves network I/O, media processing, and async operations that would benefit fromDispatchers.IO. However, on JavaScript platforms, the distinction may be less significant due to the single-threaded nature.
🏁 Script executed:
#!/bin/bash # Check how other Ktor client engines handle dispatchers on JS platform rg -A 3 -B 3 "Dispatchers\." ktor-client/ --type kotlin | grep -A 5 -B 5 "\.js\."Length of output: 2623
No changes needed for JS I/O dispatcher
Using
Dispatchers.DefaultinWebRTCEngine.js.ktis consistent with other Ktor JS engines (e.g.HttpClientEngineBase.js.kt), and Kotlin/JS only providesDefaultandUnconfineddispatchers—there is noDispatchers.IOon JS. KeepingDispatchers.Defaulthere is correct.ktor-client/ktor-client-webrtc/build.gradle.kts (2)
8-8:✅ Verification successful
Verify need for kotlinx-serialization plugin.
WebRTC protocols typically handle binary data streams rather than serialized objects. Consider whether the
kotlinx-serializationplugin is necessary for this module.
🏁 Script executed:
#!/bin/bash # Check if serialization is used in the WebRTC module source files rg -r '$1' '@Serializable|serialization|kotlinx\.serialization' ktor-client/ktor-client-webrtc/Length of output: 4345
kotlinx-serialization plugin is required
The WebRTC module defines @serializable types (e.g. SessionDescription, IceCandidate) and relies on generated serializers across multiplatform targets:
- common/src/io/ktor/client/webrtc/WebRTC.kt: imports and uses @serializable annotations
- Generated serializers in the
.klib.apifiles (e.g. WebRTC.IceCandidate.$serializer)No changes needed.
16-19:❓ Verification inconclusive
Consider adding ktor-client-core dependency.
Most Ktor client modules depend on
ktor-client-core. Verify if this WebRTC client module needs core client functionality.
🏁 Script executed:
#!/bin/bash # Check if other client modules include ktor-client-core dependency fd -t f "build.gradle.kts" ktor-client/ --exec grep -l "ktor-client-core"Length of output: 71
To verify whether a
ktor-client-coresubmodule exists and how it’s used elsewhere, let’s list the client modules and search their build files for any references:#!/bin/bash # 1. List direct submodules under ktor-client echo "=== Submodules in ktor-client/ ===" fd -t d --max-depth 1 '*' ktor-client # 2. Look for project dependency on ":ktor-client-core" echo -e "\n=== project(\":ktor-client-core\") references ===" rg -n 'project\("(:ktor-client-core)"\)' -g 'ktor-client/**/build.gradle.kts' || echo "None found" # 3. Look for external Maven coordinate usage of ktor-client-core echo -e "\n=== io.ktor.client:ktor-client-core references ===" rg -n 'io\.ktor\.client:ktor-client-core' -g 'ktor-client/**/build.gradle.kts' || echo "None found"
Verify ktor-client-core dependency
I didn’t find any other
ktor-clientmodules pulling inktor-client-core. Please confirm whether your WebRTC client needs core client APIs (e.g.HttpClient, call handling, engines). If it does, add it alongside your existing dependencies:commonMain.dependencies { api(project(":ktor-io")) api(project(":ktor-utils")) api("io.ktor:ktor-client-core:$ktor_version") }Files to check:
ktor-client/ktor-client-webrtc/build.gradle.kts(lines 16–19)ktor-client/ktor-client-webrtc/common/src/io/ktor/client/webrtc/WebRTCClient.kt (1)
12-37: Clean implementation using delegation pattern.The
WebRTCClientclass properly delegates to the engine, and the factory pattern provides a clean API for client instantiation. Good use of the@KtorDslannotation for configuration blocks.ktor-client/ktor-client-webrtc/common/src/io/ktor/client/webrtc/WebRTCEngine.kt (1)
73-73:✅ Verification successful
Verify platform-specific dispatcher implementations.
The expected function pattern is correctly used for multiplatform. The JS implementation returns
Dispatchers.Defaultwhich is appropriate for JavaScript's single-threaded nature.
🏁 Script executed:
#!/bin/bash # Description: Check if other platform implementations of ioDispatcher() exist # Search for actual implementations of ioDispatcher ast-grep --pattern 'internal actual fun ioDispatcher(): CoroutineDispatcher'Length of output: 744
✅ Confirmed platform-specific IO dispatchers
All
actualimplementations ofioDispatcher()use the appropriate dispatcher for their platform:
- JVM & POSIX:
Dispatchers.IO- JavaScript & WASM:
Dispatchers.DefaultNo changes required.
ktor-client/ktor-client-webrtc/common/src/io/ktor/client/webrtc/WebRTCPeerConnection.kt (1)
38-91: Excellent reactive API design using Kotlin flows.The use of
StateFlowandSharedFlowfor WebRTC state management provides a clean, reactive API that aligns well with Kotlin coroutines. The replay parameters for flows are configurable, which is a thoughtful design choice.ktor-client/ktor-client-webrtc/common/src/io/ktor/client/webrtc/WebRTCMedia.kt (4)
7-13: Well-structured media abstraction design!The
WebRTCMediaobject provides a clean encapsulation of media capture abstractions with appropriate documentation and references.
14-34: LGTM!The
VideoTrackConstraintsdata class is well-designed with appropriate nullable properties and clear documentation.
61-84: Excellent interface design with proper resource management!The
Trackinterface is well-designed withAutoCloseablefor proper resource management and a flexiblegetNative()method for platform-specific implementations.
86-145: LGTM!The track type enums and specialized track interfaces are well-structured and properly documented.
ktor-client/ktor-client-webrtc/common/src/io/ktor/client/webrtc/WebRTC.kt (2)
15-88: Excellent WebRTC protocol abstractions!The
WebRTCobject provides comprehensive and well-documented protocol entities with appropriate MDN references. TheisSuccessful()helper method onIceConnectionStateis a nice touch for improving API usability.
144-176: LGTM!The serializable data classes are well-designed for WebRTC signaling with appropriate use of Kotlin serialization.
ktor-client/ktor-client-webrtc/common/test/io/ktor/client/webrtc/WebRTCEngineTest.kt (3)
21-64: Excellent test setup with proper abstractions!The test framework is well-designed with:
- Platform-specific abstractions using
expectfunctions- Proper permission handling
- Clean lifecycle management with
@BeforeTestand@AfterTest- Flexible test execution with real-time support
219-292: Comprehensive connection establishment test!This test thoroughly validates the WebRTC connection lifecycle including:
- State transitions for ICE, signaling, and connection states
- ICE candidate exchange
- Negotiation callbacks
- ICE restart functionality
The use of channels and
selectfor monitoring multiple state flows is an excellent pattern for async testing.
294-318: Good error handling test coverage!The tests properly validate that the API throws appropriate exceptions for invalid inputs and ensures state remains unchanged after failed operations.
ktor-client/ktor-client-webrtc/api/ktor-client-webrtc.klib.api (3)
18-25: Well-designed coroutine-friendly interface.The
WebRTCEngineinterface follows Kotlin best practices by:
- Extending
CoroutineScopefor structured concurrency- Implementing
Closeablefor proper resource management- Using suspend functions for async operations
- Providing access to the coroutine dispatcher
This design enables clean async/await patterns and proper lifecycle management.
45-72: Excellent reactive state management implementation.The peer connection class demonstrates best practices for Kotlin Flow usage:
- Internal
MutableStateFlowproperties for state management- External
StateFlowproperties for read-only observationSharedFlowfor event streams (ICE candidates, remote tracks)- Proper encapsulation preventing external state mutation
This reactive design enables clean integration with Kotlin coroutines and UI frameworks.
264-267: Well-implemented serialization support.The selective application of kotlinx.serialization to data transfer objects (
SessionDescription,IceCandidate,SessionDescriptionType) is appropriate. These are the entities most likely to be serialized for signaling server communication, while internal state objects correctly omit serialization support.Also applies to: 340-351, 415-428
ktor-client/ktor-client-webrtc/js/test/io/ktor/client/webrtc/WebRTCEngineTest.js.kt
Outdated
Show resolved
Hide resolved
ktor-client/ktor-client-webrtc/common/src/io/ktor/client/webrtc/WebRTCEngine.kt
Outdated
Show resolved
Hide resolved
ktor-client/ktor-client-webrtc/common/src/io/ktor/client/webrtc/WebRTCEngine.kt
Outdated
Show resolved
Hide resolved
ktor-client/ktor-client-webrtc/common/src/io/ktor/client/webrtc/WebRTCPeerConnection.kt
Outdated
Show resolved
Hide resolved
ktor-client/ktor-client-webrtc/common/src/io/ktor/client/webrtc/WebRTCMedia.kt
Outdated
Show resolved
Hide resolved
ktor-client/ktor-client-webrtc/common/src/io/ktor/client/webrtc/WebRTC.kt
Outdated
Show resolved
Hide resolved
ktor-client/ktor-client-webrtc/common/src/io/ktor/client/webrtc/WebRTC.kt
Outdated
Show resolved
Hide resolved
ktor-client/ktor-client-webrtc/common/src/io/ktor/client/webrtc/WebRTCEngine.kt
Outdated
Show resolved
Hide resolved
ktor-client/ktor-client-webrtc/common/src/io/ktor/client/webrtc/WebRTCEngine.kt
Outdated
Show resolved
Hide resolved
ktor-client/ktor-client-webrtc/common/src/io/ktor/client/webrtc/WebRTCMedia.kt
Outdated
Show resolved
Hide resolved
ktor-client/ktor-client-webrtc/common/src/io/ktor/client/webrtc/WebRTCPeerConnection.kt
Outdated
Show resolved
Hide resolved
ktor-client/ktor-client-webrtc/common/src/io/ktor/client/webrtc/WebRTC.kt
Outdated
Show resolved
Hide resolved
ktor-client/ktor-client-webrtc/common/src/io/ktor/client/webrtc/WebRTCEngine.kt
Outdated
Show resolved
Hide resolved
ktor-client/ktor-client-webrtc/common/src/io/ktor/client/webrtc/WebRTCEngine.kt
Outdated
Show resolved
Hide resolved
ktor-client/ktor-client-webrtc/common/src/io/ktor/client/webrtc/WebRTCPeerConnection.kt
Outdated
Show resolved
Hide resolved
ktor-client/ktor-client-webrtc/common/src/io/ktor/client/webrtc/WebRTCPeerConnection.kt
Outdated
Show resolved
Hide resolved
ktor-client/ktor-client-webrtc/common/src/io/ktor/client/webrtc/WebRTCPeerConnection.kt
Outdated
Show resolved
Hide resolved
Subsystem
Client
Motivation
KTOR-7958 WebRTC Client.
Solution
The solution contains a common KMP interface that allows
The API is "up to Ktor standards" and utilises flows and coroutines rather than callbacks. It is unified across those platforms (configs are custom for all engines), but still allows access to native objects if some custom logic is needed. There are integration tests. A mock JS target has been added so that Gradle can include the module in the build. Code documentation contains links to MDN.
TODO:
Add datachannels to the API.
This is a part of a previously created pull request.