Include STUN packets in received packet reconnection check to mitigat…

…e edge cases when all attendee capabilities are None (#2958)
aws · Oct 2, 2024 · 1a9f8f5 · 1a9f8f5
1 parent 5cacbb6
commit 1a9f8f5
Show file tree

Hide file tree

Showing 4 changed files with 73 additions and 16 deletions.
diff --git 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
@@ -117,7 +117,7 @@ <h3>constructor</h3>
 						<li class="tsd-description">
 							<aside class="tsd-sources">
 								<ul>
-									<li>Defined in <a href="https://github.com/aws/amazon-chime-sdk-js/blob/main/src/connectionmonitor/SignalingAndMetricsConnectionMonitor.ts#L19">src/connectionmonitor/SignalingAndMetricsConnectionMonitor.ts:19</a></li>
+									<li>Defined in <a href="https://github.com/aws/amazon-chime-sdk-js/blob/main/src/connectionmonitor/SignalingAndMetricsConnectionMonitor.ts#L20">src/connectionmonitor/SignalingAndMetricsConnectionMonitor.ts:20</a></li>
 								</ul>
 							</aside>
 							<h4 class="tsd-parameters-title">Parameters</h4>
@@ -156,7 +156,7 @@ <h3>did<wbr>Miss<wbr>Pongs</h3>
 							<aside class="tsd-sources">
 								<p>Implementation of <a href="../interfaces/pingpongobserver.html">PingPongObserver</a>.<a href="../interfaces/pingpongobserver.html#didmisspongs">didMissPongs</a></p>
 								<ul>
-									<li>Defined in <a href="https://github.com/aws/amazon-chime-sdk-js/blob/main/src/connectionmonitor/SignalingAndMetricsConnectionMonitor.ts#L69">src/connectionmonitor/SignalingAndMetricsConnectionMonitor.ts:69</a></li>
+									<li>Defined in <a href="https://github.com/aws/amazon-chime-sdk-js/blob/main/src/connectionmonitor/SignalingAndMetricsConnectionMonitor.ts#L70">src/connectionmonitor/SignalingAndMetricsConnectionMonitor.ts:70</a></li>
 								</ul>
 							</aside>
 							<div class="tsd-comment tsd-typography">
@@ -180,7 +180,7 @@ <h3>did<wbr>Receive<wbr>Pong</h3>
 							<aside class="tsd-sources">
 								<p>Implementation of <a href="../interfaces/pingpongobserver.html">PingPongObserver</a>.<a href="../interfaces/pingpongobserver.html#didreceivepong">didReceivePong</a></p>
 								<ul>
-									<li>Defined in <a href="https://github.com/aws/amazon-chime-sdk-js/blob/main/src/connectionmonitor/SignalingAndMetricsConnectionMonitor.ts#L62">src/connectionmonitor/SignalingAndMetricsConnectionMonitor.ts:62</a></li>
+									<li>Defined in <a href="https://github.com/aws/amazon-chime-sdk-js/blob/main/src/connectionmonitor/SignalingAndMetricsConnectionMonitor.ts#L63">src/connectionmonitor/SignalingAndMetricsConnectionMonitor.ts:63</a></li>
 								</ul>
 							</aside>
 							<div class="tsd-comment tsd-typography">
@@ -216,7 +216,7 @@ <h3>metrics<wbr>Did<wbr>Receive</h3>
 							<aside class="tsd-sources">
 								<p>Implementation of <a href="../interfaces/audiovideoobserver.html">AudioVideoObserver</a>.<a href="../interfaces/audiovideoobserver.html#metricsdidreceive">metricsDidReceive</a></p>
 								<ul>
-									<li>Defined in <a href="https://github.com/aws/amazon-chime-sdk-js/blob/main/src/connectionmonitor/SignalingAndMetricsConnectionMonitor.ts#L76">src/connectionmonitor/SignalingAndMetricsConnectionMonitor.ts:76</a></li>
+									<li>Defined in <a href="https://github.com/aws/amazon-chime-sdk-js/blob/main/src/connectionmonitor/SignalingAndMetricsConnectionMonitor.ts#L77">src/connectionmonitor/SignalingAndMetricsConnectionMonitor.ts:77</a></li>
 								</ul>
 							</aside>
 							<div class="tsd-comment tsd-typography">
@@ -244,7 +244,7 @@ <h3>receive<wbr>Signal<wbr>Strength<wbr>Change</h3>
 						<li class="tsd-description">
 							<aside class="tsd-sources">
 								<ul>
-									<li>Defined in <a href="https://github.com/aws/amazon-chime-sdk-js/blob/main/src/connectionmonitor/SignalingAndMetricsConnectionMonitor.ts#L51">src/connectionmonitor/SignalingAndMetricsConnectionMonitor.ts:51</a></li>
+									<li>Defined in <a href="https://github.com/aws/amazon-chime-sdk-js/blob/main/src/connectionmonitor/SignalingAndMetricsConnectionMonitor.ts#L52">src/connectionmonitor/SignalingAndMetricsConnectionMonitor.ts:52</a></li>
 								</ul>
 							</aside>
 							<h4 class="tsd-parameters-title">Parameters</h4>
@@ -268,7 +268,7 @@ <h3>start</h3>
 							<aside class="tsd-sources">
 								<p>Implementation of <a href="../interfaces/connectionmonitor.html">ConnectionMonitor</a>.<a href="../interfaces/connectionmonitor.html#start">start</a></p>
 								<ul>
-									<li>Defined in <a href="https://github.com/aws/amazon-chime-sdk-js/blob/main/src/connectionmonitor/SignalingAndMetricsConnectionMonitor.ts#L37">src/connectionmonitor/SignalingAndMetricsConnectionMonitor.ts:37</a></li>
+									<li>Defined in <a href="https://github.com/aws/amazon-chime-sdk-js/blob/main/src/connectionmonitor/SignalingAndMetricsConnectionMonitor.ts#L38">src/connectionmonitor/SignalingAndMetricsConnectionMonitor.ts:38</a></li>
 								</ul>
 							</aside>
 							<div class="tsd-comment tsd-typography">
@@ -291,7 +291,7 @@ <h3>stop</h3>
 							<aside class="tsd-sources">
 								<p>Implementation of <a href="../interfaces/connectionmonitor.html">ConnectionMonitor</a>.<a href="../interfaces/connectionmonitor.html#stop">stop</a></p>
 								<ul>
-									<li>Defined in <a href="https://github.com/aws/amazon-chime-sdk-js/blob/main/src/connectionmonitor/SignalingAndMetricsConnectionMonitor.ts#L44">src/connectionmonitor/SignalingAndMetricsConnectionMonitor.ts:44</a></li>
+									<li>Defined in <a href="https://github.com/aws/amazon-chime-sdk-js/blob/main/src/connectionmonitor/SignalingAndMetricsConnectionMonitor.ts#L45">src/connectionmonitor/SignalingAndMetricsConnectionMonitor.ts:45</a></li>
 								</ul>
 							</aside>
 							<div class="tsd-comment tsd-typography">

diff --git 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
@@ -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,22 +339,53 @@ 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;
     sendClientMetricReport(testClientMetricReport);
     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', () => {