-
-
Notifications
You must be signed in to change notification settings - Fork 98
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
Add MSE/EME support #66
Conversation
More about `root:true` in https://eslint.org/docs/user-guide/configuring#using-configuration-files-1
Let's discuss this PR and if we move forward I will also update the docs and fix the rest of the lint problems. |
cb978d6
to
5cdf4a9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your hard work! The primary feature of MSE works nicely. A few feedbacks:
- Please fix the test in
src/ProgressBar.test.js
. Make sureyarn test
passes - Please also fix the Typescript errors shown in the PR Diff view
- Please update the new
props
to README - In storybook, when start playing MSE audio and click the progress bar to seek the time which hasn't been downloaded, the indicator quickly jumps to the start and then jumps to the place I clicked. Could you investigate this visual defect?
- Also in storybook, switching from "Canvas" to "Docs" crashes the page. Please take a look
- Can we also consider supporting the use case of live stream audio which doesn't require
srcDuration
?
Sure, we need a UI decision: if we don't know the live stream length how should the progress bar behave? There can't be "progress" from left to right since we don't know the end time.
|
8a840c3
to
8610f56
Compare
@lhz516 fixed |
I'm thinking about keeping the indicator always at the end of progressbar while live streaming (like Youtube Live), but that introduces some other questions: How much previous audio chunk should it cache? How to decide the beginning time when dragging indicator to the very left... Anyways, this is not a high priority thing. I'm OK to put off this feature until we want it in the future. |
Updated: used a props object instead and updated Readme file. I suggest we take this as the MVP and iterate from here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Some minor change requests, and then we should be ready to merge
Nice! Updated. Let me know if anything missing... |
Awesome! Merged. Let me do some cleaning up and bump the version. Feel free to create additional PRs if you find anything is missing |
This PR enables the player to consume Media Source Extensions and to play also encrypted audio.
The changes I made are the minimal for this, leaving most of the needed logic on the consumer app. A simple example can be found in the Story for this functionality.
This PR is divided into 3 parts:
Adding the static server was the fastest way, adding something online can bring problems if the link breaks. The actual Storybook component is quite complex. This is an example of the type of complexity that will rely on the consumer, avoiding inserting that logic into this repo.
Had to touch some of the progress bar functionality... not very happy with the progress bar on
seek
, would prefer the not loaded chunks to be more clear. But it is a UI we can iterate on later. It's not wrong now, I just think could be better in the future.Note: for the Story to work we need to run the static server
node stories/assets-server/server.js
. Should we add this to the npm script maybe withconcurrently
?Very happy to have feedback, refactor suggestions or other comments.
Hope you find this a good addition to the project
Closes #63
P.S. My idea is to use this functionality to implement the player of https://github.com/SergioCrisostomo/node-audio-server