Skip to content

Fix EME controller async errors #2552

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Mar 16, 2020
Merged

Fix EME controller async errors #2552

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 43 additions & 18 deletions src/controller/eme-controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,14 +83,16 @@ class EMEController extends EventHandler {
private _widevineLicenseUrl?: string;
private _licenseXhrSetup?: (xhr: XMLHttpRequest, url: string) => void;
private _emeEnabled: boolean;
private _requestMediaKeySystemAccess: MediaKeyFunc | null
private _requestMediaKeySystemAccess: MediaKeyFunc | null;

private _config: EMEControllerConfig;
private _mediaKeysList: MediaKeysListItem[] = [];
private _media: HTMLMediaElement | null = null;
private _hasSetMediaKeys: boolean = false;
private _requestLicenseFailureCount: number = 0;

private mediaKeysPromise: Promise<MediaKeys> | null = null;

/**
* @constructs
* @param {Hls} hls Our Hls.js instance
Expand Down Expand Up @@ -143,13 +145,14 @@ class EMEController extends EventHandler {
logger.log('Requesting encrypted media key-system access');

// expecting interface like window.navigator.requestMediaKeySystemAccess
this.requestMediaKeySystemAccess(keySystem, mediaKeySystemConfigs)
.then((mediaKeySystemAccess) => {
this._onMediaKeySystemAccessObtained(keySystem, mediaKeySystemAccess);
})
.catch((err) => {
logger.error(`Failed to obtain key-system "${keySystem}" access:`, err);
});
const keySystemAccessPromise = this.requestMediaKeySystemAccess(keySystem, mediaKeySystemConfigs);

this.mediaKeysPromise = keySystemAccessPromise.then((mediaKeySystemAccess) =>
this._onMediaKeySystemAccessObtained(keySystem, mediaKeySystemAccess));

keySystemAccessPromise.catch((err) => {
logger.error(`Failed to obtain key-system "${keySystem}" access:`, err);
Copy link
Collaborator Author

@robwalch robwalch Mar 15, 2020

Choose a reason for hiding this comment

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

Why don't we actually trigger errors in catch blocks?

Copy link
Collaborator

@itsjamie itsjamie Mar 15, 2020

Choose a reason for hiding this comment

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

We definitely can throw an error. Not sure off the top of my head what happens if you rethrow in the catch block, it might actually hit window and stop the current stack from running, rather than just reject the promise.

Copy link
Collaborator Author

@robwalch robwalch Mar 15, 2020

Choose a reason for hiding this comment

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

I'm just talking about triggering the no-key/access/session error events in catch blocks.

Throwing in the catch block throws the stack, and is caught in a wrapping promise if there is one.

We could throw in methods wrapped in promises like _attemptSetMediaKeys and _generateRequestWithPreferredKeySession and in a catch block, trigger the error events based on error object. These are just thoughts for how to improve the controller in v1. For this PR the goal is to just fix the failing functional test which it does.

});
}

get requestMediaKeySystemAccess () {
Expand All @@ -166,7 +169,7 @@ class EMEController extends EventHandler {
* @param {string} keySystem
* @param {MediaKeySystemAccess} mediaKeySystemAccess https://developer.mozilla.org/en-US/docs/Web/API/MediaKeySystemAccess
*/
private _onMediaKeySystemAccessObtained (keySystem: KeySystems, mediaKeySystemAccess: MediaKeySystemAccess) {
private _onMediaKeySystemAccessObtained (keySystem: KeySystems, mediaKeySystemAccess: MediaKeySystemAccess): Promise<MediaKeys> {
logger.log(`Access for key-system "${keySystem}" obtained`);

const mediaKeysListItem: MediaKeysListItem = {
Expand All @@ -177,17 +180,22 @@ class EMEController extends EventHandler {

this._mediaKeysList.push(mediaKeysListItem);

mediaKeySystemAccess.createMediaKeys()
const mediaKeysPromise = Promise.resolve().then(() => mediaKeySystemAccess.createMediaKeys())
.then((mediaKeys) => {
mediaKeysListItem.mediaKeys = mediaKeys;

logger.log(`Media-keys created for key-system "${keySystem}"`);

this._onMediaKeysCreated();
})
.catch((err) => {
logger.error('Failed to create media-keys:', err);

return mediaKeys;
});

mediaKeysPromise.catch((err) => {
logger.error('Failed to create media-keys:', err);
Copy link
Collaborator Author

@robwalch robwalch Mar 15, 2020

Choose a reason for hiding this comment

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

If we triggered errors in catch blocks we wouldn't need keysListItem checks in _attemptSetMediaKeys and _generateRequestWithPreferredKeySession.

My thought is to leave as-is for 0.14, but refactor in feature/v1.0.0.

});

return mediaKeysPromise;
}

/**
Expand Down Expand Up @@ -235,20 +243,37 @@ class EMEController extends EventHandler {

/**
* @private
* @param {string} initDataType
* @param {ArrayBuffer|null} initData
* @param e {MediaEncryptedEvent}
*/
private _onMediaEncrypted = (e: MediaEncryptedEvent) => {
logger.log(`Media is encrypted using "${e.initDataType}" init data type`);

this._attemptSetMediaKeys();
this._generateRequestWithPreferredKeySession(e.initDataType, e.initData);
if (!this.mediaKeysPromise) {
logger.error('Fatal: Media is encrypted but no CDM access or no keys have been requested');
this.hls.trigger(Event.ERROR, {
type: ErrorTypes.KEY_SYSTEM_ERROR,
details: ErrorDetails.KEY_SYSTEM_NO_KEYS,
fatal: true
});
Copy link
Collaborator Author

@robwalch robwalch Mar 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This would really be more of an async error.

If for whatever reason onManifestParsed was not called or did not set mediaKeysPromise, we could always call _attemptKeySystemAccess here to get the ball rolling. That way this redundant error handling could be removed.

return;
}

const finallySetKeyAndStartSession = (mediaKeys) => {
if (!this._media) {
return;
}
this._attemptSetMediaKeys(mediaKeys);
this._generateRequestWithPreferredKeySession(e.initDataType, e.initData);
};

// Could use `Promise.finally` but some Promise polyfills are missing it
this.mediaKeysPromise.then(finallySetKeyAndStartSession).catch(finallySetKeyAndStartSession);
}

/**
* @private
*/
private _attemptSetMediaKeys () {
private _attemptSetMediaKeys (mediaKeys?: MediaKeys) {
if (!this._media) {
throw new Error('Attempted to set mediaKeys without first attaching a media element');
}
Expand Down
5 changes: 3 additions & 2 deletions tests/unit/controller/eme-controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ describe('EMEController', function () {
}, 0);
});

it('should trigger key system error when bad encrypted data is received', function (done) {
it('should trigger key system error(s) when bad encrypted data is received', function (done) {
let reqMediaKsAccessSpy = sinon.spy(function () {
return Promise.resolve({
// Media-keys mock
Expand All @@ -98,8 +98,9 @@ describe('EMEController', function () {
media.emit('encrypted', badData);

setTimeout(function () {
expect(emeController.hls.trigger).to.have.been.calledTwice;
expect(emeController.hls.trigger.args[0][1].details).to.equal(ErrorDetails.KEY_SYSTEM_NO_KEYS);
expect(emeController.hls.trigger.args[1][1].details).to.equal(ErrorDetails.KEY_SYSTEM_NO_ACCESS);
expect(emeController.hls.trigger.args[1][1].details).to.equal(ErrorDetails.KEY_SYSTEM_NO_SESSION);
done();
}, 0);
});
Expand Down