feat: added API to set media keys directly #61
Conversation
src/plugin.js
Outdated
| @@ -237,6 +237,20 @@ const onPlayerReady = (player) => { | |||
| const eme = function(options = {}) { | |||
| this.eme.options = options; | |||
|
|
|||
| this.eme.init = (config) => { | |||
There was a problem hiding this comment.
We may want to consider a more precise name. Maybe something as direct as initializeMediaKeys, and provide a comment/docs around why you'd want to use this API.
src/plugin.js
Outdated
| @@ -231,18 +231,45 @@ const onPlayerReady = (player) => { | |||
| * to you; if not, remove the wait for "ready"! | |||
| * | |||
| * @function eme | |||
| * @param {Object} [player] | |||
| * A playerobject. | |||
src/plugin.js
Outdated
| const eme = function(options = {}) { | ||
| this.eme.options = options; | ||
| const eme = function(player, options = {}) { | ||
| const initializeMediaKeys = (emeOptions = {}) => { |
There was a problem hiding this comment.
It might be easier to read (and potentially test) if we make this a top level function.
src/plugin.js
Outdated
| this.eme.options = options; | ||
| const eme = function(player, options = {}) { | ||
| const initializeMediaKeys = (emeOptions = {}) => { | ||
| const e = { |
There was a problem hiding this comment.
I think we should use a more descriptive variable name. Maybe something like mockEncryptedEvent or something alongside a comment.
test/plugin.test.js
Outdated
|
|
||
| this.player.eme(); | ||
|
|
||
| // testing the rejection path because this isn't a real session |
There was a problem hiding this comment.
Is this testing a rejection?
src/plugin.js
Outdated
| * An object of eme plugin options. | ||
| */ | ||
| const initializeMediaKeys = (player, emeOptions = {}) => { | ||
| // TODO: this should be refactored |
There was a problem hiding this comment.
Since we have the TODO below we can probably remove this one.
src/plugin.js
Outdated
| // TODO: this should be refactored and renamed to be less tied | ||
| // to encrypted events | ||
| if (player.tech_.el_.setMediaKeys) { | ||
| return handleEncryptedEvent(mockEncryptedEvent, emeOptions, player.eme.sessions, player.tech_); |
There was a problem hiding this comment.
Is it worth catching the promise rejection here since the mock is always expected behavior?
There was a problem hiding this comment.
It is expected to fail if you don't provide enough capability information for EME so I believe the error should bubble up
src/plugin.js
Outdated
| * @param {Object} [options={}] | ||
| * An object of options left to the plugin author to define. | ||
| */ | ||
| const eme = function(options = {}) { |
There was a problem hiding this comment.
This worked for me:
const eme = function(options = {}) {
const player = this;
this.ready(() => onPlayerReady(this));
this.eme = {
initializeMediaKeys(emeOptions = {}) {
const mergedEmeOptions = videojs.mergeOptions(
player.currentSource(),
options,
emeOptions
);
return initializeMediaKeys(player, mergedEmeOptions);
},
options
};
};
// Register the plugin with video.js.
const registerPlugin = videojs.registerPlugin || videojs.plugin;
registerPlugin('eme', eme);
to be clear, I'm trying to avoid an anonymous function being the plugin factory here
|
As a last piece of CR feedback, since initializeMediaKeys can return a promise or nothing, we should at least mention in the README that you will get a Promise on browsers that have promises. |
test/plugin.test.js
Outdated
|
|
||
| this.player.eme(); | ||
|
|
||
| // testing the rejection path because this isn't a real session |
This is to start a discussion/review around a new API to set up EME directly. This will allow an end user to set up EME on the video element before data is loaded.