Parse multiple pssh from "encrypted" init data and fix Widevine key I… #6640
Conversation
…D extraction for playlist match Fixes #6636
src/utils/mp4-tools.ts
Outdated
| function parsePssh(view: DataView): PsshData | { size: number } { | ||
| const size = view.getUint32(0); | ||
| const length = view.byteLength; | ||
| if (view.byteLength < size) { |
There was a problem hiding this comment.
praise: 😎 shouldn't happen, but good cheap validation.
| if (result.version > 1) { | ||
| return null; | ||
| const version = view.getUint32(8) >>> 24; | ||
| if (version !== 0 && version !== 1) { |
There was a problem hiding this comment.
praise: probably a better check than prior impl 👌
src/utils/mp4-tools.ts
Outdated
| size: number; | ||
| }; | ||
|
|
||
| export function parseMultiPssh(initData: ArrayBuffer): PsshData[] { |
src/utils/mp4-tools.ts
Outdated
| while (offset + 32 < length) { | ||
| const view = new DataView(initData, offset); | ||
| const pssh = parsePssh(view); | ||
| if ('systemId' in pssh) { |
There was a problem hiding this comment.
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.sizes and comparing to byteLength
There was a problem hiding this comment.
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. Adding offset 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.
Cool about to hop on a wall of meetings but will try to circle back after. 👌
| } else if (result.version === 1) { | ||
| result.kids = []; | ||
| data = new Uint8Array(buffer, view.byteOffset + 32, dataSizeOrKidCount); | ||
| } else if (version === 1) { |
There was a problem hiding this comment.
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.
It's all present in the split kids isn't it?
src/controller/eme-controller.ts
Outdated
| psshInfo.systemId === KeySystemIds.WIDEVINE && | ||
| psshInfo.data | ||
| psshInfo.data && | ||
| psshInfo.data.length >= 30 |
There was a problem hiding this comment.
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:
keyId = psshInfo.data.subarray(8, 24);There was a problem hiding this comment.
Phrased differently, what was wrong with:
Phrased differently, what was wrong with:
The key ID ends at an offset from the end of the data. Not 8 bytes from the start.
question: Where did 30 bytes come from
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.
#6640) * Parse multiple pssh from "encrypted" init data and fix Widevine key ID extraction for playlist match Fixes #6636 * Logging and assertion improvements per @cjpillsbury's review * Logging and assertion improvements per @cjpillsbury's review (cherry picked from commit a960251)
This PR will...
Parse multiple pssh from "encrypted" init data and fix Widevine key ID extraction for playlist match
Why is this Pull Request needed?
With Widevine, HLS.js generates key sessions and license requests using key IDs from pssh data found in playlist KEY URIs or "encrypted" events - whichever it encounters first. The parsing of pssh data in "encrypted" event init data did not parse all payloads for all key-systems present. It also did not pull the key ID for the correct offset and as a result was not matching the playlist key which could result in a second session and set of license requests being made when the first payload was a Widevine PSSH. If it was for another key-system then the "encrypted" event was ignored.
Are there any points in the code the reviewer needs to double check?
Resolves issues:
Fixes #6636
Checklist