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

feat: eme controller #47

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

feat: eme controller #47

wants to merge 10 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 failed. Why did it fail? →

Name Link
🔨 Latest commit 0ea63e8
🔍 Latest deploy log https://app.netlify.com/sites/videojsdev/deploys/67bcb56e4549ac000812f51a

@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 93.33333% with 1 line in your changes missing coverage. Please review.

Project coverage is 63.98%. Comparing base (a86e68e) to head (4b4f852).

Files with missing lines Patch % Lines
...ckages/playback/src/lib/player/base/base-player.ts 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #47      +/-   ##
==========================================
+ Coverage   63.89%   63.98%   +0.09%     
==========================================
  Files         114      115       +1     
  Lines        4833     4848      +15     
  Branches      633      635       +2     
==========================================
+ Hits         3088     3102      +14     
- Misses       1737     1738       +1     
  Partials        8        8              
Flag Coverage Δ
dash-parser 63.98% <93.33%> (+0.09%) ⬆️
env-capabilities 63.98% <93.33%> (+0.09%) ⬆️
hls-parser 63.98% <93.33%> (+0.09%) ⬆️
playback 63.98% <93.33%> (+0.09%) ⬆️

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