From f4ce41d600db48014cd634a1ad0bfa84e99603e7 Mon Sep 17 00:00:00 2001 From: Bryce Tham Date: Tue, 12 Mar 2024 11:03:49 -0700 Subject: [PATCH] feat: separate API for enabled and muted states (#72) * feat: add mute reason to MuteStateChange event * feat: separate API for enabled and muted states * fix: emit MediaStateChange on RemoteStream.replaceTrack --------- Co-authored-by: Bryce Tham --- src/media/local-stream.spec.ts | 42 +++++------ src/media/local-stream.ts | 100 +++++++++++++++++++++------ src/media/local-video-stream.spec.ts | 5 +- src/media/remote-stream.spec.ts | 75 +++++++++++++++++++- src/media/remote-stream.ts | 91 +++++++++++++++++++++++- src/media/stream.ts | 46 ++++-------- 6 files changed, 277 insertions(+), 82 deletions(-) diff --git a/src/media/local-stream.spec.ts b/src/media/local-stream.spec.ts index 121cd24..5d52e03 100644 --- a/src/media/local-stream.spec.ts +++ b/src/media/local-stream.spec.ts @@ -1,7 +1,6 @@ import { WebrtcCoreError } from '../errors'; import { createMockedStream } from '../util/test-utils'; import { LocalStream, LocalStreamEventNames, TrackEffect } from './local-stream'; -import { StreamEventNames } from './stream'; /** * A dummy LocalStream implementation, so we can instantiate it for testing. @@ -16,27 +15,42 @@ describe('LocalStream', () => { localStream = new TestLocalStream(mockStream); }); - describe('setMuted', () => { + describe('constructor', () => { + it('should add the correct event handlers on the track', () => { + expect.assertions(4); + + const addEventListenerSpy = jest.spyOn(mockStream.getTracks()[0], 'addEventListener'); + + expect(addEventListenerSpy).toHaveBeenCalledTimes(3); + expect(addEventListenerSpy).toHaveBeenCalledWith('ended', expect.anything()); + expect(addEventListenerSpy).toHaveBeenCalledWith('mute', expect.anything()); + expect(addEventListenerSpy).toHaveBeenCalledWith('unmute', expect.anything()); + }); + }); + + describe('setUserMuted', () => { let emitSpy: jest.SpyInstance; beforeEach(() => { localStream = new TestLocalStream(mockStream); - emitSpy = jest.spyOn(localStream[StreamEventNames.MuteStateChange], 'emit'); + emitSpy = jest.spyOn(localStream[LocalStreamEventNames.UserMuteStateChange], 'emit'); }); it('should change the input track enabled state and fire an event', () => { - expect.assertions(6); + expect.assertions(8); // Simulate the default state of the track's enabled state. mockStream.getTracks()[0].enabled = true; - localStream.setMuted(true); + localStream.setUserMuted(true); expect(mockStream.getTracks()[0].enabled).toBe(false); + expect(localStream.userMuted).toBe(true); expect(emitSpy).toHaveBeenCalledTimes(1); expect(emitSpy).toHaveBeenLastCalledWith(true); - localStream.setMuted(false); + localStream.setUserMuted(false); expect(mockStream.getTracks()[0].enabled).toBe(true); + expect(localStream.userMuted).toBe(false); expect(emitSpy).toHaveBeenCalledTimes(2); expect(emitSpy).toHaveBeenLastCalledWith(false); }); @@ -47,21 +61,7 @@ describe('LocalStream', () => { // Simulate the default state of the track's enabled state. mockStream.getTracks()[0].enabled = true; - localStream.setMuted(false); - expect(emitSpy).toHaveBeenCalledTimes(0); - }); - - it('should not fire an event if the track has been muted by the browser', () => { - expect.assertions(2); - - // Simulate the default state of the track's enabled state. - mockStream.getTracks()[0].enabled = true; - - // Simulate the track being muted by the browser. - Object.defineProperty(mockStream.getTracks()[0], 'muted', { value: true }); - - localStream.setMuted(true); - expect(mockStream.getTracks()[0].enabled).toBe(false); + localStream.setUserMuted(false); expect(emitSpy).toHaveBeenCalledTimes(0); }); }); diff --git a/src/media/local-stream.ts b/src/media/local-stream.ts index a9b25fd..fb62a77 100644 --- a/src/media/local-stream.ts +++ b/src/media/local-stream.ts @@ -7,12 +7,16 @@ import { Stream, StreamEventNames } from './stream'; export type TrackEffect = BaseEffect; export enum LocalStreamEventNames { + UserMuteStateChange = 'user-mute-state-change', + SystemMuteStateChange = 'system-mute-state-change', ConstraintsChange = 'constraints-change', OutputTrackChange = 'output-track-change', EffectAdded = 'effect-added', } interface LocalStreamEvents { + [LocalStreamEventNames.UserMuteStateChange]: TypedEvent<(muted: boolean) => void>; + [LocalStreamEventNames.SystemMuteStateChange]: TypedEvent<(muted: boolean) => void>; [LocalStreamEventNames.ConstraintsChange]: TypedEvent<() => void>; [LocalStreamEventNames.OutputTrackChange]: TypedEvent<(track: MediaStreamTrack) => void>; [LocalStreamEventNames.EffectAdded]: TypedEvent<(effect: TrackEffect) => void>; @@ -22,6 +26,10 @@ interface LocalStreamEvents { * A stream which originates on the local device. */ abstract class _LocalStream extends Stream { + [LocalStreamEventNames.UserMuteStateChange] = new TypedEvent<(muted: boolean) => void>(); + + [LocalStreamEventNames.SystemMuteStateChange] = new TypedEvent<(muted: boolean) => void>(); + [LocalStreamEventNames.ConstraintsChange] = new TypedEvent<() => void>(); [LocalStreamEventNames.OutputTrackChange] = new TypedEvent<(track: MediaStreamTrack) => void>(); @@ -45,6 +53,51 @@ abstract class _LocalStream extends Stream { constructor(stream: MediaStream) { super(stream); this.inputStream = stream; + this.handleTrackMutedBySystem = this.handleTrackMutedBySystem.bind(this); + this.handleTrackUnmutedBySystem = this.handleTrackUnmutedBySystem.bind(this); + this.addTrackHandlersForLocalStreamEvents(this.inputTrack); + } + + /** + * Handler which is called when a track's mute event fires. + */ + private handleTrackMutedBySystem(): void { + this[LocalStreamEventNames.SystemMuteStateChange].emit(true); + } + + /** + * Handler which is called when a track's unmute event fires. + */ + private handleTrackUnmutedBySystem(): void { + this[LocalStreamEventNames.SystemMuteStateChange].emit(false); + } + + /** + * Helper function to add event handlers to a MediaStreamTrack. See + * {@link Stream.addTrackHandlersForStreamEvents} for why this is useful. + * + * @param track - The MediaStreamTrack. + */ + private addTrackHandlersForLocalStreamEvents(track: MediaStreamTrack): void { + track.addEventListener('mute', this.handleTrackMutedBySystem); + track.addEventListener('unmute', this.handleTrackUnmutedBySystem); + } + + /** + * @inheritdoc + */ + protected addTrackHandlers(track: MediaStreamTrack): void { + super.addTrackHandlers(track); + this.addTrackHandlersForLocalStreamEvents(track); + } + + /** + * @inheritdoc + */ + protected removeTrackHandlers(track: MediaStreamTrack): void { + super.removeTrackHandlers(track); + track.removeEventListener('mute', this.handleTrackMutedBySystem); + track.removeEventListener('unmute', this.handleTrackUnmutedBySystem); } /** @@ -58,45 +111,48 @@ abstract class _LocalStream extends Stream { } /** - * @inheritdoc + * Check whether or not this stream is muted. This considers both whether the stream has been + * muted by the user (see {@link userMuted}) and whether the stream has been muted by the system + * (see {@link systemMuted}). + * + * @returns True if the stream is muted, false otherwise. */ - protected handleTrackMuted() { - if (this.inputTrack.enabled) { - super.handleTrackMuted(); - } + get muted(): boolean { + return this.userMuted || this.systemMuted; } /** - * @inheritdoc + * Check whether or not this stream has been muted by the user. This is equivalent to checking the + * MediaStreamTrack "enabled" state. + * + * @returns True if the stream has been muted by the user, false otherwise. */ - protected handleTrackUnmuted() { - if (this.inputTrack.enabled) { - super.handleTrackUnmuted(); - } + get userMuted(): boolean { + return !this.inputTrack.enabled; } /** - * @inheritdoc + * Check whether or not this stream has been muted by the user. This is equivalent to checking the + * MediaStreamTrack "muted" state. + * + * @returns True if the stream has been muted by the system, false otherwise. */ - get muted(): boolean { - // Calls to `setMuted` will only affect the "enabled" state, but there are specific cases in - // which the browser may mute the track, which will affect the "muted" state but not the - // "enabled" state, e.g. minimizing a shared window or unplugging a shared screen. - return !this.inputTrack.enabled || this.inputTrack.muted; + get systemMuted(): boolean { + return this.inputTrack.muted; } /** - * Set the mute state of this stream. + * Set the user mute state of this stream. + * + * Note: This sets the user-toggled mute state, equivalent to changing the "enabled" state of the + * track. It is separate from the system-toggled mute state. * * @param isMuted - True to mute, false to unmute. */ - setMuted(isMuted: boolean): void { + setUserMuted(isMuted: boolean): void { if (this.inputTrack.enabled === isMuted) { this.inputTrack.enabled = !isMuted; - // setting `enabled` will not automatically emit MuteStateChange, so we emit it here - if (!this.inputTrack.muted) { - this[StreamEventNames.MuteStateChange].emit(isMuted); - } + this[LocalStreamEventNames.UserMuteStateChange].emit(isMuted); } } diff --git a/src/media/local-video-stream.spec.ts b/src/media/local-video-stream.spec.ts index dd7ed1a..657b668 100644 --- a/src/media/local-video-stream.spec.ts +++ b/src/media/local-video-stream.spec.ts @@ -3,7 +3,6 @@ import MediaStreamStub from '../mocks/media-stream-stub'; import { mocked } from '../mocks/mock'; import { LocalStreamEventNames } from './local-stream'; import { LocalVideoStream } from './local-video-stream'; -import { StreamEventNames } from './stream'; jest.mock('../mocks/media-stream-stub'); @@ -21,7 +20,7 @@ describe('localVideoStream', () => { it('should work', () => { expect.hasAssertions(); - videoStream.on(StreamEventNames.MuteStateChange, (muted: boolean) => { + videoStream.on(LocalStreamEventNames.UserMuteStateChange, (muted: boolean) => { // eslint-disable-next-line no-console console.log(`stream is muted? ${muted}`); }); @@ -30,7 +29,7 @@ describe('localVideoStream', () => { console.log('stream constraints changed'); }); - videoStream.setMuted(true); + videoStream.setUserMuted(true); videoStream.applyConstraints({ height: 720, width: 1280, diff --git a/src/media/remote-stream.spec.ts b/src/media/remote-stream.spec.ts index 9893cf8..8400d10 100644 --- a/src/media/remote-stream.spec.ts +++ b/src/media/remote-stream.spec.ts @@ -1,5 +1,5 @@ import { createMockedStream } from '../util/test-utils'; -import { RemoteStream } from './remote-stream'; +import { RemoteMediaState, RemoteStream, RemoteStreamEventNames } from './remote-stream'; describe('RemoteStream', () => { const mockStream = createMockedStream(); @@ -8,6 +8,19 @@ describe('RemoteStream', () => { remoteStream = new RemoteStream(mockStream); }); + describe('constructor', () => { + it('should add the correct event handlers on the track', () => { + expect.assertions(4); + + const addEventListenerSpy = jest.spyOn(mockStream.getTracks()[0], 'addEventListener'); + + expect(addEventListenerSpy).toHaveBeenCalledTimes(3); + expect(addEventListenerSpy).toHaveBeenCalledWith('ended', expect.anything()); + expect(addEventListenerSpy).toHaveBeenCalledWith('mute', expect.anything()); + expect(addEventListenerSpy).toHaveBeenCalledWith('unmute', expect.anything()); + }); + }); + describe('getSettings', () => { it('should get the settings of the output track', () => { expect.assertions(1); @@ -30,6 +43,66 @@ describe('RemoteStream', () => { expect(removeTrackSpy).toHaveBeenCalledWith(mockStream.getTracks()[0]); expect(addTrackSpy).toHaveBeenCalledWith(newTrack); }); + + it('should replace the event handlers on the output track', () => { + expect.assertions(8); + + const removeEventListenerSpy = jest.spyOn(mockStream.getTracks()[0], 'removeEventListener'); + const newTrack = new MediaStreamTrack(); + const addEventListenerSpy = jest.spyOn(newTrack, 'addEventListener'); + + remoteStream.replaceTrack(newTrack); + + expect(removeEventListenerSpy).toHaveBeenCalledTimes(3); + expect(removeEventListenerSpy).toHaveBeenCalledWith('ended', expect.anything()); + expect(removeEventListenerSpy).toHaveBeenCalledWith('mute', expect.anything()); + expect(removeEventListenerSpy).toHaveBeenCalledWith('unmute', expect.anything()); + + expect(addEventListenerSpy).toHaveBeenCalledTimes(3); + expect(addEventListenerSpy).toHaveBeenCalledWith('ended', expect.anything()); + expect(addEventListenerSpy).toHaveBeenCalledWith('mute', expect.anything()); + expect(addEventListenerSpy).toHaveBeenCalledWith('unmute', expect.anything()); + }); + + it('should emit MediaStateChange event if new track is muted but old track is not', () => { + expect.assertions(2); + + // Set old track to be unmuted. + Object.defineProperty(mockStream.getTracks()[0], 'muted', { + value: false, + configurable: true, + }); + + // Set new track to be muted. + const newTrack = new MediaStreamTrack(); + Object.defineProperty(newTrack, 'muted', { value: true }); + + const emitSpy = jest.spyOn(remoteStream[RemoteStreamEventNames.MediaStateChange], 'emit'); + remoteStream.replaceTrack(newTrack); + + expect(emitSpy).toHaveBeenCalledTimes(1); + expect(emitSpy).toHaveBeenCalledWith(RemoteMediaState.Stopped); + }); + + it('should emit MediaStateChange event if old track is muted but new track is not', () => { + expect.assertions(2); + + // Set old track to be muted. + Object.defineProperty(mockStream.getTracks()[0], 'muted', { + value: true, + configurable: true, + }); + + // Set new track to be unmuted. + const newTrack = new MediaStreamTrack(); + Object.defineProperty(newTrack, 'muted', { value: false }); + + const emitSpy = jest.spyOn(remoteStream[RemoteStreamEventNames.MediaStateChange], 'emit'); + remoteStream.replaceTrack(newTrack); + + expect(emitSpy).toHaveBeenCalledTimes(1); + expect(emitSpy).toHaveBeenCalledWith(RemoteMediaState.Started); + }); }); describe('stop', () => { diff --git a/src/media/remote-stream.ts b/src/media/remote-stream.ts index b28ee2d..23f78f2 100644 --- a/src/media/remote-stream.ts +++ b/src/media/remote-stream.ts @@ -1,14 +1,87 @@ +import { AddEvents, TypedEvent, WithEventsDummyType } from '@webex/ts-events'; import { Stream, StreamEventNames } from './stream'; +export enum RemoteMediaState { + Started = 'started', + Stopped = 'stopped', +} + +export enum RemoteStreamEventNames { + MediaStateChange = 'media-state-change', +} + +interface RemoteStreamEvents { + [RemoteStreamEventNames.MediaStateChange]: TypedEvent<(state: RemoteMediaState) => void>; +} + /** * A stream originating from a remote peer. */ -export class RemoteStream extends Stream { +class _RemoteStream extends Stream { + [RemoteStreamEventNames.MediaStateChange] = new TypedEvent<(state: RemoteMediaState) => void>(); + + /** + * Create a RemoteStream from the given values. + * + * @param stream - The initial output MediaStream for this Stream. + */ + constructor(stream: MediaStream) { + super(stream); + this.handleMediaStarted = this.handleMediaStarted.bind(this); + this.handleMediaStopped = this.handleMediaStopped.bind(this); + this.outputTrack.addEventListener('mute', this.handleMediaStopped); + this.outputTrack.addEventListener('unmute', this.handleMediaStarted); + } + + /** + * @inheritdoc + */ + protected handleMediaStarted(): void { + this[RemoteStreamEventNames.MediaStateChange].emit(RemoteMediaState.Started); + } + + /** + * @inheritdoc + */ + protected handleMediaStopped(): void { + this[RemoteStreamEventNames.MediaStateChange].emit(RemoteMediaState.Stopped); + } + + /** + * Helper function to add event handlers to a MediaStreamTrack. See + * {@link Stream.addTrackHandlersForStreamEvents} for why this is useful. + * + * @param track - The MediaStreamTrack. + */ + private addTrackHandlersForRemoteStreamEvents(track: MediaStreamTrack): void { + track.addEventListener('mute', this.handleMediaStopped); + track.addEventListener('unmute', this.handleMediaStarted); + } + + /** + * @inheritdoc + */ + protected addTrackHandlers(track: MediaStreamTrack): void { + super.addTrackHandlers(track); + this.addTrackHandlersForRemoteStreamEvents(track); + } + /** * @inheritdoc */ - get muted(): boolean { - return !this.outputTrack.enabled; + protected removeTrackHandlers(track: MediaStreamTrack): void { + super.removeTrackHandlers(track); + track.removeEventListener('mute', this.handleMediaStopped); + track.removeEventListener('unmute', this.handleMediaStarted); + } + + /** + * Get whether the media on this stream has started or stopped. + * + * @returns The state of the media. + */ + get mediaState(): RemoteMediaState { + return this.outputTrack.muted ? RemoteMediaState.Stopped : RemoteMediaState.Started; } /** @@ -31,6 +104,14 @@ export class RemoteStream extends Stream { this.outputStream.addTrack(newTrack); this.addTrackHandlers(newTrack); + if (oldTrack.muted !== newTrack.muted) { + if (newTrack.muted) { + this.handleMediaStopped(); + } else { + this.handleMediaStarted(); + } + } + // TODO: Chrome/React may not automatically refresh the media element with the new track when // the output track has changed, so we may need to emit an event here if this is the case. // this[StreamEventNames.OutputTrackChange].emit(newTrack); @@ -45,3 +126,7 @@ export class RemoteStream extends Stream { this[StreamEventNames.Ended].emit(); } } + +export const RemoteStream = AddEvents(_RemoteStream); + +export type RemoteStream = _RemoteStream & WithEventsDummyType; diff --git a/src/media/stream.ts b/src/media/stream.ts index 7040d0a..187c8e7 100644 --- a/src/media/stream.ts +++ b/src/media/stream.ts @@ -1,12 +1,10 @@ import { AddEvents, TypedEvent, WithEventsDummyType } from '@webex/ts-events'; export enum StreamEventNames { - MuteStateChange = 'mute-state-change', Ended = 'stream-ended', } interface StreamEvents { - [StreamEventNames.MuteStateChange]: TypedEvent<(muted: boolean) => void>; [StreamEventNames.Ended]: TypedEvent<() => void>; } @@ -19,8 +17,6 @@ abstract class _Stream { // TODO: these should be protected, but we need the helper type in ts-events // to hide the 'emit' method from TypedEvent. - [StreamEventNames.MuteStateChange] = new TypedEvent<(muted: boolean) => void>(); - [StreamEventNames.Ended] = new TypedEvent<() => void>(); /** @@ -30,31 +26,28 @@ abstract class _Stream { */ constructor(stream: MediaStream) { this.outputStream = stream; - this.handleTrackMuted = this.handleTrackMuted.bind(this); - this.handleTrackUnmuted = this.handleTrackUnmuted.bind(this); this.handleTrackEnded = this.handleTrackEnded.bind(this); - this.addTrackHandlers(this.outputTrack); - } - - /** - * Handler which is called when a track's mute event fires. - */ - protected handleTrackMuted() { - this[StreamEventNames.MuteStateChange].emit(true); + this.addTrackHandlersForStreamEvents(this.outputTrack); } /** - * Handler which is called when a track's unmute event fires. + * Handler which is called when a track's ended event fires. */ - protected handleTrackUnmuted() { - this[StreamEventNames.MuteStateChange].emit(false); + private handleTrackEnded() { + this[StreamEventNames.Ended].emit(); } /** - * Handler which is called when a track's ended event fires. + * Helper function to add event handlers to a MediaStreamTrack. Unlike the virtual + * {@link addTrackHandlers} function, which can be overridden, this function is internal to this + * class and will only add the event handlers relevant to this class. It prevents, for example, + * accidentally adding the same event handlers multiple times, which could happen if the virtual + * `addTrackHandlers` method was called from a subclass's constructor. + * + * @param track - The MediaStreamTrack. */ - private handleTrackEnded() { - this[StreamEventNames.Ended].emit(); + private addTrackHandlersForStreamEvents(track: MediaStreamTrack) { + track.addEventListener('ended', this.handleTrackEnded); } /** @@ -63,9 +56,7 @@ abstract class _Stream { * @param track - The MediaStreamTrack. */ protected addTrackHandlers(track: MediaStreamTrack) { - track.addEventListener('mute', this.handleTrackMuted); - track.addEventListener('unmute', this.handleTrackUnmuted); - track.addEventListener('ended', this.handleTrackEnded); + this.addTrackHandlersForStreamEvents(track); } /** @@ -74,8 +65,6 @@ abstract class _Stream { * @param track - The MediaStreamTrack. */ protected removeTrackHandlers(track: MediaStreamTrack) { - track.removeEventListener('mute', this.handleTrackMuted); - track.removeEventListener('unmute', this.handleTrackUnmuted); track.removeEventListener('ended', this.handleTrackEnded); } @@ -88,13 +77,6 @@ abstract class _Stream { return this.outputStream.id; } - /** - * Check whether or not this stream is muted. - * - * @returns True if the stream is muted, false otherwise. - */ - abstract get muted(): boolean; - /** * Get the track of the output stream. *