Skip to content

Commit 4343a87

Browse files
danxuliudanxuliu
committed
fix: Do not report very bad quality with a low packets count
In the past it was observed that, even for small videos, 10 packets per second was a reasonable threshold to detect connection issues even if there were no lost packets. However, nowadays it seems that it can sometimes trigger a false positive (typically when the background blur is enabled and the video quality is reduced due to being in a call with several participants), so for now the connection problem is no longer reported to the user but just logged. Signed-off-by: Daniel Calviño Sánchez <[email protected]>
1 parent 53feed9 commit 4343a87

File tree

2 files changed

+151
-14
lines changed

2 files changed

+151
-14
lines changed

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

+5-2
Original file line numberDiff line numberDiff line change
@@ -680,10 +680,13 @@ PeerConnectionAnalyzer.prototype = {
680680
// quality to keep a smooth video, albeit on a lower resolution. Thus
681681
// with a threshold of 10 packets issues can be detected too for videos,
682682
// although only once they can not be further downscaled.
683+
// Despite all of the above it has been observed that less than 10
684+
// packets are sometimes sent without any connection problem (for
685+
// example, when the background is blurred and the video quality is
686+
// reduced due to being in a call with several participants), so for now
687+
// it is only logged but not reported.
683688
if (packetsPerSecond.getWeightedAverage() < 10) {
684689
this._logStats(kind, 'Low packets per second: ' + packetsPerSecond.getWeightedAverage())
685-
686-
return CONNECTION_QUALITY.VERY_BAD
687690
}
688691

689692
if (packetsLostRatioWeightedAverage > 0.3) {

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

+146-12
Original file line numberDiff line numberDiff line change
@@ -663,8 +663,142 @@ describe('PeerConnectionAnalyzer', () => {
663663
})
664664

665665
test.each([
666-
['very bad quality due to low packets', 'audio'],
667-
['very bad quality due to low packets', 'video'],
666+
['very bad quality with low packets and packet loss', 'audio'],
667+
['very bad quality with low packets and packet loss', 'video'],
668+
])('%s, %s', async (name, kind) => {
669+
peerConnection.getStats
670+
.mockResolvedValueOnce(newRTCStatsReport([
671+
{ type: 'outbound-rtp', kind, packetsSent: 5, timestamp: 10000 },
672+
{ type: 'remote-inbound-rtp', kind, packetsReceived: 3, timestamp: 10000, packetsLost: 2, roundTripTime: 0.1 },
673+
]))
674+
.mockResolvedValueOnce(newRTCStatsReport([
675+
{ type: 'outbound-rtp', kind, packetsSent: 10, timestamp: 11000 },
676+
{ type: 'remote-inbound-rtp', kind, packetsReceived: 6, timestamp: 11000, packetsLost: 4, roundTripTime: 0.1 },
677+
]))
678+
.mockResolvedValueOnce(newRTCStatsReport([
679+
{ type: 'outbound-rtp', kind, packetsSent: 15, timestamp: 11950 },
680+
{ type: 'remote-inbound-rtp', kind, packetsReceived: 9, timestamp: 11950, packetsLost: 6, roundTripTime: 0.1 },
681+
]))
682+
.mockResolvedValueOnce(newRTCStatsReport([
683+
{ type: 'outbound-rtp', kind, packetsSent: 20, timestamp: 13020 },
684+
{ type: 'remote-inbound-rtp', kind, packetsReceived: 12, timestamp: 13020, packetsLost: 8, roundTripTime: 0.1 },
685+
]))
686+
.mockResolvedValueOnce(newRTCStatsReport([
687+
{ type: 'outbound-rtp', kind, packetsSent: 25, timestamp: 14010 },
688+
{ type: 'remote-inbound-rtp', kind, packetsReceived: 15, timestamp: 14010, packetsLost: 10, roundTripTime: 0.1 },
689+
]))
690+
// A sixth report is needed for the initial calculation due to
691+
// the first stats report being used as the base to calculate
692+
// relative values of cumulative stats.
693+
.mockResolvedValueOnce(newRTCStatsReport([
694+
{ type: 'outbound-rtp', kind, packetsSent: 30, timestamp: 14985 },
695+
{ type: 'remote-inbound-rtp', kind, packetsReceived: 18, timestamp: 14985, packetsLost: 12, roundTripTime: 0.1 },
696+
]))
697+
698+
peerConnectionAnalyzer.setPeerConnection(peerConnection, PEER_DIRECTION.SENDER)
699+
700+
jest.advanceTimersByTime(5000)
701+
// Force the promises returning the stats to be executed.
702+
await null
703+
704+
expect(peerConnection.getStats).toHaveBeenCalledTimes(5)
705+
706+
expect(peerConnectionAnalyzer.getConnectionQualityAudio()).toBe(CONNECTION_QUALITY.UNKNOWN)
707+
expect(peerConnectionAnalyzer.getConnectionQualityVideo()).toBe(CONNECTION_QUALITY.UNKNOWN)
708+
expect(changeConnectionQualityAudioHandler).toHaveBeenCalledTimes(0)
709+
expect(changeConnectionQualityVideoHandler).toHaveBeenCalledTimes(0)
710+
711+
jest.advanceTimersByTime(1000)
712+
// Force the promises returning the stats to be executed.
713+
await null
714+
715+
expect(peerConnection.getStats).toHaveBeenCalledTimes(6)
716+
717+
if (kind === 'audio') {
718+
expect(peerConnectionAnalyzer.getConnectionQualityAudio()).toBe(CONNECTION_QUALITY.VERY_BAD)
719+
expect(peerConnectionAnalyzer.getConnectionQualityVideo()).toBe(CONNECTION_QUALITY.UNKNOWN)
720+
expect(changeConnectionQualityAudioHandler).toHaveBeenCalledTimes(1)
721+
expect(changeConnectionQualityAudioHandler).toHaveBeenCalledWith(peerConnectionAnalyzer, CONNECTION_QUALITY.VERY_BAD)
722+
expect(changeConnectionQualityVideoHandler).toHaveBeenCalledTimes(0)
723+
} else {
724+
expect(peerConnectionAnalyzer.getConnectionQualityAudio()).toBe(CONNECTION_QUALITY.UNKNOWN)
725+
expect(peerConnectionAnalyzer.getConnectionQualityVideo()).toBe(CONNECTION_QUALITY.VERY_BAD)
726+
expect(changeConnectionQualityAudioHandler).toHaveBeenCalledTimes(0)
727+
expect(changeConnectionQualityVideoHandler).toHaveBeenCalledTimes(1)
728+
expect(changeConnectionQualityVideoHandler).toHaveBeenCalledWith(peerConnectionAnalyzer, CONNECTION_QUALITY.VERY_BAD)
729+
}
730+
})
731+
732+
test.each([
733+
['good quality even with low packets if no packet loss, missing remote packet count', 'audio'],
734+
['good quality even with low packets if no packet loss, missing remote packet count', 'video'],
735+
])('%s, %s', async (name, kind) => {
736+
peerConnection.getStats
737+
.mockResolvedValueOnce(newRTCStatsReport([
738+
{ type: 'outbound-rtp', kind, packetsSent: 5, timestamp: 10000 },
739+
{ type: 'remote-inbound-rtp', kind, timestamp: 10000, packetsLost: 2, roundTripTime: 0.1 },
740+
]))
741+
.mockResolvedValueOnce(newRTCStatsReport([
742+
{ type: 'outbound-rtp', kind, packetsSent: 10, timestamp: 11000 },
743+
{ type: 'remote-inbound-rtp', kind, timestamp: 11000, packetsLost: 4, roundTripTime: 0.1 },
744+
]))
745+
.mockResolvedValueOnce(newRTCStatsReport([
746+
{ type: 'outbound-rtp', kind, packetsSent: 15, timestamp: 11950 },
747+
{ type: 'remote-inbound-rtp', kind, timestamp: 11950, packetsLost: 6, roundTripTime: 0.1 },
748+
]))
749+
.mockResolvedValueOnce(newRTCStatsReport([
750+
{ type: 'outbound-rtp', kind, packetsSent: 20, timestamp: 13020 },
751+
{ type: 'remote-inbound-rtp', kind, timestamp: 13020, packetsLost: 8, roundTripTime: 0.1 },
752+
]))
753+
.mockResolvedValueOnce(newRTCStatsReport([
754+
{ type: 'outbound-rtp', kind, packetsSent: 25, timestamp: 14010 },
755+
{ type: 'remote-inbound-rtp', kind, timestamp: 14010, packetsLost: 10, roundTripTime: 0.1 },
756+
]))
757+
// A sixth report is needed for the initial calculation due to
758+
// the first stats report being used as the base to calculate
759+
// relative values of cumulative stats.
760+
.mockResolvedValueOnce(newRTCStatsReport([
761+
{ type: 'outbound-rtp', kind, packetsSent: 30, timestamp: 14985 },
762+
{ type: 'remote-inbound-rtp', kind, timestamp: 14985, packetsLost: 12, roundTripTime: 0.1 },
763+
]))
764+
765+
peerConnectionAnalyzer.setPeerConnection(peerConnection, PEER_DIRECTION.SENDER)
766+
767+
jest.advanceTimersByTime(5000)
768+
// Force the promises returning the stats to be executed.
769+
await null
770+
771+
expect(peerConnection.getStats).toHaveBeenCalledTimes(5)
772+
773+
expect(peerConnectionAnalyzer.getConnectionQualityAudio()).toBe(CONNECTION_QUALITY.UNKNOWN)
774+
expect(peerConnectionAnalyzer.getConnectionQualityVideo()).toBe(CONNECTION_QUALITY.UNKNOWN)
775+
expect(changeConnectionQualityAudioHandler).toHaveBeenCalledTimes(0)
776+
expect(changeConnectionQualityVideoHandler).toHaveBeenCalledTimes(0)
777+
778+
jest.advanceTimersByTime(1000)
779+
// Force the promises returning the stats to be executed.
780+
await null
781+
782+
expect(peerConnection.getStats).toHaveBeenCalledTimes(6)
783+
784+
if (kind === 'audio') {
785+
expect(peerConnectionAnalyzer.getConnectionQualityAudio()).toBe(CONNECTION_QUALITY.VERY_BAD)
786+
expect(peerConnectionAnalyzer.getConnectionQualityVideo()).toBe(CONNECTION_QUALITY.UNKNOWN)
787+
expect(changeConnectionQualityAudioHandler).toHaveBeenCalledTimes(1)
788+
expect(changeConnectionQualityAudioHandler).toHaveBeenCalledWith(peerConnectionAnalyzer, CONNECTION_QUALITY.VERY_BAD)
789+
expect(changeConnectionQualityVideoHandler).toHaveBeenCalledTimes(0)
790+
} else {
791+
expect(peerConnectionAnalyzer.getConnectionQualityAudio()).toBe(CONNECTION_QUALITY.UNKNOWN)
792+
expect(peerConnectionAnalyzer.getConnectionQualityVideo()).toBe(CONNECTION_QUALITY.VERY_BAD)
793+
expect(changeConnectionQualityAudioHandler).toHaveBeenCalledTimes(0)
794+
expect(changeConnectionQualityVideoHandler).toHaveBeenCalledTimes(1)
795+
expect(changeConnectionQualityVideoHandler).toHaveBeenCalledWith(peerConnectionAnalyzer, CONNECTION_QUALITY.VERY_BAD)
796+
}
797+
})
798+
799+
test.each([
800+
['good quality even with low packets if no packet loss', 'audio'],
801+
['good quality even with low packets if no packet loss', 'video'],
668802
])('%s, %s', async (name, kind) => {
669803
peerConnection.getStats
670804
.mockResolvedValueOnce(newRTCStatsReport([
@@ -715,23 +849,23 @@ describe('PeerConnectionAnalyzer', () => {
715849
expect(peerConnection.getStats).toHaveBeenCalledTimes(6)
716850

717851
if (kind === 'audio') {
718-
expect(peerConnectionAnalyzer.getConnectionQualityAudio()).toBe(CONNECTION_QUALITY.VERY_BAD)
852+
expect(peerConnectionAnalyzer.getConnectionQualityAudio()).toBe(CONNECTION_QUALITY.GOOD)
719853
expect(peerConnectionAnalyzer.getConnectionQualityVideo()).toBe(CONNECTION_QUALITY.UNKNOWN)
720854
expect(changeConnectionQualityAudioHandler).toHaveBeenCalledTimes(1)
721-
expect(changeConnectionQualityAudioHandler).toHaveBeenCalledWith(peerConnectionAnalyzer, CONNECTION_QUALITY.VERY_BAD)
855+
expect(changeConnectionQualityAudioHandler).toHaveBeenCalledWith(peerConnectionAnalyzer, CONNECTION_QUALITY.GOOD)
722856
expect(changeConnectionQualityVideoHandler).toHaveBeenCalledTimes(0)
723857
} else {
724858
expect(peerConnectionAnalyzer.getConnectionQualityAudio()).toBe(CONNECTION_QUALITY.UNKNOWN)
725-
expect(peerConnectionAnalyzer.getConnectionQualityVideo()).toBe(CONNECTION_QUALITY.VERY_BAD)
859+
expect(peerConnectionAnalyzer.getConnectionQualityVideo()).toBe(CONNECTION_QUALITY.GOOD)
726860
expect(changeConnectionQualityAudioHandler).toHaveBeenCalledTimes(0)
727861
expect(changeConnectionQualityVideoHandler).toHaveBeenCalledTimes(1)
728-
expect(changeConnectionQualityVideoHandler).toHaveBeenCalledWith(peerConnectionAnalyzer, CONNECTION_QUALITY.VERY_BAD)
862+
expect(changeConnectionQualityVideoHandler).toHaveBeenCalledWith(peerConnectionAnalyzer, CONNECTION_QUALITY.GOOD)
729863
}
730864
})
731865

732866
test.each([
733-
['very bad quality due to low packets, missing remote packet count', 'audio'],
734-
['very bad quality due to low packets, missing remote packet count', 'video'],
867+
['good quality even with low packets if no packet loss, missing remote packet count', 'audio'],
868+
['good quality even with low packets if no packet loss, missing remote packet count', 'video'],
735869
])('%s, %s', async (name, kind) => {
736870
peerConnection.getStats
737871
.mockResolvedValueOnce(newRTCStatsReport([
@@ -782,17 +916,17 @@ describe('PeerConnectionAnalyzer', () => {
782916
expect(peerConnection.getStats).toHaveBeenCalledTimes(6)
783917

784918
if (kind === 'audio') {
785-
expect(peerConnectionAnalyzer.getConnectionQualityAudio()).toBe(CONNECTION_QUALITY.VERY_BAD)
919+
expect(peerConnectionAnalyzer.getConnectionQualityAudio()).toBe(CONNECTION_QUALITY.GOOD)
786920
expect(peerConnectionAnalyzer.getConnectionQualityVideo()).toBe(CONNECTION_QUALITY.UNKNOWN)
787921
expect(changeConnectionQualityAudioHandler).toHaveBeenCalledTimes(1)
788-
expect(changeConnectionQualityAudioHandler).toHaveBeenCalledWith(peerConnectionAnalyzer, CONNECTION_QUALITY.VERY_BAD)
922+
expect(changeConnectionQualityAudioHandler).toHaveBeenCalledWith(peerConnectionAnalyzer, CONNECTION_QUALITY.GOOD)
789923
expect(changeConnectionQualityVideoHandler).toHaveBeenCalledTimes(0)
790924
} else {
791925
expect(peerConnectionAnalyzer.getConnectionQualityAudio()).toBe(CONNECTION_QUALITY.UNKNOWN)
792-
expect(peerConnectionAnalyzer.getConnectionQualityVideo()).toBe(CONNECTION_QUALITY.VERY_BAD)
926+
expect(peerConnectionAnalyzer.getConnectionQualityVideo()).toBe(CONNECTION_QUALITY.GOOD)
793927
expect(changeConnectionQualityAudioHandler).toHaveBeenCalledTimes(0)
794928
expect(changeConnectionQualityVideoHandler).toHaveBeenCalledTimes(1)
795-
expect(changeConnectionQualityVideoHandler).toHaveBeenCalledWith(peerConnectionAnalyzer, CONNECTION_QUALITY.VERY_BAD)
929+
expect(changeConnectionQualityVideoHandler).toHaveBeenCalledWith(peerConnectionAnalyzer, CONNECTION_QUALITY.GOOD)
796930
}
797931
})
798932

0 commit comments

Comments
 (0)