From 1a9f8f5e123e0a3289472d60a6bb4a3cf8483491 Mon Sep 17 00:00:00 2001 From: Henry Smith <36210679+hensmi-amazon@users.noreply.github.com> Date: Wed, 2 Oct 2024 15:32:10 -0700 Subject: [PATCH] Include STUN packets in received packet reconnection check to mitigate edge cases when all attendee capabilities are None (#2958) --- CHANGELOG.md | 1 + .../signalingandmetricsconnectionmonitor.html | 14 +++--- .../SignalingAndMetricsConnectionMonitor.ts | 31 ++++++++++--- ...gnalingAndMetricsConnectionMonitor.test.ts | 43 +++++++++++++++++-- 4 files changed, 73 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1795415ef2..cab640b959 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- Include STUN packets in received packet reconnection check to mitigate edge cases when all attendee capabilities are 'None' - Terminate audio RED worker before removing reference. ## [3.24.0] - 2024-07-11 diff --git a/docs/classes/signalingandmetricsconnectionmonitor.html b/docs/classes/signalingandmetricsconnectionmonitor.html index 40bd8aff3e..bed4d46017 100644 --- a/docs/classes/signalingandmetricsconnectionmonitor.html +++ b/docs/classes/signalingandmetricsconnectionmonitor.html @@ -117,7 +117,7 @@

constructor

  • Parameters

    @@ -156,7 +156,7 @@

    didMissPongs

    @@ -180,7 +180,7 @@

    didReceivePong

    @@ -216,7 +216,7 @@

    metricsDidReceive

    @@ -244,7 +244,7 @@

    receiveSignalStrengthChange

  • Parameters

    @@ -268,7 +268,7 @@

    start

    @@ -291,7 +291,7 @@

    stop

    diff --git a/src/connectionmonitor/SignalingAndMetricsConnectionMonitor.ts b/src/connectionmonitor/SignalingAndMetricsConnectionMonitor.ts index 8f906e8a15..88df0783b2 100644 --- a/src/connectionmonitor/SignalingAndMetricsConnectionMonitor.ts +++ b/src/connectionmonitor/SignalingAndMetricsConnectionMonitor.ts @@ -17,6 +17,7 @@ export default class SignalingAndMetricsConnectionMonitor private isActive = false; private hasSeenValidCandidatePairMetricsBefore = false; private lastTotalBytesReceived = 0; + private lastTotalStunMessagesReceived = 0; constructor( private audioVideoController: AudioVideoController, @@ -94,21 +95,41 @@ export default class SignalingAndMetricsConnectionMonitor // so we just use the raw report for now. const webrtcReport = clientMetricReport.getRTCStatsReport(); + // We use candidate pair bytes received as a proxy for packets received + // since not all versions of all browsers have 'packetsReceived' for candidate pairs let totalBytesReceived = 0; + // We additionally check STUN messages received, in case backend is not sending RTP/RTCP + // which may happen in None attendee capability edge cases. + let totalStunMessagedReceived = 0; webrtcReport.forEach(stat => { - if (stat.type === 'candidate-pair' && stat.bytesReceived) { - totalBytesReceived += stat.bytesReceived; + if (stat.type === 'candidate-pair') { + if (stat.bytesReceived) { + totalBytesReceived += stat.bytesReceived; + } + if (stat.requestsReceived) { + totalStunMessagedReceived += stat.requestsReceived; + } + if (stat.responsesReceived) { + totalStunMessagedReceived += stat.responsesReceived; + } } }); const bytesReceived = totalBytesReceived - this.lastTotalBytesReceived; this.lastTotalBytesReceived = totalBytesReceived; + const stunMessagesReceived = totalStunMessagedReceived - this.lastTotalStunMessagesReceived; + this.lastTotalStunMessagesReceived = totalStunMessagedReceived; if ( typeof potentialAudioPacketsReceived === 'number' && typeof potentialAudioFractionPacketsLostInbound === 'number' ) { audioPacketsReceived = potentialAudioPacketsReceived; audiofractionPacketsLostInbound = potentialAudioFractionPacketsLostInbound; - if (audioPacketsReceived < 0 || audiofractionPacketsLostInbound < 0 || bytesReceived < 0) { + if ( + audioPacketsReceived < 0 || + audiofractionPacketsLostInbound < 0 || + bytesReceived < 0 || + stunMessagesReceived < 0 + ) { // The stats collector or logic above may emit negative numbers on this metric after reconnect // which we should not use. return; @@ -125,9 +146,7 @@ export default class SignalingAndMetricsConnectionMonitor audiofractionPacketsLostInbound ); - // We use candidate pair bytes received as a proxy for packets received - // since not all versions of all browsers have 'packetsReceived' for candidate pairs - if (bytesReceived > 0) { + if (bytesReceived > 0 || stunMessagesReceived > 0) { this.hasSeenValidCandidatePairMetricsBefore = true; this.connectionHealthData.setConsecutiveStatsWithNoPackets(0); } else if (this.hasSeenValidCandidatePairMetricsBefore) { diff --git a/test/connectionmonitor/SignalingAndMetricsConnectionMonitor.test.ts b/test/connectionmonitor/SignalingAndMetricsConnectionMonitor.test.ts index d93720b50b..3f536273a7 100644 --- a/test/connectionmonitor/SignalingAndMetricsConnectionMonitor.test.ts +++ b/test/connectionmonitor/SignalingAndMetricsConnectionMonitor.test.ts @@ -124,6 +124,8 @@ describe('SignalingAndMetricsConnectionMonitor', () => { videoDownstreamFrameWidth: RawMetrics = 100; audioPacketsSent: RawMetrics = 50; totalBytesReceived: number = 0; + totalRequestsReceived: number = 0; + totalResponsesReceived: number = 0; getObservableMetrics(): { [id: string]: number } { return { @@ -155,6 +157,8 @@ describe('SignalingAndMetricsConnectionMonitor', () => { type: 'candidate-pair', ...{ bytesReceived: this.totalBytesReceived, + requestsReceived: this.totalRequestsReceived, + responsesReceived: this.totalResponsesReceived, }, }, ], @@ -164,6 +168,8 @@ describe('SignalingAndMetricsConnectionMonitor', () => { type: 'candidate-pair', ...{ bytesReceived: 0, + requestsReceived: 0, + responsesReceived: 0, }, }, ], @@ -319,7 +325,7 @@ describe('SignalingAndMetricsConnectionMonitor', () => { expect(lastPacketLossInboundTimestampMsCalled).to.equal(false); }); - it('can return without changing stats when total bytes received is negative', () => { + it('can return without changing stats when total packets received is negative', () => { testClientMetricReport.totalBytesReceived = 4; testClientMetricReport.fractionLoss = 0; testClientMetricReport.audioPacketsReceived = 1; @@ -333,7 +339,7 @@ describe('SignalingAndMetricsConnectionMonitor', () => { expect(lastPacketLossInboundTimestampMsCalled).to.equal(false); }); - it('can reset and increment consecutive stats with no bytes when bytes received are followed by no bytes', () => { + it('can reset and increment consecutive stats with no packets when packets received are followed by no packets', () => { testClientMetricReport.totalBytesReceived = 1; testClientMetricReport.fractionLoss = 0; testClientMetricReport.audioPacketsReceived = 1; @@ -341,14 +347,45 @@ describe('SignalingAndMetricsConnectionMonitor', () => { expect(consecutiveStatsWithNoPackets).to.equal(0); testClientMetricReport.totalBytesReceived = 1; testClientMetricReport.fractionLoss = 0; - testClientMetricReport.audioPacketsReceived = 0; + testClientMetricReport.audioPacketsReceived = 1; sendClientMetricReport(testClientMetricReport); expect(consecutiveStatsWithNoPackets).to.equal(1); testClientMetricReport.totalBytesReceived = 1; testClientMetricReport.fractionLoss = 0; + testClientMetricReport.audioPacketsReceived = 1; + sendClientMetricReport(testClientMetricReport); + expect(consecutiveStatsWithNoPackets).to.equal(2); + }); + + it('can reset and increment consecutive stats with no packets when stun packets received are followed by no packets', () => { + testClientMetricReport.totalBytesReceived = 0; + testClientMetricReport.totalRequestsReceived = 1; + testClientMetricReport.totalResponsesReceived = 0; + testClientMetricReport.fractionLoss = 0; + testClientMetricReport.audioPacketsReceived = 0; + sendClientMetricReport(testClientMetricReport); + expect(consecutiveStatsWithNoPackets).to.equal(0); + testClientMetricReport.totalBytesReceived = 0; + testClientMetricReport.totalRequestsReceived = 1; + testClientMetricReport.totalResponsesReceived = 0; + testClientMetricReport.fractionLoss = 0; + testClientMetricReport.audioPacketsReceived = 0; + sendClientMetricReport(testClientMetricReport); + expect(consecutiveStatsWithNoPackets).to.equal(1); + testClientMetricReport.totalBytesReceived = 0; + testClientMetricReport.totalRequestsReceived = 1; + testClientMetricReport.totalResponsesReceived = 0; + testClientMetricReport.fractionLoss = 0; testClientMetricReport.audioPacketsReceived = 0; sendClientMetricReport(testClientMetricReport); expect(consecutiveStatsWithNoPackets).to.equal(2); + testClientMetricReport.totalBytesReceived = 0; + testClientMetricReport.totalRequestsReceived = 1; + testClientMetricReport.totalResponsesReceived = 1; + testClientMetricReport.fractionLoss = 0; + testClientMetricReport.audioPacketsReceived = 0; + sendClientMetricReport(testClientMetricReport); + expect(consecutiveStatsWithNoPackets).to.equal(0); }); it('can set last packet loss inbound timestamp due to no packets', () => {