From fe8376bc3fc5c8f3e8058d2a53fa0fc5db8a26d7 Mon Sep 17 00:00:00 2001 From: Gary Katsevman Date: Fri, 25 Sep 2020 13:25:51 -0400 Subject: [PATCH] revert: fix: use playlist NAME when available as its ID (#929) (#957) This reverts commit 22694642fc53815f441560f14e36d74232fca9f3. In testing, we noticed that the Angel One DASH video is broken, so we need to revert this change and fix it later. --- src/dash-playlist-loader.js | 2 +- src/manifest.js | 25 +++---------- test/dash-playlist-loader.test.js | 58 ++----------------------------- test/manifests/dash-swapped.mpd | 24 ------------- 4 files changed, 8 insertions(+), 101 deletions(-) delete mode 100644 test/manifests/dash-swapped.mpd diff --git a/src/dash-playlist-loader.js b/src/dash-playlist-loader.js index 6fea8c125..39550cb7a 100644 --- a/src/dash-playlist-loader.js +++ b/src/dash-playlist-loader.js @@ -43,7 +43,7 @@ export const parseMasterXml = ({ masterXml, srcUrl, clientOffset, sidxMapping }) sidxMapping }); - addPropertiesToMaster(master, srcUrl, true); + addPropertiesToMaster(master, srcUrl); return master; }; diff --git a/src/manifest.js b/src/manifest.js index 29f1ec525..3d5423756 100644 --- a/src/manifest.js +++ b/src/manifest.js @@ -99,32 +99,18 @@ export const setupMediaPlaylist = ({ playlist, uri, id }) => { * * @param {Object} master * The master playlist - * @param {boolean} [useNameForId=false] - * Whether we should use the NAME property for ID. - * Generally only used for DASH and defaults to false. */ -export const setupMediaPlaylists = (master, useNameForId) => { +export const setupMediaPlaylists = (master) => { let i = master.playlists.length; while (i--) { const playlist = master.playlists[i]; - const createdId = createPlaylistID(i, playlist.uri); - let id = createdId; - - // If useNameForId is set, use the NAME attribute for the ID. - // Generally, this will be used for DASH because - // DASH Representations can change order across refreshes which can make referring to them by index not work. - if (useNameForId) { - id = playlist.attributes && playlist.attributes.NAME || id; - } setupMediaPlaylist({ playlist, - id + id: createPlaylistID(i, playlist.uri) }); playlist.resolvedUri = resolveUrl(master.uri, playlist.uri); - // make sure that if a useNameForId is true, the old "createdId" id is also available - master.playlists[createdId] = playlist; master.playlists[playlist.id] = playlist; // URI reference added for backwards compatibility master.playlists[playlist.uri] = playlist; @@ -201,11 +187,8 @@ export const masterForMedia = (media, uri) => { * Master manifest object * @param {string} uri * The source URI - * @param {boolean} [useNameForId=false] - * Whether we should use the NAME property for ID. - * Generally only used for DASH and defaults to false. */ -export const addPropertiesToMaster = (master, uri, useNameForId = false) => { +export const addPropertiesToMaster = (master, uri) => { master.uri = uri; for (let i = 0; i < master.playlists.length; i++) { @@ -238,6 +221,6 @@ export const addPropertiesToMaster = (master, uri, useNameForId = false) => { master.playlists[phonyUri] = properties.playlists[0]; }); - setupMediaPlaylists(master, useNameForId); + setupMediaPlaylists(master); resolveMediaGroupUris(master); }; diff --git a/test/dash-playlist-loader.test.js b/test/dash-playlist-loader.test.js index 06e2d50f7..bf53eae5a 100644 --- a/test/dash-playlist-loader.test.js +++ b/test/dash-playlist-loader.test.js @@ -1361,12 +1361,7 @@ QUnit.test('parseMasterXml: setup phony playlists and resolves uris', function(a assert.strictEqual(masterPlaylist.uri, loader.srcUrl, 'master playlist uri set correctly'); assert.strictEqual(masterPlaylist.playlists[0].uri, 'placeholder-uri-0'); - assert.strictEqual(masterPlaylist.playlists[0].id, '1080p'); - assert.strictEqual( - masterPlaylist.playlists['1080p'], - masterPlaylist.playlists['0-placeholder-uri-0'], - 'available via old and new ids' - ); + assert.strictEqual(masterPlaylist.playlists[0].id, '0-placeholder-uri-0'); assert.deepEqual( masterPlaylist.playlists['0-placeholder-uri-0'], masterPlaylist.playlists[0], @@ -1692,43 +1687,6 @@ QUnit.test('refreshXml_: updates media playlist reference if master changed', fu ); }); -QUnit.test('refreshXml_: keep reference to same playlist by id across mpd updates', function(assert) { - const loader = new DashPlaylistLoader('dash.mpd', this.fakeVhs); - - loader.load(); - this.standardXHRResponse(this.requests.shift()); - - const oldMaster = loader.master; - const oldMedia = loader.media(); - - loader.refreshXml_(); - - assert.strictEqual(this.requests.length, 1, 'manifest is being requested'); - - this.requests.shift().respond(200, null, testDataManifests['dash-swapped']); - - const newMaster = loader.master; - const newMedia = loader.media(); - - assert.notEqual(newMaster, oldMaster, 'master changed'); - assert.notEqual(newMedia, oldMedia, 'media changed'); - assert.equal( - newMedia, - newMaster.playlists[newMedia.id], - 'media from updated master' - ); - - // given that the only thing that changed in the new manifest is - // the mediaPresentationDuration and order of representations - // the old media and the new media should be equivalent. - // Comparing the attributes is an easy way to check. - assert.deepEqual( - newMedia.attributes, - oldMedia.attributes, - 'old media and new media references by same id' - ); -}); - QUnit.test('sidxRequestFinished_: updates master with sidx information', function(assert) { const loader = new DashPlaylistLoader('dash.mpd', this.fakeVhs); const fakePlaylist = { @@ -2366,14 +2324,9 @@ QUnit.test( 'setup phony uri for media playlist' ); assert.equal( - loader.master.playlists[0].id, '1080p', + loader.master.playlists[0].id, '0-placeholder-uri-0', 'setup phony id for media playlist' ); - assert.strictEqual( - loader.master.playlists['1080p'], - loader.master.playlists['0-placeholder-uri-0'], - 'reference by NAME and old id' - ); assert.strictEqual( loader.master.playlists['0-placeholder-uri-0'], loader.master.playlists[0], 'set reference by uri for easy access' @@ -2383,14 +2336,9 @@ QUnit.test( 'setup phony uri for media playlist' ); assert.equal( - loader.master.playlists[1].id, '720p', + loader.master.playlists[1].id, '1-placeholder-uri-1', 'setup phony id for media playlist' ); - assert.strictEqual( - loader.master.playlists['720p'], - loader.master.playlists['1-placeholder-uri-1'], - 'reference by NAME and old id' - ); assert.strictEqual( loader.master.playlists['1-placeholder-uri-1'], loader.master.playlists[1], 'set reference by uri for easy access' diff --git a/test/manifests/dash-swapped.mpd b/test/manifests/dash-swapped.mpd deleted file mode 100644 index f6ee7fa3b..000000000 --- a/test/manifests/dash-swapped.mpd +++ /dev/null @@ -1,24 +0,0 @@ - - - - main/ - - video/ - - 720/ - - - - 1080/ - - - - - audio/ - - 720/ - - - - -