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 with AudioWorkletNode in p5.SoundFile and p5.Amplitude #373

Merged
merged 3 commits into from
Jul 16, 2019

Conversation

oshoham
Copy link
Contributor

@oshoham oshoham commented Jul 10, 2019

  • Replace ScriptProcessorNode functionality with equivalent AudioWorklet processors in p5.SoundFile and p5.Amplitude
  • Ensure that a global p5 instance is created and initialized before the test suite runs, so that all AudioWorklet modules are loaded before the tests run.

@oshoham oshoham requested a review from therewasaguy July 10, 2019 21:02
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! 👏

this.processor = this.audiocontext.createScriptProcessor(this.bufferSize, 2, 1);
this._workletNode = new AudioWorkletNode(this.audiocontext, processorNames.amplitudeProcessor, {
outputChannelCount: [1],
parameterData: { smoothing: smoothing || 0 },
Copy link
Member

Choose a reason for hiding this comment

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

as a potential improvement, we could check the range here, as in the smooth method. But we could also just let the parameterDescriptors handle enforcing (and choosing whether to notify the user) about the range.

defaultValue: 0,
minValue: 0,
maxValue: 1,
automationRate: 'k-rate'
Copy link
Member

Choose a reason for hiding this comment

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

inspecting this in Chrome 75.0.3770.100, somehow it has become 'a-rate'. Confusing, but looks like you're following the spec https://webaudio.github.io/web-audio-api/#ref-for-enumdef-automationrate%E2%91%A1

Copy link
Member

Choose a reason for hiding this comment

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

@oshoham seems like this is related to a bug in the polyfill:

In Firefox and Safari (using the ScriptProcessorNode fallback) the smoothing param is being passed to the process method as a 4096 frame Float32 buffer, rather than as a number. This is causing NaN values when we attempt to smooth.

I think the best way around this for now is to make Smoothing one of the processorOptions that updates on a message, like toggleNormalize does. But what do you think?

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, I see the bug, it looks like the polyfill is assuming that all worklet parameters are a-rate: https://github.com/GoogleChromeLabs/audioworklet-polyfill/blob/d6f1a14dacc597876b1a5f9cd8719729a131b585/src/index.js#L96

I think your suggested approach makes sense - another possible solution is to treat the smoothing parameter as a-rate (i.e. a Float32 buffer of values) and update the processor code accordingly. This seems more in line with the intended use case of AudioParams, although probably less efficient for non-AudioWorklet browsers. What's your opinion?

@oshoham oshoham merged commit 7d61e7e into processing:master Jul 16, 2019
@oshoham oshoham deleted the audioworklet-soundfile-amplitude branch July 16, 2019 22:15
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.

@oshoham while reviewing #376, I found a bug introduced in this PR with amplitude getLevel when falling back to the ScriptProcessorNode. That's an important method for the library, and I'm sorry I missed this in my previous review!

defaultValue: 0,
minValue: 0,
maxValue: 1,
automationRate: 'k-rate'
Copy link
Member

Choose a reason for hiding this comment

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

@oshoham seems like this is related to a bug in the polyfill:

In Firefox and Safari (using the ScriptProcessorNode fallback) the smoothing param is being passed to the process method as a 4096 frame Float32 buffer, rather than as a number. This is causing NaN values when we attempt to smooth.

I think the best way around this for now is to make Smoothing one of the processorOptions that updates on a message, like toggleNormalize does. But what do you think?

@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