WebRTC Client #5069
Merged
WebRTC Client #5069
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…lient-webrtc-android
…lient-webrtc-android
…and audio tracks.
* WebRtc Client. Introduce data channels to the common API. * WebRtc Client. Add JS implementation to test datachannels API. * WebRtc Client. Remove `ReceiveChannel` from WebRtcDataChannel. Accept custom `coroutineContext` in the rtc connection. * WebRtc Client. Add WasmJS implementation for datachannels. * WebRtc Client. Add Android implementation for datachannels. * WebRtc Client. Split `DataChannel` interface and base implementation.
* WebRtc Client. Improve tests by using `AutoClosable.use` where possible and creating extra util functions. * WebRtc Client. Use `kotlin-wrappers` instead of manually generated browser declarations.
* WebRtc Client. Introduce Native engine based on WebRTC.rs and Gobley. Works only for jvm and separated into different submodule for now. * WebRTC.rs Client. Remove track by reference, not only sender. * WebRtc Client. Fix datachannel closing and `bufferedamountlow` event. * WebRTC.rs Client. Fix issues found on review. * WebRTC.rs Client. Rust `MediaStreamTrack` rethinked and tested. Add more targets. * WebRTC.rs Client. Remove android jvm/native targets. * WebRTC.rs Client. Disable `ktor-client-webrtc-rs` module by default. * WebRTC.rs Client. Improve `ktor-client-webrtc-rs` configuration. * WebRTC.rs Client. Improve flaky tests. * WebRTC.rs Client. Override konan properties on Linux. * WebRTC.rs Client. Update contribution guide. * WebRTC Client. Fix structured concurrency. * WebRTC.rs Client. Make media tests deterministic. * WebRTC.rs Client. Disable macOS X64 target.
Contributor
WalkthroughAdds a new multiplatform Ktor WebRTC client (common, Android, JS/WASM, and optional Rust via uniFFI), new WebRTC engine abstractions, platform implementations, extensive tests, build logic and Gradle configuration updates, and introduces a multiplatform ioDispatcher expect/actual. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~180 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
Contributor
|
Damn, the PR is so big you killed the rabbit 💀 I'll check it out now and peruse. Could you update the branch in the meantime? Looks like the allocation tests are failing because it's pre-jetty upgrade. Correction: I forgot you're off today, I'll update the branch 👌 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Comment on lines
+37
to
+39
| // should be added automatically, but fails with the new Android KMP plugin | ||
| dependsOn(commonMain.get()) | ||
| } |
Contributor
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the automatic dependency resolution
The comment indicates that commonMain dependency should be added automatically but fails with the new Android KMP plugin. This workaround might become unnecessary in future plugin versions.
🌐 Web query:
Does the Android KMP plugin version 8.9.2 automatically add commonMain dependencies for androidMain source set?
💡 Result:
Yes. Dependencies declared in commonMain are propagated to androidMain (KMP source-set inheritance), and the Android‑KMP plugin does not change that behavior — add commonMain dependencies once and the Android target will receive the platform parts automatically. [1][2]
Sources:
- Kotlin Multiplatform dependency propagation. [1]
- Android-KMP plugin (com.android.kotlin.multiplatform.library) docs. [2]
References:
[1] Kotlin Multiplatform — dependency propagation.
[2] Migrate to the Android‑KMP plugin — Android Developers.
Remove redundant dependsOn(commonMain.get())
commonMain dependencies are automatically propagated to androidMain by Android KMP plugin v8.9.2, so this manual dependency can be removed.
Location: build-logic/src/main/kotlin/ktorbuild/targets/AndroidConfig.kt (lines 37–39)
🤖 Prompt for AI Agents
In build-logic/src/main/kotlin/ktorbuild/targets/AndroidConfig.kt around lines
37 to 39, remove the redundant dependsOn(commonMain.get()) call (and its
explanatory comment if present) because the Android KMP plugin v8.9.2
automatically propagates commonMain to androidMain; simply delete that line so
the manual dependency is not applied.
ktor-client/ktor-client-core/common/src/io/ktor/client/engine/HttpClientEngineBase.kt
Show resolved
Hide resolved
Comment on lines
+59
to
+63
| override suspend fun send(text: String) { | ||
| assertOpen() | ||
| val buffer = DataChannel.Buffer(Charsets.UTF_8.encode(text), false) | ||
| nativeChannel.send(buffer) | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Check DataChannel.send return value
send(...) returns false on failure (e.g., closed channel). Currently ignored; failures will be silent.
- override suspend fun send(text: String) {
+ override suspend fun send(text: String) {
assertOpen()
- val buffer = DataChannel.Buffer(Charsets.UTF_8.encode(text), false)
- nativeChannel.send(buffer)
+ val buffer = DataChannel.Buffer(ByteBuffer.wrap(text.toByteArray(Charsets.UTF_8)), false)
+ check(nativeChannel.send(buffer)) { "Data channel send(text) failed" }
}
@@
- override suspend fun send(bytes: ByteArray) {
+ override suspend fun send(bytes: ByteArray) {
assertOpen()
val buffer = DataChannel.Buffer(ByteBuffer.wrap(bytes), true)
- nativeChannel.send(buffer)
+ check(nativeChannel.send(buffer)) { "Data channel send(bytes) failed" }
}Also applies to: 65-69
🤖 Prompt for AI Agents
In
ktor-client/ktor-client-webrtc/android/src/io/ktor/client/webrtc/DataChannel.kt
around lines 59-63 (and similarly for 65-69), the boolean return value of
nativeChannel.send(...) is ignored so failures (e.g., closed channel) are
silent; change the code to check the return value and throw an exception when
send returns false (for example throw IllegalStateException("DataChannel.send
failed: channel closed") or a suitable IOException) to surface the failure to
callers; apply the same check-and-throw pattern to both the String and
ByteBuffer overloads.
Comment on lines
+71
to
+73
| override fun setBufferedAmountLowThreshold(threshold: Long) { | ||
| bufferedAmountLowThreshold = threshold | ||
| } |
Contributor
There was a problem hiding this comment.
BufferedAmountLow never fires without setting native threshold
You only cache the threshold in Kotlin; org.webrtc requires calling setBufferedAmountLowThreshold on the native channel for notifications to occur.
Apply:
override fun setBufferedAmountLowThreshold(threshold: Long) {
- bufferedAmountLowThreshold = threshold
+ bufferedAmountLowThreshold = threshold
+ nativeChannel.setBufferedAmountLowThreshold(threshold)
}📝 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.
Suggested change
| override fun setBufferedAmountLowThreshold(threshold: Long) { | |
| bufferedAmountLowThreshold = threshold | |
| } | |
| override fun setBufferedAmountLowThreshold(threshold: Long) { | |
| bufferedAmountLowThreshold = threshold | |
| nativeChannel.setBufferedAmountLowThreshold(threshold) | |
| } |
🤖 Prompt for AI Agents
In
ktor-client/ktor-client-webrtc/android/src/io/ktor/client/webrtc/DataChannel.kt
around lines 71 to 73, the setter only caches bufferedAmountLowThreshold in
Kotlin but never calls the native WebRTC channel API, so the native
notifications never fire; update the implementation to both store the threshold
locally and call the underlying
org.webrtc.DataChannel.setBufferedAmountLowThreshold(threshold) (guarded by a
null check or synchronized access if the native channel may be unavailable) so
the native threshold is actually applied.
Comment on lines
+80
to
+84
| val frame = mutableListOf<Byte>() | ||
| parts.forEach { | ||
| frame.addAll(it.toList()) | ||
| } | ||
| frame.toByteArray() |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inefficient array concatenation
Using mutableListOf and addAll with toList() creates unnecessary intermediate collections. This impacts memory and performance.
Apply this more efficient approach:
- val frame = mutableListOf<Byte>()
- parts.forEach {
- frame.addAll(it.toList())
- }
- frame.toByteArray()
+ parts.flatMap { it.toList() }.toByteArray()Or even better, using ByteArrayOutputStream:
- val frame = mutableListOf<Byte>()
- parts.forEach {
- frame.addAll(it.toList())
- }
- frame.toByteArray()
+ ByteArrayOutputStream().apply {
+ parts.forEach { write(it) }
+ }.toByteArray()📝 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.
Suggested change
| val frame = mutableListOf<Byte>() | |
| parts.forEach { | |
| frame.addAll(it.toList()) | |
| } | |
| frame.toByteArray() | |
| parts.flatMap { it.toList() }.toByteArray() |
Suggested change
| val frame = mutableListOf<Byte>() | |
| parts.forEach { | |
| frame.addAll(it.toList()) | |
| } | |
| frame.toByteArray() | |
| ByteArrayOutputStream().apply { | |
| parts.forEach { write(it) } | |
| }.toByteArray() |
🤖 Prompt for AI Agents
In
ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/common/test/io/ktor/client/webrtc/rs/utils/MockMediaDevices.kt
around lines 80 to 84, the code builds a frame by creating a mutableList and
calling addAll with toList(), producing unnecessary intermediate collections;
replace this with a direct byte-array concat approach: precompute total length,
allocate a ByteArray of that size and copy each part into it using
System.arraycopy (or use a ByteArrayOutputStream and write each part) and return
the resulting ByteArray to avoid extra allocations and improve performance.
ktor-client/ktor-client-webrtc/src/androidMain/AndroidManifest.xml
Outdated
Show resolved
Hide resolved
Comment on lines
+15
to
+16
| <uses-feature android:name="android.hardware.camera" /> | ||
| </manifest> |
Contributor
There was a problem hiding this comment.
Library manifest must not require a camera by default.
<uses-feature android.hardware.camera> implies required="true" and will filter out devices (including front‑camera‑only). In a library this can inadvertently exclude consumer apps from Play Store devices and is hard to override.
Apply one of the following (preferred: drop the feature entirely from the library):
- <uses-feature android:name="android.hardware.camera" />
+ <!-- Libraries should not constrain device features; let the app declare if needed -->
+ <!-- (Option A: remove entirely) -->
+ <!-- (Option B if you must keep it): -->
+ <!-- <uses-feature android:name="android.hardware.camera.any" android:required="false" /> -->📝 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.
Suggested change
| <uses-feature android:name="android.hardware.camera" /> | |
| </manifest> | |
| <!-- Libraries should not constrain device features; let the app declare if needed --> | |
| <!-- (Option A: remove entirely) --> | |
| <!-- (Option B if you must keep it): --> | |
| <!-- <uses-feature android:name="android.hardware.camera.any" android:required="false" /> --> | |
| </manifest> |
🤖 Prompt for AI Agents
In ktor-client/ktor-client-webrtc/src/androidMain/AndroidManifest.xml around
lines 15-16, the manifest declares <uses-feature
android:name="android.hardware.camera"> which implies required="true" and will
exclude devices (including front-camera-only) when this is included in a
library; remove this element from the library manifest entirely (preferred) or,
if a feature hint is needed, replace it with a non-required declaration
(android:required="false") or move any required camera declaration into the
consuming app's manifest so the library does not restrict device compatibility.
Comment on lines
+1
to
+9
| /* | ||
| * Copyright 2014-2025 JetBrains s.r.o and contributors. Use of this source code is governed by the Apache 2.0 license. | ||
| */ | ||
|
|
||
| package io.ktor.utils.io | ||
|
|
||
| import kotlinx.coroutines.CoroutineDispatcher | ||
|
|
||
| public expect fun ioDispatcher(): CoroutineDispatcher |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add KDoc for new public API (required by guidelines).
Document behavior per platform on the expect declaration.
package io.ktor.utils.io
import kotlinx.coroutines.CoroutineDispatcher
-public expect fun ioDispatcher(): CoroutineDispatcher
+/**
+ * Returns a platform-appropriate dispatcher for IO-bound work.
+ *
+ * Platforms:
+ * - JVM/Native: Dispatchers.IO
+ * - JS/WASM: Dispatchers.Default
+ */
+public expect fun ioDispatcher(): CoroutineDispatcher📝 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.
Suggested change
| /* | |
| * Copyright 2014-2025 JetBrains s.r.o and contributors. Use of this source code is governed by the Apache 2.0 license. | |
| */ | |
| package io.ktor.utils.io | |
| import kotlinx.coroutines.CoroutineDispatcher | |
| public expect fun ioDispatcher(): CoroutineDispatcher | |
| /* | |
| * Copyright 2014-2025 JetBrains s.r.o and contributors. Use of this source code is governed by the Apache 2.0 license. | |
| */ | |
| package io.ktor.utils.io | |
| import kotlinx.coroutines.CoroutineDispatcher | |
| /** | |
| * Returns a platform-appropriate dispatcher for IO-bound work. | |
| * | |
| * Platforms: | |
| * - JVM/Native: Dispatchers.IO | |
| * - JS/WASM: Dispatchers.Default | |
| */ | |
| public expect fun ioDispatcher(): CoroutineDispatcher |
🤖 Prompt for AI Agents
In ktor-io/common/src/io/ktor/utils/io/IODispatcher.kt around lines 1 to 9, the
public expect function ioDispatcher() is missing KDoc required for new public
APIs; add a concise KDoc comment above the expect declaration that documents its
purpose and behavior and specifies platform-specific semantics (e.g., which
dispatcher is returned on JVM, JS, and native, thread-confinement expectations,
and any lifecycle or cancellation guarantees), include usage notes or examples
if relevant, and keep the comment consistent with project KDoc style and
visibility requirements.
Comment on lines
+130
to
+139
| +"ktor-client-webrtc" including { | ||
| // Include `ktor-client-webrtc-rs` if rust compilation is enabled in `gradle.properties` for local builds | ||
| // or if the `RUST_COMPILATION` environment variable is set to `true` for the CI. | ||
| val compileRust = providers.environmentVariable("KTOR_RUST_COMPILATION") | ||
| .orElse(providers.gradleProperty("ktorbuild.rustCompilation")) | ||
| .orNull.toBoolean() | ||
| if (compileRust) { | ||
| +"ktor-client-webrtc-rs" // requires `cargo` to be installed | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Fix env var name mismatch for Rust gating.
Comment mentions RUST_COMPILATION but code reads KTOR_RUST_COMPILATION. This can break CI toggling.
Apply this diff to support both names and fix the comment:
- // or if the `RUST_COMPILATION` environment variable is set to `true` for the CI.
- val compileRust = providers.environmentVariable("KTOR_RUST_COMPILATION")
- .orElse(providers.gradleProperty("ktorbuild.rustCompilation"))
+ // or if the `KTOR_RUST_COMPILATION` (or legacy `RUST_COMPILATION`) env var is set to `true` on CI.
+ val compileRust = providers.environmentVariable("KTOR_RUST_COMPILATION")
+ .orElse(providers.environmentVariable("RUST_COMPILATION"))
+ .orElse(providers.gradleProperty("ktorbuild.rustCompilation"))
.orNull.toBoolean()📝 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.
Suggested change
| +"ktor-client-webrtc" including { | |
| // Include `ktor-client-webrtc-rs` if rust compilation is enabled in `gradle.properties` for local builds | |
| // or if the `RUST_COMPILATION` environment variable is set to `true` for the CI. | |
| val compileRust = providers.environmentVariable("KTOR_RUST_COMPILATION") | |
| .orElse(providers.gradleProperty("ktorbuild.rustCompilation")) | |
| .orNull.toBoolean() | |
| if (compileRust) { | |
| +"ktor-client-webrtc-rs" // requires `cargo` to be installed | |
| } | |
| } | |
| "ktor-client-webrtc" including { | |
| // Include `ktor-client-webrtc-rs` if rust compilation is enabled in `gradle.properties` for local builds | |
| // or if the `KTOR_RUST_COMPILATION` (or legacy `RUST_COMPILATION`) env var is set to `true` on CI. | |
| val compileRust = providers.environmentVariable("KTOR_RUST_COMPILATION") | |
| .orElse(providers.environmentVariable("RUST_COMPILATION")) | |
| .orElse(providers.gradleProperty("ktorbuild.rustCompilation")) | |
| .orNull.toBoolean() | |
| if (compileRust) { | |
| "ktor-client-webrtc-rs" // requires `cargo` to be installed | |
| } | |
| } |
🤖 Prompt for AI Agents
In settings.gradle.kts around lines 130 to 139, the code checks
providers.environmentVariable("KTOR_RUST_COMPILATION") while the comment
references RUST_COMPILATION, causing a mismatch; update the comment to reflect
the actual env var(s) used and change the compileRust detection to accept both
env var names (KTOR_RUST_COMPILATION and RUST_COMPILATION) and still fall back
to the gradle property, e.g., read the env var pair with orElse to prefer any
set value, then convert to boolean; ensure the comment explains that either env
var can be used (and keep note about cargo requirement) so CI/local gating works
as intended.
Comment on lines
+13
to
+24
| public class AndroidWebRtcEngineConfig : WebRtcConfig() { | ||
| /** | ||
| * Android application context needed to create media tracks. | ||
| * */ | ||
| public var context: Context? = null | ||
|
|
||
| /** | ||
| * In Android WebRtc implementation PeerConnectionFactory is coupled with the MediaTrackFactory, so if you provide a | ||
| * custom MediaTrackFactory, you should specify PeerConnectionFactory to initialize a PeerConnection. | ||
| * */ | ||
| public var rtcFactory: PeerConnectionFactory? = null | ||
| } |
Contributor
There was a problem hiding this comment.
Add KDoc documentation for public API classes
Public classes and their properties should be documented according to the coding guidelines.
+/**
+ * Configuration for the Android WebRTC engine implementation.
+ */
public class AndroidWebRtcEngineConfig : WebRtcConfig() {
/**
* Android application context needed to create media tracks.
- * */
+ */
public var context: Context? = null
/**
* In Android WebRtc implementation PeerConnectionFactory is coupled with the MediaTrackFactory, so if you provide a
* custom MediaTrackFactory, you should specify PeerConnectionFactory to initialize a PeerConnection.
- * */
+ */
public var rtcFactory: PeerConnectionFactory? = 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.
Suggested change
| public class AndroidWebRtcEngineConfig : WebRtcConfig() { | |
| /** | |
| * Android application context needed to create media tracks. | |
| * */ | |
| public var context: Context? = null | |
| /** | |
| * In Android WebRtc implementation PeerConnectionFactory is coupled with the MediaTrackFactory, so if you provide a | |
| * custom MediaTrackFactory, you should specify PeerConnectionFactory to initialize a PeerConnection. | |
| * */ | |
| public var rtcFactory: PeerConnectionFactory? = null | |
| } | |
| /** | |
| * Configuration for the Android WebRTC engine implementation. | |
| */ | |
| public class AndroidWebRtcEngineConfig : WebRtcConfig() { | |
| /** | |
| * Android application context needed to create media tracks. | |
| */ | |
| public var context: Context? = null | |
| /** | |
| * In Android WebRTC implementation PeerConnectionFactory is coupled with the MediaTrackFactory, so if you provide a | |
| * custom MediaTrackFactory, you should specify PeerConnectionFactory to initialize a PeerConnection. | |
| */ | |
| public var rtcFactory: PeerConnectionFactory? = null | |
| } |
🤖 Prompt for AI Agents
In ktor-client/ktor-client-webrtc/android/src/io/ktor/client/webrtc/Engine.kt
around lines 13 to 24, the public class AndroidWebRtcEngineConfig and its public
properties lack KDoc; add KDoc for the class and for each public property
(context and rtcFactory) following project guidelines: document the purpose of
the class, annotate each property with @property describing intent, expected
lifecycle/ownership, nullability, and when/why to provide rtcFactory (e.g., when
supplying a custom MediaTrackFactory), and include any threading or
initialization notes required for correct usage.
Comment on lines
+26
to
+31
| public class AndroidWebRtcEngine( | ||
| override val config: AndroidWebRtcEngineConfig, | ||
| private val mediaTrackFactory: MediaTrackFactory = config.mediaTrackFactory | ||
| ?: AndroidMediaDevices(config.context ?: error("Context is required for AndroidWebRtcEngine")) | ||
| ) : WebRtcEngineBase("android-webrtc", config), | ||
| MediaTrackFactory by mediaTrackFactory { |
Contributor
There was a problem hiding this comment.
Add comprehensive KDoc for AndroidWebRtcEngine class
The public class needs proper documentation including constructor parameters.
+/**
+ * Android implementation of the WebRTC engine.
+ *
+ * @param config Configuration for the Android WebRTC engine
+ * @param mediaTrackFactory Factory for creating media tracks. Defaults to AndroidMediaDevices if not provided.
+ * @throws IllegalStateException if context is not provided in config when using default media track factory
+ */
public class AndroidWebRtcEngine(
override val config: AndroidWebRtcEngineConfig,
private val mediaTrackFactory: MediaTrackFactory = config.mediaTrackFactory
?: AndroidMediaDevices(config.context ?: error("Context is required for AndroidWebRtcEngine"))
) : WebRtcEngineBase("android-webrtc", config),
MediaTrackFactory by mediaTrackFactory {📝 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.
Suggested change
| public class AndroidWebRtcEngine( | |
| override val config: AndroidWebRtcEngineConfig, | |
| private val mediaTrackFactory: MediaTrackFactory = config.mediaTrackFactory | |
| ?: AndroidMediaDevices(config.context ?: error("Context is required for AndroidWebRtcEngine")) | |
| ) : WebRtcEngineBase("android-webrtc", config), | |
| MediaTrackFactory by mediaTrackFactory { | |
| /** | |
| * Android implementation of the WebRTC engine. | |
| * | |
| * @param config Configuration for the Android WebRTC engine | |
| * @param mediaTrackFactory Factory for creating media tracks. Defaults to AndroidMediaDevices if not provided. | |
| * @throws IllegalStateException if context is not provided in config when using default media track factory | |
| */ | |
| public class AndroidWebRtcEngine( | |
| override val config: AndroidWebRtcEngineConfig, | |
| private val mediaTrackFactory: MediaTrackFactory = config.mediaTrackFactory | |
| ?: AndroidMediaDevices(config.context ?: error("Context is required for AndroidWebRtcEngine")) | |
| ) : WebRtcEngineBase("android-webrtc", config), | |
| MediaTrackFactory by mediaTrackFactory { |
🤖 Prompt for AI Agents
In ktor-client/ktor-client-webrtc/android/src/io/ktor/client/webrtc/Engine.kt
around lines 26 to 31, the public AndroidWebRtcEngine class lacks KDoc; add
comprehensive KDoc above the class describing the class purpose and each
constructor parameter (config: AndroidWebRtcEngineConfig) and explain the
mediaTrackFactory parameter and its default behavior (using
config.mediaTrackFactory or creating AndroidMediaDevices and requirement for
non-null context), mention that it implements WebRtcEngineBase with name
"android-webrtc" and delegates MediaTrackFactory, and include any thread-safety
or lifecycle notes relevant for consumers.
|
|
||
| private val localFactory: PeerConnectionFactory | ||
| get() { | ||
| return (config.rtcFactory ?: (mediaTrackFactory as AndroidMediaDevices).peerConnectionFactory) |
Contributor
There was a problem hiding this comment.
Unsafe cast to AndroidMediaDevices
The cast to AndroidMediaDevices is unsafe and will fail if a custom MediaTrackFactory is used without providing rtcFactory.
private val localFactory: PeerConnectionFactory
get() {
- return (config.rtcFactory ?: (mediaTrackFactory as AndroidMediaDevices).peerConnectionFactory)
- ?: error("Please specify custom rtcFactory for custom MediaTrackFactory")
+ return config.rtcFactory ?: when (mediaTrackFactory) {
+ is AndroidMediaDevices -> mediaTrackFactory.peerConnectionFactory
+ ?: error("AndroidMediaDevices peerConnectionFactory is not initialized")
+ else -> error("A PeerConnectionFactory must be provided via config.rtcFactory when using a custom MediaTrackFactory")
+ }
}📝 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.
Suggested change
| return (config.rtcFactory ?: (mediaTrackFactory as AndroidMediaDevices).peerConnectionFactory) | |
| private val localFactory: PeerConnectionFactory | |
| get() { | |
| return config.rtcFactory ?: when (mediaTrackFactory) { | |
| is AndroidMediaDevices -> mediaTrackFactory.peerConnectionFactory | |
| ?: error("AndroidMediaDevices peerConnectionFactory is not initialized") | |
| else -> error("A PeerConnectionFactory must be provided via config.rtcFactory when using a custom MediaTrackFactory") | |
| } | |
| } |
🤖 Prompt for AI Agents
In ktor-client/ktor-client-webrtc/android/src/io/ktor/client/webrtc/Engine.kt
around line 35, the code performs an unsafe cast to AndroidMediaDevices which
will crash when a custom MediaTrackFactory is supplied without an rtcFactory;
change the logic to first prefer config.rtcFactory if present, otherwise check
if mediaTrackFactory is an AndroidMediaDevices (use a type-safe check) and
return its peerConnectionFactory, and if neither is available throw a clear
IllegalStateException explaining that rtcFactory must be provided for
non-AndroidMediaDevices implementations so callers know how to fix it.
Comment on lines
+30
to
+33
| override fun close() { | ||
| onDispose?.invoke() | ||
| nativeTrack.dispose() | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make close() idempotent and resilient to callback failure.
Prevent double-dispose and ensure native dispose still happens if onDispose throws.
- override fun close() {
- onDispose?.invoke()
- nativeTrack.dispose()
- }
+ override fun close() {
+ if (closed) return
+ closed = true
+ runCatching { onDispose?.invoke() }
+ nativeTrack.dispose()
+ }Add this field inside AndroidMediaTrack class (outside the shown range):
private var closed = false🤖 Prompt for AI Agents
In
ktor-client/ktor-client-webrtc/android/src/io/ktor/client/webrtc/media/MediaTrack.kt
around lines 30 to 33: add a private var closed = false to the AndroidMediaTrack
class, then make close() idempotent by returning immediately if closed is true,
set closed = true at the start of close(), invoke onDispose inside a try/catch
to swallow or log any exception (so it doesn't prevent cleanup), and always call
nativeTrack.dispose() in a finally or after the catch to ensure native resources
are disposed even if onDispose throws.
Comment on lines
+44
to
+47
| public fun from(nativeTrack: MediaStreamTrack): AndroidMediaTrack = when (kindOf(nativeTrack)) { | ||
| WebRtcMedia.TrackType.AUDIO -> AndroidAudioTrack(nativeTrack) | ||
| WebRtcMedia.TrackType.VIDEO -> AndroidVideoTrack(nativeTrack) | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Allow from(...) to pass an onDispose and enforce typed construction.
Expose onDispose and return strongly typed wrappers to eliminate later casts.
- public fun from(nativeTrack: MediaStreamTrack): AndroidMediaTrack = when (kindOf(nativeTrack)) {
- WebRtcMedia.TrackType.AUDIO -> AndroidAudioTrack(nativeTrack)
- WebRtcMedia.TrackType.VIDEO -> AndroidVideoTrack(nativeTrack)
- }
+ public fun from(
+ nativeTrack: MediaStreamTrack,
+ onDispose: (() -> Unit)? = null
+ ): AndroidMediaTrack = when (kindOf(nativeTrack)) {
+ WebRtcMedia.TrackType.AUDIO -> AndroidAudioTrack(nativeTrack as AudioTrack, onDispose)
+ WebRtcMedia.TrackType.VIDEO -> AndroidVideoTrack(nativeTrack as VideoTrack, onDispose)
+ }📝 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.
Suggested change
| public fun from(nativeTrack: MediaStreamTrack): AndroidMediaTrack = when (kindOf(nativeTrack)) { | |
| WebRtcMedia.TrackType.AUDIO -> AndroidAudioTrack(nativeTrack) | |
| WebRtcMedia.TrackType.VIDEO -> AndroidVideoTrack(nativeTrack) | |
| } | |
| public fun from( | |
| nativeTrack: MediaStreamTrack, | |
| onDispose: (() -> Unit)? = null | |
| ): AndroidMediaTrack = when (kindOf(nativeTrack)) { | |
| WebRtcMedia.TrackType.AUDIO -> AndroidAudioTrack(nativeTrack as AudioTrack, onDispose) | |
| WebRtcMedia.TrackType.VIDEO -> AndroidVideoTrack(nativeTrack as VideoTrack, onDispose) | |
| } |
🤖 Prompt for AI Agents
In
ktor-client/ktor-client-webrtc/android/src/io/ktor/client/webrtc/media/MediaTrack.kt
around lines 44-47, update the factory to accept an onDispose callback and pass
it into the concrete wrapper constructors, and ensure the concrete wrapper types
require that callback so callers get correctly-typed instances without needing
casts: change the signature to include onDispose: () -> Unit, call
AndroidAudioTrack(nativeTrack, onDispose) for AUDIO and
AndroidVideoTrack(nativeTrack, onDispose) for VIDEO, and update those classes'
constructors to accept the onDispose parameter (making the factory produce
strongly-typed wrappers).
| IceServer(listOf(it.urls), it.username ?: "", it.credential ?: "") | ||
| }, | ||
| addDefaultTransceivers = true, | ||
| iceCandidatePoolSize = config.iceCandidatePoolSize.toUByte(), |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Coerce iceCandidatePoolSize into UByte range.
Avoid overflow exceptions for values > 255.
- iceCandidatePoolSize = config.iceCandidatePoolSize.toUByte(),
+ iceCandidatePoolSize = config.iceCandidatePoolSize
+ .coerceIn(0, UByte.MAX_VALUE.toInt())
+ .toUByte(),📝 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.
Suggested change
| iceCandidatePoolSize = config.iceCandidatePoolSize.toUByte(), | |
| iceCandidatePoolSize = config.iceCandidatePoolSize | |
| .coerceIn(0, UByte.MAX_VALUE.toInt()) | |
| .toUByte(), |
🤖 Prompt for AI Agents
In
ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/common/src/io/ktor/client/webrtc/rs/Engine.kt
around line 39, the value passed to iceCandidatePoolSize is directly converted
to UByte which can throw for values > 255; coerce the numeric value into the
0..255 UByte range before converting by first converting to an unsigned integer
type, clamping it to the 0–255 range using coerceIn (or min/max), and then
converting that clamped value to UByte so oversized inputs cannot overflow.
Comment on lines
+37
to
+45
| public fun setMediaHandler(handler: MediaHandler, readPacketsInBackground: Boolean) { | ||
| inner.setSink(inner.createSink(handler)) | ||
| if (readPacketsInBackground) { | ||
| require(coroutineScope != null) { | ||
| "Coroutine scope is required to read RTP for track $id" | ||
| } | ||
| readRtpJob = coroutineScope.launch { inner.readAll() } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Cancel any previous background read job before starting a new one.
Prevents multiple readAll loops and leaks.
public fun setMediaHandler(handler: MediaHandler, readPacketsInBackground: Boolean) {
inner.setSink(inner.createSink(handler))
if (readPacketsInBackground) {
require(coroutineScope != null) {
"Coroutine scope is required to read RTP for track $id"
}
- readRtpJob = coroutineScope.launch { inner.readAll() }
+ readRtpJob?.cancel()
+ readRtpJob = coroutineScope.launch { inner.readAll() }
}
}📝 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.
Suggested change
| public fun setMediaHandler(handler: MediaHandler, readPacketsInBackground: Boolean) { | |
| inner.setSink(inner.createSink(handler)) | |
| if (readPacketsInBackground) { | |
| require(coroutineScope != null) { | |
| "Coroutine scope is required to read RTP for track $id" | |
| } | |
| readRtpJob = coroutineScope.launch { inner.readAll() } | |
| } | |
| } | |
| public fun setMediaHandler(handler: MediaHandler, readPacketsInBackground: Boolean) { | |
| inner.setSink(inner.createSink(handler)) | |
| if (readPacketsInBackground) { | |
| require(coroutineScope != null) { | |
| "Coroutine scope is required to read RTP for track $id" | |
| } | |
| readRtpJob?.cancel() | |
| readRtpJob = coroutineScope.launch { inner.readAll() } | |
| } | |
| } |
🤖 Prompt for AI Agents
In
ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/common/src/io/ktor/client/webrtc/rs/MediaTrack.kt
around lines 37 to 45, the code starts a new background read job without
cancelling any existing one, which can create multiple concurrent readAll loops
and leak resources; before launching a new readRtpJob call cancel() on the
existing job (readRtpJob?.cancel()), then assign the newly launched job to
readRtpJob, and add an invokeOnCompletion handler to clear the reference
(readRtpJob = null) when it finishes so previous jobs are properly cancelled and
the field does not hold stale references.
Comment on lines
+47
to
+52
| override fun close() { | ||
| if (readRtpJob?.isActive == true) { | ||
| readRtpJob?.cancel() | ||
| } | ||
| inner.destroy() | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make close() idempotent and ensure cleanup runs once.
Repeated close() calls should be safe; guard and cancel the job.
- override fun close() {
- if (readRtpJob?.isActive == true) {
- readRtpJob?.cancel()
- }
- inner.destroy()
- }
+ override fun close() {
+ if (closed) return
+ closed = true
+ readRtpJob?.cancel()
+ inner.destroy()
+ }Add this field inside RustMediaTrack (outside the shown range):
private var closed = false🤖 Prompt for AI Agents
In
ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/common/src/io/ktor/client/webrtc/rs/MediaTrack.kt
around lines 47-52, close() is not idempotent and repeated calls can attempt
cleanup multiple times; add a private var closed = false to RustMediaTrack and
update close() to check this flag at the start and return immediately if already
closed, otherwise set closed = true, cancel readRtpJob if active, and call
inner.destroy() so cleanup runs exactly once and double-close is safe.
| val jobs = mutableListOf<Job>() | ||
| when { | ||
| realTime -> runTestWithRealTime { | ||
| withTimeout(10_0000) { |
Contributor
There was a problem hiding this comment.
Fix timeout value typo
The timeout value appears to have an extra zero (10_0000 instead of 10_000).
- withTimeout(10_0000) {
+ withTimeout(10_000) {📝 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.
Suggested change
| withTimeout(10_0000) { | |
| withTimeout(10_000) { |
🤖 Prompt for AI Agents
In
ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/common/test/io/ktor/client/webrtc/rs/utils/ConnectionUtils.kt
around line 48, the coroutine timeout literal has an extra zero (10_0000) —
change it to the intended 10_000 (ten seconds) to correct the timeout value and
ensure the withTimeout call uses the correct duration.
Comment on lines
+69
to
+95
| // Offer-Answer exchange. The first peer creates an offer, the second - an answer. | ||
| suspend fun negotiate( | ||
| pc1: WebRtcPeerConnection, | ||
| pc2: WebRtcPeerConnection | ||
| ) { | ||
| // --- PC1 (Offerer) --- | ||
| val offer = pc1.createOffer() | ||
| // Set a local description. This starts ICE gathering on pc1. | ||
| pc1.setLocalDescription(offer) | ||
|
|
||
| // Wait for PC1's ICE gathering to finish. This ensures all candidates | ||
| // are included in the offer that pc2 will receive. | ||
| pc1.awaitIceGatheringComplete() | ||
|
|
||
| // Now get the local description again, which now contains all ICE candidates. | ||
| pc2.setRemoteDescription(pc1.localDescription!!) | ||
|
|
||
| // --- PC2 (Answerer) --- | ||
| val answer = pc2.createAnswer() | ||
| // Set a local description. This starts ICE gathering on pc2. | ||
| pc2.setLocalDescription(answer) | ||
|
|
||
| pc2.awaitIceGatheringComplete() | ||
|
|
||
| // Get the final answer with all of pc2's candidates. | ||
| pc1.setRemoteDescription(pc2.localDescription!!) | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for negotiation failures
The negotiate function should handle potential exceptions during offer/answer exchange.
suspend fun negotiate(
pc1: WebRtcPeerConnection,
pc2: WebRtcPeerConnection
) {
- // --- PC1 (Offerer) ---
- val offer = pc1.createOffer()
- // Set a local description. This starts ICE gathering on pc1.
- pc1.setLocalDescription(offer)
-
- // Wait for PC1's ICE gathering to finish. This ensures all candidates
- // are included in the offer that pc2 will receive.
- pc1.awaitIceGatheringComplete()
-
- // Now get the local description again, which now contains all ICE candidates.
- pc2.setRemoteDescription(pc1.localDescription!!)
-
- // --- PC2 (Answerer) ---
- val answer = pc2.createAnswer()
- // Set a local description. This starts ICE gathering on pc2.
- pc2.setLocalDescription(answer)
-
- pc2.awaitIceGatheringComplete()
-
- // Get the final answer with all of pc2's candidates.
- pc1.setRemoteDescription(pc2.localDescription!!)
+ try {
+ // --- PC1 (Offerer) ---
+ val offer = pc1.createOffer()
+ // Set a local description. This starts ICE gathering on pc1.
+ pc1.setLocalDescription(offer)
+
+ // Wait for PC1's ICE gathering to finish. This ensures all candidates
+ // are included in the offer that pc2 will receive.
+ pc1.awaitIceGatheringComplete()
+
+ // Now get the local description again, which now contains all ICE candidates.
+ val localDesc = pc1.localDescription
+ ?: error("PC1 local description should be set after setLocalDescription")
+ pc2.setRemoteDescription(localDesc)
+
+ // --- PC2 (Answerer) ---
+ val answer = pc2.createAnswer()
+ // Set a local description. This starts ICE gathering on pc2.
+ pc2.setLocalDescription(answer)
+
+ pc2.awaitIceGatheringComplete()
+
+ // Get the final answer with all of pc2's candidates.
+ val localDesc2 = pc2.localDescription
+ ?: error("PC2 local description should be set after setLocalDescription")
+ pc1.setRemoteDescription(localDesc2)
+ } catch (e: Exception) {
+ throw IllegalStateException("Failed to negotiate connection between peers", e)
+ }
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/common/test/io/ktor/client/webrtc/rs/utils/ConnectionUtils.kt
around lines 69-95, wrap the offer/answer exchange in a try/catch that catches
Throwable, logs the failure with contextual details (which peer and which step
failed), performs any required cleanup (e.g. close or dispose pc1/pc2 if
available), and rethrow the exception (or throw a custom exception) so test
harness sees the failure; ensure awaits and setLocal/setRemote calls remain in
the try block and use finally for cleanup to avoid resource leaks.
Comment on lines
+33
to
+39
| // helper method to break a dependency cycle (PeerConnection -> PeerConnectionFactory -> Observer) | ||
| public fun initialize(block: (Observer) -> PeerConnection?): AndroidWebRtcPeerConnection { | ||
| peerConnection = block(createObserver()) ?: error( | ||
| "Failed to create peer connection. On the Android platform it is usually caused by invalid configuration. For instance, missing turn server username." | ||
| ) | ||
| return this | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mark initialize(...) as internal API
This is a wiring hook not intended for end users; annotate with @internalapi to discourage external use and align with guidelines.
Apply:
+import io.ktor.util.InternalAPI
@@
- public fun initialize(block: (Observer) -> PeerConnection?): AndroidWebRtcPeerConnection {
+ @InternalAPI
+ public fun initialize(block: (Observer) -> PeerConnection?): AndroidWebRtcPeerConnection {📝 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.
Suggested change
| // helper method to break a dependency cycle (PeerConnection -> PeerConnectionFactory -> Observer) | |
| public fun initialize(block: (Observer) -> PeerConnection?): AndroidWebRtcPeerConnection { | |
| peerConnection = block(createObserver()) ?: error( | |
| "Failed to create peer connection. On the Android platform it is usually caused by invalid configuration. For instance, missing turn server username." | |
| ) | |
| return this | |
| } | |
| diff --git a/ktor-client-webrtc/android/src/io/ktor/client/webrtc/PeerConnection.kt b/ktor-client-webrtc/android/src/io/ktor/client/webrtc/PeerConnection.kt | |
| index abcdef0..1234567 100644 | |
| ++ b/ktor-client-webrtc/android/src/io/ktor/client/webrtc/PeerConnection.kt | |
| @@ -1,5 +1,6 @@ | |
| package io.ktor.client.webrtc | |
| import io.ktor.util.InternalAPI | |
| /** | |
| * Android implementation of a WebRTC PeerConnection. | |
| */ | |
| @@ -30,7 +31,8 @@ public class AndroidWebRtcPeerConnection { | |
| // helper method to break a dependency cycle (PeerConnection -> PeerConnectionFactory -> Observer) | |
| @InternalAPI | |
| public fun initialize(block: (Observer) -> PeerConnection?): AndroidWebRtcPeerConnection { | |
| peerConnection = block(createObserver()) ?: error( | |
| "Failed to create peer connection. On the Android platform it is usually caused by invalid configuration. For instance, missing turn server username." | |
| ) |
🤖 Prompt for AI Agents
In
ktor-client/ktor-client-webrtc/android/src/io/ktor/client/webrtc/PeerConnection.kt
around lines 33 to 39, the initialize(block: (Observer) -> PeerConnection?)
method is a wiring hook not meant for public use; mark it as internal API by
annotating the function with @InternalAPI and add the corresponding import
(io.ktor.util.InternalAPI) at the top of the file so the annotation compiles;
keep the method visibility unchanged but add the annotation directly above the
function declaration.
Comment on lines
+141
to
+154
| val channelInit = DataChannel.Init().apply { | ||
| if (options.id != null) { | ||
| id = options.id!! | ||
| } | ||
| if (options.maxRetransmits != null) { | ||
| maxRetransmits = options.maxRetransmits!! | ||
| } | ||
| if (options.maxPacketLifeTime != null) { | ||
| maxRetransmitTimeMs = options.maxPacketLifeTime?.inWholeMilliseconds?.toInt()!! | ||
| } | ||
| ordered = options.ordered | ||
| protocol = options.protocol | ||
| negotiated = options.negotiated | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enforce DC options invariants and guard overflow
Spec requires only one of maxRetransmits or maxPacketLifeTime. Also prevent Long→Int overflow.
Apply:
val channelInit = DataChannel.Init().apply {
- if (options.id != null) {
- id = options.id!!
- }
- if (options.maxRetransmits != null) {
- maxRetransmits = options.maxRetransmits!!
- }
- if (options.maxPacketLifeTime != null) {
- maxRetransmitTimeMs = options.maxPacketLifeTime?.inWholeMilliseconds?.toInt()!!
- }
+ options.id?.let { id = it }
+ require(!(options.maxRetransmits != null && options.maxPacketLifeTime != null)) {
+ "Only one of maxRetransmits or maxPacketLifeTime may be set"
+ }
+ options.maxRetransmits?.let { maxRetransmits = it }
+ options.maxPacketLifeTime?.let {
+ val ms = it.inWholeMilliseconds.coerceAtMost(Int.MAX_VALUE.toLong()).toInt()
+ maxRetransmitTimeMs = ms
+ }
ordered = options.ordered
protocol = options.protocol
negotiated = options.negotiated
}📝 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.
Suggested change
| val channelInit = DataChannel.Init().apply { | |
| if (options.id != null) { | |
| id = options.id!! | |
| } | |
| if (options.maxRetransmits != null) { | |
| maxRetransmits = options.maxRetransmits!! | |
| } | |
| if (options.maxPacketLifeTime != null) { | |
| maxRetransmitTimeMs = options.maxPacketLifeTime?.inWholeMilliseconds?.toInt()!! | |
| } | |
| ordered = options.ordered | |
| protocol = options.protocol | |
| negotiated = options.negotiated | |
| } | |
| val channelInit = DataChannel.Init().apply { | |
| options.id?.let { id = it } | |
| require(!(options.maxRetransmits != null && options.maxPacketLifeTime != null)) { | |
| "Only one of maxRetransmits or maxPacketLifeTime may be set" | |
| } | |
| options.maxRetransmits?.let { maxRetransmits = it } | |
| options.maxPacketLifeTime?.let { | |
| val ms = it.inWholeMilliseconds.coerceAtMost(Int.MAX_VALUE.toLong()).toInt() | |
| maxRetransmitTimeMs = ms | |
| } | |
| ordered = options.ordered | |
| protocol = options.protocol | |
| negotiated = options.negotiated | |
| } |
🤖 Prompt for AI Agents
In
ktor-client/ktor-client-webrtc/android/src/io/ktor/client/webrtc/PeerConnection.kt
around lines 141 to 154, the DataChannel.Init population currently allows both
maxRetransmits and maxPacketLifeTime to be set and unguarded Long→Int
conversions; update it so only one of options.maxRetransmits or
options.maxPacketLifeTime may be non-null (throw IllegalArgumentException if
both are set), use safe unwrapping (no !!) and when converting
maxPacketLifeTime.inWholeMilliseconds to Int check bounds to avoid overflow
(either clamp to Int.MAX_VALUE or throw if value is out of Int range) before
assigning to maxRetransmitTimeMs, and keep setting ordered/protocol/negotiated
as before.
Comment on lines
+155
to
+160
| val nativeChannel = peerConnection.createDataChannel(label, channelInit) | ||
| val receiveOptions = DataChannelReceiveOptions().apply(options.receiveOptions) | ||
| return AndroidWebRtcDataChannel(nativeChannel, channelInit, coroutineScope, receiveOptions).apply { | ||
| setupEvents(events) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Handle null from PeerConnection.createDataChannel
createDataChannel can return null on failure. Currently this would NPE later.
Apply:
- val nativeChannel = peerConnection.createDataChannel(label, channelInit)
+ val nativeChannel = peerConnection.createDataChannel(label, channelInit)
+ ?: error("Failed to create DataChannel (label=$label)")📝 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.
Suggested change
| val nativeChannel = peerConnection.createDataChannel(label, channelInit) | |
| val receiveOptions = DataChannelReceiveOptions().apply(options.receiveOptions) | |
| return AndroidWebRtcDataChannel(nativeChannel, channelInit, coroutineScope, receiveOptions).apply { | |
| setupEvents(events) | |
| } | |
| } | |
| val nativeChannel = peerConnection.createDataChannel(label, channelInit) | |
| ?: error("Failed to create DataChannel (label=$label)") | |
| val receiveOptions = DataChannelReceiveOptions().apply(options.receiveOptions) | |
| return AndroidWebRtcDataChannel(nativeChannel, channelInit, coroutineScope, receiveOptions).apply { | |
| setupEvents(events) | |
| } | |
| } |
🤖 Prompt for AI Agents
In
ktor-client/ktor-client-webrtc/android/src/io/ktor/client/webrtc/PeerConnection.kt
around lines 155-160, handle the case where
peerConnection.createDataChannel(...) can return null: check nativeChannel for
null immediately after the call and if it is null throw a clear
IllegalStateException (including the label and any relevant context) instead of
proceeding; only construct AndroidWebRtcDataChannel and call setupEvents when
nativeChannel is non-null so no NPE occurs.
Comment on lines
+198
to
+206
| override suspend fun removeTrack(track: WebRtcMedia.Track) { | ||
| val mediaTrack = track as? AndroidMediaTrack ?: error("Track should extend AndroidMediaTrack.") | ||
| val sender = rtpSenders.firstOrNull { it.track?.id == mediaTrack.id } | ||
| ?: error("Track is not found.") | ||
| if (!peerConnection.removeTrack(sender.nativeSender)) { | ||
| error("Failed to remove track.") | ||
| } | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Keep rtpSenders list in sync when removing by track
You add to rtpSenders on addTrack but never remove, leading to leaks and stale hasAudio/hasVideo checks.
Apply:
if (!peerConnection.removeTrack(sender.nativeSender)) {
error("Failed to remove track.")
}
+ rtpSenders.remove(sender)📝 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.
Suggested change
| override suspend fun removeTrack(track: WebRtcMedia.Track) { | |
| val mediaTrack = track as? AndroidMediaTrack ?: error("Track should extend AndroidMediaTrack.") | |
| val sender = rtpSenders.firstOrNull { it.track?.id == mediaTrack.id } | |
| ?: error("Track is not found.") | |
| if (!peerConnection.removeTrack(sender.nativeSender)) { | |
| error("Failed to remove track.") | |
| } | |
| } | |
| override suspend fun removeTrack(track: WebRtcMedia.Track) { | |
| val mediaTrack = track as? AndroidMediaTrack | |
| ?: error("Track should extend AndroidMediaTrack.") | |
| val sender = rtpSenders.firstOrNull { it.track?.id == mediaTrack.id } | |
| ?: error("Track is not found.") | |
| if (!peerConnection.removeTrack(sender.nativeSender)) { | |
| error("Failed to remove track.") | |
| } | |
| rtpSenders.remove(sender) | |
| } |
🤖 Prompt for AI Agents
In
ktor-client/ktor-client-webrtc/android/src/io/ktor/client/webrtc/PeerConnection.kt
around lines 198 to 206, the removeTrack implementation removes the native
sender but never updates the rtpSenders list causing memory leaks and stale
hasAudio/hasVideo checks; modify the function to locate the sender, call
peerConnection.removeTrack, and on success remove that sender entry from
rtpSenders (or always remove it if you prefer to keep internal state in sync
even when native removal fails), and ensure any derived flags
(hasAudio/hasVideo) are recalculated or computed from the current rtpSenders
after removal so the list and state remain consistent.
Comment on lines
+207
to
+212
| override suspend fun removeTrack(sender: WebRtc.RtpSender) { | ||
| val rtpSender = sender as? AndroidRtpSender ?: error("Sender should extend AndroidRtpSender.") | ||
| if (!peerConnection.removeTrack(rtpSender.nativeSender)) { | ||
| error("Failed to remove track.") | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Keep rtpSenders list in sync when removing by sender
Same leak as above when removing by sender instance.
Apply:
if (!peerConnection.removeTrack(rtpSender.nativeSender)) {
error("Failed to remove track.")
}
+ rtpSenders.remove(rtpSender)📝 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.
Suggested change
| override suspend fun removeTrack(sender: WebRtc.RtpSender) { | |
| val rtpSender = sender as? AndroidRtpSender ?: error("Sender should extend AndroidRtpSender.") | |
| if (!peerConnection.removeTrack(rtpSender.nativeSender)) { | |
| error("Failed to remove track.") | |
| } | |
| } | |
| override suspend fun removeTrack(sender: WebRtc.RtpSender) { | |
| val rtpSender = sender as? AndroidRtpSender | |
| ?: error("Sender should extend AndroidRtpSender.") | |
| if (!peerConnection.removeTrack(rtpSender.nativeSender)) { | |
| error("Failed to remove track.") | |
| } | |
| rtpSenders.remove(rtpSender) | |
| } |
Comment on lines
+85
to
+86
| public final fun initialize (Lkotlin/jvm/functions/Function1;)Lio/ktor/client/webrtc/AndroidWebRtcPeerConnection; | ||
| public fun removeTrack (Lio/ktor/client/webrtc/WebRtc$RtpSender;Lkotlin/coroutines/Continuation;)Ljava/lang/Object; |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Initialize() should be annotated as @internalapi
AndroidWebRtcPeerConnection.initialize(...) is a wiring hook and shouldn’t be part of the public stable surface.
Please annotate the function with @internalapi (and update the API dump).
🤖 Prompt for AI Agents
In ktor-client/ktor-client-webrtc/api/android/ktor-client-webrtc.api around
lines 85-86, the AndroidWebRtcPeerConnection.initialize(...) declaration must be
marked internal: annotate the function with @InternalAPI in the source where it
is declared (so it is not part of the public stable API), then regenerate/update
the API dump file to reflect the annotation change so the .api file includes the
@InternalAPI marker for this initialize signature.
bjhham
approved these changes
Sep 9, 2025
bjhham
reviewed
Sep 9, 2025
Contributor
bjhham
left a comment
bjhham
left a comment
There was a problem hiding this comment.
Noticing the build times are increased significantly on this branch. I think we should look into this before merging, since the builds are already quite slow and annoying.
Collaborator
Author
Do you mean all checks take longer now? I've noticed a significant build time increase for |
Contributor
|
Seems mostly related to Rust configurations. I've added a script to disable rust builds when the module is unchanged for certain builds in TC ktorio/ktor-build#134 |
…equestRetry since 3.2.0 (#5071) * Add some more CoroutineName * Cancel rawContent in HttpRequestRetry * Do not cancel body for saved responses
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
ktor-client/ktor-client-webrtc/src/androidMain/AndroidManifest.xml (2)
10-12: Don’t keep dangerous permissions even as comments; document instead.Good call not enabling CAMERA/RECORD_AUDIO. To avoid accidental re-introduction and reduce noise, drop these commented lines and move guidance to README.
I can add a short README section with manifest snippets and runtime permission flow. Proposed diff:
- <!-- Required for video and audio streams, for applications to declare --> - <!-- <uses-permission android:name="android.permission.CAMERA" />--> - <!-- <uses-permission android:name="android.permission.RECORD_AUDIO" />--> + <!-- Media permissions (CAMERA/RECORD_AUDIO) must be declared & requested by the app -->
16-17: Remove camera uses-feature from a library manifest (required=true by default).In a library,
<uses-feature android:name="android.hardware.camera">impliesandroid:required="true", which will merge into consuming apps and can exclude devices in Play Store. Prefer removing it entirely; if a hint is absolutely needed, mark it non-required in the app, not here.Apply this diff:
- <uses-feature android:name="android.hardware.camera" /> + <!-- Libraries should not constrain device features; apps opt-in if needed --> + <!-- (If an app wants a hint, declare camera.any with android:required="false" in the app) -->
🧹 Nitpick comments (1)
ktor-client/ktor-client-webrtc/src/androidMain/AndroidManifest.xml (1)
13-14: Consider not declaring INTERNET/ACCESS_NETWORK_STATE in a library.These are “normal” but still merge into apps. Many networking libs leave them to the app to keep transitive permissions explicit.
If you agree, apply:
- <uses-permission android:name="android.permission.INTERNET" /> - <uses-permission android:name="android.permission.ACCESS_NETWORK_STATE" /> + <!-- Apps should declare INTERNET/ACCESS_NETWORK_STATE as needed -->I can also add a README snippet listing required app-level permissions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ktor-client/ktor-client-webrtc/src/androidMain/AndroidManifest.xml(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-10T08:54:17.233Z
Learnt from: osipxd
PR: ktorio/ktor#5069
File: ktor-client/ktor-client-webrtc/src/androidMain/AndroidManifest.xml:10-11
Timestamp: 2025-09-10T08:54:17.233Z
Learning: Android library manifests should not declare dangerous permissions like CAMERA and RECORD_AUDIO as these are forced on consuming apps. Instead, document required permissions and let apps opt-in by declaring them in their own manifest and handling runtime requests.
Applied to files:
ktor-client/ktor-client-webrtc/src/androidMain/AndroidManifest.xml
This was referenced Sep 18, 2025
This was referenced Oct 6, 2025
This was referenced Oct 16, 2025
Merged
This was referenced Nov 3, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Subsystem
Client
Motivation
#KTOR-7958
Solution
WebRTC Client implementation for multiple platforms with a common interface:
getstreamimplementationWebRTC.rsandGobley.