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

feat: allow setting native hls and dash playback #106

Merged
merged 8 commits into from
Sep 7, 2017
Merged

Conversation

OrenMe
Copy link
Contributor

@OrenMe OrenMe commented Sep 7, 2017

Description of the Changes

By default we use HLS and Dash adapters for playback of HLS and Dash streams.
This feature will allow setting the required adapter to be native or not, so we can use native HLS playback on Safari for example.

{
  "playback": {
    "preferNative": {
      "hls": false,
      "dash": false
    }
  }
}

CheckLists

  • changes have been done against master branch, and PR does not conflict
  • new unit / functional tests have been added (whenever applicable)
  • test are passing in local environment
  • Travis tests are passing (or test results are not worse than on master branch :))
  • Docs have been updated

updated default method attribute values to be true for progressive playback support
@@ -7,14 +7,16 @@
"preload": "none",
"autoplay": false,
"muted": false,
"options": {
"tweaks": {
Copy link
Contributor

Choose a reason for hiding this comment

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

@OrenMe if we're changing this key we should change also the reference in dash & hls adapters

@@ -70,11 +77,15 @@ export default class NativeAdapter extends BaseMediaSourceAdapter {
* Checks if NativeAdapter can play a given mime type.
* @function canPlayType
* @param {string} mimeType - The mime type to check
* @param {boolean} preferNative - prefer native flag
Copy link
Contributor

Choose a reason for hiding this comment

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

you can annotate the default value true
@param {boolean} [preferNative=true] - prefer native flag

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice, didn't know this 👍

@@ -65,15 +65,16 @@ export default class MediaSourceProvider {
/**
* Checks if the a media source adapter can play a given source.
* @param {Source} source - The source object to check.
* @param {boolean} preferNative - prefer native flag
Copy link
Contributor

Choose a reason for hiding this comment

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

as above

@@ -95,7 +96,7 @@ export default class MediaSourceProvider {
static getMediaSourceAdapter(videoElement: HTMLVideoElement, source: Source, config: Object): ?IMediaSourceAdapter {
if (videoElement && source && config) {
if (!MediaSourceProvider._selectedAdapter) {
MediaSourceProvider.canPlaySource(source);
MediaSourceProvider.canPlaySource(source, false);
Copy link
Contributor

Choose a reason for hiding this comment

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

why false?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why do we even have this check here? we should be getting here after we already have a selectedAdapter anyway.
I'll change to true for consistency, but this seems unnecessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is api. You can asssume nothing how getting here.
Why true/false. You have the param from config

@@ -8,14 +8,16 @@
"preload": "none",
"autoplay": false,
"muted": false,
"options": {
"tweaks": {
Copy link
Contributor

Choose a reason for hiding this comment

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

i think 'options' is more intuitive...

@OrenMe OrenMe merged commit b24b7ea into master Sep 7, 2017
@OrenMe OrenMe deleted the preferNative branch September 7, 2017 11:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants