Skip to content

Commit

Permalink
fix: multi-period DASH VOD fixes (#1551)
Browse files Browse the repository at this point in the history
* chore: clear previous dash media request timeout everytime we clear or update it

* chore: call fast-quality-switch only when we enable playlist from quality selector

* chore: add isPaused for dash playlist loader to mitigate duplicate playlist trigger for the main segment loader

* chore: fix fastQualityChange_ tests

* chore: add fast quality change debounce

* chore: add debounce tick to the tests

* chore: restart all loaders when we hit fix bad timeline change

* chore: update segment loader

* chore: update run-fast-quality-switch and fix bad timeline changes

* fix test

* chore: fix lint issues

---------

Co-authored-by: Dzianis Dashkevich <ddashkevich@brightcove.com>
  • Loading branch information
dzianis-dashkevich and Dzianis Dashkevich authored Nov 21, 2024
1 parent 2f8d0af commit 6fe7d9c
Show file tree
Hide file tree
Showing 9 changed files with 117 additions and 77 deletions.
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 @@ export default class DashPlaylistLoader extends EventTarget {
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 @@ export default class DashPlaylistLoader extends EventTarget {
}
}

get isPaused() {
return this.isPaused_;
}

requestErrored_(err, request, startingState) {
// disposed
if (!this.request) {
Expand Down Expand Up @@ -384,6 +390,7 @@ export default class DashPlaylistLoader extends EventTarget {
// 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 @@ export default class DashPlaylistLoader extends EventTarget {
}

dispose() {
this.isPaused_ = true;
this.trigger('dispose');
this.stopRequest();
this.loadedPlaylists_ = {};
Expand Down Expand Up @@ -553,6 +561,7 @@ export default class DashPlaylistLoader extends EventTarget {
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 @@ export default class DashPlaylistLoader extends EventTarget {
}

pause() {
this.isPaused_ = true;

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

load(isFinalRendition) {
this.isPaused_ = false;

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

Expand Down Expand Up @@ -629,6 +642,7 @@ export default class DashPlaylistLoader extends EventTarget {
// 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 @@ export default class DashPlaylistLoader extends EventTarget {

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 {merge, createTimeRanges} from './util/vjs-compat';
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 @@ export class PlaylistController extends videojs.EventTarget {
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);

const {
src,
withCredentials,
Expand Down Expand Up @@ -701,7 +707,16 @@ export class PlaylistController extends videojs.EventTarget {

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();
}
}

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

this.timelineChangeController_.on('fixBadTimelineChange', () => {
// 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();
}
if (this.mediaTypes_.SUBTITLES.activePlaylistLoader) {
this.subtitleSegmentLoader_.pause();
this.subtitleSegmentLoader_.resetEverything();
}

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

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 @@ export class PlaylistController extends videojs.EventTarget {

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();
}
if (this.mediaTypes_.SUBTITLES.activePlaylistLoader) {
this.subtitleSegmentLoader_.pause();
this.subtitleSegmentLoader_.resetEverything();
}

// 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

0 comments on commit 6fe7d9c

Please sign in to comment.