From 0f449de6b198c2b0e3073ed0e82561d8d34b6df0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Calvi=C3=B1o=20S=C3=A1nchez?= Date: Thu, 23 Jan 2025 11:36:16 +0100 Subject: [PATCH] fix: Do not report very bad quality with a low packets count MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../analyzers/PeerConnectionAnalyzer.js | 7 +- .../analyzers/PeerConnectionAnalyzer.spec.js | 136 ++++++++++++++++-- 2 files changed, 129 insertions(+), 14 deletions(-) diff --git a/src/utils/webrtc/analyzers/PeerConnectionAnalyzer.js b/src/utils/webrtc/analyzers/PeerConnectionAnalyzer.js index 6bd537f534f..17dee309872 100644 --- a/src/utils/webrtc/analyzers/PeerConnectionAnalyzer.js +++ b/src/utils/webrtc/analyzers/PeerConnectionAnalyzer.js @@ -680,10 +680,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) { diff --git a/src/utils/webrtc/analyzers/PeerConnectionAnalyzer.spec.js b/src/utils/webrtc/analyzers/PeerConnectionAnalyzer.spec.js index 80d509720eb..d0e6f04fba5 100644 --- a/src/utils/webrtc/analyzers/PeerConnectionAnalyzer.spec.js +++ b/src/utils/webrtc/analyzers/PeerConnectionAnalyzer.spec.js @@ -575,8 +575,120 @@ describe('PeerConnectionAnalyzer', () => { }) test.each([ - ['very bad quality due to low packets', 'audio'], - ['very bad quality due to low packets', 'video'], + ['very bad quality with low packets and packet loss', 'audio'], + ['very bad quality with low packets and packet loss', 'video'], + ])('%s, %s', async (name, kind) => { + peerConnection.getStats + .mockResolvedValueOnce(newRTCStatsReport([ + { type: 'outbound-rtp', kind, packetsSent: 5, timestamp: 10000 }, + { type: 'remote-inbound-rtp', kind, packetsReceived: 3, timestamp: 10000, packetsLost: 2, roundTripTime: 0.1 }, + ])) + .mockResolvedValueOnce(newRTCStatsReport([ + { type: 'outbound-rtp', kind, packetsSent: 10, timestamp: 11000 }, + { type: 'remote-inbound-rtp', kind, packetsReceived: 6, timestamp: 11000, packetsLost: 4, roundTripTime: 0.1 }, + ])) + .mockResolvedValueOnce(newRTCStatsReport([ + { type: 'outbound-rtp', kind, packetsSent: 15, timestamp: 11950 }, + { type: 'remote-inbound-rtp', kind, packetsReceived: 9, timestamp: 11950, packetsLost: 6, roundTripTime: 0.1 }, + ])) + .mockResolvedValueOnce(newRTCStatsReport([ + { type: 'outbound-rtp', kind, packetsSent: 20, timestamp: 13020 }, + { type: 'remote-inbound-rtp', kind, packetsReceived: 12, timestamp: 13020, packetsLost: 8, roundTripTime: 0.1 }, + ])) + .mockResolvedValueOnce(newRTCStatsReport([ + { type: 'outbound-rtp', kind, packetsSent: 25, timestamp: 14010 }, + { type: 'remote-inbound-rtp', kind, packetsReceived: 15, timestamp: 14010, packetsLost: 10, roundTripTime: 0.1 }, + ])) + // A sixth report is needed for the initial calculation due to + // the first stats report being used as the base to calculate + // relative values of cumulative stats. + .mockResolvedValueOnce(newRTCStatsReport([ + { type: 'outbound-rtp', kind, packetsSent: 30, timestamp: 14985 }, + { type: 'remote-inbound-rtp', kind, packetsReceived: 18, timestamp: 14985, packetsLost: 12, roundTripTime: 0.1 }, + ])) + + peerConnectionAnalyzer.setPeerConnection(peerConnection, PEER_DIRECTION.SENDER) + + jest.advanceTimersByTime(6000) + // Force the promises returning the stats to be executed. + await null + + expect(peerConnection.getStats).toHaveBeenCalledTimes(6) + + if (kind === 'audio') { + expect(peerConnectionAnalyzer.getConnectionQualityAudio()).toBe(CONNECTION_QUALITY.VERY_BAD) + expect(peerConnectionAnalyzer.getConnectionQualityVideo()).toBe(CONNECTION_QUALITY.UNKNOWN) + expect(changeConnectionQualityAudioHandler).toHaveBeenCalledTimes(1) + expect(changeConnectionQualityAudioHandler).toHaveBeenCalledWith(peerConnectionAnalyzer, CONNECTION_QUALITY.VERY_BAD) + expect(changeConnectionQualityVideoHandler).toHaveBeenCalledTimes(0) + } else { + expect(peerConnectionAnalyzer.getConnectionQualityAudio()).toBe(CONNECTION_QUALITY.UNKNOWN) + expect(peerConnectionAnalyzer.getConnectionQualityVideo()).toBe(CONNECTION_QUALITY.VERY_BAD) + expect(changeConnectionQualityAudioHandler).toHaveBeenCalledTimes(0) + expect(changeConnectionQualityVideoHandler).toHaveBeenCalledTimes(1) + expect(changeConnectionQualityVideoHandler).toHaveBeenCalledWith(peerConnectionAnalyzer, CONNECTION_QUALITY.VERY_BAD) + } + }) + + test.each([ + ['good quality even with low packets if no packet loss, missing remote packet count', 'audio'], + ['good quality even with low packets if no packet loss, missing remote packet count', 'video'], + ])('%s, %s', async (name, kind) => { + peerConnection.getStats + .mockResolvedValueOnce(newRTCStatsReport([ + { type: 'outbound-rtp', kind, packetsSent: 5, timestamp: 10000 }, + { type: 'remote-inbound-rtp', kind, timestamp: 10000, packetsLost: 2, roundTripTime: 0.1 }, + ])) + .mockResolvedValueOnce(newRTCStatsReport([ + { type: 'outbound-rtp', kind, packetsSent: 10, timestamp: 11000 }, + { type: 'remote-inbound-rtp', kind, timestamp: 11000, packetsLost: 4, roundTripTime: 0.1 }, + ])) + .mockResolvedValueOnce(newRTCStatsReport([ + { type: 'outbound-rtp', kind, packetsSent: 15, timestamp: 11950 }, + { type: 'remote-inbound-rtp', kind, timestamp: 11950, packetsLost: 6, roundTripTime: 0.1 }, + ])) + .mockResolvedValueOnce(newRTCStatsReport([ + { type: 'outbound-rtp', kind, packetsSent: 20, timestamp: 13020 }, + { type: 'remote-inbound-rtp', kind, timestamp: 13020, packetsLost: 8, roundTripTime: 0.1 }, + ])) + .mockResolvedValueOnce(newRTCStatsReport([ + { type: 'outbound-rtp', kind, packetsSent: 25, timestamp: 14010 }, + { type: 'remote-inbound-rtp', kind, timestamp: 14010, packetsLost: 10, roundTripTime: 0.1 }, + ])) + // A sixth report is needed for the initial calculation due to + // the first stats report being used as the base to calculate + // relative values of cumulative stats. + .mockResolvedValueOnce(newRTCStatsReport([ + { type: 'outbound-rtp', kind, packetsSent: 30, timestamp: 14985 }, + { type: 'remote-inbound-rtp', kind, timestamp: 14985, packetsLost: 12, roundTripTime: 0.1 }, + ])) + + peerConnectionAnalyzer.setPeerConnection(peerConnection, PEER_DIRECTION.SENDER) + + jest.advanceTimersByTime(6000) + // Force the promises returning the stats to be executed. + await null + + expect(peerConnection.getStats).toHaveBeenCalledTimes(6) + + if (kind === 'audio') { + expect(peerConnectionAnalyzer.getConnectionQualityAudio()).toBe(CONNECTION_QUALITY.VERY_BAD) + expect(peerConnectionAnalyzer.getConnectionQualityVideo()).toBe(CONNECTION_QUALITY.UNKNOWN) + expect(changeConnectionQualityAudioHandler).toHaveBeenCalledTimes(1) + expect(changeConnectionQualityAudioHandler).toHaveBeenCalledWith(peerConnectionAnalyzer, CONNECTION_QUALITY.VERY_BAD) + expect(changeConnectionQualityVideoHandler).toHaveBeenCalledTimes(0) + } else { + expect(peerConnectionAnalyzer.getConnectionQualityAudio()).toBe(CONNECTION_QUALITY.UNKNOWN) + expect(peerConnectionAnalyzer.getConnectionQualityVideo()).toBe(CONNECTION_QUALITY.VERY_BAD) + expect(changeConnectionQualityAudioHandler).toHaveBeenCalledTimes(0) + expect(changeConnectionQualityVideoHandler).toHaveBeenCalledTimes(1) + expect(changeConnectionQualityVideoHandler).toHaveBeenCalledWith(peerConnectionAnalyzer, CONNECTION_QUALITY.VERY_BAD) + } + }) + + test.each([ + ['good quality even with low packets if no packet loss', 'audio'], + ['good quality even with low packets if no packet loss', 'video'], ])('%s, %s', async (name, kind) => { peerConnection.getStats .mockResolvedValueOnce(newRTCStatsReport([ @@ -616,23 +728,23 @@ describe('PeerConnectionAnalyzer', () => { expect(peerConnection.getStats).toHaveBeenCalledTimes(6) if (kind === 'audio') { - expect(peerConnectionAnalyzer.getConnectionQualityAudio()).toBe(CONNECTION_QUALITY.VERY_BAD) + expect(peerConnectionAnalyzer.getConnectionQualityAudio()).toBe(CONNECTION_QUALITY.GOOD) expect(peerConnectionAnalyzer.getConnectionQualityVideo()).toBe(CONNECTION_QUALITY.UNKNOWN) expect(changeConnectionQualityAudioHandler).toHaveBeenCalledTimes(1) - expect(changeConnectionQualityAudioHandler).toHaveBeenCalledWith(peerConnectionAnalyzer, CONNECTION_QUALITY.VERY_BAD) + expect(changeConnectionQualityAudioHandler).toHaveBeenCalledWith(peerConnectionAnalyzer, CONNECTION_QUALITY.GOOD) expect(changeConnectionQualityVideoHandler).toHaveBeenCalledTimes(0) } else { expect(peerConnectionAnalyzer.getConnectionQualityAudio()).toBe(CONNECTION_QUALITY.UNKNOWN) - expect(peerConnectionAnalyzer.getConnectionQualityVideo()).toBe(CONNECTION_QUALITY.VERY_BAD) + expect(peerConnectionAnalyzer.getConnectionQualityVideo()).toBe(CONNECTION_QUALITY.GOOD) expect(changeConnectionQualityAudioHandler).toHaveBeenCalledTimes(0) expect(changeConnectionQualityVideoHandler).toHaveBeenCalledTimes(1) - expect(changeConnectionQualityVideoHandler).toHaveBeenCalledWith(peerConnectionAnalyzer, CONNECTION_QUALITY.VERY_BAD) + expect(changeConnectionQualityVideoHandler).toHaveBeenCalledWith(peerConnectionAnalyzer, CONNECTION_QUALITY.GOOD) } }) test.each([ - ['very bad quality due to low packets, missing remote packet count', 'audio'], - ['very bad quality due to low packets, missing remote packet count', 'video'], + ['good quality even with low packets if no packet loss, missing remote packet count', 'audio'], + ['good quality even with low packets if no packet loss, missing remote packet count', 'video'], ])('%s, %s', async (name, kind) => { peerConnection.getStats .mockResolvedValueOnce(newRTCStatsReport([ @@ -672,17 +784,17 @@ describe('PeerConnectionAnalyzer', () => { expect(peerConnection.getStats).toHaveBeenCalledTimes(6) if (kind === 'audio') { - expect(peerConnectionAnalyzer.getConnectionQualityAudio()).toBe(CONNECTION_QUALITY.VERY_BAD) + expect(peerConnectionAnalyzer.getConnectionQualityAudio()).toBe(CONNECTION_QUALITY.GOOD) expect(peerConnectionAnalyzer.getConnectionQualityVideo()).toBe(CONNECTION_QUALITY.UNKNOWN) expect(changeConnectionQualityAudioHandler).toHaveBeenCalledTimes(1) - expect(changeConnectionQualityAudioHandler).toHaveBeenCalledWith(peerConnectionAnalyzer, CONNECTION_QUALITY.VERY_BAD) + expect(changeConnectionQualityAudioHandler).toHaveBeenCalledWith(peerConnectionAnalyzer, CONNECTION_QUALITY.GOOD) expect(changeConnectionQualityVideoHandler).toHaveBeenCalledTimes(0) } else { expect(peerConnectionAnalyzer.getConnectionQualityAudio()).toBe(CONNECTION_QUALITY.UNKNOWN) - expect(peerConnectionAnalyzer.getConnectionQualityVideo()).toBe(CONNECTION_QUALITY.VERY_BAD) + expect(peerConnectionAnalyzer.getConnectionQualityVideo()).toBe(CONNECTION_QUALITY.GOOD) expect(changeConnectionQualityAudioHandler).toHaveBeenCalledTimes(0) expect(changeConnectionQualityVideoHandler).toHaveBeenCalledTimes(1) - expect(changeConnectionQualityVideoHandler).toHaveBeenCalledWith(peerConnectionAnalyzer, CONNECTION_QUALITY.VERY_BAD) + expect(changeConnectionQualityVideoHandler).toHaveBeenCalledWith(peerConnectionAnalyzer, CONNECTION_QUALITY.GOOD) } })