Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: Add a better distinction between master and child dash loaders #992

Merged
merged 4 commits into from
Nov 19, 2020
Merged

refactor: Add a better distinction between master and child dash loaders #992

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a57e6ea
refactor: Add a better distinction between master and child dash loaders
brandonocasey Nov 10, 2020
1ece93f
migrate all shared properties over
brandonocasey Nov 10, 2020
186c2bf
update tests
brandonocasey Nov 10, 2020
5038bbf
Update test/dash-playlist-loader.test.js
brandonocasey Nov 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
131 changes: 52 additions & 79 deletions src/dash-playlist-loader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,11 @@ export default class DashPlaylistLoader extends EventTarget {
constructor(srcUrlOrPlaylist, vhs, options = { }, masterPlaylistLoader) {
super();

if (!masterPlaylistLoader) {
this.masterPlaylistLoader_ = this;
this.isMaster_ = true;
}

const { withCredentials = false, handleManifestRedirects = false } = options;

this.vhs_ = vhs;
Expand All @@ -277,20 +282,15 @@ export default class DashPlaylistLoader extends EventTarget {

// initialize the loader state
// The masterPlaylistLoader will be created with a string
if (typeof srcUrlOrPlaylist === 'string') {
if (this.isMaster_) {
Comment on lines -280 to +285
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we verify that srcUrlOrPlaylist is a string or is it basically guaranteed to be one if we're a master manifest?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For master it will always be a string, for others it can be a playlist or string. So yes it would seem so.

this.srcUrl = srcUrlOrPlaylist;
// TODO: reset sidxMapping between period changes
// once multi-period is refactored
this.sidxMapping_ = {};
return;
this.masterPlaylistLoader_.sidxMapping_ = {};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we have the conditional above for master loaders, would it be clearer to use just this?

} else {
this.masterPlaylistLoader_ = masterPlaylistLoader;
this.childPlaylist_ = srcUrlOrPlaylist;
}

this.setupChildLoader(masterPlaylistLoader, srcUrlOrPlaylist);
}

setupChildLoader(masterPlaylistLoader, playlist) {
this.masterPlaylistLoader_ = masterPlaylistLoader;
this.childPlaylist_ = playlist;
}

dispose() {
Expand Down Expand Up @@ -414,20 +414,10 @@ export default class DashPlaylistLoader extends EventTarget {
return;
}

// we have sidx mappings
let oldMaster;
let sidxMapping;

// sidxMapping is used when parsing the masterXml, so store
// it on the masterPlaylistLoader
if (this.masterPlaylistLoader_) {
oldMaster = this.masterPlaylistLoader_.master;
sidxMapping = this.masterPlaylistLoader_.sidxMapping_;
} else {
oldMaster = this.master;
sidxMapping = this.sidxMapping_;
}

const oldMaster = this.masterPlaylistLoader_.master;
const sidxMapping = this.masterPlaylistLoader_.sidxMapping_;
const sidxKey = generateSidxKey(playlist.sidx);

sidxMapping[sidxKey] = {
Expand Down Expand Up @@ -518,7 +508,7 @@ export default class DashPlaylistLoader extends EventTarget {

// We don't need to request the master manifest again
// Call this asynchronously to match the xhr request behavior below
if (this.masterPlaylistLoader_) {
if (!this.isMaster_) {
this.mediaRequest_ = window.setTimeout(
this.haveMaster_.bind(this),
0
Expand All @@ -528,7 +518,7 @@ export default class DashPlaylistLoader extends EventTarget {

// request the specified URL
this.request = this.vhs_.xhr({
uri: this.srcUrl,
uri: this.masterPlaylistLoader_.srcUrl,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we return early above for non master loaders, would it be clearer to use just this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we want it to be like this because:

  1. The distinction will help us migrate master playlist loader code into a new file after these refactors.
  2. It's much easier to know that you are accessing the correct object if you always access it the same way.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. Thanks!

withCredentials: this.withCredentials
}, (error, req) => {
// disposed
Expand All @@ -542,7 +532,7 @@ export default class DashPlaylistLoader extends EventTarget {
if (error) {
this.error = {
status: req.status,
message: 'DASH playlist request error at URL: ' + this.srcUrl,
message: 'DASH playlist request error at URL: ' + this.masterPlaylistLoader_.srcUrl,
responseText: req.responseText,
// MEDIA_ERR_NETWORK
code: 2
Expand All @@ -553,15 +543,15 @@ export default class DashPlaylistLoader extends EventTarget {
return this.trigger('error');
}

this.masterXml_ = req.responseText;
this.masterPlaylistLoader_.masterXml_ = req.responseText;

if (req.responseHeaders && req.responseHeaders.date) {
this.masterLoaded_ = Date.parse(req.responseHeaders.date);
} else {
this.masterLoaded_ = Date.now();
}

this.srcUrl = resolveManifestRedirect(this.handleManifestRedirects, this.srcUrl, req);
this.masterPlaylistLoader_.srcUrl = resolveManifestRedirect(this.handleManifestRedirects, this.masterPlaylistLoader_.srcUrl, req);

this.syncClientServerClock_(this.onClientServerClockSync_.bind(this));
});
Expand All @@ -575,22 +565,22 @@ export default class DashPlaylistLoader extends EventTarget {
* Function to call when clock sync has completed
*/
syncClientServerClock_(done) {
const utcTiming = parseUTCTiming(this.masterXml_);
const utcTiming = parseUTCTiming(this.masterPlaylistLoader_.masterXml_);

// No UTCTiming element found in the mpd. Use Date header from mpd request as the
// server clock
if (utcTiming === null) {
this.clientOffset_ = this.masterLoaded_ - Date.now();
this.masterPlaylistLoader_.clientOffset_ = this.masterLoaded_ - Date.now();
return done();
}

if (utcTiming.method === 'DIRECT') {
this.clientOffset_ = utcTiming.value - Date.now();
this.masterPlaylistLoader_.clientOffset_ = utcTiming.value - Date.now();
return done();
}

this.request = this.vhs_.xhr({
uri: resolveUrl(this.srcUrl, utcTiming.value),
uri: resolveUrl(this.masterPlaylistLoader_.srcUrl, utcTiming.value),
method: utcTiming.method,
withCredentials: this.withCredentials
}, (error, req) => {
Expand All @@ -602,7 +592,7 @@ export default class DashPlaylistLoader extends EventTarget {
if (error) {
// sync request failed, fall back to using date header from mpd
// TODO: log warning
this.clientOffset_ = this.masterLoaded_ - Date.now();
this.masterPlaylistLoader_.clientOffset_ = this.masterLoaded_ - Date.now();
return done();
}

Expand All @@ -620,7 +610,7 @@ export default class DashPlaylistLoader extends EventTarget {
serverTime = Date.parse(req.responseText);
}

this.clientOffset_ = serverTime - Date.now();
this.masterPlaylistLoader_.clientOffset_ = serverTime - Date.now();

done();
});
Expand All @@ -631,12 +621,12 @@ export default class DashPlaylistLoader extends EventTarget {
// clear media request
this.mediaRequest_ = null;

if (!this.masterPlaylistLoader_) {
if (this.isMaster_) {
this.updateMainManifest_(parseMasterXml({
masterXml: this.masterXml_,
srcUrl: this.srcUrl,
clientOffset: this.clientOffset_,
sidxMapping: this.sidxMapping_
masterXml: this.masterPlaylistLoader_.masterXml_,
srcUrl: this.masterPlaylistLoader_.srcUrl,
clientOffset: this.masterPlaylistLoader_.clientOffset_,
sidxMapping: this.masterPlaylistLoader_.sidxMapping_
Comment on lines +626 to +629
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question re: above

}));
// We have the master playlist at this point, so
// trigger this to allow MasterPlaylistController
Expand Down Expand Up @@ -710,8 +700,8 @@ export default class DashPlaylistLoader extends EventTarget {

const location = this.master.locations[0];

if (location !== this.srcUrl) {
this.srcUrl = location;
if (location !== this.masterPlaylistLoader_.srcUrl) {
this.masterPlaylistLoader_.srcUrl = location;
}
}

Expand All @@ -723,7 +713,7 @@ export default class DashPlaylistLoader extends EventTarget {
// The srcUrl here *may* need to pass through handleManifestsRedirects when
// sidx is implemented
this.request = this.vhs_.xhr({
uri: this.srcUrl,
uri: this.masterPlaylistLoader_.srcUrl,
withCredentials: this.withCredentials
}, (error, req) => {
// disposed
Expand All @@ -737,7 +727,7 @@ export default class DashPlaylistLoader extends EventTarget {
if (error) {
this.error = {
status: req.status,
message: 'DASH playlist request error at URL: ' + this.srcUrl,
message: 'DASH playlist request error at URL: ' + this.masterPlaylistLoader_.srcUrl,
responseText: req.responseText,
// MEDIA_ERR_NETWORK
code: 2
Expand All @@ -748,21 +738,21 @@ export default class DashPlaylistLoader extends EventTarget {
return this.trigger('error');
}

this.masterXml_ = req.responseText;
this.masterPlaylistLoader_.masterXml_ = req.responseText;

// This will filter out updated sidx info from the mapping
this.sidxMapping_ = filterChangedSidxMappings(
this.masterXml_,
this.srcUrl,
this.clientOffset_,
this.sidxMapping_
this.masterPlaylistLoader_.sidxMapping_ = filterChangedSidxMappings(
this.masterPlaylistLoader_.masterXml_,
this.masterPlaylistLoader_.srcUrl,
this.masterPlaylistLoader_.clientOffset_,
this.masterPlaylistLoader_.sidxMapping_
);

const master = parseMasterXml({
masterXml: this.masterXml_,
srcUrl: this.srcUrl,
clientOffset: this.clientOffset_,
sidxMapping: this.sidxMapping_
masterXml: this.masterPlaylistLoader_.masterXml_,
srcUrl: this.masterPlaylistLoader_.srcUrl,
clientOffset: this.masterPlaylistLoader_.clientOffset_,
sidxMapping: this.masterPlaylistLoader_.sidxMapping_
});
const updatedMaster = updateMaster(this.master, master);
const currentSidxInfo = this.media().sidx;
Expand All @@ -772,7 +762,7 @@ export default class DashPlaylistLoader extends EventTarget {
const sidxKey = generateSidxKey(currentSidxInfo);

// the sidx was updated, so the previous mapping was removed
if (!this.sidxMapping_[sidxKey]) {
if (!this.masterPlaylistLoader_.sidxMapping_[sidxKey]) {
const playlist = this.media();

this.request = requestSidx_(
Expand All @@ -787,7 +777,7 @@ export default class DashPlaylistLoader extends EventTarget {
}

// update loader's sidxMapping with parsed sidx box
this.sidxMapping_[sidxKey].sidx = sidx;
this.masterPlaylistLoader_.sidxMapping_[sidxKey].sidx = sidx;

this.updateMinimumUpdatePeriodTimeout_();

Expand Down Expand Up @@ -820,35 +810,18 @@ export default class DashPlaylistLoader extends EventTarget {
throw new Error('refreshMedia_ must take a media id');
}

let oldMaster;
let newMaster;

if (this.masterPlaylistLoader_) {
oldMaster = this.masterPlaylistLoader_.master;
newMaster = parseMasterXml({
masterXml: this.masterPlaylistLoader_.masterXml_,
srcUrl: this.masterPlaylistLoader_.srcUrl,
clientOffset: this.masterPlaylistLoader_.clientOffset_,
sidxMapping: this.masterPlaylistLoader_.sidxMapping_
});
} else {
oldMaster = this.master;
newMaster = parseMasterXml({
masterXml: this.masterXml_,
srcUrl: this.srcUrl,
clientOffset: this.clientOffset_,
sidxMapping: this.sidxMapping_
});
}
const oldMaster = this.masterPlaylistLoader_.master;
const newMaster = parseMasterXml({
masterXml: this.masterPlaylistLoader_.masterXml_,
srcUrl: this.masterPlaylistLoader_.srcUrl,
clientOffset: this.masterPlaylistLoader_.clientOffset_,
sidxMapping: this.masterPlaylistLoader_.sidxMapping_
});

const updatedMaster = updateMaster(oldMaster, newMaster);

if (updatedMaster) {
if (this.masterPlaylistLoader_) {
this.masterPlaylistLoader_.master = updatedMaster;
} else {
this.master = updatedMaster;
}
this.masterPlaylistLoader_.master = updatedMaster;
this.media_ = updatedMaster.playlists[mediaID];
} else {
this.media_ = oldMaster.playlists[mediaID];
Expand Down
37 changes: 4 additions & 33 deletions test/dash-playlist-loader.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -658,7 +658,8 @@ QUnit.test('constructor sets srcUrl and other properties', function(assert) {

assert.strictEqual(loader.state, 'HAVE_NOTHING', 'correct state');
assert.deepEqual(loader.loadedPlaylists_, {}, 'correct loadedPlaylist state');
assert.notOk(loader.masterPlaylistLoader_, 'should be no masterPlaylistLoader');
assert.equal(loader.masterPlaylistLoader_, loader, 'masterPlaylistLoader should be self');
assert.ok(loader.isMaster_, 'should be set as master');
assert.notOk(loader.childPlaylist_, 'should be no childPlaylist_');
assert.strictEqual(loader.srcUrl, 'dash.mpd', 'set the srcUrl');

Expand All @@ -667,6 +668,8 @@ QUnit.test('constructor sets srcUrl and other properties', function(assert) {
assert.strictEqual(childLoader.state, 'HAVE_NOTHING', 'correct state');
assert.deepEqual(childLoader.loadedPlaylists_, {}, 'correct loadedPlaylist state');
assert.ok(childLoader.masterPlaylistLoader_, 'should be a masterPlaylistLoader');
assert.notEqual(childLoader.masterPlaylistLoader_, childLoader, 'should not be a masterPlaylistLoader');
assert.notOk(childLoader.isMaster_, 'should not be master');
assert.deepEqual(
childLoader.childPlaylist_, {},
'should be a childPlaylist_'
Expand Down Expand Up @@ -1842,38 +1845,6 @@ QUnit.test('sidxRequestFinished_: uses given error object', function(assert) {
assert.strictEqual(errors, 1, 'triggered an error event');
});

QUnit.test('setupChildLoader: sets masterPlaylistLoader and ' +
'playlist on child loader', function(assert) {
const fakePlaylist = { uri: 'fakeplaylist1', id: 'fakeplaylist1' };
const newPlaylist = { uri: 'fakeplaylist2', id: 'fakeplaylist2' };
const loader = new DashPlaylistLoader('dash.mpd', this.fakeVhs);
const newLoader = new DashPlaylistLoader('dash-sidx.mpd', this.fakeVhs);
const childLoader = new DashPlaylistLoader(fakePlaylist, this.fakeVhs, false, loader);

assert.deepEqual(
childLoader.masterPlaylistLoader_,
loader,
'starts with loader passed into constructor'
);
assert.deepEqual(
childLoader.childPlaylist_,
fakePlaylist,
'starts with playlist passed in constructor'
);

childLoader.setupChildLoader(newLoader, newPlaylist);
assert.deepEqual(
childLoader.masterPlaylistLoader_,
newLoader,
'masterPlaylistLoader correctly set'
);
assert.deepEqual(
childLoader.childPlaylist_,
newPlaylist,
'child playlist correctly set'
);
});

QUnit.test('hasPendingRequest: returns true if async code is running in master loader', function(assert) {
const loader = new DashPlaylistLoader('dash.mpd', this.fakeVhs);

Expand Down