fix(llc): publisher track mid mismatch - #1291
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughPublisher track announcements now resolve MIDs from live transceivers, publisher SDP, or cached acknowledgements. Unresolved MIDs cause rollback and negotiation failure, while sender-aware cache matching supports reconnect announcements. Tests cover resolution, recovery, and cache behavior. ChangesPublisher track announcements
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CallSession
participant RtcManager
participant PeerConnection
participant LocalSdp
CallSession->>RtcManager: request announced tracks
RtcManager->>PeerConnection: fetch live transceivers
RtcManager->>LocalSdp: read publisher local SDP
RtcManager->>RtcManager: resolve track MID
RtcManager-->>CallSession: return track info or unresolved result
CallSession->>PeerConnection: roll back unresolved local offer
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1291 +/- ##
==========================================
+ Coverage 11.37% 11.57% +0.20%
==========================================
Files 686 686
Lines 50408 50441 +33
==========================================
+ Hits 5734 5841 +107
+ Misses 44674 44600 -74 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/stream_video/lib/src/call/session/call_session.dart`:
- Around line 1093-1102: Recheck the call/session connection state after
acquiring and waiting on _negotiationLock, immediately before the fallback in
the initial publisher-offer failure path. If the session is already disconnected
or closing, return without invoking onReconnectionNeeded; only trigger
SfuReconnectionStrategy.rejoin for an active session, preserving the existing
rollback behavior.
In `@packages/stream_video/lib/src/webrtc/rtc_manager.dart`:
- Around line 654-662: Update resolveTrackMid’s SDP media matching to split each
m-section’s msid into tokens and compare trackId only against the exact track-id
token, not via contains. Preserve the existing MID return behavior, and add
regression coverage for a prefix track ID and an ID appearing only in the
stream-id token.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ecbdd0fd-78e7-4bcc-b929-57bd297d7136
📒 Files selected for processing (3)
packages/stream_video/CHANGELOG.mdpackages/stream_video/lib/src/call/session/call_session.dartpackages/stream_video/lib/src/webrtc/rtc_manager.dart
renefloor
left a comment
renefloor
left a comment
There was a problem hiding this comment.
🔴 The recovery Result is thrown away — _onRenegotiationNeeded always returns success
Every return the PR adds is inside _negotiationLock.synchronized(() async { ... }), so it returns from the closure, not from _onRenegotiationNeeded. call_session.dart:1150 unconditionally returns const Result.success(null). This predates the PR (the old Result.error('Failed to create offer') was equally discarded), but the PR is built entirely around returning recovery results, so it needs fixing here.
Consequence — startPublisherConnectionCheck at call_session.dart:638-646:
final result = await _onRenegotiationNeeded(publisher);
if (result.isFailure && !_isLeavingOrClosed) { ... rejoin ... } // dead branch
The have-local-offer stall path can never escalate to rejoin. That's one of the two stalls this PR claims to fix, and it's still broken. Same for the onIceRestart failure logging at line 974.
return await _negotiationLock.synchronized<Result>(() async {
...
return const Result.success(null); // explicit success at the end of the closure
});
Note the interaction once fixed: _recoverFailedPublisherNegotiation returning Result.success(null) after firing onReconnectionNeeded becomes load-bearing — it's what stops line 645 from firing a second rejoin. That's a defensible choice but it's currently implicit; please comment it.
🟠 getTransceivers() can throw, and neither call site is protected
getAnnouncedTracks is invoked at call_session.dart:1111 — before the try at line 1122 — and createOffer has already set the local description by then. The native impl throws a bare String on PlatformException, and RTCRtpSenderNative.fromMap does Map<dynamic, dynamic> trackMap = map['track']; (non-nullable) — a TypeError if the platform omits track. Any throw escapes _onRenegotiationNeeded entirely: no rollback, no rejoin, publisher wedged in have-local-offer. That is precisely the failure mode this PR set out to eliminate.
The reconnect path is worse: getReconnectDetails (call_session.dart:172) awaits getAnnouncedTracksForReconnect, which now also calls getTransceivers(). On a publisher pc that's already closing during a rejoin, a throw here aborts the whole reconnect rather than degrading.
Wrap it:
final liveTransceivers = await publisher?.pc.getTransceivers().onError((_, __) => []) ?? [];
…or move getAnnouncedTracks inside the existing try in _onRenegotiationNeeded.
🟡 Dropping the type == kind check makes the SDP fallback fail closed and silent
Old predicate: m['type'] == track.kind && ((m['msid'] as String?)?.contains(track.id!) ?? true) — the ?? true meant "no msid on this m-section → match on kind alone." New predicate requires msid present and containing the trackId, else null → the track is dropped from the announce and therefore never published.
I think failing closed is the right call (announcing a wrong mid is worse than announcing nothing), but two asks:
- Keep m['type'] == kind as an additional guard on the msid match. It's free and protects against the msid appearing in a recycled/inactive m-section that .reversed might land on.
- The drop is only a _logger.w. The user-visible symptom is "my camera isn't being published" with no error surfaced anywhere. Add a _tracer.trace so it shows up in telemetry — you already trace publisherConnectionCheckStalled for comparable conditions.
🟡 No tests
resolveTrackMid is the most testable function in this PR — pure over a transceiver list plus an SDP string, and test/src/webrtc/peer_connection_renegotiation_test.dart already has the mock-PC scaffolding. Worth covering:
- live mid wins over SDP
- empty live mid → SDP msid fallback resolves
- .reversed picks the last matching m-section (the recycled-m-line case that motivated this fix)
- msid absent → null (pins the new fail-closed contract)
- mediaTrack.id == null → null
Plus one on the new escalation: empty tracksInfo + no remote description → onReconnectionNeeded(rejoin).
🔵 Minor
- Dead code: TransceiverManager.indexOf (transceiver_cache.dart:114) and its backing _transceiverOrder list are now unused — rtc_manager.dart:703 was the only caller. Both should go in this PR.
- resolveTrackMid(TransceiverCache cache, ...) reads only cache.track.mediaTrack.id. Taking String? trackId would be a more honest signature and trivially testable.
- _recoverFailedPublisherNegotiation logs 'initial publisher offer failed' for all three call sites, including setPublisher failure. Pass a reason string in.
- parse(sdp) runs per unresolved track inside the loop. Rare in practice (step 1 usually wins, since createOffer sets the local description before mids are read), so low priority — but hoisting the parse into the two getAnnouncedTracks* callers would remove it entirely.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/stream_video/lib/src/webrtc/rtc_manager.dart (1)
637-660: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFix the substring
msidmatch and add a regression test for it. The root cause isresolveTrackMid's SDP fallback matchingtrackIdviaString.containsinstead of an exact token comparison, which can select an unrelated m-section when one id is a substring of another.
packages/stream_video/lib/src/webrtc/rtc_manager.dart#L637-L660: replace(m['msid'] as String?)?.contains(trackId)with an exact-token comparison (splitmsidon whitespace and compare the track-id token), as previously proposed.packages/stream_video/test/src/webrtc/rtc_manager_mid_resolution_test.dart#L120-L137: add a case where avideoTrackIdis a prefix/substring of another m-section'smsid(stream-id or track-id token) to lock in the exact-match fix and prevent regression.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/stream_video/lib/src/webrtc/rtc_manager.dart` around lines 637 - 660, The resolveTrackMid SDP fallback in packages/stream_video/lib/src/webrtc/rtc_manager.dart:637-660 must match trackId as an exact whitespace-delimited msid token rather than using substring matching; add a regression case in packages/stream_video/test/src/webrtc/rtc_manager_mid_resolution_test.dart:120-137 where one videoTrackId is a prefix or substring of another m-section’s msid and verify the correct m-section is selected.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/stream_video/lib/src/webrtc/rtc_manager.dart`:
- Around line 637-660: The resolveTrackMid SDP fallback in
packages/stream_video/lib/src/webrtc/rtc_manager.dart:637-660 must match trackId
as an exact whitespace-delimited msid token rather than using substring
matching; add a regression case in
packages/stream_video/test/src/webrtc/rtc_manager_mid_resolution_test.dart:120-137
where one videoTrackId is a prefix or substring of another m-section’s msid and
verify the correct m-section is selected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9a661810-8143-4c34-9e3c-2a0087568f18
📒 Files selected for processing (2)
packages/stream_video/lib/src/webrtc/rtc_manager.dartpackages/stream_video/test/src/webrtc/rtc_manager_mid_resolution_test.dart
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/stream_video/lib/src/call/session/call_session.dart (1)
1173-1183: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMissing rollback on
setRemoteAnswerfailure, unlike the siblingsetPublisherfailure branch.The
pubResultfailure path just above rolls back before returning an error (Line 1167:await pc.rollbackLocalDescription();). This branch only returns an error, leavingpcwedged inhave-local-offerwith no rollback attempt. Recovery then depends entirely on thepublisherConnectionCheckwatchdog picking up the stall later, rather than surfacing/self-healing immediately like the other failure paths in this function.Proposed fix
if (pubResult.data.hasSdp()) { final ansResult = await pc.setRemoteAnswer(pubResult.data.sdp); if (ansResult is! Success<void>) { _logger.w( () => '[negotiate] `#setRemoteAnswer`; failed: $ansResult', ); + await pc.rollbackLocalDescription(); return Result<void>.error( 'Failed to set remote answer: ${ansResult.getErrorOrNull()}', ); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/stream_video/lib/src/call/session/call_session.dart` around lines 1173 - 1183, Update the setRemoteAnswer failure branch in the negotiation flow to await pc.rollbackLocalDescription() before returning the existing error result, matching the rollback behavior of the preceding pubResult failure path and preserving the current logging and error propagation.packages/stream_video/lib/src/webrtc/rtc_manager.dart (1)
950-992: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winStale
negotiatedflag lets republished audio/video tracks skip renegotiation. BothpublishAudioTrackandpublishVideoTrackattach a freshMediaStreamTrackclone to a cached, already-negotiated transceiver viareplaceTrack, but only force renegotiationif (cachedBundle != null && !cachedBundle.negotiated). SinceTransceiverManager.update()never resetsnegotiated, an already-negotiated transceiver silently keeps sending the SFU's old trackId while new media flows over the same mid — the same "published while sending... incorrect media" defect class this PR targets elsewhere.
packages/stream_video/lib/src/webrtc/rtc_manager.dart#L950-L992: inpublishAudioTrack, force renegotiation whenever the cached transceiver's track is swapped, not only when previously unnegotiated (or fix centrally, see below).packages/stream_video/lib/src/webrtc/rtc_manager.dart#L1054-L1088: apply the identical fix inpublishVideoTrack.packages/stream_video/lib/src/webrtc/transceiver_cache.dart#L81-L92(root-cause fix, not directly in this diff): resetbundle.negotiated = falseinTransceiverManager.update()whenever a newtrackis provided, so every consumer of the flag (including_renegotiateIfUnacknowledged) sees an accurate state.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/stream_video/lib/src/webrtc/rtc_manager.dart` around lines 950 - 992, Reset the cached bundle’s negotiated state in TransceiverManager.update() whenever a new track is supplied, covering packages/stream_video/lib/src/webrtc/transceiver_cache.dart#L81-L92. In publishAudioTrack at packages/stream_video/lib/src/webrtc/rtc_manager.dart#L950-L992 and publishVideoTrack at packages/stream_video/lib/src/webrtc/rtc_manager.dart#L1054-L1088, ensure replacing a cached transceiver track triggers renegotiation based on the updated state rather than allowing a stale negotiated flag to suppress it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/stream_video/lib/src/call/session/call_session.dart`:
- Around line 1173-1183: Update the setRemoteAnswer failure branch in the
negotiation flow to await pc.rollbackLocalDescription() before returning the
existing error result, matching the rollback behavior of the preceding pubResult
failure path and preserving the current logging and error propagation.
In `@packages/stream_video/lib/src/webrtc/rtc_manager.dart`:
- Around line 950-992: Reset the cached bundle’s negotiated state in
TransceiverManager.update() whenever a new track is supplied, covering
packages/stream_video/lib/src/webrtc/transceiver_cache.dart#L81-L92. In
publishAudioTrack at
packages/stream_video/lib/src/webrtc/rtc_manager.dart#L950-L992 and
publishVideoTrack at
packages/stream_video/lib/src/webrtc/rtc_manager.dart#L1054-L1088, ensure
replacing a cached transceiver track triggers renegotiation based on the updated
state rather than allowing a stale negotiated flag to suppress it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8d430a33-fd99-4308-a157-0b6265dbd7dd
📒 Files selected for processing (9)
packages/stream_video/CHANGELOG.mdpackages/stream_video/lib/src/call/session/call_session.dartpackages/stream_video/lib/src/call/stats/trace_tag.dartpackages/stream_video/lib/src/webrtc/rtc_manager.dartpackages/stream_video/lib/src/webrtc/transceiver_cache.dartpackages/stream_video/test/src/call/session/call_session_announce_test.dartpackages/stream_video/test/src/webrtc/rtc_manager_announced_tracks_test.dartpackages/stream_video/test/src/webrtc/rtc_manager_mid_resolution_test.dartpackages/stream_video/test/src/webrtc/transceiver_cache_test.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/stream_video/test/src/webrtc/rtc_manager_mid_resolution_test.dart
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/stream_video/lib/src/webrtc/rtc_manager.dart`:
- Line 831: Update _transceiverToTrackInfo so transceiverCache.negotiatedMid is
used as lastKnownMid only for getAnnouncedTracksForReconnect, while normal
getAnnouncedTracks calls pass no fallback and return null when the current MID
cannot be resolved. Add a regression test covering a previously negotiated track
that cannot resolve during a normal announcement.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 42ccb68a-6d3d-4972-895e-7c0fb27ecf65
📒 Files selected for processing (6)
packages/stream_video/lib/src/call/session/call_session.dartpackages/stream_video/lib/src/webrtc/rtc_manager.dartpackages/stream_video/lib/src/webrtc/transceiver_cache.dartpackages/stream_video/test/src/webrtc/rtc_manager_announced_tracks_test.dartpackages/stream_video/test/src/webrtc/rtc_manager_mid_resolution_test.dartpackages/stream_video/test/src/webrtc/transceiver_cache_test.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/stream_video/lib/src/call/session/call_session.dart
2 participants
Fixes publisher-negotiation bugs that could leave a track "published" but sending no/wrong media.
The announced track mid was read from a cached transceiver and could drift from the offer after a renegotiation/retry, so the SFU couldn't match the track to its codec. It's now resolved from the live peer-connection state to always match the offer being sent.
Summary by CodeRabbit