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

Multi period dash VOD fixes #1551

Merged
merged 12 commits into from
Nov 21, 2024
15 changes: 15 additions & 0 deletions src/dash-playlist-loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -304,6 +304,8 @@
constructor(srcUrlOrPlaylist, vhs, options = { }, mainPlaylistLoader) {
super();

this.isPaused_ = true;

this.mainPlaylistLoader_ = mainPlaylistLoader || this;
if (!mainPlaylistLoader) {
this.isMain_ = true;
Expand Down Expand Up @@ -345,6 +347,10 @@
}
}

get isPaused() {
return this.isPaused_;

Check warning on line 351 in src/dash-playlist-loader.js

View check run for this annotation

Codecov / codecov/patch

src/dash-playlist-loader.js#L350-L351

Added lines #L350 - L351 were not covered by tests
}

requestErrored_(err, request, startingState) {
// disposed
if (!this.request) {
Expand Down Expand Up @@ -384,6 +390,7 @@
// playlist lacks sidx or sidx segments were added to this playlist already.
if (!playlist.sidx || !sidxKey || this.mainPlaylistLoader_.sidxMapping_[sidxKey]) {
// keep this function async
window.clearTimeout(this.mediaRequest_);
this.mediaRequest_ = window.setTimeout(() => cb(false), 0);
return;
}
Expand Down Expand Up @@ -465,6 +472,7 @@
}

dispose() {
this.isPaused_ = true;
this.trigger('dispose');
this.stopRequest();
this.loadedPlaylists_ = {};
Expand Down Expand Up @@ -553,6 +561,7 @@
haveMetadata({startingState, playlist}) {
this.state = 'HAVE_METADATA';
this.loadedPlaylists_[playlist.id] = playlist;
window.clearTimeout(this.mediaRequest_);
this.mediaRequest_ = null;

// This will trigger loadedplaylist
Expand All @@ -569,6 +578,8 @@
}

pause() {
this.isPaused_ = true;

if (this.mainPlaylistLoader_.createMupOnMedia_) {
this.off('loadedmetadata', this.mainPlaylistLoader_.createMupOnMedia_);
this.mainPlaylistLoader_.createMupOnMedia_ = null;
Expand All @@ -588,6 +599,8 @@
}

load(isFinalRendition) {
this.isPaused_ = false;

window.clearTimeout(this.mediaUpdateTimeout);
this.mediaUpdateTimeout = null;

Expand Down Expand Up @@ -629,6 +642,7 @@
// We don't need to request the main manifest again
// Call this asynchronously to match the xhr request behavior below
if (!this.isMain_) {
window.clearTimeout(this.mediaRequest_);
this.mediaRequest_ = window.setTimeout(() => this.haveMain_(), 0);
return;
}
Expand Down Expand Up @@ -773,6 +787,7 @@

handleMain_() {
// clear media request
window.clearTimeout(this.mediaRequest_);
this.mediaRequest_ = null;

const oldMain = this.mainPlaylistLoader_.main;
Expand Down
51 changes: 45 additions & 6 deletions src/playlist-controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import { addMetadata, createMetadataTrackIfNotExists, addDateRangeMetadata } from './util/text-tracks';
import ContentSteeringController from './content-steering-controller';
import { bufferToHexString } from './util/string.js';
import {debounce} from './util/fn';

const ABORT_EARLY_EXCLUSION_SECONDS = 10;

Expand Down Expand Up @@ -152,6 +153,11 @@
constructor(options) {
super();

// Adding a slight debounce to avoid duplicate calls during rapid quality changes, for example:
// When selecting quality from the quality list,
// where we may have multiple bandwidth profiles for the same vertical resolution.
this.fastQualityChange_ = debounce(this.fastQualityChange_.bind(this), 100);
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for actually adding this! This has been on my list of "quick improvements" to fast quality change for a while and keeps getting buried by other priorities.


const {
src,
withCredentials,
Expand Down Expand Up @@ -701,7 +707,16 @@

if (this.sourceType_ === 'dash') {
// we don't want to re-request the same hls playlist right after it was changed
this.mainPlaylistLoader_.load();

// Initially it was implemented as workaround to restart playlist loader for live
// when playlist loader is paused because of playlist exclusions:
// see: https://github.com/videojs/http-streaming/pull/1339
// but this introduces duplicate "loadedplaylist" event.
// Ideally we want to re-think playlist loader life-cycle events,
// but simply checking "paused" state should help a lot
if (this.mainPlaylistLoader_.isPaused) {
this.mainPlaylistLoader_.load();

Check warning on line 718 in src/playlist-controller.js

View check run for this annotation

Codecov / codecov/patch

src/playlist-controller.js#L717-L718

Added lines #L717 - L718 were not covered by tests
}
}

// TODO: Create a new event on the PlaylistLoader that signals
Expand Down Expand Up @@ -962,6 +977,24 @@
this.tech_.setCurrentTime(newTime);
});

this.timelineChangeController_.on('fixBadTimelineChange', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Also love moving this to the playlist-controller level. 🥇

// pause, reset-everything and load for all segment-loaders
this.logger_('Fix bad timeline change. Restarting al segment loaders...');
this.mainSegmentLoader_.pause();
this.mainSegmentLoader_.resetEverything();
if (this.mediaTypes_.AUDIO.activePlaylistLoader) {
this.audioSegmentLoader_.pause();
this.audioSegmentLoader_.resetEverything();

Check warning on line 987 in src/playlist-controller.js

View check run for this annotation

Codecov / codecov/patch

src/playlist-controller.js#L982-L987

Added lines #L982 - L987 were not covered by tests
}
if (this.mediaTypes_.SUBTITLES.activePlaylistLoader) {
this.subtitleSegmentLoader_.pause();
this.subtitleSegmentLoader_.resetEverything();

Check warning on line 991 in src/playlist-controller.js

View check run for this annotation

Codecov / codecov/patch

src/playlist-controller.js#L989-L991

Added lines #L989 - L991 were not covered by tests
}

// start segment loader loading in case they are paused
this.load();

Check warning on line 995 in src/playlist-controller.js

View check run for this annotation

Codecov / codecov/patch

src/playlist-controller.js#L995

Added line #L995 was not covered by tests
});

this.mainSegmentLoader_.on('earlyabort', (event) => {
// never try to early abort with the new ABR algorithm
if (this.bufferBasedABR) {
Expand Down Expand Up @@ -1109,13 +1142,19 @@

runFastQualitySwitch_() {
this.waitingForFastQualityPlaylistReceived_ = false;
// Delete all buffered data to allow an immediate quality switch.
this.mainSegmentLoader_.pause();
this.mainSegmentLoader_.resetEverything(() => {
this.mainSegmentLoader_.load();
});
this.mainSegmentLoader_.resetEverything();
if (this.mediaTypes_.AUDIO.activePlaylistLoader) {
this.audioSegmentLoader_.pause();
this.audioSegmentLoader_.resetEverything();

Check warning on line 1149 in src/playlist-controller.js

View check run for this annotation

Codecov / codecov/patch

src/playlist-controller.js#L1148-L1149

Added lines #L1148 - L1149 were not covered by tests
}
if (this.mediaTypes_.SUBTITLES.activePlaylistLoader) {
this.subtitleSegmentLoader_.pause();
this.subtitleSegmentLoader_.resetEverything();

Check warning on line 1153 in src/playlist-controller.js

View check run for this annotation

Codecov / codecov/patch

src/playlist-controller.js#L1152-L1153

Added lines #L1152 - L1153 were not covered by tests
}

// don't need to reset audio as it is reset when media changes
// start segment loader loading in case they are paused
this.load();
}

/**
Expand Down
3 changes: 2 additions & 1 deletion src/rendition-mixin.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,9 @@ const enableFunction = (loader, playlistID, changePlaylistFn) => (enable) => {

if (enable !== currentlyEnabled && !incompatible) {
// Ensure the outside world knows about our changes
changePlaylistFn(playlist);
if (enable) {
// call fast quality change only when the playlist is enabled
changePlaylistFn(playlist);
loader.trigger({ type: 'renditionenabled', metadata});
} else {
loader.trigger({ type: 'renditiondisabled', metadata});
Expand Down
26 changes: 10 additions & 16 deletions src/segment-loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -423,21 +423,6 @@ export const shouldFixBadTimelineChanges = (timelineChangeController) => {
return false;
};

/**
* Fixes certain bad timeline scenarios by resetting the loader.
*
* @param {SegmentLoader} segmentLoader
*/
export const fixBadTimelineChange = (segmentLoader) => {
if (!segmentLoader) {
return;
}

segmentLoader.pause();
segmentLoader.resetEverything();
segmentLoader.load();
};

/**
* Check if the pending audio timeline change is behind the
* pending main timeline change.
Expand Down Expand Up @@ -480,7 +465,7 @@ const checkAndFixTimelines = (segmentLoader) => {
return;
}

fixBadTimelineChange(segmentLoader);
segmentLoader.timelineChangeController_.trigger('fixBadTimelineChange');
}
};

Expand Down Expand Up @@ -872,6 +857,7 @@ export default class SegmentLoader extends videojs.EventTarget {
if (this.pendingSegment_) {
this.pendingSegment_ = null;
}
this.timelineChangeController_.clearPendingTimelineChange(this.loaderType_);
return;
}

Expand Down Expand Up @@ -1118,6 +1104,14 @@ export default class SegmentLoader extends videojs.EventTarget {
return;
}

if (this.playlist_ &&
this.playlist_.endList &&
newPlaylist.endList &&
this.playlist_.uri === newPlaylist.uri) {
// skip update if both prev and new are vod and have the same URI
return;
}

const oldPlaylist = this.playlist_;
const segmentInfo = this.pendingSegment_;

Expand Down
11 changes: 11 additions & 0 deletions src/util/fn.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
export const debounce = (callback, wait) => {
let timeoutId = null;

return (...args) => {
clearTimeout(timeoutId);

timeoutId = setTimeout(() => {
callback.apply(null, args);
}, wait);
};
};
7 changes: 5 additions & 2 deletions test/playlist-controller.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -726,8 +726,9 @@ QUnit.test('resets everything for a fast quality change then calls load', functi

segmentLoader.remove = (start, end, doneFn) => {
assert.equal(end, Infinity, 'on a remove all, end should be Infinity');
assert.ok(doneFn);
doneFn();
if (doneFn) {
doneFn();
}
origRemove.call(segmentLoader, start, end, doneFn);
};

Expand Down Expand Up @@ -4871,6 +4872,7 @@ QUnit.test('can pass or select a playlist for fastQualityChange', function(asser
};

pc.fastQualityChange_(pc.main().playlists[1]);
this.clock.tick(110);
pc.runFastQualitySwitch_();
assert.deepEqual(calls, {
resetEverything: 1,
Expand All @@ -4880,6 +4882,7 @@ QUnit.test('can pass or select a playlist for fastQualityChange', function(asser
}, 'calls expected function when passed a playlist');

pc.fastQualityChange_();
this.clock.tick(110);
pc.runFastQualitySwitch_();
assert.deepEqual(calls, {
resetEverything: 2,
Expand Down
2 changes: 1 addition & 1 deletion test/rendition-mixin.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,7 @@ QUnit.test(

renditions[1].enabled(false);

assert.equal(pc.fastQualityChange_.calls, 2, 'fastQualityChange_ was called twice');
assert.equal(pc.fastQualityChange_.calls, 1, 'fastQualityChange_ was called once');
}
);

Expand Down
Loading
Loading