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-1111 | Fix sound definitions #153

Merged
merged 5 commits into from
Mar 31, 2023
Merged

VBLOCKS-1111 | Fix sound definitions #153

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1d4b7aa
VBLOCKS-1534 | Stop using deprecated RTCIceCandidateStats
charliesantos Mar 30, 2023
5b7c76d
VBLOCKS-1111 | Fix sound definitions
charliesantos Mar 30, 2023
312dff9
Tests and changelog
charliesantos Mar 30, 2023
da15d6d
Merge branch 'bugfix/github_issues' into audio_sound_def
charliesantos Mar 31, 2023
1eb9189
Update docs
charliesantos Mar 31, 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
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,15 @@ Changes

- Updated the description of [Device.updateToken](https://twilio.github.io/twilio-voice.js/classes/voice.device.html#updatetoken) API. It is recommended to call this API after [Device.tokenWillExpireEvent](https://twilio.github.io/twilio-voice.js/classes/voice.device.html#tokenwillexpireevent) is emitted, and before or after a call to prevent a potential ~1s audio loss during the update process.

- Updated stats reporting to stop using deprecated `RTCIceCandidateStats` - `ip` and `deleted`.

Bug Fixes
---------

- Fixed an [issue](https://github.com/twilio/twilio-voice.js/issues/100) where a `TypeError` is thrown after rejecting a call then invoking `updateToken`.

- Fixed an [issue](https://github.com/twilio/twilio-voice.js/issues/14) where `device.audio.disconnect`, `device.audio.incoming` and `device.audio.outgoing` do not have the correct type definitions.

2.3.2 (February 27, 2023)
===================

Expand Down
81 changes: 49 additions & 32 deletions lib/twilio/audiohelper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,15 @@ class AudioHelper extends EventEmitter {
*/
private _audioContext?: AudioContext;

/**
* Whether each sound is enabled.
*/
private _enabledSounds: Record<Device.ToggleableSound, boolean> = {
[Device.SoundName.Disconnect]: true,
[Device.SoundName.Incoming]: true,
[Device.SoundName.Outgoing]: true,
};

/**
* The `getUserMedia()` function to use.
*/
Expand Down Expand Up @@ -172,10 +181,6 @@ class AudioHelper extends EventEmitter {
this.isOutputSelectionSupported = isEnumerationSupported && isSetSinkSupported;
this.isVolumeSupported = isAudioContextSupported;

if (options.enabledSounds) {
this._addEnabledSounds(options.enabledSounds);
}

if (this.isVolumeSupported) {
this._audioContext = options.audioContext || options.AudioContext && new options.AudioContext();
if (this._audioContext) {
Expand Down Expand Up @@ -287,6 +292,33 @@ class AudioHelper extends EventEmitter {
}
}

/**
* Set whether the disconnect sound is enabled or not
* @param isEnabled - Whether the disconnect sound is enabled or not
* @returns Whether the disconnect sound is enabled or not
*/
disconnect(isEnabled?: boolean): boolean {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we pass undefined it seems like these functions just report the enable status of the sound, is that correct? Should we put that behavior in the docstring?

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 that's correct. If you looks at the generated API doc, the return value kind of explains that right? That is, it's always returning the enable status of the sound whether you pass a boolean or undefined.

Copy link
Collaborator

Choose a reason for hiding this comment

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

My confusion is that I wasn't sure if passing undefined would be inferred as false since it's a falsy value, or if it would make it a no-op.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It won't be. See _maybeEnableSound implementation in this file. I also added tests for this. Please see the tests.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah did you mean something like disconnect(undefined) instead of disconnect()?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's clear when reading the implementation but not when reading only the docstring, is what I'm trying to say. Applies to either implicit or explicit undefined.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok gotcha. Can you provide a suggestion on the docstring? What would make it clear?

Copy link
Collaborator

Choose a reason for hiding this comment

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

How does this sound?

Enable or disable the disconnect sound.
@param doEnable - An optional boolean value. Passing `true` will enable the sound and `false` will disable the sound. Not passing this parameter will not alter the enable-status of the sound.
@returns - A boolean value representing the enable-status of the sound.

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 is too verbose.
Re: @param doEnable. Typedoc already generates explanation that this is an optional boolean value. I don't think mentioning it again in the docstring is good.
Re: @returns. Typedoc also mentions that the return value is boolean. No need to mention again.
I tweaked it a bit. Please see latest.

return this._maybeEnableSound(Device.SoundName.Disconnect, isEnabled);
}

/**
* Set whether the incoming sound is enabled or not
* @param isEnabled - Whether the incoming sound is enabled or not
* @returns Whether the incoming sound is enabled or not
*/
incoming(isEnabled?: boolean): boolean {
return this._maybeEnableSound(Device.SoundName.Incoming, isEnabled);
}

/**
* Set whether the outgoing sound is enabled or not
* @param isEnabled - Whether the outgoing sound is enabled or not
* @returns Whether the outgoing sound is enabled or not
*/
outgoing(isEnabled?: boolean): boolean {
return this._maybeEnableSound(Device.SoundName.Outgoing, isEnabled);
}

/**
* Set the MediaTrackConstraints to be applied on every getUserMedia call for new input
* device audio. Any deviceId specified here will be ignored. Instead, device IDs should
Expand Down Expand Up @@ -343,27 +375,6 @@ class AudioHelper extends EventEmitter {
});
}

/**
* Merge the passed enabledSounds into {@link AudioHelper}. Currently used to merge the deprecated
* Device.sounds object onto the new {@link AudioHelper} interface. Mutates
* by reference, sharing state between {@link Device} and {@link AudioHelper}.
* @param enabledSounds - The initial sound settings to merge.
* @private
*/
private _addEnabledSounds(enabledSounds: { [name: string]: boolean }) {
function setValue(key: Device.ToggleableSound, value: boolean) {
if (typeof value !== 'undefined') {
enabledSounds[key] = value;
}

return enabledSounds[key];
}

Object.keys(enabledSounds).forEach(key => {
(this as any)[key] = setValue.bind(null, key);
});
}

/**
* Get the index of an un-labeled Device.
* @param mediaDeviceInfo
Expand Down Expand Up @@ -407,6 +418,19 @@ class AudioHelper extends EventEmitter {
});
}

/**
* Set whether the sound is enabled or not
* @param soundName
* @param isEnabled
* @returns Whether the sound is enabled or not
*/
private _maybeEnableSound(soundName: Device.ToggleableSound, isEnabled?: boolean): boolean {
if (typeof isEnabled !== 'undefined') {
this._enabledSounds[soundName] = isEnabled;
}
return this._enabledSounds[soundName];
}

/**
* Remove an input device from inputs
* @param lostDevice
Expand Down Expand Up @@ -669,13 +693,6 @@ namespace AudioHelper {
*/
audioContext?: AudioContext;

/**
* A Record of sounds. This is modified by reference, and is used to
* maintain backward-compatibility. This should be removed or refactored in 2.0.
* TODO: Remove / refactor in 2.0. (CLIENT-5302)
*/
enabledSounds?: Record<Device.ToggleableSound, boolean>;

/**
* A custom MediaDevices instance to use.
*/
Expand Down
20 changes: 4 additions & 16 deletions lib/twilio/device.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -354,15 +354,6 @@ class Device extends EventEmitter {
*/
private _edge: string | null = null;

/**
* Whether each sound is enabled.
*/
private _enabledSounds: Record<Device.ToggleableSound, boolean> = {
[Device.SoundName.Disconnect]: true,
[Device.SoundName.Incoming]: true,
[Device.SoundName.Outgoing]: true,
};

/**
* The name of the home region the {@link Device} is connected to.
*/
Expand Down Expand Up @@ -978,7 +969,7 @@ class Device extends EventEmitter {
maxAverageBitrate: this._options.maxAverageBitrate,
preflight: this._options.preflight,
rtcConstraints: this._options.rtcConstraints,
shouldPlayDisconnect: () => this._enabledSounds.disconnect,
shouldPlayDisconnect: () => this.audio?.disconnect(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

When would this.audio be undefined or null?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's initial value is null. Or when it gets destroyed and you still have lingering call objects with listeners that references this.audio. Very edgy but we want to fail silently instead of throwing benign errors.

twimlParams,
voiceEventSidGenerator: this._options.voiceEventSidGenerator,
}, options);
Expand All @@ -1003,7 +994,7 @@ class Device extends EventEmitter {
this._audio._maybeStartPollingVolume();
}

if (call.direction === Call.CallDirection.Outgoing && this._enabledSounds.outgoing) {
if (call.direction === Call.CallDirection.Outgoing && this.audio?.outgoing()) {
this._soundcache.get(Device.SoundName.Outgoing).play();
}

Expand Down Expand Up @@ -1213,7 +1204,7 @@ class Device extends EventEmitter {
this._publishNetworkChange();
});

const play = (this._enabledSounds.incoming && !wasBusy)
const play = (this.audio?.incoming() && !wasBusy)
? () => this._soundcache.get(Device.SoundName.Incoming).play()
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 this kind of logic could be placed in the AudioHelper class itself. Probably out of scope for this ticket though.

: () => Promise.resolve();

Expand Down Expand Up @@ -1320,10 +1311,7 @@ class Device extends EventEmitter {
this._updateSinkIds,
this._updateInputStream,
getUserMedia,
{
audioContext: Device.audioContext,
enabledSounds: this._enabledSounds,
},
{ audioContext: Device.audioContext },
);

this._audio.on('deviceChange', (lostActiveDevices: MediaDeviceInfo[]) => {
Expand Down
1 change: 1 addition & 0 deletions lib/twilio/rtc/icecandidate.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
*/
interface RTCIceCandidatePayload {
candidate_type: string;
// Deprecated by newer browsers. Will likely not show on most recent versions of browsers.
deleted: boolean;
ip: string;
is_remote: boolean;
Expand Down
5 changes: 3 additions & 2 deletions lib/twilio/rtc/stats.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,8 +204,9 @@ function createRTCSample(statsReport) {
}

Object.assign(sample, {
localAddress: localCandidate && localCandidate.ip,
remoteAddress: remoteCandidate && remoteCandidate.ip,
// ip is deprecated. use address first then ip if on older versions of browser
localAddress: localCandidate && (localCandidate.address || localCandidate.ip),
remoteAddress: remoteCandidate && (remoteCandidate.address || remoteCandidate.ip),
});

return sample;
Expand Down
3 changes: 2 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
"url": "[email protected]:twilio/twilio-voice.js.git"
},
"scripts": {
"build": "npm-run-all clean build:constants build:errors docs:ts build:es5 build:ts build:dist build:dist-min",
"build": "npm-run-all clean build:constants build:errors docs:ts build:es5 build:ts build:dist build:dist-min test:typecheck",
"build:errors": "node ./scripts/errors.js",
"build:es5": "rimraf ./es5 && babel lib -d es5",
"build:dev": "ENV=dev npm run build",
Expand Down Expand Up @@ -55,6 +55,7 @@
"test:integration": "karma start $PWD/karma.conf.ts",
"test:network": "node ./scripts/karma.js $PWD/karma.network.conf.ts",
"test:selenium": "mocha tests/browser/index.js",
"test:typecheck": "./node_modules/typescript/bin/tsc tests/typecheck/index.ts --noEmit",
"test:unit": "nyc mocha -r ts-node/register ./tests/index.ts",
"test:webpack": "cd ./tests/webpack && npm install && npm test"
},
Expand Down
30 changes: 28 additions & 2 deletions tests/audiohelper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,7 +297,33 @@ describe('AudioHelper', () => {
});
});

describe('setAudioConstraints', () => {
['disconnect', 'incoming', 'outgoing'].forEach(soundName => {
describe(`.${soundName}`, () => {
let testFn;

beforeEach(() => {
testFn = audio[soundName].bind(audio);
});

it('should return true as default', () => {
assert.strictEqual(testFn(), true);
});

it('should return false after setting to false', () => {
assert.strictEqual(testFn(false), false);
assert.strictEqual(testFn(), false);
});

it('should return true after setting to true', () => {
assert.strictEqual(testFn(false), false);
assert.strictEqual(testFn(), false);
assert.strictEqual(testFn(true), true);
assert.strictEqual(testFn(), true);
});
});
});

describe('.setAudioConstraints', () => {
context('when no input device is active', () => {
it('should set .audioConstraints', () => {
audio.setAudioConstraints({ foo: 'bar' });
Expand Down Expand Up @@ -328,7 +354,7 @@ describe('AudioHelper', () => {
});
});

describe('unsetAudioConstraints', () => {
describe('.unsetAudioConstraints', () => {
beforeEach(() => {
audio.setAudioConstraints({ foo: 'bar' });
});
Expand Down
6 changes: 2 additions & 4 deletions tests/payloads/rtcstatsreport-edge.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,7 @@
"port": 52513,
"protocol": "udp",
"candidateType": "prflx",
"priority": 1853824767,
"deleted": false
"priority": 1853824767
},
{
"id": "RTCIceCandidate_yB1+bJsu",
Expand All @@ -59,8 +58,7 @@
"port": 11270,
"protocol": "udp",
"candidateType": "host",
"priority": 2130714367,
"deleted": false
"priority": 2130714367
},
{
"id": "RTCInboundRTPAudioStream_774468856",
Expand Down
18 changes: 6 additions & 12 deletions tests/payloads/rtcstatsreport-with-transport.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,8 +284,7 @@
"port": 53585,
"protocol": "udp",
"candidateType": "srflx",
"priority": 1686052607,
"deleted": false
"priority": 1686052607
},
{
"id": "RTCIceCandidate_Di/J4tEz",
Expand All @@ -297,8 +296,7 @@
"port": 19282,
"protocol": "udp",
"candidateType": "host",
"priority": 2130706431,
"deleted": false
"priority": 2130706431
},
{
"id": "RTCIceCandidate_UqrtRLJZ",
Expand All @@ -312,8 +310,7 @@
"protocol": "udp",
"relayProtocol": "udp",
"candidateType": "relay",
"priority": 41754879,
"deleted": false
"priority": 41754879
},
{
"id": "RTCIceCandidate_e3FLKFoG",
Expand All @@ -326,8 +323,7 @@
"port": 52497,
"protocol": "udp",
"candidateType": "srflx",
"priority": 1685921535,
"deleted": false
"priority": 1685921535
},
{
"id": "RTCIceCandidate_oYROhau8",
Expand All @@ -341,8 +337,7 @@
"protocol": "udp",
"relayProtocol": "udp",
"candidateType": "relay",
"priority": 41885951,
"deleted": false
"priority": 41885951
},
{
"id": "RTCIceCandidate_qQRsdncV",
Expand All @@ -355,8 +350,7 @@
"port": 50951,
"protocol": "udp",
"candidateType": "host",
"priority": 2122194687,
"deleted": false
"priority": 2122194687
},
{
"id": "RTCInboundRTPAudioStream_976775258",
Expand Down
8 changes: 3 additions & 5 deletions tests/payloads/rtcstatsreport.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,12 +162,11 @@
"type": "local-candidate",
"transportId": "RTCTransport_audio_1",
"isRemote": false,
"ip": "107.20.226.156",
"address": "107.20.226.156",
"port": 52513,
"protocol": "udp",
"candidateType": "prflx",
"priority": 1853824767,
"deleted": false
"priority": 1853824767
},
{
"id": "RTCIceCandidate_yB1+bJsu",
Expand All @@ -179,8 +178,7 @@
"port": 11270,
"protocol": "udp",
"candidateType": "host",
"priority": 2130714367,
"deleted": false
"priority": 2130714367
},
{
"id": "RTCInboundRTPAudioStream_774468856",
Expand Down
Loading