Bugfixes for p5.Amplitude and p5.Soundfile for browsers without AudioWorklet support #380
Conversation
There was a problem hiding this comment.
Nice fixes, @oshoham ! I found one remaining small issue. I added a log of soundFile.currentTime() to examples/pause_soundfile to test it manually. Of course, it would be nice if the test were automated!
| process(inputs) { | ||
| const input = inputs[0]; | ||
| const inputChannel = input[0]; | ||
| const position = inputChannel[inputChannel.length - 1] || 0; | ||
|
|
||
| this.port.postMessage({ name: 'position', position: position }); | ||
| if (!this.paused) { | ||
| this.port.postMessage({ name: 'position', position: position }); |
There was a problem hiding this comment.
it seems we can't rely on this.paused having updated in Safari. Maybe this logic needs to be in the SoundFile?
There was a problem hiding this comment.
sure, it's easy enough to move this logic to the SoundFile class
src/soundfile.js
Outdated
| @@ -539,6 +539,8 @@ define(function (require) { | |||
| var pTime = time + now; | |||
|
|
|||
| if (this.isPlaying() && this.buffer && this.bufferSourceNode) { | |||
| this._workletNode.port.postMessage({ name: 'pause' }); | |||
There was a problem hiding this comment.
in Safari, this message doesn't always update the worker's paused attribute before it can post a position message of 0. As a result, SoundFile._lastPos gets set to 0 and so SoundFile.currentTime() returns 0 while the soundfile is paused. It's inconsistent, happens maybe half the time, and only in Safari.
It resumes playback from the proper position because pauseTime gets set below. Maybe we also/instead need to set a paused attribute on the SoundFile rather than the Processor?
There was a problem hiding this comment.
The solution I arrived at was to remove the pause logic from the processor and add a guard statement in the worklet node's port callback that returns early if the SoundFile is paused or the reported playback position is 0, which handles the race condition that you encountered in Safari.
Because the processor always looks for the last position value in the current frame of the counter buffer, the playback position will only ever be 0 if the SoundFile is paused (i.e., on the 1st frame, the playback position will be 128/256/whatever the buffer size of the worklet processor is).
There was a problem hiding this comment.
There was a problem hiding this comment.
the playback position will only ever be 0 if the SoundFile is paused
I'm not sure if this is the case in Firefox, where if you pause and un-pause rapidly, the soundfile will stutter at the same position (including position 0), rather than advancing one buffer frame at a time. That's a tiny bug but probably not a blocker on this PR.
| @@ -539,6 +539,8 @@ define(function (require) { | |||
| var pTime = time + now; | |||
|
|
|||
| if (this.isPlaying() && this.buffer && this.bufferSourceNode) { | |||
| this._workletNode.port.postMessage({ name: 'pause' }); | |||
|
|
|||
| this.pauseTime = this.currentTime(); | |||
| this.bufferSourceNode.stop(pTime); | |||
| this._counterNode.stop(pTime); | |||
There was a problem hiding this comment.
one issue we should be aware of is that a pause can be scheduled in the future...perhaps a reason to make pause an a-rate param so that we could setValueAtTime?
There was a problem hiding this comment.
if we move the pause logic out of the worklet processor and into the SoundFile, is that still an issue?
There was a problem hiding this comment.
Yes, unfortunately that wouldn’t solve the issue. It was an issue before this pr. Just thinking whether keeping pause logic in the processor could help fix it.
There was a problem hiding this comment.
I tried making pause an a-rate AudioParam that would get converted to a boolean in the worklet processor, but I had trouble getting that approach to work properly, even in Chrome. For some reason, setValueAtTime wasn't changing the value of the AudioParam.
One way to handle this problem with the approach that I settled on (see the comment thread above) could be to modify this guard statement to only check event.position.data === 0 rather than this._paused || event.position.data === 0.
event.data.position should only equal 0 when the counter buffer has actually stopped playing, which would be compatible with scheduling a pause in the future. I'm not 100% sure this would work, but it's my best guess...
There was a problem hiding this comment.
nice solution! At some point, I'd be curious to experiment more with setValueAtTime on worklet AudioParams, but this works for now.
Either way, it seems like we only need to check event.position.data === 0 in the guard statement?
There was a problem hiding this comment.
I believe so, yes. I changed the guard statement to only check event.position.data === 0 and it seems to work on Chrome, Firefox, and Safari.
examples/pause_soundfile/sketch.js
Outdated
| createP('Press any key to pause. Resume when the key is released') | ||
| } | ||
|
|
||
| function touchStarted() { | ||
| if (getAudioContext().state !== 'running') { | ||
| getAudioContext().resume(); |
There was a problem hiding this comment.
We have a userStartAudio method that can be useful for this, too https://p5js.org/reference/#/p5.sound/userStartAudio
There was a problem hiding this comment.
I've updated the sketch to use userStartAudio instead.
…pause position logic out of AudioWorklet processor
…sume() on user interaction before any other audio code
oshoham
force-pushed
the
amplitude-getlevel-bugfix
branch
from
9c5f0ef to
b32e460
Compare
As pointed out by @therewasaguy, there is a bug in the AudioWorklet processor for p5.Amplitude in Firefox and Safari, in which the
smoothingAudioParam is being passed to the processor as a Float32 array by the AudioWorklet polyfill, rather than as a single value. This PR fixes that issue by setting thesmoothingparameter via the worklet's message port instead of using an AudioParam.This PR also fixes an issue with pausing p5.SoundFiles, in which the SoundFile AudioWorklet processor was incorrectly setting the SoundFile's playback position to 0 on pause.