Widevine eme #2194
Widevine eme #2194
Conversation
|
@realeyes-matthew Looks like you need to rebase against master |
There was a problem hiding this comment.
I'm always open to possibly expanding functionality, especially where the EMEController is relatively new - it's not surprising there is room for additional config hooks.
That being said, I'm having a hard time figuring out all the changes in this PR.
I'm seeing many changes unrelated to Widevine/EME
@realeyes-matthew Could you update this PR to keep changes to a minimum & focused on the task at hand? It makes reviewing far easier and makes us more confident in what we're approving/merging into master.
Feel free to open issues (following the templates) for the unrelated changes you've made here - assuming they actually fix an issue / or are otherwise beneficial changes.
A couple more things:
- Could you also squash your commits? 47 commits in one PR is rather large
- You don't need to commit anything in the
distdirectory 😄
realeyes-matthew
force-pushed
the
widevine-eme
branch
from
b2e187b to
ffdd3d9
Compare
|
@michaelcunningham19 Thanks for the reply. I've removed the code that is not related to EME, squashed my commits and removed the changes in dist. The PR should actually now be ready for review :) EDIT: Spoke too soon. Squashing my commits and rebasing onto master brought in all sorts of changes I did not make. Can you point me in the direction of something that would help me understand how to fix this? EDIT 2: Finally got the code back to where it needs to be for the PR. Once again ready to review |
realeyes-matthew
force-pushed
the
widevine-eme
branch
4 times, most recently
from
8e23aa3 to
703c5c8
Compare
|
While I apologize this is unsquashed (a bad merge is causing rebase problems), this PR is ready for review. I updated the changes to listen for the |
|
@johnBartos @itsjamie I finished writing the tests for the new EME controller. However, when I tried changing the base to 1.0.0, we have all sorts of changes unrelated to EME. Do you have any advice on how I should proceed since we want this to go into 1.0.0? |
|
@realeyes-matthew Try again, I rebased 1.0.0 against master |
|
@johnBartos Worked like a charm, now just needs review |
* Compute live start position for alt audio streams
* Move codecs to typescript file. * Type codecs helpers. * Rename cues to be TypeScript. * Rename cea-608 parser to TS. * Type CEA-608 parser and Cue utility. * Rename output filter to ts. * Add OutputFilter class. * Rename ewma bandwidth estimator to TS. * Rename ewma to TS. * Type EWMA utilties. * Add MediaKeys TS and type HlsConfig in EME controller. * Rename TextTrack utilities. * Add types to TextTrack utils. * Rename time-ranges to TS. * Add Types to TimeRange helpers. * Remove unused variables from cea-608 parser.. * Type defaultEstimate as a number for ewma estimator.
…xer flush (video-dev#215) * Refactor minProbeByteLength to only be enforced on transmuxer flush JW8-8993
docs: fix typo
This should also prevent future <type> asserts from sneaking in.
…ot start with 0x47") and optimize packet parser
This reverts commit 39559be.
Eslint type assertion rule
…-fmp4 Bugfix/v1 audiovideo fmp4
| onMediaDetaching() { | ||
| if (this.emeEnabled) { | ||
| const keySessionClosePromises: Promise<void>[] = this._keySessions.map((keySession) => { | ||
| return keySession.close(); |
There was a problem hiding this comment.
nice!
should we execute this._media.setMediaKeys(null) as well?
# Conflicts: # karma.conf.js # src/config.ts # src/controller/audio-stream-controller.js # src/controller/eme-controller.ts # src/controller/stream-controller.js # src/errors.ts # src/events.js # tests/unit/controller/eme-controller.js
Note: I can't check all the boxes on the checklist at the bottom, so any help or guidance to align this with standards is greatly appreciated
This PR will...
...revise the EME controller to allow the user to control the EME configuration process. It exposes 3 hooks that the user implements if
emeEnabledis true. These hooks allow the user to select the desired KeySystem (requestMediaKeySystemAccessFunc), provide the key session with the init data when a request is generated (getEMEInitializationDataFunc) and control the license retrieval flow (getEMELicenseFunc). Events and errors have been added for each step in the EME configuration process, as well as guards in the stream controllers to ensure that DRM-encrypted data is not decrypted by the key loader. API docs have also been updated.Why is this Pull Request needed?
Currently, the user does not have much control over the EME configuration process, especially around license requests (e.g. appending tokens). The user can now also choose the KeySystem they want to use in the requestMediaKeySystemAccessFunc, which helps us prepare to support Playready. This PR also splits out EME-related events from key events, and guards against DRM-encrypted data being decrypted like traditional AES-encrypted content.
Are there any points in the code the reviewer needs to double check?
I need to write tests, so any help or guidance with doing this correctly would be greatly appreciated.
Resolves issues:
This PR includes documentation per Issue #2016
Checklist