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

feat: make dash keySystems configurable #3276

Merged

Conversation

valotvince
Copy link
Contributor

@valotvince valotvince commented Mar 23, 2021

Description

Turns out the solution I developed for PlayReady recommendation (#1495 & #2750) is way too complicated and brings too many different use cases to handle over multiple files to be considered fit to be merged, without being scared for regressions (in my own opinion). Alongside that issue, some errors due to recommendation use could be raised outside the silent try catch, making it quite difficult to debug them for developers

In that PR, I make the DASH keySystems configurable so that any developer could chose to opt-in for recommendation based on DASH DRM UUID.

player.configure({
  dash: {
    keySystemsByURI: {
      'urn:uuid:9a04f079-9840-4286-ab92-e65be0885f95': 'com.microsoft.playready.recommendation',
      'urn:uuid:79f0049a-4098-8642-ab92-e65be0885f95': 'com.microsoft.playready.recommendation',
    }
  }
});

I'd like to have your opinion on this, and if you're favorable on such a change I would:

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@shaka-bot
Copy link
Collaborator

Test Failure:

Chrome 89.0.4389.90 (Linux x86_64)
  Demo config has an entry for every config option FAILED
    Error: Failed: Demo does not have a config field for "manifest.dash.keySystems.urn:uuid:1077efec-c0b2-4d02-ace3-3c1e52e2fb4b"
        at <Jasmine>
        at test/demo/demo_unit.js:89:11 <- test/demo/demo_unit.js:204:11
        at check (test/demo/demo_unit.js:113:15 <- test/demo/demo_unit.js:224:15)
        at check (test/demo/demo_unit.js:117:13 <- test/demo/demo_unit.js:228:13)
        at check (test/demo/demo_unit.js:117:13 <- test/demo/demo_unit.js:228:13)

    Error: Failed: Demo does not have a config field for "manifest.dash.keySystems.urn:uuid:edef8ba9-79d6-4ace-a3c8-27dcd51d21ed"
        at <Jasmine>
        at test/demo/demo_unit.js:89:11 <- test/demo/demo_unit.js:204:11
        at check (test/demo/demo_unit.js:113:15 <- test/demo/demo_unit.js:224:15)
        at check (test/demo/demo_unit.js:117:13 <- test/demo/demo_unit.js:228:13)
        at check (test/demo/demo_unit.js:117:13 <- test/demo/demo_unit.js:228:13)

    Error: Failed: Demo does not have a config field for "manifest.dash.keySystems.urn:uuid:9a04f079-9840-4286-ab92-e65be0885f95"
        at <Jasmine>
        at test/demo/demo_unit.js:89:11 <- test/demo/demo_unit.js:204:11
        at check (test/demo/demo_unit.js:113:15 <- test/demo/demo_unit.js:224:15)
        at check (test/demo/demo_unit.js:117:13 <- test/demo/demo_unit.js:228:13)
        at check (test/demo/demo_unit.js:117:13 <- test/demo/demo_unit.js:228:13)

    Error: Failed: Demo does not have a config field for "manifest.dash.keySystems.urn:uuid:79f0049a-4098-8642-ab92-e65be0885f95"
        at <Jasmine>
        at test/demo/demo_unit.js:89:11 <- test/demo/demo_unit.js:204:11
        at check (test/demo/demo_unit.js:113:15 <- test/demo/demo_unit.js:224:15)
        at check (test/demo/demo_unit.js:117:13 <- test/demo/demo_unit.js:228:13)
        at check (test/demo/demo_unit.js:117:13 <- test/demo/demo_unit.js:228:13)

    Error: Failed: Demo does not have a config field for "manifest.dash.keySystems.urn:uuid:f239e769-efa3-4850-9c16-a903c6932efb"
        at <Jasmine>
        at test/demo/demo_unit.js:89:11 <- test/demo/demo_unit.js:204:11
        at check (test/demo/demo_unit.js:113:15 <- test/demo/demo_unit.js:224:15)
        at check (test/demo/demo_unit.js:117:13 <- test/demo/demo_unit.js:228:13)
        at check (test/demo/demo_unit.js:117:13 <- test/demo/demo_unit.js:228:13)

Failed 1 tests, passed 2062, skipped 33

Copy link
Member

@joeyparrish joeyparrish left a comment

Choose a reason for hiding this comment

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

This all looks super reasonable to me. Please coordinate with @michellezhuogg, who is working on DrmEngine changes related to MediaCapabilities. Her changes may conflict.
Michelle, please make sure this PR gets rebased on top of any new changes you merge, and then run tests on all platforms in the lab before you merge this PR. Thanks!

test/media/drm_engine_unit.js Outdated Show resolved Hide resolved
test/media/drm_engine_unit.js Outdated Show resolved Hide resolved
Copy link
Contributor

@michellezhuogg michellezhuogg left a comment

Choose a reason for hiding this comment

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

This all looks super reasonable to me. Please coordinate with @michellezhuogg, who is working on DrmEngine changes related to MediaCapabilities. Her changes may conflict.
Michelle, please make sure this PR gets rebased on top of any new changes you merge, and then run tests on all platforms in the lab before you merge this PR. Thanks!

Will do!

externs/shaka/player.js Outdated Show resolved Hide resolved
externs/shaka/player.js Outdated Show resolved Hide resolved
@shaka-bot
Copy link
Collaborator

All tests passed!

Copy link
Contributor

@michellezhuogg michellezhuogg left a comment

Choose a reason for hiding this comment

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

Thank you!

@michellezhuogg michellezhuogg merged commit b4595d5 into shaka-project:master Apr 3, 2021
@valotvince valotvince deleted the configurable-key-systems branch April 3, 2021 17:13
@michellezhuogg michellezhuogg added this to the v3.1 milestone Apr 7, 2021
@github-actions github-actions bot added the status: archived Archived and locked; will not be updated label Jul 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: archived Archived and locked; will not be updated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants