Repository navigation
Create data channels as negotiated in single PC mode (protocol 18) #2160
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| 'livekit-client': minor | ||
| --- | ||
|
|
||
| Create the publisher data channels as negotiated (fixed IDs) in single peer connection mode when the server supports it (protocol 18), saving the data channel open round trip. Falls back to in-band channels against older servers. |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -126,6 +126,42 @@ describe('DataChannelManager', () => { | |
| manager.lossy.stopThresholdTuning(); | ||
| }); | ||
|
|
||
| it('creates negotiated channels with fixed ids and can fall back to in-band ones', () => { | ||
| const { manager, pcManager } = makeManager(); | ||
| const createSpy = vi.mocked(pcManager.createPublisherDataChannel); | ||
|
|
||
| manager.createPublisherChannels(pcManager, true); | ||
|
|
||
| expect(manager.publisherChannelsNegotiated).toBe(true); | ||
| expect(createSpy).toHaveBeenCalledWith('_reliable', { | ||
| ordered: true, | ||
| negotiated: true, | ||
| id: 100, | ||
| }); | ||
| expect(createSpy).toHaveBeenCalledWith('_lossy', { | ||
| ordered: false, | ||
| maxRetransmits: 0, | ||
| negotiated: true, | ||
| id: 101, | ||
| }); | ||
| expect(createSpy).toHaveBeenCalledWith('_data_track', { | ||
| ordered: false, | ||
| maxRetransmits: 0, | ||
| negotiated: true, | ||
| id: 102, | ||
| }); | ||
|
Comment on lines
+136
to
+152
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nitpick: Is there value in including the literal numbers like |
||
|
|
||
| createSpy.mockClear(); | ||
| manager.createPublisherChannels(pcManager, false); | ||
|
|
||
| expect(manager.publisherChannelsNegotiated).toBe(false); | ||
| expect(createSpy).toHaveBeenCalledWith('_reliable', { ordered: true }); | ||
| manager.teardown(); | ||
| expect(manager.publisherChannelsNegotiated).toBe(false); | ||
|
|
||
| manager.lossy.stopThresholdTuning(); | ||
| }); | ||
|
|
||
| it('adopts subscriber channels by label and rejects unknown labels', () => { | ||
| const { manager, opts } = makeManager(); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,7 +1,11 @@ | ||||||||||||||||||||||||||||||
| import { version as v } from '../package.json'; | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| export const version = v; | ||||||||||||||||||||||||||||||
| export const protocolVersion = 17; | ||||||||||||||||||||||||||||||
| export const protocolVersion = 18; | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| /** First server protocol version that creates the single peer connection data channels as | ||||||||||||||||||||||||||||||
| * negotiated (fixed SCTP stream IDs) instead of opening them in-band. */ | ||||||||||||||||||||||||||||||
| export const PROTOCOL_NEGOTIATED_DATA_CHANNELS = 18; | ||||||||||||||||||||||||||||||
|
Comment on lines
3
to
+8
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nitpick: Use the constant in
Suggested change
|
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| /** Initial client protocol. */ | ||||||||||||||||||||||||||||||
| export const CLIENT_PROTOCOL_DEFAULT = 0; | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
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.
🔴 Publisher data stops after server downgrade
When a session resumes on an older server,
useNegotiatedDataChannelschanges its verdict but leaves the negotiated channels open. The older server never receives their DCEP OPEN, so published data stops reaching it.Learn more
A single-PC session with negotiated channels keeps its peer connection during signal resume. The resume response replaces latestJoinResponse.serverInfo, but channel recreation only runs for an open channel with a null ID in resumeConnection. Negotiated channels have fixed non-null IDs, so a resume onto a pre-18 server leaves them unchanged. That server does not use the fixed negotiated IDs and receives no in-band channel open message, while the client continues sending through the old channels.
Example: A client joins a protocol-18 server using negotiated ID 100 for reliable data. Its signaling session resumes on a protocol-17 server. The response updates the protocol to 17, but the client continues sending reliable packets on ID 100 instead of opening an in-band channel.
Recommended fix: On resume, compare the active publisher channel mode with the new server's protocol before completing reconnection. If it changed, replace the channels with the appropriate type and wait for the replacement to open; handle queued reliable messages and sync-state channel IDs during this transition.
Was this helpful? React with 👍 or 👎 to provide feedback.
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.
a session resume is not expected to land on an older version.