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

Mediaelement safari fixes #1964

Merged
merged 5 commits into from
Jun 15, 2020

Conversation

osheroff
Copy link
Contributor

@osheroff osheroff commented Jun 13, 2020

This fixes a condition where Safari was not playing with mediaelement/webaudio. It seems to implement the webaudio spec slightly differently than chrome and requires that you call AudioContext#resume().

I also rewrote the header comment into more natural english.

refs #1531

this was happening in the webaudio backend, but due to the odd
inheritance layout of mediaelement-webaudio it wasn't happening here.
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 80.371% when pulling c81e7aa on osheroff:mediaelement-safari-fixes into 54b27a5 on katspaugh:master.

Copy link
Contributor

@marizuccara marizuccara left a comment

Choose a reason for hiding this comment

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

Why rewrite the description?

@osheroff
Copy link
Contributor Author

Why rewrite the description?

I felt the language used could be clearer and more accurate.

@thijstriemstra
Copy link
Contributor

@katspaugh can you add @osheroff to the team? I think he made some valuable contributions: https://github.com/katspaugh/wavesurfer.js/commits?author=osheroff

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.

Can you also add a single sentence entry to the changelog?

Copy link
Owner

@katspaugh katspaugh left a comment

Choose a reason for hiding this comment

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

👍

@katspaugh
Copy link
Owner

@katspaugh can you add @osheroff to the team? I think he made some valuable contributions: https://github.com/katspaugh/wavesurfer.js/commits?author=osheroff

Sure, good idea. Ben, I've sent you an invite. Many thanks for your work!

src/webaudio.js Outdated Show resolved Hide resolved
@osheroff
Copy link
Contributor Author

off-topic, I was trying to figure out why all the builds were failing. I got down the rabbit hole so far as "something, somewhere, for some reason is adding a --no-eslintrc flag to the eslint command line", but I got quite stuck after that....

@thijstriemstra
Copy link
Contributor

thijstriemstra commented Jun 15, 2020

"something, somewhere, for some reason is adding a --no-eslintrc flag to the eslint command line"

I added this yesterday, but local build was fine with it. See lint command in package.json. Feel free to remove/fix of course. Looks like eslint broke though: https://ci.appveyor.com/project/katspaugh/wavesurfer-js/builds/33533693#L304

@osheroff
Copy link
Contributor Author

I added this yesterday, but local build was fine with it. See lint command in package.json. Feel free to remove/fix of course.

ah, mystery solved, this is one of those odd travis "merged with master" builds. got it.

What's the merge policy here? 2 plus-ones?

@thijstriemstra
Copy link
Contributor

What's the merge policy here? 2 plus-ones?

What do you mean?

@osheroff
Copy link
Contributor Author

What do you mean?

like, how many approvals do you want before a PR is merged?

@thijstriemstra
Copy link
Contributor

thijstriemstra commented Jun 15, 2020

like, how many approvals do you want before a PR is merged?

There is not really such a rule in this project. If you see a PR that is 99.99% correct to you, go ahead and merge it. If you have doubts, or just want someone to review your work, pick a couple of people and request a review. Hopefully someone responds soon and you can then merge it. If you open a PR that is trivial (doc/formatting changes), you can even merge it yourself afterwards (or commit direct to master but only if it's, like i said, trivial). Now that we add more and more people to the team it might make sense to have some formal policy at some point but for now it all seems to work out like this.

@thijstriemstra
Copy link
Contributor

Why rewrite the description?
I felt the language used could be clearer and more accurate.

I think it has improved @marizuccara.

@thijstriemstra thijstriemstra merged commit f96f3b3 into katspaugh:master Jun 15, 2020
@bwhitty
Copy link

bwhitty commented Jun 18, 2020

@thijstriemstra is it possible that this gets into a 3.x release? Or is the version 3 release line finished in favor of everything going towards the version 4 release?

@thijstriemstra
Copy link
Contributor

I think 4.0.0 can be released next week.

sandiz pushed a commit to sandiz/wavesurfer.js that referenced this pull request Sep 1, 2021
* rewrite mediaelement-webaudio comment

* resume audiocontext for Safari + mediaelement/webaudio

this was happening in the webaudio backend, but due to the odd
inheritance layout of mediaelement-webaudio it wasn't happening here.

* add changelog entry

* hoist common code into utility function

* generalize comment
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

6 participants