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

Enable shaka in hls source videos #31

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

tanquetav
Copy link

I am using shaka to play my videos, but some of them are hls format, with m3u8 extension. All segments are downloaded without respect buffer by default handler in clappr . Fix canPlay to handle m3u8 files

@kslimani
Copy link
Contributor

aren't .m3u8 files supposed to be handled by HLSJS playback (or Flash HLS or Native on some device) ?

@tanquetav maybe it should be better to add a plugin option to enable this feature and let it disabled by default ?

@kslimani
Copy link
Contributor

kslimani commented Jul 17, 2017

Maybe add a static method to enable HLS for example :

static set enableHls(isEnabled) {
  // [...] default is false, if set to true also check for 'm3u8' file extension or HLS mime type
}

Example usage :

// Assuming plugin script is loaded
DashShakaPlayback && DashShakaPlayback.enableHls = true;

var player = new Clappr.Player({
    parentId: '#player',
    source: 'example.com/some.m3u8',
    plugins: [DashShakaPlayback]
});

@tchakabam
Copy link
Contributor

Well, I'd say it should just be that the plugins advertise exactly what they can, in this case DASH and HLS.

Then the player should just choose the first plugin it has that fulfills the necessary capabilities, based on some ordinality.

Plugins should have an order of selection in Clappr.

Plugins should not have to enable/disable showing their capabilities imho.

@tanquetav
Copy link
Author

@kslimani In fact, if the plugin is not enabled in "plugins: [DashShakaPlayback]" it will fallback to hlsjs . If user select DashShakaPlayback he wants play with shaka, that can be smarter than hlsjs handle the stream, like @tchakabam said. If shaka can handle, it will works. There is some cases that shaka notify that can' t handle (it is the browserSupported flag ) . I tested play hls stream in safari and ios and shaka cannot handle them. It fallback to clappr default implementation that can be HLSJS / Flash HLS / Native and plays ok.

@kslimani
Copy link
Contributor

@tchakabam @tanquetav i understand your point. But enabling hls playback by default on DashShakaPlayback will introduce a breaking change.

Without any option to enable/disable this feature, if you load DashShakaPlayback plugin, then you will be not able to use HLSJS playback to handle hls source. (this is why i propose this solution, so the developper can chosse which playback will handle hls source, without changing current plugin behaviour).

I would like to read also @leandromoreira opinion on this.

@tchakabam
Copy link
Contributor

well isn't the problem here really that you can not set an order of preference between Hlsjs and DashShakaPlugin, plus the fact that Hlsjs is bundled into Clappr now?

I d still say that it makes total sense if any plugin always advertise its potential of handling this many formats on a given environment. But then it is the role of Clappr as a good player framework to make the decision (eventually based on configuration given by the application using it) what plugin to use of the ones that have been made available at player creation time.

@tchakabam
Copy link
Contributor

If this needs changes in Clappr (like assigning priorities for plugins per formats) I'd be happy to do it.

Every plugin needs a settable ordinal per mime-type it wants to handle. GStreamer has been working like this and it proves to be a really great concept.

We were also thinking of using Shaka for HLS, but of course one would still want to be able to use it for DASH and have it Hls.js at the same time available.

I'd prefer that we find a clean way to deal with many plugins and mixed capabilities in Clappr, than to find some kind of workaround that might be half-satisfying in the end.

Interested to know what you guys think! :) 👍

@kslimani
Copy link
Contributor

@tchakabam when you add an external plugin to Clappr it is automatically prioritized over Clappr internal plugins.
This mean, that if you automatically enable hls in DashShakaPlugin then is it is not possible to use HLSJS Playback for HLS.

With my solution, by default DashShakaPlugin will handle dash source and Clappr internals plugins will handle hls source. (which is the current default behaviour). And you only set the property to true if you want also DashShakaPlugin to handle hls playback. (so the developper can decide which plugin will handle hls source).

@kslimani
Copy link
Contributor

An even more elegant way could be something like :

static withHls(isEnabled=true) {
  DashShakaPlayback._enableHls = !!isEnabled
  return DashShakaPlayback
}

static canPlay (resource, mimeType = '') {
  shaka.polyfill.installAll()
  var browserSupported = shaka.Player.isBrowserSupported()
  var resourceParts = resource.split('?')[0].match(/.*\.(.*)$/) || []
  return browserSupported && ((resourceParts[1] === 'mpd') || mimeType.indexOf('application/dash+xml') > -1 || (this._hlsEnabled && ((resourceParts[1] === 'm3u8') || mimeType === 'application/x-mpegURL' || mimeType === 'application/vnd.apple.mpegurl')))
}

Usage example :

var player = new Clappr.Player({
    parentId: '#player',
    source: 'example.com/some.m3u8',
    plugins: [DashShakaPlayback.withHls()]
});

@tanquetav
Copy link
Author

I updated my pull request with kslimani sugestion. It is ok to me.

@tchakabam
Copy link
Contributor

@kslimani I definitely understood the problem, and I do see how you are solving it with the existing Clappr conditions.

Now I would say that my proposal offers a more generic solution, and a way to generally identify plugin capabilities. What do you think?

@kslimani
Copy link
Contributor

kslimani commented Jul 18, 2017

@tchakabam if you have a solution to propose for enhance Clappr plugin loading order, feel free to create a PR or an issue on Clappr repository 👍

@kslimani
Copy link
Contributor

kslimani commented Jul 18, 2017

@tanquetav i tested your branch, and if enabling hls i got an error with shaka : category = 4 code = 4011 (which is "Errors parsing the Manifest.", "UNPLAYABLE_PERIOD" according doc). Does it work for you with dev-server- page ?

EDIT: the manifest i use for test hls source is http://www.streambox.fr/playlists/x36xhzz/x36xhzz.m3u8

@kslimani
Copy link
Contributor

kslimani commented Jul 18, 2017

hmmm it seems shaka player is currently bugged / unfinished for hls source.

Otherwise, the withHls() methods is working. 👍

@tanquetav would be nice also to edit readme file to explain new feature.

@tanquetav
Copy link
Author

Streambox url is a MPEG2-TS stream, that is handled by Edge, Chromecast, and Safari (shaka-project/shaka-player#860) . I published a stream using mp4 instead (built with ffmpeg and shaka-package) , you can check :
http://sabiadownload.s3-website-us-west-1.amazonaws.com/t6/master_playlist.m3u8

Here there is a sample to play:
http://sabiadownload.s3-website-us-west-1.amazonaws.com/t6/

This is correctly cors configured (Access-Control-Allow-Origin:*) , I think that you can use from your machine

@leandromoreira
Copy link
Member

I'd like to bring @towerz to this conversation he has a good idea to tackle this feature! 😄

@tanquetav
Copy link
Author

@kslimani About the mpeg2-ts stream that you tried before, shaka is planning support it in 2.3 release. In fact, hls.js transmux mpeg2-ts stream to fmp4 to play:
shaka-project/shaka-player#887
I hope that 2.3 be available soon.

@greenya
Copy link

greenya commented Sep 3, 2018

Hello guys.
Any plans to approve this PR soon?

We need just same thing -- to have ability to use ShakaPlayer for DASH and HLS. Currently we are using your awesome library here to handle only DASH.

@leandromoreira
Copy link
Member

leandromoreira commented Sep 3, 2018

@towerz @vagnervjs @jhonatangomes @joaopaulovieira ^ I think it's a good feature to have as well as to bump up version.

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.

5 participants