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

Replace ScriptProcessorNode in p5.SoundRecorder with AudioWorkletNode #369

Merged
merged 2 commits into from
Jun 27, 2019

Conversation

oshoham
Copy link
Contributor

@oshoham oshoham commented Jun 25, 2019

  • Polyfill AudioWorklet for non-Chrome browsers using audioworklet-polyfill (falls back to ScriptProcessorNode)

  • Rewrite p5.SoundRecorder to use an AudioWorkletNode instead of a ScriptProcessorNode

  • Use raw-loader to:

    1. transpile an ES6 AudioWorkletProcessor class to ES5
    2. import the transpiled ES5 source as a string
    3. create a Blob from the source string
    4. create an ObjectURL for the Blob
    5. load the AudioWorkletProcessor via audioContext.audioWorklet.addModule using the ObjectURL
  • Use babel-plugin-preval to share the AudioWorkletProcessor name (passed to both registerProcessor and the AudioWorkletNode constructor) between the transpiled processor and p5.SoundRecorder

  • Take advantage of p5's init hook and preload system to synchronously load all AudioWorkletProcessor Blobs before the p5 setup() function runs

@oshoham oshoham requested a review from therewasaguy June 25, 2019 07:54
@oshoham
Copy link
Contributor Author

oshoham commented Jun 27, 2019

@therewasaguy One issue that I haven't addressed in this PR is that the buffer size for the ScriptProcessorNode in p5.SoundRecorder was 1024, while the buffer size for an AudioWorkletNode is locked at 128. I haven't noticed any audible glitches resulting from this change, but presumably the decreased buffer size means an increase in overall CPU usage.

The recommended way of dealing with this AudioWorklet problem seems to be a ring buffer. That article from Google even suggests audio recording as an ideal use case.

Do you think I should look into adding a ring buffer as part of this PR, or should I work on that separately?

Copy link
Member

@therewasaguy therewasaguy left a comment

Choose a reason for hiding this comment

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

This is fantastic, @oshoham ! Many great finds in this PR, and while I'd love to simplify sharing the AudioWorkletProcessor name, I think you've come up with a great solution given the constraints of the transpiled processor.

I think it's ok to look into the RingBuffer in a separate PR—great suggestion, I haven't noticed any glitches but buffer size mismatch seems like something we should handle.

@@ -0,0 +1,9 @@
{
"globals": {
"sampleRate": true,
Copy link
Member

Choose a reason for hiding this comment

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

ah, so this is global to the AudioWorklet global scope? Maybe worth adding the other attributes listed here?
https://www.w3.org/TR/webaudio/#AudioWorkletGlobalScope-attributes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah that's a good suggestion, I'll go ahead and add the other two global variables listed there

const buffers = this.getBuffers();
const leftBuffer = buffers[0].buffer;
const rightBuffer = buffers[1].buffer;
this.port.postMessage({ name: 'buffers', leftBuffer: leftBuffer, rightBuffer: rightBuffer }, [leftBuffer, rightBuffer]);
Copy link
Member

Choose a reason for hiding this comment

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

do we need the array?

Copy link
Contributor Author

@oshoham oshoham Jun 27, 2019

Choose a reason for hiding this comment

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

we do, actually - this syntax transfers ownership of the buffers to the main thread rather than copying them, which (at least as I understand it) is faster

see https://developer.mozilla.org/en-US/docs/Web/API/Worker/postMessage#Transfer_Example

return true;
} else if (this.sampleLimit && this.recordedSamples >= this.sampleLimit) {
this.stop();
return true;
Copy link
Member

Choose a reason for hiding this comment

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

interesting, reading this:

Nodes that act as sources of output, typically with a lifetime. Such nodes SHOULD return true from process() until the point at which they are no longer producing an output

so this should always return true

Copy link
Contributor Author

@oshoham oshoham Jun 27, 2019

Choose a reason for hiding this comment

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

yeah, my understanding is that you should only return false from a processor when you're ready for that node to be garbage-collected, although I could be wrong there

@oshoham oshoham force-pushed the audioworklet-soundrecorder branch from e978673 to 50a06b5 Compare June 27, 2019 08:22
@oshoham oshoham merged commit e03ab3d into processing:master Jun 27, 2019
@oshoham oshoham deleted the audioworklet-soundrecorder branch June 27, 2019 08:25
@therewasaguy therewasaguy mentioned this pull request Jan 6, 2020
therewasaguy added a commit that referenced this pull request Jan 6, 2020
### New
- AudioWorklet replaces the deprecated ScriptProcessorNode (when available) in p5.SoundRecorder, p5.Amplitude, and p5.SoundFile, as part of @oshoham's GSoC project https://github.com/processing/p5.js/blob/master/developer_docs/project_wrapups/orenshoham_gsoc_2019.md
#369
#373
- p5 library compiled with webpack so it looks a little different and we had to remove comments that were tripping up YUIDoc here in the p5.js repo

### Fixes
- p5.SoundFile: stop() won't stop a soundfile loop https://github.com/processing/p5.js-sound/issues/401
- p5.SoundFile: addCue() not triggering function calls https://github.com/processing/p5.js-sound/issues/371
- p5.SoundFile: jump() only called if soundfile is playing #404
- p5.SoundFile: save() bugfix #406
- remove bad console call (#378)
therewasaguy added a commit to processing/p5.js that referenced this pull request Jan 6, 2020
- improved inline documentation - all examples start audio on user gesture processing/p5.js-sound#403
- AudioWorklet replaces the deprecated ScriptProcessorNode (when available) in p5.SoundRecorder, p5.Amplitude, and p5.SoundFile, as part of @oshoham's GSoC project https://github.com/processing/p5.js/blob/master/developer_docs/project_wrapups/orenshoham_gsoc_2019.md
processing/p5.js-sound#369
processing/p5.js-sound#373
- p5 library compiled with webpack so it looks a little different and we had to remove comments that were tripping up YUIDoc here in the p5.js repo

- p5.SoundFile: stop() won't stop a soundfile loop https://github.com/processing/p5.js-sound/issues/401
- p5.SoundFile: addCue() not triggering function calls https://github.com/processing/p5.js-sound/issues/371
- p5.SoundFile: jump() only called if soundfile is playing processing/p5.js-sound#404
- p5.SoundFile: save() bugfix processing/p5.js-sound#406
- remove bad console call (processing/p5.js-sound#378)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants