Skip to content

feat: eme controller #47

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

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from
Draft

feat: eme controller #47

wants to merge 11 commits into from

Conversation

wseymour15
Copy link
Collaborator

NOTE: WIP ⚠️
As of now, I am looking for discussion regarding the API/Config. Values that are commented out are things I am leabing towards EXCLUDING from the API/Config. Anything not commented is what I am expecting to include.

Please read through the comments, as I have some thoughts that I'd appreciate feedback on.

Description

Draft PR for EME controller related changes.

Specific Changes proposed

Please list the specific changes involved in this pull request.

Requirements Checklist

  • Feature implemented / Bug fixed
  • Change has been verified in an actual browser (Chrome, Firefox, Safari, Edge) (if applicable)
  • Unit Tests updated or fixed (if applicable)
  • Docs/guides updated (if applicable)

Copy link

netlify bot commented Jan 15, 2025

Deploy Preview for videojsdev ready!

Name Link
🔨 Latest commit efcab47
🔍 Latest deploy log https://app.netlify.com/sites/videojsdev/deploys/67c8902900efd90008a2eeb3
😎 Deploy Preview https://deploy-preview-47--videojsdev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@wseymour15 wseymour15 marked this pull request as draft January 15, 2025 20:30
Copy link

codecov bot commented Jan 15, 2025

Codecov Report

Attention: Patch coverage is 12.47265% with 800 lines in your changes missing coverage. Please review.

Project coverage is 55.91%. Comparing base (a86e68e) to head (efcab47).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
packages/playback/src/lib/eme/eme-manager.ts 0.00% 491 Missing and 1 partial ⚠️
packages/playback/src/lib/eme/string-utils.ts 0.00% 135 Missing and 1 partial ⚠️
packages/playback/src/lib/eme/buffer-utils.ts 1.28% 77 Missing ⚠️
packages/playback/src/lib/errors/eme-errors.ts 52.27% 42 Missing ⚠️
packages/playback/src/lib/events/eme-events.ts 55.55% 16 Missing ⚠️
packages/playback/src/lib/eme/eme-utils.ts 6.25% 15 Missing ⚠️
packages/playback/src/lib/events/parse-events.ts 0.00% 12 Missing and 1 partial ⚠️
...ckages/playback/src/lib/player/base/base-player.ts 52.63% 9 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #47      +/-   ##
==========================================
- Coverage   63.89%   55.91%   -7.99%     
==========================================
  Files         114      120       +6     
  Lines        4833     5718     +885     
  Branches      633      640       +7     
==========================================
+ Hits         3088     3197     +109     
- Misses       1737     2510     +773     
- Partials        8       11       +3     
Flag Coverage Δ
dash-parser 55.91% <12.47%> (-7.99%) ⬇️
env-capabilities 55.91% <12.47%> (-7.99%) ⬇️
hls-parser 55.91% <12.47%> (-7.99%) ⬇️
playback 55.91% <12.47%> (-7.99%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@adrums86 adrums86 left a comment

Choose a reason for hiding this comment

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

Great start! Just added a few thoughts after my first quick read through.

* @param keySystem The key system string
* @returns Whether the key system is PlayReady
*/
private isPlayReadyKeySystem_(keySystem: string): boolean {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think helpers such as these are perfect for either adding to, or importing from the CML.

keyStatusMap.forEach((status, keyId) => {
// Edge has the order of these values swaped from the spec.
// We need to account for this
if (typeof keyId === 'string') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We might want to split out the Edge specific logic into its own function here.


let hasExpiredKeys = false;

keyStatusMap.forEach((status, keyId) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is some additional complexity here that will likely have to be accounted for with HDCP, specifically mapping keyIds to 'usable' status'.

export type EventTypeToEventMap = NetworkEventMap & PlayerEventMap;
export interface ParseEventMap {
[PlayerEventType.HlsPlaylistParsed]: LoggerLevelChangedEvent;
[PlayerEventType.DashManifestParsed]: VolumeChangedEvent;
Copy link
Collaborator

Choose a reason for hiding this comment

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

is it just a mock for now?

public readonly category = ErrorCategory.Eme;
}

export class SourceNotSetError extends EmeError {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we have this type of error for EME?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just a safety check to ensure that we don't try setting init data when the source is not set on the player. This can probably be handled on the player side.

// TODO: check if we have pending request or init one if we have init data
}

public stop(): void {
for (const [id] of this.activeSessions_) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

not sure if we can have multiple active sessions

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.

3 participants