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.

Sign up for GitHub

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

VBLOCKS-2214 | Remove unnecessary enumerateDevices calls #199

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
096f7ad
VBLOCKS-2214 | Remove unnecessary enumerateDevices calls
charliesantos Sep 15, 2023
dfc5c22
changelog
charliesantos Sep 19, 2023
2359e9f
Using callback instead
charliesantos Sep 20, 2023
3ddb292
Changelog tweak
charliesantos Sep 20, 2023
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
2 changes: 1 addition & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +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.
- 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
9 changes: 9 additions & 0 deletions lib/twilio/call.ts
View file
Open in desktop
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
23 changes: 13 additions & 10 deletions lib/twilio/device.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -939,15 +939,7 @@ class Device extends EventEmitter {

const config: Call.Config = {
audioHelper: this._audio,
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);
}),
getUserMedia: this._options.getUserMedia || getUserMedia,
isUnifiedPlanDefault: Device._isUnifiedPlanDefault,
onIgnore: (): void => {
this._soundcache.get(Device.SoundName.Incoming).stop();
Expand Down Expand Up @@ -977,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 @@ -1096,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
13 changes: 11 additions & 2 deletions tests/unit/call.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -399,12 +399,14 @@ describe('Call', function() {
});

context('when getInputStream is present and succeeds', () => {
let getInputStream;
let getInputStream: any;
let onGetUserMedia: any;
let wait: Promise<any>;

beforeEach(() => {
onGetUserMedia = sinon.stub();
getInputStream = sinon.spy(() => 'foo');
Object.assign(options, { getInputStream });
Object.assign(options, { getInputStream, onGetUserMedia });
conn = new Call(config, options);

mediaHandler.setInputTracksFromStream = sinon.spy(() => {
Expand All @@ -426,6 +428,13 @@ describe('Call', function() {
});
});

it('should call onGetUserMedia callback', () => {
conn.accept();
return wait.then(() => {
sinon.assert.calledWith(onGetUserMedia);
});
});

context('when incoming', () => {
beforeEach(() => {
getInputStream = sinon.spy(() => 'foo');
Expand Down
6 changes: 2 additions & 4 deletions tests/unit/device.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ const ClientCapability = require('twilio').jwt.ClientCapability;
describe('Device', function() {
let activeCall: any;
let audioHelper: any;
let callConfig: any;
let clock: SinonFakeTimers;
let connectOptions: Record<string, any> | undefined;
let device: Device;
Expand Down Expand Up @@ -58,8 +57,7 @@ describe('Device', function() {
audioHelper.outgoing = () => enabledSounds[Device.SoundName.Outgoing];
return audioHelper;
};
const Call = (_config?: any, _connectOptions?: Record<string, any>) => {
callConfig = _config;
const Call = (_?: any, _connectOptions?: Record<string, any>) => {
connectOptions = _connectOptions;
return activeCall = createEmitterStub(require('../../lib/twilio/call').default);
};
Expand Down Expand Up @@ -203,7 +201,7 @@ describe('Device', function() {
describe('.connect(params?, audioConstraints?, iceServers?)', () => {
it('should update device list after a getUserMediaCall', async () => {
await device.connect();
await callConfig.getUserMedia();
connectOptions!.onGetUserMedia();
sinon.assert.calledOnce(updateAvailableDevicesStub);
});

Expand Down