-
Notifications
You must be signed in to change notification settings - Fork 1.2k
WebRtc Client. Introduce data channels to the common API. #5010
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WebRtc Client. Introduce data channels to the common API. #5010
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 Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
b4ea915 to
93ecff0
Compare
ktor-client/ktor-client-webrtc/common/src/io/ktor/client/webrtc/WebRtcPeerConnectionEvents.kt
Outdated
Show resolved
Hide resolved
|
@zibet27 is there a resource I can use to read about data channels? |
93ecff0 to
0ba2ff7
Compare
ktor-client/ktor-client-webrtc/common/src/io/ktor/client/webrtc/WebRtcDataChannel.kt
Outdated
Show resolved
Hide resolved
ktor-client/ktor-client-webrtc/common/src/io/ktor/client/webrtc/WebRtcDataChannel.kt
Outdated
Show resolved
Hide resolved
ktor-client/ktor-client-webrtc/common/src/io/ktor/client/webrtc/WebRtcPeerConnectionEvents.kt
Show resolved
Hide resolved
ktor-client/ktor-client-webrtc/common/src/io/ktor/client/webrtc/WebRtcDataChannel.kt
Outdated
Show resolved
Hide resolved
ktor-client/ktor-client-webrtc/common/src/io/ktor/client/webrtc/WebRtcDataChannel.kt
Outdated
Show resolved
Hide resolved
ktor-client/ktor-client-webrtc/common/src/io/ktor/client/webrtc/WebRtc.kt
Outdated
Show resolved
Hide resolved
fb6a1c4 to
aae0b90
Compare
aae0b90 to
4441aad
Compare
| launch { | ||
| while (true) { | ||
| delay(config.statsRefreshRate) | ||
| events.emitStats(getStatistics()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If getStatistics() throws an exception, this coroutine will get cancelled silently, causing stats emission to stop, which might be confusing to some users. Consider catching the exception:
launch {
while (true) {
try {
delay(config.statsRefreshRate)
events.emitStats(getStatistics())
} catch (e: CancellationException) {
throw e // Respect coroutine cancellation
} catch (e: Exception) {
// Optional: log or emit an error event
}
}
}Hope this helps 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! In the new commit, I've updated WebRtcConnectionConfig so it accepts a custom coroutinesContext, which could be CoroutineExceptionHandler. Do you think we should provide a method to restart stats collection or do it automatically?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great question! I think it depends on how resilient the stats collection is intended to be.
If it's meant to be long-running (e.g. like telemetry), I would lean toward restarting it automatically after non-cancellation exceptions, possibly logging or emitting an error event on the first failure. That way, users won't be surprised by stats stopping silently.
Alternatively, if we expect more advanced control or debugging scenarios, exposing a restartStatsCollection() method could be a good fallback.
Either way, I think you're on the right track by letting the coroutineContext carry the handler!
… custom `coroutineContext` in the rtc connection.
Mr3zee
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the new way here, thanks! Great job @zibet27
ktor-client/ktor-client-webrtc/common/src/io/ktor/client/webrtc/WebRtcDataChannel.kt
Outdated
Show resolved
Hide resolved
064e153 to
a91a285
Compare
77956ec
into
zibet27/ktor-client-webrtc-android
Subsystem
Ktor Client WebRtc
Motivation
The data channel is an important part of the WebRTC protocol. I've implemented the common API and added stubs to existing engines. In the following commits, I will add real implementations.
Please review.