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

Jump to bottom

feat(HLS): Support for HLS key rotation #4568

Merged
merged 6 commits into from
Nov 8, 2022
Merged

feat(HLS): Support for HLS key rotation #4568

merged 6 commits into from
Nov 8, 2022

Conversation

varvaruc
Copy link
Contributor

@varvaruc varvaruc commented Oct 11, 2022 •
edited by joeyparrish
Loading

We tested this fix internally our HLS streams but also with the stream mentioned in #741 (comment)

test stream: https://content.uplynk.com/playlist/ebbbf2111be144a4971dd8426b70e809.m3u8?rmt=wv&m4fenctype=cenc

license server url: https://content.uplynk.com/wv

Fixes #741

@google-cla
Copy link

google-cla bot commented Oct 11, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@varvaruc varvaruc changed the title Support for HLS key rotation fix: Support for HLS key rotation Oct 11, 2022
@avelad
Copy link
Member

avelad commented Oct 11, 2022

Please, sign the CLA. Thanks!

@avelad
Copy link
Member

avelad commented Oct 11, 2022

Can you add some tests?

@avelad avelad added type: bug Something isn't working correctly component: HLS The issue involves Apple's HLS manifest format labels Oct 11, 2022
@avelad avelad requested review from joeyparrish and theodab October 11, 2022 13:58
@avelad avelad added this to the v4.3 milestone Oct 11, 2022
@avelad
Copy link
Member

avelad commented Oct 11, 2022

Please rebase to resolve the conflicts

@joeyparrish joeyparrish changed the title fix: Support for HLS key rotation feat(HLS): Support for HLS key rotation Oct 11, 2022
@avelad avelad added type: enhancement New feature or request and removed type: bug Something isn't working correctly labels Oct 12, 2022
@varvaruc
Copy link
Contributor Author

Can you add some tests?

working on this currently

@avelad avelad added the priority: P1 Big impact or workaround impractical; resolve before feature release label Oct 12, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Oct 12, 2022 •
edited
Loading

Incremental code coverage: 98.31%

@tdevittcarol tdevittcarol mentioned this pull request Oct 27, 2022
Closed
@varvaruc
Copy link
Contributor Author

Hey guys, anything else I can do for this PR so it gets reviewed / merged ?

@joeyparrish
Copy link
Member

No, you haven't done anything wrong. I've been juggling many projects and haven't had time to review yet. I've also been prioritizing bug fixes over feature review.

But this is at the top of my list now that all the bug fixes have been reviewed. Thanks!

joeyparrish
joeyparrish requested changes Nov 4, 2022
View reviewed changes
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.

Looks great! Just a few changes I'd like to see. Thanks!

lib/hls/hls_parser.js
/**
* @param {!shaka.hls.Playlist} playlist
* @param {string} mimeType
* @private
Copy link
Member

Choose a reason for hiding this comment

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

This needs a return-type annotation. This would be a record type in Closure:

/**
 * @return {{
 *   drmInfos: !Array.<shaka.extern.DrmInfo>,
 *   keyIds: !Set.<string>,
 *   encrypted: boolean,
 *   aesEncrypted: boolean
 * }}
 */

lib/hls/hls_parser.js
stream.keyIds = keyIds;
stream.drmInfos = drmInfos;
if (this.manifest_) {
this.playerInterface_.filter(this.manifest_);
Copy link
Member

Choose a reason for hiding this comment

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

We just added a method to the player interface that may be more appropriate for this. Please merge main into your branch. Then instead of filtering the entire manifest on every stream, try this:

  this.playerInterface_.newDrmInfo(stream);

lib/hls/hls_parser.js
const drmInfos = [];
const keyIds = new Set();

// TODO: May still need changes to support key rotation.
Copy link
Member

Choose a reason for hiding this comment

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

Please remove

@joeyparrish joeyparrish added the status: waiting on response Waiting on a response from the reporter(s) of the issue label Nov 7, 2022
@joeyparrish joeyparrish mentioned this pull request Nov 7, 2022
Merged
@joeyparrish
Copy link
Member

I'm going to make the changes I requested, then merge this PR.

All the content URLs mentioned here and in #741 are unavailable. So I'm going to take a small risk in the changes that I make to this PR before merging. I'm changing filter() to newDrmInfo(), but I will only be unable to test that change with the provided unit test.

@joeyparrish joeyparrish removed the status: waiting on response Waiting on a response from the reporter(s) of the issue label Nov 8, 2022
@joeyparrish
Copy link
Member

I'm going to make the changes I requested, then merge this PR.

Actually, I'm not. The author has not granted permission for maintainers to make changes. So I'll follow up with another PR of my own.

@joeyparrish joeyparrish merged commit 3846eea into shaka-project:main Nov 8, 2022
@github-actions github-actions bot mentioned this pull request Nov 8, 2022
Merged
joeyparrish added a commit to joeyparrish/shaka-player that referenced this pull request Nov 8, 2022
@joeyparrish
chore: Cleanup after HLS key rotation PR
ad48fa6 
See PR shaka-project#4568, which was merged without feedback addressed.
@joeyparrish joeyparrish mentioned this pull request Nov 8, 2022
Merged
joeyparrish added a commit that referenced this pull request Nov 8, 2022
@joeyparrish
chore: Cleanup after HLS key rotation PR (#4664)
5bde080 
See PR #4568, which was merged without feedback addressed.
@varvaruc
Copy link
Contributor Author

varvaruc commented Nov 9, 2022

Hey, sorry I missed the recent updates on this PR, my github notifications are messed up. @joeyparrish Thanks for for doing the updates.

@joeyparrish
Copy link
Member

No problem. Thanks for contributing!

@varvaruc varvaruc deleted the key-rotation branch April 5, 2023 09:02
@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.
Reviewers

@avelad avelad avelad left review comments

@joeyparrish joeyparrish joeyparrish approved these changes

@theodab theodab Awaiting requested review from theodab

Assignees
No one assigned
Labels
component: HLS The issue involves Apple's HLS manifest format priority: P1 Big impact or workaround impractical; resolve before feature release status: archived Archived and locked; will not be updated type: enhancement New feature or request
Projects
None yet
Milestone
v4.3
Development

Successfully merging this pull request may close these issues.

HLS key rotation
3 participants
@varvaruc @avelad @joeyparrish