From d4f1c573adf109460fcbd86c5043d570bb690714 Mon Sep 17 00:00:00 2001 From: Michael Weimann Date: Sat, 22 Oct 2022 13:07:39 +0200 Subject: [PATCH] Fix voice broadcast recording limit (#9478) --- src/audio/VoiceRecording.ts | 11 ++ .../audio/VoiceBroadcastRecorder.ts | 4 +- test/audio/VoiceRecording-test.ts | 105 ++++++++++++++++++ .../audio/VoiceBroadcastRecorder-test.ts | 19 ++-- 4 files changed, 128 insertions(+), 11 deletions(-) create mode 100644 test/audio/VoiceRecording-test.ts diff --git a/src/audio/VoiceRecording.ts b/src/audio/VoiceRecording.ts index 0e18756fe5..99f878868d 100644 --- a/src/audio/VoiceRecording.ts +++ b/src/audio/VoiceRecording.ts @@ -60,6 +60,7 @@ export class VoiceRecording extends EventEmitter implements IDestroyable { private recorderProcessor: ScriptProcessorNode; private recording = false; private observable: SimpleObservable; + private targetMaxLength: number | null = TARGET_MAX_LENGTH; public amplitudes: number[] = []; // at each second mark, generated private liveWaveform = new FixedRollingArray(RECORDING_PLAYBACK_SAMPLES, 0); public onDataAvailable: (data: ArrayBuffer) => void; @@ -83,6 +84,10 @@ export class VoiceRecording extends EventEmitter implements IDestroyable { return true; // we don't ever care if the event had listeners, so just return "yes" } + public disableMaxLength(): void { + this.targetMaxLength = null; + } + private async makeRecorder() { try { this.recorderStream = await navigator.mediaDevices.getUserMedia({ @@ -203,6 +208,12 @@ export class VoiceRecording extends EventEmitter implements IDestroyable { // In testing, recorder time and worker time lag by about 400ms, which is roughly the // time needed to encode a sample/frame. // + + if (!this.targetMaxLength) { + // skip time checks if max length has been disabled + return; + } + const secondsLeft = TARGET_MAX_LENGTH - this.recorderSeconds; if (secondsLeft < 0) { // go over to make sure we definitely capture that last frame // noinspection JSIgnoredPromiseFromCall - we aren't concerned with it overlapping diff --git a/src/voice-broadcast/audio/VoiceBroadcastRecorder.ts b/src/voice-broadcast/audio/VoiceBroadcastRecorder.ts index ff1d22a41c..df7ae362d9 100644 --- a/src/voice-broadcast/audio/VoiceBroadcastRecorder.ts +++ b/src/voice-broadcast/audio/VoiceBroadcastRecorder.ts @@ -139,5 +139,7 @@ export class VoiceBroadcastRecorder } export const createVoiceBroadcastRecorder = (): VoiceBroadcastRecorder => { - return new VoiceBroadcastRecorder(new VoiceRecording(), getChunkLength()); + const voiceRecording = new VoiceRecording(); + voiceRecording.disableMaxLength(); + return new VoiceBroadcastRecorder(voiceRecording, getChunkLength()); }; diff --git a/test/audio/VoiceRecording-test.ts b/test/audio/VoiceRecording-test.ts new file mode 100644 index 0000000000..ac4f52eabe --- /dev/null +++ b/test/audio/VoiceRecording-test.ts @@ -0,0 +1,105 @@ +/* +Copyright 2022 The Matrix.org Foundation C.I.C. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +import { VoiceRecording } from "../../src/audio/VoiceRecording"; + +/** + * The tests here are heavily using access to private props. + * While this is not so great, we can at lest test some behaviour easily this way. + */ +describe("VoiceRecording", () => { + let recording: VoiceRecording; + let recorderSecondsSpy: jest.SpyInstance; + + const itShouldNotCallStop = () => { + it("should not call stop", () => { + expect(recording.stop).not.toHaveBeenCalled(); + }); + }; + + const simulateUpdate = (recorderSeconds: number) => { + beforeEach(() => { + recorderSecondsSpy.mockReturnValue(recorderSeconds); + // @ts-ignore + recording.processAudioUpdate(recorderSeconds); + }); + }; + + beforeEach(() => { + recording = new VoiceRecording(); + // @ts-ignore + recording.observable = { + update: jest.fn(), + }; + jest.spyOn(recording, "stop").mockImplementation(); + recorderSecondsSpy = jest.spyOn(recording, "recorderSeconds", "get"); + }); + + afterEach(() => { + jest.resetAllMocks(); + }); + + describe("when recording", () => { + beforeEach(() => { + // @ts-ignore + recording.recording = true; + }); + + describe("and there is an audio update and time left", () => { + simulateUpdate(42); + itShouldNotCallStop(); + }); + + describe("and there is an audio update and time is up", () => { + // one second above the limit + simulateUpdate(901); + + it("should call stop", () => { + expect(recording.stop).toHaveBeenCalled(); + }); + }); + + describe("and the max length limit has been disabled", () => { + beforeEach(() => { + recording.disableMaxLength(); + }); + + describe("and there is an audio update and time left", () => { + simulateUpdate(42); + itShouldNotCallStop(); + }); + + describe("and there is an audio update and time is up", () => { + // one second above the limit + simulateUpdate(901); + itShouldNotCallStop(); + }); + }); + }); + + describe("when not recording", () => { + describe("and there is an audio update and time left", () => { + simulateUpdate(42); + itShouldNotCallStop(); + }); + + describe("and there is an audio update and time is up", () => { + // one second above the limit + simulateUpdate(901); + itShouldNotCallStop(); + }); + }); +}); diff --git a/test/voice-broadcast/audio/VoiceBroadcastRecorder-test.ts b/test/voice-broadcast/audio/VoiceBroadcastRecorder-test.ts index e60d7e2d96..df7da24ce5 100644 --- a/test/voice-broadcast/audio/VoiceBroadcastRecorder-test.ts +++ b/test/voice-broadcast/audio/VoiceBroadcastRecorder-test.ts @@ -26,6 +26,8 @@ import { VoiceBroadcastRecorderEvent, } from "../../../src/voice-broadcast"; +jest.mock("../../../src/audio/VoiceRecording"); + describe("VoiceBroadcastRecorder", () => { describe("createVoiceBroadcastRecorder", () => { beforeEach(() => { @@ -44,6 +46,7 @@ describe("VoiceBroadcastRecorder", () => { it("should return a VoiceBroadcastRecorder instance with targetChunkLength from config", () => { const voiceBroadcastRecorder = createVoiceBroadcastRecorder(); + expect(mocked(VoiceRecording).mock.instances[0].disableMaxLength).toHaveBeenCalled(); expect(voiceBroadcastRecorder).toBeInstanceOf(VoiceBroadcastRecorder); expect(voiceBroadcastRecorder.targetChunkLength).toBe(1337); }); @@ -72,16 +75,12 @@ describe("VoiceBroadcastRecorder", () => { }; beforeEach(() => { - voiceRecording = { - contentType, - start: jest.fn().mockResolvedValue(undefined), - stop: jest.fn().mockResolvedValue(undefined), - on: jest.fn(), - off: jest.fn(), - emit: jest.fn(), - destroy: jest.fn(), - recorderSeconds: 23, - } as unknown as VoiceRecording; + voiceRecording = new VoiceRecording(); + // @ts-ignore + voiceRecording.recorderSeconds = 23; + // @ts-ignore + voiceRecording.contentType = contentType; + voiceBroadcastRecorder = new VoiceBroadcastRecorder(voiceRecording, chunkLength); jest.spyOn(voiceBroadcastRecorder, "removeAllListeners"); onChunkRecorded = jest.fn();