Bundle the publisher offer with the JoinRequest (offer-with-join) - #1111
Conversation
Benchmarked and validated against staging CloudE2EAgainst Connect-time benchmark
~50 ms (−17%) off the transport phase, 291 → 242 versus single PC on Two things worth recording:
Reproducecd Benchmarks
LK_BENCHMARK=1 LIVEKIT_URL=wss://… LIVEKIT_API_KEY=… LIVEKIT_API_SECRET=… \
swiftly run +xcode swift package --disable-sandbox benchmark \
--filter "BM-CONN-003-SinglePC"
|
In single peer connection mode the publisher offer is now created before the signal socket opens and carried in the JoinRequest, so the server answers it in the same exchange. That removes a client<->server round trip from the connect path, and building the peer connection up front moves the WebRTC cold start (SSL init, peer connection factory, audio device module) off it as well -- it now overlaps the TLS/WebSocket handshake. setLocalDescription is deferred until the answer arrives: applying it starts ICE gathering, and at creation time the connection only has the client-side configuration, so it would gather without the server's TURN servers. JoinDependencies adopts the early publisher and installs the server's configuration onto it, which is what releases the deferred offer. The early publisher is owned lexically by the connect sequence rather than being a stage payload, so the stage invariant -- staged transports exist if and only if the stage is .connected -- still holds. It is closed on every exit before the JOIN is applied, including the v1 -> v0 fallback, where the legacy path needs a publisher with different immutable properties. Ports rtc_session.rs:511-549 and :703-710 from rust-sdks, and the equivalent path in client-sdk-js RTCEngine.ts:334-396. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
b8e31c3 to
f0a630d
Compare
| // A deferred initial offer counts as awaiting an answer even though the | ||
| // connection still reads `.stable`, so a publish racing the JOIN queues a | ||
| // renegotiation instead of offering over the top of it. | ||
| let isAwaitingAnswer = signalingState == .haveLocalOffer || _pendingInitialOffer != nil |
There was a problem hiding this comment.
If a server serves /rtc/v1 but ignores publisher_offer, no answer ever arrives and _pendingInitialOffer stays set forever, so this gate makes every later createAndSendOffer a no-op and the connect dies on the transport timeout rather than falling back — worth a capability gate or a timeout that drops the pending offer.
There was a problem hiding this comment.
Good point, though I think /rtc/v1 supports publisher_offer from its launch on the server side, and Rust has similar solution as in swift.
If we want to be extra safe, I can add a timeout that drops the pending offer and falls back. Please let me know if you think that is the right thing to do, and I can follow up on the Rust / JS side as well.
| /// both remote-description paths are safe. | ||
| private func applyPendingInitialOffer() async throws { | ||
| guard let pendingInitialOffer = _pendingInitialOffer else { return } | ||
| _pendingInitialOffer = nil |
There was a problem hiding this comment.
Clearing _pendingInitialOffer before the await means a throw from set(localDescription:) loses the offer, the answer and any queued renegotiation — and since didReceiveAnswer only logs the error, nothing recovers; nil-ing it after the await succeeds would keep a retry possible.
There was a problem hiding this comment.
Done, now applyPendingInitialOffer restores the offer on throw
There was a problem hiding this comment.
Updating this thread — my earlier reply said the offer is restored on throw, but 97b210c6 has since reversed that, and I agree with the reversal.
Restoring leaves isAwaitingAnswer true with no answer ever coming, so every later createAndSendOffer becomes a no-op and the publisher can never negotiate again. That is strictly worse than losing the in-flight negotiation, which the connect timeout and reconnect rebuild anyway. Current code drops it, matching take() in rust-sdks.
| _pendingInitialOffer = nil | ||
|
|
||
| log("Applying the initial offer deferred from JOIN") | ||
| try await set(localDescription: pendingInitialOffer) |
There was a problem hiding this comment.
didReceiveMediaSectionsRequirement and a publish racing the JOIN can both add transceivers before this line runs, so the deferred offer may no longer describe the transceiver set — the isAwaitingAnswer gate stops the racing offer but not the mutation.
There was a problem hiding this comment.
I added transceiverAddedWhileTheInitialOfferIsPending in OfferWithJoinTests.swift to investigate the race here.
The race is exactly as you describe: the gate suppresses the racing offer but not the transceiver addition, so the deferred offer can describe a stale set by the time it's applied. libwebrtc tolerates that: setLocalDescription succeeds, the added transceiver stays unassociated, and the queued renegotiation picks it up in the follow-up offer. Test asserts both halves so we notice if that ever stops being true.
Unless I miss some details here, there shouldn't be any production code that needs to change here.
|
|
||
| /// The publisher's three outbound data channels, created together so both the early and the | ||
| /// post-JOIN publisher paths negotiate the same layout. | ||
| struct PublisherDataChannels: Sendable { |
There was a problem hiding this comment.
Storing the three channels as raw proxies trips no_raw_webrtc_proxy_storage three times (the Lint job fails on --strict), and it also means the v1→v0 fallback drops their last reference on a cooperative-pool thread instead of through parkChannelRelease.
There was a problem hiding this comment.
Good points.
I addressed the comments with storing RTCBox?; install is @rtc.
| try await configureTransports(connectResponse: connectResponse, | ||
| singlePeerConnection: singlePC, | ||
| earlyPublisher: earlyPublisher) | ||
| isAdopted = true |
There was a problem hiding this comment.
Adoption only happens on the .join branch inside configureTransports, so setting isAdopted unconditionally here makes the catch skip earlyPublisher?.close() on a .reconnect response and the peer connection is dropped without close().
There was a problem hiding this comment.
Fixed in 7ed347ab — isAdopted is now set only when the response is .join, which is the branch that actually adopts:
if case .join = connectResponse { isAdopted = true }For the record I couldn't convince myself it was reachable: fullConnectSequence should always get a JOIN, and .reconnect is the resume path. So I treated it as cheap hardening rather than a live leak — if you know a path that reaches it, worth a test.
| // Built before the socket opens so its offer rides along with the JOIN request, | ||
| // removing the offer→answer round trip from the connect path, and so the WebRTC cold | ||
| // start overlaps the TLS/WebSocket handshake instead of following it. | ||
| var earlyPublisher: EarlyPublisher? = if singlePC { |
There was a problem hiding this comment.
quickReconnectSequence gates on await !signalClient.useV0SignalPath; doing the same here would avoid a full peer-connection-plus-three-data-channels cold start on every full-reconnect and region-failover attempt against a v0-only server.
There was a problem hiding this comment.
Done in 7ed347ab — gated on await !signalClient.useV0SignalPath, matching quickReconnectSequence.
I left singlePC itself untouched so v1 is still attempted (a region failover could land on a v1-capable node); only the cold start is skipped. With no early publisher the existing earlyPublisher?.offer != nil branch falls through to ordinary negotiation, so that path is already handled.
| // MARK: - Early publisher | ||
|
|
||
| /// A publisher peer connection built *before* the signal socket opens, so its offer can be | ||
| /// bundled with the JOIN request and the WebRTC cold start (SSL init, peer connection factory, |
There was a problem hiding this comment.
EarlyPublisher.make is awaited to completion before signalClient.connect (it has to be — the offer goes in the URL), so the cold start is serialized ahead of the handshake rather than overlapping it; the same wording is in the inline comment and in .changes/offer-with-join.
There was a problem hiding this comment.
Correct, and it was wrong in all three places. EarlyPublisher.make is awaited to completion before signalClient.connect — it has to be, the offer goes in the URL — so it is serialized ahead of the handshake, not overlapped with it. Fixed in 7ed347ab: the docstring, the inline comment in fullConnectSequence, and .changes/offer-with-join now claim only the saved round trip.
The benchmark itself still stands — 291 → 242 ms is one RTT, and the absent D_ICE_DTLS_MS independently confirms the mechanism is active. Only the second claimed mechanism was fiction. rust-sdks orders it the same way (create_initial_offer().await completes before SignalClient::connect), so there is nothing to restructure — though whether genuinely overlapping the cold start would recover more time is worth measuring separately.
Box the publisher data channels. Three raw `LKRTCDataChannel?` properties tripped `no_raw_webrtc_proxy_storage` (the Lint job was failing on all three), and they also meant the v1→v0 fallback dropped three proxies' last references — each a BlockingCall destructor — on the connect sequence's cooperative-pool thread. `RTCBox` parks the release from its own `deinit`, so no caller has to remember. `install` becomes `@RTC`, which also moves its `label`/`channelId` reads onto the RTC executor where they belong. Validate `offerId` before applying the deferred offer. Applying first meant an answer we were about to reject still consumed the offer and moved the connection to `.haveLocalOffer`. Restore the deferred offer if applying it throws. Clearing before the `await` keeps take-once across the suspension, but dropping it on failure lost the offer, the answer and any queued renegotiation at once, and `didReceiveAnswer` only logs. rust-sdks has the same hazard; this diverges deliberately. Set `isAdopted` only for a `.join` response. Adoption happens on that branch alone, so a `.reconnect` left the early publisher unowned and the catch skipped closing it. Skip the early publisher when the signal client already knows the server speaks only v0, matching `quickReconnectSequence`. Otherwise a peer connection plus three data channels is built and discarded on every full-reconnect and failover attempt. Correct the docs. `EarlyPublisher.make` is awaited to completion before `signalClient.connect` — the offer has to be in the JOIN URL — so the cold start is serialized ahead of the handshake, not overlapped with it. The measured win is the saved round trip alone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…he transceiver race `fullConnectSequence` went over `function_body_length` once the v0 gate and the `isAdopted` fix were added. Extract only the signal-connect-with-legacy-fallback block, which was already a self-contained step; the body is 46 lines with that alone. Blank lines and comments are excluded from the count, so reflowing cannot help — only removing statements does. Add a test for the transceiver added while the bundled offer is outstanding. `didReceiveMediaSectionsRequirement` adds recvonly sections and a publish adds a sender, and the `isAwaitingAnswer` gate suppresses the racing *offer* but not the mutation — so the deferred offer can describe a stale transceiver set by the time it is applied. libwebrtc tolerates exactly that: the stale offer applies, the added transceiver stays unassociated, and the queued renegotiation picks it up in the follow-up offer. No production change is needed; the test exists because that behaviour is an undocumented libwebrtc property nothing else asserts. Note the fix that looks obvious here is unsafe: applying the deferred offer earlier (after `set(configuration:)`, before `resumeQueues()`) would gather ICE while the request queue is suspended, and `.trickle` is non-queueable, so every candidate in that window is dropped rather than queued. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
348ecfc to
290c11e
Compare
`swiftlint --strict` rejects the three-member tuple as `large_tuple`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
f598387 to
e823a46
Compare
Restoring it leaves `isAwaitingAnswer` true with no answer coming, so every later `createAndSendOffer` becomes a no-op and the publisher can never negotiate again. The answer is discarded either way — `didReceiveAnswer` only logs — so keeping the offer buys nothing and costs recovery. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
pblazej
left a comment
There was a problem hiding this comment.
LGTM, fixed the lint + devin combo.
Let's merge it after the release 🎉
…th-the-joinrequest-offer-with-join
Fixes CLT-2202.
Swift performed one more client↔server round trip on connect than JS or Rust, and serialized the WebRTC cold start into the middle of the connect sequence. A customer reports iOS/Android p50 connect of 800 ms in Asia vs 400 ms in US; each removed round trip is worth proportionally more on high-RTT paths.
Livekit_JoinRequest.publisherOfferexisted in the protos but was never populated. Swift waited for the join response, then created the peer connections, then negotiated. Both other SDKs create the PC and initial offer before opening the socket and bundle the offer into the join request (rust-sdks/livekit/src/rtc_engine/rtc_session.rs:511-549and:703-710;client-sdk-js/src/room/RTCEngine.ts:334-396).What changed
EarlyPublisher(new,RoomDependencies.swift) — builds the publisher transport, its three data channels, and the initial offer beforesignalClient.connect. Only in single PC mode, where the publisher is primary, so itsprimary/singlePCModevalues are known without the join response.Deferred
setLocalDescription(Transport.createInitialOffer()) — the offer is produced and signalled but not applied. Applying it starts ICE gathering, and at that point the connection only has the client-side configuration, so it would gather without the server's TURN servers and never produce relay candidates.JoinDependencies.makeinstalls the server configuration onto the adopted transport, andset(remoteDescription:)applies the pending offer when the answer lands. Same mechanism aspendingInitialOfferin JS andpending_initial_offerin Rust.JoinDependencies.make(…, earlyPublisher:)— adopts the early publisher instead of rebuilding it, or creates one as before when there is none. Data channel creation moved into a sharedPublisherDataChannelsso both paths negotiate the same layout.Skipping the eager negotiate — when the offer was bundled,
fullConnectSequencemarkshasPublishedand does not callpublisherShouldNegotiate, matching Rust'ssent_publisher_offerbranch.createAndSendOfferalso treats a pending initial offer as "awaiting an answer", so a publish racing the JOIN queues a renegotiation instead of offering over the top of it.Ownership — the early publisher is a local in the connect sequence, deliberately not a stage payload, so the documented stage invariant (staged transports exist iff the stage is
.connected) still holds. It is closed on every exit before the JOIN is applied, including the v1→v0 fallback, where the legacy path needs a publisher with different immutable properties and so cannot reuse it.Verified against a real server
Ran the E2E signaling suite against
livekit-server 1.13.1locally — all 9 tests pass in both dual-PC and single-PC modes (connect, two participants, audio track, data channel, quick reconnect, full reconnect, double reconnect, publish-many-tracks, v1→v0 fallback).Server logs confirm the mechanism actually engages rather than silently falling back. For a single-PC session:
"PublisherOffer": {"type": "offer"...}received offermessages over the signal channel for that connectionA dual-PC session in the same run shows the opposite — an empty
PublisherOfferand a separatereceived offerwithofferId: 1. So the round trip is genuinely removed, not duplicated.Unit tests
New
OfferWithJoinTests(6 tests) using two local peer connections, one standing in for the SFU:offerId == 1and a non-empty SDP whilelocalDescriptionstaysniland signaling state stays.stable— then applying the answer setslocalDescriptionjoin_requestparameter; absent when there is noneNotes for review
Server compatibility is the main thing to confirm beyond localhost. If a server serves
/rtc/v1but ignorespublisherOffer, no answer ever arrives, ICE never starts, and the connect fails on the transport timeout. Rust has no guard for this either (JS gates only on a browser capability), andpublisherOffershipped with the v1 path — but it's worth a check against staging/production Cloud before enabling single-PC by default (CLT ticket 4).Munge fallback does not apply to this path.
set(localDescription:munging:)normally drops a munge libwebrtc rejects and retries; a bundled offer can't, since the peer has already been told what we offered. Both munges here (mungeInactiveToRecvOnlyForMedia,mungeOpusStereoForAllAudio) are the ones every single-PC offer already carries, so this isn't a new risk class, but it is a behavior difference worth a look. Documented oncreateInitialOffer().Synergy with Fix join_request encoding and add gzip compression to the v1 signal URL #1110: real join requests now carry an SDP, so they are large enough that the gzip added in Fix join_request encoding and add gzip compression to the v1 signal URL #1110 engages — ~3032 B → ~592 B of URL for a 4-section offer, keeping the upgrade request inside one TCP segment.
Pre-existing failures (not from this change)
DataTrackPublishTests.publishWithFrameMetadata()anddefineAndGetSchema()fail locally. I verified they fail identically onorigin/mainwith none of these changes — locallivekit-server 1.13.1appears not to support data-track schema metadata.CooperativePoolBlockingTestspasses in isolation; it only failed as collateral when a screen-share E2E test hung the full-suite run on missing screen-recording permission.Builds verified on macOS, Mac Catalyst and iOS Simulator.
swiftlintandswiftformat --lintclean. No public API change.🤖 Generated with Claude Code