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

Fix performance issues with seeking while audio is playing #2045

Merged
merged 2 commits into from
Sep 25, 2020
Merged

Fix performance issues with seeking while audio is playing #2045

merged 2 commits into from
Sep 25, 2020

Conversation

todoroff
Copy link
Contributor

When making calls to the seekTo method while the audio is playing, we were experiencing a issues with console errors and performance (to the point of the UI becoming completely unresponsive in the case of high-frequency seeking) because of unnecessary calls to start and pause inside that method.

Making the seekTo method async and awaiting the calls to pause and start inside solved the errors and the performance hit for low-frequency seeks (less than ~3 seeks per second) but wasn't good enough for higher-frequency seeking due to too much delay to resolve the promises.

We finally decided to remove those calls altogether, because they appear to be redundant. They were first introduced in #918, and supposedly solved an issue with backward seeking on MediaElement, which I wasn't able to replicate (tested with a 30 min long .ogg file).

@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 80.0% when pulling 6c84519 on the-podcast-host:master into 590d9cc on katspaugh:master.

@sundayz
Copy link
Contributor

sundayz commented Aug 20, 2020

It should probably be removed, but did you run the javascript profiler on your code to confirm that the calls to play() and pause() are slow?

It's been 6 years so the original issue with mediaelement is probably fixed and we can remove the play/pause calls

@todoroff
Copy link
Contributor Author

todoroff commented Aug 21, 2020

It should probably be removed, but did you run the javascript profiler on your code to confirm that the calls to play() and pause() are slow?

It's been 6 years so the original issue with mediaelement is probably fixed and we can remove the play/pause calls

I haven't run a profiler.

What happens is that if you play and pause too fast one after the other, you get errors DOMException: The play() request was interrupted. After several of those, the entire browser bogs down until you hit pause.

If you do make sure play and pause run sequentially, meaning after each one is complete, and also awaiting each seekTo before shooting the next, there are no errors and browser responsiveness issues but you can't seek as fast as when the audio is not playing, which simply doesn't feel nice.

My benchmark was that in the last secnario you can make many more seeks while the audio isn't playing, compared to when it is playing, and assumed that it's because it's not making the play/pause calls and waiting for them to resolve inside.

@osheroff
Copy link
Contributor

i'd definitely check this against a bunch of browsers and html-audio vs media-element combos. iOS/Safari tends towards "wildly different in behavior" vs firefox/chrome

@sundayz
Copy link
Contributor

sundayz commented Aug 25, 2020

BTW if you want high performance you should be using WebAudio instead of MediaElement.

I assume you want to allow users to seek by dragging the cursor with the mouse during playback? With WebAudio you have more fine control over audio buffers etc which is needed for something like that.

@realies
Copy link

realies commented Aug 26, 2020

Works fine on Safari 13.1.2 (15609.3.5.1.3) over here.

@todoroff
Copy link
Contributor Author

@osheroff Works on Safari.
@sundayz We need MediaElement for our usecase.

Copy link
Contributor

@thijstriemstra thijstriemstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @todoroff. I think we should give it a try; less is more. Can you add a changelog entry?

CHANGES.md Outdated Show resolved Hide resolved
@thijstriemstra thijstriemstra merged commit a33cea8 into katspaugh:master Sep 25, 2020
marizuccara pushed a commit to marizuccara/wavesurfer.js that referenced this pull request Nov 28, 2020
sandiz pushed a commit to sandiz/wavesurfer.js that referenced this pull request Sep 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants