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

Add support for IE/Edge/PlayReady #176

Conversation

jonoward
Copy link
Contributor

@jonoward jonoward commented Sep 9, 2015

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:

  1. Had to increase shaka.player.Player.UNDERFLOW_THRESHOLD_ from 0.2 to 0.5; because IE11 + Edge would automatically halt video before playhead reached the underflow theshold, a state it couldn't exit out of.
  2. Had to tweak shaka.util.FakeEventTarget.prototype.dispatchEvent to only define properties once as this caused an issue in IE11 (Edge OK). Also, IE11 didn't like the properties being defined with value/writable, only got it to work properly with get/set funcs. Issue was that event.target was always null, even if it was being set on the preceding line. I did experiment with using Object.defineProperty for each, instead of Object.defineProperties, as was discussed a little while back, but I had the same issue.
  3. Not sure if this is a browser bug in IE11, but when I handled PSSH data in the msneedkey event (within the new EME polyfill), I was occasionally getting the PSSH data duplicated back-to-back (e.g. if media contained PlayReady PSSH "ABC", sometime event.initData in msneedkey would have "ABC", and sometimes it would have "ABCABC"). Never found a reason why this was happening. This eventually caused an exception in IE11. So I tweaked the PSSH parser to keep track of the boundaries for each distinct PSSH box within a given array, which is used by the EME polyfil to remove duplicates within the msneedkey handler.
  4. EME manager uses shaka.util.Uint8ArrayUtils.key() to generate a unique key for a PSSH data array by converting to a comma separated string, which was later used as a property on an object. There must be a limit to the length of property names in IE11, as large values generated by the key() method (when PSSH data is large) caused the value of object.key to always be undefined. Modified this method to keep string representations of the given array internally, and assign it a randomly generated ID if it's not already present in the cache.

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?

@joeyparrish
Copy link
Member

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.

@shaka-bot
Copy link
Collaborator

Testing in progress...

@@ -0,0 +1,108 @@
/**
* Copyright 2014 Google Inc.
Copy link
Member

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?

@shaka-bot
Copy link
Collaborator

All tests passed!

@@ -0,0 +1,108 @@
/**
Copy link
Member

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?

@joeyparrish joeyparrish self-assigned this Sep 9, 2015
@joeyparrish joeyparrish added the type: enhancement New feature or request label Sep 9, 2015
// This is only a guess, since we don't really know from the prefixed API.
var allowPersistentState = true;

if (keySystem == 'org.w3.clearkey') {
Copy link
Member

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.

Copy link
Contributor Author

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);
Copy link
Member

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.

@joeyparrish
Copy link
Member

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.

@jonoward
Copy link
Contributor Author

Thanks Joey, I'll aim to incorporate these changes in the next few days.

@joeyparrish
Copy link
Member

Thanks, Jono. Ping me when the next version is ready.

joeyparrish added a commit that referenced this pull request Oct 20, 2015
Inspired by code in pull #176

Change-Id: I2e29310c8a3583ed208d7bd1ae2e747a92ddf480
joeyparrish added a commit that referenced this pull request Oct 21, 2015
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
@joeyparrish
Copy link
Member

We've written our own Promise polyfill to more fully support IE11 after your pull request is merged. Hope that helps.

@joeyparrish
Copy link
Member

@jonoward @jonoblinkbox

Any idea when you'll be able to make revisions? I'm hoping to wrap up v1.6.0 soon.

@jonoward
Copy link
Contributor Author

jonoward commented Nov 2, 2015

@joeyparrish Apologies for the delay, I'm working on this now, so I should have something within a 1-2 days

@jonoward jonoward force-pushed the pull-request/ie-edge-playready-support branch from 5325658 to 6db78ba Compare November 2, 2015 12:37
@jonoward
Copy link
Contributor Author

jonoward commented Nov 2, 2015

@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.

@joeyparrish
Copy link
Member

@jonoward Thanks for the revisions. Looks good! I'll run this PR through our build bot and then we can merge.

@shaka-bot
Copy link
Collaborator

Testing in progress...

@shaka-bot
Copy link
Collaborator

All tests passed!

joeyparrish added a commit that referenced this pull request Nov 2, 2015
…support

Add support for IE/Edge/PlayReady
@joeyparrish joeyparrish merged commit 80f48ea into shaka-project:master Nov 2, 2015
@joeyparrish
Copy link
Member

Woohoo! IE support landed! 1,000,000 thanks to @jonoward!

@jonoward
Copy link
Contributor Author

jonoward commented Nov 3, 2015

Awesome, thanks

@github-actions github-actions bot added the status: archived Archived and locked; will not be updated label Jul 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: archived Archived and locked; will not be updated type: enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants