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

Add MSE/EME support #66

Merged
merged 8 commits into from
Sep 2, 2020
Merged

Add MSE/EME support #66

merged 8 commits into from
Sep 2, 2020

Conversation

SergioCrisostomo
Copy link
Contributor

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 a node server to serve the audio chunks
  • the actual changes to the component
  • added a Story so we can visualise and test this new functionality

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 with concurrently?

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

@SergioCrisostomo
Copy link
Contributor Author

SergioCrisostomo commented Aug 25, 2020

Let's discuss this PR and if we move forward I will also update the docs and fix the rest of the lint problems.
(late night here, time to sleep)

Copy link
Owner

@lhz516 lhz516 left a 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:

  1. Please fix the test in src/ProgressBar.test.js. Make sure yarn test passes
  2. Please also fix the Typescript errors shown in the PR Diff view
  3. Please update the new props to README
  4. 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?
  5. Also in storybook, switching from "Canvas" to "Docs" crashes the page. Please take a look
  6. Can we also consider supporting the use case of live stream audio which doesn't require srcDuration?

src/index.tsx Outdated Show resolved Hide resolved
stories/mse-eme-player.tsx Outdated Show resolved Hide resolved
@lhz516 lhz516 changed the title Mse eme Add MSE/EME support Aug 27, 2020
@SergioCrisostomo
Copy link
Contributor Author

  1. 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.
Ideas:

@SergioCrisostomo
Copy link
Contributor Author

@lhz516 fixed #1, #2 and #4 on your comments. Please check my questions above.

@lhz516
Copy link
Owner

lhz516 commented Aug 29, 2020

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.

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.

@SergioCrisostomo
Copy link
Contributor Author

Updated: used a props object instead and updated Readme file. I suggest we take this as the MVP and iterate from here.

Copy link
Owner

@lhz516 lhz516 left a 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

src/index.tsx Outdated Show resolved Hide resolved
src/index.tsx Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@SergioCrisostomo
Copy link
Contributor Author

Nice! Updated. Let me know if anything missing...

@lhz516 lhz516 merged commit 418a08b into lhz516:master Sep 2, 2020
@lhz516
Copy link
Owner

lhz516 commented Sep 2, 2020

Awesome! Merged. Let me do some cleaning up and bump the version.

Feel free to create additional PRs if you find anything is missing

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.

Support Media Source Extensions and EME
2 participants