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: use in-spec EME for versions of Safari which support it #142

Merged
merged 11 commits into from
Oct 15, 2021

Conversation

brandonocasey
Copy link
Contributor

Closes #87

src/eme.js Outdated
Comment on lines 241 to 243
if (getContentId) {
sessionData.getContentId = getContentId;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the initData shouldn't change, we may be better off getting the contentId early and potentially saving that to the session data instead of passing the function around and saving it.

Copy link
Member

Choose a reason for hiding this comment

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

It does seem like maybe we should save it, but I'm not sure if I want to bother untangling it right now.
@brandonocasey did you have thoughts on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤔

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 will work on it, I think it might be easy enough to do.

src/eme.js Outdated Show resolved Hide resolved
test/plugin.test.js Outdated Show resolved Hide resolved
src/eme.js Outdated
@@ -463,7 +451,7 @@ export const standard5July2016 = ({
);

getLicenseFn = promisifyGetLicense(keySystem, getLicense, eventBus);
getContentIdFn = getContentId;
contentId = getContentId ? getContentId(initData) : null;
Copy link
Contributor

Choose a reason for hiding this comment

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

We'll also have to do it above in addPendingSessions. It might be worth trying to do it in standardizeKeySystemOptions

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 don't think it makes sense at that point, We now do it in the just before addSession

src/fairplay.js Outdated
@@ -128,7 +128,7 @@ export const defaultGetCertificate = (fairplayOptions) => {
};
};

export const defaultGetContentId = (emeOptions, initData) => {
export const defaultGetContentId = (initData) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically getContentId is an exposed API (from the README), and emeOptions should be passed in case someone passes a custom getContentId

Copy link
Member

Choose a reason for hiding this comment

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

yeah, we can't change this method signature or it'll be a breaking change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Copy link
Member

@gkatsev gkatsev left a comment

Choose a reason for hiding this comment

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

Verified latest changes locally.

@gkatsev gkatsev merged commit 5897655 into main Oct 15, 2021
@gkatsev gkatsev deleted the fix/fairplay-spec-support branch October 15, 2021 16:01
gkatsev added a commit that referenced this pull request Oct 19, 2021
gkatsev added a commit that referenced this pull request Oct 19, 2021
gkatsev added a commit that referenced this pull request Oct 19, 2021
gkatsev added a commit that referenced this pull request Oct 19, 2021
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.

None yet

4 participants