Reuse SoundFile's worklet node to prevent distortion

[davepagurek]

Resolves https://github.com/processing/p5.js-sound/issues/647

Possibly also addresses https://github.com/processing/p5.js-sound/issues/494, https://github.com/processing/p5.js-sound/issues/635, and https://github.com/processing/p5.js-sound/issues/100 but will need confirmation.

Users reported that old versions of p5.sound don't have the same glitchy behaviour as reported in #647. Looking at the diff between v0.3.5 and v1.0.1, I found I could get the glitchiness to go away by not creating new worklet nodes, a change introduced in #373 due to ScriptProcessorNode getting deprecated in the Web Audio API. While AudioWorkletNode is the supposed replacement, it seems that disposal of it doesn't work quite the same as before (maybe something to do with the messaging port holding a reference? although even disposing of that doesn't seem to fix this for everyone.)

All that to say: reusing the sound file's worklet node when you play it seems to reduce the glitchiness in the output. I haven't personally noticed any negative side effects due to this, but it means that there may still be some buffered data left over from the last play, so everyone keep a heads up for bugs related to that.

Live version here: https://editor.p5js.org/davepagurek/sketches/yyOsE4doe (In index.html, try uncommenting the reference to p5.sound.js from the CDN to see the old behaviour.)

[davepagurek]

Hey @Abhijay007, if you have a sec would you mind giving this a review? A second pair of eyes on this would be nice 🙂

[Abhijay007]

Sure

…

On Mon, 16 Jan 2023, 06:09 Dave Pagurek, ***@***.***> wrote: Hey @Abhijay007 <https://github.com/Abhijay007>, if you have a sec would you mind giving this a review? A second pair of eyes on this would be nice 🙂 — Reply to this email directly, view it on GitHub <#728 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/APLHP3VRQTZ4WL4C5HBTDV3WSSKDXANCNFSM6AAAAAAT4FEPTQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[Abhijay007]

@davepagurek code LGTM too but can you re-check the Live version shared by you here: https://editor.p5js.org/davepagurek/sketches/yyOsE4doe , it is working fine for the old Behaviour but when we run the same example by commenting the existing CDN (new one maybe) = <script src="https://cdnjs.cloudflare.com/ajax/libs/p5.js/1.5.0/p5.js"></script> it drops an error, saying "TypeError: Cannot read properties of undefined (reading '_report')" but I think as it has the old CDN it should work with that too?

Edit: ya I got it, you mentioned that is for the "reference to p5.sound.js"

And as per my understanding, This code creates an AudioWorkletNode and connects it to a silent node. If the worklet node already exists, it reuses it rather than creating a new one. The worklet node has an event listener that listens for a "position" message and updates the last position accordingly.

And in the previous approach, we might be using the Singleton pattern, that way the worklet node is only created once and can be accessed by multiple components without creating new instances. maybe we were keeping the track of the state of the workletNode and only creating a new one if it has been destroyed. I am still new to the codebase 😅 , I am just exploring how it is working for now, I will comment better once I understand it by running some components of soundfile using the same, by looking at the example you shared and the code you wrote it LGTM, I will once again go through the issues you mentioned/attached with this PR and share my views regarding the same and maybe look for what could be the better approach/solution to cover all those issues, till then I think we can move forward and merge this PR :)

[davepagurek]

Haha honestly I'm still not 100% sure I understand it either! In the old approach, we seemed to be doing everything we could to destroy the previous node when we make a new one, or at least we were removing all references to it so that it could be garbage collected. It just seems like, for whatever reason, it wasn't fully getting garbage collected, so when it happens enough times, memory piles up and the browser starts to stutter. I guess the current code still suffers from this same problem when we dispose of a SoundFile, but at least it no longer leaks memory when calling play(), which happens a lot more often.

Thanks for taking the time to look into this for me! I'm going to merge this for now, and we can keep an eye out for any new playback bugs that might be related.

[davepagurek]

oh apparently I need approving review in order to merge, would you mind doing that?

[davepagurek]

Ah, apparently I need an approval from a maintainer, and I can't approve my own PR 😅