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

Initial implementation of audio input ring buffer. #268

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

namark
Copy link
Contributor

@namark namark commented Mar 10, 2023

The ring buffer uses SharedArrayBuffer which requires cross origin isolation. For local testing the client can be hosted using statikk which has a built in convenience option:

npx statikk --port 8080 --coi

The ringbuf.js source is included since I had trouble importing the node module in the audio worklet. The module was also outdated, missing some useful features.

@namark namark marked this pull request as ready for review March 14, 2023 15:54
@ctrlaltdavid ctrlaltdavid added CR approved At least one code reviewer has approved the PR bugfix needs CR (code review) needs QA (testing) and removed CR approved At least one code reviewer has approved the PR labels Mar 14, 2023
src/AudioMixer.ts Outdated Show resolved Hide resolved
Copy link
Member

@Gigabyte5671 Gigabyte5671 left a comment

Choose a reason for hiding this comment

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

Looks great apart from a few linting errors. I'll push a fix for these in a moment.

@Gigabyte5671 Gigabyte5671 added CR approved At least one code reviewer has approved the PR and removed needs CR (code review) labels Mar 15, 2023

constructor(options?: AudioWorkletNodeOptions) {
super(options); // eslint-disable-line

// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
this._ringBufferStorage = options?.processorOptions?.ringBufferStorage as SharedArrayBuffer;
Copy link
Member

Choose a reason for hiding this comment

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

This line is not necessarily type safe. Do we need to account for a situation where options, processorOptions, or ringBufferStorage are undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really, that would be a programmer error and should fail loudly cause there is sort of a type check in the RingBuffer constructor. Ideally we would have options parameter typed to require this, but the call site is not direct in this case, It happens in the AudioInput.ts through instantiation of AudioWorkletNode with an string id that is associated with AudioInputProcessor in AudioWorklets.ts. In theory typescript should be able to handle all of that, I think I saw a feature that allows mapping string literals to a types, but it might get tricky if the browser builtin typing we're using can't accommodate something like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ow no, the "sort of type check" is for another parameter, so I'll add explicit check there with an exception.

@Gigabyte5671 Gigabyte5671 added the question Further information is requested label Mar 15, 2023
@Gigabyte5671 Gigabyte5671 removed the question Further information is requested label Mar 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix CR approved At least one code reviewer has approved the PR needs QA (testing)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants