This repository has been archived by the owner on Jan 12, 2019. It is now read-only.
attach attributes property to playlist objects in cases the m3u8-pars… #1214
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gesinger
reviewed
Jul 28, 2017
src/playlist-loader.js
Outdated
| @@ -510,6 +508,12 @@ const PlaylistLoader = function(srcUrl, hls, withCredentials) { | |||
| playlist = loader.master.playlists[i]; | |||
| loader.master.playlists[playlist.uri] = playlist; | |||
| playlist.resolvedUri = resolveUrl(loader.master.uri, playlist.uri); | |||
| // Although the spec states an #EXT-X-STREAM-INF tag MUST have a | |||
| // BANDWIDTH attribute, we can play the stream without it. This means a poorly | |||
| // formated master playlist may not have an attribute list. An attributes | |||
src/playlist-loader.js
Outdated
| // formated master playlist may not have an attribute list. An attributes | ||
| // property is added here to prevent undefined references when we encounter | ||
| // this scenario. | ||
| playlist.attributes = playlist.attributes || {}; |
There was a problem hiding this comment.
Would be good to at least have a warning log here.
gesinger
reviewed
Jul 31, 2017
src/playlist-loader.js
Outdated
| // this scenario. | ||
| playlist.attributes = {}; | ||
|
|
||
| log.warn('Invalid playlist STREAM-INF detected. Missing attribute list.'); |
There was a problem hiding this comment.
Since BANDWIDTH is the only required attribute, maybe it makes sense to just write something like "Invalid playlist STREAM-INF detected. Missing BANDWIDTH attribute."
src/rendition-mixin.js
Outdated
| let attributes = playlist.attributes; | ||
| // some playlist attributes are optional | ||
| if (playlist.attributes.RESOLUTION) { | ||
| let resolution = playlist.attributes.RESOLUTION; |
There was a problem hiding this comment.
Kind of a nitpick, by why let and not const? If statement is the scope and the value doesn't seem to change.
gesinger
reviewed
Aug 1, 2017
test/playlist-loader.test.js
Outdated
| 'video2/media.m3u8\n'); | ||
|
|
||
| assert.ok(loader.master, 'infers a master playlist'); | ||
| assert.ok(loader.master.playlists[0], 'parsed invalid stream'); |
There was a problem hiding this comment.
Technically, I think the second one is the one that is invalid, but it may be worth checking both.
test/playlist-loader.test.js
Outdated
| assert.ok(loader.master, 'infers a master playlist'); | ||
| assert.ok(loader.master.playlists[0], 'parsed invalid stream'); | ||
| assert.ok(loader.master.playlists[0].attributes, 'attached attributes property'); | ||
| assert.equal(this.env.log.warn.calls, 1, 'logged a warning'); |
There was a problem hiding this comment.
Can also check the warning text.
gesinger
approved these changes
Aug 2, 2017
6 tasks
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
…er does not
Description
The m3u8-parser does not attach
attributesproperty to the playlist object when it is parsing a media playlist or a master manifest that has anEXT-X-STREAM-INFtag without an attribute list. This has caused someundefinedreference errors asattributesis not always checked first.Specific Changes proposed
This change attaches
attributesproperty to any playlist object returned from the parser that does not have it.addresses #1205