Drop the inactive-to-recvonly offer munge by xianshijing-lk · Pull Request #1190 · livekit/client-sdk-swift

xianshijing-lk · 2026-10-30T22:43:15Z

Swift port of livekit/rust-sdks#1610, which removes the same rewrite there.

Why rust needed it, and why this SDK never did

rust-sdks passed libwebrtc's legacy offer_to_receive_audio/_video as 0. That makes libwebrtc stop every receiving transceiver — a=inactive in the offer and Inactive on the transceiver itself — so it rewrote a=inactive back to a=recvonly on each single PC offer. client-sdk-android passes the same option today (RTCEngine.getPublisherOfferConstraints). client-sdk-js never has, and has no such rewrite.

This SDK never passes it either. The only media constraint it builds anywhere is DtlsSrtpKeyAgreement. The rewrite was imported from rust without the cause, and its docstring — "WebRTC can generate inactive direction even when transceivers were configured as recvonly" — is simply not true on the pinned build.

Verified against real libwebrtc rather than inferred (RecvOnlyOfferDirectionTests):

scenario	offer	transceiver
3 audio + 3 video `recvonly` transceivers	6/6 `a=recvonly`	—
after the peer answers `inactive` on every section	still `a=recvonly`	`RecvOnly`

That matches rust#1610's own "unset" row, including the persistence case.

It was not inert

There is a second way a section reaches inactive: unpublish. That is the only case where this munge actually fired, and there it rewrote the section of a transceiver removeTrack had already stopped (isStopped == true):

offer after removeTrack:        a=inactive
same offer, munged:             a=recvonly   <- munge fires
transceiver after removeTrack:  Stopped, isStopped = true

So it advertised a willingness to receive on a transceiver that can never deliver media — the same SDP-versus-transceiver divergence rust#1610 describes, reached from the other direction. Removing it is a fix, not only a cleanup.

Behaviour change

Unpublished sections now stay a=inactive instead of being advertised a=recvonly. That is the half the server observes, so it is covered end-to-end by SinglePCRepublishTests: publish → unpublish → publish again, subscriber renders frames both times.

Testing

RecvOnlyOfferDirectionTests — 3 new tests against real libwebrtc, pinning why the munge is unnecessary and why it was harmful. If a future WebRTC bump reintroduces inactive for receive-only transceivers these fail loudly, and the fix is the offer options, not a munge.
SinglePCRepublishTests — new E2E unpublish/republish round trip.
Removed SDPMungingTests.swift (all 3 tests existed only for this munge) and 3 tests from TransportSDPMungeTests.swift, keeping the shared fixture still used by the stereo test.
LiveKitCoreTests: 586 tests, only droppingTheBinderDetaches — a pre-existing deinit-timing flake that passes in isolation and also fails on clean main.
Staging benchmarks, 6 variants / 102 samples, 0 failed iterations. Single PC is unchanged end-to-end: 1 pub firstSub/firstFrame 400/578 → 398/572; 5 pub 416/565 → 414/562. No Failed to set local description anywhere, so no munge was ever being rejected.

Not covered

iOS was build-checked only — the libwebrtc behaviour above was probed on macOS. The probe is cheap to re-run on a device if a reviewer wants it before merge.

🤖 Generated with Claude Code

Ported from rust-sdks#1610, which removes the same rewrite there. rust-sdks passed libwebrtc's legacy `offer_to_receive_audio`/`_video` as 0. That makes libwebrtc stop every receiving transceiver — `inactive` in the offer and `Inactive` on the transceiver itself — so it rewrote `a=inactive` back to `a=recvonly` on every single PC offer. client-sdk-android passes the same option in `getPublisherOfferConstraints`. This SDK never passes it. The only constraint it builds is `DtlsSrtpKeyAgreement`, so receive-only transceivers already offer `recvonly`, and stay `recvonly` even after the server answers `inactive` for sections it has no media for. The rewrite was imported without the cause. It was not inert, though. The one case where it fired was a section whose transceiver `removeTrack` had stopped: it rewrote that section's `inactive` to `recvonly`, advertising a willingness to receive on a stopped transceiver that can never deliver media. That is the same SDP-versus-transceiver divergence rust-sdks#1610 describes, reached from the other direction. `RecvOnlyOfferDirectionTests` pins all three behaviours against real libwebrtc, so a future bump that reintroduces `inactive` fails loudly rather than silently needing a munge back. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

The removed munge only ever rewrote the section an unpublish leaves behind, so that round trip is the one the removal can regress. Pins that the subscriber still renders frames after the publisher unpublishes and publishes again. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

createSampleVideoTrack decodes 1080p at 30fps. Two of those overlapped RoomTests.resourcesCleanUp, which allows one second for a disconnected room to deallocate — and those releases are blocking WebRTC destructors on the shared RTC.park queue, so saturating it made an unrelated suite fail. A 320x240 buffer at 10fps proves the same thing, and each cycle now stops its feeder and unpublishes before the next begins. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

devin-ai-integration

🔍 Devin Review: 1 flag

Not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)

Git history and RecvOnlyOfferDirectionTests already carry this. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

The subscriber's view of the unpublished publication can outlive the unpublish, so polling for *a* video track let the second cycle attach its renderer to the dead one and time out. Each cycle now excludes the SID the previous one saw. Also drops a comment that restated set(localDescription:munging:)'s own docstring; the load-bearing clause it used to carry was the claim about the direction rewrite, which is gone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Two things that only macOS tolerated: - `.timeLimit` needs iOS 16 and the package deploys to iOS 13, so the republish suite failed to compile on every non-macOS leg. TestLimits.e2e exists for exactly this and resolves the trait at runtime; the suite is an e2e one and should have carried that tag anyway. - swiftformat's wrapLoopBodies on the recvonly helper. The repo lints with swiftformat as well as swiftlint. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

pblazej

The production change is right, and it fixes a real bug: on main, a track subscribed after a local unpublish can land in the dead section and never render. Only the regression guard blocks, because both new suites still pass with the munge restored. This also lands before #1163, which still names the munge.

pblazej · 2026-11-01T20:57:31Z

+
+        try await RTC.run { try transport.remove(track: sender) }
+
+        let second = try await transport.createOffer()


issue (blocking,test): This reads the raw createOffer(), where the munge never runs, so it passes with the munge restored. Capture the offer from createAndSendOffer() with set(onOfferBlock:), as writesTheStartBitrateOnceForTheConnection does.

pblazej · 2026-11-01T20:57:31Z

+            try await Self.publishAwaitFramesUnpublish("after-republish",
+                                                       publisher: rooms[0],
+                                                       subscriber: rooms[1],
+                                                       excluding: firstSid)


issue (blocking,test): The publisher never subscribes after it unpublishes, so this passes on main behavior too. Have it subscribe to another participant's video and render a frame. That scenario fails 2/2 with the munge and passes 2/2 without it.

pblazej · 2026-11-01T20:57:31Z

+            let firstSid = try await Self.publishAwaitFramesUnpublish("before-unpublish",
+                                                                      publisher: rooms[0],
+                                                                      subscriber: rooms[1])
+            try await Task.sleep(nanoseconds: 1_000_000_000)


suggestion (non-blocking,test): Wait on the shared poll from LiveKitTestSupport instead of a fixed sleep and the file-private copy.

pblazej · 2026-11-01T20:57:31Z

+            options: BufferCaptureOptions(dimensions: Dimensions(width: Int32(width), height: Int32(height))),
+        )
+        let capturer = try #require(track.capturer as? BufferCapturer)
+        let feeder = Task.detached {


issue (non-blocking): If a later try throws, nothing cancels this feeder. Add defer { feeder.cancel() }, or use capturer.startFeedingFrames(dimensions:) and drop the pixel-buffer helper.

pblazej · 2026-11-01T20:57:31Z

@@ -0,0 +1 @@
+patch type="fixed" "Single peer connection offers no longer rewrite `a=inactive` to `a=recvonly`. The rewrite was ported from rust-sdks, where it compensated for passing libwebrtc's legacy `offerToReceive` option as 0; this SDK never passes that option, so receive-only transceivers already offer `recvonly`. Its only effect here was to advertise `recvonly` for sections whose transceiver `removeTrack` had stopped, inviting media that section could never receive"


suggestion (non-blocking): Name the symptom: a track subscribed after a local unpublish in single PC mode did not play.

xianshijing-lk and others added 3 commits October 30, 2026 14:42

xianshijing-lk requested review from hiroshihorie and pblazej as code owners October 30, 2026 22:43

devin-ai-integration Bot reviewed Oct 30, 2026

View reviewed changes

xianshijing-lk and others added 4 commits October 30, 2026 15:49

Drop the tombstone comment for the removed munge

7000ef0

Git history and RecvOnlyOfferDirectionTests already carry this. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Extract the republish helper to satisfy function_body_length

ae62882

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

pblazej requested changes Nov 1, 2026

View reviewed changes

Navigation Menu

Repository navigation

Drop the inactive-to-recvonly offer munge - #1190

Drop the inactive-to-recvonly offer munge#1190
xianshijing-lk wants to merge 7 commits into
mainfrom
sxian/drop-inactive-recvonly-munge

xianshijing-lk commented Oct 30, 2026

Uh oh!

devin-ai-integration Bot left a comment

Uh oh!

pblazej left a comment

Uh oh!

pblazej Nov 1, 2026

Uh oh!

pblazej Nov 1, 2026

Uh oh!

pblazej Nov 1, 2026

Uh oh!

pblazej Nov 1, 2026

Uh oh!

pblazej Nov 1, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants


		try await RTC.run { try transport.remove(track: sender) }

		let second = try await transport.createOffer()

		@@ -0,0 +1 @@
		patch type="fixed" "Single peer connection offers no longer rewrite `a=inactive` to `a=recvonly`. The rewrite was ported from rust-sdks, where it compensated for passing libwebrtc's legacy `offerToReceive` option as 0; this SDK never passes that option, so receive-only transceivers already offer `recvonly`. Its only effect here was to advertise `recvonly` for sections whose transceiver `removeTrack` had stopped, inviting media that section could never receive"

Conversation

xianshijing-lk commented Oct 30, 2026

Why rust needed it, and why this SDK never did

It was not inert

Behaviour change

Testing

Not covered

Uh oh!

devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

🔍 Devin Review: 1 flag

Uh oh!

pblazej left a comment

Choose a reason for hiding this comment

Uh oh!

pblazej Nov 1, 2026

Choose a reason for hiding this comment

Uh oh!

pblazej Nov 1, 2026

Choose a reason for hiding this comment

Uh oh!

pblazej Nov 1, 2026

Choose a reason for hiding this comment

Uh oh!

pblazej Nov 1, 2026

Choose a reason for hiding this comment

Uh oh!

pblazej Nov 1, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants