lucisgit / moodle-filter_jwplayer

Moodle (<=3.1) filter that allows using JW Player for playing HTML5 and flash content.
https://moodle.org/plugins/view/filter_jwplayer
6 stars 6 forks source link

make player features available through filter #20

Closed jojoob closed 8 years ago

jojoob commented 9 years ago

I think it would be great if the additional player features are accessible through the filter functionality. This could be achieved by parsing additional url get params and passing them to the renderer as option parameter. Or maybe there is an alternative solution?

I've implemented a prototype that excepts url get params for subtitle tracks (sub-*), poster image (image) and customid within the href url. But I doesn't really understand the $urls = explode('#', $combinedurl); part in filter_jwplayer_split_alternatives function. In what circumstances a "combinedurl" is passed to the function?

Prototype: https://github.com/sudile/moodle-filter_jwplayer/blob/dev-filterparams/lib.php#L61

kabalin commented 9 years ago

Hello Johannes!

_split_alternatives is originally coming from here https://github.com/moodle/moodle/blob/master/lib/medialib.php#L115 and implemented in jwplayer for compatibility, the idea of this split is to allow more than one link to the same video If, say, you have one mp4 and another one hls stream, you may combine them into one URL, then the player embed them both and let browser/device to decide which one to use. I personally do not like the idea of using '#', because it is reserved symbol in URL for anchor, but we can't do much here.

Regarding parameters, it is a nice idea, but again, the question is how to deal with splitting? The alternative way could be using data attributes in the tag, e.g. <a href="./video.mp4" data-image="./image.jpg" data-playerid="player1" data-sub-en="./subtitles.vtt">video</a>.

Though, this approach with data attributes might be correct semantically, I think this is reinvention of the wheel. If it were possible to embed JW Player on exisitng <video> tag, the problems would be solved altogehter, because it already has functionality for specifying multiple files, subtitles, title images and many other parameters. But at the moment JW Player only works with empty "div" and custom JS init object unfortinately.

jojoob commented 9 years ago

Thank you for the explanation. The '#' as separator makes it difficult. :-/ As a workaround we could handle it like you did with width and hight params: ignore multiple occurrences and just use the last.

If it were possible to embed JW Player on exisitng

Maybe it's possible with a polyfill. If there is no existing one we could probably implement one by ourself if it's not too hard. But I think this is also overlapping with the whole Moodle media player discussion. mediaelements.js is maybe the better choice at all. But you are already discussing this at https://tracker.moodle.org/browse/MDL-38158 and https://docs.moodle.org/dev/HTML5_player.

kabalin commented 9 years ago

Width and hight params are also coming from the original Moode code, yes, same approach can be used for extra params if you like.

It is indeed overlapping with Moodle media player discussion. Polyfill might not be needed at all since video tag will just be used as a data source for parsing and building a parameters object to initialise JW Player.

jojoob commented 9 years ago

Polyfill might not be needed at all since video tag will just be used as a data source for parsing and building a parameters object to initialise JW Player.

Oh, yes of course you're right.

same approach can be used for extra params if you like.

Ok, it isn't the best solution indeed but the only one if we don't want to change the filter to use video/audio tags instead. I will implement this and create a pull request ok?

jojoob commented 9 years ago

_split_alternatives is originally coming from here https://github.com/moodle/moodle/blob/master/lib/medialib.php#L115 and implemented in jwplayer for compatibility, the idea of this split is to allow more than one link to the same video If, say, you have one mp4 and another one hls stream, you may combine them into one URL, then the player embed them both and let browser/device to decide which one to use. I personally do not like the idea of using '#', because it is reserved symbol in URL for anchor, but we can't do much here.

At which point we have to minding compatibility? Did you think this undocumented(?) feature is used a lot? Maybe we could do it our own way and document it well. Use the data attributes variant sounds actual really good. It would also avoid ugly href values like: http://testserver.vm/moodle/pluginfile.php/37/mod_video/videofile/0/sintel_trailer-480p.mp4?image=/pluginfile.php/37/mod_video/posterfile/0/Screen%20Shot%202015-05-08%20at%2017.28.53.png&sub-chapters=/pluginfile.php/37/mod_video/tocfile/0/tocfile.vtt&sub-default=/pluginfile.php/37/mod_video/subtitlefile/0/sintel_trailer-WEBVTT.vtt

By the way, what's the reason for defining the filter_jwplayer_split_alternatives function instead of a static filter_jwplayer_media::split_alternatives like in medialib.php?

kabalin commented 9 years ago

@jojoob data attributes is a good idea as long as you are happy with it and it is not stripped by htmlpurifier when video is added by student, so feel free to come up with pull request.

Backward compatibility should be preserved, we do not use this feature though, but someone else may and will be dissapointed.

filter_jwplayer_media::split_alternatives does not like rtmp://, so had to redefine function (the difference is this line https://github.com/lucisgit/moodle-filter_jwplayer/commit/ad908c9e87b2084ab068439ddca7f8ab52796f00#diff-828043dc9f8d10ccd58ff8b96297794fR89)

jojoob commented 9 years ago

so had to redefine function

What I mean is why not overriding the static split_alternatives method instead of defining a new function outside the class definition? It makes not a big difference, just wondering.

jojoob commented 9 years ago

Here is a prototype for passing options as data attributes: https://github.com/sudile/moodle-filter_jwplayer/commit/eb68efdf48840f78794b1ad754dda51fbc5f6690

owenbarritt commented 9 years ago

This was also something I was wondering about, and came up with an alternative solution (passing the image and subtitles with the other media files in the # separated url): https://github.com/wset/moodle-filter_jwplayer/commit/2a5b6e07f49429236bf6b638541a71d00a719c0c

The filter_jwplayer_split_alternatives function then pulls these out when it splits the urls.

kabalin commented 9 years ago

@owenbarritt, using # as a split symbol is not a good idea, I mentioned it earlier in this discussion and explained why. I suggest to stick to something more "standard", like data attributes @jojoob has implemented.

owenbarritt commented 9 years ago

Yes, but it's already in use for embedding media in multiple formats and for passing dimension settings (as https://docs.moodle.org/dev/Media_embedding#Multiple_formats)

From a usability point of view, if you are already passing several media files together this way then it makes sense to be able to include the poster images and subtitles with them.