Add variable substitution support to HLS parser #2509
Conversation
lib/hls/hls_parser.js
Outdated
| if (replaceValue) { | ||
| newUri = newUri.replace(variable, replaceValue); | ||
| } else { | ||
| shaka.log.error(uriVariables, variableName); |
There was a problem hiding this comment.
Could you add some text to this log? Without context, it's a bid odd.
There was a problem hiding this comment.
I have added more information to the log.
lib/util/error.js
Outdated
| @@ -650,6 +650,11 @@ shaka.util.Error.Code = { | |||
| */ | |||
| 'INCONSISTENT_DRM_ACROSS_PERIODS': 4038, | |||
|
|
|||
| /** | |||
| * The HLS manifest requires undeclared variables. | |||
There was a problem hiding this comment.
How about "refers to" instead of "requires". I think that might be clearer.
It might also be good to add the variable name to the error using the "data" array. Just add additional parameters to the new shaka.util.Error() call and document them here.
There was a problem hiding this comment.
I have added that when the error is launched also include the missing varibale.
test/hls/hls_parser_unit.js
Outdated
| goog.asserts.assert( | ||
| audioPosition != null, 'Cannot find first audio segment'); | ||
|
|
||
| const videoReference = video.segmentIndex.get(videoPosition); |
There was a problem hiding this comment.
Since we refactored the manifest in #1339, SegmentIndex now implements Iterable, so you can now do things like this:
const videoReference = Array.from(video.segmentIndex)[0]And not worry about positions and null checks.
There was a problem hiding this comment.
I have updated the tests to use this new mechanism. Thank you!
test/hls/hls_parser_unit.js
Outdated
| expect(videoReference).not.toBe(null); | ||
| expect(audioReference).not.toBe(null); | ||
| if (videoReference) { | ||
| expect(videoReference.getUris()) |
There was a problem hiding this comment.
I'm not sure why you check the URIs of the segments, when the test only has variables in the playlist URIs. Could you add comments?
There was a problem hiding this comment.
I have added a comment that explains the need for this.
|
I will review all your comments tomorrow. Thank you! |
|
All tests passed! |
github-actions
bot
added
the
status: archived
Resolves #1561