-
Notifications
You must be signed in to change notification settings - Fork 143
added Single Peer Connection support to Rust #888
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
base: main
Are you sure you want to change the base?
Conversation
livekit/src/room/mod.rs
Outdated
| @@ -1059,6 +1066,35 @@ impl RoomSession { | |||
| track_id = stream_id.into(); | |||
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.
nitpick: This could be moved into the else block below.
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.
Done
livekit/src/room/mod.rs
Outdated
| return; | ||
| } | ||
| // In dual PC mode: answer is local, offer is remote | ||
| (remote_desc.unwrap(), Some(local_desc.unwrap())) |
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.
question: Is it possible for sync state to be requested at a time when the subscriber PC lacks a local or remote description? If yes or it is unclear, we should avoid unwrapping here, and instead log a warning.
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.
Good point, I actually don't know if it is possible, but I think it is dangerous to do unwrap() here, let me address it.
| }; | ||
|
|
||
| // Create ClientInfo | ||
| let client_info = proto::ClientInfo { |
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.
nitpick: Spreading ..Default::default() would be cleaner here:
let client_info = proto::ClientInfo {
sdk: proto::client_info::Sdk::Rust as i32,
version: options.sdk_options.sdk_version.clone().unwrap_or_default(),
protocol: PROTOCOL_VERSION as i32,
os: std::env::consts::OS.to_string(),
client_protocol: 1, // Indicates support for RPC compression
..Default::default()
};Same for the other protobuf types below where we set an empty string or Option::None.
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.
Done
ladvoc
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.
It looking good so far! Before merging, I think it's important to test various reconnect scenarios, as this change could potentially break them.
livekit-api/src/signal_client/mod.rs
Outdated
| return Err(err); | ||
| } | ||
|
|
||
| // If using v1 path and it failed, always try fallback to v0 path. |
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.
on the browser websocket implementation we unfortunately don't get a 404 for this error case back, but arguably we don't want to retry with v0 path if the connection error is anything other than "v1 path isn't available".
Would be good to try and see what kind of error we get here if the server doesn't support it and only initiate the fallback in that case
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.
when you are running a livekit dev server at your localhost, it will fail.
The error that I get is :
[INFO livekit_api::signal_client::signal_stream] connecting to ws://localhost:7880/rtc?sdk=rust&protocol=16&auto_subscribe=1&adaptive_stream=0&version=0.7.30&reconnect=1&sid=PA_8z9tP8yTwhtr
[INFO peer_connection_signaling_test::signaling_tests] [V1 (Single PC)] Reconnecting after node failure...
[ERROR livekit::rtc_engine] resuming connection failed: signal failure: ws failure: Connection closed normally
[ERROR livekit::rtc_engine] restarting connection... attempt: 1
[INFO livekit_api::signal_client::signal_stream] connecting to ws://localhost:7880/rtc/v1?join_request=EhkKEwgIEgYwLjcuMzAYECIFbWFjb3MSAggB
[WARN livekit_api::signal_client] signal connection failed on v1 path: WsError(Http(Response { status: 404, version: HTTP/1.1, headers: {"content-type": "text/plain; charset=utf-8", "vary": "Origin", "x-content-type-options": "nosniff", "date": "Wed, 11 Feb 2026 21:38:09 GMT", "content-length": "19"}, body: Some([52, 48, 52, 32, 112, 97, 103, 101, 32, 110, 111, 116, 32, 102, 111, 117, 110, 100, 10]) }))
[WARN livekit_api::signal_client] v1 path failed, falling back to v0 path
[INFO livekit_api::signal_client::signal_stream] connecting to ws://localhost:7880/rtc?sdk=rust&protocol=16&auto_subscribe=1&adaptive_stream=0&version=0.7.30
[INFO livekit_api::signal_client::signal_stream] connecting to ws://localhost:7880/rtc/v1?join_request=EhkKEwgIEgYwLjcuMzAYECIFbWFjb3MSAggB
[WARN livekit_api::signal_client] signal connection failed on v1 path: WsError(Http(Response { status: 404, version: HTTP/1.1, headers: {"content-type": "text/plain; charset=utf-8", "vary": "Origin", "x-content-type-options": "nosniff", "date": "Wed, 11 Feb 2026 21:38:09 GMT", "content-length": "19"}, body: Some([52, 48, 52, 32, 112, 97, 103, 101, 32, 110, 111, 116, 32, 102, 111, 117, 110, 100, 10]) }))
[WARN livekit_api::signal_client] v1 path failed, falling back to v0 path
[INFO livekit_api::signal_client::signal_stream] connecting to ws://localhost:7880/rtc?sdk=rust&protocol=16&auto_subscribe=1&adaptive_stream=0&version=0.7.30
[INFO peer_connection_signaling_test::signaling_tests] [V1 (Single PC)] Connected! Room: "test_SinglePC_64069a7c-c66a-46ca-b3db-7d73a81051b8"
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.
[WARN livekit_api::signal_client] signal connection failed on v1 path: WsError(Http(Response { status: 404, version: HTTP/1.1, headers: {"content-type": "text/plain; charset=utf-8", "vary": "Origin", "x-content-type-options": "nosniff", "date": "Wed, 11 Feb 2026 21:38:09 GMT", "content-length": "19"}, body: Some([52, 48, 52, 32, 112, 97, 103, 101, 32, 110, 111, 116, 32, 102, 111, 117, 110, 100, 10]) }))
the WsError here has a 404 status, which is great insight. It tells us that the endpoint for v1 doesn't exist.
This also means the fallback to v0 should only be applied if the error status is a 404.
…n) signaling, add tests
69d4597 to
a13c540
Compare
xianshijing-lk
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 addressed all the comments, please take another look
livekit-api/src/signal_client/mod.rs
Outdated
| return Err(err); | ||
| } | ||
|
|
||
| // If using v1 path and it failed, always try fallback to v0 path. |
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.
when you are running a livekit dev server at your localhost, it will fail.
The error that I get is :
[INFO livekit_api::signal_client::signal_stream] connecting to ws://localhost:7880/rtc?sdk=rust&protocol=16&auto_subscribe=1&adaptive_stream=0&version=0.7.30&reconnect=1&sid=PA_8z9tP8yTwhtr
[INFO peer_connection_signaling_test::signaling_tests] [V1 (Single PC)] Reconnecting after node failure...
[ERROR livekit::rtc_engine] resuming connection failed: signal failure: ws failure: Connection closed normally
[ERROR livekit::rtc_engine] restarting connection... attempt: 1
[INFO livekit_api::signal_client::signal_stream] connecting to ws://localhost:7880/rtc/v1?join_request=EhkKEwgIEgYwLjcuMzAYECIFbWFjb3MSAggB
[WARN livekit_api::signal_client] signal connection failed on v1 path: WsError(Http(Response { status: 404, version: HTTP/1.1, headers: {"content-type": "text/plain; charset=utf-8", "vary": "Origin", "x-content-type-options": "nosniff", "date": "Wed, 11 Feb 2026 21:38:09 GMT", "content-length": "19"}, body: Some([52, 48, 52, 32, 112, 97, 103, 101, 32, 110, 111, 116, 32, 102, 111, 117, 110, 100, 10]) }))
[WARN livekit_api::signal_client] v1 path failed, falling back to v0 path
[INFO livekit_api::signal_client::signal_stream] connecting to ws://localhost:7880/rtc?sdk=rust&protocol=16&auto_subscribe=1&adaptive_stream=0&version=0.7.30
[INFO livekit_api::signal_client::signal_stream] connecting to ws://localhost:7880/rtc/v1?join_request=EhkKEwgIEgYwLjcuMzAYECIFbWFjb3MSAggB
[WARN livekit_api::signal_client] signal connection failed on v1 path: WsError(Http(Response { status: 404, version: HTTP/1.1, headers: {"content-type": "text/plain; charset=utf-8", "vary": "Origin", "x-content-type-options": "nosniff", "date": "Wed, 11 Feb 2026 21:38:09 GMT", "content-length": "19"}, body: Some([52, 48, 52, 32, 112, 97, 103, 101, 32, 110, 111, 116, 32, 102, 111, 117, 110, 100, 10]) }))
[WARN livekit_api::signal_client] v1 path failed, falling back to v0 path
[INFO livekit_api::signal_client::signal_stream] connecting to ws://localhost:7880/rtc?sdk=rust&protocol=16&auto_subscribe=1&adaptive_stream=0&version=0.7.30
[INFO peer_connection_signaling_test::signaling_tests] [V1 (Single PC)] Connected! Room: "test_SinglePC_64069a7c-c66a-46ca-b3db-7d73a81051b8"
| }; | ||
|
|
||
| // Create ClientInfo | ||
| let client_info = proto::ClientInfo { |
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.
Done
livekit/src/room/mod.rs
Outdated
| @@ -1059,6 +1066,35 @@ impl RoomSession { | |||
| track_id = stream_id.into(); | |||
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.
Done
livekit/src/room/mod.rs
Outdated
| return; | ||
| } | ||
| // In dual PC mode: answer is local, offer is remote | ||
| (remote_desc.unwrap(), Some(local_desc.unwrap())) |
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.
Good point, I actually don't know if it is possible, but I think it is dangerous to do unwrap() here, let me address it.
| mid_to_track_id: Default::default(), | ||
| })) | ||
| .await; | ||
| if self.single_pc_mode { |
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.
This path should be unnecessary. There should not be an offer on publisher PC from server. In single PC mode, client always offers and server answers.
after setting the LIVEKIT_URL, LIVEKIT_API_KEY, LIVEKIT_API_SECRET to staging endpoint, you will see things like
run tests:
cargo test -p livekit --features "__lk-e2e-test,native-tls" -- --nocapture
[INFO audio_test] Testing with 48000Hz, 1ch. -> 48000Hz, 1ch.
[INFO livekit_api::signal_client::signal_stream] connecting to wss://xianstaging-hixkk74p.staging.livekit.cloud/rtc/v1?sdk=rust&protocol=16&auto_subscribe=1&adaptive_stream=0&version=0.7.30&join_request=EhsKFQgIEgYwLjcuMzAYECIFbWFjb3NgARICCAE%3D
[INFO livekit_api::signal_client::signal_stream] connecting to wss://xianstaging-hixkk74p.staging.livekit.cloud/rtc/v1?sdk=rust&protocol=16&auto_subscribe=1&adaptive_stream=0&version=0.7.30&join_request=EhsKFQgIEgYwLjcuMzAYECIFbWFjb3NgARICCAE%3D
[INFO livekit_api::signal_client] [CONNECT] SUCCESS: v1 path connection successful, single_pc_mode=true
[ERROR livekit_api::signal_client] [CONNECT] ====== CONNECTION ESTABLISHED ======