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

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

Open
milianmori opened this issue Oct 3, 2022 · 5 comments

Comments

@milianmori
Copy link

milianmori commented Oct 3, 2022

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

  • I took a lot of Heap Snapshots. If the Tone.BitCrusher() in the code you can filter for the "BitCrusher" in the snapshot. You'll see a hanging node there.
  • I also checked if the Tone.BitCrusher() is hanging in the DOM by filtering "detached" in the memory snapshot but with no results.
  • Also there I took a lot of performance snapshots... nothing unusual there.
  • I worked with the Web Audio Dev Tools Debug Extension.
  • I tried many different variations in the code like disposing the whole Tone Context...
  • Of course I could just not use Tone.BitCrusher() but like I mentioned above, it also appears to have a leak when using the same code in Safari but with a much more fundamental object I can't work without: Tone.BiquadFilter().
  • I went through many guides to fix memory leaks like this one: https://developer.chrome.com/docs/devtools/memory-problems/

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)

@milianmori milianmori changed the title Garbage Collector cannot dispose ToneAudioBuffer if the audio is generated in Tone.Offline() using Tone.BitCrusher Garbage Collector cannot dispose ToneAudioBuffer if the audio is generated in Tone.Offline() using Tone.BitCrusher() Oct 5, 2022
@milianmori milianmori changed the title Garbage Collector cannot dispose ToneAudioBuffer if the audio is generated in Tone.Offline() using Tone.BitCrusher() Garbage Collector cannot dispose ToneAudioBuffer if the audio is generated in 'Tone.Offline()' using Tone.BitCrusher() Oct 9, 2022
@milianmori milianmori changed the title Garbage Collector cannot dispose ToneAudioBuffer if the audio is generated in 'Tone.Offline()' using Tone.BitCrusher() Garbage Collector cannot dispose ToneAudioBuffer if the audio is generated in Tone.Offline() using Tone.BitCrusher() Oct 9, 2022
@milianmori milianmori changed the title Garbage Collector cannot dispose ToneAudioBuffer if the audio is generated in Tone.Offline() using Tone.BitCrusher() Garbage Collector cannot dispose ToneAudioBuffer if the audio is generated in Tone.Offline() using Tone.BitCrusher() Oct 9, 2022
@milianmori milianmori changed the title Garbage Collector cannot dispose ToneAudioBuffer if the audio is generated in Tone.Offline() using Tone.BitCrusher() Garbage Collector cannot dispose ToneAudioBuffer if the audio is generated in Tone.Offline() using Tone.BitCrusher() or Tone.BiquadFilter() Oct 9, 2022
@marcelblum
Copy link
Contributor

marcelblum commented Oct 22, 2022

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:

  • I can confirm the issue and was able to reproduce it on multiple machines. I made a more minimal demo here which tries everything I could think of to release all references to the buffer and context and trigger GC of the buffer: https://codepen.io/keymapper/pen/ZEoPBjm
  • Behavior varies across browsers:
    • On Chrome (Win & macOS), the buffer is properly GCed as long as the offline context uses no AudioWorklet-based effects. So when using Gain, BiquadFilter, Chebyshev, etc., GC occurs normally. But when using any worklet-based effect such as BitCrusher or JCReverb, the memory used by the resulting buffer doesn't clear, even after using the Chrome devtools "collect garbage" button. Sometimes after a very long time (10+ minutes) it is eventually cleared. Overall Chrome behavior is very consistent: the use of AudioWorklet in a Tone offline context prevents proper GC.
    • On Firefox (Win & macOS), it works as it should: the buffer is consistently GCed almost immediately after disposing regardless of what nodes/effects are used in the offline context.
    • On Safari/macOS, results are inconsistent. Sometimes it GCs but sometimes it does not clear, even after navigating to a different page. Type of effect used (worklet/non-worklet) has no bearing on the result. Seems like any buffer returned by Tone.Offline() has a good chance of not getting properly GCed in Safari.
  • Since we were seeing such different results across browsers, I suspected perhaps browser-specific code in standardized-audio-context might be to blame, but I also tested with native offline contexts and got the same results. (Note that there is a Tone bug preventing use of worklets in native contexts, see allow worklet-based effects to be used with native contexts #1131. My tests worked around the bug with a kludge reassigning a Tone internal function, see the commented out line 6 in my codepen).

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Author

milianmori commented Dec 21, 2022

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
Copy link
Contributor

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.

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

No branches or pull requests

3 participants