Update protocol to v15 #394

nbsp · 2024-08-16T20:09:16Z

exported as LocalTrackSubscribed in the client and FFI.

at the moment the SFU assumes Rust doesn't accept TrackSubscribed so it doesn't send it: see livekit/livekit#2939

davidzhao · 2024-08-16T21:30:59Z

Currently Rust stack is on protocol version 9. part of this update will require bringing it to 10+. and I think we should just bring it up to date to support protocol 15.

Here are the changes required (JS PRs)

exported as LocalTrackSubscribed in the client and FFI.

nbsp · 2024-08-16T23:18:41Z

v10 through v13 added or already there. re: RequestResponse: we don't keep a list of pending requests and we don't care about arbitrary responses at all -- most of our signal_client requests go to _. should i add it regardless and make it no-op?

davidzhao · 2024-08-16T23:32:14Z

v10 through v13 added or already there. re: RequestResponse: we don't keep a list of pending requests and we don't care about arbitrary responses at all -- most of our signal_client requests go to _. should i add it regardless and make it no-op?

yeah no-op is fine. we can add the capability later of firing error responses for things, but not required to be protocol-compatible.

davidzhao

nice work! Just a couple of things:

see inline about handling action
the streamID change is a bit tricky and we would need to test e2e to be sure. the way to test is creating a room with sync-stream enabled:

lk room create --name <myroom> --sync-streams
then starting a publisher immediately: lk load-test --room <myroom> --publishers 1 --audio-publishers 1
then join the room with Rust and ensure both streams are retrieved correctly

davidzhao · 2024-08-17T00:05:11Z

livekit/src/rtc_engine/rtc_session.rs

@@ -778,8 +798,8 @@ impl SessionInner {
    async fn simulate_scenario(&self, scenario: SimulateScenario) -> EngineResult<()> {
        let simulate_leave = || {
            self.on_signal_event(proto::signal_response::Message::Leave(proto::LeaveRequest {
+                action: proto::leave_request::Action::Reconnect.into(),


it doesn't look like this is handled? this would determine if a client should reconnect vs disconnect for good.

from what i've seen rust-sdks reconnection logic is fundamentally different, and doesn't use LeaveRequest for retrying at all? it attempts to restart the publisher, or completely closing it (with Action::Disconnect) then reconnecting and recovering? i'll see if i can make it consistent with JS

I think the key here is if server is not sending a reconnect action, Rust should not attempt to reconnect. CC: @theomonnom

also can confirm streams are received and handled correctly

livekit/src/rtc_engine/rtc_session.rs

livekit/src/room/mod.rs

theomonnom

otherwise lgtm

theomonnom

nvm, seems like it is still not returning an Error when receiving a bad RequestResponse.

(e.g update_metadata, ..)

livekit-api/src/signal_client/mod.rs

livekit/src/rtc_engine/rtc_session.rs

theomonnom · 2024-08-20T18:07:23Z

livekit/src/room/mod.rs

@@ -69,6 +69,8 @@ pub enum RoomError {
    TrackAlreadyPublished,
    #[error("already closed")]
    AlreadyClosed,
+    #[error("request error: {}", .0.message)]
+    Request(proto::RequestResponse),


Can you recreate another struct, basically having the same fields of the RequestResponse but removing the internal fields like the request_id

theomonnom · 2024-08-20T18:07:40Z

livekit/src/room/participant/local_participant.rs

+                _ => Err(RoomError::Request(response)),
+            }
+        } else {
+            log::error!("could not receive response for set_metadata request");


Logs aren't useful if we're already returning an error

theomonnom · 2024-08-20T18:09:28Z

livekit/src/room/participant/local_participant.rs

-            .await;
-        Ok(())
+        if let Ok(response) = timeout(REQUEST_TIMEOUT, {
+            let request_id = self.inner.rtc_engine.session().signal_client().request_id();


~~I think this should be moved to the Room impl? request_id doesn't necessarily needs to be on the signal client?~~
Also making it more explicit like "new_request_id"?

the JS SDK puts it in the signal client, but it's a bit more cumbersome here, yeah
we can maybe move it to RtcEngine?

theomonnom

lgtm otherwise

nbsp requested review from keepingitneil and theomonnom August 16, 2024 20:09

nbsp mentioned this pull request Aug 16, 2024

Remove check for rust on TrackSubscribed livekit/livekit#2939

Closed

Add support for TrackSubscribed

f557ec7

exported as LocalTrackSubscribed in the client and FFI.

nbsp force-pushed the nbsp/add/tracksubscribe branch from 9d7ba47 to 739c00e Compare August 16, 2024 23:16

nbsp added 3 commits August 16, 2024 16:47

Support sync stream id

f0c1781

use leave_request::Action

0c5eac0

support RequestResponse

dd5fe53

nbsp force-pushed the nbsp/add/tracksubscribe branch from 739c00e to dd5fe53 Compare August 16, 2024 23:47

generated protobuf

4a28dd2

davidzhao reviewed Aug 17, 2024

View reviewed changes

nbsp force-pushed the nbsp/add/tracksubscribe branch from d4349fc to df2392c Compare August 19, 2024 19:23

use leave_request.Action internally

ad4baad

nbsp force-pushed the nbsp/add/tracksubscribe branch from df2392c to ad4baad Compare August 19, 2024 21:34

nbsp requested a review from davidzhao August 19, 2024 21:34

theomonnom reviewed Aug 19, 2024

View reviewed changes

livekit/src/rtc_engine/rtc_session.rs Outdated Show resolved Hide resolved

TrackSubscribed -> LocalTrackSubscribed everywhere

a504aad

theomonnom reviewed Aug 19, 2024

View reviewed changes

livekit/src/room/mod.rs Show resolved Hide resolved

theomonnom approved these changes Aug 19, 2024

View reviewed changes

theomonnom requested changes Aug 19, 2024

View reviewed changes

nbsp added 2 commits August 19, 2024 16:23

draft: handle RequestResponse as RoomEvent

c41428d

rust confuses me

a427d1a

nbsp requested a review from theomonnom August 20, 2024 00:10

theomonnom reviewed Aug 20, 2024

View reviewed changes

livekit-api/src/signal_client/mod.rs Outdated Show resolved Hide resolved

nbsp requested a review from theomonnom August 20, 2024 03:31

use AtomicU32

00583a8

nbsp force-pushed the nbsp/add/tracksubscribe branch from 0c7c83a to 00583a8 Compare August 20, 2024 05:45

lukasIO reviewed Aug 20, 2024

View reviewed changes

livekit/src/rtc_engine/rtc_session.rs Show resolved Hide resolved

time out set_metadata/name/attributes requests

df60ece

nbsp requested a review from lukasIO August 20, 2024 16:41

fix request_id not working

b9644e8

nbsp mentioned this pull request Aug 20, 2024

bump(ffi): LocalTrackSubscribed, DisconnectReason livekit/node-sdks#256

Merged

theomonnom approved these changes Aug 20, 2024

View reviewed changes

nbsp mentioned this pull request Aug 20, 2024

bump(ffi): LocalTrackSubscribed, DisconnectReason livekit/python-sdks#236

Merged

theomonnom reviewed Aug 20, 2024

View reviewed changes

address feedback

498d3f5

theomonnom approved these changes Aug 20, 2024

View reviewed changes

nbsp added 2 commits August 20, 2024 11:44

next_request_id

c8a23b5

fmt

fc9f896

nbsp changed the title ~~Add support for TrackSubscribed~~ Update protocol to v15 Aug 20, 2024

nbsp merged commit 1abf092 into main Aug 20, 2024
11 of 17 checks passed

nbsp deleted the nbsp/add/tracksubscribe branch August 20, 2024 20:02

theomonnom mentioned this pull request Sep 23, 2024

fix: resume/reconnection not working! #440

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update protocol to v15 #394

Update protocol to v15 #394

nbsp commented Aug 16, 2024 •

edited

Loading

davidzhao commented Aug 16, 2024

nbsp commented Aug 16, 2024

davidzhao commented Aug 16, 2024

davidzhao left a comment

davidzhao Aug 17, 2024

nbsp Aug 17, 2024 •

edited

Loading

davidzhao Aug 17, 2024

nbsp Aug 17, 2024

theomonnom left a comment

theomonnom left a comment •

edited

Loading

theomonnom Aug 20, 2024

theomonnom Aug 20, 2024

theomonnom Aug 20, 2024 •

edited

Loading

nbsp Aug 20, 2024

theomonnom left a comment

Update protocol to v15 #394

Update protocol to v15 #394

Conversation

nbsp commented Aug 16, 2024 • edited Loading

davidzhao commented Aug 16, 2024

nbsp commented Aug 16, 2024

davidzhao commented Aug 16, 2024

davidzhao left a comment

Choose a reason for hiding this comment

davidzhao Aug 17, 2024

Choose a reason for hiding this comment

nbsp Aug 17, 2024 • edited Loading

Choose a reason for hiding this comment

davidzhao Aug 17, 2024

Choose a reason for hiding this comment

nbsp Aug 17, 2024

Choose a reason for hiding this comment

theomonnom left a comment

Choose a reason for hiding this comment

theomonnom left a comment • edited Loading

Choose a reason for hiding this comment

theomonnom Aug 20, 2024

Choose a reason for hiding this comment

theomonnom Aug 20, 2024

Choose a reason for hiding this comment

theomonnom Aug 20, 2024 • edited Loading

Choose a reason for hiding this comment

nbsp Aug 20, 2024

Choose a reason for hiding this comment

theomonnom left a comment

Choose a reason for hiding this comment

nbsp commented Aug 16, 2024 •

edited

Loading

nbsp Aug 17, 2024 •

edited

Loading

theomonnom left a comment •

edited

Loading

theomonnom Aug 20, 2024 •

edited

Loading