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.

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

fix: do not request manifests until play when preload is none #1060

Merged
merged 2 commits into from
Feb 9, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 10 additions & 0 deletions index.html
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@
input[type=url], select {
min-width: 600px;
}
#preload {
min-width: auto;
}
h3 {
margin-bottom: 5px;
}
Expand Down Expand Up @@ -85,6 +88,13 @@ <h3>Options</h3>
<input id=override-native type="checkbox" checked>
Override Native (reloads player)
</label>
<label>
Copy link
Member

Choose a reason for hiding this comment

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

awesome

Preload (reloads player)
<select id=preload>
<option selected>auto</option>
<option>none</option>
<option>metadata</option>
</select>
</div>

<h3>Load a URL</h3>
Expand Down
17 changes: 16 additions & 1 deletion scripts/index-demo-page.js
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,8 @@
var getInputValue = function(el) {
if (el.type === 'url' || el.type === 'text' || el.nodeName.toLowerCase() === 'textarea') {
return encodeURIComponent(el.value);
} else if (el.type === 'select-one') {
return el.options[el.selectedIndex].value;
} else if (el.type === 'checkbox') {
return el.checked;
}
Expand All @@ -84,6 +86,12 @@
var setInputValue = function(el, value) {
if (el.type === 'url' || el.type === 'text' || el.nodeName.toLowerCase() === 'textarea') {
el.value = decodeURIComponent(value);
} else if (el.type === 'select-one') {
for (let i = 0; i < el.options.length; i++) {
if (el.options[i].value === value) {
el.options[i].selected = true;
}
}
} else {
// get the `value` into a Boolean.
el.checked = JSON.parse(value);
Expand Down Expand Up @@ -217,7 +225,7 @@
representationsEl.selectedIndex = selectedIndex;
};

['debug', 'autoplay', 'muted', 'minified', 'sync-workers', 'liveui', 'partial', 'url', 'type', 'keysystems', 'buffer-water', 'override-native'].forEach(function(name) {
['debug', 'autoplay', 'muted', 'minified', 'sync-workers', 'liveui', 'partial', 'url', 'type', 'keysystems', 'buffer-water', 'override-native', 'preload'].forEach(function(name) {
stateEls[name] = document.getElementById(name);
});

Expand Down Expand Up @@ -251,6 +259,12 @@
stateEls.minified.dispatchEvent(newEvent('change'));
});

stateEls.preload.addEventListener('change', function(event) {
saveState();
// reload the player and scripts
stateEls.minified.dispatchEvent(newEvent('change'));
});

stateEls.debug.addEventListener('change', function(event) {
saveState();
window.videojs.log.level(event.target.checked ? 'debug' : 'info');
Expand Down Expand Up @@ -315,6 +329,7 @@
var videoEl = document.createElement('video-js');

videoEl.setAttribute('controls', '');
videoEl.setAttribute('preload', stateEls.preload.options[stateEls.preload.selectedIndex].value || 'auto');
videoEl.className = 'vjs-default-skin';
fixture.appendChild(videoEl);

Expand Down
18 changes: 17 additions & 1 deletion src/master-playlist-controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -279,7 +279,16 @@ export class MasterPlaylistController extends videojs.EventTarget {
this.logger_ = logger('MPC');

this.triggeredFmp4Usage = false;
this.masterPlaylistLoader_.load();
if (this.tech_.preload() === 'none') {
Copy link
Member

Choose a reason for hiding this comment

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

glad it's surprisingly simple.

this.loadOnPlay_ = () => {
this.loadOnPlay_ = null;
this.masterPlaylistLoader_.load();
};

this.tech_.one('play', this.loadOnPlay_);
} else {
this.masterPlaylistLoader_.load();
}
}

/**
Expand Down Expand Up @@ -381,6 +390,9 @@ export class MasterPlaylistController extends videojs.EventTarget {
});

this.masterPlaylistLoader_.on('loadedplaylist', () => {
if (this.loadOnPlay_) {
this.tech_.off('play', this.loadOnPlay_);
}
let updatedPlaylist = this.masterPlaylistLoader_.media();

if (!updatedPlaylist) {
Expand Down Expand Up @@ -1406,6 +1418,10 @@ export class MasterPlaylistController extends videojs.EventTarget {
this.masterPlaylistLoader_.dispose();
this.mainSegmentLoader_.dispose();

if (this.loadOnPlay_) {
this.tech_.off('play', this.loadOnPlay_);
}

['AUDIO', 'SUBTITLES'].forEach((type) => {
const groups = this.mediaTypes_[type].groups;

Expand Down
20 changes: 12 additions & 8 deletions test/videojs-http-streaming.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -3718,7 +3718,7 @@ QUnit.test('switching playlists with an outstanding key request aborts request a
);
});

QUnit.test('does not download segments if preload option set to none', function(assert) {
QUnit.test('does not download anything until play if preload option set to none', function(assert) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test was failing, and didn't really make sense anymore. I modified for the new behavior.

this.player.preload('none');
this.player.src({
src: 'master.m3u8',
Expand All @@ -3728,19 +3728,23 @@ QUnit.test('does not download segments if preload option set to none', function(
this.clock.tick(1);

openMediaSource(this.player, this.clock);
// master
this.standardXHRResponse(this.requests.shift());
// media
this.standardXHRResponse(this.requests.shift());
this.clock.tick(10 * 1000);

this.requests = this.requests.filter(function(request) {
return !(/m3u8$/).test(request.uri);
});
assert.equal(this.requests.length, 0, 'did not download any segments');

// verify stats
assert.equal(this.player.tech_.vhs.stats.bandwidth, 4194304, 'default');

this.player.tech_.paused = () => false;
this.player.tech_.trigger('play');

// master
this.standardXHRResponse(this.requests.shift());

// media
this.standardXHRResponse(this.requests.shift());

assert.equal(this.requests.length, 1, 'requested segment');
});

// workaround https://bugzilla.mozilla.org/show_bug.cgi?id=548397
Expand Down