Skip to content

Commit

Permalink
fix: prevent double sound detectors set up (#1371)
Browse files Browse the repository at this point in the history
Calling `startSpeakingWhileMutedDetection` multiple types without
awaiting leads to multiple sound detectors being set up. Calling
`stopSpeakingWhileMutedDetection` afterwords cleans up only one of them.

Since `startSpeakingWhileMutedDetection` is called in a callback to an
Rx observable, it's very possible that multiple sound detectors will be
set up when the state updates fire rapidly.

This fix prevents that.
  • Loading branch information
myandrienko authored May 29, 2024
1 parent ecf19af commit 51c9198
Show file tree
Hide file tree
Showing 2 changed files with 49 additions and 35 deletions.
63 changes: 35 additions & 28 deletions packages/client/src/devices/MicrophoneManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -212,38 +212,45 @@ export class MicrophoneManager extends InputMediaDeviceManager<MicrophoneManager
}

private async startSpeakingWhileMutedDetection(deviceId?: string) {
await this.stopSpeakingWhileMutedDetection();
if (isReactNative()) {
this.rnSpeechDetector = new RNSpeechDetector();
await this.rnSpeechDetector.start();
const unsubscribe = this.rnSpeechDetector?.onSpeakingDetectedStateChange(
(event) => {
this.state.setSpeakingWhileMuted(event.isSoundDetected);
},
);
this.soundDetectorCleanup = () => {
unsubscribe();
this.rnSpeechDetector?.stop();
this.rnSpeechDetector = undefined;
};
} else {
// Need to start a new stream that's not connected to publisher
const stream = await this.getStream({
deviceId,
});
this.soundDetectorCleanup = createSoundDetector(stream, (event) => {
this.state.setSpeakingWhileMuted(event.isSoundDetected);
});
}
const startPromise: Promise<(() => void) | (() => Promise<void>)> =
(async () => {
await this.stopSpeakingWhileMutedDetection();
if (isReactNative()) {
this.rnSpeechDetector = new RNSpeechDetector();
await this.rnSpeechDetector.start();
const unsubscribe =
this.rnSpeechDetector?.onSpeakingDetectedStateChange((event) => {
this.state.setSpeakingWhileMuted(event.isSoundDetected);
});
return () => {
unsubscribe();
this.rnSpeechDetector?.stop();
this.rnSpeechDetector = undefined;
};
} else {
// Need to start a new stream that's not connected to publisher
const stream = await this.getStream({
deviceId,
});
return createSoundDetector(stream, (event) => {
this.state.setSpeakingWhileMuted(event.isSoundDetected);
});
}
})();

this.soundDetectorCleanup = async () => {
const cleanup = await startPromise;
await cleanup();
};

await startPromise;
}

private async stopSpeakingWhileMutedDetection() {
if (!this.soundDetectorCleanup) return;
const soundDetectorCleanup = this.soundDetectorCleanup;
this.soundDetectorCleanup = undefined;
this.state.setSpeakingWhileMuted(false);
try {
await this.soundDetectorCleanup();
} finally {
this.soundDetectorCleanup = undefined;
}
await soundDetectorCleanup();
}
}
21 changes: 14 additions & 7 deletions packages/client/src/devices/__tests__/MicrophoneManager.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ vi.mock('../devices.ts', () => {
vi.mock('../../helpers/sound-detector.ts', () => {
console.log('MOCKING sound detector');
return {
createSoundDetector: vi.fn(),
createSoundDetector: vi.fn(() => () => {}),
};
});

Expand Down Expand Up @@ -170,22 +170,28 @@ describe('MicrophoneManager', () => {
it('should update speaking while muted state', async () => {
const mock = createSoundDetector as Mock;
let handler: SoundStateChangeHandler;
let prevMockImplementation = mock.getMockImplementation();
mock.mockImplementation((_: MediaStream, h: SoundStateChangeHandler) => {
handler = h;
});
await manager['startSpeakingWhileMutedDetection']();
try {
await manager['startSpeakingWhileMutedDetection']();

expect(manager.state.speakingWhileMuted).toBe(false);
expect(manager.state.speakingWhileMuted).toBe(false);

handler!({ isSoundDetected: true, audioLevel: 2 });
handler!({ isSoundDetected: true, audioLevel: 2 });

expect(manager.state.speakingWhileMuted).toBe(true);
expect(manager.state.speakingWhileMuted).toBe(true);

handler!({ isSoundDetected: false, audioLevel: 0 });
handler!({ isSoundDetected: false, audioLevel: 0 });

expect(manager.state.speakingWhileMuted).toBe(false);
expect(manager.state.speakingWhileMuted).toBe(false);
} finally {
mock.mockImplementation(prevMockImplementation!);
}
});

// --- this ---
it('should stop speaking while muted notifications if user loses permission to send audio', async () => {
await manager.enable();
await manager.disable();
Expand All @@ -197,6 +203,7 @@ describe('MicrophoneManager', () => {
expect(manager['stopSpeakingWhileMutedDetection']).toHaveBeenCalled();
});

// --- this ---
it('should start speaking while muted notifications if user gains permission to send audio', async () => {
await manager.enable();
await manager.disable();
Expand Down

0 comments on commit 51c9198

Please sign in to comment.