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 2 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 Chrome browser is choppy when another application is using the audio devices.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- Fixed an [issue](https://github.com/twilio/twilio-voice.js/issues/197) where audio in Chrome browser is choppy when another application is using the audio devices.
- 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
10 changes: 9 additions & 1 deletion lib/twilio/device.ts
Original file line number Diff line number Diff line change
Expand Up @@ -939,7 +939,15 @@ class Device extends EventEmitter {

const config: Call.Config = {
audioHelper: this._audio,
getUserMedia: this._options.getUserMedia || getUserMedia,
getUserMedia: (...args) =>
(this._options.getUserMedia || getUserMedia)(...args)
.then((gumResponse: any) => {
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);
});
return Promise.resolve(gumResponse);
}),
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems like we're attaching extra responsibility to gUM here that it normally is not responsible for. Seems kinda strange, my gut instinct would be to pass the _audio to the Call so the Call can perform that logic itself after using gUM. Otherwise, this logic is kind of obscure and the Call gUM is intrinsically different than other places we use gUM.

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 did consider it. However, the Call object does not know anything about the audio helper. The Device object has that responsibility. Putting audio helper inside the Call object means, we're now sharing that between Call and Device, which is not necessary.

We're not attaching extra responsibility to gUM. The Device is responsible for audio devices, including the sequence of MediaDevice's API calls (gUM, enumerateDevices). This implementation is doing that.

Also, no need to call _updateAvailableDevices in other places where we call gUM. We only need it at the start of the call.

Copy link
Collaborator

@mhuynh5757 mhuynh5757 Sep 19, 2023

Choose a reason for hiding this comment

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

Could we rename the Call configuration parameter from gUM to something else? I know it looks cumbersome but I don't like having unknown side-effects.

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 that might create more confusion. Instead, I changed the implementation to use a callback instead. Please take another look.

isUnifiedPlanDefault: Device._isUnifiedPlanDefault,
onIgnore: (): void => {
this._soundcache.get(Device.SoundName.Incoming).stop();
Expand Down
190 changes: 0 additions & 190 deletions lib/twilio/shims/mediadevices.ts
Copy link
Collaborator

Choose a reason for hiding this comment

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

Love to see us not needing these shims anymore.

This file was deleted.

46 changes: 5 additions & 41 deletions tests/audiohelper.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,11 @@ describe('AudioHelper', () => {
let oldNavigator;

beforeEach(() => {
audio = new AudioHelper(noop, noop, getUserMedia, { mediaDevices: {} });
audio = new AudioHelper(noop, noop, getUserMedia, { mediaDevices: {
enumerateDevices: function(){
return Promise.resolve();
}
} });
});

before(() => {
Expand Down Expand Up @@ -505,45 +509,5 @@ describe('AudioHelper', () => {
})));
});
});

describe('mediaDevices:deviceinfochange', () => {
let audio;
const wait = () => new Promise(r => setTimeout(r, 0));
const noop = () => {};

beforeEach(() => {
audio = new AudioHelper(noop, noop, getUserMedia, {
mediaDevices,
setSinkId: () => {}
});
audio.speakerDevices = {};
audio.ringtoneDevices = {
get: () => ({ size: 1 }),
set: sinon.stub(),
};
audio.speakerDevices = {
get: () => ({ size: 0 }),
set: sinon.stub().rejects('foobar'),
};
audio._log.warn = sinon.stub();
audio.isOutputSelectionSupported = true;
});

it('should set output devices', () => {
handlers.get('deviceinfochange')();
wait().then(() => sinon.assert.called(audio.speakerDevices.set));
});

it('should catch error when setting output devices', () => {
handlers.get('deviceinfochange')();
wait().then(() => sinon.assert.called(audio._log.warn));
});

it('should not set device when isSupported is false', () => {
audio.isOutputSelectionSupported = false;
handlers.get('deviceinfochange')();
wait().then(() => sinon.assert.notCalled(audio.speakerDevices.set));
});
});
});
});
2 changes: 0 additions & 2 deletions tests/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -102,5 +102,3 @@ require('./unit/error');
require('./unit/log');
require('./unit/regions');
require('./unit/uuid');

require('./shims/mediadevices');
Loading