From 2c3ebb7e49432ec1e8ec9fa9d76551ac76836e23 Mon Sep 17 00:00:00 2001 From: shijing xian Date: Tue, 20 Oct 2026 15:40:51 -0700 Subject: [PATCH 1/5] Conform bundled codec fmtp across receive sections MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A payload type is bundle-wide, so libwebrtc expects it to map to identical codec parameters in every m-line and reports "A BUNDLE group contains a codec collision between ..." where it does not. Single peer connection mode creates exactly that divergence: the video start bitrate is written on the sections that send video, while the receive sections the same connection carries for subscribed media share those payload types without it. Conform the receive sections to the sending ones after munging. Sending sections always win, so a published track's encoder parameters are never altered; where no sending section declares a payload type the receive sections converge on the first config seen. Codec-agnostic by design — it conforms whatever diverged rather than `x-google-start-bitrate` specifically, which is what keeps it correct as other munges are added. On the local-description path the collision is logged rather than enforced: the description is accepted, the munge is not dropped, and the hint reaches the peer connection either way. This runs because an SDP whose bundle is self-inconsistent is still wrong, and because what libwebrtc tolerates here it does not everywhere. The same divergence in the server's answer is reported on every single-PC negotiation and is not addressed here, since conforming an answer means selecting sections by transceiver rather than by direction (an answer mirrors it) and munging a remote description. Mirrors `conformBundledCodecFmtp` in client-sdk-js. Co-Authored-By: Claude Opus 5 (1M context) --- .changes/conform-bundled-codec-fmtp | 1 + Sources/LiveKit/Core/Transport.swift | 87 +++++++++- Sources/LiveKit/Support/SDP/SDP.swift | 19 ++ .../SDP/BundledCodecFmtpTests.swift | 164 ++++++++++++++++++ .../SDP/TransportMungeFallbackTests.swift | 75 ++++++++ 5 files changed, 344 insertions(+), 2 deletions(-) create mode 100644 .changes/conform-bundled-codec-fmtp create mode 100644 Tests/LiveKitCoreTests/SDP/BundledCodecFmtpTests.swift diff --git a/.changes/conform-bundled-codec-fmtp b/.changes/conform-bundled-codec-fmtp new file mode 100644 index 000000000..4d8572cfc --- /dev/null +++ b/.changes/conform-bundled-codec-fmtp @@ -0,0 +1 @@ +patch type="fixed" "Publisher offers now keep one fmtp config per payload type across the BUNDLE group, by conforming receive-only sections to the sending ones after munging — previously the video start bitrate written on a sending section left the preallocated receive sections of single peer connection mode declaring the same payload types without it" diff --git a/Sources/LiveKit/Core/Transport.swift b/Sources/LiveKit/Core/Transport.swift index 226857aed..e8cf1e81c 100644 --- a/Sources/LiveKit/Core/Transport.swift +++ b/Sources/LiveKit/Core/Transport.swift @@ -324,9 +324,13 @@ final class Transport: NSObject, Loggable { let startBitrate: (String) -> String = { hasAppliedStartBitrate ? $0 : Self.mungeVideoStartBitrate($0, kbpsBySenderId: kbpsBySenderId) } + // `conformBundledCodecFmtp` runs last in both modes: it repairs whatever the munges + // before it made inconsistent across the BUNDLE group, and it is a no-op on an offer + // with no receive-only section — which is every dual PC offer until a track is + // unpublished. offer = try await set(localDescription: offer, munging: singlePCMode - ? [Self.mungeInactiveToRecvOnlyForMedia, Self.mungeOpusStereoForAllAudio, startBitrate] - : [startBitrate]) + ? [Self.mungeInactiveToRecvOnlyForMedia, Self.mungeOpusStereoForAllAudio, startBitrate, Self.conformBundledCodecFmtp] + : [startBitrate, Self.conformBundledCodecFmtp]) // A rejected munge is dropped and the offer retried without it, so the one-shot // hint is consumed only once the description that carries it has been accepted. if !_hasAppliedVideoStartBitrate, Self.carriesVideoStartBitrate(offer.sdp) { @@ -599,6 +603,85 @@ extension Transport { return modified ? document.write() : sdp } + /// Whether `section` cannot carry local media, and so is one of the sections + /// ``conformBundledCodecFmtp(_:)`` rewrites. + /// + /// `recvonly` and `inactive` are exactly the two directions a section lands on when it has + /// no outgoing track: the receive sections preallocated for single PC mode are offered + /// `recvonly`, and a section whose track was unpublished reverts to one or the other. + /// Everything else sends — `sendonly`, `sendrecv`, and a section with no direction + /// attribute, which SDP defaults to `sendrecv`. + nonisolated static func isReceiveOnlySection(_ section: SDPMediaSection) -> Bool { + section.direction == .recvonly || section.direction == .inactive + } + + /// Munge the offer so every payload type carries identical fmtp parameters across the BUNDLE + /// group, by conforming the sections that cannot send to the ones that can. + /// + /// A payload type is bundle-wide, so libwebrtc expects it to map to identical codec + /// parameters in every m-line and reports "A BUNDLE group contains a codec collision + /// between ..." where it does not. Single PC mode creates exactly that divergence: + /// ``mungeVideoStartBitrate(_:kbpsBySenderId:)`` writes `x-google-start-bitrate` on the + /// sections that send video, while the receive sections preallocated for subscribed media + /// share those payload types without it. + /// + /// On the local-description path that report is a log, not an enforcement: the description + /// is accepted, the munge is not dropped, and the hint reaches the peer connection either + /// way. This runs because an SDP whose bundle is self-inconsistent is still wrong, and + /// because what libwebrtc tolerates here it does not everywhere — the same divergence in + /// the server's *answer* is reported on every single-PC negotiation, which this cannot + /// reach, since conforming an answer means selecting sections by transceiver rather than by + /// direction (an answer mirrors it) and munging a remote description. + /// + /// Sending sections always win, so a published track's encoder parameters are never altered. + /// Where no sending section declares a payload type, the receive sections converge on the + /// first config seen so two of them cannot disagree either. It is codec-agnostic by design: + /// it conforms whatever diverged rather than `x-google-start-bitrate` specifically, which is + /// what keeps it correct as other munges are added — ``mungeOpusStereoForAllAudio(_:)`` only + /// avoids the same fate by writing `stereo=1` to receive sections too. + /// + /// Must run last, after every munge that can introduce divergence. + /// + /// Mirrors `conformBundledCodecFmtp` in client-sdk-js, with two differences. That version + /// selects the sections to rewrite by transceiver — those with no outgoing track — where + /// this reads the direction the SDP already states, which libwebrtc derives from that same + /// state and which keeps the munge a pure `String -> String` like its siblings. And it only + /// rewrites fmtp lines that exist, where this inserts one when a receive section declares + /// the payload in an `a=rtpmap` but has no fmtp for it; "parameters vs. no parameters" is a + /// collision too, and it is the one VP8 hits, since libwebrtc offers it without an fmtp line. + nonisolated static func conformBundledCodecFmtp(_ sdp: String) -> String { + var document = SDP(parsing: sdp) + + var canonicalByPayload: [String: String] = [:] + for section in document.mediaSections { + let isReceiveOnly = isReceiveOnlySection(section) + for fmtp in section.fmtps { + // A sending section overwrites unconditionally, so it wins regardless of the + // order the sections appear in. + if !isReceiveOnly { + canonicalByPayload[fmtp.payload] = fmtp.config + } else if canonicalByPayload[fmtp.payload] == nil { + canonicalByPayload[fmtp.payload] = fmtp.config + } + } + } + guard !canonicalByPayload.isEmpty else { return sdp } + + var modified = false + for index in document.mediaSections.indices where isReceiveOnlySection(document.mediaSections[index]) { + // Keyed off `a=rtpmap` rather than the existing fmtp lines: a payload the section + // declares but gives no parameters still collides with a sending section that gives + // some, and that section has no fmtp line to iterate. + for rtpmap in document.mediaSections[index].rtpmaps { + guard let canonical = canonicalByPayload[rtpmap.payload] else { continue } + if document.mediaSections[index].setFmtp(config: canonical, forPayload: rtpmap.payload) { + modified = true + } + } + } + return modified ? document.write() : sdp + } + /// Whether any video section of `sdp` declares `x-google-start-bitrate`. Checked on the /// description libwebrtc accepted, since a rejected munge is dropped rather than applied. nonisolated static func carriesVideoStartBitrate(_ sdp: String) -> Bool { diff --git a/Sources/LiveKit/Support/SDP/SDP.swift b/Sources/LiveKit/Support/SDP/SDP.swift index 21a3a467e..8d5d2529e 100644 --- a/Sources/LiveKit/Support/SDP/SDP.swift +++ b/Sources/LiveKit/Support/SDP/SDP.swift @@ -152,6 +152,25 @@ struct SDPMediaSection { return true } + /// Replaces the whole fmtp config of `payload`, inserting an `a=fmtp: ` + /// line at the end of the section when the payload has none — which is the case libwebrtc + /// leaves for codecs it offers without parameters, VP8 among them. Returns `true` if the + /// section was modified; `false` when the config is already exactly `config`. + /// + /// Where ``setFmtpParameter(_:value:forPayload:)`` edits one parameter and leaves the rest, + /// this overwrites all of them — for conforming a section to a config decided elsewhere. + @discardableResult + mutating func setFmtp(config: String, forPayload payload: String) -> Bool { + for (index, line) in lines.enumerated() { + guard let fmtp = Self.fmtp(fromLine: line), fmtp.payload == payload else { continue } + guard fmtp.config != config else { return false } + lines[index] = "a=fmtp:\(payload) \(config)" + return true + } + lines.append("a=fmtp:\(payload) \(config)") + return true + } + /// Whether an `a=rtcp-fb: ` line with exactly `value` exists for /// `payload`. Exact-value match: `nack` does not match `nack pli` (RFC 4585 §4.2 /// — they are distinct feedback parameters). diff --git a/Tests/LiveKitCoreTests/SDP/BundledCodecFmtpTests.swift b/Tests/LiveKitCoreTests/SDP/BundledCodecFmtpTests.swift new file mode 100644 index 000000000..5b0f48f7c --- /dev/null +++ b/Tests/LiveKitCoreTests/SDP/BundledCodecFmtpTests.swift @@ -0,0 +1,164 @@ +/* + * Copyright 2026 LiveKit + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +@testable import LiveKit +import Testing + +@Suite(.tags(.media)) +struct BundledCodecFmtpTests { + /// The single PC publisher shape: a sending Opus section and a sending video section + /// carrying the start-bitrate hint, each paired with a receive section preallocated for + /// subscribed media that shares its payload types — payload 100 (VP8) without an fmtp line + /// at all, which is how libwebrtc offers it. + private static let singlePCOffer = """ + v=0 + o=- 0 0 IN IP4 127.0.0.1 + s=- + t=0 0 + a=group:BUNDLE 0 1 2 3 + m=audio 9 UDP/TLS/RTP/SAVPF 111 + a=mid:0 + a=msid:- mic + a=rtpmap:111 opus/48000/2 + a=fmtp:111 minptime=10;useinbandfec=1;stereo=1 + a=sendonly + m=audio 9 UDP/TLS/RTP/SAVPF 111 + a=mid:1 + a=rtpmap:111 opus/48000/2 + a=fmtp:111 minptime=10;useinbandfec=1 + a=recvonly + m=video 9 UDP/TLS/RTP/SAVPF 96 100 + a=mid:2 + a=msid:- cam + a=rtpmap:96 H264/90000 + a=fmtp:96 packetization-mode=1;x-google-start-bitrate=900 + a=rtpmap:100 VP8/90000 + a=fmtp:100 x-google-start-bitrate=900 + a=sendonly + m=video 9 UDP/TLS/RTP/SAVPF 96 100 + a=mid:3 + a=rtpmap:96 H264/90000 + a=fmtp:96 packetization-mode=1 + a=rtpmap:100 VP8/90000 + a=recvonly + """.replacingOccurrences(of: "\n", with: "\r\n") + "\r\n" + + /// Both ways a receive section can diverge are repaired: payload 96 has an fmtp line + /// missing the parameter, payload 100 has no fmtp line at all. + @Test func conformsReceiveSectionsToTheSendingConfig() { + let sections = SDP(parsing: Transport.conformBundledCodecFmtp(Self.singlePCOffer)).mediaSections + + #expect(sections[1].fmtps.map(\.config) == ["minptime=10;useinbandfec=1;stereo=1"]) + #expect(sections[3].fmtps.map(\.config) == ["packetization-mode=1;x-google-start-bitrate=900", + "x-google-start-bitrate=900"]) + // Inserted rather than rewritten, so it lands at the end of its own section. + #expect(sections[3].lines.last == "a=fmtp:100 x-google-start-bitrate=900") + } + + @Test func leavesSendingSectionsUntouched() { + let conformed = SDP(parsing: Transport.conformBundledCodecFmtp(Self.singlePCOffer)).mediaSections + let original = SDP(parsing: Self.singlePCOffer).mediaSections + + #expect(conformed[0].lines == original[0].lines) + #expect(conformed[2].lines == original[2].lines) + } + + /// A sending section is authoritative wherever it appears, so an offer that lists the + /// receive sections first conforms the same way. + @Test func sendingSectionWinsRegardlessOfOrder() { + let reordered = SDP(parsing: Self.singlePCOffer) + var swapped = reordered + swapped.mediaSections = [reordered.mediaSections[1], reordered.mediaSections[0], + reordered.mediaSections[3], reordered.mediaSections[2]] + + let sections = SDP(parsing: Transport.conformBundledCodecFmtp(swapped.write())).mediaSections + + #expect(sections[0].fmtps.map(\.config) == ["minptime=10;useinbandfec=1;stereo=1"]) + #expect(sections[2].fmtps.map(\.config) == ["packetization-mode=1;x-google-start-bitrate=900", + "x-google-start-bitrate=900"]) + } + + /// With no sending section to copy from, the receive sections still converge on one + /// config — two of them disagreeing is a bundle collision just the same. + @Test func receiveSectionsConvergeWithoutASendingSection() { + let receiveOnly = Self.singlePCOffer + .replacingOccurrences(of: "a=fmtp:96 packetization-mode=1;x-google-start-bitrate=900\r\n", with: "") + .replacingOccurrences(of: "a=fmtp:100 x-google-start-bitrate=900\r\n", with: "") + .replacingOccurrences(of: "a=sendonly", with: "a=recvonly") + + let sections = SDP(parsing: Transport.conformBundledCodecFmtp(receiveOnly)).mediaSections + + #expect(Set(sections[0].fmtps.map(\.config)) == Set(sections[1].fmtps.map(\.config))) + #expect(sections[2].fmtps.map(\.config) == sections[3].fmtps.map(\.config)) + } + + /// `inactive` is the other direction a section with no outgoing track lands on — an + /// unpublished track leaves one behind — so it is conformed too. + @Test func treatsInactiveAsReceiveOnly() { + let inactive = Self.singlePCOffer.replacingOccurrences(of: "a=mid:3\r\n", with: "a=mid:3\r\na=inactive\r\n") + let sections = SDP(parsing: Transport.conformBundledCodecFmtp(inactive)).mediaSections + + #expect(sections[3].fmtps.map(\.config) == ["packetization-mode=1;x-google-start-bitrate=900", + "x-google-start-bitrate=900"]) + } + + /// A section with no direction attribute defaults to `sendrecv` (RFC 8866 §6.7), so it is + /// a sending section and must not be rewritten. + @Test func sectionWithoutDirectionCountsAsSending() { + let noDirection = Self.singlePCOffer.replacingOccurrences( + of: "a=rtpmap:100 VP8/90000\r\na=recvonly", with: "a=rtpmap:100 VP8/90000", + ) + let sections = SDP(parsing: Transport.conformBundledCodecFmtp(noDirection)).mediaSections + let original = SDP(parsing: noDirection).mediaSections + + #expect(sections[3].lines == original[3].lines) + } + + @Test func isIdempotentAndPreservesUnrelatedSDP() { + let once = Transport.conformBundledCodecFmtp(Self.singlePCOffer) + + #expect(Transport.conformBundledCodecFmtp(once) == once) + #expect(once.hasSuffix("\r\n")) + // Nothing to conform: no receive section, so byte-identical. + #expect(Transport.conformBundledCodecFmtp(once.replacingOccurrences(of: "a=recvonly", with: "a=sendrecv")) + == once.replacingOccurrences(of: "a=recvonly", with: "a=sendrecv")) + } + + /// The pipeline the publisher actually runs: the start-bitrate munge diverges the shared + /// payload types, the conform pass puts them back. Expressed as the invariant libwebrtc + /// enforces — one fmtp config per payload type across the BUNDLE group — rather than as + /// specific strings. + @Test func startBitrateFollowedByConformLeavesNoCollision() { + let unconformed = Transport.mungeVideoStartBitrate(Self.singlePCOffer, kbpsBySenderId: ["cam": 1000]) + let conformed = Transport.conformBundledCodecFmtp(unconformed) + + #expect(Self.fmtpConfigsByPayload(unconformed).contains { $0.value.count > 1 }) + #expect(Self.fmtpConfigsByPayload(conformed).allSatisfy { $0.value.count == 1 }) + } + + /// Every fmtp config each payload type is given across the document, keyed by payload. A + /// payload declared by `a=rtpmap` but given no fmtp line contributes the empty config, + /// since "parameters" and "no parameters" collide just as two different parameter sets do. + private static func fmtpConfigsByPayload(_ sdp: String) -> [String: Set] { + var result: [String: Set] = [:] + for section in SDP(parsing: sdp).mediaSections { + for rtpmap in section.rtpmaps { + result[rtpmap.payload, default: []].insert(section.fmtp(forPayload: rtpmap.payload)?.config ?? "") + } + } + return result + } +} diff --git a/Tests/LiveKitCoreTests/SDP/TransportMungeFallbackTests.swift b/Tests/LiveKitCoreTests/SDP/TransportMungeFallbackTests.swift index 963474a52..8cebb2076 100644 --- a/Tests/LiveKitCoreTests/SDP/TransportMungeFallbackTests.swift +++ b/Tests/LiveKitCoreTests/SDP/TransportMungeFallbackTests.swift @@ -215,6 +215,81 @@ struct TransportMungeFallbackTests { } } + // MARK: - Bundled codec fmtp + + /// Adds the single PC publisher shape — receive sections preallocated for subscribed media + /// alongside a sending one — and returns the sender id of the sending section. + private func addSinglePCVideoLayout(to transport: Transport) async throws -> String { + try await RTC.run { + let receive = LKRTCRtpTransceiverInit() + receive.direction = .recvOnly + for _ in 0 ..< 3 { + _ = try transport.addTransceiver(ofType: .video, transceiverInit: receive) + } + let send = LKRTCRtpTransceiverInit() + send.direction = .sendOnly + return try transport.addTransceiver(ofType: .video, transceiverInit: send).sender.senderId + } + } + + /// Every payload type's distinct fmtp configs across the document, keyed by payload. A + /// payload declared by `a=rtpmap` with no fmtp line contributes the empty config, since + /// libwebrtc counts "parameters" against "no parameters" as a collision too. + private func fmtpConfigsByPayload(_ sdp: String) -> [String: Set] { + var result: [String: Set] = [:] + for section in SDP(parsing: sdp).mediaSections { + for rtpmap in section.rtpmaps { + result[rtpmap.payload, default: []].insert(section.fmtp(forPayload: rtpmap.payload)?.config ?? "") + } + } + return result + } + + /// What the conform pass exists for, on the shipped libwebrtc: with receive sections sharing + /// the sending section's payload types, writing `x-google-start-bitrate` on the sending + /// section alone leaves a payload type with two different configs inside one BUNDLE group — + /// which libwebrtc reports as "a BUNDLE group contains a codec collision". + /// + /// Note what it does *not* do: `setLocalDescription` still succeeds, the munge is not + /// dropped, and the hint survives. The collision is logged, not enforced, on this path. + @Test func startBitrateAloneLeavesTheBundleInconsistent() async throws { + try await withTransport { transport in + let senderId = try await addSinglePCVideoLayout(to: transport) + let offer = try await transport.createOffer() + + let applied = try await transport.set(localDescription: offer, munging: [ + { Transport.mungeVideoStartBitrate($0, kbpsBySenderId: [senderId: 1000]) }, + ]) + + #expect(applied !== offer) + #expect(Transport.carriesVideoStartBitrate(applied.sdp)) + #expect(fmtpConfigsByPayload(applied.sdp).contains { $0.value.count > 1 }) + } + } + + /// And the fix: conforming the receive sections to the sending one leaves every shared + /// payload type with one config, with the hint still on the description that is applied. + @Test func conformedStartBitrateIsAcceptedWithReceiveSections() async throws { + try await withTransport { transport in + let senderId = try await addSinglePCVideoLayout(to: transport) + let offer = try await transport.createOffer() + + let applied = try await transport.set(localDescription: offer, munging: [ + { Transport.mungeVideoStartBitrate($0, kbpsBySenderId: [senderId: 1000]) }, + Transport.conformBundledCodecFmtp, + ]) + + #expect(applied !== offer) + #expect(Transport.carriesVideoStartBitrate(applied.sdp)) + #expect(fmtpConfigsByPayload(applied.sdp).allSatisfy { $0.value.count == 1 }) + let sending = try #require(SDP(parsing: applied.sdp).mediaSections.first { $0.msidTrackId == senderId }) + for rtpmap in sending.rtpmaps where Transport.startBitrateCodecs.contains(rtpmap.codec.uppercased()) { + #expect(sending.fmtp(forPayload: rtpmap.payload)?.parameters.contains("x-google-start-bitrate=1000") == true, + "payload \(rtpmap.payload) (\(rtpmap.codec))") + } + } + } + private actor OfferBox { private(set) var all: [LKRTCSessionDescription] = [] func append(_ offer: LKRTCSessionDescription) { all.append(offer) } From 250cec3b75e8bbceb514c52c17334478daa4e933 Mon Sep 17 00:00:00 2001 From: shijing xian Date: Wed, 28 Oct 2026 16:20:57 -0700 Subject: [PATCH 2/5] Treat a sending section's absence of fmtp as canonical too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The canonical map was built by walking fmtp lines, so a payload a sending section declares in `a=rtpmap` with no `a=fmtp` contributed nothing. If a receive section carried parameters for that payload its config became canonical, and since the rewrite only visits receive sections the collision survived: the sender still advertised the payload bare. Walk the sending sections' rtpmaps instead and record the empty config where there is no fmtp, then conform a receive section to it by removing its fmtp line rather than writing one. "No parameters" collides with "some parameters" exactly as two different parameter sets do, so it has to be representable. Reachable whenever a receive section has fmtp for a payload the sender leaves bare — VP8 being the case that matters, since libwebrtc offers it without an fmtp line until something adds one. Co-Authored-By: Claude Opus 5 (1M context) --- Sources/LiveKit/Core/Transport.swift | 32 +++++++++++-------- Sources/LiveKit/Support/SDP/SDP.swift | 14 ++++++++ .../SDP/BundledCodecFmtpTests.swift | 16 ++++++++++ 3 files changed, 49 insertions(+), 13 deletions(-) diff --git a/Sources/LiveKit/Core/Transport.swift b/Sources/LiveKit/Core/Transport.swift index e8cf1e81c..cbab788a0 100644 --- a/Sources/LiveKit/Core/Transport.swift +++ b/Sources/LiveKit/Core/Transport.swift @@ -653,16 +653,21 @@ extension Transport { var document = SDP(parsing: sdp) var canonicalByPayload: [String: String] = [:] - for section in document.mediaSections { - let isReceiveOnly = isReceiveOnlySection(section) - for fmtp in section.fmtps { - // A sending section overwrites unconditionally, so it wins regardless of the - // order the sections appear in. - if !isReceiveOnly { - canonicalByPayload[fmtp.payload] = fmtp.config - } else if canonicalByPayload[fmtp.payload] == nil { - canonicalByPayload[fmtp.payload] = fmtp.config - } + for section in document.mediaSections where !isReceiveOnlySection(section) { + // Every payload a sending section declares is canonical, including the ones it + // declares with no fmtp at all: "no parameters" collides with "some parameters" + // exactly as two different parameter sets do, so the absence has to be recorded or + // a receive section's fmtp would become canonical for a payload the sender leaves + // bare — and the rewrite below, which only touches receive sections, could never + // resolve it. A sending section overwrites unconditionally, so it wins regardless + // of the order the sections appear in. + for rtpmap in section.rtpmaps { + canonicalByPayload[rtpmap.payload] = section.fmtp(forPayload: rtpmap.payload)?.config ?? "" + } + } + for section in document.mediaSections where isReceiveOnlySection(section) { + for fmtp in section.fmtps where canonicalByPayload[fmtp.payload] == nil { + canonicalByPayload[fmtp.payload] = fmtp.config } } guard !canonicalByPayload.isEmpty else { return sdp } @@ -674,9 +679,10 @@ extension Transport { // some, and that section has no fmtp line to iterate. for rtpmap in document.mediaSections[index].rtpmaps { guard let canonical = canonicalByPayload[rtpmap.payload] else { continue } - if document.mediaSections[index].setFmtp(config: canonical, forPayload: rtpmap.payload) { - modified = true - } + let changed = canonical.isEmpty + ? document.mediaSections[index].removeFmtp(forPayload: rtpmap.payload) + : document.mediaSections[index].setFmtp(config: canonical, forPayload: rtpmap.payload) + if changed { modified = true } } } return modified ? document.write() : sdp diff --git a/Sources/LiveKit/Support/SDP/SDP.swift b/Sources/LiveKit/Support/SDP/SDP.swift index 8d5d2529e..92ee039cf 100644 --- a/Sources/LiveKit/Support/SDP/SDP.swift +++ b/Sources/LiveKit/Support/SDP/SDP.swift @@ -171,6 +171,20 @@ struct SDPMediaSection { return true } + /// Removes the fmtp line of `payload`, if any. Returns `true` if the section was modified. + /// + /// The counterpart to ``setFmtp(config:forPayload:)`` for conforming a section to a codec + /// that is declared with no parameters at all: "no fmtp" is a config like any other, and a + /// section cannot be conformed to it by writing. + @discardableResult + mutating func removeFmtp(forPayload payload: String) -> Bool { + guard let index = lines.indices.first(where: { + Self.fmtp(fromLine: lines[$0])?.payload == payload + }) else { return false } + lines.remove(at: index) + return true + } + /// Whether an `a=rtcp-fb: ` line with exactly `value` exists for /// `payload`. Exact-value match: `nack` does not match `nack pli` (RFC 4585 §4.2 /// — they are distinct feedback parameters). diff --git a/Tests/LiveKitCoreTests/SDP/BundledCodecFmtpTests.swift b/Tests/LiveKitCoreTests/SDP/BundledCodecFmtpTests.swift index 5b0f48f7c..b64ead11c 100644 --- a/Tests/LiveKitCoreTests/SDP/BundledCodecFmtpTests.swift +++ b/Tests/LiveKitCoreTests/SDP/BundledCodecFmtpTests.swift @@ -137,6 +137,22 @@ struct BundledCodecFmtpTests { == once.replacingOccurrences(of: "a=recvonly", with: "a=sendrecv")) } + /// A payload a sending section declares with no fmtp at all is canonical in that state, so a + /// receive section carrying parameters for it is stripped rather than left to collide. The + /// rewrite only visits receive sections, so recording the absence is the only way this + /// resolves. + @Test func sendingSectionWithoutFmtpStripsTheReceiveSection() { + let senderBare = Self.singlePCOffer + .replacingOccurrences(of: "a=fmtp:100 x-google-start-bitrate=900\r\n", with: "") + let sections = SDP(parsing: Transport.conformBundledCodecFmtp(senderBare)).mediaSections + + // Sending section (mid 2) declares 100 with no fmtp; the receive section must match. + #expect(sections[2].fmtp(forPayload: "100") == nil) + #expect(sections[3].fmtp(forPayload: "100") == nil) + #expect(Self.fmtpConfigsByPayload(Transport.conformBundledCodecFmtp(senderBare)) + .allSatisfy { $0.value.count == 1 }) + } + /// The pipeline the publisher actually runs: the start-bitrate munge diverges the shared /// payload types, the conform pass puts them back. Expressed as the invariant libwebrtc /// enforces — one fmtp config per payload type across the BUNDLE group — rather than as From b173289780d1ffda31ef7491ea3c7f83d2b9f6e0 Mon Sep 17 00:00:00 2001 From: shijing xian Date: Mon, 2 Nov 2026 12:51:05 -0800 Subject: [PATCH 3/5] Address review on the bundled codec fmtp conform pass - sendingSectionWithoutFmtpStripsTheReceiveSection stripped the fmtp from both sections, so the receive section never had one and the removeFmtp branch was never exercised. It now keeps the sender bare and gives the receive section parameters, over both section orders, and fails with that branch deleted (verified). - Two sending sections that disagree now resolve to the first rather than the last, so what the receive sections follow does not depend on section order. Covered by firstSendingSectionWinsWhenSendersDisagree, which fails under the old last-wins rule. - Nothing pinned the wiring: deleting the conformBundledCodecFmtp call from negotiate failed no test. singlePCOfferIsBundleConsistentEndToEnd drives createAndSendOffer on a single PC transport with a real start-bitrate sender and asserts both the signalled offer and localDescription carry one fmtp config per payload. It fails with the call removed (verified). - Dropped the receive-vs-receive convergence pass, its early-out and its test: no munge makes two receive sections disagree. mungeOpusStereo writes to every audio section uniformly and the start bitrate only touches sending video. - isSendingVideoSection defers to isReceiveOnlySection, so the direction rule has one home; docstring trimmed, and it no longer claims BUNDLE-group scope for a pass that works document-wide. - fmtpConfigsByPayload is shared rather than copied between the two suites. Co-Authored-By: Claude Opus 5 --- Sources/LiveKit/Core/Transport.swift | 61 ++++++-------- .../SDP/BundledCodecFmtpTests.swift | 83 +++++++++++++------ .../SDP/TransportMungeFallbackTests.swift | 54 +++++++++--- 3 files changed, 125 insertions(+), 73 deletions(-) diff --git a/Sources/LiveKit/Core/Transport.swift b/Sources/LiveKit/Core/Transport.swift index cb934bab3..3d435d813 100644 --- a/Sources/LiveKit/Core/Transport.swift +++ b/Sources/LiveKit/Core/Transport.swift @@ -550,7 +550,7 @@ extension Transport { /// pre-populated or unpublished section lands; `sendonly`, `sendrecv` and an omitted /// direction (which defaults to `sendrecv`, RFC 8866 §6.7) all send. nonisolated static func isSendingVideoSection(_ section: SDPMediaSection) -> Bool { - section.mediaType == "video" && section.direction != .recvonly && section.direction != .inactive + section.mediaType == "video" && !isReceiveOnlySection(section) } /// The single start bitrate for the peer connection described by `document`: the largest @@ -618,39 +618,31 @@ extension Transport { section.direction == .recvonly || section.direction == .inactive } - /// Munge the offer so every payload type carries identical fmtp parameters across the BUNDLE - /// group, by conforming the sections that cannot send to the ones that can. + /// Munge the offer so every payload type carries identical fmtp parameters document-wide, + /// by conforming the sections that cannot send to the ones that can. /// /// A payload type is bundle-wide, so libwebrtc expects it to map to identical codec /// parameters in every m-line and reports "A BUNDLE group contains a codec collision - /// between ..." where it does not. Single PC mode creates exactly that divergence: - /// ``mungeVideoStartBitrate(_:kbpsBySenderId:)`` writes `x-google-start-bitrate` on the - /// sections that send video, while the receive sections preallocated for subscribed media - /// share those payload types without it. + /// between ..." where it does not. ``mungeVideoStartBitrate(_:kbpsBySenderId:)`` creates + /// exactly that divergence: it writes `x-google-start-bitrate` on the sections that send + /// video, while the receive sections of single PC mode share those payload types without it. /// - /// On the local-description path that report is a log, not an enforcement: the description - /// is accepted, the munge is not dropped, and the hint reaches the peer connection either - /// way. This runs because an SDP whose bundle is self-inconsistent is still wrong, and - /// because what libwebrtc tolerates here it does not everywhere — the same divergence in - /// the server's *answer* is reported on every single-PC negotiation, which this cannot - /// reach, since conforming an answer means selecting sections by transceiver rather than by - /// direction (an answer mirrors it) and munging a remote description. + /// On the local-description path that report is a log, not an enforcement. This runs because + /// an SDP whose bundle is self-inconsistent is still wrong, and because what libwebrtc + /// tolerates here it does not everywhere. It cannot reach the same divergence in the + /// server's *answer*, which would mean selecting sections by transceiver and munging a + /// remote description. /// - /// Sending sections always win, so a published track's encoder parameters are never altered. - /// Where no sending section declares a payload type, the receive sections converge on the - /// first config seen so two of them cannot disagree either. It is codec-agnostic by design: - /// it conforms whatever diverged rather than `x-google-start-bitrate` specifically, which is - /// what keeps it correct as other munges are added — ``mungeOpusStereoForAllAudio(_:)`` only - /// avoids the same fate by writing `stereo=1` to receive sections too. + /// Sending sections always win, so a published track's encoder parameters are never altered; + /// where two of them disagree the first wins, which at least keeps the receive sections + /// consistent with one of them. Codec-agnostic by design: it conforms whatever diverged + /// rather than `x-google-start-bitrate` specifically. /// /// Must run last, after every munge that can introduce divergence. /// - /// Mirrors `conformBundledCodecFmtp` in client-sdk-js, with two differences. That version - /// selects the sections to rewrite by transceiver — those with no outgoing track — where - /// this reads the direction the SDP already states, which libwebrtc derives from that same - /// state and which keeps the munge a pure `String -> String` like its siblings. And it only - /// rewrites fmtp lines that exist, where this inserts one when a receive section declares - /// the payload in an `a=rtpmap` but has no fmtp for it; "parameters vs. no parameters" is a + /// Mirrors `conformBundledCodecFmtp` in client-sdk-js, which selects sections by transceiver + /// where this reads the direction the SDP already states, and which only rewrites fmtp lines + /// that exist where this also inserts and removes them — "parameters vs. no parameters" is a /// collision too, and it is the one VP8 hits, since libwebrtc offers it without an fmtp line. nonisolated static func conformBundledCodecFmtp(_ sdp: String) -> String { var document = SDP(parsing: sdp) @@ -662,19 +654,16 @@ extension Transport { // exactly as two different parameter sets do, so the absence has to be recorded or // a receive section's fmtp would become canonical for a payload the sender leaves // bare — and the rewrite below, which only touches receive sections, could never - // resolve it. A sending section overwrites unconditionally, so it wins regardless - // of the order the sections appear in. - for rtpmap in section.rtpmaps { + // resolve it. + // + // First writer wins. Two sending sections can disagree — libwebrtc offers that + // after a second video publish — and neither can be rewritten, so the collision + // between them stands either way; picking the first at least makes what the receive + // sections are conformed to independent of section order. + for rtpmap in section.rtpmaps where canonicalByPayload[rtpmap.payload] == nil { canonicalByPayload[rtpmap.payload] = section.fmtp(forPayload: rtpmap.payload)?.config ?? "" } } - for section in document.mediaSections where isReceiveOnlySection(section) { - for fmtp in section.fmtps where canonicalByPayload[fmtp.payload] == nil { - canonicalByPayload[fmtp.payload] = fmtp.config - } - } - guard !canonicalByPayload.isEmpty else { return sdp } - var modified = false for index in document.mediaSections.indices where isReceiveOnlySection(document.mediaSections[index]) { // Keyed off `a=rtpmap` rather than the existing fmtp lines: a payload the section diff --git a/Tests/LiveKitCoreTests/SDP/BundledCodecFmtpTests.swift b/Tests/LiveKitCoreTests/SDP/BundledCodecFmtpTests.swift index b64ead11c..956a533f9 100644 --- a/Tests/LiveKitCoreTests/SDP/BundledCodecFmtpTests.swift +++ b/Tests/LiveKitCoreTests/SDP/BundledCodecFmtpTests.swift @@ -91,18 +91,27 @@ struct BundledCodecFmtpTests { "x-google-start-bitrate=900"]) } - /// With no sending section to copy from, the receive sections still converge on one - /// config — two of them disagreeing is a bundle collision just the same. - @Test func receiveSectionsConvergeWithoutASendingSection() { - let receiveOnly = Self.singlePCOffer - .replacingOccurrences(of: "a=fmtp:96 packetization-mode=1;x-google-start-bitrate=900\r\n", with: "") - .replacingOccurrences(of: "a=fmtp:100 x-google-start-bitrate=900\r\n", with: "") - .replacingOccurrences(of: "a=sendonly", with: "a=recvonly") - - let sections = SDP(parsing: Transport.conformBundledCodecFmtp(receiveOnly)).mediaSections - - #expect(Set(sections[0].fmtps.map(\.config)) == Set(sections[1].fmtps.map(\.config))) - #expect(sections[2].fmtps.map(\.config) == sections[3].fmtps.map(\.config)) + /// Two sending sections can disagree — libwebrtc offers that after a second video publish. + /// Neither is rewritten, so the collision between them stands; what this pins is that the + /// receive sections are conformed to the first of them whichever order they appear in. + @Test(arguments: [false, true]) + func firstSendingSectionWinsWhenSendersDisagree(reversed: Bool) { + var document = SDP(parsing: Self.singlePCOffer) + // A second sending video section declaring payload 100 with a different config. + let second = SDP(parsing: Self.singlePCOffer).mediaSections[2] + var rival = second + _ = rival.setFmtp(config: "x-google-start-bitrate=250", forPayload: "100") + document.mediaSections.insert(rival, at: 3) + if reversed { document.mediaSections.swapAt(2, 3) } + + let sections = SDP(parsing: Transport.conformBundledCodecFmtp(document.write())).mediaSections + let receiving = try? #require(sections.last { $0.direction == .recvonly }) + let firstSenderConfig = sections.first { !($0.direction == .recvonly || $0.direction == .inactive) + && $0.mediaType == "video" + }?.fmtp(forPayload: "100")?.config + + #expect(receiving?.fmtp(forPayload: "100")?.config == firstSenderConfig, + "receive sections must follow the first sending section, not the last") } /// `inactive` is the other direction a section with no outgoing track lands on — an @@ -137,20 +146,42 @@ struct BundledCodecFmtpTests { == once.replacingOccurrences(of: "a=recvonly", with: "a=sendrecv")) } - /// A payload a sending section declares with no fmtp at all is canonical in that state, so a - /// receive section carrying parameters for it is stripped rather than left to collide. The - /// rewrite only visits receive sections, so recording the absence is the only way this + /// Both orders of a bare sender and a parameterised receive section. The sender's *absence* + /// of fmtp is canonical, so the receive section's line is removed rather than adopted — + /// the rewrite only visits receive sections, so recording the absence is the only way this /// resolves. - @Test func sendingSectionWithoutFmtpStripsTheReceiveSection() { - let senderBare = Self.singlePCOffer - .replacingOccurrences(of: "a=fmtp:100 x-google-start-bitrate=900\r\n", with: "") - let sections = SDP(parsing: Transport.conformBundledCodecFmtp(senderBare)).mediaSections - - // Sending section (mid 2) declares 100 with no fmtp; the receive section must match. - #expect(sections[2].fmtp(forPayload: "100") == nil) - #expect(sections[3].fmtp(forPayload: "100") == nil) - #expect(Self.fmtpConfigsByPayload(Transport.conformBundledCodecFmtp(senderBare)) - .allSatisfy { $0.value.count == 1 }) + @Test(arguments: [false, true]) + func sendingSectionWithoutFmtpStripsTheReceiveSection(receiveSectionFirst: Bool) throws { + // Sender (mid 2) bare for payload 100; receive section (mid 3) carries parameters for it. + var offer = Self.singlePCOffer + .replacingOccurrences(of: "a=rtpmap:100 VP8/90000\r\na=fmtp:100 x-google-start-bitrate=900\r\na=sendonly", + with: "a=rtpmap:100 VP8/90000\r\na=sendonly") + .replacingOccurrences(of: "a=rtpmap:100 VP8/90000\r\na=recvonly", + with: "a=rtpmap:100 VP8/90000\r\na=fmtp:100 x-google-start-bitrate=900\r\na=recvonly") + + if receiveSectionFirst { + offer = try Self.swappingLastTwoVideoSections(offer) + } + + // The divergence the conform pass has to resolve: payload 100 is declared bare by one + // section and parameterised by the other. + #expect(Self.fmtpConfigsByPayload(offer)["100"] == ["", "x-google-start-bitrate=900"]) + + let conformed = Transport.conformBundledCodecFmtp(offer) + #expect(Self.fmtpConfigsByPayload(conformed)["100"] == [""], + "the receive section's fmtp:100 must be removed to match the bare sender") + #expect(Self.fmtpConfigsByPayload(conformed).allSatisfy { $0.value.count == 1 }) + } + + /// Reorders the document so the receive video section precedes the sending one, which is a + /// layout libwebrtc produces when a receive section is negotiated before a publish. + private static func swappingLastTwoVideoSections(_ sdp: String) throws -> String { + let document = SDP(parsing: sdp) + let videoIndices = document.mediaSections.indices.filter { document.mediaSections[$0].mediaType == "video" } + let (first, second) = (try #require(videoIndices.first), try #require(videoIndices.last)) + var reordered = document + reordered.mediaSections.swapAt(first, second) + return reordered.write() } /// The pipeline the publisher actually runs: the start-bitrate munge diverges the shared @@ -168,7 +199,7 @@ struct BundledCodecFmtpTests { /// Every fmtp config each payload type is given across the document, keyed by payload. A /// payload declared by `a=rtpmap` but given no fmtp line contributes the empty config, /// since "parameters" and "no parameters" collide just as two different parameter sets do. - private static func fmtpConfigsByPayload(_ sdp: String) -> [String: Set] { + static func fmtpConfigsByPayload(_ sdp: String) -> [String: Set] { var result: [String: Set] = [:] for section in SDP(parsing: sdp).mediaSections { for rtpmap in section.rtpmaps { diff --git a/Tests/LiveKitCoreTests/SDP/TransportMungeFallbackTests.swift b/Tests/LiveKitCoreTests/SDP/TransportMungeFallbackTests.swift index 8cebb2076..af1fccb1d 100644 --- a/Tests/LiveKitCoreTests/SDP/TransportMungeFallbackTests.swift +++ b/Tests/LiveKitCoreTests/SDP/TransportMungeFallbackTests.swift @@ -232,17 +232,49 @@ struct TransportMungeFallbackTests { } } - /// Every payload type's distinct fmtp configs across the document, keyed by payload. A - /// payload declared by `a=rtpmap` with no fmtp line contributes the empty config, since - /// libwebrtc counts "parameters" against "no parameters" as a collision too. - private func fmtpConfigsByPayload(_ sdp: String) -> [String: Set] { - var result: [String: Set] = [:] - for section in SDP(parsing: sdp).mediaSections { - for rtpmap in section.rtpmaps { - result[rtpmap.payload, default: []].insert(section.fmtp(forPayload: rtpmap.payload)?.config ?? "") + /// The wiring, not the munge: `createAndSendOffer` on a single PC transport must put the + /// conform pass in the chain, so deleting that call fails here rather than only showing up + /// as a collision log in a benchmark run. + @Test func singlePCOfferIsBundleConsistentEndToEnd() async throws { + let transport = try await Transport(config: .liveKitDefault(), + target: .publisher, + primary: true, + singlePCMode: true, + delegate: StubTransportDelegate()) + do { + // The sender is added with a start-bitrate hint, so the offer genuinely diverges: + // without one there is nothing for the conform pass to repair and this would pass + // with the call deleted. + try await RTC.run { + let receive = LKRTCRtpTransceiverInit() + receive.direction = .recvOnly + for _ in 0 ..< 3 { + _ = try transport.addTransceiver(ofType: .video, transceiverInit: receive) + } + let send = LKRTCRtpTransceiverInit() + send.direction = .sendOnly + let track = RTC.createVideoTrack(source: RTC.createVideoSource(forScreenShare: false)) + _ = try transport.addTransceiver(with: track, transceiverInit: send, startBitrateKbps: 1000) } + + let offers = OfferBox() + await transport.set(onOfferBlock: { offer, _ in await offers.append(offer) }) + try await transport.createAndSendOffer() + + let signalled = try #require(await offers.all.last) + let applied = try #require(await transport.localDescription) + + #expect(Transport.carriesVideoStartBitrate(signalled.sdp), + "the start bitrate must still reach the offer") + for sdp in [signalled.sdp, applied.sdp] { + #expect(BundledCodecFmtpTests.fmtpConfigsByPayload(sdp).allSatisfy { $0.value.count == 1 }, + "the offer the SDK sends must carry one fmtp config per payload type") + } + } catch { + await transport.close() + throw error } - return result + await transport.close() } /// What the conform pass exists for, on the shipped libwebrtc: with receive sections sharing @@ -263,7 +295,7 @@ struct TransportMungeFallbackTests { #expect(applied !== offer) #expect(Transport.carriesVideoStartBitrate(applied.sdp)) - #expect(fmtpConfigsByPayload(applied.sdp).contains { $0.value.count > 1 }) + #expect(BundledCodecFmtpTests.fmtpConfigsByPayload(applied.sdp).contains { $0.value.count > 1 }) } } @@ -281,7 +313,7 @@ struct TransportMungeFallbackTests { #expect(applied !== offer) #expect(Transport.carriesVideoStartBitrate(applied.sdp)) - #expect(fmtpConfigsByPayload(applied.sdp).allSatisfy { $0.value.count == 1 }) + #expect(BundledCodecFmtpTests.fmtpConfigsByPayload(applied.sdp).allSatisfy { $0.value.count == 1 }) let sending = try #require(SDP(parsing: applied.sdp).mediaSections.first { $0.msidTrackId == senderId }) for rtpmap in sending.rtpmaps where Transport.startBitrateCodecs.contains(rtpmap.codec.uppercased()) { #expect(sending.fmtp(forPayload: rtpmap.payload)?.parameters.contains("x-google-start-bitrate=1000") == true, From 19b54e5c4298c5701d188ffb36ae2f0f02edb0b3 Mon Sep 17 00:00:00 2001 From: shijing xian Date: Mon, 2 Nov 2026 13:10:59 -0800 Subject: [PATCH 4/5] Hoist try out of the tuple binding for swiftformat Co-Authored-By: Claude Opus 5 --- Tests/LiveKitCoreTests/SDP/BundledCodecFmtpTests.swift | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Tests/LiveKitCoreTests/SDP/BundledCodecFmtpTests.swift b/Tests/LiveKitCoreTests/SDP/BundledCodecFmtpTests.swift index 956a533f9..0dc1fdf0f 100644 --- a/Tests/LiveKitCoreTests/SDP/BundledCodecFmtpTests.swift +++ b/Tests/LiveKitCoreTests/SDP/BundledCodecFmtpTests.swift @@ -178,7 +178,8 @@ struct BundledCodecFmtpTests { private static func swappingLastTwoVideoSections(_ sdp: String) throws -> String { let document = SDP(parsing: sdp) let videoIndices = document.mediaSections.indices.filter { document.mediaSections[$0].mediaType == "video" } - let (first, second) = (try #require(videoIndices.first), try #require(videoIndices.last)) + let first = try #require(videoIndices.first) + let second = try #require(videoIndices.last) var reordered = document reordered.mediaSections.swapAt(first, second) return reordered.write() From 04448ca58672ada00ce80237c4f99839adfceaf6 Mon Sep 17 00:00:00 2001 From: shijing xian Date: Mon, 2 Nov 2026 14:51:15 -0800 Subject: [PATCH 5/5] Restore the receive-section fallback, with a reachable test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Devin flagged that removing it leaves two receive-only sections disagreeing with nothing to conform them to. That is correct, and it is also why the earlier "which munge writes to a receive section?" reasoning missed it: no munge does. A section that *stops sending* keeps the fmtp it was last munged with, so an unpublish leaves one receive-only section carrying x-google-start-bitrate beside one that never had it. Reachable because conform runs last and is therefore the first munge dropped when libwebrtc rejects a set, so an offer can carry the start bitrate unconformed. Driving that sequence on a real peer connection gives ["", "x-google-start-bitrate=1000"] across the two receive-only sections. The restored pass is keyed off a=rtpmap rather than existing fmtp lines, so a section declaring the payload bare can be canonical too — the asymmetry the original version had, and what Devin asked for. unpublishedSectionCanDivergeFromAReceiveSection drives the whole sequence instead of asserting on a fixture, and fails with the pass removed. The bundled-fmtp integration tests move into an extension so the suite stays within type_body_length. Co-Authored-By: Claude Opus 5 --- Sources/LiveKit/Core/Transport.swift | 15 ++++- .../SDP/TransportMungeFallbackTests.swift | 60 ++++++++++++++++++- 2 files changed, 73 insertions(+), 2 deletions(-) diff --git a/Sources/LiveKit/Core/Transport.swift b/Sources/LiveKit/Core/Transport.swift index 3d435d813..5d386eab1 100644 --- a/Sources/LiveKit/Core/Transport.swift +++ b/Sources/LiveKit/Core/Transport.swift @@ -635,7 +635,8 @@ extension Transport { /// /// Sending sections always win, so a published track's encoder parameters are never altered; /// where two of them disagree the first wins, which at least keeps the receive sections - /// consistent with one of them. Codec-agnostic by design: it conforms whatever diverged + /// consistent with one of them. Where no sending section declares a payload at all, the + /// receive sections converge on the first config among themselves. Codec-agnostic by design: it conforms whatever diverged /// rather than `x-google-start-bitrate` specifically. /// /// Must run last, after every munge that can introduce divergence. @@ -664,6 +665,18 @@ extension Transport { canonicalByPayload[rtpmap.payload] = section.fmtp(forPayload: rtpmap.payload)?.config ?? "" } } + // Where no sending section declares a payload, the receive sections settle on the first + // config seen. They can genuinely disagree: a section that stops sending keeps the fmtp + // it was last munged with, so an unpublish leaves a receive-only section carrying + // `x-google-start-bitrate` beside one that never had it — and with the sender gone there + // is nothing left to conform them to. Keyed off `a=rtpmap` for the same reason as the + // rewrite below, so a section that declares the payload bare can be canonical too. + for section in document.mediaSections where isReceiveOnlySection(section) { + for rtpmap in section.rtpmaps where canonicalByPayload[rtpmap.payload] == nil { + canonicalByPayload[rtpmap.payload] = section.fmtp(forPayload: rtpmap.payload)?.config ?? "" + } + } + var modified = false for index in document.mediaSections.indices where isReceiveOnlySection(document.mediaSections[index]) { // Keyed off `a=rtpmap` rather than the existing fmtp lines: a payload the section diff --git a/Tests/LiveKitCoreTests/SDP/TransportMungeFallbackTests.swift b/Tests/LiveKitCoreTests/SDP/TransportMungeFallbackTests.swift index af1fccb1d..f9ae4e6f7 100644 --- a/Tests/LiveKitCoreTests/SDP/TransportMungeFallbackTests.swift +++ b/Tests/LiveKitCoreTests/SDP/TransportMungeFallbackTests.swift @@ -214,9 +214,13 @@ struct TransportMungeFallbackTests { #expect(await transport.connectionSetupTime == 0.8) } } +} - // MARK: - Bundled codec fmtp +// MARK: - Bundled codec fmtp +/// The conform pass against a real peer connection: what libwebrtc actually reports, +/// and what the SDK actually sends, rather than what a fixture says. +extension TransportMungeFallbackTests { /// Adds the single PC publisher shape — receive sections preallocated for subscribed media /// alongside a sending one — and returns the sender id of the sending section. private func addSinglePCVideoLayout(to transport: Transport) async throws -> String { @@ -232,6 +236,60 @@ struct TransportMungeFallbackTests { } } + /// Two receive-only sections can disagree without any munge writing to one: a section that + /// stops sending keeps the fmtp it was last munged with, so an unpublish leaves one carrying + /// `x-google-start-bitrate` beside a receive section that never had it — and with the sender + /// gone, nothing is left to conform them to. + /// + /// Reachable because conform runs last and is therefore the first munge dropped when + /// libwebrtc rejects a set, so an offer can carry the start bitrate unconformed. This drives + /// that sequence rather than asserting it on a fixture. + @Test func unpublishedSectionCanDivergeFromAReceiveSection() async throws { + let transport = try await Transport(config: .liveKitDefault(), target: .publisher, + primary: true, singlePCMode: true, + delegate: StubTransportDelegate()) + let answerer = try await Transport(config: .liveKitDefault(), target: .subscriber, + primary: false, delegate: StubTransportDelegate()) + do { + let sender = try await RTC.run { () -> RTCSender in + let recv = LKRTCRtpTransceiverInit() + recv.direction = .recvOnly + _ = try transport.addTransceiver(ofType: .video, transceiverInit: recv) + let send = LKRTCRtpTransceiverInit() + send.direction = .sendOnly + let track = RTC.createVideoTrack(source: RTC.createVideoSource(forScreenShare: false)) + return try RTCSender(transport.addTransceiver(with: track, + transceiverInit: send, + startBitrateKbps: 1000).sender) + } + + // Start bitrate applied, conform not — the state a dropped conform leaves behind. + let first = try await transport.createOffer() + let munged = try await transport.set(localDescription: first, munging: [ + { Transport.mungeVideoStartBitrate($0, kbpsBySenderId: [sender.senderId: 1000]) }, + ]) + try await answerer.set(remoteDescription: munged) + let answer = try await answerer.createAnswer() + try await answerer.set(localDescription: answer) + try await transport.set(remoteDescription: answer) + + try await RTC.run { try transport.remove(track: sender) } + let unpublished = try await transport.createOffer().sdp + + #expect(BundledCodecFmtpTests.fmtpConfigsByPayload(unpublished).contains { $0.value.count > 1 }, + "the unpublish must leave a divergence for the conform pass to resolve") + #expect(BundledCodecFmtpTests.fmtpConfigsByPayload(Transport.conformBundledCodecFmtp(unpublished)) + .allSatisfy { $0.value.count == 1 }, + "with no sending section left, the receive sections must still converge") + } catch { + await transport.close() + await answerer.close() + throw error + } + await transport.close() + await answerer.close() + } + /// The wiring, not the munge: `createAndSendOffer` on a single PC transport must put the /// conform pass in the chain, so deleting that call fails here rather than only showing up /// as a collision log in a benchmark run.