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

avoid call to HTMLMediaElement#load when peaks are given #1969

Merged
merged 8 commits into from
Jun 26, 2020

Conversation

osheroff
Copy link
Contributor

calling .load() explicitly on a mediaelement entirely defeats the "preload" setting and makes it impossible to avoid downloading data before playback, which is often what the user is trying to do by providing peak data.

When HTMLMediaElement#load is called, all of the major browsers make some request for the file.
Chrome and FF make a range request for the entire file, but only report downloading them partially. Safari reports downloading the file in its entirety.

calling `.load()` explicitly on a mediaelement entirely defeats the
"preload" setting and makes it impossible to avoid downloading
data before playback.

When HTMLMediaElement#load is called, all of the major browsers make
some request for the file.  Chrome and FF make a range request for the
entire file, but only report downloading them partially.  Safari reports
downloading the file in its entirety.
@coveralls
Copy link

coveralls commented Jun 16, 2020

Coverage Status

Coverage remained the same at 80.512% when pulling d953b44 on osheroff/avoid_mediaelement_play into a7c7344 on master.

@thijstriemstra
Copy link
Contributor

thanks @osheroff, could you check if there are any tickets that get fixed by this change (and add them to the PR description)?

@osheroff
Copy link
Contributor Author

I would have guessed there would have been more.

There's also a whole host of people talking about memory issues etc, but they're likely using the webaudio backend.

@thijstriemstra
Copy link
Contributor

can you add a changelog entry?

@thijstriemstra
Copy link
Contributor

ps. it seems only Chrome dies during the CI builds, Firefox seems fine. We can disable Chrome on CI for a while and try again when it's back up and stable. Not being able to trust the CI results is annoying.

@marizuccara
Copy link
Contributor

Hi maybe the check can be done not if !peaks but for example if preload attribute is of type none?

@osheroff
Copy link
Contributor Author

Hi maybe the check can be done not if !peaks but for example if preload attribute is of type none?

@marizuccara I think it's a good point, but maybe it should be both !peaks && (preload == 'none')? If we're not given server-generated peaks, I think we always have to call .load in order to trigger the waveform drawing (at least that's what the comment in the file seemed to indicated).

@osheroff
Copy link
Contributor Author

any further comments here? @marizuccara I modified it to check if preload == 'none'

@thijstriemstra
Copy link
Contributor

branch has merge conflict.

@osheroff
Copy link
Contributor Author

Ok I fixed the merge conflict. The system of putting changelog entries into PRs is a bit problematic, as every time another PR is merged into master my branch goes back into conflict (this is like the 3rd time I've had to update CHANGES.md on this PR). I'm wondering if there's a better solution...

@thijstriemstra
Copy link
Contributor

thijstriemstra commented Jun 25, 2020

The system of putting changelog entries into PRs is a bit problematic, as every time another PR is merged into master my branch goes back into conflict

We can use a section like this at top to reduce merge conflicts:

Next (unreleased)
-----------------

Made that change in this branch.

@osheroff osheroff merged commit 4cb1fe1 into master Jun 26, 2020
@thijstriemstra thijstriemstra deleted the osheroff/avoid_mediaelement_play branch June 26, 2020 16:34
osheroff added a commit that referenced this pull request Jul 2, 2020
PR #1969 removed the call to MediaElement#load, but this means that we
weren't properly triggering the waveform to render in the case where
peaks were provided.
osheroff added a commit that referenced this pull request Jul 6, 2020
* add a call to drawBuffer in `loadMediaElement`

PR #1969 removed the call to MediaElement#load, but this means that we
weren't properly triggering the waveform to render in the case where
peaks were provided.

* update changelog entry

* update comment, changelog
sandiz pushed a commit to sandiz/wavesurfer.js that referenced this pull request Sep 1, 2021
)

* avoid call to HTMLMediaElement#load when peaks are given

calling `.load()` explicitly on a mediaelement entirely defeats the
"preload" setting and makes it impossible to avoid downloading
data before playback.

When HTMLMediaElement#load is called, all of the major browsers make
some request for the file.  Chrome and FF make a range request for the
entire file, but only report downloading them partially.  Safari reports
downloading the file in its entirety.

* code review; only avoid .load call when preload == 'none'

* add changelog entry

* bump changelog entry to next release

* add next section to changelog

Co-authored-by: Thijs Triemstra <info@collab.nl>
sandiz pushed a commit to sandiz/wavesurfer.js that referenced this pull request Sep 1, 2021
* add a call to drawBuffer in `loadMediaElement`

PR katspaugh#1969 removed the call to MediaElement#load, but this means that we
weren't properly triggering the waveform to render in the case where
peaks were provided.

* update changelog entry

* update comment, changelog
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

Successfully merging this pull request may close these issues.

None yet

4 participants