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

Improve EME Controller: Add PlayReady support #1918

Closed
wants to merge 25 commits into from

Conversation

ssreed
Copy link
Contributor

@ssreed ssreed commented Sep 15, 2018

This PR will...

  • Add support for PlayReady.

Depends on: #1915

Why is this Pull Request needed?

Are there any points in the code the reviewer needs to double check?

Resolves issues:

Checklist

  • changes have been done against master branch, and PR does not conflict
  • new unit / functional tests have been added (whenever applicable)
  • API or design changes are documented in API.md

@tchakabam
Copy link
Collaborator

Would be nice to have public test content!

Awesome job 👍

Copy link
Collaborator

@tchakabam tchakabam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a quick look over, but in general looks good.

I will have more comments after a more in depth look at it,.

src/controller/eme-controller.js Outdated Show resolved Hide resolved
@tchakabam
Copy link
Collaborator

#1915 can be closed if start looking at this?

@ssreed
Copy link
Contributor Author

ssreed commented Sep 17, 2018

#1915 can be closed if start looking at this?

Yeah, that works for me.

Copy link
Collaborator

@johnBartos johnBartos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IE11 doesn't support the navigator.requestMediaKeySystemAccess api. You need to do something like:

        let mediaKeys;
        try {
            mediaKeys = new window.MSMediaKeys(keyName);
        } catch (error) {
            return;
        }

src/controller/eme-controller.js Outdated Show resolved Hide resolved
@johnBartos
Copy link
Collaborator

@ssreed A test stream would be nice to have so that we can automate testing of this.

@@ -67,6 +72,7 @@ const createWidevineMediaKeySystemConfigurations = function (audioCodecs, videoC
const getSupportedMediaKeySystemConfigurations = function (keySystem, audioCodecs, videoCodecs) {
switch (keySystem) {
case KeySystems.WIDEVINE:
case KeySystems.PLAYREADY:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if this the same proc, we could rename createWidevineMediaKeySystemConfigurations to reflect it is not widevine specific?

@@ -404,28 +419,16 @@ class EMEController extends EventHandler {
let challenge;

if (keysListItem.mediaKeySystemDomain === KeySystems.PLAYREADY) {
logger.error('PlayReady is not supported (yet)');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was firmly believing we'd remove this line one day 👍

@turbidwater
Copy link
Contributor

@mikrohard Thanks for making that asset! It took a couple tries, but I am getting playback in the demo page you put up. It took a minute or so to start playback, but now it is starting quickly on subsequent plays. I've tried on 2 versions of Edge (41.16299.726.0 and 42.17134.1.0).

I'm not getting playback on a local build of this branch of hls.js and a couple demo players ( http://playready.azurewebsites.net/Home/TestPlayer and https://bitmovin.com/demos/drm). On the local build, I'm getting no errors, and I see loading, but no playback. For my config, I have

var hlsConfig = {
          	autoStartLoad: true,
          	debug: true,
            playreadyLicenseUrl: "https://test.playready.microsoft.com/service/rightsmanager.asmx?PlayRight=1&UseSimpleNonPersistentLicense=1"
}

Is there anything I'm missing? I will pull this down and continue looking locally. Thanks again!

@mikrohard
Copy link
Contributor

mikrohard commented Dec 11, 2018

You're missing these two:
hlsConfig.emeEnabled = true; hlsConfig.drmSystem = 'PLAYREADY';
bitmovin.com has domain based license protection... not sure about azure test player... but it has probably poor HLS support.

@turbidwater
Copy link
Contributor

Brilliant! That works and I have playback in my local build. Thanks!

@mikrohard
Copy link
Contributor

That's why I suggested that drmSystem should be optional. And emeEnabled should also be automatically enabled if license server url is provided. I lost more than one hour of debugging the first time trying to use encrypted streams with hls.js. All other players I used before require only the license url parameter.

@ssreed
Copy link
Contributor Author

ssreed commented Dec 11, 2018

And emeEnabled should also be automatically enabled if license server url is provided. I lost more than one hour of debugging the first time trying to use encrypted streams with hls.js. All other players I used before require only the license url parameter.

That is how it actually works for Widevine but I never updated the demo page to properly support PlayReady. 😅

@turbidwater
Copy link
Contributor

@mikrohard Do you have any instructions handy for using that KeySeed to do content for that MSFT Playready Public Test license server? We're trying to generate a fragmented playready file in addition to the bytearray asset you provided

@mikrohard
Copy link
Contributor

Using microsofts test keyseed only means using the right keyId/contentKey combination while encrypting the content (you can calculate the contentKey using the keySeed & keyId). You can find the instructions here: https://brokenpipe.wordpress.com/2016/10/06/generating-a-playready-content-key-using-a-key-seed-and-key-id/ Actually I didn't bother calculating this and just used the sample values provided on this website. If you do the same, you can also reuse EXT-X-KEY tags from my playlists... otherwise you'll have to do that manually. I used shaka-packager to encrypt my content & generate the manifests... but AFAIK shaka-packager can only produce bytearray assets.

PS: Later I also found out that https://test.playready.microsoft.com/service/rightsmanager.asmx implements the AcquirePackagingData method which means that you should be able to create playready content using this tutorial (again... only bytearray afaik): https://google.github.io/shaka-packager/html/tutorials/playready.html

@turbidwater
Copy link
Contributor

@mikrohard Great, thanks! We'll give that a try.

@ssreed
Copy link
Contributor Author

ssreed commented Jan 23, 2019

Added key rotation support but there are some issues and work that needs to be addressed.

  • currently there is an issue where multiple keysessions may get created per initData but according to the spec "Each Key session is associated with a single instance of Initialization Data provided in the generateRequest() call." (https://w3c.github.io/encrypted-media/#key-session)
  • need to clean up items and events when destroyed
  • seeking can cause an issue on manifests where multiple keys exist
  • more unit tests

Would love feedback or advice on tackling some of these issues!

@michaelcunningham19
Copy link
Member

@ssreed Thanks for the work you've done here !

I haven't had time to review it yet, but it is on my plate.

In the meantime as part of our effort to bring more TypeScript into the project we just merged in a PR that converts / adds typings to the EME controller in its existing state causing conflicts with your changes here.

Just an FYI, that's all. Here's the related PR for reference: #2076

@ssreed
Copy link
Contributor Author

ssreed commented Jan 29, 2019

@ssreed Thanks for the work you've done here !

I haven't had time to review it yet, but it is on my plate.

In the meantime as part of our effort to bring more TypeScript into the project we just merged in a PR that converts / adds typings to the EME controller in its existing state causing conflicts with your changes here.

Just an FYI, that's all. Here's the related PR for reference: #2076

Hope those changes make sense, I'm pretty new to TypeScript.

@johnBartos johnBartos modified the milestones: 0.13.0, 1.0.0 Jun 19, 2019
@mikrohard
Copy link
Contributor

Is this pull request still active? I would love to see playready support getting merged. We're planning to go into production with this patch... but I would very much prefer seeing this merged. Otherwise we'll have to maintain the patch by ourself.

@ssreed
Copy link
Contributor Author

ssreed commented Sep 14, 2019

Is this pull request still active? I would love to see playready support getting merged. We're planning to go into production with this patch... but I would very much prefer seeing this merged. Otherwise we'll have to maintain the patch by ourself.

I'm not sure how much bandwidth I will have but I wouldn't mind some assistance in seeing this one through!

@robwalch robwalch added this to DRM (needs prioritization) in Release Planning and Backlog Oct 28, 2019
@robwalch
Copy link
Collaborator

robwalch commented Mar 14, 2020

Is this pull request still active? I would love to see playready support getting merged. We're planning to go into production with this patch... but I would very much prefer seeing this merged. Otherwise we'll have to maintain the patch by ourself.

I'm not sure how much bandwidth I will have but I wouldn't mind some assistance in seeing this one through!

@ssreed @mikrohard Sorry for sleeping on this PR for so long. I'm looking at fixing up the EME controller and would like to get these changes in. The current blockers for me are:

  1. I don't have a active PlayReady stream to test agains
  2. This PR has merge conflicts

Our Widevine functional tests fail quite often and it looks to me like the controller is not handling async EME operations correctly. I'm working on a quick fix or at least better logging, but also thinking about doing some refactoring here. @ssreed if you could address the conflicts that would be great.

Any test stream contributions (apologies if I missed the window of opportunity on the stream/test demo above). Creating DRM streams of my own would take away from the time I can allocate to actually fixing and testing these issues, so connecting me with providers (Azure, EZDRM for example) and test streams would be really helpful.

I read through the conversation and it also looks like more could be done on the API config side. url licenseXhrSetup don't cut it with some license servers. I'm thinking along the lines of what shaka-player has or what we do in JWP for "license-wrapping".

drm: {
  widevine: {
  url: licenseServerUrl,
    licenseRequestFilter: function (request, drmInfo) {
        request.headers["Content-Type"] = ["application/json"];
        request.body = JSON.stringify({ desiredPayload: goesHere });
    },
    licenseResponseFilter: function (response, drmInfo) {
        response.data = ArrayBufferfromBase64(wrappedLicense);
    },
  }, 
  playready: {
    // same
  }
}

I suggested that drmSystem should be optional. And emeEnabled should also be automatically enabled if license server url is provided

Totally agree. Usually the solution is just finding the intersection between requestMediaKeySystemAccess results and license systems specified in the config.

Most of the concerns around configuration and docs were addressed in this PR that never made it in https://github.com/video-dev/hls.js/pull/2194/files#diff-96cc378dcdef180641176501f42183c7R79-R82 I'm keeping eyes on this change set because anyone wanting to integrate with different DRM solutions is going to need this level of configuration.

@robwalch robwalch mentioned this pull request Mar 15, 2020
3 tasks
@robwalch robwalch changed the base branch from master to patch/v0.15.x December 23, 2020 18:09
@robwalch robwalch removed this from the 1.0.0 milestone Feb 22, 2021
@robwalch robwalch added the DRM label Apr 6, 2021
@robwalch
Copy link
Collaborator

Replaced by #4930

@robwalch robwalch closed this Oct 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

7 participants