-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Add support for IE/Edge/PlayReady #176
Add support for IE/Edge/PlayReady #176
Conversation
We are having issues with test flakiness in general. It has gotten better and continues to improve, but we still have some issues with the integration tests. I'll run this PR on our bot and see how it goes. Thank you so much for putting this PR together. I'll review it as soon as possible. |
Testing in progress... |
@@ -0,0 +1,108 @@ | |||
/** | |||
* Copyright 2014 Google Inc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2015
Also, add a license annotation just before the copyright header, as in 4cc4e96?
All tests passed! |
@@ -0,0 +1,108 @@ | |||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To keep with the existing convention (externs/mediakeys.js), can you rename this to externs/msmediakeys.js?
// This is only a guess, since we don't really know from the prefixed API. | ||
var allowPersistentState = true; | ||
|
||
if (keySystem == 'org.w3.clearkey') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I can tell, IE11 doesn't seem to support clearkey.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually you're right, it doesn't, I'll remove this (and any other refs in the polyfill). I can't find definitive documentation about Edge, so I'll presume it doesn't support it either, as most of the EME implementation so far seems to be pretty much identical between them
delete this['mediaKeys']; // in case there is an existing getter | ||
this['mediaKeys'] = mediaKeys; // work around read-only declaration | ||
|
||
return mediaKeys.setMedia(this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you aren't calling setMedia(null) on the old value of mediaKeys, there seems to be no way for this polyfill to remove listeners created by setMedia. This could lead to a memory leak by holding references to the media element.
I think we're nearly there. The last version I tested (not current) had issues with non-zero start times, which also breaks all live content. The GPAC clip in the demo app uses a non-zero start time, so you can reproduce with that. The issue seems to be similar to one we had in #101 getting Firefox to work. We'll have to change how/when we set currentTime during startup, but that can wait for another patch. I'll also need to add some PR DRM settings to the demo app. There's one clip ("multi-DRM") that should work but doesn't yet. |
Thanks Joey, I'll aim to incorporate these changes in the next few days. |
Thanks, Jono. Ping me when the next version is ready. |
Inspired by code in pull #176 Change-Id: I2e29310c8a3583ed208d7bd1ae2e747a92ddf480
This will help with porting to IE11. Chrome, Firefox, Safari, and Edge all have native Promises. This polyfill does not support thenables because Shaka does not use them. Other than tests related to thenables, this polyfill passes the A+ test suite. It is also worth noting that this polyfill is incompatible with native Promises, so it should not be used to replace a native implementation or mixed with browser APIs that may use a native implementation internally. To safely test in Chrome, force prefixed EME (to avoid native Promises), set window.Promise to null, then load some content in the test app. If using a verison of Chrome after prefixed EME was dropped, use unencrypted content. To run the A+ test suite, compile the library, install nodejs and the module 'promises-aplus-tests', then run ./test_promise_polyfill.js. Inspired by pull #176 Change-Id: I0d25049f162ff7f3b57bbc795403fcdedf927262
We've written our own Promise polyfill to more fully support IE11 after your pull request is merged. Hope that helps. |
Any idea when you'll be able to make revisions? I'm hoping to wrap up v1.6.0 soon. |
@joeyparrish Apologies for the delay, I'm working on this now, so I should have something within a 1-2 days |
…and annotation improvements
…nt handlers, removing some redundant code and some code-style tweaks
5325658
to
6db78ba
Compare
@joeyparrish Right, that should hopefully be all the discussed changes pushed up. I've rebased on latest master and have removed our usage of the shaka.util.Uint8ArrayUtils.key from the polyfill. Regarding non-zero start times, yes this still seems to be an issue. In our fork, we made some additional changes to pass playbackStartTime down to each individual stream, to avoid relying on currentTime being set. It's a little messy though, have a look at this branch if you're interested - https://github.com/blinkbox/shaka-player/tree/release/1.4.0. Quite like the idea of waiting for the buffer to contain data before setting currentTime (that you mentioned in #101), rather than waiting on loadedmetadata. |
@jonoward Thanks for the revisions. Looks good! I'll run this PR through our build bot and then we can merge. |
Testing in progress... |
All tests passed! |
…support Add support for IE/Edge/PlayReady
Woohoo! IE support landed! 1,000,000 thanks to @jonoward! |
Awesome, thanks |
Added new EME polyfill for v20140218 which is the flavour of EME implemented by IE11/Edge. No support for peristent licenses, not 100% sure if this is possible in IE11/Edge. Not tested on live streams, don't have suitable content.
Also had to make some changes to work-around browser specific issues:
Tests:
Unit tests are passing, but am having some flakiness in the integration tests, in both this branch as well as latest master:
PR branch failing tests:
Player end-of-stream behavior permits looping
Master failing tests:
Player setPlaybackRate plays in reverse for negative rates
Player end-of-stream behavior permits looping
Any idea if there are known issues with the integration tests?