Skip to content

Single Peer Connection#919

Open
pblazej wants to merge 29 commits intomainfrom
blaze/single-pc
Open

Single Peer Connection#919
pblazej wants to merge 29 commits intomainfrom
blaze/single-pc

Conversation

@pblazej
Copy link
Contributor

@pblazej pblazej commented Feb 17, 2026

  • Type-safe transport management via TransportMode (mirrors JS naming) 🎉
    • This is not purely additive, so requires some regression testing 💣
  • Benchmarks will come in a separate PR (once we have the methodology)
  • There's no separate test for this, just switching the default behavior
    • This is debatable, but the question is what to duplicate here... (which cases)
    • I think JS does the exact opposite
    • Rust creates separate (basic) cases

@github-actions
Copy link

github-actions bot commented Feb 17, 2026

⚠️ This PR does not contain any files in the .changes directory.

@pblazej pblazej marked this pull request as ready for review February 18, 2026 14:44
@xianshijing-lk
Copy link
Contributor

can you port these single / dual PC rust tests over to swift ?
https://github.com/livekit/rust-sdks/blob/c0b0868cef64a5d705d7f91def1d99f715730aba/livekit/tests/peer_connection_signaling_test.rs

They should be able to catch most of the issues I ran into when working on the single peer connection issues on Rust

pblazej and others added 3 commits February 27, 2026 13:45
Port V0 (dual PC) and V1 (single PC) signaling integration tests from
rust-sdks peer_connection_signaling_test.rs. Adds 17 tests covering
connect, two-participant discovery, audio track pub/sub, reconnect,
data channels, node failure recovery, many-tracks publish, and double
reconnect for both signaling modes.

- Add PeerConnectionSignalingTests with SignalingMode enum and
  ReconnectWatcher helper
- Parameterize singlePeerConnection in RoomTestingOptions
- Extract shared TestAudioTrack to LiveKitTestSupport/Tracks.swift
- Consolidate duplicate TestTrack from TrackTests.swift

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Verify 50 audio frames received (not just first frame) in audio track
  tests, matching Rust's NativeAudioStream frame count validation
- Add subscriber-side track verification after reconnect, confirming
  remote participant still sees publisher's tracks post-reconnect
- Add frame counting and expect(minimumFrames:) to AudioTrackWatcher

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@pblazej
Copy link
Contributor Author

pblazej commented Feb 27, 2026

rust tests

Done, there's some duplication with existing tests, plus I left true as the default pc mode for other tests (no hard feelings here).

The reason I did not add them is most of these scenarios are covered somewhere e.g. in data channel tests (reconnect, etc.).

@pblazej
Copy link
Contributor Author

pblazej commented Mar 2, 2026

Getting some timeouts in tests, looks like general problem with the runner.

@xianshijing-lk
Copy link
Contributor

Curiously, do you need the same DSP munging in Swift SDK like in Rust SDK ?

Munge SDP to change a=inactive to a=recvonly for RTP media m-lines in single PC mode.
This is needed because WebRTC can generate inactive direction even when transceivers
were configured as recvonly.

@pblazej
Copy link
Contributor Author

pblazej commented Mar 2, 2026

DSP munging

Yes, seems like it's necessary for the same reasons ⬇️

pblazej and others added 2 commits March 3, 2026 10:06
WebRTC's sdp_offer_answer.cc can generate a=inactive instead of
a=recvonly for recvOnly transceivers during renegotiation in single
peer connection mode, breaking video track subscription. This matches
the fix applied in the Rust SDK (commit 2827707f).

- Add singlePCMode flag to Transport
- Add mungeInactiveToRecvOnlyForMedia() to rewrite a=inactive in RTP
  m-sections while preserving data channel sections
- Apply munging in _negotiateSequence() before setLocalDescription
- Add 3 unit tests for SDP munging

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@pblazej
Copy link
Contributor Author

pblazej commented Mar 3, 2026

There's some flakiness in these stress tests - not sure if caused by invalid server state or something on the client yet.

@pblazej pblazej force-pushed the blaze/single-pc branch 9 times, most recently from 39340bc to f608281 Compare March 6, 2026 09:55
- Retry room connect (3 attempts, 2s delay) to handle transient POSIX 57
- Graceful track cleanup: unpublishAll + disconnect in withRooms teardown
- Reduce post-disconnect sleep from 5s to 1s
- Set server departure_timeout to 1s for faster room cleanup
- Use didPublishTrack delegate to fix fullReconnect race condition
- Replace nodeFailure with fullReconnect in signaling tests
- Update macos-26 to Xcode 26.3 and 26.2 simulators

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made some global changes here to reduce test flakiness, key findings:

  • publishManyTracks can poison other tests as the server is unable to listen for socket connections
  • it does not look like a bug in the server code (listener is a separate goroutine), but rather a restricted VM environment (3 cores here)
  • it works reasonably well for fewer tracks
  • we can consider moving to -xlarge runners for other "stress" tests, however it's paid

@pblazej
Copy link
Contributor Author

pblazej commented Mar 12, 2026

@lukasIO @xianshijing-lk do you see any gaps in the logic? You've got the full context.

@hiroshihorie is the swift side acceptable?

Copy link
Contributor

@xianshijing-lk xianshijing-lk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, two nits.

delegate: self)
let isSinglePC = singlePeerConnection
let isSubscriberPrimary = isSinglePC ? false : joinResponse.subscriberPrimary
log("subscriberPrimary: \(isSubscriberPrimary), singlePeerConnection: \(isSinglePC)")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the log intentional ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's .debug and it existed before: log("subscriberPrimary: \(joinResponse.subscriberPrimary)")

let publisher = try Transport(config: rtcConfiguration,
target: .publisher,
primary: !isSubscriberPrimary,
primary: isSinglePC || !isSubscriberPrimary,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

curiously, what does primary mean here ? why primary is true when isSinglePC is true ?

Copy link
Contributor Author

@pblazej pblazej Mar 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it maps the inverse relationship between the TransportMode and Transport, mostly in Room+TransportDelegate (transport wouldn't know if it's "the only one" in single PC mode):

    func transport(_ transport: Transport, didUpdateState pcState: LKRTCPeerConnectionState) {
        log("target: \(transport.target), connectionState: \(pcState.description)")

        // primary connected
        if transport.isPrimary {
            if pcState.isConnected {
                primaryTransportConnectedCompleter.resume(returning: ())

@xianshijing-lk
Copy link
Contributor

@lukasIO @xianshijing-lk do you see any gaps in the logic? You've got the full context.

@hiroshihorie is the swift side acceptable?

@pblazej , can you check if things are working with auto_subscribe is set to false ?
We just found out some bugs there in Rust.

pblazej added 2 commits March 19, 2026 15:06
# Conflicts:
#	Sources/LiveKit/Protos/livekit_metrics.pb.swift
@pblazej
Copy link
Contributor Author

pblazej commented Mar 19, 2026

We just found out some bugs there in Rust.

@xianshijing-lk I think there's some regression here vs main: after unsubscribing a video track I'm left with 8x8 "empty" video - is it confirmed to be on the SFU side?

In single PC mode the receive transceiver is reused (direction changes)
rather than removed, so peerConnection(didRemove:) never fires and the
VideoView is left with a stale track producing 8x8 empty frames.
Clear the track immediately like the Rust SDK does.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@pblazej
Copy link
Contributor Author

pblazej commented Mar 20, 2026

The above issue with (un)subscribe seems to be tangentially related to transceiver management again, as we were listening to a wrong signal: b64d296

Now it mimics Rust (see TODO) https://github.com/livekit/rust-sdks/blob/56669aecf4bbd1875889b7af4612cf37e86ed364/livekit/src/room/publication/remote.rs#L251

cc @xianshijing-lk anything else reproducible in Rust?

@pblazej pblazej requested a review from xianshijing-lk March 20, 2026 08:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants