Skip to content

Commit 0f449de

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 968629d commit 0f449de

File tree

2 files changed

+129
-14
lines changed

2 files changed

+129
-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

+124-12
Original file line numberDiff line numberDiff line change
@@ -575,8 +575,120 @@ describe('PeerConnectionAnalyzer', () => {
575575
})
576576

577577
test.each([
578-
['very bad quality due to low packets', 'audio'],
579-
['very bad quality due to low packets', 'video'],
578+
['very bad quality with low packets and packet loss', 'audio'],
579+
['very bad quality with low packets and packet loss', 'video'],
580+
])('%s, %s', async (name, kind) => {
581+
peerConnection.getStats
582+
.mockResolvedValueOnce(newRTCStatsReport([
583+
{ type: 'outbound-rtp', kind, packetsSent: 5, timestamp: 10000 },
584+
{ type: 'remote-inbound-rtp', kind, packetsReceived: 3, timestamp: 10000, packetsLost: 2, roundTripTime: 0.1 },
585+
]))
586+
.mockResolvedValueOnce(newRTCStatsReport([
587+
{ type: 'outbound-rtp', kind, packetsSent: 10, timestamp: 11000 },
588+
{ type: 'remote-inbound-rtp', kind, packetsReceived: 6, timestamp: 11000, packetsLost: 4, roundTripTime: 0.1 },
589+
]))
590+
.mockResolvedValueOnce(newRTCStatsReport([
591+
{ type: 'outbound-rtp', kind, packetsSent: 15, timestamp: 11950 },
592+
{ type: 'remote-inbound-rtp', kind, packetsReceived: 9, timestamp: 11950, packetsLost: 6, roundTripTime: 0.1 },
593+
]))
594+
.mockResolvedValueOnce(newRTCStatsReport([
595+
{ type: 'outbound-rtp', kind, packetsSent: 20, timestamp: 13020 },
596+
{ type: 'remote-inbound-rtp', kind, packetsReceived: 12, timestamp: 13020, packetsLost: 8, roundTripTime: 0.1 },
597+
]))
598+
.mockResolvedValueOnce(newRTCStatsReport([
599+
{ type: 'outbound-rtp', kind, packetsSent: 25, timestamp: 14010 },
600+
{ type: 'remote-inbound-rtp', kind, packetsReceived: 15, timestamp: 14010, packetsLost: 10, roundTripTime: 0.1 },
601+
]))
602+
// A sixth report is needed for the initial calculation due to
603+
// the first stats report being used as the base to calculate
604+
// relative values of cumulative stats.
605+
.mockResolvedValueOnce(newRTCStatsReport([
606+
{ type: 'outbound-rtp', kind, packetsSent: 30, timestamp: 14985 },
607+
{ type: 'remote-inbound-rtp', kind, packetsReceived: 18, timestamp: 14985, packetsLost: 12, roundTripTime: 0.1 },
608+
]))
609+
610+
peerConnectionAnalyzer.setPeerConnection(peerConnection, PEER_DIRECTION.SENDER)
611+
612+
jest.advanceTimersByTime(6000)
613+
// Force the promises returning the stats to be executed.
614+
await null
615+
616+
expect(peerConnection.getStats).toHaveBeenCalledTimes(6)
617+
618+
if (kind === 'audio') {
619+
expect(peerConnectionAnalyzer.getConnectionQualityAudio()).toBe(CONNECTION_QUALITY.VERY_BAD)
620+
expect(peerConnectionAnalyzer.getConnectionQualityVideo()).toBe(CONNECTION_QUALITY.UNKNOWN)
621+
expect(changeConnectionQualityAudioHandler).toHaveBeenCalledTimes(1)
622+
expect(changeConnectionQualityAudioHandler).toHaveBeenCalledWith(peerConnectionAnalyzer, CONNECTION_QUALITY.VERY_BAD)
623+
expect(changeConnectionQualityVideoHandler).toHaveBeenCalledTimes(0)
624+
} else {
625+
expect(peerConnectionAnalyzer.getConnectionQualityAudio()).toBe(CONNECTION_QUALITY.UNKNOWN)
626+
expect(peerConnectionAnalyzer.getConnectionQualityVideo()).toBe(CONNECTION_QUALITY.VERY_BAD)
627+
expect(changeConnectionQualityAudioHandler).toHaveBeenCalledTimes(0)
628+
expect(changeConnectionQualityVideoHandler).toHaveBeenCalledTimes(1)
629+
expect(changeConnectionQualityVideoHandler).toHaveBeenCalledWith(peerConnectionAnalyzer, CONNECTION_QUALITY.VERY_BAD)
630+
}
631+
})
632+
633+
test.each([
634+
['good quality even with low packets if no packet loss, missing remote packet count', 'audio'],
635+
['good quality even with low packets if no packet loss, missing remote packet count', 'video'],
636+
])('%s, %s', async (name, kind) => {
637+
peerConnection.getStats
638+
.mockResolvedValueOnce(newRTCStatsReport([
639+
{ type: 'outbound-rtp', kind, packetsSent: 5, timestamp: 10000 },
640+
{ type: 'remote-inbound-rtp', kind, timestamp: 10000, packetsLost: 2, roundTripTime: 0.1 },
641+
]))
642+
.mockResolvedValueOnce(newRTCStatsReport([
643+
{ type: 'outbound-rtp', kind, packetsSent: 10, timestamp: 11000 },
644+
{ type: 'remote-inbound-rtp', kind, timestamp: 11000, packetsLost: 4, roundTripTime: 0.1 },
645+
]))
646+
.mockResolvedValueOnce(newRTCStatsReport([
647+
{ type: 'outbound-rtp', kind, packetsSent: 15, timestamp: 11950 },
648+
{ type: 'remote-inbound-rtp', kind, timestamp: 11950, packetsLost: 6, roundTripTime: 0.1 },
649+
]))
650+
.mockResolvedValueOnce(newRTCStatsReport([
651+
{ type: 'outbound-rtp', kind, packetsSent: 20, timestamp: 13020 },
652+
{ type: 'remote-inbound-rtp', kind, timestamp: 13020, packetsLost: 8, roundTripTime: 0.1 },
653+
]))
654+
.mockResolvedValueOnce(newRTCStatsReport([
655+
{ type: 'outbound-rtp', kind, packetsSent: 25, timestamp: 14010 },
656+
{ type: 'remote-inbound-rtp', kind, timestamp: 14010, packetsLost: 10, roundTripTime: 0.1 },
657+
]))
658+
// A sixth report is needed for the initial calculation due to
659+
// the first stats report being used as the base to calculate
660+
// relative values of cumulative stats.
661+
.mockResolvedValueOnce(newRTCStatsReport([
662+
{ type: 'outbound-rtp', kind, packetsSent: 30, timestamp: 14985 },
663+
{ type: 'remote-inbound-rtp', kind, timestamp: 14985, packetsLost: 12, roundTripTime: 0.1 },
664+
]))
665+
666+
peerConnectionAnalyzer.setPeerConnection(peerConnection, PEER_DIRECTION.SENDER)
667+
668+
jest.advanceTimersByTime(6000)
669+
// Force the promises returning the stats to be executed.
670+
await null
671+
672+
expect(peerConnection.getStats).toHaveBeenCalledTimes(6)
673+
674+
if (kind === 'audio') {
675+
expect(peerConnectionAnalyzer.getConnectionQualityAudio()).toBe(CONNECTION_QUALITY.VERY_BAD)
676+
expect(peerConnectionAnalyzer.getConnectionQualityVideo()).toBe(CONNECTION_QUALITY.UNKNOWN)
677+
expect(changeConnectionQualityAudioHandler).toHaveBeenCalledTimes(1)
678+
expect(changeConnectionQualityAudioHandler).toHaveBeenCalledWith(peerConnectionAnalyzer, CONNECTION_QUALITY.VERY_BAD)
679+
expect(changeConnectionQualityVideoHandler).toHaveBeenCalledTimes(0)
680+
} else {
681+
expect(peerConnectionAnalyzer.getConnectionQualityAudio()).toBe(CONNECTION_QUALITY.UNKNOWN)
682+
expect(peerConnectionAnalyzer.getConnectionQualityVideo()).toBe(CONNECTION_QUALITY.VERY_BAD)
683+
expect(changeConnectionQualityAudioHandler).toHaveBeenCalledTimes(0)
684+
expect(changeConnectionQualityVideoHandler).toHaveBeenCalledTimes(1)
685+
expect(changeConnectionQualityVideoHandler).toHaveBeenCalledWith(peerConnectionAnalyzer, CONNECTION_QUALITY.VERY_BAD)
686+
}
687+
})
688+
689+
test.each([
690+
['good quality even with low packets if no packet loss', 'audio'],
691+
['good quality even with low packets if no packet loss', 'video'],
580692
])('%s, %s', async (name, kind) => {
581693
peerConnection.getStats
582694
.mockResolvedValueOnce(newRTCStatsReport([
@@ -616,23 +728,23 @@ describe('PeerConnectionAnalyzer', () => {
616728
expect(peerConnection.getStats).toHaveBeenCalledTimes(6)
617729

618730
if (kind === 'audio') {
619-
expect(peerConnectionAnalyzer.getConnectionQualityAudio()).toBe(CONNECTION_QUALITY.VERY_BAD)
731+
expect(peerConnectionAnalyzer.getConnectionQualityAudio()).toBe(CONNECTION_QUALITY.GOOD)
620732
expect(peerConnectionAnalyzer.getConnectionQualityVideo()).toBe(CONNECTION_QUALITY.UNKNOWN)
621733
expect(changeConnectionQualityAudioHandler).toHaveBeenCalledTimes(1)
622-
expect(changeConnectionQualityAudioHandler).toHaveBeenCalledWith(peerConnectionAnalyzer, CONNECTION_QUALITY.VERY_BAD)
734+
expect(changeConnectionQualityAudioHandler).toHaveBeenCalledWith(peerConnectionAnalyzer, CONNECTION_QUALITY.GOOD)
623735
expect(changeConnectionQualityVideoHandler).toHaveBeenCalledTimes(0)
624736
} else {
625737
expect(peerConnectionAnalyzer.getConnectionQualityAudio()).toBe(CONNECTION_QUALITY.UNKNOWN)
626-
expect(peerConnectionAnalyzer.getConnectionQualityVideo()).toBe(CONNECTION_QUALITY.VERY_BAD)
738+
expect(peerConnectionAnalyzer.getConnectionQualityVideo()).toBe(CONNECTION_QUALITY.GOOD)
627739
expect(changeConnectionQualityAudioHandler).toHaveBeenCalledTimes(0)
628740
expect(changeConnectionQualityVideoHandler).toHaveBeenCalledTimes(1)
629-
expect(changeConnectionQualityVideoHandler).toHaveBeenCalledWith(peerConnectionAnalyzer, CONNECTION_QUALITY.VERY_BAD)
741+
expect(changeConnectionQualityVideoHandler).toHaveBeenCalledWith(peerConnectionAnalyzer, CONNECTION_QUALITY.GOOD)
630742
}
631743
})
632744

633745
test.each([
634-
['very bad quality due to low packets, missing remote packet count', 'audio'],
635-
['very bad quality due to low packets, missing remote packet count', 'video'],
746+
['good quality even with low packets if no packet loss, missing remote packet count', 'audio'],
747+
['good quality even with low packets if no packet loss, missing remote packet count', 'video'],
636748
])('%s, %s', async (name, kind) => {
637749
peerConnection.getStats
638750
.mockResolvedValueOnce(newRTCStatsReport([
@@ -672,17 +784,17 @@ describe('PeerConnectionAnalyzer', () => {
672784
expect(peerConnection.getStats).toHaveBeenCalledTimes(6)
673785

674786
if (kind === 'audio') {
675-
expect(peerConnectionAnalyzer.getConnectionQualityAudio()).toBe(CONNECTION_QUALITY.VERY_BAD)
787+
expect(peerConnectionAnalyzer.getConnectionQualityAudio()).toBe(CONNECTION_QUALITY.GOOD)
676788
expect(peerConnectionAnalyzer.getConnectionQualityVideo()).toBe(CONNECTION_QUALITY.UNKNOWN)
677789
expect(changeConnectionQualityAudioHandler).toHaveBeenCalledTimes(1)
678-
expect(changeConnectionQualityAudioHandler).toHaveBeenCalledWith(peerConnectionAnalyzer, CONNECTION_QUALITY.VERY_BAD)
790+
expect(changeConnectionQualityAudioHandler).toHaveBeenCalledWith(peerConnectionAnalyzer, CONNECTION_QUALITY.GOOD)
679791
expect(changeConnectionQualityVideoHandler).toHaveBeenCalledTimes(0)
680792
} else {
681793
expect(peerConnectionAnalyzer.getConnectionQualityAudio()).toBe(CONNECTION_QUALITY.UNKNOWN)
682-
expect(peerConnectionAnalyzer.getConnectionQualityVideo()).toBe(CONNECTION_QUALITY.VERY_BAD)
794+
expect(peerConnectionAnalyzer.getConnectionQualityVideo()).toBe(CONNECTION_QUALITY.GOOD)
683795
expect(changeConnectionQualityAudioHandler).toHaveBeenCalledTimes(0)
684796
expect(changeConnectionQualityVideoHandler).toHaveBeenCalledTimes(1)
685-
expect(changeConnectionQualityVideoHandler).toHaveBeenCalledWith(peerConnectionAnalyzer, CONNECTION_QUALITY.VERY_BAD)
797+
expect(changeConnectionQualityVideoHandler).toHaveBeenCalledWith(peerConnectionAnalyzer, CONNECTION_QUALITY.GOOD)
686798
}
687799
})
688800

0 commit comments

Comments
 (0)