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.

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

fix(DRM): close properly webkit media key sessions #6775

Merged
merged 1 commit into from
Jun 11, 2024
Merged

fix(DRM): close properly webkit media key sessions #6775

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
2 changes: 1 addition & 1 deletion externs/polyfill.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,5 +11,5 @@
*/


/** @type {boolean} */
/** @type {string} */
window.shakaMediaKeysPolyfill;
8 changes: 7 additions & 1 deletion lib/media/drm_engine.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,12 +179,18 @@ shaka.media.DrmEngine = class {

// |video_| will be |null| if we never attached to a video element.
if (this.video_) {
goog.asserts.assert(!this.video_.src, 'video src must be removed first!');
// Webkit EME implementation requires the src to be defined to clear
// the MediaKeys.
if (!shaka.util.Platform.isMediaKeysPolyfilled('webkit')) {
goog.asserts.assert(!this.video_.src,
'video src must be removed first!');
}

try {
await this.video_.setMediaKeys(null);
} catch (error) {
// Ignore any failures while removing media keys from the video element.
shaka.log.debug(`DrmEngine.destroyNow_ exception`, error);
}

this.video_ = null;
Expand Down
7 changes: 7 additions & 0 deletions lib/player.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1356,6 +1356,13 @@ shaka.Player = class extends shaka.util.FakeEventTarget {
this.playhead_ = null;
}

// EME v0.1b requires the media element to clear the MediaKeys
if (shaka.util.Platform.isMediaKeysPolyfilled('webkit') &&
this.drmEngine_) {
await this.drmEngine_.destroy();
Copy link
Member

Choose a reason for hiding this comment

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

Is there a downside to destroying DRM engine this early in all cases? Do we need two paths for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

We wanted to make this patch not invasive as much as possible. But I agree though, having diverged paths is not elegant. I can try to move drm engine teardown before the MSE one.

Copy link
Member

Choose a reason for hiding this comment

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

@tykus160 please create a feat with the change

Copy link
Member Author

@tykus160 tykus160 Jun 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hey @joeyparrish @avelad
According to EME spec:

Support for clearing or replacing the associated MediaKeys object during playback is a quality of implementation issue. In many cases it will result in a bad user experience or rejected promise.

In Chrome if I move drm destroy before mse, next playbacks of protected content are starting to stall after few secs. I’m using Angel One to test, I guess first few secs are clear there... I'm not sure why it is like that, but I see that MediaKeys are attached to the video element.
Explicitly unloading video element in DRM Engine before setting MediaKeys to null seems to fix the issue mentioned above and I see no drawbacks across Chrome, Firefox & Safari in MSE mode. I need to check though how it behaves on SmartTVs.

this.drmEngine_ = null;
}

// Media source engine holds onto the media element, and in order to
// detach the media keys (with drm engine), we need to break the
// connection between media source engine and the media element.
Expand Down
11 changes: 9 additions & 2 deletions lib/polyfill/patchedmediakeys_apple.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ shaka.polyfill.PatchedMediaKeysApple = class {
navigator.requestMediaKeySystemAccess =
PatchedMediaKeysApple.requestMediaKeySystemAccess;

window.shakaMediaKeysPolyfill = true;
window.shakaMediaKeysPolyfill = PatchedMediaKeysApple.apiName_;
}

/**
Expand Down Expand Up @@ -115,7 +115,7 @@ shaka.polyfill.PatchedMediaKeysApple = class {
PatchedMediaKeysApple.originalNavigatorRequestMediaKeySystemAccess = null;
PatchedMediaKeysApple.originalHTMLMediaElementPrototypeMediaKeys = null;

window.shakaMediaKeysPolyfill = false;
window.shakaMediaKeysPolyfill = '';
}

/**
Expand Down Expand Up @@ -791,3 +791,10 @@ shaka.polyfill.PatchedMediaKeysApple.MediaKeyStatusMap = class {
goog.asserts.assert(false, 'Not used! Provided only for the compiler.');
}
};

/**
* API name.
*
* @private {string}
*/
shaka.polyfill.PatchedMediaKeysApple.apiName_ = 'apple';
9 changes: 8 additions & 1 deletion lib/polyfill/patchedmediakeys_nop.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ shaka.polyfill.PatchedMediaKeysNop = class {
window.MediaKeys = PatchedMediaKeysNop.MediaKeys;
window.MediaKeySystemAccess = PatchedMediaKeysNop.MediaKeySystemAccess;

window.shakaMediaKeysPolyfill = true;
window.shakaMediaKeysPolyfill = PatchedMediaKeysNop.apiName_;
}

/**
Expand Down Expand Up @@ -136,6 +136,13 @@ shaka.polyfill.PatchedMediaKeysNop.MediaKeySystemAccess = class {
createMediaKeys() {}
};

/**
* API name.
*
* @private {string}
*/
shaka.polyfill.PatchedMediaKeysNop.apiName_ = 'nop';


// A low priority ensures this is the last and acts as a fallback.
shaka.polyfill.register(shaka.polyfill.PatchedMediaKeysNop.install, -10);
14 changes: 12 additions & 2 deletions lib/polyfill/patchedmediakeys_webkit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ shaka.polyfill.PatchedMediaKeysWebkit = class {
window.MediaKeys = PatchedMediaKeysWebkit.MediaKeys;
window.MediaKeySystemAccess = PatchedMediaKeysWebkit.MediaKeySystemAccess;

window.shakaMediaKeysPolyfill = true;
window.shakaMediaKeysPolyfill = PatchedMediaKeysWebkit.apiName_;
}

/**
Expand Down Expand Up @@ -860,7 +860,9 @@ class extends shaka.util.FakeEventTarget {
PatchedMediaKeysWebkit.prefixApi_('cancelKeyRequest');
try {
this.media_[cancelKeyRequestName](this.keySystem_, this.sessionId);
} catch (exception) {}
} catch (exception) {
shaka.log.debug(`${cancelKeyRequestName} exception`, exception);
}
}

// Resolve the 'closed' promise and return it.
Expand Down Expand Up @@ -977,4 +979,12 @@ shaka.polyfill.PatchedMediaKeysWebkit.MediaKeyStatusMap = class {
shaka.polyfill.PatchedMediaKeysWebkit.prefix_ = '';


/**
* API name.
*
* @private {string}
*/
shaka.polyfill.PatchedMediaKeysWebkit.apiName_ = 'webkit';


shaka.polyfill.register(shaka.polyfill.PatchedMediaKeysWebkit.install);
11 changes: 6 additions & 5 deletions lib/util/platform.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -648,16 +648,17 @@ shaka.util.Platform = class {
/**
* Returns true if MediaKeys is polyfilled
*
* @param {string=} polyfillType
* @return {boolean}
*/
static isMediaKeysPolyfilled() {
if (window.shakaMediaKeysPolyfill) {
return true;
static isMediaKeysPolyfilled(polyfillType) {
if (polyfillType) {
return polyfillType === window.shakaMediaKeysPolyfill;
}

return false;
return !!window.shakaMediaKeysPolyfill;
}


/**
* Detect the maximum resolution that the platform's hardware can handle.
*
Expand Down
19 changes: 19 additions & 0 deletions test/player_unit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,25 @@ describe('Player', () => {
}
});

it('destroys drmEngine before mediaSourceEngine with webkit polyfill',
async () => {
spyOn(shaka.util.Platform, 'isMediaKeysPolyfilled')
.and.returnValue(true);
goog.asserts.assert(manifest, 'Manifest should be non-null');

Util.funcSpy(drmEngine.destroy).and.callFake(async () => {
expect(mediaSourceEngine.destroy).not.toHaveBeenCalled();
await Util.shortDelay();
expect(mediaSourceEngine.destroy).not.toHaveBeenCalled();
});

await player.load(fakeManifestUri, 0, fakeMimeType);
await player.destroy();

expect(drmEngine.destroy).toHaveBeenCalled();
expect(mediaSourceEngine.destroy).toHaveBeenCalled();
});

it('destroys mediaSourceEngine before drmEngine', async () => {
goog.asserts.assert(manifest, 'Manifest should be non-null');

Expand Down
4 changes: 2 additions & 2 deletions test/polyfill/patchedmediakeys_apple_unit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ describe('PatchedMediaKeys_Apple', () => {
.toEqual(PatchedMediaKeysApple.MediaKeySystemAccess);
expect(navigator.requestMediaKeySystemAccess)
.toEqual(PatchedMediaKeysApple.requestMediaKeySystemAccess);
expect(window.shakaMediaKeysPolyfill).toBe(true);
expect(window.shakaMediaKeysPolyfill).toBe('apple');
});
});

Expand Down Expand Up @@ -124,7 +124,7 @@ describe('PatchedMediaKeys_Apple', () => {
.toEqual(originalWindowMediaKeySystemAccess);
expect(navigator.requestMediaKeySystemAccess)
.toEqual(originalNavigatorRequestMediaKeySystemAccess);
expect(window.shakaMediaKeysPolyfill).toBe(false);
expect(window.shakaMediaKeysPolyfill).toBe('');
});
});
});
36 changes: 36 additions & 0 deletions test/util/platform_unit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,42 @@ describe('Platform', () => {
setUserAgent(webOs5);
expect(shaka.util.Platform.isWebOS5()).toBe(true);
});

describe('isMediaKeysPolyfilled', () => {
let shakaMediaKeysPolyfill;

beforeAll(() => {
shakaMediaKeysPolyfill = window.shakaMediaKeysPolyfill;
});

afterAll(() => {
window.shakaMediaKeysPolyfill = shakaMediaKeysPolyfill;
});

it('should return true if media keys are polyfilled', () => {
window.shakaMediaKeysPolyfill = 'webkit';
const result = shaka.util.Platform.isMediaKeysPolyfilled();
expect(result).toBe(true);
});

it('should return false if media keys are not polyfilled', () => {
window.shakaMediaKeysPolyfill = '';
const result = shaka.util.Platform.isMediaKeysPolyfilled();
expect(result).toBe(false);
});

it('should return true with a matching polyfill type', () => {
window.shakaMediaKeysPolyfill = 'webkit';
const result = shaka.util.Platform.isMediaKeysPolyfilled('webkit');
expect(result).toBe(true);
});

it('should return false with a non-matching polyfill type', () => {
window.shakaMediaKeysPolyfill = 'webkit';
const result = shaka.util.Platform.isMediaKeysPolyfilled('apple');
expect(result).toBe(false);
});
});
});

/** @param {string} userAgent */
Expand Down
Loading