Skip to content

add com.microsoft.playready.recommendation as a recognised key system - #171

Open
benesva4 wants to merge 1 commit into
videojs:mainfrom
benesva4:main
Open

add com.microsoft.playready.recommendation as a recognised key system#171
benesva4 wants to merge 1 commit into
videojs:mainfrom
benesva4:main

Conversation

@benesva4

Copy link
Copy Markdown

I've added com.microsoft.playready.recommendation as a recognised key system. The keysystems behave almost identically. Function getSupportedKeySystem is then dependant on the name of the key system so there shouldn't be any changes needed there.

I wanted to add a test that would check that both of the key systems to go through defaultFairplayGetCertificate(), but there is no test that check getCeritificate for any key system.

@misteroneill misteroneill left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the PR @benesva4 - one quick question!

Comment thread src/eme.js
}

const isFairplay = isFairplayKeySystem(keySystem);
const isPlayready = keySystem === 'com.microsoft.playready' || keySystem === 'com.microsoft.playready.recommendation';

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should this include the recommendation.3000 variant as well?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Not in this state. It should be something like:

const isPlayready = [
    'com.microsoft.playready',
    'com.microsoft.playready.recommendation',
    'com.microsoft.playready.recommendation.3000'
    ].includes(keySystem)'

or a little bit less safe keySyste.startsWith('com.microsoft.playready') for .3000 to work. I wanted to add that up but I never got to that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants