From edfb30577c1e92b991e225207e682db06c340540 Mon Sep 17 00:00:00 2001 From: Matthew Neil Date: Fri, 28 Jul 2017 17:17:23 -0400 Subject: [PATCH 1/5] attach attributes property to playlist objects in cases the m3u8-parser does not --- src/master-playlist-controller.js | 8 ++++---- src/playlist-loader.js | 19 +++++++++++++------ src/playlist-selectors.js | 18 +++++++----------- src/rendition-mixin.js | 19 +++++++------------ src/segment-loader.js | 2 +- test/playlist-loader.test.js | 2 ++ test/rendition-mixin.test.js | 11 +++-------- 7 files changed, 37 insertions(+), 42 deletions(-) diff --git a/src/master-playlist-controller.js b/src/master-playlist-controller.js index a1c711cb7..8f9ac3fa0 100644 --- a/src/master-playlist-controller.js +++ b/src/master-playlist-controller.js @@ -836,7 +836,7 @@ export class MasterPlaylistController extends videojs.EventTarget { let videoPlaylist = this.masterPlaylistLoader_.media(); let result; - if (videoPlaylist.attributes && videoPlaylist.attributes.AUDIO) { + if (videoPlaylist.attributes.AUDIO) { result = this.audioGroups_[videoPlaylist.attributes.AUDIO]; } @@ -855,7 +855,7 @@ export class MasterPlaylistController extends videojs.EventTarget { return null; } - if (videoPlaylist.attributes && videoPlaylist.attributes.SUBTITLES) { + if (videoPlaylist.attributes.SUBTITLES) { result = this.subtitleGroups_.groups[videoPlaylist.attributes.SUBTITLES]; } @@ -1619,7 +1619,7 @@ export class MasterPlaylistController extends videojs.EventTarget { let videoCodec = null; let codecs; - if (media.attributes && media.attributes.CODECS) { + if (media.attributes.CODECS) { codecs = parseCodecs(media.attributes.CODECS); videoCodec = codecs.videoCodec; codecCount = codecs.codecCount; @@ -1630,7 +1630,7 @@ export class MasterPlaylistController extends videojs.EventTarget { videoCodec: null }; - if (variant.attributes && variant.attributes.CODECS) { + if (variant.attributes.CODECS) { let codecString = variant.attributes.CODECS; variantCodecs = parseCodecs(codecString); diff --git a/src/playlist-loader.js b/src/playlist-loader.js index a924686d2..e219d4d64 100644 --- a/src/playlist-loader.js +++ b/src/playlist-loader.js @@ -172,6 +172,9 @@ const PlaylistLoader = function(srcUrl, hls, withCredentials) { parser.push(xhr.responseText); parser.end(); parser.manifest.uri = url; + // m3u8-parser does not attach an attributes property to media playlists so make + // sure that the property is attached to avoid undefined reference errors + parser.manifest.attributes = parser.manifest.attributes || {}; // merge this playlist into the master update = updateMaster(loader.master, parser.manifest); @@ -250,12 +253,7 @@ const PlaylistLoader = function(srcUrl, hls, withCredentials) { return false; } - let bandwidth = 0; - - if (playlist && playlist.attributes) { - bandwidth = playlist.attributes.BANDWIDTH; - } - return bandwidth < currentBandwidth; + return (playlist.attributes.BANDWIDTH || 0) < currentBandwidth; }).length === 0); }; @@ -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 + // property is added here to prevent undefined references when we encounter + // this scenario. + playlist.attributes = playlist.attributes || {}; } // resolve any media group URIs @@ -552,6 +556,9 @@ const PlaylistLoader = function(srcUrl, hls, withCredentials) { }; loader.master.playlists[srcUrl] = loader.master.playlists[0]; loader.master.playlists[0].resolvedUri = srcUrl; + // m3u8-parser does not attach an attributes property to media playlists so make + // sure that the property is attached to avoid undefined reference errors + loader.master.playlists[0].attributes = loader.master.playlists[0].attributes || {}; haveMetadata(req, srcUrl); return loader.trigger('loadedmetadata'); }); diff --git a/src/playlist-selectors.js b/src/playlist-selectors.js index 888ec13b1..04f20d9ac 100644 --- a/src/playlist-selectors.js +++ b/src/playlist-selectors.js @@ -62,11 +62,11 @@ export const comparePlaylistBandwidth = function(left, right) { let leftBandwidth; let rightBandwidth; - if (left.attributes && left.attributes.BANDWIDTH) { + if (left.attributes.BANDWIDTH) { leftBandwidth = left.attributes.BANDWIDTH; } leftBandwidth = leftBandwidth || window.Number.MAX_VALUE; - if (right.attributes && right.attributes.BANDWIDTH) { + if (right.attributes.BANDWIDTH) { rightBandwidth = right.attributes.BANDWIDTH; } rightBandwidth = rightBandwidth || window.Number.MAX_VALUE; @@ -87,16 +87,14 @@ export const comparePlaylistResolution = function(left, right) { let leftWidth; let rightWidth; - if (left.attributes && - left.attributes.RESOLUTION && + if (left.attributes.RESOLUTION && left.attributes.RESOLUTION.width) { leftWidth = left.attributes.RESOLUTION.width; } leftWidth = leftWidth || window.Number.MAX_VALUE; - if (right.attributes && - right.attributes.RESOLUTION && + if (right.attributes.RESOLUTION && right.attributes.RESOLUTION.width) { rightWidth = right.attributes.RESOLUTION.width; } @@ -135,11 +133,9 @@ const simpleSelector = function(master, playerBandwidth, playerWidth, playerHeig let height; let bandwidth; - if (playlist.attributes) { - width = playlist.attributes.RESOLUTION && playlist.attributes.RESOLUTION.width; - height = playlist.attributes.RESOLUTION && playlist.attributes.RESOLUTION.height; - bandwidth = playlist.attributes.BANDWIDTH; - } + width = playlist.attributes.RESOLUTION && playlist.attributes.RESOLUTION.width; + height = playlist.attributes.RESOLUTION && playlist.attributes.RESOLUTION.height; + bandwidth = playlist.attributes.BANDWIDTH; bandwidth = bandwidth || window.Number.MAX_VALUE; diff --git a/src/rendition-mixin.js b/src/rendition-mixin.js index 4164f84ce..8d9afc20d 100644 --- a/src/rendition-mixin.js +++ b/src/rendition-mixin.js @@ -54,21 +54,16 @@ class Representation { .fastQualityChange_ .bind(hlsHandler.masterPlaylistController_); - // Carefully descend into the playlist's attributes since most - // properties are optional - if (playlist.attributes) { - let attributes = playlist.attributes; + // some playlist attributes are optional + if (playlist.attributes.RESOLUTION) { + let resolution = playlist.attributes.RESOLUTION; - if (attributes.RESOLUTION) { - let resolution = attributes.RESOLUTION; - - this.width = resolution.width; - this.height = resolution.height; - } - - this.bandwidth = attributes.BANDWIDTH; + this.width = resolution.width; + this.height = resolution.height; } + this.bandwidth = playlist.attributes.BANDWIDTH; + // The id is simply the ordinality of the media playlist // within the master playlist this.id = id; diff --git a/src/segment-loader.js b/src/segment-loader.js index 3fc9b7c9f..76c026676 100644 --- a/src/segment-loader.js +++ b/src/segment-loader.js @@ -753,7 +753,7 @@ export default class SegmentLoader extends videojs.EventTarget { // the lowestEnabledRendition. !this.xhrOptions_.timeout || // Don't abort if we have no bandwidth information to estimate segment sizes - !(this.playlist_.attributes && this.playlist_.attributes.BANDWIDTH)) { + !(this.playlist_.attributes.BANDWIDTH)) { return false; } diff --git a/test/playlist-loader.test.js b/test/playlist-loader.test.js index 8078c57e0..dd35f1b0f 100644 --- a/test/playlist-loader.test.js +++ b/test/playlist-loader.test.js @@ -91,6 +91,7 @@ function(assert) { assert.ok(loader.master, 'infers a master playlist'); assert.ok(loader.media(), 'sets the media playlist'); assert.ok(loader.media().uri, 'sets the media playlist URI'); + assert.ok(loader.media().attributes, 'sets the media playlist attributes'); assert.strictEqual(loader.state, 'HAVE_METADATA', 'the state is correct'); assert.strictEqual(this.requests.length, 0, 'no more requests are made'); assert.strictEqual(loadedmetadatas, 1, 'fired one loadedmetadata'); @@ -318,6 +319,7 @@ function(assert) { '0.ts\n'); assert.ok(loader.master, 'infers a master playlist'); assert.ok(loader.media(), 'sets the media playlist'); + assert.ok(loader.media().attributes, 'sets the media playlist attributes'); assert.strictEqual(loader.state, 'HAVE_METADATA', 'the state is correct'); }); diff --git a/test/rendition-mixin.test.js b/test/rendition-mixin.test.js index e734a553b..7bb581992 100644 --- a/test/rendition-mixin.test.js +++ b/test/rendition-mixin.test.js @@ -8,24 +8,19 @@ const makeMockPlaylist = function(options) { options = options || {}; let playlist = { - segments: [] + segments: [], + attributes: {} }; - if ('bandwidth' in options) { - playlist.attributes = playlist.attributes || {}; - - playlist.attributes.BANDWIDTH = options.bandwidth; - } + playlist.attributes.BANDWIDTH = options.bandwidth; if ('width' in options) { - playlist.attributes = playlist.attributes || {}; playlist.attributes.RESOLUTION = playlist.attributes.RESOLUTION || {}; playlist.attributes.RESOLUTION.width = options.width; } if ('height' in options) { - playlist.attributes = playlist.attributes || {}; playlist.attributes.RESOLUTION = playlist.attributes.RESOLUTION || {}; playlist.attributes.RESOLUTION.height = options.height; From b7adc8505d3edf33027e1a49f6d99af16ee180b9 Mon Sep 17 00:00:00 2001 From: Matthew Neil Date: Fri, 28 Jul 2017 19:01:52 -0400 Subject: [PATCH 2/5] add warning log when missing attribute for stream-inf --- src/playlist-loader.js | 19 ++++++++++++------- test/playlist-loader.test.js | 21 +++++++++++---------- 2 files changed, 23 insertions(+), 17 deletions(-) diff --git a/src/playlist-loader.js b/src/playlist-loader.js index e219d4d64..fd694fc4e 100644 --- a/src/playlist-loader.js +++ b/src/playlist-loader.js @@ -6,7 +6,7 @@ * */ import resolveUrl from './resolve-url'; -import { mergeOptions, EventTarget } from 'video.js'; +import { mergeOptions, EventTarget, log } from 'video.js'; import { isEnabled } from './playlist.js'; import m3u8 from 'm3u8-parser'; import window from 'global/window'; @@ -508,12 +508,17 @@ 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 - // property is added here to prevent undefined references when we encounter - // this scenario. - playlist.attributes = playlist.attributes || {}; + + if (!playlist.attributes) { + // 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 + // formatted 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 = {}; + + log.warn('Invalid playlist STREAM-INF detected. Missing attribute list.'); + } } // resolve any media group URIs diff --git a/test/playlist-loader.test.js b/test/playlist-loader.test.js index dd35f1b0f..ea7008145 100644 --- a/test/playlist-loader.test.js +++ b/test/playlist-loader.test.js @@ -67,7 +67,7 @@ QUnit.test('moves to HAVE_MASTER after loading a master playlist', function(asse }); this.requests.pop().respond(200, null, '#EXTM3U\n' + - '#EXT-X-STREAM-INF:\n' + + '#EXT-X-STREAM-INF:BANDWIDTH=1\n' + 'media.m3u8\n'); assert.ok(loader.master, 'the master playlist is available'); assert.strictEqual(state, 'HAVE_MASTER', 'the state at loadedplaylist correct'); @@ -104,7 +104,7 @@ QUnit.test('resolves relative media playlist URIs', function(assert) { this.requests.shift().respond(200, null, '#EXTM3U\n' + - '#EXT-X-STREAM-INF:\n' + + '#EXT-X-STREAM-INF:BANDWIDTH=1\n' + 'video/media.m3u8\n'); assert.equal(loader.master.playlists[0].resolvedUri, urlTo('video/media.m3u8'), 'resolved media URI'); @@ -118,9 +118,9 @@ function(assert) { this.requests.shift().respond(200, null, '#EXTM3U\n' + - '#EXT-X-STREAM-INF:\n' + + '#EXT-X-STREAM-INF:BANDWIDTH=1\n' + 'video1/media.m3u8\n' + - '#EXT-X-STREAM-INF:\n' + + '#EXT-X-STREAM-INF:BANDWIDTH=1\n' + 'video2/media.m3u8\n'); assert.equal(loader.enabledPlaylists_(), 2, 'Returned initial amount of playlists'); loader.master.playlists[0].excludeUntil = Date.now() + 100000; @@ -134,9 +134,9 @@ QUnit.test('playlist loader detects if we are on lowest rendition', function(ass loader.load(); this.requests.shift().respond(200, null, '#EXTM3U\n' + - '#EXT-X-STREAM-INF:\n' + + '#EXT-X-STREAM-INF:BANDWIDTH=1\n' + 'video1/media.m3u8\n' + - '#EXT-X-STREAM-INF:\n' + + '#EXT-X-STREAM-INF:BANDWIDTH=1\n' + 'video2/media.m3u8\n'); loader.media = function() { return {attributes: {BANDWIDTH: 10}}; @@ -183,7 +183,7 @@ QUnit.test('recognizes absolute URIs and requests them unmodified', function(ass this.requests.shift().respond(200, null, '#EXTM3U\n' + - '#EXT-X-STREAM-INF:\n' + + '#EXT-X-STREAM-INF:BANDWIDTH=1\n' + 'http://example.com/video/media.m3u8\n'); assert.equal(loader.master.playlists[0].resolvedUri, 'http://example.com/video/media.m3u8', 'resolved media URI'); @@ -204,7 +204,7 @@ QUnit.test('recognizes domain-relative URLs', function(assert) { this.requests.shift().respond(200, null, '#EXTM3U\n' + - '#EXT-X-STREAM-INF:\n' + + '#EXT-X-STREAM-INF:BANDWIDTH=1\n' + '/media.m3u8\n'); assert.equal(loader.master.playlists[0].resolvedUri, window.location.protocol + '//' + @@ -338,8 +338,9 @@ QUnit.test('moves to HAVE_METADATA after loading a media playlist', function(ass }); this.requests.pop().respond(200, null, '#EXTM3U\n' + - '#EXT-X-STREAM-INF:\n' + + '#EXT-X-STREAM-INF:BANDWIDTH=1\n' + 'media.m3u8\n' + + '#EXT-X-STREAM-INF:BANDWIDTH=1\n' + 'alt.m3u8\n'); assert.strictEqual(loadedPlaylist, 1, 'fired loadedplaylist once'); assert.strictEqual(loadedMetadata, 0, 'did not fire loadedmetadata'); @@ -438,7 +439,7 @@ QUnit.test('errors when an initial media playlist request fails', function(asser }); this.requests.pop().respond(200, null, '#EXTM3U\n' + - '#EXT-X-STREAM-INF:\n' + + '#EXT-X-STREAM-INF:BANDWIDTH=1\n' + 'media.m3u8\n'); assert.strictEqual(errors.length, 0, 'emitted no errors'); From cdb41bb268fdbcc201c190c0bffb755d642db72e Mon Sep 17 00:00:00 2001 From: Matthew Neil Date: Tue, 1 Aug 2017 14:51:22 -0400 Subject: [PATCH 3/5] minor syntax changes --- src/playlist-loader.js | 3 ++- src/rendition-mixin.js | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/playlist-loader.js b/src/playlist-loader.js index fd694fc4e..25cdb904b 100644 --- a/src/playlist-loader.js +++ b/src/playlist-loader.js @@ -517,7 +517,8 @@ const PlaylistLoader = function(srcUrl, hls, withCredentials) { // this scenario. playlist.attributes = {}; - log.warn('Invalid playlist STREAM-INF detected. Missing attribute list.'); + log.warn( + 'Invalid playlist STREAM-INF detected. Missing BANDWIDTH attribute.'); } } diff --git a/src/rendition-mixin.js b/src/rendition-mixin.js index 8d9afc20d..723106391 100644 --- a/src/rendition-mixin.js +++ b/src/rendition-mixin.js @@ -56,7 +56,7 @@ class Representation { // some playlist attributes are optional if (playlist.attributes.RESOLUTION) { - let resolution = playlist.attributes.RESOLUTION; + const resolution = playlist.attributes.RESOLUTION; this.width = resolution.width; this.height = resolution.height; From 412456de7144411560e42197b04d06edb5d8ae38 Mon Sep 17 00:00:00 2001 From: Matthew Neil Date: Tue, 1 Aug 2017 17:16:41 -0400 Subject: [PATCH 4/5] add unit test --- test/playlist-loader.test.js | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/test/playlist-loader.test.js b/test/playlist-loader.test.js index ea7008145..cdcf09261 100644 --- a/test/playlist-loader.test.js +++ b/test/playlist-loader.test.js @@ -73,6 +73,24 @@ QUnit.test('moves to HAVE_MASTER after loading a master playlist', function(asse assert.strictEqual(state, 'HAVE_MASTER', 'the state at loadedplaylist correct'); }); +QUnit.test('logs warning for master playlist with invalid STREAM-INF', function(assert) { + let loader = new PlaylistLoader('master.m3u8', this.fakeHls); + + loader.load(); + + this.requests.pop().respond(200, null, + '#EXTM3U\n' + + '#EXT-X-STREAM-INF:BANDWIDTH=1\n' + + 'video1/media.m3u8\n' + + '#EXT-X-STREAM-INF:\n' + + 'video2/media.m3u8\n'); + + 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'); +}); + QUnit.test('jumps to HAVE_METADATA when initialized with a media playlist', function(assert) { let loadedmetadatas = 0; From a3ab773c4a4f82c58931f6aedc87c6dd554e78ba Mon Sep 17 00:00:00 2001 From: Matthew Neil Date: Tue, 1 Aug 2017 17:31:21 -0400 Subject: [PATCH 5/5] update test --- test/playlist-loader.test.js | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/test/playlist-loader.test.js b/test/playlist-loader.test.js index cdcf09261..af2bf0d99 100644 --- a/test/playlist-loader.test.js +++ b/test/playlist-loader.test.js @@ -86,9 +86,13 @@ QUnit.test('logs warning for master playlist with invalid STREAM-INF', function( 'video2/media.m3u8\n'); 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(loader.master.playlists[1].uri, 'video2/media.m3u8', + 'parsed invalid stream'); + assert.ok(loader.master.playlists[1].attributes, 'attached attributes property'); assert.equal(this.env.log.warn.calls, 1, 'logged a warning'); + assert.equal(this.env.log.warn.args[0], + 'Invalid playlist STREAM-INF detected. Missing BANDWIDTH attribute.', + 'logged a warning'); }); QUnit.test('jumps to HAVE_METADATA when initialized with a media playlist',