Repository navigation
fix(engine): ignore the mid mapping of a stale publisher answer - #2196
Conversation
🦋 Changeset detectedLatest commit: 3e13dc4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| // transport has no sending tracks) and must not clobber the subscriber offer mapping | ||
| if (this.pcManager.mode === 'publisher-only') { | ||
| // an answer to an older offer is ignored by the publisher transport, so its mapping | ||
| // is stale too and must not replace the one installed by the current answer | ||
| if ( | ||
| this.pcManager.mode === 'publisher-only' && | ||
| !this.pcManager.publisher.isStaleAnswer(offerId) | ||
| ) { | ||
| this.midToTrackId = midToTrackId; |
There was a problem hiding this comment.
question: Is there a reason that the stale answer check couldn't be an if with an early return at the top of onAnswer? I'd think this would be cleaner as I don't think there's a case where a late answer should be processed normally.
There was a problem hiding this comment.
Good call, no: a stale answer is dropped by PCTransport.setRemoteDescription anyway, so nothing in it should be processed. In 3e13dc4 onAnswer now checks publisher.isStaleAnswer(offerId) first and returns early (with the same "ignoring answer for old offer" warning), so it no longer reaches setPublisherAnswer or emits RequestSubscribedCodecRefresh either. Added a test for that.
| // is stale too and must not replace the one installed by the current answer | ||
| if ( | ||
| this.pcManager.mode === 'publisher-only' && | ||
| !this.pcManager.publisher.isStaleAnswer(offerId) |
There was a problem hiding this comment.
curiously on your thought on the following problem ？
A current, non-stale answer carrying an empty mapping does this.midToTrackId = {} and destroys the map just as a stale one does. SignalClient.ts:981 passes midToTrackId unconditionally and protobuf-es gives {} for an empty map, so the assignment is a real wipe, and getTrackIdForReceiver has no fallback.
rust guards this explicitly (!answer.mid_to_track_id.is_empty(), rtc_session.rs:1354); Swift merges, so empty is a no-op. JS has neither, before or after this PR.
There was a problem hiding this comment.
Agreed, that is the same wipe through another path. In 3e13dc4 a current answer only replaces the mapping when it is non-empty, matching the Rust guard (!answer.mid_to_track_id.is_empty()). Keeping the old entries is harmless: a mid is only looked up for a receiver the SFU described, and any answer that adds a receiver carries a non-empty mapping that replaces them. Added a test where an answer to the latest offer with {} keeps the existing mapping. I kept it to replace-when-non-empty rather than the Swift merge so the change stays small; happy to switch to a merge if you prefer.
Fixes #2195
Problem
In single peer connection mode (
publisher-only),client.onAnswerreplacedmidToTrackIdwith the answer's mapping before callingpcManager.setPublisherAnswer.PCTransport.setRemoteDescriptionthen ignores an answer whose offer id is older thanlatestOfferId("ignoring answer for old offer"), but by then the stale mapping had already replaced the one from the current answer. Atrackevent that reaches the SDK after that (react-native-webrtc dispatches them late) can't resolve its mid ingetTrackIdForReceiver, soRoom.onTrackAddedlogs "Tried to add a track whose 'sid' could not be found" and the track is dropped.Change
PCTransport.isStaleAnswer(offerId)holds the existing "answer to an older offer" check, andsetRemoteDescriptionuses it, so behaviour there is unchanged.onAnswerreturns early for a stale answer (same "ignoring answer for old offer" warning), so neither its mapping norsetPublisherAnswer/RequestSubscribedCodecRefreshare processed (review feedback).{}, which would wipe the mapping the same way (review feedback). The mapping is still installed beforesetRemoteDescription, becausetrackfires inside that call.Dual peer connection mode is unaffected.
Tests
New
publisher answer mid mappingtests inRTCEngine.test.ts:main)setPublisherAnswer, no codec refresh)vitest run src/room/RTCEngine.test.ts src/room/PCTransport*(121 passed),tsc --noEmit, eslint and prettier pass. Includes a patch changeset.This PR was prepared with AI assistance and reviewed/tested by the author.