media: support additional scenario in packet loss analyser (#468)

whereby · Nov 13, 2024 · b5ec7b9 · b5ec7b9
1 parent 87d6e88
commit b5ec7b9
Show file tree

Hide file tree

Showing 4 changed files with 44 additions and 16 deletions.
diff --git a/.changeset/wet-jobs-grab.md b/.changeset/wet-jobs-grab.md
@@ -0,0 +1,5 @@
+---
+"@whereby.com/media": patch
+---
+
+Extend PacketLossAnalyser to support additional scenario
diff --git a/packages/media/src/webrtc/stats/IssueMonitor/__tests__/packetLossAnalyser.spec.ts b/packages/media/src/webrtc/stats/IssueMonitor/__tests__/packetLossAnalyser.spec.ts
@@ -31,7 +31,7 @@ describe("PacketLossAnalyser", () => {
     });
 
     it("reports no periodic packet loss on missing ssrc", () => {
-        expect(pla.hasPeriodicPacketLoss(ssrcId)).toBe(false);
+        expect(pla.hasPeriodicPacketLoss(ssrcId, Date.now())).toBe(false);
     });
 
     it("reports periodic packet loss for 3 periods with equal interval", () => {
@@ -46,7 +46,7 @@ describe("PacketLossAnalyser", () => {
         timestamp += 60000; // Next packet loss period 60s after previous.
         pla.addPacketLossMeasurement(ssrcId, 0.05, timestamp);
         pla.addPacketLossMeasurement(ssrcId, 0, timestamp + 2000);
-        expect(pla.hasPeriodicPacketLoss(ssrcId)).toBe(true);
+        expect(pla.hasPeriodicPacketLoss(ssrcId, timestamp + 2000)).toBe(true);
     });
 
     it("reports periodic packet loss for 3 periods with some variation in interval", () => {
@@ -65,35 +65,44 @@ describe("PacketLossAnalyser", () => {
         pla.addPacketLossMeasurement(ssrcId, 0.05, timestamp + 2000);
         pla.addPacketLossMeasurement(ssrcId, 0, timestamp + 4000);
 
-        expect(pla.hasPeriodicPacketLoss(ssrcId)).toBe(true);
+        expect(pla.hasPeriodicPacketLoss(ssrcId, timestamp + 4000)).toBe(true);
 
         timestamp += 28000; // Next packet loss period 28s after previous.
         pla.addPacketLossMeasurement(ssrcId, 0.05, timestamp);
         pla.addPacketLossMeasurement(ssrcId, 0.05, timestamp + 2000);
         pla.addPacketLossMeasurement(ssrcId, 0, timestamp + 4000);
 
-        expect(pla.hasPeriodicPacketLoss(ssrcId)).toBe(true);
+        expect(pla.hasPeriodicPacketLoss(ssrcId, timestamp + 4000)).toBe(true);
     });
 
     it("reports no periodic packet loss on interval change from 30s to 15s", () => {
         let lastPeriodTimestamp = addPeriodicPacketLoss(pla, ssrcId);
-        expect(pla.hasPeriodicPacketLoss(ssrcId)).toBe(true);
+        expect(pla.hasPeriodicPacketLoss(ssrcId, lastPeriodTimestamp)).toBe(true);
 
         lastPeriodTimestamp += 15000; // Begin new packet loss period 15s after previous period.
         pla.addPacketLossMeasurement(ssrcId, 0.05, lastPeriodTimestamp);
         pla.addPacketLossMeasurement(ssrcId, 0, lastPeriodTimestamp + 2000);
 
-        expect(pla.hasPeriodicPacketLoss(ssrcId)).toBe(false);
+        expect(pla.hasPeriodicPacketLoss(ssrcId, lastPeriodTimestamp + 2000)).toBe(false);
     });
 
-    it("invalidates periodic packet loss measurements after a while", () => {
+    it("reports no periodic packet loss after a while when there are no new measurements", () => {
         jest.useFakeTimers();
-        addPeriodicPacketLoss(pla, ssrcId);
-        expect(pla.hasPeriodicPacketLoss(ssrcId)).toBe(true);
+        const lastPeriodTimestamp = addPeriodicPacketLoss(pla, ssrcId);
+        expect(pla.hasPeriodicPacketLoss(ssrcId, lastPeriodTimestamp)).toBe(true);
 
         jest.runAllTimers();
 
-        expect(pla.hasPeriodicPacketLoss(ssrcId)).toBe(false);
+        expect(pla.hasPeriodicPacketLoss(ssrcId, lastPeriodTimestamp)).toBe(false);
         jest.useRealTimers();
     });
+
+    it("reports no periodic packet loss when previous interval is exceeded", () => {
+        let lastPeriodTimestamp = addPeriodicPacketLoss(pla, ssrcId); // 30s interval
+        expect(pla.hasPeriodicPacketLoss(ssrcId, lastPeriodTimestamp)).toBe(true);
+
+        lastPeriodTimestamp += 40000; // Past the 30s interval + our accepted diff.
+
+        expect(pla.hasPeriodicPacketLoss(ssrcId, lastPeriodTimestamp)).toBe(false);
+    });
 });
diff --git a/packages/media/src/webrtc/stats/IssueMonitor/issueDetectors.ts b/packages/media/src/webrtc/stats/IssueMonitor/issueDetectors.ts
@@ -31,8 +31,7 @@ export const periodicPacketLossDetector: IssueDetector = {
     },
     check: ({ ssrc0 }) => {
         packetLossAnalyser.addPacketLossMeasurement(ssrc0.ssrc, ssrc0.fractionLost || 0, Date.now());
-        if (packetLossAnalyser.hasPeriodicPacketLoss(ssrc0.ssrc)) return true;
-        return false;
+        return packetLossAnalyser.hasPeriodicPacketLoss(ssrc0.ssrc, Date.now());
     },
 };
 

diff --git a/packages/media/src/webrtc/stats/IssueMonitor/packetLossAnalyser.ts b/packages/media/src/webrtc/stats/IssueMonitor/packetLossAnalyser.ts
@@ -17,7 +17,7 @@ type PacketLossHistory = {
 const logger = new Logger();
 const debugLogger = {
     // eslint-disable-next-line no-console
-    print: (...args: any[]) => console.log(args[0], ...args.slice(1)),
+    print: (...args: any[]) => console.debug(args[0], ...args.slice(1)),
 };
 logger.withDebugLogger(debugLogger);
 
@@ -29,7 +29,6 @@ export class PacketLossAnalyser {
     private staleMeasurementTimeouts = new Map<string, NodeJS.Timeout>();
 
     addPacketLossMeasurement(id: string, packetLoss: number, timestamp: number) {
-        logger.debug("addPacketLossMeasurement() [ssrcId: %s, loss: %s, timestamp: %s]", id, packetLoss, timestamp);
         this.handleStaleMeasurements(id);
         const hasPacketLoss = packetLoss > this.PACKET_LOSS_PERIOD_THRESHOLD;
 
@@ -63,8 +62,24 @@ export class PacketLossAnalyser {
         }
     }
 
-    hasPeriodicPacketLoss(id: string) {
-        return this.ssrcsHistory.get(id)?.hasPeriodicPacketLoss || false;
+    hasPeriodicPacketLoss(id: string, timestamp: number) {
+        const history = this.ssrcsHistory.get(id);
+
+        // Reset state for ssrc if interval is exceeded.
+        if (history && this.intervalExceeded(history, timestamp)) {
+            this.ssrcsHistory.delete(history.id);
+            return false;
+        }
+        return history?.hasPeriodicPacketLoss || false;
+    }
+
+    private intervalExceeded(history: PacketLossHistory, timestamp: number) {
+        if (history.prevPeriod && history.prevIntervalInMs) {
+            const intervalLimitTimestamp =
+                this.calculatePeriodCenterTimestamp(history.prevPeriod) + history.prevIntervalInMs;
+            return timestamp > intervalLimitTimestamp;
+        }
+        return false;
     }
 
     private handleStaleMeasurements(id: string) {