chore: add audio level and access to call stats on media-signaling lib#37160
Conversation
|
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 level tracking and stats retrieval to media calls. Extends interfaces with audioLevel, localAudioLevel, and getStats(selector?). ClientMediaCall exposes getters and getStats delegating to the WebRTC processor. Processor implements periodic getStats polling to update audio levels and manages lifecycle registration/unregistration. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant ClientMediaCall
participant WebRTCProcessor
participant RTCPeerConnection as RTCPeerConnection
Note over WebRTCProcessor: Initialization
WebRTCProcessor->>WebRTCProcessor: registerAudioLevelTracker()
loop interval tick
WebRTCProcessor->>RTCPeerConnection: getStats(selector?)
RTCPeerConnection-->>WebRTCProcessor: RTCStatsReport \| null
WebRTCProcessor->>WebRTCProcessor: parse audio reports<br/>update audioLevel/localAudioLevel (or 0 on error)
end
App->>ClientMediaCall: getStats(selector?)
ClientMediaCall->>WebRTCProcessor: getStats(selector?)
WebRTCProcessor-->>ClientMediaCall: RTCStatsReport \| null
ClientMediaCall-->>App: RTCStatsReport \| null
App->>ClientMediaCall: read audioLevel/localAudioLevel
ClientMediaCall->>WebRTCProcessor: audioLevel/localAudioLevel
WebRTCProcessor-->>ClientMediaCall: numbers
Note over WebRTCProcessor: Stop
App->>WebRTCProcessor: stop()
WebRTCProcessor->>WebRTCProcessor: unregisterAudioLevelTracker()<br/>reset levels
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/media-signaling/src/lib/services/webrtc/Processor.ts (3)
237-244: Consider using selector to reduce getStats loadWhen available, pass the specific audio track to limit report size; this reduces overhead versus full-graph stats.
334-366: Fix stats parsing and reduce polling overhead; current filter likely misses inbound audio
- Correctness: Many browsers expose audio level on 'media-source' and/or 'track'; inbound-rtp may not have kind, and may use mediaType. The current
report.kind !== 'audio'filter will skip valid inbound reports, keeping audioLevel at 0.- Performance: Polling the full stats graph every 50ms per call is heavy.
Suggested improvements:
- Parse per type and check mediaType for inbound-rtp; include 'track' fallback with 'audioLevel'.
- Add minimal casting to satisfy TS DOM typings.
- Gate updates by connected state.
- Reduce interval (e.g., 200ms). Optionally pass a selector track to getStats to narrow results.
- this._audioLevelTracker = setInterval(() => { - this.getStats() + this._audioLevelTracker = setInterval(() => { + // Avoid noisy polling when not connected + if (this.peer.connectionState !== 'connected') { + this._audioLevel = 0; + this._localAudioLevel = 0; + return; + } + + this.getStats() .then((stats) => { if (!stats) { return; } - stats.forEach((report) => { - if (report.kind !== 'audio') { - return; - } - - switch (report.type) { - case 'inbound-rtp': - this._audioLevel = report.audioLevel ?? 0; - break; - case 'media-source': - this._localAudioLevel = report.audioLevel ?? 0; - break; - } - }); + (stats as any).forEach((report: any) => { + switch (report.type) { + case 'inbound-rtp': { + // Chrome/Safari: mediaType; some don't expose `kind` here + if (report.kind === 'audio' || report.mediaType === 'audio') { + this._audioLevel = report.audioLevel ?? 0; + } + break; + } + case 'media-source': { + // Local mic level + if (report.kind === 'audio') { + this._localAudioLevel = report.audioLevel ?? 0; + } + break; + } + case 'track': { + // Fallback: some browsers report audioLevel on track stats + if (report.kind === 'audio' && 'audioLevel' in report) { + if (report.remoteSource) { + this._audioLevel = report.audioLevel ?? this._audioLevel; + } else { + this._localAudioLevel = report.audioLevel ?? this._localAudioLevel; + } + } + break; + } + } + }); }) .catch(() => { this._audioLevel = 0; this._localAudioLevel = 0; }); - }, 50); + }, 200);Optionally, you can call
getStats(this.remoteMediaStream.getAudioTracks()[0] ?? undefined as any)and a second pass for the local track to further shrink the report size.
368-377: Good cleanup on stop; consider also resetting when connection failsUnregistering the tracker and zeroing levels on stop is good. Also consider zeroing when the connection transitions to failed/closed.
📜 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 (4)
packages/media-signaling/src/definition/call/IClientMediaCall.ts(2 hunks)packages/media-signaling/src/definition/services/webrtc/IWebRTCProcessor.ts(1 hunks)packages/media-signaling/src/lib/Call.ts(2 hunks)packages/media-signaling/src/lib/services/webrtc/Processor.ts(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/media-signaling/src/lib/services/webrtc/Processor.ts (1)
packages/media-signaling/src/definition/services/webrtc/IWebRTCProcessor.ts (1)
WebRTCProcessorConfig(40-46)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (4)
packages/media-signaling/src/definition/call/IClientMediaCall.ts (1)
96-96: LGTM on getStats surfaceSignature matches processor; nullable return is a pragmatic choice.
packages/media-signaling/src/lib/Call.ts (2)
160-166: LGTM: safe gettersCleanly expose processor levels with 0 fallback when unavailable.
652-654: LGTM: getStats delegationNull on missing/stopped processor is reasonable.
packages/media-signaling/src/lib/services/webrtc/Processor.ts (1)
52-65: LGTM: audio level fields and accessorsInitialization and public getters look good.
Also applies to: 71-74
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #37160 +/- ##
===========================================
+ Coverage 67.35% 67.39% +0.03%
===========================================
Files 3286 3286
Lines 111584 111584
Branches 20366 20377 +11
===========================================
+ Hits 75158 75200 +42
+ Misses 33740 33702 -38
+ Partials 2686 2682 -4
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Proposed changes (including videos or screenshots)
Issue(s)
VGA-30
Steps to test or reproduce
Further comments
Summary by CodeRabbit