From 4e1b4fae19b5dacbcb0ba366bd87ff063f51c547 Mon Sep 17 00:00:00 2001 From: Hugh Nimmo-Smith Date: Sat, 23 Nov 2024 08:59:15 +0000 Subject: [PATCH] Refactor the speaker detection logic into observeSpeaker and add tests (#2814) * Refactor the speaker detection logic into observeSpeaker and add tests @robintown the tests pass, but some of the values were off by 1ms from what I was expecting. Please can you sanity check them? * Extra test cases and clean up * Make distinctUntilChanged part of the observable itself * More suggestions from code review --- src/state/CallViewModel.ts | 19 +---- src/state/observeSpeaker.test.ts | 119 +++++++++++++++++++++++++++++++ src/state/observeSpeaker.ts | 36 ++++++++++ 3 files changed, 157 insertions(+), 17 deletions(-) create mode 100644 src/state/observeSpeaker.test.ts create mode 100644 src/state/observeSpeaker.ts diff --git a/src/state/CallViewModel.ts b/src/state/CallViewModel.ts index 8999dc89..83ccd48c 100644 --- a/src/state/CallViewModel.ts +++ b/src/state/CallViewModel.ts @@ -27,7 +27,6 @@ import { EMPTY, Observable, Subject, - audit, combineLatest, concat, distinctUntilChanged, @@ -76,6 +75,7 @@ import { spotlightExpandedLayout } from "./SpotlightExpandedLayout"; import { oneOnOneLayout } from "./OneOnOneLayout"; import { pipLayout } from "./PipLayout"; import { EncryptionSystem } from "../e2ee/sharedKeyManagement"; +import { observeSpeaker } from "./observeSpeaker"; // How long we wait after a focus switch before showing the real participant // list again @@ -248,22 +248,7 @@ class UserMedia { livekitRoom, ); - this.speaker = this.vm.speaking.pipe( - // Require 1 s of continuous speaking to become a speaker, and 60 s of - // continuous silence to stop being considered a speaker - audit((s) => - merge( - timer(s ? 1000 : 60000), - // If the speaking flag resets to its original value during this time, - // end the silencing window to stick with that original value - this.vm.speaking.pipe(filter((s1) => s1 !== s)), - ), - ), - startWith(false), - // Make this Observable hot so that the timers don't reset when you - // resubscribe - this.scope.state(), - ); + this.speaker = observeSpeaker(this.vm.speaking).pipe(this.scope.state()); this.presenter = observeParticipantEvents( participant, diff --git a/src/state/observeSpeaker.test.ts b/src/state/observeSpeaker.test.ts new file mode 100644 index 00000000..daa5f033 --- /dev/null +++ b/src/state/observeSpeaker.test.ts @@ -0,0 +1,119 @@ +/* +Copyright 2024 New Vector Ltd. + +SPDX-License-Identifier: AGPL-3.0-only +Please see LICENSE in the repository root for full details. +*/ + +import { describe, test } from "vitest"; + +import { withTestScheduler } from "../utils/test"; +import { observeSpeaker } from "./observeSpeaker"; + +const yesNo = { + y: true, + n: false, +}; + +describe("observeSpeaker", () => { + describe("does not activate", () => { + const expectedOutputMarbles = "n"; + test("starts correctly", () => { + // should default to false when no input is given + const speakingInputMarbles = ""; + withTestScheduler(({ hot, expectObservable }) => { + expectObservable(observeSpeaker(hot(speakingInputMarbles, yesNo))).toBe( + expectedOutputMarbles, + yesNo, + ); + }); + }); + + test("after no speaking", () => { + const speakingInputMarbles = "n"; + withTestScheduler(({ hot, expectObservable }) => { + expectObservable(observeSpeaker(hot(speakingInputMarbles, yesNo))).toBe( + expectedOutputMarbles, + yesNo, + ); + }); + }); + + test("with speaking for 1ms", () => { + const speakingInputMarbles = "y n"; + withTestScheduler(({ hot, expectObservable }) => { + expectObservable(observeSpeaker(hot(speakingInputMarbles, yesNo))).toBe( + expectedOutputMarbles, + yesNo, + ); + }); + }); + + test("with speaking for 999ms", () => { + const speakingInputMarbles = "y 999ms n"; + withTestScheduler(({ hot, expectObservable }) => { + expectObservable(observeSpeaker(hot(speakingInputMarbles, yesNo))).toBe( + expectedOutputMarbles, + yesNo, + ); + }); + }); + + test("with speaking intermittently", () => { + const speakingInputMarbles = + "y 199ms n 199ms y 199ms n 199ms y 199ms n 199ms y 199ms n 199ms y 199ms n 199ms y 199ms n 199ms y 199ms n 199ms y 199ms n"; + withTestScheduler(({ hot, expectObservable }) => { + expectObservable(observeSpeaker(hot(speakingInputMarbles, yesNo))).toBe( + expectedOutputMarbles, + yesNo, + ); + }); + }); + + test("with consecutive speaking then stops speaking", () => { + const speakingInputMarbles = "y y y y y y y y y y n"; + withTestScheduler(({ hot, expectObservable }) => { + expectObservable(observeSpeaker(hot(speakingInputMarbles, yesNo))).toBe( + expectedOutputMarbles, + yesNo, + ); + }); + }); + }); + + describe("activates", () => { + test("after 1s", () => { + // this will active after 1s as no `n` follows it: + const speakingInputMarbles = " y"; + const expectedOutputMarbles = "n 999ms y"; + withTestScheduler(({ hot, expectObservable }) => { + expectObservable(observeSpeaker(hot(speakingInputMarbles, yesNo))).toBe( + expectedOutputMarbles, + yesNo, + ); + }); + }); + + test("speaking for 1001ms activates for 60s", () => { + const speakingInputMarbles = " y 1s n "; + const expectedOutputMarbles = "n 999ms y 60s n"; + withTestScheduler(({ hot, expectObservable }) => { + expectObservable(observeSpeaker(hot(speakingInputMarbles, yesNo))).toBe( + expectedOutputMarbles, + yesNo, + ); + }); + }); + + test("speaking for 5s activates for 64s", () => { + const speakingInputMarbles = " y 5s n "; + const expectedOutputMarbles = "n 999ms y 64s n"; + withTestScheduler(({ hot, expectObservable }) => { + expectObservable(observeSpeaker(hot(speakingInputMarbles, yesNo))).toBe( + expectedOutputMarbles, + yesNo, + ); + }); + }); + }); +}); diff --git a/src/state/observeSpeaker.ts b/src/state/observeSpeaker.ts new file mode 100644 index 00000000..d32fbdaa --- /dev/null +++ b/src/state/observeSpeaker.ts @@ -0,0 +1,36 @@ +/* +Copyright 2024 New Vector Ltd. + +SPDX-License-Identifier: AGPL-3.0-only +Please see LICENSE in the repository root for full details. +*/ +import { + Observable, + audit, + merge, + timer, + filter, + startWith, + distinctUntilChanged, +} from "rxjs"; + +/** + * Require 1 second of continuous speaking to become a speaker, and 60 second of + * continuous silence to stop being considered a speaker + */ +export function observeSpeaker( + isSpeakingObservable: Observable, +): Observable { + const distinct = isSpeakingObservable.pipe(distinctUntilChanged()); + + return distinct.pipe( + // Either change to the new value after the timer or re-emit the same value if it toggles back + // (audit will return the latest (toggled back) value) before the timeout. + audit((s) => + merge(timer(s ? 1000 : 60000), distinct.pipe(filter((s1) => s1 !== s))), + ), + // Filter the re-emissions (marked as: | ) that happen if we toggle quickly (<1s) from false->true->false|->.. + startWith(false), + distinctUntilChanged(), + ); +}