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

Array of effects import causing mem leak? #1118

Closed cryptozachary closed 1 year ago

cryptozachary commented 1 year ago

Hello All,

I appreciate you taking the time to read this.

At this point in the development of this app , my chrome browser keeps giving me an aww snap , out of memory error. After about 3 weeks troubleshooting I found when I remove an import that contains an array of effects and any state referencing those effects , I no longer have this issue. In my post recent update , you'll see the changes from when I had no mem leak , to when the mem leak returned.

https://github.com/cryptozachary/holla/commit/a284b00c703300909d7535cecae6a74d1dc94dd4

display.js - meat of the app , issues are located here effects - effects menu effects2.js - effects classes exported as array header.js - contains effectToggle state that is updated when particular effect is toggled

I cant seem to figure out how to solve this issue. I have a cleanup function on the sampler instance that's referencing the effects. But to no avail. Any insight would be extremely helpful !

Thanks,

Zachary

marcelblum commented 1 year ago

What platform/device are you getting the error on? One thing that stood out to me there is new Reverb(9000). Assuming that's an instance of standard Tone.Reverb, note that the argument is in seconds not milliseconds. So you're creating a reverb with a decay of 2.5 hours long! Tone.Reverb uses convolution reverb which means it's creating a synthesized impulse response buffer the length of the decay argument. So that means new Reverb(9000) is generating 2.5 hours of audio in memory (>3GB of audio data), likely enough to single handedly aww snap you right there.

cryptozachary commented 1 year ago

What platform/device are you getting the error on? One thing that stood out to me there is new Reverb(9000). Assuming that's an instance of standard Tone.Reverb, note that the argument is in seconds not milliseconds. So you're creating a reverb with a decay of 2.5 hours long! Tone.Reverb uses convolution reverb which means it's creating a synthesized impulse response buffer the length of the decay argument. So that means new Reverb(9000) is generating 2.5 hours of audio in memory (>3GB of audio data), likely enough to single handedly aww snap you right there.

I'm getting the error on my windows pc chrome browser. I have not tried on any other browsers or devices. I'll try on edge and see what happens as well and update.

Yikes , good find!! I will lower that reverb and mess around with the app and see if it gives me the aww snap. Typically it does so without the effect even being connected/toggled. Update soon.

cryptozachary commented 1 year ago

On edge , it almost immediately gives me the out of memory error about 7 seconds after the app loads on my local machine. Unfortunately the reverb change did not work. I'm also wondering if the effectsToggle state that I'm calling is causing an infinite loop. I don't think it is , but I'm not 100% sure because there IS a dependency array, but its an object.

// updates the settings toggle to on or off
    function toggleSettings(effectPosition) {
        let updatedCheckedState = effectsToggle.map((effect, index) => {
            if (effect.key == effectPosition) {
                return { ...effect, state: !effect.state }
            }
            return { ...effect }
        });

        setEffectsToggle(prev => {
            return updatedCheckedState
        })

    }
// //calls function to turn on/off effects
    // React.useEffect(() => {

    //     connectEffect()

    // }, [effectsToggle])
// //connect and disconnect effect when effect turned on/off
    // function connectEffect() {

    //     let selected = []

    //     effectsToggle.forEach((effect, index, arr) => {
    //         if (effect.state === true) {
    //             sampler.disconnect()
    //             sampler.connect(effArr[index].toDestination())
    //             selected.push(index)
    //             console.log(selected)
    //         }
    //     })

    //     console.log(selected)

    //     if (selected) {
    //         selected.forEach((effectposition) => {
    //             if (effectsToggle[effectposition].state === false) {
    //                 sampler.disconnect(effArr[effectposition])

    //             }
    //         })

    //     }

    // }
marcelblum commented 1 year ago

How many instances of those effects are you trying to create? Aside from an extreme value for the reverb decay, it would take dozens or even hundreds to crash a desktop. Note that on Windows Chrome you're limited to about 8gb of ram usage. So you've probably got a loop somewhere going longer than intended, unless you're loading tons of audio elsewhere.

cryptozachary commented 1 year ago

Below is the only instance of the effects I create, export as array, and retrieve them in the display.js as a prop. I've searched my code many times for a possible infinite loop but cannot find one.

import { FeedbackDelay, Reverb, StereoWidener, Distortion, BitCrusher, Phaser, Chorus } from 'tone'

export const reverb = new Reverb(900)
export const delay = new FeedbackDelay(0.5, 0.9)
export const stereo = new StereoWidener(1)
export const distortion = new Distortion(0.5)
export const phaser = new Phaser({
    frequency: 15,
    octaves: 5,
    baseFrequency: 1000
})
export const chorus = new Chorus(4, 2.5, 0.5)
export const crusher = new BitCrusher(9)

export const effArr = [reverb, delay, stereo, distortion, phaser, chorus, crusher]

One idea I wanted to try is utilizing a specific property of the effectsToggle object as a dependency instead of the entire object. Problem is, I need to check the entire object to see if any of the state changes ( true/false = on/off for the settings menu)

// //calls function to turn on/off effects
    // React.useEffect(() => {

    //     connectEffect()

    // }, [effectsToggle])

The effectsToggle data exported and setup in the effectsToggle state ( imported in the app.js as EffectsData )

let effectObj =
    [
        {
            name: "reverb",
            state: false,
            key: "0"
        },
        {
            name: "delay",
            state: false,
            key: "1"
        },
        {
            name: "stereo",
            state: false,
            key: "2"
        },
        {
            name: "distortion",
            state: false,
            key: "3"
        },
        {
            name: "phaser",
            state: false,
            key: "4"
        },
        {
            name: "chorus",
            state: false,
            key: "5"
        },
        {
            name: "crusher",
            state: false,
            key: "6"
        }
    ]

export default effectObj

This is how the effectsToggle state is being passed down and imported

import EffectsData from "./components/EffectsData"

function App() {

  // new array to convert object to booleans 
  //const effArr = new Array(EffectsData.length).fill(false)

  // state of effect toggles
  const [effectsToggle, setEffectsToggle] = React.useState(EffectsData)

  //reverb decay state
  const [verbDecay, setVerbDecay] = React.useState(9000)

  return (
    <div className="main-app-body">
      <Header effectsToggle={effectsToggle} setEffectsToggle={setEffectsToggle} />
      <div className="app">
        <Display
          effectsToggle={effectsToggle}
          setEffectsToggle={setEffectsToggle}
          verbDecay={verbDecay}
        />
      </div>
    </div>
  )
}

export default App;
marcelblum commented 1 year ago

But wait, your code above still has an extreme value for the reverb new Reverb(900). 900 seconds is still an absurdly huge decay time and will use a ton of memory. You probably want a value of 5 or 10 seconds.

cryptozachary commented 1 year ago

I changed the reverb decay to 5 seconds , and I no longer have the out of memory error! Thank you so much for your time and help Marcel.