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 TimeSyncController/ManifestUpdater race condition #2503

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
21 changes: 16 additions & 5 deletions src/streaming/ManifestUpdater.js
Original file line number Diff line number Diff line change
Expand Up @@ -103,11 +103,16 @@ function ManifestUpdater() {
}
}

function startManifestRefreshTimer() {
function startManifestRefreshTimer(delay) {
stopManifestRefreshTimer();
if (!isNaN(refreshDelay)) {
log('Refresh manifest in ' + refreshDelay + ' seconds.');
refreshTimer = setTimeout(onRefreshTimer, refreshDelay * 1000);

if (isNaN(delay) && !isNaN(refreshDelay)) {
delay = refreshDelay * 1000;
}

if (!isNaN(delay)) {
log('Refresh manifest in ' + delay + ' milliseconds.');
refreshTimer = setTimeout(onRefreshTimer, delay);
}
}

Expand Down Expand Up @@ -139,7 +144,13 @@ function ManifestUpdater() {
}

function onRefreshTimer() {
if (isPaused && !mediaPlayerModel.getScheduleWhilePaused() || isUpdating) return;
if (isPaused && !mediaPlayerModel.getScheduleWhilePaused()) {
return;
}
if (isUpdating) {
startManifestRefreshTimer(mediaPlayerModel.getManifestUpdateRetryInterval());
return;
}
refreshManifest();
}

Expand Down
32 changes: 32 additions & 0 deletions src/streaming/MediaPlayer.js
Original file line number Diff line number Diff line change
Expand Up @@ -1765,6 +1765,36 @@ function MediaPlayer() {
return mediaPlayerModel.getSmallGapLimit();
}

/**
* For live streams, set the interval-frequency in milliseconds at which
* dash.js will check if the current manifest is still processed before
* downloading the next manifest once the minimumUpdatePeriod time has
* expired.
* @param {int} value
* @default 100
* @memberof module:MediaPlayer
* @instance
* @see {@link module:MediaPlayer#getManifestUpdateRetryInterval getManifestUpdateRetryInterval()}
*
*/
function setManifestUpdateRetryInterval(value) {
mediaPlayerModel.setManifestUpdateRetryInterval(value);
}

/**
* For live streams, get the interval-frequency in milliseconds at which
* dash.js will check if the current manifest is still processed before
* downloading the next manifest once the minimumUpdatePeriod time has
* expired.
* @returns {int} Current retry delay for manifest update
* @memberof module:MediaPlayer
* @instance
* @see {@link module:MediaPlayer#setManifestUpdateRetryInterval setManifestUpdateRetryInterval()}
*/
function getManifestUpdateRetryInterval() {
return mediaPlayerModel.getManifestUpdateRetryInterval();
}

/*
---------------------------------------------------------------------------

Expand Down Expand Up @@ -2835,6 +2865,8 @@ function MediaPlayer() {
getJumpGaps: getJumpGaps,
setSmallGapLimit: setSmallGapLimit,
getSmallGapLimit: getSmallGapLimit,
setManifestUpdateRetryInterval: setManifestUpdateRetryInterval,
getManifestUpdateRetryInterval: getManifestUpdateRetryInterval,
setLongFormContentDurationThreshold: setLongFormContentDurationThreshold,
setSegmentOverlapToleranceTime: setSegmentOverlapToleranceTime,
setCacheLoadThresholdForType: setCacheLoadThresholdForType,
Expand Down
14 changes: 13 additions & 1 deletion src/streaming/models/MediaPlayerModel.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ const BUFFER_TIME_AT_TOP_QUALITY_LONG_FORM = 60;
const LONG_FORM_CONTENT_DURATION_THRESHOLD = 600;
const SEGMENT_OVERLAP_TOLERANCE_TIME = 0.05;
const SMALL_GAP_LIMIT = 0.8;
const MANIFEST_UPDATE_RETRY_INTERVAL = 100;

const CACHE_LOAD_THRESHOLD_VIDEO = 50;
const CACHE_LOAD_THRESHOLD_AUDIO = 5;
Expand Down Expand Up @@ -107,7 +108,8 @@ function MediaPlayerModel() {
movingAverageMethod,
cacheLoadThresholds,
jumpGaps,
smallGapLimit;
smallGapLimit,
manifestUpdateRetryInterval;

function setup() {
UTCTimingSources = [];
Expand Down Expand Up @@ -140,6 +142,7 @@ function MediaPlayerModel() {
wallclockTimeUpdateInterval = WALLCLOCK_TIME_UPDATE_INTERVAL;
jumpGaps = false;
smallGapLimit = SMALL_GAP_LIMIT;
manifestUpdateRetryInterval = MANIFEST_UPDATE_RETRY_INTERVAL;
xhrWithCredentials = {
default: DEFAULT_XHR_WITH_CREDENTIALS
};
Expand Down Expand Up @@ -498,6 +501,13 @@ function MediaPlayerModel() {
return smallGapLimit;
}

function setManifestUpdateRetryInterval(value) {
manifestUpdateRetryInterval = value;
}

function getManifestUpdateRetryInterval() {
return manifestUpdateRetryInterval;
}
function reset() {
//TODO need to figure out what props to persist across sessions and which to reset if any.
//setup();
Expand Down Expand Up @@ -574,6 +584,8 @@ function MediaPlayerModel() {
getJumpGaps: getJumpGaps,
setSmallGapLimit: setSmallGapLimit,
getSmallGapLimit: getSmallGapLimit,
setManifestUpdateRetryInterval: setManifestUpdateRetryInterval,
getManifestUpdateRetryInterval: getManifestUpdateRetryInterval,
reset: reset
};

Expand Down
15 changes: 15 additions & 0 deletions test/unit/mocks/MediaPlayerModelMock.js
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,8 @@ const CACHE_LOAD_THRESHOLD_AUDIO = 5;

const SMALL_GAP_LIMIT = 0.8;

const MANIFEST_UPDATE_RETRY_INTERVAL = 100;

class MediaPlayerModelMock {

// Constants
Expand Down Expand Up @@ -141,6 +143,10 @@ class MediaPlayerModelMock {
return MANIFEST_RETRY_INTERVAL;
}

static get MANIFEST_UPDATE_RETRY_INTERVAL() {
return MANIFEST_UPDATE_RETRY_INTERVAL;
}

static get XLINK_RETRY_ATTEMPTS() {
return XLINK_RETRY_ATTEMPTS;
}
Expand Down Expand Up @@ -205,6 +211,7 @@ class MediaPlayerModelMock {
this.cacheLoadThresholds[Constants.AUDIO] = CACHE_LOAD_THRESHOLD_AUDIO;
this.jumpGaps = false;
this.smallGapLimit = SMALL_GAP_LIMIT;
this.manifestUpdateRetryInterval = MANIFEST_UPDATE_RETRY_INTERVAL;
}

//TODO Should we use Object.define to have setters/getters? makes more readable code on other side.
Expand Down Expand Up @@ -502,6 +509,14 @@ class MediaPlayerModelMock {
return this.jumpGaps;
}

setManifestUpdateRetryInterval(value) {
this.manifestUpdateRetryInterval = value;
}

getManifestUpdateRetryInterval() {
return this.manifestUpdateRetryInterval;
}

setSmallGapLimit(value) {
this.smallGapLimit = value;
}
Expand Down
13 changes: 13 additions & 0 deletions test/unit/streaming.MediaPlayerSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -816,6 +816,19 @@ describe('MediaPlayer', function () {
expect(smallGapLimit).to.equal(0.5);
});

it('should configure manifestUpdateRetryInterval', function () {
let manifestUpdateRetryInterval = player.getManifestUpdateRetryInterval();
expect(manifestUpdateRetryInterval).to.equal(MediaPlayerModelMock.MANIFEST_UPDATE_RETRY_INTERVAL);

player.setManifestUpdateRetryInterval(200);

manifestUpdateRetryInterval = mediaPlayerModel.getManifestUpdateRetryInterval();
expect(manifestUpdateRetryInterval).to.equal(200);

manifestUpdateRetryInterval = player.getManifestUpdateRetryInterval();
expect(manifestUpdateRetryInterval).to.equal(200);
});

it('should configure BandwidthSafetyFactor', function () {
let BandwidthSafetyFactor = mediaPlayerModel.getBandwidthSafetyFactor();
expect(BandwidthSafetyFactor).to.equal(0.9);
Expand Down