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: cache aes keys for text tracks (#973) #1228

Merged
merged 4 commits into from
May 26, 2022
Merged

Conversation

phloxic
Copy link
Contributor

@phloxic phloxic commented Dec 1, 2021

Description

So far aes keys for vtt are not cached even when cacheEncryptionKeys: true.

Specific Changes proposed

Call segmentKey() if the requested vtt segment includes a key before appending, so key can be cached.

Requirements Checklist

  • Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
  • Reviewed by Two Core Contributors

@welcome
Copy link

welcome bot commented Dec 1, 2021

💖 Thanks for opening this pull request! 💖

Things that will help get your PR across the finish line:

  • Run npm run lint -- --errors locally to catch formatting errors earlier.
  • Include tests when adding/changing behavior.
  • Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

@codecov
Copy link

codecov bot commented Dec 1, 2021

Codecov Report

Merging #1228 (03047ce) into main (5223427) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1228   +/-   ##
=======================================
  Coverage   86.30%   86.30%           
=======================================
  Files          39       39           
  Lines        9840     9842    +2     
  Branches     2292     2293    +1     
=======================================
+ Hits         8492     8494    +2     
  Misses       1348     1348           
Impacted Files Coverage Δ
src/vtt-segment-loader.js 81.11% <100.00%> (+0.21%) ⬆️

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 5223427...03047ce. Read the comment docs.

@phloxic
Copy link
Contributor Author

phloxic commented Dec 2, 2021

Here's an example.
In network panel filter URLs with .key.
Example before fix.

@phloxic
Copy link
Contributor Author

phloxic commented Dec 20, 2021

@gkatsev the fix is easy and works, but right now chances are low that I will time to dive into the testing rabbit hole anytime soon, sorry.

@phloxic
Copy link
Contributor Author

phloxic commented Apr 6, 2022

@gkatsev - is the lack of a test blocking this?

@gkatsev
Copy link
Member

gkatsev commented Apr 8, 2022

A test would be greatly appreciated!

@phloxic
Copy link
Contributor Author

phloxic commented Apr 14, 2022

@gkatsev - I feared as much ;-)
Unfortunately I have no clue how the tests work, especially the media related ones. For instance test/manifests/subtitles.m3u8 loads ./[0-2].webvtt which I can't find, probably because they are generated somehow, similarly to the manifests referenced by test/manifests/master-subtitles.m3u8 ... and at that point I give up because I am getting too embarrased ;-) but mainly because, as much as I'd like to, I simply do not have the time atm to really dig into it. Sorry.

@mister-ben mister-ben added help wanted needs: tests This PR needs (more) tests labels Apr 20, 2022
@gesinger
Copy link
Contributor

Thank you for the PR @phloxic !

Our tests can use a good cleanup, and can be hard to parse, so no worries there. Thankfully, we already have some tests around caching encryption keys over in the segment loader tests, so I made a PR to move those over to loader common: phloxic#1 . The loader common tests are run against all types of loaders (including VTT).

I ran the tests with your change commented out to prove the tests failed. Then ran again with the change included to show that they passed. And it looks good.

Thanks again for the contribution.

Move encryption key caching tests to common loader tests to cover VTT loader
@gesinger gesinger merged commit 721e1bf into videojs:main May 26, 2022
@welcome
Copy link

welcome bot commented May 26, 2022

Congrats on merging your first pull request! 🎉🎉🎉

kchang-brightcove pushed a commit that referenced this pull request Aug 1, 2022
* fix: cache aes keys for text tracks (#973)

* Move encryption key caching tests to common loader tests to cover VTT loader

Co-authored-by: Garrett Singer <gesinger@gmail.com>
@phloxic phloxic deleted the issue/973 branch August 15, 2022 22:42
misteroneill pushed a commit that referenced this pull request Aug 19, 2022
* fix: cache aes keys for text tracks (#973)

* Move encryption key caching tests to common loader tests to cover VTT loader

Co-authored-by: Garrett Singer <gesinger@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs: review needs: tests This PR needs (more) tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants