feat(llc): handle ice restart sfu event, rollback pc#1175
feat(llc): handle ice restart sfu event, rollback pc#1175
Conversation
📝 WalkthroughWalkthroughThe changes implement ICE restart functionality for WebRTC peer connections, including SFU event handling, SDP validation and rollback mechanisms, enhanced error recovery in signaling flows, and improved renegotiation logic with comprehensive error handling. Changes
Sequence DiagramsequenceDiagram
participant SFU as SFU Server
participant CallSession as CallSession
participant PC as StreamPeerConnection
participant Offer as Remote Offer/Answer
SFU->>CallSession: SfuIceRestartEvent (peerType)
activate CallSession
alt Publisher Path
CallSession->>PC: restartIce()
activate PC
PC-->>CallSession: success
deactivate PC
CallSession->>CallSession: _onRenegotiationNeeded()
activate CallSession
CallSession->>PC: createOffer()
activate PC
PC-->>CallSession: Result<SessionDescription>
deactivate PC
alt SDP Valid
CallSession->>PC: setLocalDescription(offer)
PC-->>CallSession: Result
CallSession->>SFU: publisherConnection.setPublisher()
SFU-->>CallSession: RemoteAnswer
CallSession->>PC: setRemoteAnswer(answer)
PC-->>CallSession: Result
CallSession-->>CallSession: Return Result.success()
else SDP Invalid
CallSession-->>CallSession: Rollback & Return error
end
deactivate CallSession
else Subscriber Path
CallSession->>PC: restartIce()
activate PC
PC-->>CallSession: success
deactivate PC
end
deactivate CallSession
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/call/session/call_session.dart (1)
917-974:⚠️ Potential issue | 🟠 MajorReturn values from inside
_negotiationLock.synchronizedlambda are silently discarded.The
returnstatements on Lines 928, 940, 956, and 969 exit the closure passed tosynchronized(), not_onRenegotiationNeededitself. Since the result ofawait _negotiationLock.synchronized(...)is not captured, the method always reaches Line 973 and returnsResult.success(null), regardless of failures inside the lock.Currently no caller inspects the return value, so this has no functional impact yet. However, the
Future<Result<void>>signature is misleading — it promises meaningful error reporting that doesn't actually work.Fix by capturing and returning the synchronized result:
Proposed fix
- await _negotiationLock.synchronized(() async { + final lockResult = await _negotiationLock.synchronized<Result<void>>(() async { _logger.d(() => '[negotiate] type: ${pc.type}'); final offer = await pc.createOffer(); if (offer is! Success<rtc.RTCSessionDescription>) { return Result<void>.error( 'Failed to create offer: ${offer.getErrorOrNull()}', ); } // ... rest of lock body ... + return const Result<void>.success(null); }); - - return const Result.success(null); + return lockResult;
🧹 Nitpick comments (2)
packages/stream_video/CHANGELOG.md (1)
1-5: LGTM!Changelog entries clearly describe both changes. Minor nit: the iceRestart event handling reads more like a feature ("Added handling…") than a fix, but the section header says "Fixed". Consider using "✅ Added" for the first item if it's new functionality rather than a bug fix.
packages/stream_video/lib/src/call/session/call_session.dart (1)
804-838: Publisher ICE restart proceeds to renegotiation even ifrestartIce()fails.On Line 817,
publisher.restartIce()result is logged but not checked before triggering renegotiation on Line 824. If the ICE restart itself failed, proceeding to renegotiation may be wasteful or could produce confusing errors. Consider returning early on failure.Suggested improvement
_logger.i(() => '[onIceRestart] restarting ICE for publisher'); final result = await publisher.restartIce(); _logger.d(() => '[onIceRestart] publisher ICE restart result: $result'); + if (result.isFailure) { + _logger.w(() => '[onIceRestart] publisher ICE restart failed, skipping renegotiation'); + return; + } // After marking ICE restart, explicitly trigger renegotiation to create
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1175 +/- ##
========================================
- Coverage 6.47% 6.46% -0.01%
========================================
Files 601 601
Lines 42031 42094 +63
========================================
Hits 2723 2723
- Misses 39308 39371 +63 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
resolves FLU-376
Summary by CodeRabbit