Skip to content

Commit 51ed92b

Browse files
danxuliudanxuliu
committed
Fix calculating stats without enough meaningful values
When the initial stats reports are added the first value of cumulative stats is used as a base when converting the rest of values to relative. Therefore, that initial value is not meaningful and should not be used in calculations, as otherwise the reported quality would be off. Due to that now no quality is reported until the values of the first report added were shifted. Signed-off-by: Daniel Calviño Sánchez <[email protected]>
1 parent a78aad4 commit 51ed92b

File tree

4 files changed

+153
-12
lines changed

4 files changed

+153
-12
lines changed

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

+16-2
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ const STAT_VALUE_TYPE = {
2020
*
2121
* The number of items to keep track of must be set when the AverageStatValue is
2222
* created. Once N items have been added adding a new one will discard the
23-
* oldest value. "hasEnoughData()" can be used to check if at least N items have
23+
* oldest value. "hasEnoughData()" can be used to check if enough items have
2424
* been added already and the average is reliable.
2525
*
2626
* An RTCStatsReport value can be cumulative since the creation of the
@@ -34,6 +34,10 @@ const STAT_VALUE_TYPE = {
3434
* however, that the first value added to a cumulative AverageStatValue after
3535
* creating or resetting it will be treated as 0 in the average calculation,
3636
* as it will be the base from which the rest of relative values are calculated.
37+
* Therefore, if the values added to an AverageStatValue are relative,
38+
* "hasEnoughData()" will not return true until at least N items were added,
39+
* but if the values are cumulative, it will not return true until at least N+1
40+
* items were added.
3741
*
3842
* Besides the weighted average it is possible to "peek" the last value, either
3943
* the raw value that was added or the relative one after the conversion (which,
@@ -54,15 +58,25 @@ function AverageStatValue(count, type = STAT_VALUE_TYPE.CUMULATIVE, lastValueWei
5458

5559
this._rawValues = []
5660
this._relativeValues = []
61+
62+
this._hasEnoughData = false
5763
}
5864
AverageStatValue.prototype = {
5965

6066
reset() {
6167
this._rawValues = []
6268
this._relativeValues = []
69+
70+
this._hasEnoughData = false
6371
},
6472

6573
add(value) {
74+
if ((this._type === STAT_VALUE_TYPE.CUMULATIVE && this._rawValues.length === this._count)
75+
|| (this._type === STAT_VALUE_TYPE.RELATIVE && this._rawValues.length >= (this._count - 1))
76+
) {
77+
this._hasEnoughData = true
78+
}
79+
6680
if (this._rawValues.length === this._count) {
6781
this._rawValues.shift()
6882
this._relativeValues.shift()
@@ -97,7 +111,7 @@ AverageStatValue.prototype = {
97111
},
98112

99113
hasEnoughData() {
100-
return this._rawValues.length === this._count
114+
return this._hasEnoughData
101115
},
102116

103117
getWeightedAverage() {

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

+84-9
Original file line numberDiff line numberDiff line change
@@ -25,17 +25,92 @@ describe('AverageStatValue', () => {
2525
})
2626
})
2727

28-
test('returns whether there are enough values for a meaningful calculation', () => {
29-
const testValues = [100, 200, 150, 123, 30, 50, 22, 33]
30-
const stat = new AverageStatValue(3, STAT_VALUE_TYPE.CUMULATIVE, 3)
31-
const stat2 = new AverageStatValue(3, STAT_VALUE_TYPE.RELATIVE, 3)
28+
describe('returns whether there are enough values for a meaningful calculation', () => {
29+
test('after creating', () => {
30+
const testValues = [100, 200, 150, 123, 30, 50, 22, 33]
31+
const stat = new AverageStatValue(3, STAT_VALUE_TYPE.CUMULATIVE, 3)
32+
const stat2 = new AverageStatValue(3, STAT_VALUE_TYPE.RELATIVE, 3)
3233

33-
testValues.forEach((val, index) => {
34-
stat.add(val)
35-
expect(stat.hasEnoughData()).toBe(index >= 2)
34+
testValues.forEach((val, index) => {
35+
stat.add(val)
36+
expect(stat.hasEnoughData()).toBe(index >= 3)
3637

37-
stat2.add(val)
38-
expect(stat2.hasEnoughData()).toBe(index >= 2)
38+
stat2.add(val)
39+
expect(stat2.hasEnoughData()).toBe(index >= 2)
40+
})
41+
})
42+
43+
describe('resetting', () => {
44+
let stat
45+
let stat2
46+
47+
const addValues = (values) => {
48+
values.forEach(val => {
49+
stat.add(val)
50+
stat2.add(val)
51+
})
52+
}
53+
54+
beforeEach(() => {
55+
stat = new AverageStatValue(3, STAT_VALUE_TYPE.CUMULATIVE, 3)
56+
stat2 = new AverageStatValue(3, STAT_VALUE_TYPE.RELATIVE, 3)
57+
})
58+
59+
test('before having enough values', () => {
60+
addValues([100, 200])
61+
62+
expect(stat.hasEnoughData()).toBe(false)
63+
expect(stat2.hasEnoughData()).toBe(false)
64+
65+
stat.reset()
66+
stat2.reset()
67+
68+
expect(stat.hasEnoughData()).toBe(false)
69+
expect(stat2.hasEnoughData()).toBe(false)
70+
71+
addValues([150, 123])
72+
73+
expect(stat.hasEnoughData()).toBe(false)
74+
expect(stat2.hasEnoughData()).toBe(false)
75+
76+
addValues([30])
77+
78+
expect(stat.hasEnoughData()).toBe(false)
79+
expect(stat2.hasEnoughData()).toBe(true)
80+
81+
addValues([50])
82+
83+
expect(stat.hasEnoughData()).toBe(true)
84+
expect(stat2.hasEnoughData()).toBe(true)
85+
})
86+
87+
test('after having enough values', () => {
88+
addValues([100, 200, 150, 123])
89+
90+
expect(stat.hasEnoughData()).toBe(true)
91+
expect(stat2.hasEnoughData()).toBe(true)
92+
93+
stat.reset()
94+
stat2.reset()
95+
96+
expect(stat.hasEnoughData()).toBe(false)
97+
expect(stat2.hasEnoughData()).toBe(false)
98+
99+
addValues([30, 50])
100+
101+
expect(stat.hasEnoughData()).toBe(false)
102+
expect(stat2.hasEnoughData()).toBe(false)
103+
104+
addValues([22])
105+
106+
expect(stat.hasEnoughData()).toBe(false)
107+
expect(stat2.hasEnoughData()).toBe(true)
108+
109+
addValues([33])
110+
111+
expect(stat.hasEnoughData()).toBe(true)
112+
expect(stat2.hasEnoughData()).toBe(true)
113+
})
39114
})
40115
})
41116

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

+9-1
Original file line numberDiff line numberDiff line change
@@ -612,11 +612,19 @@ PeerConnectionAnalyzer.prototype = {
612612
},
613613

614614
_calculateConnectionQuality(kind) {
615+
const packets = this._packets[kind]
616+
const packetsLost = this._packetsLost[kind]
617+
const timestamps = this._timestamps[kind]
615618
const packetsLostRatio = this._packetsLostRatio[kind]
616619
const packetsPerSecond = this._packetsPerSecond[kind]
617620
const roundTripTime = this._roundTripTime[kind]
618621

619-
if (!packetsLostRatio.hasEnoughData() || !packetsPerSecond.hasEnoughData()) {
622+
// packetsLostRatio and packetsPerSecond are relative values, but they
623+
// are calculated from cumulative values. Therefore, it is necessary to
624+
// check if the cumulative values that are their source have enough data
625+
// or not, rather than checking if the relative values themselves have
626+
// enough data.
627+
if (!packets.hasEnoughData() || !packetsLost.hasEnoughData() || !timestamps.hasEnoughData()) {
620628
return CONNECTION_QUALITY.UNKNOWN
621629
}
622630

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

+44
Original file line numberDiff line numberDiff line change
@@ -95,13 +95,21 @@ function newRTCStatsReport(stats) {
9595
describe('PeerConnectionAnalyzer', () => {
9696

9797
let peerConnectionAnalyzer
98+
let changeConnectionQualityAudioHandler
99+
let changeConnectionQualityVideoHandler
98100
let peerConnection
99101

100102
beforeEach(() => {
101103
jest.useFakeTimers()
102104

103105
peerConnectionAnalyzer = new PeerConnectionAnalyzer()
104106

107+
changeConnectionQualityAudioHandler = jest.fn()
108+
peerConnectionAnalyzer.on('change:connectionQualityAudio', changeConnectionQualityAudioHandler)
109+
110+
changeConnectionQualityVideoHandler = jest.fn()
111+
peerConnectionAnalyzer.on('change:connectionQualityVideo', changeConnectionQualityVideoHandler)
112+
105113
peerConnection = newRTCPeerConnection()
106114
})
107115

@@ -161,8 +169,12 @@ describe('PeerConnectionAnalyzer', () => {
161169

162170
if (kind === 'audio') {
163171
expect(peerConnectionAnalyzer.getConnectionQualityAudio()).toBe(CONNECTION_QUALITY.GOOD)
172+
expect(changeConnectionQualityAudioHandler).toHaveBeenCalledTimes(1)
173+
expect(changeConnectionQualityAudioHandler).toHaveBeenCalledWith(peerConnectionAnalyzer, CONNECTION_QUALITY.GOOD)
164174
} else {
165175
expect(peerConnectionAnalyzer.getConnectionQualityVideo()).toBe(CONNECTION_QUALITY.GOOD)
176+
expect(changeConnectionQualityVideoHandler).toHaveBeenCalledTimes(1)
177+
expect(changeConnectionQualityVideoHandler).toHaveBeenCalledWith(peerConnectionAnalyzer, CONNECTION_QUALITY.GOOD)
166178
}
167179
})
168180

@@ -209,8 +221,12 @@ describe('PeerConnectionAnalyzer', () => {
209221

210222
if (kind === 'audio') {
211223
expect(peerConnectionAnalyzer.getConnectionQualityAudio()).toBe(CONNECTION_QUALITY.MEDIUM)
224+
expect(changeConnectionQualityAudioHandler).toHaveBeenCalledTimes(1)
225+
expect(changeConnectionQualityAudioHandler).toHaveBeenCalledWith(peerConnectionAnalyzer, CONNECTION_QUALITY.MEDIUM)
212226
} else {
213227
expect(peerConnectionAnalyzer.getConnectionQualityVideo()).toBe(CONNECTION_QUALITY.MEDIUM)
228+
expect(changeConnectionQualityVideoHandler).toHaveBeenCalledTimes(1)
229+
expect(changeConnectionQualityVideoHandler).toHaveBeenCalledWith(peerConnectionAnalyzer, CONNECTION_QUALITY.MEDIUM)
214230
}
215231
})
216232

@@ -257,8 +273,12 @@ describe('PeerConnectionAnalyzer', () => {
257273

258274
if (kind === 'audio') {
259275
expect(peerConnectionAnalyzer.getConnectionQualityAudio()).toBe(CONNECTION_QUALITY.BAD)
276+
expect(changeConnectionQualityAudioHandler).toHaveBeenCalledTimes(1)
277+
expect(changeConnectionQualityAudioHandler).toHaveBeenCalledWith(peerConnectionAnalyzer, CONNECTION_QUALITY.BAD)
260278
} else {
261279
expect(peerConnectionAnalyzer.getConnectionQualityVideo()).toBe(CONNECTION_QUALITY.BAD)
280+
expect(changeConnectionQualityVideoHandler).toHaveBeenCalledTimes(1)
281+
expect(changeConnectionQualityVideoHandler).toHaveBeenCalledWith(peerConnectionAnalyzer, CONNECTION_QUALITY.BAD)
262282
}
263283
})
264284

@@ -305,8 +325,12 @@ describe('PeerConnectionAnalyzer', () => {
305325

306326
if (kind === 'audio') {
307327
expect(peerConnectionAnalyzer.getConnectionQualityAudio()).toBe(CONNECTION_QUALITY.VERY_BAD)
328+
expect(changeConnectionQualityAudioHandler).toHaveBeenCalledTimes(1)
329+
expect(changeConnectionQualityAudioHandler).toHaveBeenCalledWith(peerConnectionAnalyzer, CONNECTION_QUALITY.VERY_BAD)
308330
} else {
309331
expect(peerConnectionAnalyzer.getConnectionQualityVideo()).toBe(CONNECTION_QUALITY.VERY_BAD)
332+
expect(changeConnectionQualityVideoHandler).toHaveBeenCalledTimes(1)
333+
expect(changeConnectionQualityVideoHandler).toHaveBeenCalledWith(peerConnectionAnalyzer, CONNECTION_QUALITY.VERY_BAD)
310334
}
311335
})
312336

@@ -353,8 +377,12 @@ describe('PeerConnectionAnalyzer', () => {
353377

354378
if (kind === 'audio') {
355379
expect(peerConnectionAnalyzer.getConnectionQualityAudio()).toBe(CONNECTION_QUALITY.VERY_BAD)
380+
expect(changeConnectionQualityAudioHandler).toHaveBeenCalledTimes(1)
381+
expect(changeConnectionQualityAudioHandler).toHaveBeenCalledWith(peerConnectionAnalyzer, CONNECTION_QUALITY.VERY_BAD)
356382
} else {
357383
expect(peerConnectionAnalyzer.getConnectionQualityVideo()).toBe(CONNECTION_QUALITY.VERY_BAD)
384+
expect(changeConnectionQualityVideoHandler).toHaveBeenCalledTimes(1)
385+
expect(changeConnectionQualityVideoHandler).toHaveBeenCalledWith(peerConnectionAnalyzer, CONNECTION_QUALITY.VERY_BAD)
358386
}
359387
})
360388

@@ -401,8 +429,12 @@ describe('PeerConnectionAnalyzer', () => {
401429

402430
if (kind === 'audio') {
403431
expect(peerConnectionAnalyzer.getConnectionQualityAudio()).toBe(CONNECTION_QUALITY.VERY_BAD)
432+
expect(changeConnectionQualityAudioHandler).toHaveBeenCalledTimes(1)
433+
expect(changeConnectionQualityAudioHandler).toHaveBeenCalledWith(peerConnectionAnalyzer, CONNECTION_QUALITY.VERY_BAD)
404434
} else {
405435
expect(peerConnectionAnalyzer.getConnectionQualityVideo()).toBe(CONNECTION_QUALITY.VERY_BAD)
436+
expect(changeConnectionQualityVideoHandler).toHaveBeenCalledTimes(1)
437+
expect(changeConnectionQualityVideoHandler).toHaveBeenCalledWith(peerConnectionAnalyzer, CONNECTION_QUALITY.VERY_BAD)
406438
}
407439
})
408440

@@ -449,8 +481,12 @@ describe('PeerConnectionAnalyzer', () => {
449481

450482
if (kind === 'audio') {
451483
expect(peerConnectionAnalyzer.getConnectionQualityAudio()).toBe(CONNECTION_QUALITY.NO_TRANSMITTED_DATA)
484+
expect(changeConnectionQualityAudioHandler).toHaveBeenCalledTimes(1)
485+
expect(changeConnectionQualityAudioHandler).toHaveBeenCalledWith(peerConnectionAnalyzer, CONNECTION_QUALITY.NO_TRANSMITTED_DATA)
452486
} else {
453487
expect(peerConnectionAnalyzer.getConnectionQualityVideo()).toBe(CONNECTION_QUALITY.NO_TRANSMITTED_DATA)
488+
expect(changeConnectionQualityVideoHandler).toHaveBeenCalledTimes(1)
489+
expect(changeConnectionQualityVideoHandler).toHaveBeenCalledWith(peerConnectionAnalyzer, CONNECTION_QUALITY.NO_TRANSMITTED_DATA)
454490
}
455491
})
456492

@@ -506,8 +542,12 @@ describe('PeerConnectionAnalyzer', () => {
506542

507543
if (kind === 'audio') {
508544
expect(peerConnectionAnalyzer.getConnectionQualityAudio()).toBe(CONNECTION_QUALITY.NO_TRANSMITTED_DATA)
545+
expect(changeConnectionQualityAudioHandler).toHaveBeenCalledTimes(1)
546+
expect(changeConnectionQualityAudioHandler).toHaveBeenCalledWith(peerConnectionAnalyzer, CONNECTION_QUALITY.NO_TRANSMITTED_DATA)
509547
} else {
510548
expect(peerConnectionAnalyzer.getConnectionQualityVideo()).toBe(CONNECTION_QUALITY.NO_TRANSMITTED_DATA)
549+
expect(changeConnectionQualityVideoHandler).toHaveBeenCalledTimes(1)
550+
expect(changeConnectionQualityVideoHandler).toHaveBeenCalledWith(peerConnectionAnalyzer, CONNECTION_QUALITY.NO_TRANSMITTED_DATA)
511551
}
512552
})
513553

@@ -562,8 +602,12 @@ describe('PeerConnectionAnalyzer', () => {
562602

563603
if (kind === 'audio') {
564604
expect(peerConnectionAnalyzer.getConnectionQualityAudio()).toBe(CONNECTION_QUALITY.GOOD)
605+
expect(changeConnectionQualityAudioHandler).toHaveBeenCalledTimes(1)
606+
expect(changeConnectionQualityAudioHandler).toHaveBeenCalledWith(peerConnectionAnalyzer, CONNECTION_QUALITY.GOOD)
565607
} else {
566608
expect(peerConnectionAnalyzer.getConnectionQualityVideo()).toBe(CONNECTION_QUALITY.GOOD)
609+
expect(changeConnectionQualityVideoHandler).toHaveBeenCalledTimes(1)
610+
expect(changeConnectionQualityVideoHandler).toHaveBeenCalledWith(peerConnectionAnalyzer, CONNECTION_QUALITY.GOOD)
567611
}
568612
})
569613
})

0 commit comments

Comments
 (0)