-
Notifications
You must be signed in to change notification settings - Fork 53
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-2030 | API stubs and error checking #202
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -3,6 +3,7 @@ | |||||||||||||||||||||
* @module Voice | ||||||||||||||||||||||
*/ | ||||||||||||||||||||||
import { EventEmitter } from 'events'; | ||||||||||||||||||||||
import AudioProcessor from './audioprocessor'; | ||||||||||||||||||||||
import Device from './device'; | ||||||||||||||||||||||
import { InvalidArgumentError, NotSupportedError } from './errors'; | ||||||||||||||||||||||
import Log from './log'; | ||||||||||||||||||||||
|
@@ -147,6 +148,11 @@ class AudioHelper extends EventEmitter { | |||||||||||||||||||||
*/ | ||||||||||||||||||||||
private _onActiveInputChanged: (stream: MediaStream | null) => Promise<void>; | ||||||||||||||||||||||
|
||||||||||||||||||||||
/** | ||||||||||||||||||||||
* Internal reference to the added AudioProcessor | ||||||||||||||||||||||
*/ | ||||||||||||||||||||||
private _processor: AudioProcessor | null; | ||||||||||||||||||||||
|
||||||||||||||||||||||
/** | ||||||||||||||||||||||
* A record of unknown devices (Devices without labels) | ||||||||||||||||||||||
*/ | ||||||||||||||||||||||
|
@@ -342,6 +348,35 @@ class AudioHelper extends EventEmitter { | |||||||||||||||||||||
}); | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
/** | ||||||||||||||||||||||
* Adds an {@link AudioProcessor}. If an {@link AudioProcessor} exists in the | ||||||||||||||||||||||
* {@link AudioHelper}, the {@link AudioHelper} routes the input audio stream | ||||||||||||||||||||||
* to the {@link AudioProcessor}. | ||||||||||||||||||||||
* | ||||||||||||||||||||||
* The {@link AudioHelper} guarantees that this audio stream is always | ||||||||||||||||||||||
* the active input audio stream and will automatically | ||||||||||||||||||||||
* update whenever the user's preference changes. | ||||||||||||||||||||||
* | ||||||||||||||||||||||
* Only one {@link AudioProcessor} can be added at this time. | ||||||||||||||||||||||
* @param processor | ||||||||||||||||||||||
*/ | ||||||||||||||||||||||
addProcessor(processor: AudioProcessor): void { | ||||||||||||||||||||||
if (this._processor) { | ||||||||||||||||||||||
throw new NotSupportedError('Adding multiple AudioProcessors is not supported at this time.'); | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
if (!processor) { | ||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we want to do something a little stricter here like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. But @manjeshbhargav 's suggestion seems better. It accounts for null. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Testing it more, your suggestion is better but with a check on null. Will update. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change will make sure a value of
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't want the user to set this to null using this method. I'll check for object and null instead. |
||||||||||||||||||||||
throw new InvalidArgumentError('Missing AudioProcessor argument.'); | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
if (typeof processor.createProcessedStream !== 'function' || | ||||||||||||||||||||||
typeof processor.destroyProcessedStream !== 'function') { | ||||||||||||||||||||||
throw new InvalidArgumentError('Missing createProcessedStream or destroyProcessedStream.'); | ||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||||
} | ||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Nitpick |
||||||||||||||||||||||
|
||||||||||||||||||||||
this._processor = processor; | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
/** | ||||||||||||||||||||||
* Enable or disable the disconnect sound. | ||||||||||||||||||||||
* @param doEnable Passing `true` will enable the sound and `false` will disable the sound. | ||||||||||||||||||||||
|
@@ -372,6 +407,18 @@ class AudioHelper extends EventEmitter { | |||||||||||||||||||||
return this._maybeEnableSound(Device.SoundName.Outgoing, doEnable); | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
/** | ||||||||||||||||||||||
* Removes an AudioProcessor. | ||||||||||||||||||||||
* @param processor | ||||||||||||||||||||||
*/ | ||||||||||||||||||||||
removeProcessor(processor: AudioProcessor): void { | ||||||||||||||||||||||
if (this._processor !== processor) { | ||||||||||||||||||||||
throw new InvalidArgumentError('Cannot remove an AudioProcessor that has not been previously added.'); | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
this._processor = null; | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
/** | ||||||||||||||||||||||
* 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 | ||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
/** | ||
* @packageDocumentation | ||
* @module Voice | ||
*/ | ||
|
||
/** | ||
* Represents an AudioProcessor object that receives an audio stream for processing. | ||
* @publicapi | ||
*/ | ||
interface AudioProcessor { | ||
/** | ||
* Called whenever the active input audio stream is updated | ||
* and is ready for processing such as adding audio filters | ||
* or removing background noise. | ||
* Use this method to initiate your audio processing pipeline. | ||
* | ||
* @param stream The input audio stream. | ||
* @returns The modified input audio stream after applying filters. | ||
*/ | ||
createProcessedStream(stream: MediaStream): Promise<MediaStream>; | ||
|
||
/** | ||
* Called after the processed stream has been destroyed. | ||
* This happens whenever the current input stream is updated. | ||
* Use this method to run any necessary teardown routines | ||
* needed by your audio processing pipeline. | ||
* | ||
* @param stream The torn down processed audio stream. | ||
* @returns | ||
*/ | ||
destroyProcessedStream(stream: MediaStream): Promise<void>; | ||
} | ||
|
||
export default AudioProcessor; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion, the wording here seems to imply "If an AudioProcess (already) exists in the..." due to grammatical structure. My gut instinct reading this is that the functionality only works on the second or further invocation of the function, which I am assuming is not true. I'm trying to think of how I would word this... maybe something like
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that currently we do not support multiple AudioProcessors, so maybe you'd want to reword that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what you mean by "the functionality only works on the second or further invocation of the function". This was reviewed with the whole team. And another one with you in the IDL. I'm just wondering why the confusion now? Can you elaborate?