-
Notifications
You must be signed in to change notification settings - Fork 3
fix(calls): harden video join without pairwise WebRTC churn #2310
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -132,6 +132,128 @@ type InboundRtpVideoStats = { | |
| ssrc?: number; | ||
| }; | ||
|
|
||
| type IceTransportSummary = { | ||
| iceConnectionState: RTCIceConnectionState; | ||
| iceGatheringState: RTCIceGatheringState; | ||
| connectionState: RTCPeerConnectionState; | ||
| localCandidateType?: string; | ||
| remoteCandidateType?: string; | ||
| pairState?: string; | ||
| inboundAudioBytes?: number; | ||
| inboundVideoBytes?: number; | ||
| }; | ||
|
|
||
| function readIceTransportSummary( | ||
| peerConnection: RTCPeerConnection, | ||
| stats: RTCStatsReport, | ||
| ): IceTransportSummary { | ||
| const candidates = new Map<string, string>(); | ||
| let selectedPairId: string | undefined; | ||
| let nominatedPairId: string | undefined; | ||
| let nominatedLocalCandidateId: string | undefined; | ||
| let nominatedRemoteCandidateId: string | undefined; | ||
|
|
||
| for (const report of stats.values()) { | ||
| if ( | ||
| report.type === 'local-candidate' || | ||
| report.type === 'remote-candidate' | ||
| ) { | ||
| const candidateType = report.candidateType; | ||
| if (typeof candidateType === 'string') { | ||
| candidates.set(report.id, candidateType); | ||
| } | ||
| continue; | ||
| } | ||
| if (report.type === 'transport' && 'selectedCandidatePairId' in report) { | ||
| const id = report.selectedCandidatePairId; | ||
| if (typeof id === 'string' && id.length > 0) { | ||
| selectedPairId = id; | ||
| } | ||
| continue; | ||
| } | ||
| if (report.type !== 'candidate-pair') continue; | ||
| if (report.nominated === true && report.state === 'succeeded') { | ||
| nominatedPairId = report.id; | ||
| nominatedLocalCandidateId = | ||
| typeof report.localCandidateId === 'string' | ||
| ? report.localCandidateId | ||
| : undefined; | ||
| nominatedRemoteCandidateId = | ||
| typeof report.remoteCandidateId === 'string' | ||
| ? report.remoteCandidateId | ||
| : undefined; | ||
| } | ||
| } | ||
|
|
||
| const pairId = selectedPairId ?? nominatedPairId; | ||
| let localCandidateType: string | undefined; | ||
| let remoteCandidateType: string | undefined; | ||
| let pairState: string | undefined; | ||
|
|
||
| if (pairId) { | ||
| const pair = stats.get(pairId); | ||
| if (pair?.type === 'candidate-pair') { | ||
| pairState = typeof pair.state === 'string' ? pair.state : undefined; | ||
| const localId = | ||
| typeof pair.localCandidateId === 'string' | ||
| ? pair.localCandidateId | ||
| : nominatedLocalCandidateId; | ||
| const remoteId = | ||
| typeof pair.remoteCandidateId === 'string' | ||
| ? pair.remoteCandidateId | ||
| : nominatedRemoteCandidateId; | ||
| if (localId) localCandidateType = candidates.get(localId); | ||
| if (remoteId) remoteCandidateType = candidates.get(remoteId); | ||
| } | ||
| } | ||
|
|
||
| let inboundAudioBytes = 0; | ||
| let inboundVideoBytes = 0; | ||
| stats.forEach((report) => { | ||
| if (report.type !== 'inbound-rtp') return; | ||
| const bytes = report.bytesReceived; | ||
| if (typeof bytes !== 'number' || bytes <= 0) return; | ||
| if (report.kind === 'audio') inboundAudioBytes += bytes; | ||
| if (report.kind === 'video') inboundVideoBytes += bytes; | ||
| }); | ||
|
|
||
| return { | ||
| iceConnectionState: peerConnection.iceConnectionState, | ||
| iceGatheringState: peerConnection.iceGatheringState, | ||
| connectionState: peerConnection.connectionState, | ||
| localCandidateType, | ||
| remoteCandidateType, | ||
| pairState, | ||
| inboundAudioBytes: inboundAudioBytes || undefined, | ||
| inboundVideoBytes: inboundVideoBytes || undefined, | ||
| }; | ||
| } | ||
|
|
||
| async function logIceTransportForPeerConnection(options: { | ||
| roomId: string; | ||
| groupCallId: string; | ||
| userId: string | null; | ||
| peerConnection: RTCPeerConnection; | ||
| }): Promise<void> { | ||
| const { roomId, groupCallId, userId, peerConnection } = options; | ||
| const stats = await peerConnection.getStats(); | ||
| const transport = readIceTransportSummary(peerConnection, stats); | ||
|
Comment on lines
+239
to
+240
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🌐 Web query:
💡 Result: The behavior of RTCPeerConnection.getStats when called on a closed PeerConnection varies by browser implementation, despite the W3C WebRTC Statistics API specification intending for stats to remain available even after a connection is closed [1][2]. According to the W3C specification, monitored objects should generally persist for the duration of the associated RTCPeerConnection, allowing their statistics to be accessed via getStats even after the connection has been closed [2]. However, implementation has historically been inconsistent across major browsers [1][3][4][5]. In Firefox, historically, calling getStats on a closed RTCPeerConnection resulted in a promise rejection with an InvalidStateError or similar DOMException [3][4]. Recent updates (circa April 2026) have moved toward allowing getStats on closed connections, mirroring the intended specification behavior and avoiding the previous rejection/hanging behavior [1][6][5]. In Chromium-based browsers, the behavior generally allows calling getStats on a closed connection, which typically returns a resolution with the final state of the statistics, rather than an error [5]. Due to these historical inconsistencies, many WebRTC applications use polyfills (such as those found in the adapter.js library) to normalize this behavior across browsers, often by guarding the call or manually resolving a promise with an empty map if the connection state is 'closed' [4]. Summary of error behavior: - Specification: Intends for stats to be accessible after closing; getStats should not reject solely because the connection is closed [2]. - Chromium: Generally allows the call and returns the final available statistics [5]. - Firefox: Historically rejected with an InvalidStateError, but recent versions have been updated to support calling getStats after closure without throwing [1][4][6]. If your application relies on consistent behavior, check the connectionState of the RTCPeerConnection before invoking getStats and handle the state accordingly [4]. Citations:
Handle
Proposed fix async function logIceTransportForPeerConnection(options: {
roomId: string;
groupCallId: string;
userId: string | null;
peerConnection: RTCPeerConnection;
}): Promise<void> {
const { roomId, groupCallId, userId, peerConnection } = options;
- const stats = await peerConnection.getStats();
- const transport = readIceTransportSummary(peerConnection, stats);
- logSpaceGroupCallEvent({
- name: 'hypha.group_call.ice_transport',
- roomId,
- groupCallId,
- remoteUserId: userId ?? undefined,
- iceConnectionState: transport.iceConnectionState,
- iceGatherState: transport.iceGatheringState,
- connectionState: transport.connectionState,
- localCandidateType: transport.localCandidateType,
- remoteCandidateType: transport.remoteCandidateType,
- pairState: transport.pairState,
- inboundAudioBytes: transport.inboundAudioBytes,
- inboundVideoBytes: transport.inboundVideoBytes,
- });
+ try {
+ const stats = await peerConnection.getStats();
+ const transport = readIceTransportSummary(peerConnection, stats);
+ logSpaceGroupCallEvent({
+ name: 'hypha.group_call.ice_transport',
+ roomId,
+ groupCallId,
+ remoteUserId: userId ?? undefined,
+ iceConnectionState: transport.iceConnectionState,
+ iceGatherState: transport.iceGatheringState,
+ connectionState: transport.connectionState,
+ localCandidateType: transport.localCandidateType,
+ remoteCandidateType: transport.remoteCandidateType,
+ pairState: transport.pairState,
+ inboundAudioBytes: transport.inboundAudioBytes,
+ inboundVideoBytes: transport.inboundVideoBytes,
+ });
+ } catch {
+ // Peer connection may close between interval ticks; ignore transient diagnostics failure.
+ }
}🤖 Prompt for AI Agents |
||
| logSpaceGroupCallEvent({ | ||
| name: 'hypha.group_call.ice_transport', | ||
| roomId, | ||
| groupCallId, | ||
| remoteUserId: userId ?? undefined, | ||
| iceConnectionState: transport.iceConnectionState, | ||
| iceGatherState: transport.iceGatheringState, | ||
| connectionState: transport.connectionState, | ||
| localCandidateType: transport.localCandidateType, | ||
| remoteCandidateType: transport.remoteCandidateType, | ||
| pairState: transport.pairState, | ||
| inboundAudioBytes: transport.inboundAudioBytes, | ||
| inboundVideoBytes: transport.inboundVideoBytes, | ||
| }); | ||
| } | ||
|
|
||
| export function readInboundRtpVideoFrameSizes( | ||
| stats: RTCStatsReport, | ||
| ): InboundRtpVideoStats[] { | ||
|
|
@@ -247,9 +369,28 @@ export function attachGroupCallWebRtcDiagnostics(options: { | |
| } | ||
| }; | ||
|
|
||
| const logIceTransport = () => { | ||
| if (!enumeratePeerConnections) return; | ||
| for (const { userId, peerConnection } of enumeratePeerConnections(gc)) { | ||
| void logIceTransportForPeerConnection({ | ||
| roomId, | ||
| groupCallId: gc.groupCallId, | ||
| userId, | ||
| peerConnection, | ||
| }); | ||
| } | ||
| }; | ||
|
|
||
| if (inboundRtpFrameLogIntervalMs > 0 && enumeratePeerConnections) { | ||
| logFrameSizes(); | ||
| frameLogInterval = setInterval(logFrameSizes, inboundRtpFrameLogIntervalMs); | ||
| logIceTransport(); | ||
| frameLogInterval = setInterval(() => { | ||
| logFrameSizes(); | ||
| logIceTransport(); | ||
| }, inboundRtpFrameLogIntervalMs); | ||
|
Comment on lines
+386
to
+390
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win Avoid double In the support-debug interval path, 🤖 Prompt for AI Agents |
||
| } else if (summaryStatsIntervalMs > 0 && enumeratePeerConnections) { | ||
| logIceTransport(); | ||
| frameLogInterval = setInterval(logIceTransport, summaryStatsIntervalMs); | ||
| } | ||
|
|
||
| return () => { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Restore
navigator.permissionsafter each test to prevent global-state bleed.These tests overwrite a global but never reset it, which can cause order-dependent failures as this file grows.
Suggested fix
🤖 Prompt for AI Agents