fix: Fix distributing staged stats when not updated three times in a row

The stats were supposed to be distributed once they had changed, but
in practice they were always distributed, as the packet count is
absolute rather than relative. Nevertheless, if the packet count did not
change distributing them would have no effect. The problem could appear
in the (rare) case of the timestamps not being updated three times in a
row, as the distribution algorithm failed if the final timestamp was the
same as the initial timestamp (causing NaN to be set for the packets and
packets lost).

Due to all that now the staged stats are always distributed before being
commited (which is still done when the stats stalled for two seconds),
although the distribution exits early if the timestamps did not change.

Signed-off-by: Daniel Calviño Sánchez <[email protected]>

Author: danxuliu

src/utils/webrtc/analyzers/PeerConnectionAnalyzer.js

@@ -506,11 +506,10 @@ PeerConnectionAnalyzer.prototype = {
 
 		this._stageStats(kind, packets, packetsLost, timestamp, roundTripTime)
 
-		// If the packets have changed now it is assumed that the previous stats
-		// were stalled.
-		if (packets > 0) {
-			this._distributeStagedStats(kind)
-		}
+		// Distributing the stats has no effect if the stats were not stalled
+		// (that is, if the values are still unchanged, so it is probably an
+		// actual connection problem rather than a stalled report).
+		this._distributeStagedStats(kind)
 
 		while (this._stagedPackets[kind].length > 0) {
 			const stagedPackets = this._stagedPackets[kind].shift()
@@ -550,6 +549,12 @@ PeerConnectionAnalyzer.prototype = {
 		let packetsLostTotal = 0
 		let timestampsTotal = 0
 
+		// If the last timestamp is still stalled there is nothing to
+		// distribute.
+		if (this._stagedTimestamps[kind][this._stagedTimestamps[kind].length - 1] === timestampsBase) {
+			return
+		}
+
 		// If the first timestamp stalled it is assumed that all of them
 		// stalled and are thus evenly distributed based on the new timestamp.
 		if (this._stagedTimestamps[kind][0] === timestampsBase) {

src/utils/webrtc/analyzers/PeerConnectionAnalyzer.spec.js

@@ -3437,6 +3437,51 @@ describe('PeerConnectionAnalyzer', () => {
 			expect(peerConnectionAnalyzer._stagedRoundTripTime[kind]).toEqual([])
 		})
 
+		test.each([
+			['packet count repeated two times', 'audio'],
+			['packet count repeated two times', 'video'],
+		])('%s, %s', (name, kind) => {
+			peerConnectionAnalyzer._addStats(kind, 150, 40, 10000, 0.2)
+			peerConnectionAnalyzer._addStats(kind, 150, 40, 10000, 0.2)
+			peerConnectionAnalyzer._addStats(kind, 150, 40, 10000, 0.2)
+
+			expect(peerConnectionAnalyzer._packets[kind]._relativeValues).toEqual([0, 0, 0])
+			expect(peerConnectionAnalyzer._packetsLost[kind]._relativeValues).toEqual([0, 0, 0])
+			expect(peerConnectionAnalyzer._packetsLostRatio[kind]._relativeValues).toEqual([1.5, 1.5, 1.5])
+			expect(peerConnectionAnalyzer._timestamps[kind]._relativeValues).toEqual([0, 0])
+			expect(peerConnectionAnalyzer._timestampsForLogs[kind]._relativeValues).toEqual([0, 0, 0])
+			expect(peerConnectionAnalyzer._packetsPerSecond[kind]._relativeValues).toEqual([NaN, NaN, NaN])
+			expect(peerConnectionAnalyzer._roundTripTime[kind]._relativeValues).toEqual([0.2, 0.2, 0.2])
+
+			expect(peerConnectionAnalyzer._stagedPackets[kind]).toEqual([])
+			expect(peerConnectionAnalyzer._stagedPacketsLost[kind]).toEqual([])
+			expect(peerConnectionAnalyzer._stagedTimestamps[kind]).toEqual([])
+			expect(peerConnectionAnalyzer._stagedRoundTripTime[kind]).toEqual([])
+		})
+
+		test.each([
+			['packet count repeated two times then changed', 'audio'],
+			['packet count repeated two times then changed', 'video'],
+		])('%s, %s', (name, kind) => {
+			peerConnectionAnalyzer._addStats(kind, 150, 40, 10000, 0.2)
+			peerConnectionAnalyzer._addStats(kind, 150, 40, 10000, 0.2)
+			peerConnectionAnalyzer._addStats(kind, 150, 40, 10000, 0.2)
+			peerConnectionAnalyzer._addStats(kind, 300, 70, 13750, 0.3)
+
+			expect(peerConnectionAnalyzer._packets[kind]._relativeValues).toEqual([0, 0, 0, 150])
+			expect(peerConnectionAnalyzer._packetsLost[kind]._relativeValues).toEqual([0, 0, 0, 30])
+			expect(peerConnectionAnalyzer._packetsLostRatio[kind]._relativeValues).toEqual([1.5, 1.5, 1.5, 0.2])
+			expect(peerConnectionAnalyzer._timestamps[kind]._relativeValues).toEqual([0, 3750])
+			expect(peerConnectionAnalyzer._timestampsForLogs[kind]._relativeValues).toEqual([0, 0, 0, 3750])
+			expect(peerConnectionAnalyzer._packetsPerSecond[kind]._relativeValues).toEqual([NaN, NaN, NaN, 40])
+			expect(peerConnectionAnalyzer._roundTripTime[kind]._relativeValues).toEqual([0.2, 0.2, 0.2, 0.3])
+
+			expect(peerConnectionAnalyzer._stagedPackets[kind]).toEqual([])
+			expect(peerConnectionAnalyzer._stagedPacketsLost[kind]).toEqual([])
+			expect(peerConnectionAnalyzer._stagedTimestamps[kind]).toEqual([])
+			expect(peerConnectionAnalyzer._stagedRoundTripTime[kind]).toEqual([])
+		})
+
 		describe('distribute staged stats', () => {
 
 			const expectRelativeStagedStats = (kind, index, expectedPackets, expectedPacketsLost, expectedTimestamps, expectedRoundTripTime) => {
@@ -3502,6 +3547,20 @@ describe('PeerConnectionAnalyzer', () => {
 				expectRelativeStagedStats(kind, 1, 150, 40, 14000, 0.2)
 			})
 
+			test.each([
+				['two sets of fully repeated values', 'audio'],
+				['two sets of fully repeated values', 'video'],
+			])('%s, %s', (name, kind) => {
+				peerConnectionAnalyzer._commitStats(kind, 150, 40, 10000, 0.2)
+				peerConnectionAnalyzer._stageStats(kind, 150, 40, 10000, 0.2)
+				peerConnectionAnalyzer._stageStats(kind, 150, 40, 10000, 0.2)
+
+				peerConnectionAnalyzer._distributeStagedStats(kind)
+
+				expectRelativeStagedStats(kind, 0, 150, 40, 10000, 0.2)
+				expectRelativeStagedStats(kind, 1, 150, 40, 10000, 0.2)
+			})
+
 			test.each([
 				['several sets of different values with repeated timestamps', 'audio'],
 				['several sets of different values with repeated timestamps', 'video'],
@@ -3573,6 +3632,24 @@ describe('PeerConnectionAnalyzer', () => {
 				expectRelativeStagedStats(kind, 2, 150, 40, 17500, 0.2)
 				expectRelativeStagedStats(kind, 3, 150, 40, 20000, 0.2)
 			})
+
+			test.each([
+				['several sets of fully repeated values', 'audio'],
+				['several sets of fully repeated values', 'video'],
+			])('%s, %s', (name, kind) => {
+				peerConnectionAnalyzer._commitStats(kind, 150, 40, 10000, 0.2)
+				peerConnectionAnalyzer._stageStats(kind, 150, 40, 10000, 0.2)
+				peerConnectionAnalyzer._stageStats(kind, 150, 40, 10000, 0.2)
+				peerConnectionAnalyzer._stageStats(kind, 150, 40, 10000, 0.2)
+				peerConnectionAnalyzer._stageStats(kind, 150, 40, 10000, 0.2)
+
+				peerConnectionAnalyzer._distributeStagedStats(kind)
+
+				expectRelativeStagedStats(kind, 0, 150, 40, 10000, 0.2)
+				expectRelativeStagedStats(kind, 1, 150, 40, 10000, 0.2)
+				expectRelativeStagedStats(kind, 2, 150, 40, 10000, 0.2)
+				expectRelativeStagedStats(kind, 3, 150, 40, 10000, 0.2)
+			})
 		})
 	})
 })