WebRtc Client. Introduce Native engine based on WebRTC.rs #5044
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughIntroduces a Rust-based WebRTC engine and bindings, adds Kotlin wrappers/integration and tests, restructures project to include a nested rs module, and updates APIs: splits DataChannel close() into closeTransport() with default close(), exposes an events emitter, and adds awaitIceGatheringComplete(). Minor gradle description and ignore additions. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ minutes Suggested labels
Suggested reviewers
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
zibet27
force-pushed
the
zibet27/ktor-client-webrtc-gobley
branch
from
2a9c333 to
23bc3ae
Compare
… Works only for jvm and separated into different submodule for now.
zibet27
force-pushed
the
zibet27/ktor-client-webrtc-gobley
branch
from
23bc3ae to
aa65b57
Compare
bjhham
left a comment
bjhham
left a comment
There was a problem hiding this comment.
Very cool! I'll take it for a spin locally and see if anything comes up!
ktor-client/ktor-client-webrtc/android/src/io/ktor/client/webrtc/DataChannel.kt
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/ktor-client-webrtc-rs/common/rust/media/sink.rs
Outdated
Show resolved
Hide resolved
ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/common/rust/media/sink.rs
Outdated
Show resolved
Hide resolved
ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/common/rust/media/track.rs
Outdated
Show resolved
Hide resolved
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 32
🔭 Outside diff range comments (2)
ktor-client/ktor-client-webrtc/android/src/io/ktor/client/webrtc/DataChannel.kt (1)
119-123: Fix BufferedAmountLow emission logic (wrong condition prevents event)You should emit BufferedAmountLow when the buffered amount crosses the threshold from above to at-or-below. The current range check inverts that and suppresses the event in the exact scenario where it should fire.
- if (bufferedAmountLowThreshold in (bufferedAmount + 1)..<previousAmount) { - return@launch - } - val event = DataChannelEvent.BufferedAmountLow(this@AndroidWebRtcDataChannel) - eventsEmitter.emitDataChannelEvent(event) + val crossedLow = + previousAmount > bufferedAmountLowThreshold && + bufferedAmount <= bufferedAmountLowThreshold + if (crossedLow) { + val event = DataChannelEvent.BufferedAmountLow(this@AndroidWebRtcDataChannel) + eventsEmitter.emitDataChannelEvent(event) + }ktor-client/ktor-client-webrtc/api/jvm/ktor-client-webrtc.api (1)
474-485: WebRtcDataChannel.close() exposure on JVM: align with interface defaultEnsure
WebRtcDataChannel.close()aligns with the interface’s defaultclose()contract (delegation tocloseTransport()and higher-level cleanup). Also, confirm that any pending receive calls are unblocked/cancelled to prevent leaks or hung coroutines.If not already, consider adding a test that calls
close()while a receiver is suspended to validate unblocking behavior.
♻️ Duplicate comments (4)
ktor-client/ktor-client-webrtc/common/src/io/ktor/client/webrtc/WebRtcPeerConnection.kt (1)
123-130: Simplify awaitIceGatheringComplete and handle flow completion edge casesYou can simplify this to a single call;
first { it == COMPLETE }will also return immediately when the currentStateFlowvalue is COMPLETE, making the explicit pre-check redundant.Also consider what should happen if the flow completes without emitting COMPLETE (e.g., connection closed early). Right now this suspends indefinitely in that case.
- public suspend fun awaitIceGatheringComplete() { - if (iceGatheringState.value == WebRtc.IceGatheringState.COMPLETE) { - return - } - iceGatheringState - .filter { it == WebRtc.IceGatheringState.COMPLETE } - .first() // Suspends until the first "COMPLETE" is emitted - } + public suspend fun awaitIceGatheringComplete() { + // Suspends until COMPLETE is observed (returns immediately if already COMPLETE) + iceGatheringState.first { it == WebRtc.IceGatheringState.COMPLETE } + }If you want to fail fast when the flow completes (or on connection close), I can propose a variant that races with a closure signal or wraps this in a timeout. Do you want that?
ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/common/rust/media/sink.rs (2)
16-16: The doc comment should use//for consistency with the past review suggestion.Based on the past review comment, regular line comments are preferred here instead of doc comments.
131-142: Video resolution and frame rate should be configurable.The VP8 sink creation hard-codes width: 640, height: 480, and FPS: 30. These should be parameterized or obtained from the actual track properties to ensure proper encoding.
Consider passing video parameters through the handler or retrieving them from the track:
pub fn create_vp8_video_sink( &self, handler: Arc<dyn MediaSinkHandler>, ) -> Result<Arc<MediaStreamSink>, RtcError> { + // TODO: Get actual video parameters from track or configuration + let (width, height, fps) = self.get_video_parameters().unwrap_or((640, 480, 30)); // Create IVF header for VP8 let header = IVFFileHeader { signature: *b"DKIF", version: 0, header_size: IVF_FILE_HEADER_SIZE as u16, four_cc: *b"VP80", // VP8 FourCC - width: 640, - height: 480, - timebase_denominator: 30, // 30 FPS default + width, + height, + timebase_denominator: fps, timebase_numerator: 1, num_frames: 0, // Unknown for streaming unused: 0, };ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/common/rust/media/track.rs (1)
21-35: Kind detection based on MIME prefix — future-proofing and explicit errorsThe MIME prefix check is fine for current codecs. Consider adding an explicit error or fallback for unknown prefixes to avoid silently classifying as video.
Optionally, map MIME→kind via a minimal function and default to error on unknown input.
🧹 Nitpick comments (51)
ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/.gitignore (1)
1-2: Anchor ignore patterns to this directoryUse a leading slash to prevent accidentally ignoring nested target/build directories in deeper paths under this module.
Apply this diff:
-target/ -build/ +/target/ +/build/ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/Cargo.toml (3)
7-14: Don’t depend on tracing-subscriber by default in a libraryPulling in tracing-subscriber in a library can cause double-global-initialization conflicts and surprises for embedders. Make it optional and let the application set up subscribers.
[dependencies] uniffi = { version = "0.29.4", features = ["tokio"] } tokio = "1.47.1" webrtc = "0.13.0" thiserror = "2.0.12" arc-swap = "1.7.1" serde_json = "1.0.141" -tracing-subscriber = "0.3.19" +tracing = "0.1" +tracing-subscriber = { version = "0.3.19", optional = true } + +[features] +default = [] +# Enable this feature in binaries/tests to install a subscriber. +enable-tracing = ["dep:tracing-subscriber"]
10-10: Minimize the webrtc crate surface by disabling default featuresTo reduce compile time, binary size, and transitive deps, consider disabling default features and enabling only what’s required for data channels. The webrtc crate’s defaults pull in media stacks you may not need.
-webrtc = "0.13.0" +# TODO: enable only the minimal features needed for data channels. +# See webrtc-rs crate documentation for the definitive feature list. +webrtc = { version = "0.13.0", default-features = false /*, features = ["<fill-in>"] */ }If you want, I can scan your Rust sources to infer the minimal set and propose a precise feature list.
2-5: Consider marking the crate as non-publishable if internalIf this crate is not intended for crates.io, prevent accidental publishing.
[package] name = "ktor-client-webrtc" version = "0.1.0" edition = "2024" +publish = falsektor-client/ktor-client-webrtc/ktor-client-webrtc-rs/gradle.properties (1)
4-10: Document the rationale and re-enable plan inline.
Add a short comment so future maintainers know this module is intentionally JVM-only right now due to WebRTC.rs cross-compilation constraints, and where to track re-enablement.Apply this diff to insert an explanatory comment:
target.js=false target.wasmJs=false target.androidNative=false target.ios=false target.tvos=false target.watchos=false target.posix=false +# +# Note: +# This module is JVM-only for now because WebRTC.rs/UniFFI cross-compilation is not ready. +# See PR #5044 for context. Revisit these flags as native/JS/Wasm support lands.ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/build.gradle.kts (3)
7-7: Nit: tighten the module description phrasingConsider rephrasing to remove the article and clarify provenance.
-description = "Ktor WebRTC Engine based on the WebRTC.rs and Gobley" +description = "Ktor WebRTC engine based on WebRTC.rs and Gobley"
9-14: Centralize plugin versions; keep them consistent with the version catalogHardcoding "0.3.1" here fragments version management. Prefer pulling versions from the version catalog or using plugin aliases if available, similar to how Kotlin version is sourced from libs.versions.
Two options:
- If plugin aliases exist in libs.versions.toml:
- id("dev.gobley.cargo") version "0.3.1" - id("dev.gobley.uniffi") version "0.3.1" + alias(libs.plugins.gobley.cargo) + alias(libs.plugins.gobley.uniffi)
- Otherwise, define versions.gobley in the catalog and reference it:
- id("dev.gobley.cargo") version "0.3.1" - id("dev.gobley.uniffi") version "0.3.1" + id("dev.gobley.cargo") version libs.versions.gobley.get() + id("dev.gobley.uniffi") version libs.versions.gobley.get()Also, verify the atomicfu plugin is needed for this module (see comment below).
20-25: Embedding native library only on host: LGTM; consider build reproducibility knobsThe host-only embedding is a sensible default to avoid CI cross-compilation. Consider adding reproducibility knobs if supported by the Gobley plugin, e.g., setting a deterministic cargo target directory under build/, locking the Rust toolchain/profile, and enabling LTO/strip for release artifacts.
Examples (adjust if supported by the plugin):
- Place Cargo target under Gradle build dir
- Pin Rust toolchain/profile in cargo config for CI
ktor-client/ktor-client-webrtc/common/src/io/ktor/client/webrtc/WebRtc.kt (1)
461-471: Clarify and deduplicate KDoc for closeTransport(); add idempotency note.Minor doc issues:
- Grammar: “After calling a channel will start a closing process” → “After calling, the channel will start…”
- The sentence “The underlying message receiving channel will be closed” is duplicated (once standalone, once as a bullet).
- Consider stating idempotency explicitly.
Proposed KDoc tweak:
- /** - * Closes the data channel transport. The underlying message receiving channel will be closed. - * - * After calling a channel will start a closing process: + /** + * Closes the data channel transport. + * + * After calling, the channel will start the closing process: * - The channel state will transition to [WebRtc.DataChannel.State.CLOSED] * - No more messages can be sent through this channel - * - The underlying message receiving channel will be closed + * - The underlying message receiving channel will be closed * - Any pending send operations may fail * - A [DataChannelEvent.Closed] event will be emitted + * + * This method is idempotent; calling it multiple times has no additional effect. */ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/common/src/io/ktor/client/webrtc/rs/Utils.kt (2)
30-34: Improve error message clarity for null sdpMLineIndex.Make the message explicit to ease debugging.
internal fun IceCandidate.toKtor() = WebRtc.IceCandidate( candidate = candidate, sdpMid = sdpMid ?: error("sdpMid is null"), - sdpMLineIndex = sdpMlineIndex?.toInt() ?: error("sdpMLineIndex") + sdpMLineIndex = sdpMlineIndex?.toInt() ?: error("sdpMLineIndex is null") )
36-40: Bounds check for sdpMLineIndex conversion to UShort (optional).
toUShort()will truncate ifsdpMLineIndexis out of [0, 65535]. While SDP m-line indices are expected to be small, adding a sanity check could prevent silent truncation.Optional guard:
internal fun WebRtc.IceCandidate.toRust() = IceCandidate( sdpMid = sdpMid, candidate = candidate, - sdpMlineIndex = sdpMLineIndex.toUShort() + sdpMlineIndex = sdpMLineIndex.also { + require(it in 0..UShort.MAX_VALUE.toInt()) { "sdpMLineIndex out of UShort range: $it" } + }.toUShort() )ktor-client/ktor-client-webrtc/common/src/io/ktor/client/webrtc/WebRtcPeerConnection.kt (1)
11-12: Remove unnecessary import and simplify await logicYou can avoid importing
filterby usingfirst { predicate }directly. See the suggested refactor on awaitIceGatheringComplete().-import kotlinx.coroutines.flow.filter import kotlinx.coroutines.flow.firstktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/common/test/io/ktor/client/webrtc/rs/utils/MockMediaDevices.kt (1)
14-14: Optional: make id private or thread-safe in testsIf tests construct tracks concurrently,
var id = 0can race. Consider:
- Make it
private var id = 0if only used internally.- Or use
AtomicIntegerfor thread-safety.ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/common/src/io/ktor/client/webrtc/rs/MediaTrack.kt (2)
51-57: Nit: documentation wordingThis extension returns the native media stream track for any track (audio or video), not specifically “video”. Adjust wording to avoid confusion.
-/** - * Returns implementation of the native video stream track used under the hood. Use it with caution. - */ +/** + * Returns the underlying native MediaStreamTrack used under the hood. Use it with caution. + */
28-31: Idempotent close considerationIf
inner.destroy()is not idempotent, repeated close() calls from user and GC/finalizer paths could error. If the UniFFI object guarantees idempotency, this is fine; otherwise consider guarding.Would you like me to add a simple guard (e.g., an
AtomicBooleanflag) to ensure idempotency?ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/common/src/io/ktor/client/webrtc/rs/DataChannel.kt (5)
17-19: KDoc incorrectly states “Android platform”This implementation is for the WebRTC.rs-based engine, not specifically Android. Please correct to avoid confusion.
Apply:
-/** - * WebRtc data channel implementation for the Android platform. - */ +/** + * WebRTC DataChannel implementation backed by WebRTC.rs (via UniFFI). + */
35-37: Avoid blocking the caller in property gettersUsing runBlocking inside getters can deadlock when called from a single-threaded or constrained dispatcher and will block the caller thread. If inner.bufferedAmount() is suspend, consider either:
- Making the API explicitly asynchronous (if possible in the interface), or
- Reading from a dedicated IO/FFI dispatcher with withContext and caching the last known value, or
- Documenting this as a blocking call and avoiding calling it from hot paths.
Given the current interface constraints, at minimum add KDoc clarifying its blocking nature and ensure it runs on a dispatcher safe for blocking operations.
77-81: Guard event dispatch with error handlingrunInScope launches UNDISPATCHED to preserve ordering, which is good. However, any exception in block() will cancel the launched coroutine and may get lost. Consider catching and routing to a connection-level error emitter/log.
Example:
internal inline fun runInScope(crossinline block: suspend () -> Unit) { coroutineScope.launch(start = CoroutineStart.UNDISPATCHED) { try { block() } catch (t: Throwable) { // TODO: route to a logger or events.emitError if available } } }
83-87: Avoid runBlocking during observer registrationsetupEvents blocks the caller to register the observer. If registerObserver is synchronous, remove runBlocking. If it’s suspend, consider dispatching to the connection context rather than blocking.
Apply:
- internal fun setupEvents(events: WebRtcConnectionEventsEmitter): Unit = runBlocking { - inner.registerObserver(object : DataChannelObserver { + internal fun setupEvents(events: WebRtcConnectionEventsEmitter) { + inner.registerObserver(object : DataChannelObserver {If registerObserver is suspend, prefer: runInScope { inner.registerObserver(…) } to avoid blocking.
103-110: Rename shadowed variable in onMessageThe local val message shadows the parameter, which hurts readability. Rename to evt or parsed.
Apply:
- override fun onMessage(message: DataChannelMessage) = runInScope { + override fun onMessage(message: DataChannelMessage) = runInScope { // This coroutine should start immediately because the protocol relies on the message order - val message = when (message.isString) { - true -> WebRtc.DataChannel.Message.Text(data = message.data.toString(Charsets.UTF_8)) - false -> WebRtc.DataChannel.Message.Binary(data = message.data) - } - emitMessage(message) + val evt = if (message.isString) { + WebRtc.DataChannel.Message.Text(data = message.data.toString(Charsets.UTF_8)) + } else { + WebRtc.DataChannel.Message.Binary(data = message.data) + } + emitMessage(evt) }ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/common/src/io/ktor/client/webrtc/rs/Senders.kt (1)
33-38: rtcp typed as Any with a string sentinelExposing rtcp as Any = "Unsupported parameter" is error-prone. Prefer a well-defined nullable type or a sealed representation (e.g., null or a dedicated Unsupported singleton) aligned with the common API.
- If WebRtc.RtpParameters.rtcp is not used, make it nullable and return null.
- If the common API already expects Any, document the sentinel and ensure consumers won’t cast it.
ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/common/test/io/ktor/client/webrtc/rs/WebRtcEngineTest.kt (3)
58-76: Consider awaiting ICE gathering before setting remote description (stability)In testCreateAnswer you set local on offerer and immediately set remote on answerer. For more deterministic behavior across environments, await ICE gathering completion on the offerer before setting remote on the answerer, similar to negotiate().
Apply:
// Create and set offer val offer = offerPeerConnection.createOffer() offerPeerConnection.setLocalDescription(offer) + // For stability, ensure ICE candidates are gathered on the offerer + offerPeerConnection.awaitIceGatheringComplete() answerPeerConnection.setRemoteDescription(offer)
90-143: End-to-end flow is solid; consider adding a data channel testThis suite exercises SDP/ICE/tracks/stats well. Given the new RS data channel implementation, add a test validating:
- onOpen/onMessage ordering,
- Text/binary roundtrip,
- closeTransport vs close semantics,
- bufferedAmountLow event.
I can draft a test using two peer connections with negotiated data channels.
171-184: Stats test is pragmatic; consider a slightly larger timeout for CI varianceWith statsRefreshRate=100ms, 5s is usually plenty, but CI under load can be spiky. A 7–10s timeout reduces flakiness with minimal cost.
ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/common/src/io/ktor/client/webrtc/rs/Engine.kt (3)
22-24: Outdated comment about “simplified implementation”Bindings are already integrated; this comment is stale and may confuse maintainers.
Apply:
-/** - * Implementation of WebRtcEngine using Rust and webrtc.rs. - * This is a simplified implementation that will be expanded once the UniFfi bindings are generated. - */ +/** + * WebRtcEngine implementation backed by WebRTC.rs (via UniFFI). + */
28-30: Error message grammar and wordingClarify engine name and fix grammar.
Apply:
- ?: error( - "There are no default media track factory when using the common engine. Please provide one in the config." - ) + ?: error( + "No default MediaTrackFactory is provided for the WebRTC.rs engine. Please supply one via WebRtcConfig.mediaTrackFactory." + )
39-55: Config mapping looks good; consider making addDefaultTransceivers configurableHardcoding addDefaultTransceivers = true may be surprising. If the common config supports it (or can be extended), forward the flag to native to avoid implicit behavior.
ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/common/rust/media/mod.rs (2)
41-44: Consider clarifying the writer mutex choiceMediaStreamSink wraps the Writer in tokio::sync::Mutex. If the underlying Writer is used only on a single async task, a synchronous std::sync::Mutex may be sufficient and cheaper. If it’s awaited across async tasks, tokio::Mutex is appropriate. Add a short comment to explain the choice.
71-76: CodecMimeType set is minimalIf you plan to support VP9/AV1/Opus variations later, consider documenting the limitation here or in the UDL so consumers don’t assume full parity with browser implementations.
ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/common/test/io/ktor/client/webrtc/rs/WebRtcMediaTest.kt (3)
17-24: Avoid duplicating test harness helpers; centralize in utils.
testConnectionduplicates the same pattern used elsewhere (see WebRtcEngineTest). Consider moving this helper tors/utils/ConnectionUtils.ktto keep tests DRY and consistent.
61-64: SDP assertion too weak; check for media section explicitly.
contains("audio")can yield false positives. Check form=audioon a line boundary to assert an audio media section is present.Apply this diff:
- assertTrue(offerWithTrack.sdp.contains("audio"), "SDP should contain audio media section") + assertTrue( + Regex("(?m)^m=audio\\b").containsMatchIn(offerWithTrack.sdp), + "SDP should contain an 'm=audio' media section" + )
123-137: Use structural equality for enums (==) instead of identity (===).While
===happens to work for enums,==is the idiomatic and clearer choice in Kotlin.Apply this diff:
- assertEquals(1, tracks.filter { it.track.kind === WebRtcMedia.TrackType.AUDIO }.size) - assertEquals(1, tracks.filter { it.track.kind === WebRtcMedia.TrackType.VIDEO }.size) + assertEquals(1, tracks.filter { it.track.kind == WebRtcMedia.TrackType.AUDIO }.size) + assertEquals(1, tracks.filter { it.track.kind == WebRtcMedia.TrackType.VIDEO }.size)And similarly below:
- assertEquals(1, tracks.filter { it.track.kind === WebRtcMedia.TrackType.AUDIO }.size) - assertEquals(1, tracks.filter { it.track.kind === WebRtcMedia.TrackType.VIDEO }.size) + assertEquals(1, tracks.filter { it.track.kind == WebRtcMedia.TrackType.AUDIO }.size) + assertEquals(1, tracks.filter { it.track.kind == WebRtcMedia.TrackType.VIDEO }.size)ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/common/test/io/ktor/client/webrtc/rs/utils/ConnectionUtils.kt (2)
39-64: Timeout literal is confusing; prefer clear duration.
withTimeout(10_0000)equals 100000ms but looks like a typo. Prefer100_000or a Duration-based helper.Apply this diff:
- withTimeout(10_0000) { + withTimeout(100_000) {Optionally, rework to Duration-based APIs (e.g.,
withTimeout(100.seconds)if available).
94-105: Collector drops silently; handle trySend failures or close channel.
channel.trySend(it)return value is ignored. On rare occasions (e.g., closed channel), this may silently drop events. Either check.isSuccessor close the channel when the collector is cancelled.Apply this diff:
- val collectorJob = scope.launch { - collect { channel.trySend(it) } - } + val collectorJob = scope.launch { + try { + collect { + val _ = channel.trySend(it) + } + } finally { + channel.close() + } + }ktor-client/ktor-client-webrtc/common/src/io/ktor/client/webrtc/WebRtcPeerConnectionEvents.kt (2)
83-92: Class KDoc still says “Internal” but visibility is now public.Update the documentation to reflect its public role and intended usage boundaries (e.g., public type with restricted construction/emitters used by engine implementations).
Apply this diff:
-/** - * Internal implementation of [WebRtcConnectionEvents] that emits events to flows. - * - * This class maintains the mutable flows and provides methods to emit events - * from platform-specific WebRTC implementations. - */ +/** + * Emitter-backed implementation of [WebRtcConnectionEvents] that exposes read-only flows + * and provides methods to emit events from engine/platform implementations. + * + * Library consumers should treat this as an internal plumbing type; construction and emission + * are reserved for engine integrations. + */
89-92: Consider restricting construction while keeping the type public.Making the emitter public is necessary for cross-module engines, but it opens emission to any consumer. Consider making the constructor
internal(if feasible across module boundaries) or annotating with@InternalKtorApito signal restricted use.If
internal constructoris not possible due to module boundaries, you can:
- Keep the class public, constructor public, but gate emit methods behind an
@InternalKtorApiopt-in.- Or expose a public read-only interface and keep an internal emitter implementation. Engine modules can get it via friend modules if the build allows it.
Also applies to: 124-162
ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/common/rust/datachannel.rs (1)
137-143: Consider using string reference parameter instead of owned reference.The
send_textmethod takes&Stringwhich is unconventional. It should take&strfor better flexibility and idiomatic Rust.-pub async fn send_text(&self, text: &String) -> Result<u64, RtcError> { +pub async fn send_text(&self, text: &str) -> Result<u64, RtcError> { self.inner .send_text(text) .await .map(|bytes_sent| bytes_sent as u64) .map_err(|e| DataChannelError(e.to_string())) }ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/common/rust/rtc.rs (1)
223-232: Consider using a more reliable time conversion approach.The current implementation approximates the epoch time from an
Instantby calculating the duration offset from the current system time. This approach could be inaccurate if there's significant time between when the Instant was created and when this function is called, or if the system clock changes.Consider storing the system time alongside the Instant when stats are collected, or use a monotonic clock with a known epoch reference:
fn instant_to_epoch_millis(instant: &Instant) -> Result<u64, RtcError> { let system_now = SystemTime::now(); let instant_now = Instant::now(); + // Note: This approximation assumes minimal delay between instant creation and conversion let approx = system_now - (instant_now - *instant); let epoch = approx .duration_since(UNIX_EPOCH) .map_err(|_| StatsError("Time went backwards".to_string()))?; Ok(epoch.as_millis() as u64) }ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/common/src/io/ktor/client/webrtc/rs/Connection.kt (1)
41-47: Shadowing variable name could cause confusion.The local variable
channelshadows the parameter name, which could be confusing.override fun onDataChannel(channel: DataChannel) { - val channel = RustWebRtcDataChannel( + val wrappedChannel = RustWebRtcDataChannel( inner = channel, coroutineScope = coroutineScope, receiveOptions = DataChannelReceiveOptions() ) - channel.setupEvents(events) + wrappedChannel.setupEvents(events) }ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/common/rust/connection.rs (2)
211-211: Consider using DataChannel::new() for consistency.For consistency with other parts of the codebase (like in create_data_channel), consider using the DataChannel::new() constructor.
-observer.on_data_channel(Arc::new(DataChannel { inner: channel })); +observer.on_data_channel(Arc::new(DataChannel::new(channel)));
136-150: Track removal implementation may be inefficient for many senders.The current implementation iterates through all senders to find the matching track. This could be inefficient with many senders.
Consider maintaining a mapping of track IDs to senders for O(1) lookup, or document that this is acceptable for typical use cases with a small number of tracks.
ktor-client/ktor-client-webrtc/api/jvm/ktor-client-webrtc.api (1)
642-642: awaitIceGatheringComplete(): consider timeout or cancellation guidanceThis is a useful utility. Recommend documenting expected behavior if ICE gathering never reaches COMPLETE (e.g., long-running trickle or network issues). Consider an overload with timeout, or clearly state that the call is cancellable and intended to be used with a timeout at the callsite.
ktor-client/ktor-client-webrtc/api/ktor-client-webrtc.klib.api (1)
171-173: awaitIceGatheringComplete(): document behavior across platformsEnsure the suspend function’s completion condition and cancellation behavior are consistent across JS/native/JVM. Document recommended usage (e.g., with timeout) to avoid indefinite suspension.
ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/common/rust/media/track.rs (5)
171-173: set_enabled() relaxed ordering is likely fine; consider document consistencyRelaxed ordering is typically acceptable for a simple enabled flag, but a short comment explaining why Relaxed is safe (no cross-thread invariants) would help future maintainers.
175-193: write_sample: conversion and error mapping look correctSample creation and error mapping are straightforward. One minor: consider reusing allocation for large buffers or documenting that ownership is transferred to Bytes to avoid unnecessary clones.
194-208: write_data: consider non-async variant or separate convenience wrapperThis is a good convenience. Optionally provide a non-async
write_data_blockingwrapper (if used in sync contexts) or document that it’s async due to underlyingwrite_sample.
214-231: read_rtp: single-packet read path looks correct; consider behavior without sinkCurrent behavior is no-op when sink absent. Consider returning a specific Ok state or docstring clarifying it intentionally discards the packet when sink is not set.
233-255: read_all: on error, silently closes sink; consider surfacing error or loggingSwallowing read errors and returning Ok may hide root causes. Suggest logging the error, or returning a distinct error type if that helps callers decide how to proceed.
Also, since you cache
writerbefore the loop, updates viaset_sinkwon’t be seen. If hot-swapping sinks is expected, re-load the sink within the loop.ktor-client/ktor-client-webrtc/api/android/ktor-client-webrtc.api (3)
198-221: DataChannel interface change mirrored on Android — migration noteSame notes as JVM: default close() + abstract closeTransport(). Please add migration docs for external implementers of the Android channel, if any.
553-575: WebRtcConnectionEventsEmitter: Android API parity — looks goodAndroid API mirrors JVM/KMP emitter surface. Ensure backpressure and replay settings follow
WebRtcConnectionConfig.
744-744: awaitIceGatheringComplete(): add guidance for timeoutsDocument timeout/cancellation guidance to avoid indefinite waits on Android.
ktor-client/ktor-client-webrtc/android/src/io/ktor/client/webrtc/DataChannel.kt
Show resolved
Hide resolved
ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/build.gradle.kts
Outdated
Show resolved
Hide resolved
...-webrtc/ktor-client-webrtc-rs/common/test/io/ktor/client/webrtc/rs/utils/MockMediaDevices.kt
Show resolved
Hide resolved
...t-webrtc/ktor-client-webrtc-rs/common/test/io/ktor/client/webrtc/rs/WebRtcDataChannelTest.kt
Outdated
Show resolved
Hide resolved
...t-webrtc/ktor-client-webrtc-rs/common/test/io/ktor/client/webrtc/rs/WebRtcDataChannelTest.kt
Show resolved
Hide resolved
...t-webrtc/ktor-client-webrtc-rs/common/test/io/ktor/client/webrtc/rs/WebRtcDataChannelTest.kt
Show resolved
Hide resolved
...-client-webrtc/ktor-client-webrtc-rs/common/test/io/ktor/client/webrtc/rs/WebRtcMediaTest.kt
Show resolved
Hide resolved
|
Could you also include build instructions in the readme / contributors guide? It looks like the builds are failing for native targets even when they're disabled due to the missing cargo binary. |
ktor-client/ktor-client-webrtc/common/src/io/ktor/client/webrtc/WebRtc.kt
Outdated
Show resolved
Hide resolved
ktor-client/ktor-client-webrtc/jsAndWasmShared/src/io/ktor/client/webrtc/DataChannel.kt
Outdated
Show resolved
Hide resolved
Working on that. CI doesn't have the Rust toolchain and Android NDK installed. |
ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/build.gradle.kts
Outdated
Show resolved
Hide resolved
zibet27
force-pushed
the
zibet27/ktor-client-webrtc-gobley
branch
from
89af783 to
d2250cb
Compare
zibet27
force-pushed
the
zibet27/ktor-client-webrtc-gobley
branch
from
d2250cb to
cfac5b1
Compare
|
Looks like |
@bjhham Yes, it is. I am thinking about the improvement. The nature of this test is flaky because we don't know exactly the number of frames after which the |
|
@zibet27 looks like it finishes quickly when it passes, so it could be a problem with the connection or something happening out of sequence. It may help to add some logging. |
zibet27
force-pushed
the
zibet27/ktor-client-webrtc-gobley
branch
from
35be464 to
f113482
Compare
zibet27
force-pushed
the
zibet27/ktor-client-webrtc-gobley
branch
from
f113482 to
b8c5e43
Compare
zibet27
merged commit 80ed420
into
zibet27/ktor-client-webrtc-android
Subsystem
WebRTC Client
Motivation
Sometimes, we need only WebRTC data channels without capturing and sending any video or audio streams. In this case, almost all targets could be easily supported (native, JVM, etc).
Solution
MediaTrackFactoryprovided.MediaStream APIabstractions (without much protocol-specific logic) are included for compatibility with Ktor's WebRTC Client common API. They are rather mock than ready-to-use and require further testing.Autoclosablenegotiation_neededevent.