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

fix: exclude playlists on DRM key status other than usable #1171

Merged
merged 10 commits into from
Jul 28, 2021

Conversation

brandonocasey
Copy link
Contributor

Description

This prevents us from trying to play drm content that is going to fail to play.

@codecov
Copy link

codecov bot commented Jul 21, 2021

Codecov Report

Merging #1171 (811b986) into main (5f60612) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1171   +/-   ##
=======================================
  Coverage   86.50%   86.51%           
=======================================
  Files          39       39           
  Lines        9599     9602    +3     
  Branches     2218     2219    +1     
=======================================
+ Hits         8304     8307    +3     
  Misses       1295     1295           
Impacted Files Coverage Δ
src/videojs-http-streaming.js 90.36% <100.00%> (+0.06%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5f60612...811b986. Read the comment docs.

},
{
"name": "HDCP v2.1 DRM dash",
"uri": "https://storage.googleapis.com/wvmedia/cenc/h264/tears/tears.mpd@21",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"uri": "https://storage.googleapis.com/wvmedia/cenc/h264/tears/tears.mpd@21",
"uri": "https://storage.googleapis.com/wvmedia/cenc/h264/tears/tears.mpd#21",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the # so that the source will still work, but we will have a url to sync the dropdown with when we reload the page.

src/videojs-http-streaming.js Outdated Show resolved Hide resolved
src/videojs-http-streaming.js Outdated Show resolved Hide resolved
this.player_.tech_.on('keystatuschange', (e) => {
// typically 'output-restricted', but anything other than usable will
// result in a failure to play.
if (e.status !== 'usable') {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should be careful here. Since there are various statuses: https://w3c.github.io/encrypted-media/#dom-mediakeystatus , some of which we handle, i.e., expired: https://github.com/videojs/videojs-contrib-eme/blob/5c441dca1896fc73dff2eeb855dd8e60f99c5da4/src/eme.js#L121-L126

It might be better to only blacklist on specific statuses.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

switched this to only exclude on output-restricted.

@@ -4503,6 +4503,56 @@ QUnit.test('configures eme for HLS on source buffer creation', function(assert)
}, 'set source eme options');
});

QUnit.test('eme handles keystatuschange where status is not usable', function(assert) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to have one more test where status is usable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added.

gesinger
gesinger previously approved these changes Jul 27, 2021
@brandonocasey brandonocasey merged commit de5baa7 into main Jul 28, 2021
@brandonocasey brandonocasey deleted the fix/drm-not-usable branch July 28, 2021 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants