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

Wavesurfer causing page renderer to crash (too many DOM nodes) #3696

Closed
ndeshpande2022 opened this issue May 11, 2024 · 35 comments · Fixed by #3700 or #3712
Closed

Wavesurfer causing page renderer to crash (too many DOM nodes) #3696

ndeshpande2022 opened this issue May 11, 2024 · 35 comments · Fixed by #3700 or #3712
Labels

Comments

@ndeshpande2022
Copy link

Bug description

When importing longer media files (> 3 hours) with peak data, Wavesurfer creates too many canvas elements at one time on the page. This causes the renderer to crash and the page to become unresponsive.

Environment

  • Browser: Chrome
  • Electron: Chromium

Minimal code snippet

I'm using the pre-decoded example in my project with a longer file. https://wavesurfer.xyz/examples/?predecoded.js
I've created the peaks using AudioWaveform from the BBC, and everything works great except for the number of Canvas elements being added to the DOM.

Expected result

I think Wavesurfer should implement a "virtual list" so that only the canvas in view gets added to the DOM.

Obtained result

Browser window doesn't crash.

Screenshots

image

@katspaugh
Copy link
Owner

Hey, thanks for the bug report and your donation! 💖

A virtualized list is a good idea. I’ll see if I can look into it.

@ndeshpande2022
Copy link
Author

ndeshpande2022 commented May 11, 2024 via email

@katspaugh
Copy link
Owner

I've just published wavesurfer.js@7.8.0-beta with the virtualization, please let me know if it works for you. 😄

@ndeshpande2022
Copy link
Author

This is great! Thanks so much for doing this so quickly. I'm wondering how I check out 7.8.0-beta. I'm not seeing the branch anywhere in the repo.

Thanks again @katspaugh

@katspaugh
Copy link
Owner

Oh it’s an npm version, npm install wavesurfer.js@7.8.0-beta.

@ndeshpande2022
Copy link
Author

Hi @katspaugh,

Loading times of v7.8.0-beta are much better so I believe we're on the right track. However, as the video plays, new canvas elements are created, without removing the canvas elements that are no longer in view. This eventually leads to the window render process crashing (screenshot below).

The other note I had was that this work may need to be applied to all plugins. We're currently using the region plugin, and timeline plugin. With the Region plugin we've written our own logic to create and destroy regions that are in view.

I don't expect this to work to be simple, but I wanted to provide the feedback in case it's possible to apply. Please let me know if you need any additional information. I've attached the peak data that we are currently using for testing. The file is 04:05:19:12.
peaks.json

image

@katspaugh katspaugh reopened this May 13, 2024
@katspaugh
Copy link
Owner

Thank you for testing and another donation! 🙏

There's currently a limit of 100 canvases that it will create before clearing the old ones and starting over again. Looks like even that might be too many, so I'll cap it at 10.

@ndeshpande2022
Copy link
Author

Hi @katspaugh,

I think I may be providing bad data. I tried to load a smaller video with the same peak data file (by accident), and it appears that Wavesurfer is still interrogating the video for audio data. The peak data was generated properly, even though I provided my own peak data. I'm wondering if the renderer process is crashing because Wavesurfer is still looking at the video file I have loaded.

@katspaugh
Copy link
Owner

Hmm, can you paste the code snippet you're using?

To clarify, wavesurfer will still load the audio for playback as it uses peaks only for rendering. So it's normal to see "media" requests even with peaks provided.

Screenshot 2024-05-13 at 16 29 28
@ndeshpande2022
Copy link
Author

Hi @katspaugh,

I think this may be user/dev error on my part, but I would appreciate any feedback you have.

We create the Wavesurfer instance when we mount our component:

let ws = WaveSurfer.create({
            container: "#Timeline",
            waveColor: "#0fbc8c",
            progressColor: "#0e6049",
            cursorColor: "#FFF700",
            height: timelineHeight, //calculated above
            minPxPerSec: 100,
            dragToSeek: false,
            hideScrollbar: true,
            autoCenter: true,
            autoScroll: true,
            normalize: false,
            interact: true,
            barWidth: 2,
            barGap: 1,
            barRadius: 2
});

We then load our video and peak data after the user fills in some form data:

ws.load(mediaPath, peakFile.data);

If I provide peak data in the initial Create function, then everything seems to work great, and I'm not seeing any crashes. Does this help explain anything?

image

ps. High number of DOM nodes is related to the Timeline component in the screenshot above and not the Canvas elements ^

@katspaugh
Copy link
Owner

I'll need to test it with a separate load call. It might be a bug as it should behave identically to passing a URL and peaks to the create function.

Regarding the Timeline plugin – makes sense. It creates a div for each notch on the timeline, so it can indeed create too many. I'll be able to look into it on the weekend.

Btw, unrelated but you can also pass a duration to make it render a little bit faster. You can calculate the duration from your data according to this formula:

const duration = peakFile.length * peakFile.samples_per_pixel / peakFile.sample_rate

Then pass it like so:

ws.load(mediaPath, peakFile.data, duration);
@ndeshpande2022
Copy link
Author

Thanks so much - that's very helpful. I will add that to our code.

Thanks again -

@ndeshpande2022
Copy link
Author

Hi @katspaugh,

I just wanted to follow up. We now create the Wavesurfer instance after the user fills in the form data and things seem to be working much better (so I'm guessing there is a bug when loading the peak data in a separate load function).

My wish-list moving forward is:

  1. Add the same "virtual list" logic to the Region and Timeline Plugins.
  2. Make the number of Canvas elements an option when creating the Wavesurfer instance. This would allow us to optimize for performance.

Thanks again -

@katspaugh
Copy link
Owner

The load bug is fixed now, and I published a new beta version on NPM wavesurfer.js@7.8.0-beta.1.

The number of canvases is now capped at 10, and each canvas is double the size of the container, so it will remove all the canvases if you scroll past 20 full lengths of the visible container. Making this parameter a public option, unfortunately, goes against what we typically expose as wavesurfer options – options must alter the behavior or appearance of the waveform but not optimization parameters.

I will now look into virtualizing Timeline and Region elements.

@katspaugh
Copy link
Owner

Regions and Timeline are now also "virtualized". Published wavesurfer.js@7.8.0-beta.2.
Please let me know if you encounter any issues!

@larsrgit
Copy link

Great change! Huge performance improvements with long audio files.
However: Is it possible that zooming is not handled correctly? Using the beta, when zooming in, sometimes parts of the waveform are not rendered.

@katspaugh
Copy link
Owner

I thought I tested zooming and scrolling but might've missed some edge case. Could you record a screencast with this behavior?

@larsrgit
Copy link

Sure. As you can see in the video, at some zoom levels, not the full view is rendered, only by moving the view (in the video by clicking inside the not rendered part and autoscroll moving the view) the missing part is rendered.

This might be an old issue, however until this change zooming in close zoom-levels with long audios was really slow (as it always startet to render from the beginning of the audio), so I never extensively used it.

zoom-render-bug.mp4
@katspaugh
Copy link
Owner

Gotcha, thank you. Looks like a regression bug. I'll fix it.

@katspaugh
Copy link
Owner

@larsrgit I haven't been able to reproduce this. Could you upload the audio file here?
Also please make sure you're trying the latest beta version. I've just published wavesurfer.js@7.8.0-beta.3.

npm install wavesurfer.js@7.8.0-beta.3
@ndeshpande2022
Copy link
Author

Hi @katspaugh,
I think I'm seeing a similar issue to @larsrgit. The waveform disappears when zooming. Workaround is to navigate to a different timecode and the waveform re-appears.

image

@katspaugh
Copy link
Owner

@ndeshpande2022 thanks, I’ll continue debugging. And thank you for another donation! ❤️

@ndeshpande2022
Copy link
Author

No problem! I'm just going through testing beta v3 today to see how it runs.

The new timeline plugin seems to make the playback animation really choppy for longform content (>58 minutes). I'll see if I can gather more information about why.

@ndeshpande2022
Copy link
Author

@katspaugh - the new version is working really well - very impressed.

The only major issue I ran into was with the timeline plugin. It makes playback very choppy, but I'm not sure how we could fix it without adding it to the Canvas element instead of separate div(s).

I also had the same issue as @larsrgit above with the timeline not always redrawing on zoom change, but it's rare, and difficult to reproduce.

Thanks again for all your work -

@katspaugh
Copy link
Owner

Gotcha, thanks for testing! I’ll look into both the timeline performance and the disappearing waveform.

@ndeshpande2022
Copy link
Author

@katspaugh - Is there any way to direct message you? I have some questions I wanted to ask that weren't issue related.

Thanks again -

@katspaugh
Copy link
Owner

Sure, my GitHub username at gmail dot com.

@katspaugh
Copy link
Owner

I managed to reproduce the disappearing waveform and think I fixed it. Also optimized the Timeline rendering a bit so that it's less choppy during playback. Published wavesurfer.js@7.8.0-beta.4.

@larsrgit
Copy link

Thanks! The disappering waveform on zoom is now fixed for nearly all cases. Just when I zoom really close on a long audio, (displaying less than half a second of a 1h+ audio in my case) it shows the waveform like in the video below.

zoom-render-bug2.mp4
@katspaugh
Copy link
Owner

Thanks @larsrgit! More debugging to do! :)

@ndeshpande2022
Copy link
Author

Hi @katspaugh,

We upgraded this morning and the waveform drawing issue appears to be fixed. Great work -

We do have a new issue with the region plugin. When dragging a region to another location, the original region remains on the timeline.

e.g. I have two Regions on the timeline like so:
image

If I click and drag the first region over to the right everything works great and renders correctly:
image

If I click and drag the second region over to the right I see the issue:
image

The first region in the screenshot above doesn't exist in the region object and can't be removed:
image

@katspaugh
Copy link
Owner

@ndeshpande2022 gotcha, thank you. Will fix it shortly. Thanks for your continuous support.

@ndeshpande2022
Copy link
Author

@katspaugh No problem! Please ignore my issue above. It seems to be related to a part of our code. We have race condition where the region is being created twice. I've been able to identify the function in our own code and I have someone working on it. It is NOT a Wavesurfer issue as far as I can tell.

I think we can assume that beta 4 is working well.

I will post any new issues once we deploy to production.

ps. I'm not sure if I close this ticket or if @larsrgit would like to close once they are happy with the fix.

@katspaugh
Copy link
Owner

Awesome, thanks!
I’ll look into the remaining disappearance issue and we can close it then. 👍

@katspaugh
Copy link
Owner

@larsrgit I haven't been able to reproduce this. Could you share the audio URL and your wavesurfer settings? I'll open a new issue to investigate this.

In the meanwhile, I went ahead and released v7.8.0 (wavesurfer.js@7.8.0).

Thank you both!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 participants