Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

VBLOCKS-2214 | Remove unnecessary enumerateDevices calls #199

Merged
merged 4 commits into from
Sep 20, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
Changes
-------

- Fixed an [issue](https://github.com/twilio/twilio-voice.js/issues/197) where audio in the Chrome browser is choppy when another application is also using the audio devices.
- Added missing documentation for the following events:
- `call.on('ringing', handler)`
- `call.on('warning', handler)`
Expand Down
71 changes: 34 additions & 37 deletions lib/twilio/audiohelper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import { InvalidArgumentError, NotSupportedError } from './errors';
import Log from './log';
import OutputDeviceCollection from './outputdevicecollection';
import MediaDeviceInfoShim from './shims/mediadeviceinfo';
import getMediaDevicesInstance from './shims/mediadevices';
import { average, difference, isFirefox } from './util';

/**
Expand Down Expand Up @@ -176,11 +175,11 @@ class AudioHelper extends EventEmitter {
}, options);

this._getUserMedia = getUserMedia;
this._mediaDevices = options.mediaDevices || getMediaDevicesInstance() as AudioHelper.MediaDevicesLike;
this._mediaDevices = options.mediaDevices || navigator.mediaDevices;
this._onActiveInputChanged = onActiveInputChanged;
this._enumerateDevices = typeof options.enumerateDevices === 'function'
? options.enumerateDevices
: this._mediaDevices && this._mediaDevices.enumerateDevices;
: this._mediaDevices && this._mediaDevices.enumerateDevices.bind(this._mediaDevices);

const isAudioContextSupported: boolean = !!(options.AudioContext || options.audioContext);
const isEnumerationSupported: boolean = !!this._enumerateDevices;
Expand Down Expand Up @@ -308,10 +307,41 @@ class AudioHelper extends EventEmitter {

if (this._mediaDevices.removeEventListener) {
this._mediaDevices.removeEventListener('devicechange', this._updateAvailableDevices);
this._mediaDevices.removeEventListener('deviceinfochange', this._updateAvailableDevices);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this a browser compatibility thing we no longer need?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

}
}

/**
* Update the available input and output devices
* @private
*/
_updateAvailableDevices = (): Promise<void> => {
if (!this._mediaDevices || !this._enumerateDevices) {
return Promise.reject('Enumeration not supported');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to reject an error here, or just a string? Also, could we make this an async function and then use await and throw in the function?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was the original implementation and did not see any reason to change it. The only change that was required is to make it an internal-only public API.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. Let's leave it as-is then.

}

return this._enumerateDevices().then((devices: MediaDeviceInfo[]) => {
this._updateDevices(devices.filter((d: MediaDeviceInfo) => d.kind === 'audiooutput'),
this.availableOutputDevices,
this._removeLostOutput);

this._updateDevices(devices.filter((d: MediaDeviceInfo) => d.kind === 'audioinput'),
this.availableInputDevices,
this._removeLostInput);

const defaultDevice = this.availableOutputDevices.get('default')
|| Array.from(this.availableOutputDevices.values())[0];

[this.speakerDevices, this.ringtoneDevices].forEach(outputDevices => {
if (!outputDevices.get().size && this.availableOutputDevices.size && this.isOutputSelectionSupported) {
outputDevices.set(defaultDevice.deviceId)
.catch((reason) => {
this._log.warn(`Unable to set audio output devices. ${reason}`);
});
}
});
});
}

/**
* Enable or disable the disconnect sound.
* @param doEnable Passing `true` will enable the sound and `false` will disable the sound.
Expand Down Expand Up @@ -426,7 +456,6 @@ class AudioHelper extends EventEmitter {

if (this._mediaDevices.addEventListener) {
this._mediaDevices.addEventListener('devicechange', this._updateAvailableDevices);
this._mediaDevices.addEventListener('deviceinfochange', this._updateAvailableDevices);
}

this._updateAvailableDevices().then(() => {
Expand Down Expand Up @@ -542,37 +571,6 @@ class AudioHelper extends EventEmitter {
});
}

/**
* Update the available input and output devices
*/
private _updateAvailableDevices = (): Promise<void> => {
if (!this._mediaDevices || !this._enumerateDevices) {
return Promise.reject('Enumeration not supported');
}

return this._enumerateDevices().then((devices: MediaDeviceInfo[]) => {
this._updateDevices(devices.filter((d: MediaDeviceInfo) => d.kind === 'audiooutput'),
this.availableOutputDevices,
this._removeLostOutput);

this._updateDevices(devices.filter((d: MediaDeviceInfo) => d.kind === 'audioinput'),
this.availableInputDevices,
this._removeLostInput);

const defaultDevice = this.availableOutputDevices.get('default')
|| Array.from(this.availableOutputDevices.values())[0];

[this.speakerDevices, this.ringtoneDevices].forEach(outputDevices => {
if (!outputDevices.get().size && this.availableOutputDevices.size && this.isOutputSelectionSupported) {
outputDevices.set(defaultDevice.deviceId)
.catch((reason) => {
this._log.warn(`Unable to set audio output devices. ${reason}`);
});
}
});
});
}

/**
* Update a set of devices.
* @param updatedDevices - An updated list of available Devices
Expand Down Expand Up @@ -680,7 +678,6 @@ namespace AudioHelper {
* that were lost as a result of this deviceChange event.
* @example `device.audio.on('deviceChange', lostActiveDevices => { })`
* @event
* @private
*/
declare function deviceChangeEvent(lostActiveDevices: MediaDeviceInfo[]): void;

Expand Down
9 changes: 9 additions & 0 deletions lib/twilio/call.ts
Original file line number Diff line number Diff line change
Expand Up @@ -669,6 +669,10 @@ class Call extends EventEmitter {
data: { audioConstraints },
}, this);

if (this._options.onGetUserMedia) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about onGetUserMediaResolved?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's too verbose..
if you look at existing events, we don't really use past tense. For example, after .accept() is invoked, we emit accept event, no event for failures. Same for other things. Using the convention onEventName has an implicit meaning that an event happened and succeeded.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. That seems to be true with the JS SDK, but the RN SDK (which takes direct inspiration from the mobile SDKs) uses past tense like connected or reconnected. But if that is the convention then this is a plus one from me.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just capturing our slack discussion here for future reference. This is more of a JS convention. For example, onClick, and not onClicked. onMouseMove, onTouchStart etc. There are exceptions of course. If we need to capture different stages, then we need to change. For example, connecting, connected, reconnecting, reconnected.

this._options.onGetUserMedia();
}

connect();
}, (error: Record<string, any>) => {
let twilioError;
Expand Down Expand Up @@ -1914,6 +1918,11 @@ namespace Call {
*/
offerSdp?: string | null;

/**
* Called after a successful getUserMedia call
*/
onGetUserMedia?: () => void;

/**
* Whether this is a preflight call or not
*/
Expand Down
13 changes: 12 additions & 1 deletion lib/twilio/device.ts
Original file line number Diff line number Diff line change
Expand Up @@ -969,6 +969,7 @@ class Device extends EventEmitter {
getInputStream: (): MediaStream | null => this._options.fileInputStream || this._callInputStream,
getSinkIds: (): string[] => this._callSinkIds,
maxAverageBitrate: this._options.maxAverageBitrate,
onGetUserMedia: () => this._onGetUserMedia(),
preflight: this._options.preflight,
rtcConstraints: this._options.rtcConstraints,
shouldPlayDisconnect: () => this._audio?.disconnect(),
Expand Down Expand Up @@ -1088,10 +1089,20 @@ class Device extends EventEmitter {
}
}

/**
* Called after a successful getUserMedia call
*/
private _onGetUserMedia = () => {
this._audio?._updateAvailableDevices().catch(error => {
// Ignore error, we don't want to break the call flow
this._log.warn('Unable to updateAvailableDevices after gUM call', error);
});
}

/**
* Called when a 'close' event is received from the signaling stream.
*/
private _onSignalingClose = () => {
private _onSignalingClose = () => {
this._stream = null;
this._streamConnectedPromise = null;
}
Expand Down
190 changes: 0 additions & 190 deletions lib/twilio/shims/mediadevices.ts

This file was deleted.

Loading