Tonejs / Tone.js

A Web Audio framework for making interactive music in the browser.
https://tonejs.github.io
MIT License
13.37k stars 976 forks source link

Garbage Collector cannot dispose `ToneAudioBuffer` if the audio is generated in `Tone.Offline()` using `Tone.BitCrusher()` or `Tone.BiquadFilter()` #1128

Open milianmori opened 1 year ago

milianmori commented 1 year ago

Hello everyone...

I just wanted the developers know that I may have discovered a bug: Its very nice to pre-render audio in Tone.Offline() for later use. BUT: If you use Tone.BitCrusher() inside Tone.Offline() and you want to get rid of the ToneAudioBuffer (generated in Tone.Offline()) via the dispose() method you will end up with a memory leak. Try it out, its very easy to reproduce...

To Reproduce Here you find a codepen to reproduce the bug.

  1. open the Codpen
  2. open Chrome Taskmanager to monitor the "memory footprint".
  3. press "1. generate Buffers" button
  4. wait until the memory footprint of " subframe cnpn.io" dosen't increase anymore, it'll take around 10s to generate 850 MB
  5. press "2. dispose Buffers" button
  6. trigger the garbage collector via dev tools --> memory --> trash bin button

Even though you disposed the buffer and triggered the garbage collector, it wont be able to collect and delete the buffer from the memory. It's still these 850 MB! To make the garbage to do it's job you have to change the following:

  1. comment out line 7
  2. change line 8 to this: const noise = new Tone.Noise("pink").start().toDestination();
  3. comment out line 9

If you repeat the above steps now with the changes you made, the GC will delete the disposed buffer! So this means Tone.BitCrusher() sets a reference but I can't find out how where?! HEEEELLLP! :)

Additional context UPDATE 1: I was able to discover that on Safari you have the same issue but additional to BitCrusher(), it also creates a memory leak when using Tone.BiquadFilter(). Maybe there is even more effects which prevent the garbage collector to clear memory...

UPDATE 2: I just tried the alternative way and used Tone.OfflineContext() instead of Tone.Offline(). It also dosen't work!

What I've tried

Expected behavior Release of the memory from the buffer I want to dispose. @tambien maybe you can help me here?

Greetings, M

using: Chrome: Version 105.0.5195.125 (Official Build) (arm64) on macOS 12.4 on MacBook Pro (16-inch, 2021) M1 Max, Safari: Version 15.5 (17613.2.7.1.8)

marcelblum commented 1 year ago

Wanted to post some findings here following some offsite discussions I had with @milianmori. Caveat being that JS engine GC has always been a bit of an enigma with a strong Heisenberg effect distorting tests like this. That said, between the two of us and a few different types of test environments here are our findings:

My conclusion is that there is most likely a Chrome internal bug with AudioWorklets in offline contexts; this should be repro'd in vanilla web audio api code to verify and if so submitted to crbug. In Chrome there is direct evidence of some kind of memory hole because it clearly does not consider the buffer to be garbage when clicking "collect garbage". Meanwhile Safari be how Safari be, it's long had a stunted implementation of web audio. But perhaps someone else reading has some better idea of whether there's a possibility that Tone.js itself could possibly cause any of this? @chrisguttandin I know you have much experience with x-browser quirks in web audio, have you ever seen similar issues with GC and offline contexts, and in particular AudioWorklets in offline contexts?

chrisguttandin commented 1 year ago

Hi @marcelblum, hi @milianmori,

as already mentioned above I think it would be good to somehow isolate the different layers used by Tone.js to discover where the bug is. One of the features of standardized-audio-context is that it tries really hard to not add any workaround to a browser which doesn't need it. But in this case it means standardized-audio-context is actually doing different things in each browser which makes the test matrix more complicated. :-(

It would be good to know if the problem in Chrome persists when using standardized-audio-context alone without using Tone.js. Or if it even persists with the native Web Audio implementation.

There was a bug in Chrome which caused the OfflineAudioContext to never release the memory. It's fixed in Chrome but Safari's implementation is dating back to the days when Chrome and Safari both used WebKit. That means there is a good chance that Safari is suffering from the same bug. Unfortunately I don't know of any reliable way to measure memory in Safari. From my experience the dev tools can't be trusted at all and sometimes cause issues which wouldn't be present otherwise.

In case Safari suffers from the issue that's already fixed in Chrome it could possibly be mitigated by the same hack mentioned by Andrew in this comment.

Another idea would be to run each offline computation in an iframe which gets created just for that reason and gets destroyed right after.

In short, I think there are ways to "fix" this but I think it would be good to know where the problem actually is at first.

marcelblum commented 1 year ago

Thanks @chrisguttandin for the tips and leads. I didn't realize that there might still be shared web audio source code between Safari and Chrome dating from before Chromium forked WebKit.

milianmori commented 1 year ago

Hello everyone.

I reported the bug in Safari to Apple and got an answer. I just want to give you a quick insight:

This is what I sent:

FB11766600

13.12.22, 20:51:32 CET

Safari never releases buffer memory when using OfflineAudioContext FB11766600 — macOS

Basic Information

Please provide a descriptive title for your feedback: Safari never releases buffer memory when using OfflineAudioContext

Which area are you seeing an issue with? Safari

What type of issue are you reporting? Incorrect/Unexpected Behavior

Details

What does the Safari issue you are seeing involve? Audio & Video

Please provide the URL to one or more websites where you are seeing this problem: JavaScript

What extensions or content blockers do you have enabled? Example: AdBlock, 1Blocker none

What time was it when this last occurred? (Example: 12:00 pm EST 02/14/2018) 08/11/2022

Description

Please describe the issue and what steps we can take to reproduce it: REPRODUCTION: This is a very very basic bug and super easy to reproduce. Safari never releases the buffer memory generated with OfflineAudioContext. If you run the following JS code and open it on a localhost in Safari, you'll end up with a memory leak. To reproduce the bug, it's super easy! You can also open the same files in the attachment.

  1. create a simple HTML file and add a button called "generate"
  2. copy and paste the following code as a JS file in the same folder as your HTML file
  3. implement/ reference the JS file in your code.
  4. run the code anywhere you want, I run it with simple localhost server
  5. press the button "generate"

document.getElementById('generate').addEventListener('click', () => { var ctx = new OfflineAudioContext(1, 44100 * 600, 44100) var osc = ctx.createOscillator(); osc.connect(ctx.destination); osc.start();

ctx.startRendering().then(buffer => {
  osc = null
  buffer = null
  ctx = null
})

});

ADDTIONAL INFO: There was a bug in Chrome (https://bugs.chromium.org/p/chromium/issues/detail?id=894322#c15) which caused the OfflineAudioContext to never release the memory. It's fixed in Chrome but Safari's implementation is dating back to the days when Chrome and Safari both used WebKit. That means there is a good chance that Safari is suffering from the same bug.

What results you expected: The garbage collector of Safari should clear the memory of the buffer (generated by OfflineAudioContext) which is being disposed.

What results you actually saw: memory leak, buffer never will be cleared off the memory.

This is what came back:

Hello Milian,

Thank you for filing this feedback report. We reviewed your report and determined the behavior you experienced is currently functioning as intended.

We added logging in our constructors / destructors and run the test case:

Constructed all 3 objects as expected: CHRIS: 0x14900b640 - DestinationNode() CHRIS: 0x14900b310 - OfflineAudioContext() CHRIS: 0x14900b730 - OscillatorNode()

Then we sent a memory pressure signal to trigger garbage collection: CHRIS: 0x14900b730 - ~OscillatorNode() CHRIS: 0x14900b310 - ~OfflineAudioContext() CHRIS: 0x14900b640 - ~DestinationNode()

All 3 objects got destroyed. We don’t see anything wrong here. It is expected that these object stick around until the next JavaScript garbage collection.

It is also incorrect to say that WebKit’s WebAudio implementation dates back from the time before Chrome had forked. We did a lot of work on WebAudio after the fork to ship the unprefixed version.

Finally, we looked at the Chrome bug and their fix. We saw that our code already looks like their fixed code so we couldn’t even port Chrome’s patch.

You can close this feedback by selecting "Close Feedback" via the Actions button found above. This Feedback will no longer be monitored, and incoming messages will not be reviewed. We appreciate your feedback.

marcelblum commented 1 year ago

we sent a memory pressure signal to trigger garbage collection

Interesting, I wonder how to trigger that in real world JavaScript. I guess this means Safari doesn't do GC periodically, rather only when under "memory pressure"? Jives with what I observed that even when navigating to a new page that often wasn't triggering GC.