-
Notifications
You must be signed in to change notification settings - Fork 793
use last segment duration + 2*targetDuration for safe live point instead of 3 segments #1271
Changes from 10 commits
0918559
ee2e4fc
8050a02
3917852
b781765
54ea94f
3881c2e
2e31dd6
ab60fd7
fb9fbc9
3a77d68
94d9d91
1b69a71
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -820,14 +820,15 @@ export class MasterPlaylistController extends videojs.EventTarget { | |
|
||
if (!buffered.length) { | ||
// return true if the playhead reached the absolute end of the playlist | ||
return absolutePlaylistEnd - currentTime <= Ranges.TIME_FUDGE_FACTOR; | ||
return absolutePlaylistEnd - currentTime <= 0.1; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not super familiar with this... why not just change the value of TIME_FUDGE_FACTOR instead of making this a hard coded value? Is TIME_FUDGE_FACTOR using in a bunch of spots? Might also be good to comment on why 0.1 seconds for future explorers. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is used in many different places. Increasing the |
||
} | ||
let bufferedEnd = buffered.end(buffered.length - 1); | ||
|
||
// return true if there is too little buffer left and | ||
// buffer has reached absolute end of playlist | ||
return bufferedEnd - currentTime <= Ranges.TIME_FUDGE_FACTOR && | ||
absolutePlaylistEnd - bufferedEnd <= Ranges.TIME_FUDGE_FACTOR; | ||
// return true if there is too little buffer left and buffer has reached absolute | ||
// end of playlist. We use 0.1 here to account for possible precision errors and for | ||
// media that does not have well aligned audio and video, which may prevent the | ||
// browser from letting currentTime reach the absolute end of the buffer. | ||
return bufferedEnd - currentTime <= 0.1 && absolutePlaylistEnd - bufferedEnd <= 0.1; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same re: comment. Or we can have a constant elsewhere. Like |
||
} | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -230,16 +230,24 @@ export default class PlaylistLoader extends EventTarget { | |
|
||
// merge this playlist into the master | ||
const update = updateMaster(this.master, parser.manifest); | ||
let refreshDelay = (parser.manifest.targetDuration || 10) * 1000; | ||
|
||
this.targetDuration = parser.manifest.targetDuration; | ||
|
||
// if the playlist is unchanged since the last reload or last segment duration cannot | ||
// be determined, try again after half the target duration | ||
let refreshDelay = (this.targetDuration || 10) * 500; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For readability sake, I think the logic is easier to follow if this line is kept within the else below and we simply keep the variable declaration here. |
||
|
||
if (update) { | ||
this.master = update; | ||
this.media_ = this.master.playlists[parser.manifest.uri]; | ||
|
||
// the client MUST wait for at least the duration of the last segment in the | ||
// Playlist before attempting to reload the Playlist file again | ||
if (this.media_.segments.length) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If there are no media segments, should we be more or less aggressive than half the target duration? Is there any recommendation of what to do in that case from Apple, or what Safari does? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It only has the recommendation of if the playlist is unchanged, to do half of target duration. It also has this statement
So I think it we should not be more aggressive and just stick with the half target duration when we encounter strange edge cases |
||
refreshDelay = this.media_.segments[this.media_.segments.length - 1].duration; | ||
refreshDelay *= 1000; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Preference, but this can be one line. |
||
} | ||
} else { | ||
// if the playlist is unchanged since the last reload, | ||
// try again after half the target duration | ||
refreshDelay /= 2; | ||
this.trigger('playlistunchanged'); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,16 +6,6 @@ | |
import {createTimeRange} from 'video.js'; | ||
import window from 'global/window'; | ||
|
||
let Playlist = { | ||
/** | ||
* The number of segments that are unsafe to start playback at in | ||
* a live stream. Changing this value can cause playback stalls. | ||
* See HTTP Live Streaming, "Playing the Media Playlist File" | ||
* https://tools.ietf.org/html/draft-pantos-http-live-streaming-18#section-6.3.3 | ||
*/ | ||
UNSAFE_LIVE_SEGMENTS: 3 | ||
}; | ||
|
||
/** | ||
* walk backward until we find a duration we can use | ||
* or return a failure | ||
|
@@ -219,6 +209,38 @@ export const sumDurations = function(playlist, startIndex, endIndex) { | |
return durations; | ||
}; | ||
|
||
/** | ||
* Determines the media index of the segment corresponding to the safe edge of the live | ||
* window which is the duration of the last segment plus 2 target durations from the end | ||
* of the playlist. | ||
* | ||
* @param {Object} playlist | ||
* a media playlist object | ||
* @return {Number} | ||
* The media index of the segment at the safe live point. 0 if there is no "safe" | ||
* point. | ||
* @function safeLiveIndex | ||
*/ | ||
export const safeLiveIndex = function(playlist) { | ||
if (!playlist.segments.length) { | ||
return 0; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While the HLS specification is stricter, since someone can theoretically have a playlist of 3 segments, would it be better to return -1 or null here? I know that the value gets used directly later, but maybe it would be better to consider that case and warn or debug? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It might be worth considering adding a warn or debug after loading the first playlist and see it is too short to be safe, but I don't think here is a good place as this is done everytime we calculate seekable, which would flood the console with warnings when one would be sufficient |
||
} | ||
|
||
let i = playlist.segments.length - 1; | ||
let distanceFromEnd = playlist.segments[i].duration || playlist.targetDuration; | ||
const safeDistance = distanceFromEnd + playlist.targetDuration * 2; | ||
|
||
while (i--) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Preference, but you could move it into a for loop:
|
||
distanceFromEnd += playlist.segments[i].duration; | ||
|
||
if (distanceFromEnd >= safeDistance) { | ||
break; | ||
} | ||
} | ||
|
||
return Math.max(0, i); | ||
}; | ||
|
||
/** | ||
* Calculates the playlist end time | ||
* | ||
|
@@ -246,9 +268,7 @@ export const playlistEnd = function(playlist, expired, useSafeLiveEnd) { | |
|
||
expired = expired || 0; | ||
|
||
let endSequence = useSafeLiveEnd ? | ||
Math.max(0, playlist.segments.length - Playlist.UNSAFE_LIVE_SEGMENTS) : | ||
Math.max(0, playlist.segments.length); | ||
const endSequence = useSafeLiveEnd ? safeLiveIndex(playlist) : playlist.segments.length; | ||
|
||
return intervalDuration(playlist, | ||
playlist.mediaSequence + endSequence, | ||
|
@@ -506,18 +526,19 @@ export const estimateSegmentRequestTime = function(segmentDuration, | |
return (size - (bytesReceived * 8)) / bandwidth; | ||
}; | ||
|
||
Playlist.duration = duration; | ||
Playlist.seekable = seekable; | ||
Playlist.getMediaInfoForTime = getMediaInfoForTime; | ||
Playlist.isEnabled = isEnabled; | ||
Playlist.isDisabled = isDisabled; | ||
Playlist.isBlacklisted = isBlacklisted; | ||
Playlist.isIncompatible = isIncompatible; | ||
Playlist.playlistEnd = playlistEnd; | ||
Playlist.isAes = isAes; | ||
Playlist.isFmp4 = isFmp4; | ||
Playlist.hasAttribute = hasAttribute; | ||
Playlist.estimateSegmentRequestTime = estimateSegmentRequestTime; | ||
|
||
// exports | ||
export default Playlist; | ||
export default { | ||
duration, | ||
seekable, | ||
safeLiveIndex, | ||
getMediaInfoForTime, | ||
isEnabled, | ||
isDisabled, | ||
isBlacklisted, | ||
isIncompatible, | ||
playlistEnd, | ||
isAes, | ||
isFmp4, | ||
hasAttribute, | ||
estimateSegmentRequestTime | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -908,6 +908,12 @@ export default class SegmentLoader extends videojs.EventTarget { | |
trimBackBuffer_(segmentInfo) { | ||
const seekable = this.seekable_(); | ||
const currentTime = this.currentTime_(); | ||
const targetDuration = this.playlist_.targetDuration || 10; | ||
|
||
// Don't allow removing from the buffer within target duration of current time | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this from a different PR? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With the changes to the safe live point, it created the problem where on some streams, we would be removing a portion of the GOP that currentTime was currently playing. This would cause chrome to remove the entire GOP, leaving currentTime outside of a buffered range, causing a playback stall. |
||
// to avoid the possibility of removing the GOP currently being played which could | ||
// cause playback stalls. | ||
const safeRemoveToTimeLimit = currentTime - targetDuration; | ||
let removeToTime = 0; | ||
|
||
// Chrome has a hard limit of 150MB of | ||
|
@@ -926,6 +932,8 @@ export default class SegmentLoader extends videojs.EventTarget { | |
removeToTime = currentTime - 30; | ||
} | ||
|
||
removeToTime = Math.min(removeToTime, safeRemoveToTimeLimit); | ||
|
||
if (removeToTime > 0) { | ||
this.remove(0, removeToTime); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -356,7 +356,7 @@ function(assert) { | |
|
||
QUnit.test('seekable end and playlist end account for non-standard target durations', | ||
function(assert) { | ||
let playlist = { | ||
const playlist = { | ||
targetDuration: 2, | ||
mediaSequence: 0, | ||
syncInfo: { | ||
|
@@ -385,11 +385,119 @@ function(assert) { | |
|
||
assert.equal(seekable.start(0), 0, 'starts at the earliest available segment'); | ||
assert.equal(seekable.end(0), | ||
9 - (2 + 2 + 1), | ||
'allows seeking no further than three segments from the end'); | ||
// Playlist duration is 9s. Target duration 2s. Seekable end should be at | ||
// least 6s from end. Adding segment durations starting from the end to get | ||
// that 6s target | ||
9 - (2 + 2 + 1 + 2), | ||
'allows seeking no further than three times target duration from the end'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we're going by this comment, shouldn't it be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So I tried to make this more clear by including the wording There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. does that make sense? any ideas on how i can make that more clear in the comment? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe |
||
assert.equal(playlistEnd, 9, 'playlist end at the last segment'); | ||
}); | ||
|
||
QUnit.test('safeLiveIndex is correct for standard segment durations', function(assert) { | ||
const playlist = { | ||
targetDuration: 6, | ||
mediaSequence: 10, | ||
syncInfo: { | ||
time: 0, | ||
mediaSequence: 10 | ||
}, | ||
segments: [ | ||
{ | ||
duration: 6 | ||
}, | ||
{ | ||
duration: 6 | ||
}, | ||
{ | ||
duration: 6 | ||
}, | ||
{ | ||
duration: 6 | ||
}, | ||
{ | ||
duration: 6 | ||
}, | ||
{ | ||
duration: 6 | ||
} | ||
] | ||
}; | ||
|
||
const expected = 3; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Preference, but for this (and below) the variables don't add much to just making the call in a single line. |
||
const actual = Playlist.safeLiveIndex(playlist); | ||
|
||
assert.equal(actual, expected, 'correct media index for standard durations'); | ||
}); | ||
|
||
QUnit.test('safeLiveIndex is correct for variable segment durations', function(assert) { | ||
const playlist = { | ||
targetDuration: 6, | ||
mediaSequence: 10, | ||
syncInfo: { | ||
time: 0, | ||
mediaSequence: 10 | ||
}, | ||
segments: [ | ||
{ | ||
duration: 6 | ||
}, | ||
{ | ||
duration: 4 | ||
}, | ||
{ | ||
duration: 5 | ||
}, | ||
{ | ||
// this segment is 16 seconds from the end of playlist, the safe live point | ||
duration: 6 | ||
}, | ||
{ | ||
duration: 3 | ||
}, | ||
{ | ||
duration: 4 | ||
}, | ||
{ | ||
duration: 3 | ||
} | ||
] | ||
}; | ||
|
||
// safe live point is no less than 15 seconds (3s + 2 * 6s) from the end of the playlist | ||
const expected = 3; | ||
const actual = Playlist.safeLiveIndex(playlist); | ||
|
||
assert.equal(actual, expected, 'correct media index for variable segment durations'); | ||
}); | ||
|
||
QUnit.test('safeLiveIndex is 0 when no safe live point', function(assert) { | ||
const playlist = { | ||
targetDuration: 6, | ||
mediaSequence: 10, | ||
syncInfo: { | ||
time: 0, | ||
mediaSequence: 10 | ||
}, | ||
segments: [ | ||
{ | ||
duration: 6 | ||
}, | ||
{ | ||
duration: 3 | ||
}, | ||
{ | ||
duration: 3 | ||
} | ||
] | ||
}; | ||
|
||
const expected = 0; | ||
const actual = Playlist.safeLiveIndex(playlist); | ||
|
||
assert.equal(actual, expected, | ||
'returns media index 0 when playlist has no safe live point'); | ||
}); | ||
|
||
QUnit.test( | ||
'seekable end and playlist end account for non-zero starting VOD media sequence', | ||
function(assert) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it is used in multiple places, may be worth creating a variable like
Ranges.SAFE_END_DELTA
and putting the comment there.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this one still needs to be changed.