feat: enable playlists with 'usable' keystatus #1460
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1460 +/- ##
==========================================
+ Coverage 86.01% 86.03% +0.02%
==========================================
Files 42 42
Lines 10673 10712 +39
Branches 2456 2462 +6
==========================================
+ Hits 9180 9216 +36
- Misses 1493 1496 +3 ☔ View full report in Codecov by Sentry. |
src/videojs-http-streaming.js
Outdated
| @@ -1127,6 +1127,7 @@ class VhsHandler extends Component { | |||
|
|
|||
| this.player_.tech_.on('keystatuschange', (e) => { | |||
| if (e.status !== 'output-restricted') { | |||
| this.playlistController_.updatePlaylistByKeyStatus(e.keyId, e.status); | |||
There was a problem hiding this comment.
Do we want to keep all the code listed further for cases when we receive output-restricted?
There was a problem hiding this comment.
I guess it is not longer valid
There was a problem hiding this comment.
Yeah I wasn't sure the best approach here. Leaving the code below, we'll maintain the same behavior as before where we exclude all the HD renditions on a single output-restricted status. I was also originally considering having this behind an options flag? Definitely open to some discussion on what we ultimately should have as the default behavior here.
There was a problem hiding this comment.
as far as I understand it conflicts with the proposed solution, since when we receive output-restricted we will exclude every HD+ rendition.
There was a problem hiding this comment.
I guess we should just remove it
There was a problem hiding this comment.
I mean the change is straightforward and should just work, do we need a flag for this?
There was a problem hiding this comment.
Yeah, I don't really see much downside in keeping this as is without a flag as the playlists with a keyId and no usable keystatus would not be playable anyway. Also agree, I think the HD rendition filtering is safe to remove here as the same thing would be accomplished through the new logic. Removing the rest below and simplifying this listener.
src/dash-playlist-loader.js
Outdated
| // // default KID is a 32 digit hext string separated by '-'. | ||
| // return playlist.contentProtection.mp4protection.attributes['cenc:default_KID'].replace(/-/g, ''); | ||
| // } | ||
| // } |
| } else if (hasUsableKeyStatus && nonUsableExclusion) { | ||
| delete playlist.excludeUntil; | ||
| delete playlist.lastExcludeReason_; | ||
| } |
There was a problem hiding this comment.
can we add logs for cases when we exclude it and when we restore it back
|
@wseymour15 and @dzianis-dashkevich Thanks for taking a look at this! I just pushed some updates, another look would be appreciated when you have time. |
Description
Only enable playlists with a 'usable' keystatus when playing DASH with DRM.
Specific Changes proposed
Reference the playlist keyId after getting a
keystatuschangeevent from contrib-eme and only enable encrypted playlists that we have a matching usable keyId for.Note: This PR includes changes from #1461
Requirements Checklist