Add support for HLS tags with both value and attributes #1810
Conversation
lib/hls/manifest_text_parser.js
Outdated
| if (blockValue) { | ||
| value = blockValue[1]; | ||
| } | ||
|
|
There was a problem hiding this comment.
Could you move attributeRegex to here, so no unrelated code is between it being declared and it being used?
There was a problem hiding this comment.
I'd rather define regexes in one place and use them later as needed, but you're the maintainer.
There was a problem hiding this comment.
Yeah, it's just personal preference. I don't even know if the rest of the team would agree with me, but I'm the first responder so I get to make the calls.
| '(1) <NAME>:<VALUE> ' + | ||
| '(2) <NAME>:<ATTRIBUTE_LIST> ' + | ||
| ' (3) <NAME>'); | ||
|
|
||
| /** @const {number} */ | ||
| this.id = id; | ||
|
|
There was a problem hiding this comment.
A bit below this, Tag has a "toString" method. That method was also made assuming that tags cannot have both values and attribute lists. It should also be updated to account for this change.
Correspondingly, there's also a few simple tests for "toString" in the manifest text parser unit tests. There should be a new test for attribute+value tags in that.
There was a problem hiding this comment.
Good catch. Been a while since I've used protoypes directly. The new test case also caught that hls.Tag.toString() is treating all attributes as strings and stringifying them with quotes, so I added numeric value detection to avoid adding quotes. Detecting a numeric string with isNan(Number(str)) is shaky but all the ways of tricking it that I know shouldn't happen in this case.
lib/hls/manifest_text_parser.js
Outdated
| An attribute's format is 'AttributeName=AttributeValue'. | ||
| The parsing logic goes like this: | ||
| 1. Everything before ':' is a name (we ignore '#'). | ||
| 2. Everything after should be parsed as attributes if it contains '='. | ||
| 3. Otherwise, this is a value. | ||
| 2. Everything after ':' is a list of items, |
There was a problem hiding this comment.
This is confusingly worded. I think it'd be clearer like...
- Everything after ':' is a list of comma-separated items.
2a. The first item might be a value, if it does not contain '='.
2b. Otherwise, items are attributes.
There was a problem hiding this comment.
Yeah, it was better originally but exceeded the 80 character limit, then it went downhill. Copy-pasted your wording. Sounds much better.
…tes and stringify numeric attributes properly, small code style and comment changes
|
Okay, I think this looks good now. Thanks for the PR! |
|
All tests passed! |
github-actions
bot
added
the
status: archived
Fixes the problem reported in #1808
HLS parser wrongly assumes that a tag may have either a value or attributes, not both.
The RFC doesn't describe a general tag format or even give a name to what's called tag value in the Shaka Player codebase, but it clearly gives the format of EXTINF tag:
#EXTINF:<duration>,[<title>]This PR makes the following changes:
shaka.hls.Tagshaka.hls.ManifestTextParser.parseTag()letinshaka.hls.ManifestTextParser.parseTag(), most local variables are never reassigned and can beconst