-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Fix PlayReady DRM in Edge browser #5699
Conversation
Related to #5667 |
src/controller/eme-controller.ts
Outdated
if (keySessionContext.keySystem == KeySystems.PLAYREADY) { | ||
licenseChallenge = this._unpackPlayReadyLicenseRequest( | ||
xhr, | ||
licenseChallenge | ||
); | ||
} |
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.
Shouldn't this be moved to the top of licenseXhrSetup
so that the xhr is updated before licenseXhrSetup
is called and final modification of the request object can be made by developers?
In shaka-player the message is unpacked before the network engine applies request filters.
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.
Just unpacking the license request can easily be moved to the top. But this method also applies additional headers to the request, which requires the XHR to be in "open" state. Shaka uses their own request objects without this limitation.
Maybe we could extract the headers before and apply them afterwards. But this would (again) prevent the developers from modifying these specific headers.
Any suggestions?
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.
Maybe we could extract the headers before and apply them afterwards.
👍
There are several paths where XHR could be opened in setupLicenseXHR
. It can also be opened in licenseXhrSetup
:
hls.js/src/controller/eme-controller.ts
Lines 979 to 1030 in 0d3352d
private setupLicenseXHR( | |
xhr: XMLHttpRequest, | |
url: string, | |
keysListItem: MediaKeySessionContext, | |
licenseChallenge: Uint8Array | |
): Promise<{ xhr: XMLHttpRequest; licenseChallenge: Uint8Array }> { | |
const licenseXhrSetup = this.config.licenseXhrSetup; | |
if (!licenseXhrSetup) { | |
xhr.open('POST', url, true); | |
return Promise.resolve({ xhr, licenseChallenge }); | |
} | |
return Promise.resolve() | |
.then(() => { | |
if (!keysListItem.decryptdata) { | |
throw new Error('Key removed'); | |
} | |
return licenseXhrSetup.call( | |
this.hls, | |
xhr, | |
url, | |
keysListItem, | |
licenseChallenge | |
); | |
}) | |
.catch((error: Error) => { | |
if (!keysListItem.decryptdata) { | |
// Key session removed. Cancel license request. | |
throw error; | |
} | |
// let's try to open before running setup | |
xhr.open('POST', url, true); | |
return licenseXhrSetup.call( | |
this.hls, | |
xhr, | |
url, | |
keysListItem, | |
licenseChallenge | |
); | |
}) | |
.then((licenseXhrSetupResult) => { | |
// if licenseXhrSetup did not yet call open, let's do it now | |
if (!xhr.readyState) { | |
xhr.open('POST', url, true); | |
} | |
const finalLicenseChallenge = licenseXhrSetupResult | |
? licenseXhrSetupResult | |
: licenseChallenge; | |
return { xhr, licenseChallenge: finalLicenseChallenge }; |
Unpacking the license challenge first, before licenseXhrSetup
is invoked , and then saving the headers until after XHR is opened for any of the paths above make sense if we insist on HLS.js applying these changes.
But this would (again) prevent the developers from modifying these specific headers.
Not necessarily. If licenseXhrSetup
is implemented such that it throws when XHR is not opened, then licenseXhrSetup
will be invoked again.
Otherwise, if licenseXhrSetup
completes successfully on the first run then no, the headers would not be exposed to it - if this was an important path to cover, consider adding something like a headers
argument to licenseXhrSetup
.
So the question is, should the licenseChallenge
argument in licenseXhrSetup
always be the original keyMessage
so that developers can unpack it themselves, or the unpacked version with headers?
Follow up question, why not both?
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.
Well... by unpacking the licenseChallenge
after licenseXhrSetup
(like it is done now) the developers still get the original keyMessage
and can do the unpacking on their own if the choose to do so. But for the majority of the users (who most likely only need to add some authentication headers required by their DRM provider) the unpacking is handled by hls.js before sending the request.
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.
by unpacking the licenseChallenge after licenseXhrSetup (like it is done now) the developers still get the original keyMessage and can do the unpacking on their own if the choose to do so
Fair enough. That keeps it simple and no change needed here.
Please address the other change requests
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.
I've pushed the requested changes. Let me know if there is anything else that needs to be changed.
7251280
to
580659a
Compare
This PR fixes PlayReady DRM in Edge browser.
Why is this Pull Request needed?
On Edge, the raw license message is an UTF-16-encoded XML. It needs to be unpacked. The Challenge element is and base64 encoded xml string (the actual license challenge). Any HttpHeader elements need to be added to the xhr.
Same code can be found in shaka-player: https://github.com/shaka-project/shaka-player/blob/d6d5b155ff38428e96808094a341694d353ba289/lib/media/drm_engine.js#L1608
Are there any points in the code the reviewer needs to double check?
Code works and builds without warnings. I'm not primarily a JS developer. Check if everything is fine.
Resolves issues:
PlayReady playback not working in Edge browser.
Before:
After:
Checklist