ensure webaudio is removed from ram/cpu usage #2164

entonbiba · 2021-01-23T03:26:36Z

fixes #1940

coveralls · 2021-01-23T03:37:36Z

Coverage increased (+0.01%) to 82.454% when pulling 30526d4 on entonbiba-patch-webaudio-usage-fix into 658bd92 on master.

entonbiba · 2021-01-23T03:40:09Z

@katspaugh @thijstriemstra backend = null will happen as long as backend is available

if (this.backend) {
  this.backend.destroy();
  this.backend = null;
}

sundayz · 2021-01-23T08:02:51Z

Your comment on #1940 says we should set this.backend to null but this commit sets this.backend.buffer to null, so which is it :)

Your research into the issue is great. if we know loading new audio allows the memory to be garbage collected, we can follow that code path and see what it is doing that we are not doing in the destroy() function.

marizuccara · 2021-01-23T12:06:02Z

Hi, also the audio buffer is already set to null in the WebAudio backend (which is the only backend that uses the AudioBuffer).

I hope that setting the backend to null will resolve all your problems! :)

sundayz · 2021-01-23T12:11:05Z

Here is something interesting I found that may be a piece of the puzzle...

WebAudio createSource sets a reference of the buffer on the source.

wavesurfer.js/src/webaudio.js

Line 580 in 658bd92

this.source.buffer = this.buffer;

WebAudio disconnects the source, but never sets it to null.

wavesurfer.js/src/webaudio.js

Line 509 in 658bd92

this.source.disconnect();

So if when we call destroy() a reference to the backend still exists, then a reference to the buffer will still also exist. Might be part of the problem. And could explain why calling load() again as entonbiba has shown frees the memory, because a new source is set and the old one is collected. I haven't tried setting source to null yet, and it may not be the only case where this happens.

marizuccara · 2021-01-23T14:06:32Z

I found this interesting response on stackoverflow:
https://stackoverflow.com/questions/55207924/does-the-web-audio-api-clear-out-a-source-node-after-it-finishes-playing

On destroy we can call also stop() on the source node, if it's playing (in WebAudio). On onendend event, we can disconnect it, sets to null the buffer, and then the node, as you suggest

entonbiba · 2021-01-23T15:06:29Z

@sundayz lol yes backend = null is correct, updated

thijstriemstra

thanks. can you also expand the existing test for destroy and add an assertion that backend is null. Also add a changelog entry please (with a reference to the ticket this pr is closing).

CHANGES.md

entonbiba · 2021-01-24T19:26:19Z

@thijstriemstra added null check,

@sundayz yes the observation about the load() is correct as its replacing backend with new. In the destroy case it would become null for this to work since even if we did .close or .disconnect it would still keep it as suspended taking up memory usage in webaudio.

thijstriemstra · 2021-01-24T19:37:26Z

@entonbiba I meant more somewhere along these lines: https://github.com/katspaugh/wavesurfer.js/blob/entonbiba-patch-webaudio-usage-fix/spec/wavesurfer.spec.js#L351

entonbiba · 2021-01-24T20:29:05Z

@thijstriemstra I'll add expect(wavesurfer.backend).toBeNull(); after the destroy(); like below:

it('destroy', function(done) {
        manualDestroy = true;

        wavesurfer.once('destroy', function() {
            TestHelpers.removeElement(element);
            done();
        });
        wavesurfer.destroy();

        expect(wavesurfer.backend).toBeNull();
    });

src/wavesurfer.js

katspaugh

Great work!

sundayz · 2021-01-25T19:59:51Z

I haven't tried the new branch but in my project when I set backend = null, a little bit more memory gets collected but the 50MB arraybuffer still remains there.

thijstriemstra · 2021-01-25T20:15:31Z

In that case, re-open ticket closed by this pr: #1940

entonbiba · 2021-01-26T00:06:28Z

@sundayz do you have the code you are using the create the instances ?
Just tried it again with backend = null and the results are working:

create
load
backend = null

sundayz · 2021-01-26T00:49:08Z

The file is 2000 lines long and it uses Vue so we should test in a code pen :D I will try that tomorrow. I just have a feeling that if the issue was as simple as backend = null then it would have been fixed a long time ago.

If your PR fixes the leak for most people then it's still a very good thing. I will try to reproduce the issue without Vue later.

Maria had some interesting insights and it might still be worth to set the source to null after disconnecting for example, just to make sure. But that can be done in a separate PR

entonbiba · 2021-01-26T01:16:02Z

@sundayz if it's 2000 lines of vue then it's likely rendering a whole lot more than the audio file that's taking up memory.

The best test is to the library without any additional things. I just do create load destroy (backend=null) pr and repeat that multiple times with multiple loads at once to see the differences of usage.

ping me when the sample code is available I'll test it as well :)

thijstriemstra · 2021-02-24T20:08:10Z

Unfortunately this PR introduced a new issue with timing issues around backend = null. In unit tests for a wavesurfer related project I started seeing this error:

  TypeError: Cannot read property 'getCurrentTime' of null
      at WaveSurfer.getCurrentTime (node_modules/wavesurfer.js/dist/wavesurfer.js:4199:27)
      at d.value (dist/commons.js:8:71353)
      at d.value (dist/commons.js:8:69669)
      at d.value (dist/commons.js:8:74618)
      at node_modules/wavesurfer.js/dist/wavesurfer.js:3214:12
      at <Jasmine>
      at WaveSurfer.fireEvent (node_modules/wavesurfer.js/dist/wavesurfer.js:3213:28)
      at node_modules/wavesurfer.js/dist/wavesurfer.js:4133:16
      at node_modules/wavesurfer.js/dist/wavesurfer.js:3214:12
      at <Jasmine>

Commenting out the backend = null change from this PR makes the test pass again.

Basically any code in wavesurfer that relies on the backend property, e.g. wavesurfer.getCurrentTime() will now break if destroy() is called.

/**
     * Get the current playback position
     *
     * @example const currentTime = wavesurfer.getCurrentTime();
     * @return {number} Playback position in seconds
     */
    getCurrentTime() {
        return this.backend.getCurrentTime();
    }

This is not necessarily a bad thing, the instance was destroyed afterall, but should wavesurfer fix this (e.g. add a backend !== null check everywhere) or is it up to the user to check for an proper, existing wavesurfer.backend property?

thijstriemstra · 2021-02-24T20:41:59Z

but should wavesurfer fix this (e.g. add a backend !== null check everywhere) or is it up to the user to check for an proper, existing wavesurfer.backend property?

Decided it was a valid issue with my tests and this change brought it to light, so no worries, sorry for the noise.

entonbiba · 2021-03-01T23:36:51Z

sounds good

* ensure webaudio is removed from ram/cpu usage * update changes.md to include ticket katspaugh#1940 * added test for null * remove whitespace

ensure webaudio is removed from ram/cpu usage

000f865

entonbiba added bug webaudio labels Jan 23, 2021

entonbiba requested a review from thijstriemstra January 23, 2021 03:26

Update wavesurfer.js

ed5db9f

entonbiba mentioned this pull request Jan 23, 2021

Every call to wavesurferjs keeps on increasing RAM memory, without releasing it on destroy. #1940

Closed

Update wavesurfer.js

82c673d

thijstriemstra requested review from katspaugh, marizuccara, osheroff and sundayz January 24, 2021 17:52

thijstriemstra suggested changes Jan 24, 2021

View reviewed changes

Update CHANGES.md

c4c5db8

thijstriemstra reviewed Jan 24, 2021

View reviewed changes

CHANGES.md Outdated Show resolved Hide resolved

entonbiba added 2 commits January 24, 2021 14:02

update changes.md to include ticket #1940

5b06f5b

Update wavesurfer.js

a2a41ae

entonbiba and others added 2 commits January 24, 2021 15:29

added test for null

188b731

remove whitespace

6112702

thijstriemstra reviewed Jan 24, 2021

View reviewed changes

src/wavesurfer.js Outdated Show resolved Hide resolved

Update wavesurfer.js

30526d4

thijstriemstra approved these changes Jan 24, 2021

View reviewed changes

entonbiba self-assigned this Jan 24, 2021

entonbiba mentioned this pull request Jan 24, 2021

Playing without audio on iOS 13.3.1 #1864

Closed

katspaugh approved these changes Jan 25, 2021

View reviewed changes

thijstriemstra merged commit d200517 into master Jan 25, 2021

thijstriemstra deleted the entonbiba-patch-webaudio-usage-fix branch January 25, 2021 12:30

entonbiba mentioned this pull request Apr 28, 2021

soundtouch with playlist example generates memory leak. #2246

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ensure webaudio is removed from ram/cpu usage #2164

ensure webaudio is removed from ram/cpu usage #2164

entonbiba commented Jan 23, 2021

coveralls commented Jan 23, 2021 •

edited

Loading

entonbiba commented Jan 23, 2021 •

edited

Loading

sundayz commented Jan 23, 2021

marizuccara commented Jan 23, 2021

sundayz commented Jan 23, 2021 •

edited

Loading

marizuccara commented Jan 23, 2021

entonbiba commented Jan 23, 2021

thijstriemstra left a comment

entonbiba commented Jan 24, 2021

thijstriemstra commented Jan 24, 2021

entonbiba commented Jan 24, 2021

katspaugh left a comment

sundayz commented Jan 25, 2021

thijstriemstra commented Jan 25, 2021 •

edited

Loading

entonbiba commented Jan 26, 2021

sundayz commented Jan 26, 2021 •

edited

Loading

entonbiba commented Jan 26, 2021

thijstriemstra commented Feb 24, 2021 •

edited

Loading

thijstriemstra commented Feb 24, 2021 •

edited

Loading

entonbiba commented Mar 1, 2021

ensure webaudio is removed from ram/cpu usage #2164

ensure webaudio is removed from ram/cpu usage #2164

Conversation

entonbiba commented Jan 23, 2021

coveralls commented Jan 23, 2021 • edited Loading

entonbiba commented Jan 23, 2021 • edited Loading

sundayz commented Jan 23, 2021

marizuccara commented Jan 23, 2021

sundayz commented Jan 23, 2021 • edited Loading

marizuccara commented Jan 23, 2021

entonbiba commented Jan 23, 2021

thijstriemstra left a comment

Choose a reason for hiding this comment

entonbiba commented Jan 24, 2021

thijstriemstra commented Jan 24, 2021

entonbiba commented Jan 24, 2021

katspaugh left a comment

Choose a reason for hiding this comment

sundayz commented Jan 25, 2021

thijstriemstra commented Jan 25, 2021 • edited Loading

entonbiba commented Jan 26, 2021

sundayz commented Jan 26, 2021 • edited Loading

entonbiba commented Jan 26, 2021

thijstriemstra commented Feb 24, 2021 • edited Loading

thijstriemstra commented Feb 24, 2021 • edited Loading

entonbiba commented Mar 1, 2021

coveralls commented Jan 23, 2021 •

edited

Loading

entonbiba commented Jan 23, 2021 •

edited

Loading

sundayz commented Jan 23, 2021 •

edited

Loading

thijstriemstra commented Jan 25, 2021 •

edited

Loading

sundayz commented Jan 26, 2021 •

edited

Loading

thijstriemstra commented Feb 24, 2021 •

edited

Loading

thijstriemstra commented Feb 24, 2021 •

edited

Loading