videojs / videojs-contrib-eme

Supports Encrypted Media Extensions for playback of encrypted content in Video.js
Other
203 stars 72 forks source link

Promisify all the things and error on player #47

Closed squarebracket closed 5 years ago

squarebracket commented 6 years ago

Errors in EME will now fire player.error(). For spec-compliant EME and Safari, it's promises all the way down.

Also cleaned up the handleEncryptedEvent stuff; it was calling requestMediaKeySystemAccess twice for every encrypted.

Added a bunch of tests to test the various error conditions and make sure they spit out the correct error message strings.

A few things to note:

  1. I am using player.tech_ as an error event bus for IE11 since it doesn't have promises. Those events are then caught and the error handler is called.
  2. The tests will fail on IE11, because promises. I may just do a batch exclude of those tests when running on IE11. (this is the same on master)
  3. I do not shim video.msSetMediaKeys when available since it is apparently read-only and thus can't be assigned to in strict mode.
  4. I removed the shimming of window.navigator.requestMediaKeySystemAccess in a bunch of tests because I don't think it's necessary now (so long as you're running on localhost). Let me know if you think it should be shimmed out anyway.
squarebracket commented 5 years ago

Bump. Now ready for review.

squarebracket commented 5 years ago

Figured out why the tests weren't passing: I forgot getLicense was getting promisified. Fixed now.

squarebracket commented 5 years ago

I thought my tests would catch problems after rebasing, but they did not -- initializeMediaKeys is causing problems with the error handling, which I will have to fix.

squarebracket commented 5 years ago

I opted to simply automatically fire an error on the player unless the callback explicitly returns false.

squarebracket commented 5 years ago

Changed to use a parameter passed to initializeMediaKeys to suppress errors when possible, and cleaned up the testing code a bit.

squarebracket commented 5 years ago

uh, looks like videojs-generate-karma-config broke?

squarebracket commented 5 years ago

Actually maybe it's a transitive dependency that broke, I can't really tell.

squarebracket commented 5 years ago

Using lts/carbon fixed it.

squarebracket commented 5 years ago

One thing that came up in the process of this PR is that setupSessions is called on every encrypted, which is a bit excessive. I tested with

player.on('loadstart', () => {
  // wipe old sessions when changing sources
  player.eme.sessions = [];
});

and it fires as desired on all browsers. I've got a small diff from this PR to what I'm actually using in our internal player, which has the changes for key rotation, where I am using loadstart.

Is that something I should add to this PR, or keep it for the key rotation PR?

squarebracket commented 5 years ago

Actually, the key rotation PR includes some general cleanup and simplification, so it probably makes sense to just hold it off for that PR.

gesinger commented 5 years ago

I think holding off from this PR makes sense, just to keep changes as isolated as possible.

misteroneill commented 5 years ago

@squarebracket Is this PR depending on the key rotation PR or vice versa?

EDIT: Nevermind, answered question elsewhere.