From 3d5615d6fa54c9c9e3ca5a7178cb3c0c970e37e7 Mon Sep 17 00:00:00 2001 From: shijing xian Date: Mon, 14 Sep 2026 09:52:23 -0700 Subject: [PATCH 1/3] Write the video start bitrate hint as one connection-level value, once x-google-start-bitrate is connection-scoped in libwebrtc: ApplyChangedParams reads it per m-section but pushes it into the shared Call via SetSdpBitrateParameters, where RtpBitrateConfigurator holds one config for the whole peer connection. Differing per-section values were last-writer-wins on m-section order, so a camera plus a screen share could seed the estimator from either one depending on SDP layout. Every video section now carries the same value: the largest hint among the sections that map to a published track. Write it only on the first offer that carries local video. libwebrtc retains start_bitrate_bps and re-applies it on network route changes, so rewriting it later is at best a no-op and at worst a restart of a converged bandwidth estimator. The latch is set only once the offer carrying the hint is accepted locally, so a rejected munge retries on the next offer. Add a 300 kbps target floor, matching the Rust SDK: below that, seeding above the real capacity costs more than the ramp it saves. applyVideoStartBitrate no longer matches the section to a track; that moves to findTrackCodecPayload so the dependent DD extension munging still runs on every offer. Co-Authored-By: Claude Opus 5 (1M context) --- .../video-start-bitrate-connection-level.md | 11 ++ src/room/PCTransport.test.ts | 68 +++++++- src/room/PCTransport.ts | 164 ++++++++++++++---- 3 files changed, 200 insertions(+), 43 deletions(-) create mode 100644 .changeset/video-start-bitrate-connection-level.md diff --git a/.changeset/video-start-bitrate-connection-level.md b/.changeset/video-start-bitrate-connection-level.md new file mode 100644 index 0000000000..af5807e0c2 --- /dev/null +++ b/.changeset/video-start-bitrate-connection-level.md @@ -0,0 +1,11 @@ +--- +'livekit-client': patch +--- + +Write the `x-google-start-bitrate` hint as a single connection-level value, once per publisher connection. + +libwebrtc reads this fmtp parameter per m-section but applies it to the shared `Call` (`WebRtcVideoSendChannel::ApplyChangedParams` → `SetSdpBitrateParameters`), where `RtpBitrateConfigurator` holds one config for the whole peer connection. Differing per-section values were therefore last-writer-wins on m-section order, so publishing a camera and a screen share together could seed the estimator from either one depending on SDP layout. Every video section now carries the same value: the largest hint among the sections that map to a published track. + +The hint is also written only on the first offer that carries local video, instead of on every offer. libwebrtc retains `start_bitrate_bps` and re-applies it on network route changes (`RtpTransportControllerSend::OnNetworkRouteChanged`), so rewriting it later is at best a no-op and at worst restarts a converged bandwidth estimator. A full reconnect builds a new peer connection and seeds the new estimator again. + +Targets below 300 kbps now get no hint, matching the Rust SDK: below that, seeding above the real capacity costs more than the ramp it saves. diff --git a/src/room/PCTransport.test.ts b/src/room/PCTransport.test.ts index af12f5dbdc..334bdb03e9 100644 --- a/src/room/PCTransport.test.ts +++ b/src/room/PCTransport.test.ts @@ -2,10 +2,13 @@ import { type MediaDescription, parse } from 'sdp-transform'; import { describe, expect, it } from 'vitest'; import { applyVideoStartBitrate, + computeConnectionStartBitrate, + computeTrackStartBitrate, conformBundledCodecFmtp, ensureAudioNackAndStereo, ensureVideoDDExtension, extractStereoAndNackAudioFromOffer, + findTrackCodecPayload, fmtpConfigHasParam, placeholderMidsFromTransceivers, } from './PCTransport'; @@ -61,9 +64,7 @@ a=recvonly a=rtpmap:49 H265/90000 a=fmtp:49 level-id=180;profile-id=1;tier-flag=0;tx-mode=SRST`; -describe('video start bitrate', () => { - it('applies the bitrate only to the section whose msid track ID matches the cid', () => { - const { media } = parse(`v=0 +const TWO_VIDEO_SECTIONS = `v=0 o=- 0 0 IN IP4 127.0.0.1 s=- t=0 0 @@ -79,15 +80,68 @@ c=IN IP4 0.0.0.0 a=mid:1 a=sendonly a=msid:PA_remote|camera camera-cid -a=rtpmap:96 VP8/90000`); +a=rtpmap:96 VP8/90000`; + +describe('video start bitrate', () => { + it('matches only the section whose msid track ID matches the cid', () => { + const { media } = parse(TWO_VIDEO_SECTIONS); + + expect(findTrackCodecPayload(media[0], 'camera-cid', 'VP8')).toBeUndefined(); + expect(findTrackCodecPayload(media[1], 'camera-cid', 'VP8')).toBe(96); + // Section belongs to the track but does not offer the codec. + expect(findTrackCodecPayload(media[1], 'camera-cid', 'AV1')).toBe(0); + }); + + it('applies the bitrate only to the section it is given', () => { + const { media } = parse(TWO_VIDEO_SECTIONS); - for (const section of media) { - applyVideoStartBitrate(section, 'camera-cid', 'VP8', 1_000); - } + applyVideoStartBitrate(media[1], 96, 900); expect(fmtpOf(media, '0', 96)).toBeUndefined(); expect(paramSet(fmtpOf(media, '1', 96)!)).toContain('x-google-start-bitrate=900'); }); + + it('caps camera at 1 Mbps but leaves screen share uncapped', () => { + const camera = { cid: 'c', codec: 'VP8', maxbr: 3_000 }; + const screenShare = { ...camera, isScreenShare: true }; + + expect(computeTrackStartBitrate(camera)).toBe(1_000); + expect(computeTrackStartBitrate(screenShare)).toBe(2_700); + }); + + it('gives no hint below the 300 kbps target floor', () => { + expect(computeTrackStartBitrate({ cid: 'c', codec: 'VP8', maxbr: 299 })).toBeUndefined(); + expect(computeTrackStartBitrate({ cid: 'c', codec: 'VP8', maxbr: 300 })).toBe(270); + }); + + it('uses one connection-level value: the largest hint across video sections', () => { + const { media } = parse(TWO_VIDEO_SECTIONS); + + const startBitrate = computeConnectionStartBitrate(media, [ + { cid: 'camera-cid', codec: 'VP8', maxbr: 1_000 }, + { cid: 'other-track', codec: 'VP8', maxbr: 3_000, isScreenShare: true }, + ]); + + expect(startBitrate).toBe(2_700); + }); + + it('ignores registered tracks with no section in the current SDP', () => { + const { media } = parse(TWO_VIDEO_SECTIONS); + + const startBitrate = computeConnectionStartBitrate(media, [ + { cid: 'camera-cid', codec: 'VP8', maxbr: 1_000 }, + // Stale entry: trackBitrates is append-only and outlives an unpublish. + { cid: 'unpublished-cid', codec: 'VP8', maxbr: 8_000, isScreenShare: true }, + ]); + + expect(startBitrate).toBe(900); + }); + + it('gives no connection value when no section maps to a published track', () => { + const { media } = parse(TWO_VIDEO_SECTIONS); + + expect(computeConnectionStartBitrate(media, [])).toBeUndefined(); + }); }); describe('placeholderMidsFromTransceivers', () => { diff --git a/src/room/PCTransport.ts b/src/room/PCTransport.ts index 6983a35486..54afd416bf 100644 --- a/src/room/PCTransport.ts +++ b/src/room/PCTransport.ts @@ -34,51 +34,118 @@ const startBitrateMultiplier = 0.9; /** Maximum x-google-start-bitrate in kbps. 1 Mbps prevents BWE from starting too aggressively. */ const maxStartBitrateKbps = 1000; +/** + * Minimum target bitrate in kbps for the start bitrate hint. Below this, seeding above the + * real capacity costs more than the ramp it saves, so libwebrtc's default is left in place. + */ +const minTargetBitrateKbps = 300; + const debounceInterval = 20; /** - * Applies the configured start bitrate when this media section belongs to `cid`. - * This SDP munging is used for a bitrate setting that cannot be applied through - * `RTCRtpEncodingParameters`. + * Codec payload for `codec` in this media section, when the section carries `cid`. * - * Returns `undefined` when the section does not belong to the track, `0` when - * it does but does not offer the requested codec, and the codec payload when the - * requested codec is present (whether the bitrate was added or already set). + * Returns `undefined` when the section does not belong to the track, `0` when it does but + * does not offer the requested codec, and the codec payload otherwise. * * @internal */ -export function applyVideoStartBitrate( +export function findTrackCodecPayload( media: MediaDescription, cid: string, codec: string, - maxbr: number, - isScreenShare = false, ): number | undefined { if (!media.msid?.includes(cid)) { return undefined; } + return media.rtp.find((rtp) => rtp.codec.toUpperCase() === codec.toUpperCase())?.payload ?? 0; +} - const codecPayload = - media.rtp.find((rtp) => rtp.codec.toUpperCase() === codec.toUpperCase())?.payload ?? 0; - if (codecPayload === 0) { - return 0; +/** + * Start bitrate hinted for a single track, or `undefined` when its target is too low to + * be worth seeding. + * + * 90% of the target leaves ~10% headroom for the estimator to settle. The same multiplier + * is used for every codec because the target already reflects the codec's efficiency. + * Camera is capped at 1 Mbps so the estimator does not open too aggressively on a + * high-bitrate track; screen share is exempt, because its content needs the bitrate + * immediately to stay legible. + * + * TODO: adjust dynamically from network conditions (e.g. a previous BWE estimate) rather + * than a fixed cap. + * + * @internal + */ +export function computeTrackStartBitrate(trackbr: TrackBitrateInfo): number | undefined { + if (trackbr.maxbr < minTargetBitrateKbps) { + return undefined; } + const calculated = Math.round(trackbr.maxbr * startBitrateMultiplier); + return trackbr.isScreenShare ? calculated : Math.min(calculated, maxStartBitrateKbps); +} - // Use 90% of target bitrate, capped at 1 Mbps for camera to prevent BWE - // from starting too aggressively. Screen share is not capped since text/UI - // clarity requires high bitrate from the start. - // TODO: dynamically adjust start bitrate based on network conditions (e.g., previous BWE estimate) - const calculatedStartBitrate = Math.round(maxbr * startBitrateMultiplier); - const startBitrate = isScreenShare - ? calculatedStartBitrate - : Math.min(calculatedStartBitrate, maxStartBitrateKbps); +/** + * The single start bitrate for this peer connection: the largest hint among the video + * m-sections of `media` that map to a published track. + * + * libwebrtc reads `x-google-start-bitrate` per m-section but applies it to the shared + * `Call` (`WebRtcVideoSendChannel::ApplyChangedParams` -> `SetSdpBitrateParameters`), where + * `RtpBitrateConfigurator` holds one config for the whole connection. Differing per-section + * values are therefore last-writer-wins, decided by m-section order, so every video section + * gets the same number instead. + * + * Only sections present in the current SDP are considered: `trackBitrates` is append-only + * and can hold entries for tracks that are no longer published. + * + * @internal + */ +export function computeConnectionStartBitrate( + media: MediaDescription[], + trackBitrates: TrackBitrateInfo[], +): number | undefined { + let connectionStartBitrate: number | undefined; + for (const m of media) { + if (m.type !== 'video') { + continue; + } + for (const trackbr of trackBitrates) { + if (!trackbr.cid) { + continue; + } + const codecPayload = findTrackCodecPayload(m, trackbr.cid, trackbr.codec); + if (codecPayload === undefined) { + continue; + } + const startBitrate = codecPayload > 0 ? computeTrackStartBitrate(trackbr) : undefined; + if ( + startBitrate !== undefined && + (connectionStartBitrate === undefined || startBitrate > connectionStartBitrate) + ) { + connectionStartBitrate = startBitrate; + } + break; + } + } + return connectionStartBitrate; +} +/** + * Declares `x-google-start-bitrate` on `codecPayload`'s fmtp. This SDP munging is used for + * a bitrate setting that cannot be applied through `RTCRtpEncodingParameters`. + * + * Returns whether the section now carries the hint. + * + * @internal + */ +export function applyVideoStartBitrate( + media: MediaDescription, + codecPayload: number, + startBitrate: number, +): boolean { const fmtp = media.fmtp.find((entry) => entry.payload === codecPayload); if (fmtp) { - // If another track's fmtp already has a start bitrate, it cannot be - // overridden here because the payload type is shared across the bundle. - // This forces every track sharing that payload to use the initial track's - // start bitrate. + // A payload type is shared across the bundle, so a value written for one section is + // already the connection-level one; leave it rather than rewrite it. if (!fmtp.config.includes('x-google-start-bitrate')) { fmtp.config += `;x-google-start-bitrate=${startBitrate}`; } @@ -90,7 +157,7 @@ export function applyVideoStartBitrate( }); } - return codecPayload; + return true; } export const PCEvents = { @@ -141,6 +208,15 @@ export default class PCTransport extends (EventEmitter as new () => TypedEmitter trackBitrates: TrackBitrateInfo[] = []; + /** + * Whether an offer carrying the connection-level `x-google-start-bitrate` has been + * accepted locally. The hint is written once per peer connection: libwebrtc retains + * `start_bitrate_bps` in `RtpBitrateConfigurator` and re-applies it on network route + * changes, so a later rewrite is at best a no-op and at worst restarts a converged + * bandwidth estimator. A new peer connection (full reconnect) seeds a new estimator. + */ + private hasAppliedVideoStartBitrate = false; + remoteStereoMids: string[] = []; remoteNackMids: string[] = []; @@ -453,6 +529,14 @@ export default class PCTransport extends (EventEmitter as new () => TypedEmitter this.log.debug('original offer', { sdp: offer.sdp }); const sdpParsed = parse(offer.sdp ?? ''); + // One value for every video m-section, written only on the first offer that carries + // local video: the hint is connection-level in libwebrtc, so differing per-section + // values would be last-writer-wins on m-section order. Offers before any video is + // published (data channel or audio only) find no target and leave the latch unset. + const connectionStartBitrate = this.hasAppliedVideoStartBitrate + ? undefined + : computeConnectionStartBitrate(sdpParsed.media, this.trackBitrates); + let appliedVideoStartBitrate = false; sdpParsed.media.forEach((media) => { ensureIPAddrMatchVersion(media); if (media.type === 'audio') { @@ -463,19 +547,21 @@ export default class PCTransport extends (EventEmitter as new () => TypedEmitter return false; } - const codecPayload = applyVideoStartBitrate( - media, - trackbr.cid, - trackbr.codec, - trackbr.maxbr, - trackbr.isScreenShare, - ); + const codecPayload = findTrackCodecPayload(media, trackbr.cid, trackbr.codec); if (codecPayload === undefined) { return false; } - if (codecPayload > 0 && isSVCCodec(trackbr.codec) && !isSafari()) { - this.ddExtID = ensureVideoDDExtension(media, sdpParsed, this.ddExtID); + if (codecPayload > 0) { + if (connectionStartBitrate !== undefined) { + appliedVideoStartBitrate = + applyVideoStartBitrate(media, codecPayload, connectionStartBitrate) || + appliedVideoStartBitrate; + } + + if (isSVCCodec(trackbr.codec) && !isSafari()) { + this.ddExtID = ensureVideoDDExtension(media, sdpParsed, this.ddExtID); + } } return true; @@ -500,7 +586,13 @@ export default class PCTransport extends (EventEmitter as new () => TypedEmitter }); return; } - await this.setMungedSDP(offer, write(sdpParsed)); + const mungedSdp = write(sdpParsed); + await this.setMungedSDP(offer, mungedSdp); + // setMungedSDP falls back to the unmunged SDP on rejection. Only consume the + // one-shot hint once the SDP carrying it has been accepted locally. + if (appliedVideoStartBitrate && offer.sdp === mungedSdp) { + this.hasAppliedVideoStartBitrate = true; + } this.onOffer(offer, this.latestOfferId); } finally { unlock(); From f9333adc3c7f519c28cdac5cafd86f2aa5621692 Mon Sep 17 00:00:00 2001 From: shijing xian Date: Mon, 21 Sep 2026 16:34:17 -0700 Subject: [PATCH 2/3] Count only sending sections toward the connection start bitrate `trackBitrates` is append-only and an unpublished section keeps its `a=msid`, so matching a section to a track by msid alone still paired a stale entry with the section it used to occupy. Two consequences, both reachable while `hasAppliedVideoStartBitrate` is still unset (a superseded offer, or a munge the browser rejected): an uncapped screen-share target could seed a connection that now carries only a camera, and the unpublish renegotiation itself could consume the one-shot hint on a section that sends nothing, leaving every later publish with no hint at all. `a=sendonly` separates a live send from an unpublished or pre-populated section: every publish creates its transceiver with `direction: 'sendonly'`, `unpublishTrack` sets it to `inactive` (and `removeTrack` does the same transition for the simulcast senders), and the pre-populated placeholders are `recvonly`. The matching in the munge loop is left as it was: it now writes the correct connection-level value, and suppressing it on reverted sections could leave them without an fmtp line the live section has, which is the bundled payload type collision `conformBundledCodecFmtp` works around. Co-Authored-By: Claude Opus 5 (1M context) --- .../video-start-bitrate-connection-level.md | 4 +- src/room/PCTransport.test.ts | 48 +++++++++++++++++++ src/room/PCTransport.ts | 12 +++-- 3 files changed, 60 insertions(+), 4 deletions(-) diff --git a/.changeset/video-start-bitrate-connection-level.md b/.changeset/video-start-bitrate-connection-level.md index af5807e0c2..a04e579227 100644 --- a/.changeset/video-start-bitrate-connection-level.md +++ b/.changeset/video-start-bitrate-connection-level.md @@ -4,7 +4,9 @@ Write the `x-google-start-bitrate` hint as a single connection-level value, once per publisher connection. -libwebrtc reads this fmtp parameter per m-section but applies it to the shared `Call` (`WebRtcVideoSendChannel::ApplyChangedParams` → `SetSdpBitrateParameters`), where `RtpBitrateConfigurator` holds one config for the whole peer connection. Differing per-section values were therefore last-writer-wins on m-section order, so publishing a camera and a screen share together could seed the estimator from either one depending on SDP layout. Every video section now carries the same value: the largest hint among the sections that map to a published track. +libwebrtc reads this fmtp parameter per m-section but applies it to the shared `Call` (`WebRtcVideoSendChannel::ApplyChangedParams` → `SetSdpBitrateParameters`), where `RtpBitrateConfigurator` holds one config for the whole peer connection. Differing per-section values were therefore last-writer-wins on m-section order, so publishing a camera and a screen share together could seed the estimator from either one depending on SDP layout. Every video section now carries the same value: the largest hint among the sections that are currently sending. + +Only sending sections count. The list of registered track bitrates is append-only, and an unpublished section keeps its `a=msid`, so matching a section to a track by msid alone would still pair a stale entry with the section it used to occupy — letting an uncapped screen-share target seed a connection that now carries only a camera, or consuming the one-shot hint on a section that sends nothing, which would leave later publishes with no hint at all. `a=sendonly` distinguishes a live send from an unpublished (`a=inactive`) or pre-populated (`a=recvonly`) section. The hint is also written only on the first offer that carries local video, instead of on every offer. libwebrtc retains `start_bitrate_bps` and re-applies it on network route changes (`RtpTransportControllerSend::OnNetworkRouteChanged`), so rewriting it later is at best a no-op and at worst restarts a converged bandwidth estimator. A full reconnect builds a new peer connection and seeds the new estimator again. diff --git a/src/room/PCTransport.test.ts b/src/room/PCTransport.test.ts index 334bdb03e9..80476985c8 100644 --- a/src/room/PCTransport.test.ts +++ b/src/room/PCTransport.test.ts @@ -82,6 +82,27 @@ a=sendonly a=msid:PA_remote|camera camera-cid a=rtpmap:96 VP8/90000`; +// The same bundle after the screen share is unpublished: `unpublishTrack` sets the +// transceiver to `inactive`, but the section keeps its `a=msid`, so it still matches the +// append-only trackBitrates entry. Only the camera is still sending. +const UNPUBLISHED_SCREEN_SHARE = `v=0 +o=- 0 0 IN IP4 127.0.0.1 +s=- +t=0 0 +a=group:BUNDLE 0 1 +m=video 9 UDP/TLS/RTP/SAVPF 96 +c=IN IP4 0.0.0.0 +a=mid:0 +a=inactive +a=msid:PA_remote|camera other-track +a=rtpmap:96 VP8/90000 +m=video 9 UDP/TLS/RTP/SAVPF 96 +c=IN IP4 0.0.0.0 +a=mid:1 +a=sendonly +a=msid:PA_remote|camera camera-cid +a=rtpmap:96 VP8/90000`; + describe('video start bitrate', () => { it('matches only the section whose msid track ID matches the cid', () => { const { media } = parse(TWO_VIDEO_SECTIONS); @@ -137,11 +158,38 @@ describe('video start bitrate', () => { expect(startBitrate).toBe(900); }); + it('ignores a section that stopped sending but kept its msid', () => { + const trackBitrates = [ + { cid: 'camera-cid', codec: 'VP8', maxbr: 1_000 }, + { cid: 'other-track', codec: 'VP8', maxbr: 8_000, isScreenShare: true }, + ]; + + // While both send, the uncapped screen share wins the connection-level max. + expect(computeConnectionStartBitrate(parse(TWO_VIDEO_SECTIONS).media, trackBitrates)).toBe( + 7_200, + ); + // Once it is unpublished its entry is stale, so only the capped camera counts. + expect( + computeConnectionStartBitrate(parse(UNPUBLISHED_SCREEN_SHARE).media, trackBitrates), + ).toBe(900); + }); + it('gives no connection value when no section maps to a published track', () => { const { media } = parse(TWO_VIDEO_SECTIONS); expect(computeConnectionStartBitrate(media, [])).toBeUndefined(); }); + + it('leaves the hint unset when only non-sending sections match', () => { + // Nothing is published, so the one-shot hint must not be consumed on a dead section. + const { media } = parse(UNPUBLISHED_SCREEN_SHARE); + + expect( + computeConnectionStartBitrate(media, [ + { cid: 'other-track', codec: 'VP8', maxbr: 8_000, isScreenShare: true }, + ]), + ).toBeUndefined(); + }); }); describe('placeholderMidsFromTransceivers', () => { diff --git a/src/room/PCTransport.ts b/src/room/PCTransport.ts index 54afd416bf..94173edfa1 100644 --- a/src/room/PCTransport.ts +++ b/src/room/PCTransport.ts @@ -94,8 +94,14 @@ export function computeTrackStartBitrate(trackbr: TrackBitrateInfo): number | un * values are therefore last-writer-wins, decided by m-section order, so every video section * gets the same number instead. * - * Only sections present in the current SDP are considered: `trackBitrates` is append-only - * and can hold entries for tracks that are no longer published. + * Only sections that currently send are considered. `trackBitrates` is append-only and an + * unpublished section keeps its `a=msid`, so matching on msid alone would still pair a stale + * entry with its old section — letting an uncapped screen-share target seed a connection that + * now carries only a camera, or consuming the one-shot hint on a section that sends nothing. + * `a=sendonly` is the discriminator: every publish creates its transceiver with + * `direction: 'sendonly'`, unpublish sets it to `inactive` (explicitly in + * `LocalParticipant.unpublishTrack`, and by `removeTrack`'s sendonly -> inactive transition for + * the simulcast senders), and the pre-populated placeholders are `recvonly`. * * @internal */ @@ -105,7 +111,7 @@ export function computeConnectionStartBitrate( ): number | undefined { let connectionStartBitrate: number | undefined; for (const m of media) { - if (m.type !== 'video') { + if (m.type !== 'video' || m.direction !== 'sendonly') { continue; } for (const trackbr of trackBitrates) { From 04f3556a922def768a1af273d39149221ac40415 Mon Sep 17 00:00:00 2001 From: shijing xian Date: Mon, 21 Sep 2026 16:59:11 -0700 Subject: [PATCH 3/3] Exclude only sections that cannot send, not everything but sendonly Matching `a=sendonly` exactly dropped the legacy `addTrack` fallback: `createSender` uses it when `addTransceiver` is unavailable, and `addTrack` reuses a transceiver rather than creating a sendonly one, so a published camera lands on a `sendrecv` section. A section with no direction attribute has the same problem, since SDP defaults it to sendrecv. Those clients got no start bitrate hint at all. Invert the test: skip `recvonly` and `inactive`, the only two directions that cannot carry local media, and count everything else. That still excludes both shapes a dead section takes -- `inactive` from a sendonly transceiver, `recvonly` from the sendrecv one `addTrack` reuses -- and the pre-populated placeholders, which are `recvonly`. Co-Authored-By: Claude Opus 5 (1M context) --- .../video-start-bitrate-connection-level.md | 2 +- src/room/PCTransport.test.ts | 45 ++++++++++++++++++- src/room/PCTransport.ts | 18 +++++--- 3 files changed, 57 insertions(+), 8 deletions(-) diff --git a/.changeset/video-start-bitrate-connection-level.md b/.changeset/video-start-bitrate-connection-level.md index a04e579227..b8881637ec 100644 --- a/.changeset/video-start-bitrate-connection-level.md +++ b/.changeset/video-start-bitrate-connection-level.md @@ -6,7 +6,7 @@ Write the `x-google-start-bitrate` hint as a single connection-level value, once libwebrtc reads this fmtp parameter per m-section but applies it to the shared `Call` (`WebRtcVideoSendChannel::ApplyChangedParams` → `SetSdpBitrateParameters`), where `RtpBitrateConfigurator` holds one config for the whole peer connection. Differing per-section values were therefore last-writer-wins on m-section order, so publishing a camera and a screen share together could seed the estimator from either one depending on SDP layout. Every video section now carries the same value: the largest hint among the sections that are currently sending. -Only sending sections count. The list of registered track bitrates is append-only, and an unpublished section keeps its `a=msid`, so matching a section to a track by msid alone would still pair a stale entry with the section it used to occupy — letting an uncapped screen-share target seed a connection that now carries only a camera, or consuming the one-shot hint on a section that sends nothing, which would leave later publishes with no hint at all. `a=sendonly` distinguishes a live send from an unpublished (`a=inactive`) or pre-populated (`a=recvonly`) section. +Only sending sections count. The list of registered track bitrates is append-only, and an unpublished section keeps its `a=msid`, so matching a section to a track by msid alone would still pair a stale entry with the section it used to occupy — letting an uncapped screen-share target seed a connection that now carries only a camera, or consuming the one-shot hint on a section that sends nothing, which would leave later publishes with no hint at all. The section's direction distinguishes them: `a=recvonly` and `a=inactive` cannot carry local media and are exactly where an unpublished or pre-populated section lands, while `a=sendonly`, `a=sendrecv` and an omitted direction all send. The hint is also written only on the first offer that carries local video, instead of on every offer. libwebrtc retains `start_bitrate_bps` and re-applies it on network route changes (`RtpTransportControllerSend::OnNetworkRouteChanged`), so rewriting it later is at best a no-op and at worst restarts a converged bandwidth estimator. A full reconnect builds a new peer connection and seeds the new estimator again. diff --git a/src/room/PCTransport.test.ts b/src/room/PCTransport.test.ts index 80476985c8..6a9d445377 100644 --- a/src/room/PCTransport.test.ts +++ b/src/room/PCTransport.test.ts @@ -103,6 +103,26 @@ a=sendonly a=msid:PA_remote|camera camera-cid a=rtpmap:96 VP8/90000`; +// The legacy `addTrack` fallback (no `addTransceiver` support) reuses a transceiver instead +// of creating a sendonly one, so the published camera lands on a `sendrecv` section. Mid 1 +// omits the direction attribute entirely, which SDP also defaults to sendrecv. Both send. +const LEGACY_ADD_TRACK_SECTIONS = `v=0 +o=- 0 0 IN IP4 127.0.0.1 +s=- +t=0 0 +a=group:BUNDLE 0 1 +m=video 9 UDP/TLS/RTP/SAVPF 96 +c=IN IP4 0.0.0.0 +a=mid:0 +a=sendrecv +a=msid:PA_remote|camera camera-cid +a=rtpmap:96 VP8/90000 +m=video 9 UDP/TLS/RTP/SAVPF 96 +c=IN IP4 0.0.0.0 +a=mid:1 +a=msid:PA_remote|camera other-track +a=rtpmap:96 VP8/90000`; + describe('video start bitrate', () => { it('matches only the section whose msid track ID matches the cid', () => { const { media } = parse(TWO_VIDEO_SECTIONS); @@ -168,10 +188,18 @@ describe('video start bitrate', () => { expect(computeConnectionStartBitrate(parse(TWO_VIDEO_SECTIONS).media, trackBitrates)).toBe( 7_200, ); - // Once it is unpublished its entry is stale, so only the capped camera counts. + // Once it is unpublished its entry is stale, so only the capped camera counts. Both + // directions a removed sender can land on are excluded: `inactive` from a sendonly + // transceiver, `recvonly` from the sendrecv one the addTrack fallback reuses. expect( computeConnectionStartBitrate(parse(UNPUBLISHED_SCREEN_SHARE).media, trackBitrates), ).toBe(900); + expect( + computeConnectionStartBitrate( + parse(UNPUBLISHED_SCREEN_SHARE.replace('a=inactive', 'a=recvonly')).media, + trackBitrates, + ), + ).toBe(900); }); it('gives no connection value when no section maps to a published track', () => { @@ -180,6 +208,21 @@ describe('video start bitrate', () => { expect(computeConnectionStartBitrate(media, [])).toBeUndefined(); }); + it('counts sendrecv and direction-less sections, which still send local media', () => { + // The legacy addTrack fallback never produces `sendonly`, so a strict match on it would + // leave those clients with no hint at all. + const { media } = parse(LEGACY_ADD_TRACK_SECTIONS); + + expect( + computeConnectionStartBitrate(media, [{ cid: 'camera-cid', codec: 'VP8', maxbr: 1_000 }]), + ).toBe(900); + expect( + computeConnectionStartBitrate(media, [ + { cid: 'other-track', codec: 'VP8', maxbr: 2_000, isScreenShare: true }, + ]), + ).toBe(1_800); + }); + it('leaves the hint unset when only non-sending sections match', () => { // Nothing is published, so the one-shot hint must not be consumed on a dead section. const { media } = parse(UNPUBLISHED_SCREEN_SHARE); diff --git a/src/room/PCTransport.ts b/src/room/PCTransport.ts index 94173edfa1..4fe9ee0dde 100644 --- a/src/room/PCTransport.ts +++ b/src/room/PCTransport.ts @@ -94,14 +94,20 @@ export function computeTrackStartBitrate(trackbr: TrackBitrateInfo): number | un * values are therefore last-writer-wins, decided by m-section order, so every video section * gets the same number instead. * - * Only sections that currently send are considered. `trackBitrates` is append-only and an + * Only sections that can send local media are considered. `trackBitrates` is append-only and an * unpublished section keeps its `a=msid`, so matching on msid alone would still pair a stale * entry with its old section — letting an uncapped screen-share target seed a connection that * now carries only a camera, or consuming the one-shot hint on a section that sends nothing. - * `a=sendonly` is the discriminator: every publish creates its transceiver with - * `direction: 'sendonly'`, unpublish sets it to `inactive` (explicitly in - * `LocalParticipant.unpublishTrack`, and by `removeTrack`'s sendonly -> inactive transition for - * the simulcast senders), and the pre-populated placeholders are `recvonly`. + * + * The direction is the discriminator, as an exclusion rather than a match: `recvonly` and + * `inactive` are the only directions that cannot carry local media, and they are exactly the + * two a dead section lands on — unpublish sets `inactive` (explicitly in + * `LocalParticipant.unpublishTrack`, and via `removeTrack`'s sendonly -> inactive transition for + * the simulcast senders), `removeTrack` on a `sendrecv` transceiver leaves `recvonly`, and the + * pre-populated placeholders are `recvonly`. Everything else sends: `sendonly` from the + * `addTransceiver` path, `sendrecv` from the legacy `addTrack` fallback (which reuses a + * transceiver rather than creating a sendonly one), and a section with no direction attribute, + * which SDP defaults to `sendrecv`. * * @internal */ @@ -111,7 +117,7 @@ export function computeConnectionStartBitrate( ): number | undefined { let connectionStartBitrate: number | undefined; for (const m of media) { - if (m.type !== 'video' || m.direction !== 'sendonly') { + if (m.type !== 'video' || m.direction === 'recvonly' || m.direction === 'inactive') { continue; } for (const trackbr of trackBitrates) {