[#156] fix: DASH content protection schemeIdUri as lowercase #157
Conversation
CHANGELOG.md
Outdated
| @@ -1,3 +1,10 @@ | |||
| ### TODO add version | |||
There was a problem hiding this comment.
can this be reverted? The changelog is generated automatically from commits on release.
test/inheritAttributes.test.js
Outdated
| @@ -9,6 +9,7 @@ import { stringToMpdXml } from '../src/stringToMpdXml'; | |||
| import errors from '../src/errors'; | |||
| import QUnit from 'qunit'; | |||
| import { toPlaylists } from '../src/toPlaylists'; | |||
| import decodeB64ToUint8Array from '@videojs/vhs-utils/cjs/decode-b64-to-uint8-array'; | |||
There was a problem hiding this comment.
in inheritAttributes, we import the es version, it would be better to stay consistent.
mpd-parser/src/inheritAttributes.js
Line 7 in fb0d298
| import decodeB64ToUint8Array from '@videojs/vhs-utils/cjs/decode-b64-to-uint8-array'; | |
| import decodeB64ToUint8Array from '@videojs/vhs-utils/es/decode-b64-to-uint8-array'; |
| @@ -2065,3 +2066,175 @@ QUnit.test('Test to check use of either Segment Template or Segment List when bo | |||
| assert.equal(actual.length, 1); | |||
| assert.deepEqual(actual, expected); | |||
| }); | |||
|
|
|||
| QUnit.test('keySystem info for representation - lowercase UUIDs', function(assert) { | |||
There was a problem hiding this comment.
No problem. I was wondering how thorough I should have been (all available keySystems, empty ContentProtection xml segment etc,,,)
….test.js Co-authored-by: Gary Katsevman <[email protected]>
| @@ -2065,3 +2066,175 @@ QUnit.test('Test to check use of either Segment Template or Segment List when bo | |||
| assert.equal(actual.length, 1); | |||
| assert.deepEqual(actual, expected); | |||
| }); | |||
|
|
|||
| QUnit.test('keySystem info for representation - lowercase UUIDs', function(assert) { | |||
|
@gkatsev is there something left for me to do, or are we just waiting before someone else merges/builds it? Just to be sure, this is my 1st PR. |
src/inheritAttributes.js
Outdated
| @@ -175,6 +175,11 @@ export const inheritBaseUrls = | |||
| const generateKeySystemInformation = (contentProtectionNodes) => { | |||
| return contentProtectionNodes.reduce((acc, node) => { | |||
| const attributes = parseAttributes(node); | |||
|
|
|||
| // case-insensitive schemeIdUris | |||
There was a problem hiding this comment.
I think it makes sense to add reasoning why here. perhaps your original reasoning from the issue:
Although it could be argued that according to the UUID RFC spec the UUID string (a-f chars) should be generated as a lowercase string it also mentions it should be treated as case-insensitive on input. Since the key system UUIDs in the keySystemsMap are hardcoded as lowercase in the codebase there isn't any reason not to do .toLowerCase() on the input UUID string from the manifest (at least I could not think of one).
There was a problem hiding this comment.
@brandonocasey added
Codecov Report
@@ Coverage Diff @@
## main #157 +/- ##
=======================================
Coverage 94.30% 94.30%
=======================================
Files 18 18
Lines 772 773 +1
Branches 240 241 +1
=======================================
+ Hits 728 729 +1
Misses 44 44
Continue to review full report at Codecov.
|
Original issue
DASH ContentProtection schemeIdUri attributes are converted to lowercase on input to avoid keySystems mismatch since UUIDs should be handled case-insensitively.