Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Parse multiple pssh from "encrypted" init data and fix Widevine key I… #6640
Parse multiple pssh from "encrypted" init data and fix Widevine key I… #6640
Changes from 1 commit
eb136b2
7bd5b91
a3eb86b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: Where did 30 bytes come from/why the change in
keyId
, below? Couldn't figure it out from various specs/"specs" (i.e. example code in various languages in the wild for creating and/or parsing widevine pssh data). Phrased differently, what was wrong with:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The key ID ends at an offset from the end of the data. Not 8 bytes from the start.
That was incorrect. The length check has been moved into the helper. The key bytes start 22 bytes from the end of the PSSH data matching how we extract the playlist key ID in level-key.ts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
praise: 👌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion(non-blocking): should we consider signaling somehow when a pssh box was invalid and thus dropped (would say a dum dum console.warn but that wouldn't get control from hls logger)? We could check this "from the outside" by doing a sum of
psshInfo.size
s and comparing tobyteLength
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dealing with this outside of helper functions would be preferred so that we keep logging in the controller instances for log messages to have access to player and component context (v1.6 will include logger enhancements including #6131, #6198, #6242).
What do you think about returning all results including the invalid ones and typing them
PsshInvalidData
. Addingoffset
to the results might help too.I expect the most common issue to be incomplete data, but we might also run into bad boundaries/data without the
type
sync value (0x70737368). I'm not sure how granular we need to be here.It would help if you could comment on the logging for the "encrypted" event and let me know what elements you'd like to see like selected key-system or pssh system ids found.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool about to hop on a wall of meetings but will try to circle back after. 👌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
praise: 😎 shouldn't happen, but good cheap validation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
praise: probably a better check than prior impl 👌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question(non-blocking): Since this was already status quo before the changes, it doesn't need to be included in this PR, but is there any reason why we "leave the data on the floor" for the parsed
pssh
for version 1?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's all present in the split
kids
isn't it?