Merge pull request #14250 from nextcloud/backport/14095/stable29

[stable29] Fix (known) false positives in connection warning

Author: danxuliu

src/utils/webrtc/analyzers/AverageStatValue.js

@@ -36,7 +36,7 @@ const STAT_VALUE_TYPE = {
  *
  * The number of items to keep track of must be set when the AverageStatValue is
  * created. Once N items have been added adding a new one will discard the
- * oldest value. "hasEnoughData()" can be used to check if at least N items have
+ * oldest value. "hasEnoughData()" can be used to check if enough items have
  * been added already and the average is reliable.
  *
  * An RTCStatsReport value can be cumulative since the creation of the
@@ -50,6 +50,10 @@ const STAT_VALUE_TYPE = {
  * however, that the first value added to a cumulative AverageStatValue after
  * creating or resetting it will be treated as 0 in the average calculation,
  * as it will be the base from which the rest of relative values are calculated.
+ * Therefore, if the values added to an AverageStatValue are relative,
+ * "hasEnoughData()" will not return true until at least N items were added,
+ * but if the values are cumulative, it will not return true until at least N+1
+ * items were added.
  *
  * Besides the weighted average it is possible to "peek" the last value, either
  * the raw value that was added or the relative one after the conversion (which,
@@ -70,15 +74,25 @@ function AverageStatValue(count, type = STAT_VALUE_TYPE.CUMULATIVE, lastValueWei
 
 	this._rawValues = []
 	this._relativeValues = []
+
+	this._hasEnoughData = false
 }
 AverageStatValue.prototype = {
 
 	reset() {
 		this._rawValues = []
 		this._relativeValues = []
+
+		this._hasEnoughData = false
 	},
 
 	add(value) {
+		if ((this._type === STAT_VALUE_TYPE.CUMULATIVE && this._rawValues.length === this._count)
+			|| (this._type === STAT_VALUE_TYPE.RELATIVE && this._rawValues.length >= (this._count - 1))
+		) {
+			this._hasEnoughData = true
+		}
+
 		if (this._rawValues.length === this._count) {
 			this._rawValues.shift()
 			this._relativeValues.shift()
@@ -113,7 +127,7 @@ AverageStatValue.prototype = {
 	},
 
 	hasEnoughData() {
-		return this._rawValues.length === this._count
+		return this._hasEnoughData
 	},
 
 	getWeightedAverage() {

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

@@ -21,17 +21,92 @@ describe('AverageStatValue', () => {
 		})
 	})
 
-	test('returns whether there are enough values for a meaningful calculation', () => {
-		const testValues = [100, 200, 150, 123, 30, 50, 22, 33]
-		const stat = new AverageStatValue(3, STAT_VALUE_TYPE.CUMULATIVE, 3)
-		const stat2 = new AverageStatValue(3, STAT_VALUE_TYPE.RELATIVE, 3)
+	describe('returns whether there are enough values for a meaningful calculation', () => {
+		test('after creating', () => {
+			const testValues = [100, 200, 150, 123, 30, 50, 22, 33]
+			const stat = new AverageStatValue(3, STAT_VALUE_TYPE.CUMULATIVE, 3)
+			const stat2 = new AverageStatValue(3, STAT_VALUE_TYPE.RELATIVE, 3)
 
-		testValues.forEach((val, index) => {
-			stat.add(val)
-			expect(stat.hasEnoughData()).toBe(index >= 2)
+			testValues.forEach((val, index) => {
+				stat.add(val)
+				expect(stat.hasEnoughData()).toBe(index >= 3)
 
-			stat2.add(val)
-			expect(stat2.hasEnoughData()).toBe(index >= 2)
+				stat2.add(val)
+				expect(stat2.hasEnoughData()).toBe(index >= 2)
+			})
+		})
+
+		describe('resetting', () => {
+			let stat
+			let stat2
+
+			const addValues = (values) => {
+				values.forEach(val => {
+					stat.add(val)
+					stat2.add(val)
+				})
+			}
+
+			beforeEach(() => {
+				stat = new AverageStatValue(3, STAT_VALUE_TYPE.CUMULATIVE, 3)
+				stat2 = new AverageStatValue(3, STAT_VALUE_TYPE.RELATIVE, 3)
+			})
+
+			test('before having enough values', () => {
+				addValues([100, 200])
+
+				expect(stat.hasEnoughData()).toBe(false)
+				expect(stat2.hasEnoughData()).toBe(false)
+
+				stat.reset()
+				stat2.reset()
+
+				expect(stat.hasEnoughData()).toBe(false)
+				expect(stat2.hasEnoughData()).toBe(false)
+
+				addValues([150, 123])
+
+				expect(stat.hasEnoughData()).toBe(false)
+				expect(stat2.hasEnoughData()).toBe(false)
+
+				addValues([30])
+
+				expect(stat.hasEnoughData()).toBe(false)
+				expect(stat2.hasEnoughData()).toBe(true)
+
+				addValues([50])
+
+				expect(stat.hasEnoughData()).toBe(true)
+				expect(stat2.hasEnoughData()).toBe(true)
+			})
+
+			test('after having enough values', () => {
+				addValues([100, 200, 150, 123])
+
+				expect(stat.hasEnoughData()).toBe(true)
+				expect(stat2.hasEnoughData()).toBe(true)
+
+				stat.reset()
+				stat2.reset()
+
+				expect(stat.hasEnoughData()).toBe(false)
+				expect(stat2.hasEnoughData()).toBe(false)
+
+				addValues([30, 50])
+
+				expect(stat.hasEnoughData()).toBe(false)
+				expect(stat2.hasEnoughData()).toBe(false)
+
+				addValues([22])
+
+				expect(stat.hasEnoughData()).toBe(false)
+				expect(stat2.hasEnoughData()).toBe(true)
+
+				addValues([33])
+
+				expect(stat.hasEnoughData()).toBe(true)
+				expect(stat2.hasEnoughData()).toBe(true)
+			})
 		})
 	})
 

src/utils/webrtc/analyzers/PeerConnectionAnalyzer.js

@@ -412,6 +412,19 @@ PeerConnectionAnalyzer.prototype = {
 				packetsLost[kind] = this._packetsLost[kind].getLastRawValue()
 			}
 
+			// In some (also strange) cases a newer stat may report a lower
+			// value than a previous one (it happens sometimes with garbage
+			// remote reports in simulcast video that cause the values to
+			// overflow, although it was also seen with a small value regression
+			// when enabling video). If that happens the stats are reset to
+			// prevent distorting the analysis with negative packet counts; note
+			// that in this case the previous value is not kept because it is
+			// not just an isolated wrong value, all the following stats
+			// increase from the regressed value.
+			if (packets[kind] >= 0 && packets[kind] < this._packets[kind].getLastRawValue()) {
+				this._resetStats(kind)
+			}
+
 			this._addStats(kind, packets[kind], packetsLost[kind], timestamp[kind], roundTripTime[kind])
 		}
 	},
@@ -482,12 +495,13 @@ PeerConnectionAnalyzer.prototype = {
 	 * The stats reported by the browser can sometimes stall for a second (or
 	 * more, but typically they stall only for a single report). When that
 	 * happens the stats are still reported, but with the same number of packets
-	 * as in the previous report (timestamp and round trip time are updated,
-	 * though). In that case the given stats are not added yet to the average
-	 * stats; they are kept on hold until more stats are provided by the browser
-	 * and it can be determined if the previous stats were stalled or not. If
-	 * they were stalled the previous and new stats are distributed, and if they
-	 * were not they are added as is to the average stats.
+	 * as in the previous report (timestamp and round trip time may be updated
+	 * or not, apparently depending on browser version and/or Janus version). In
+	 * that case the given stats are not added yet to the average stats; they
+	 * are kept on hold until more stats are provided by the browser and it can
+	 * be determined if the previous stats were stalled or not. If they were
+	 * stalled the previous and new stats are distributed, and if they were not
+	 * they are added as is to the average stats.
 	 *
 	 * @param {string} kind the type of the stats ("audio" or "video")
 	 * @param {number} packets the cumulative number of packets
@@ -552,6 +566,18 @@ PeerConnectionAnalyzer.prototype = {
 		let packetsLostTotal = 0
 		let timestampsTotal = 0
 
+		// 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) {
+			const lastTimestamp = this._stagedTimestamps[kind][this._stagedTimestamps[kind].length - 1]
+			const timestampsTotalDifference = lastTimestamp - timestampsBase
+			const timestampsDelta = timestampsTotalDifference / this._stagedTimestamps[kind].length
+
+			for (let i = 0; i < this._stagedTimestamps[kind].length - 1; i++) {
+				this._stagedTimestamps[kind][i] += timestampsDelta * (i + 1)
+			}
+		}
+
 		for (let i = 0; i < this._stagedPackets[kind].length; i++) {
 			packetsTotal += (this._stagedPackets[kind][i] - packetsBase)
 			packetsBase = this._stagedPackets[kind][i]
@@ -578,7 +604,11 @@ PeerConnectionAnalyzer.prototype = {
 			packetsLostBase = this._stagedPacketsLost[kind][i]
 
 			// Timestamps and round trip time are not distributed, as those
-			// values are properly updated even if the stats are stalled.
+			// values may be properly updated even if the stats are stalled. In
+			// case they were not timestamps were already evenly distributed
+			// above, and round trip time can not be distributed, as it is
+			// already provided in the stats as a relative value rather than a
+			// cumulative one.
 		}
 	},
 
@@ -628,11 +658,19 @@ PeerConnectionAnalyzer.prototype = {
 	},
 
 	_calculateConnectionQuality(kind) {
+		const packets = this._packets[kind]
+		const packetsLost = this._packetsLost[kind]
+		const timestamps = this._timestamps[kind]
 		const packetsLostRatio = this._packetsLostRatio[kind]
 		const packetsPerSecond = this._packetsPerSecond[kind]
 		const roundTripTime = this._roundTripTime[kind]
 
-		if (!packetsLostRatio.hasEnoughData() || !packetsPerSecond.hasEnoughData()) {
+		// packetsLostRatio and packetsPerSecond are relative values, but they
+		// are calculated from cumulative values. Therefore, it is necessary to
+		// check if the cumulative values that are their source have enough data
+		// or not, rather than checking if the relative values themselves have
+		// enough data.
+		if (!packets.hasEnoughData() || !packetsLost.hasEnoughData() || !timestamps.hasEnoughData()) {
 			return CONNECTION_QUALITY.UNKNOWN
 		}
 
@@ -671,10 +709,13 @@ PeerConnectionAnalyzer.prototype = {
 		// quality to keep a smooth video, albeit on a lower resolution. Thus
 		// with a threshold of 10 packets issues can be detected too for videos,
 		// although only once they can not be further downscaled.
+		// Despite all of the above it has been observed that less than 10
+		// packets are sometimes sent without any connection problem (for
+		// example, when the background is blurred and the video quality is
+		// reduced due to being in a call with several participants), so for now
+		// it is only logged but not reported.
 		if (packetsPerSecond.getWeightedAverage() < 10) {
 			this._logStats(kind, 'Low packets per second: ' + packetsPerSecond.getWeightedAverage())
-
-			return CONNECTION_QUALITY.VERY_BAD
 		}
 
 		if (packetsLostRatioWeightedAverage > 0.3) {