chore: change audio direction based on the call's "on hold" status #37381
Conversation
…l's "on hold" status
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
WalkthroughAdds audio-transceiver direction management around WebRTC negotiation: new helpers to get audio transceivers and to update their directions before and after SDP offer/answer flows; integrates these into createOffer, createAnswer, and setRemoteDescription paths with logging. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Proc as MediaCallWebRTCProcessor
participant PC as RTCPeerConnection
participant T as Audio Transceivers
App->>Proc: createOffer()
Proc->>Proc: updateAudioDirectionBeforeNegotiation()
Proc->>T: set desired directions (sendrecv/sendonly)
Proc->>PC: createOffer()
PC-->>Proc: SDP offer
Proc->>PC: setLocalDescription(offer)
Proc->>Proc: updateAudioDirectionAfterNegotiation()
Proc->>T: reconcile directions
Proc-->>App: return offer
App->>Proc: setRemoteDescription(offer/answer)
Proc->>Proc: (if offer) updateAudioDirectionBeforeNegotiation()
Proc->>PC: setRemoteDescription()
Proc->>Proc: (if answer) updateAudioDirectionAfterNegotiation()
Proc-->>App: complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ 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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feat/voip-sync-audio-direction #37381 +/- ##
==================================================================
- Coverage 68.03% 67.85% -0.19%
==================================================================
Files 3361 3345 -16
Lines 115574 114470 -1104
Branches 20796 20721 -75
==================================================================
- Hits 78636 77668 -968
+ Misses 34254 34124 -130
+ Partials 2684 2678 -6
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
pierre-lehnen-rc
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
…into chore/sync-on-hold-audio-direction
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/media-signaling/src/lib/services/webrtc/Processor.ts(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: pierre-lehnen-rc
Repo: RocketChat/Rocket.Chat PR: 36718
File: packages/media-signaling/src/lib/Call.ts:633-642
Timestamp: 2025-09-23T00:27:05.438Z
Learning: In PR #36718, pierre-lehnen-rc prefers to maintain consistency with the old architecture patterns for DTMF handling rather than implementing immediate validation improvements, deferring enhancements to future work.
🔇 Additional comments (7)
packages/media-signaling/src/lib/services/webrtc/Processor.ts (7)
111-120: LGTM! Good refactoring to use the helper method.The refactoring to use
getAudioTransceivers()improves consistency, and the added logging will help with debugging.
122-123: Appropriate placement of direction update.Calling
updateAudioDirectionBeforeNegotiation()before ICE restart and offer creation correctly sets the desired direction based on current state.
173-173: LGTM! Consistent use of helper method.
195-199: LGTM! Proper reconciliation after answer.Calling
updateAudioDirectionAfterNegotiation()after setting the local answer appropriately reconciles the transceiver direction with the negotiated result.
213-221: LGTM! Correct direction management in remote description flow.The placement of direction updates is appropriate:
- Before processing a remote offer: set desired direction
- After processing a remote answer: reconcile with negotiated result
331-355: Reconciliation logic looks sound, pending verification of desired direction.The logic correctly avoids redundant renegotiations by adjusting
directionto matchcurrentDirectionwhen the actual result is acceptable. The method appropriately:
- Skips transceivers not set to
desiredDirection- Skips when
currentDirectionis null, 'stopped', or already matches- Updates
directionwhencurrentDirectionis anacceptableDirectionHowever, the correctness depends on the direction values set in
updateAudioDirectionBeforeNegotiation()(see previous comment).
357-361: No issues found with the audio transceiver filter.The filter correctly identifies audio transceivers in all scenarios. When
addTransceiver('audio')is called at line 117, the resulting transceiver'sreceiver.track.kindis set to'audio'immediately per the WebRTC specification, even before a sender track exists. The filter's OR logic (sender.track?.kind === 'audio' || receiver.track?.kind === 'audio') ensures newly created transceivers are caught by the receiver track condition. The optional chaining safely handles null sender tracks.
pierre-lehnen-rc
merged commit 465e541
into
feat/voip-sync-audio-direction
Proposed changes (including videos or screenshots)
Issue(s)
VGA-39
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Bug Fixes