Skip to content

Commit

Permalink
Fix reconnections when setting audio Attendee Capability to 'None' or…
Browse files Browse the repository at this point in the history 
… 'Send' mid call (#2817)
  • Loading branch information
@hensmi-amazon
hensmi-amazon authored Jan 24, 2024
1 parent eeecb95 commit 635e9ec
Show file tree
Hide file tree
Showing 4 changed files with 115 additions and 47 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
### Changed

### Fixed

- Fix reconnections when setting audio Attendee Capability to 'None' or 'Send' mid call. The connection health monitor will now look at all packets received on all candidate pairs instead of just audio received media packets.
- Setup passthrough streams for insertable streams case in the redundant audio worker so that passthrough streams do not get blocked on the main thread

## [3.19.0] - 2023-09-20
Expand Down
14 changes: 7 additions & 7 deletions docs/classes/signalingandmetricsconnectionmonitor.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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#L18">src/connectionmonitor/SignalingAndMetricsConnectionMonitor.ts:18</a></li>
<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>
</ul>
</aside>
<h4 class="tsd-parameters-title">Parameters</h4>
Expand Down Expand Up @@ -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#L68">src/connectionmonitor/SignalingAndMetricsConnectionMonitor.ts:68</a></li>
<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>
</ul>
</aside>
<div class="tsd-comment tsd-typography">
Expand All @@ -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#L61">src/connectionmonitor/SignalingAndMetricsConnectionMonitor.ts:61</a></li>
<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>
</ul>
</aside>
<div class="tsd-comment tsd-typography">
Expand Down Expand Up @@ -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#L75">src/connectionmonitor/SignalingAndMetricsConnectionMonitor.ts:75</a></li>
<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>
</ul>
</aside>
<div class="tsd-comment tsd-typography">
Expand Down Expand Up @@ -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#L50">src/connectionmonitor/SignalingAndMetricsConnectionMonitor.ts:50</a></li>
<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>
</ul>
</aside>
<h4 class="tsd-parameters-title">Parameters</h4>
Expand All @@ -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#L36">src/connectionmonitor/SignalingAndMetricsConnectionMonitor.ts:36</a></li>
<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>
</ul>
</aside>
<div class="tsd-comment tsd-typography">
Expand All @@ -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#L43">src/connectionmonitor/SignalingAndMetricsConnectionMonitor.ts:43</a></li>
<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>
</ul>
</aside>
<div class="tsd-comment tsd-typography">
Expand Down
47 changes: 33 additions & 14 deletions src/connectionmonitor/SignalingAndMetricsConnectionMonitor.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ export default class SignalingAndMetricsConnectionMonitor
implements ConnectionMonitor, PingPongObserver, AudioVideoObserver {
private isActive = false;
private hasSeenValidPacketMetricsBefore = false;
private lastTotalPacketsReceived = 0;

constructor(
private audioVideoController: AudioVideoController,
Expand Down Expand Up @@ -73,11 +74,11 @@ export default class SignalingAndMetricsConnectionMonitor
}

metricsDidReceive(clientMetricReport: ClientMetricReport): void {
let packetsReceived = 0;
let fractionPacketsLostInbound = 0;
let audioPacketsReceived = 0;
let audiofractionPacketsLostInbound = 0;
const metricReport = clientMetricReport.getObservableMetrics();
const potentialPacketsReceived = metricReport.audioPacketsReceived;
const potentialFractionPacketsLostInbound = metricReport.audioPacketsReceivedFractionLoss;
const potentialAudioPacketsReceived = metricReport.audioPacketsReceived;
const potentialAudioFractionPacketsLostInbound = metricReport.audioPacketsReceivedFractionLoss;

const audioSpeakerDelayMs = metricReport.audioSpeakerDelayMs;

Expand All @@ -86,24 +87,42 @@ export default class SignalingAndMetricsConnectionMonitor
this.connectionHealthData.setAudioSpeakerDelayMs(audioSpeakerDelayMs);
}

// To get the total packets received, including RTCP, we need to sum up
// all candidate pair metrics (in case the candidate pair changes).
//
// The stats collector currently doesn't account for candidate pair stats
// so we just use the raw report for now.
const webrtcReport = clientMetricReport.getRTCStatsReport();

let totalPacketsReceived = 0;
webrtcReport.forEach(stat => {
if (stat.type === 'candidate-pair' && stat.packetsReceived) {
totalPacketsReceived += stat.packetsReceived;
}
});
const packetsReceived = totalPacketsReceived - this.lastTotalPacketsReceived;
this.lastTotalPacketsReceived = totalPacketsReceived;
if (
typeof potentialPacketsReceived === 'number' &&
typeof potentialFractionPacketsLostInbound === 'number'
typeof potentialAudioPacketsReceived === 'number' &&
typeof potentialAudioFractionPacketsLostInbound === 'number'
) {
packetsReceived = potentialPacketsReceived;
fractionPacketsLostInbound = potentialFractionPacketsLostInbound;
if (packetsReceived < 0 || fractionPacketsLostInbound < 0) {
// TODO: getting negative numbers on this metric after reconnect sometimes
// For now, just skip the metric if it looks weird.
audioPacketsReceived = potentialAudioPacketsReceived;
audiofractionPacketsLostInbound = potentialAudioFractionPacketsLostInbound;
if (audioPacketsReceived < 0 || audiofractionPacketsLostInbound < 0 || packetsReceived < 0) {
// The stats collector or logic above may emit negative numbers on this metric after reconnect
// which we should not use.
return;
}
} else {
return;
}
this.addToMinuteWindow(this.connectionHealthData.packetsReceivedInLastMinute, packetsReceived);
this.addToMinuteWindow(
this.connectionHealthData.packetsReceivedInLastMinute,
audioPacketsReceived
);
this.addToMinuteWindow(
this.connectionHealthData.fractionPacketsLostInboundInLastMinute,
fractionPacketsLostInbound
audiofractionPacketsLostInbound
);
if (packetsReceived > 0) {
this.hasSeenValidPacketMetricsBefore = true;
Expand All @@ -113,7 +132,7 @@ export default class SignalingAndMetricsConnectionMonitor
this.connectionHealthData.consecutiveStatsWithNoPackets + 1
);
}
if (packetsReceived === 0 || fractionPacketsLostInbound > 0) {
if (audioPacketsReceived === 0 || audiofractionPacketsLostInbound > 0) {
this.connectionHealthData.setLastPacketLossInboundTimestampMs(Date.now());
}
if (typeof metricReport.audioPacketsSent === 'number') {
Expand Down
99 changes: 73 additions & 26 deletions test/connectionmonitor/SignalingAndMetricsConnectionMonitor.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ describe('SignalingAndMetricsConnectionMonitor', () => {
let connectionHealthData: ConnectionHealthData;
let pingPongStartCalled: boolean;
let consecutiveMissedPongsCalled: boolean;
let consecutiveStatsWithNoPacketsCalled: boolean;
let consecutiveStatsWithNoPackets: number | undefined = undefined;
let lastPacketLossInboundTimestampMsCalled: boolean;
let setLastNoSignalTimestampMsCalled: boolean;
let setLastWeakSignalTimestampMsCalled: boolean;
Expand Down Expand Up @@ -65,8 +65,9 @@ describe('SignalingAndMetricsConnectionMonitor', () => {
setConsecutiveMissedPongs(_pongs: number): void {
consecutiveMissedPongsCalled = true;
}
setConsecutiveStatsWithNoPackets(_stats: number): void {
consecutiveStatsWithNoPacketsCalled = true;
setConsecutiveStatsWithNoPackets(count: number): void {
super.setConsecutiveStatsWithNoPackets(count);
consecutiveStatsWithNoPackets = count;
}
setLastPacketLossInboundTimestampMs(_timeStamp: number): void {
lastPacketLossInboundTimestampMsCalled = true;
Expand Down Expand Up @@ -97,7 +98,7 @@ describe('SignalingAndMetricsConnectionMonitor', () => {
}

class TestClientMetricReport extends ClientMetricReport {
packetsReceived: RawMetrics = null;
audioPacketsReceived: RawMetrics = null;
fractionLoss: RawMetrics = null;
videoPacketSentPerSecond: RawMetrics = 1000;
videoUpstreamBitrate: RawMetrics = 100;
Expand All @@ -113,10 +114,11 @@ describe('SignalingAndMetricsConnectionMonitor', () => {
videoDownstreamFrameHeight: RawMetrics = 100;
videoDownstreamFrameWidth: RawMetrics = 100;
audioPacketsSent: RawMetrics = 50;
totalPacketsReceived: number = 0;

getObservableMetrics(): { [id: string]: number } {
return {
audioPacketsReceived: this.packetsReceived,
audioPacketsReceived: this.audioPacketsReceived,
audioPacketsReceivedFractionLoss: this.fractionLoss,
videoPacketSentPerSecond: this.videoPacketSentPerSecond,
videoUpstreamBitrate: this.videoUpstreamBitrate,
Expand All @@ -135,6 +137,31 @@ describe('SignalingAndMetricsConnectionMonitor', () => {
videoUpstreamFrameWidth: this.videoUpstreamFrameWidth,
};
}

getRTCStatsReport(): RTCStatsReport {
const rtcStatsReport = new Map<string, RawMetrics>([
[
'candidatePairId1',
{
type: 'candidate-pair',
...{
packetsReceived: this.totalPacketsReceived,
},
},
],
[
'candidatePairId2',
{
type: 'candidate-pair',
...{
packetsReceived: 0,
},
},
],
]);

return rtcStatsReport as RTCStatsReport;
}
}

const expect: Chai.ExpectStatic = chai.expect;
Expand All @@ -151,7 +178,7 @@ describe('SignalingAndMetricsConnectionMonitor', () => {
domMockBuilder = new DOMMockBuilder();
pingPongStartCalled = false;
consecutiveMissedPongsCalled = false;
consecutiveStatsWithNoPacketsCalled = false;
consecutiveStatsWithNoPackets = 0;
lastPacketLossInboundTimestampMsCalled = false;
setLastNoSignalTimestampMsCalled = false;
setLastWeakSignalTimestampMsCalled = false;
Expand Down Expand Up @@ -204,7 +231,7 @@ describe('SignalingAndMetricsConnectionMonitor', () => {
const window: number[] = [];
const num = 1;
connectionHealthData.packetsReceivedInLastMinute = window;
testClientMetricReport.packetsReceived = num;
testClientMetricReport.audioPacketsReceived = num;
testClientMetricReport.fractionLoss = 1;
sendClientMetricReport(testClientMetricReport);
expect(window.length).to.equal(1);
Expand All @@ -216,7 +243,7 @@ describe('SignalingAndMetricsConnectionMonitor', () => {
let num: number;
for (num = 0; num < 61; num++) {
connectionHealthData.packetsReceivedInLastMinute = window;
testClientMetricReport.packetsReceived = num;
testClientMetricReport.audioPacketsReceived = num;
testClientMetricReport.fractionLoss = 1;
sendClientMetricReport(testClientMetricReport);
}
Expand All @@ -225,62 +252,82 @@ describe('SignalingAndMetricsConnectionMonitor', () => {
});

it('can return without changing stats when fraction loss is negative', () => {
testClientMetricReport.packetsReceived = 1;
testClientMetricReport.audioPacketsReceived = 1;
testClientMetricReport.fractionLoss = -1;
sendClientMetricReport(testClientMetricReport);
expect(consecutiveMissedPongsCalled).to.equal(false);
expect(consecutiveStatsWithNoPacketsCalled).to.equal(false);
expect(consecutiveStatsWithNoPackets).to.equal(0);
expect(lastPacketLossInboundTimestampMsCalled).to.equal(false);
});

it('can return without changing stats when packets received is negative', () => {
testClientMetricReport.packetsReceived = -1;
testClientMetricReport.audioPacketsReceived = -1;
testClientMetricReport.fractionLoss = 1;
sendClientMetricReport(testClientMetricReport);
expect(consecutiveMissedPongsCalled).to.equal(false);
expect(consecutiveStatsWithNoPacketsCalled).to.equal(false);
expect(consecutiveStatsWithNoPackets).to.equal(0);
expect(lastPacketLossInboundTimestampMsCalled).to.equal(false);
});

it('can return without changing stats when packets received is not a number', () => {
testClientMetricReport.packetsReceived = 'not a number';
testClientMetricReport.audioPacketsReceived = 'not a number';
testClientMetricReport.fractionLoss = 1;
sendClientMetricReport(testClientMetricReport);
expect(consecutiveMissedPongsCalled).to.equal(false);
expect(consecutiveStatsWithNoPacketsCalled).to.equal(false);
expect(consecutiveStatsWithNoPackets).to.equal(0);
expect(lastPacketLossInboundTimestampMsCalled).to.equal(false);
});

it('can return without changing stats when fractional packet loss is not a number', () => {
testClientMetricReport.packetsReceived = 1;
testClientMetricReport.audioPacketsReceived = 1;
testClientMetricReport.fractionLoss = 'not a number';
sendClientMetricReport(testClientMetricReport);
expect(consecutiveMissedPongsCalled).to.equal(false);
expect(consecutiveStatsWithNoPacketsCalled).to.equal(false);
expect(consecutiveStatsWithNoPackets).to.equal(0);
expect(lastPacketLossInboundTimestampMsCalled).to.equal(false);
});

it('can return without changing stats when total packets received is negative', () => {
testClientMetricReport.totalPacketsReceived = 4;
testClientMetricReport.fractionLoss = 0;
testClientMetricReport.audioPacketsReceived = 1;
sendClientMetricReport(testClientMetricReport);
testClientMetricReport.totalPacketsReceived = 2;
testClientMetricReport.fractionLoss = 0;
testClientMetricReport.audioPacketsReceived = 1;
sendClientMetricReport(testClientMetricReport);
expect(consecutiveMissedPongsCalled).to.equal(false);
expect(consecutiveStatsWithNoPackets).to.equal(0);
expect(lastPacketLossInboundTimestampMsCalled).to.equal(false);
});

it('can reset and increment consecutive stats with no packets when packets received are followed by no packets', () => {
testClientMetricReport.packetsReceived = 1;
testClientMetricReport.totalPacketsReceived = 1;
testClientMetricReport.fractionLoss = 0;
testClientMetricReport.audioPacketsReceived = 1;
sendClientMetricReport(testClientMetricReport);
expect(consecutiveStatsWithNoPacketsCalled).to.equal(true);
testClientMetricReport.packetsReceived = 0;
testClientMetricReport.fractionLoss = 1;
consecutiveStatsWithNoPacketsCalled = false;
expect(consecutiveStatsWithNoPackets).to.equal(0);
testClientMetricReport.totalPacketsReceived = 1;
testClientMetricReport.fractionLoss = 0;
testClientMetricReport.audioPacketsReceived = 0;
sendClientMetricReport(testClientMetricReport);
expect(consecutiveStatsWithNoPackets).to.equal(1);
testClientMetricReport.totalPacketsReceived = 1;
testClientMetricReport.fractionLoss = 0;
testClientMetricReport.audioPacketsReceived = 0;
sendClientMetricReport(testClientMetricReport);
expect(consecutiveStatsWithNoPacketsCalled).to.equal(true);
expect(consecutiveStatsWithNoPackets).to.equal(2);
});

it('can set last packet loss inbound timestamp due to no packets', () => {
testClientMetricReport.packetsReceived = 0;
testClientMetricReport.audioPacketsReceived = 0;
testClientMetricReport.fractionLoss = 0;
sendClientMetricReport(testClientMetricReport);
expect(lastPacketLossInboundTimestampMsCalled).to.equal(true);
});

it('can set last packet loss inbound timestamp due to fractional packet loss', () => {
testClientMetricReport.packetsReceived = 1;
testClientMetricReport.audioPacketsReceived = 1;
testClientMetricReport.fractionLoss = 1;
sendClientMetricReport(testClientMetricReport);
expect(lastPacketLossInboundTimestampMsCalled).to.equal(true);
Expand Down Expand Up @@ -317,7 +364,7 @@ describe('SignalingAndMetricsConnectionMonitor', () => {
});

it('can increment and reset consecutive stats with no audio packets sent', () => {
testClientMetricReport.packetsReceived = 1;
testClientMetricReport.audioPacketsReceived = 1;
testClientMetricReport.fractionLoss = 0;
sendClientMetricReport(testClientMetricReport);
expect(connectionHealthData.consecutiveStatsWithNoAudioPacketsSent).to.equal(0);
Expand All @@ -332,7 +379,7 @@ describe('SignalingAndMetricsConnectionMonitor', () => {
});

it('does not set consecutive stats with no audio packets sent when audioPacketsSent is not present', () => {
testClientMetricReport.packetsReceived = 1;
testClientMetricReport.audioPacketsReceived = 1;
testClientMetricReport.fractionLoss = 0;
testClientMetricReport.audioPacketsSent = undefined;
sendClientMetricReport(testClientMetricReport);
Expand Down

0 comments on commit 635e9ec

Please sign in to comment.