Skip to content
This repository has been archived by the owner on Jan 12, 2019. It is now read-only.

use last segment duration + 2*targetDuration for safe live point instead of 3 segments #1271

Merged
merged 13 commits into from
Oct 19, 2017

Conversation

mjneil
Copy link
Contributor

@mjneil mjneil commented Oct 5, 2017

Description

Right now we use 3 segments from the end of the live window for the safe live point. We should be using target duration instead of number of segments because segments can have varying durations up to target duration. Using target duration will help prevent playback stalls in streams that have segments with much smaller duration than target duration.

https://developer.apple.com/streaming/HLS-WWDC-2017-Preliminary-Spec.pdf

The client SHALL choose which Media Segment to play first from the
Media Playlist when playback starts. If the EXT-X-ENDLIST tag is not
present and the client intends to play the media normally, the client
SHOULD NOT choose a segment which starts less than the duration of
the last segment in the Playlist plus two target durations from the
end of the Playlist file. Doing so can trigger playback stalls.

When a client loads a Playlist file for the first time or reloads a
Playlist file and finds that it has changed since the last time it
was loaded, the client MUST wait for at least the duration of the
last segment in the Playlist before attempting to reload the Playlist
file again, measured from the last time the client began loading the
Playlist file.

Changes

  • Use last segment duration + 2 * target duration for the safe live point
  • Do not let trim the back buffer within 1 target duration of current time
  • increase threshold for stuck at playlist end check from 1/30 to 0.1

Copy link
Contributor

@OshinKaramian OshinKaramian left a comment

Choose a reason for hiding this comment

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

Mostly a wide series of nitpicks and test suggestions.

src/playlist.js Outdated
let endSequence = useSafeLiveEnd ?
Math.max(0, playlist.segments.length - Playlist.UNSAFE_LIVE_SEGMENTS) :
Math.max(0, playlist.segments.length);
let endSequence = useSafeLiveEnd ? safeLiveIndex(playlist) : playlist.segments.length;
Copy link
Contributor

Choose a reason for hiding this comment

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

The most nitpicky of comments: this should probably be a const.

src/playlist.js Outdated
@@ -485,6 +510,7 @@ export const estimateSegmentRequestTime = function(segmentDuration,

Playlist.duration = duration;
Copy link
Contributor

Choose a reason for hiding this comment

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

If not for the UNSAFE_LIVE_SEGMENTS piece above, I think you could do:

Playlist = { duration, seekable, safeLiveIndex, etc... };

Which would be cleaner.

@@ -385,11 +385,92 @@ 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');
9 - (2 + 2 + 1 + 2),
Copy link
Contributor

Choose a reason for hiding this comment

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

It's slightly unclear what's going on here. It might make sense to make this wordier so that the test is a bit quicker to understand. I'm guessing something like playlists.segments[0].duration + segments[1].duration.... I'm also guessing the 9 is the total sum of the duration. Either that or a comment to highlight what these numbers are would be helpful.

Copy link
Contributor

@OshinKaramian OshinKaramian Oct 13, 2017

Choose a reason for hiding this comment

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

Also I can't comment on code you haven't touched :) But it might be wise to change the let playlist to a const playlist unless you're expecting the functions it's passed into to reassign it in some way.

assert.equal(playlistEnd, 9, 'playlist end at the last segment');
});

QUnit.test('safeLiveIndex returns the correct media index for the safe live point',
function(assert) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be wise to break this up into 3 tests;

safe live point for standard durations, correct media index for variable durations, no safe live point.

This will make it easier to debug (it's very clear which test is failing), and I believe that the following assertions won't get hit if one assertion fails. As an example, if standard durations fails, but variable segments passes that is useful debug information (or if both fail). I think that gets lost with this though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previous assertions failing won't prevent the following from being run, tested your example and variable segments still gets hit and passes. I can still break them up if you think its a good idea, but all the assertions in this test are testing the same function, just different input edge cases, so to me it makes sense to keep all the safeLiveIndex assertions within the same test

Copy link
Contributor

Choose a reason for hiding this comment

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

I think keeping the tests kind of small/specific will make maintenance longer term a bit simpler. Particularly if another case gets tacked on here.

@mjneil mjneil changed the title use 3*targetDuration for safe live point instead of 3 segments use last segment duration + 2*targetDuration for safe live point instead of 3 segments Oct 16, 2017
src/playlist.js Outdated
* @param {Object} playlist
* a media playlist object;
* @return {Number}
* The media index of the segment
Copy link
Contributor

Choose a reason for hiding this comment

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

Might need something like "Unless segment length does not exist, in which case returns 0."

@@ -820,14 +820,13 @@ 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;
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is used in many different places. Increasing the TIME_FUDGE_FACTOR to 0.1 for everywhere might be a good idea, but it increases the scope of this change a lot and I'm not sure if the other places using this value require more precision.

@@ -820,14 +820,13 @@ 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;
Copy link
Contributor

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.

Copy link
Contributor

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.

}
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 bufferedEnd - currentTime <= 0.1 && absolutePlaylistEnd - bufferedEnd <= 0.1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same re: comment. Or we can have a constant elsewhere. Like Ranges.SAFE_END_DELTA and leave a comment above it.


// 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;
Copy link
Contributor

Choose a reason for hiding this comment

The 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.


// 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) {
Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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

However the client MUST NOT attempt to reload the Playlist file more
   frequently than specified by this section, in order to limit the
   collective load on the server.

So I think it we should not be more aggressive and just stick with the half target duration when we encounter strange edge cases

// Playlist before attempting to reload the Playlist file again
if (this.media_.segments.length) {
refreshDelay = this.media_.segments[this.media_.segments.length - 1].duration;
refreshDelay *= 1000;
Copy link
Contributor

Choose a reason for hiding this comment

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

Preference, but this can be one line.

*/
export const safeLiveIndex = function(playlist) {
if (!playlist.segments.length) {
return 0;
Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 distanceFromEnd = playlist.segments[i].duration || playlist.targetDuration;
const safeDistance = distanceFromEnd + playlist.targetDuration * 2;

while (i--) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Preference, but you could move it into a for loop:

let distanceFromEnd = playlist.segments[playlist.segments.length - 1].duration || playlist.targetDuration;
const safeDistance = distanceFromEnd + playlist.targetDuration * 2;

for (let i = playlist.segments.length - 1; i >= 0; i--) {
  distanceFromEnd += playlist.segments[i].duration;
  
  if (distanceFromEnd >= safeDistance) {
    return i;
  }
}

return null; // changed as per comment above

@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this from a different PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

// 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');
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're going by this comment, shouldn't it be 9 - 3 * 2?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I tried to make this more clear by including the wording Adding segment durations starting from the end to get that 6s target. Basically the rule we are following is that we cannot start playback any later than lastSegment Duration + 2 * targetDuration from the end of the playlist (which reminds me I need to update this comment as its still saying 3 * targetDuration). However, for robustness sake, instead of just setting seekable end to be PlaylistEnd - (lastSegment Duration + 2 * targetDuration) We find the segment that that value falls under and use that segment's start time as seekable end. So in this case, the safest point we could start loading from is at 3 seconds. We have a segment that goes from 2-4s, so we set seekable end to the start of that segment, which is 2s

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe allows seeking no further than the start of the segment 2 target durations back from the beginning of the last segment?

]
};

const expected = 3;
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

@@ -820,14 +820,13 @@ 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;
Copy link
Contributor

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.

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.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say either caps Return or remove period

this.trigger('playlistunchanged');
}

// refresh live playlists after a target duration passes
if (!this.media().endList) {
const lastSegment = this.media_.segments[this.media_.segments.length - 1];
Copy link
Contributor

Choose a reason for hiding this comment

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

This logic block looks like a good candidate for creating a function.

src/ranges.js Outdated
// aligned audio and video, which can cause values to be slightly off from what you would
// expect. This value is what we consider to be safe to use in such comparisons to account
// for these scenarios.
const SAFE_TIME_DELTA = 0.1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should just use TIME_FUDGE_FACTOR * 3?

// 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');
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe allows seeking no further than the start of the segment 2 target durations back from the beginning of the last segment?

@@ -2628,6 +2629,74 @@ QUnit.test('cleans up the buffer based on currentTime when loading a live segmen
assert.deepEqual(removes[0], [0, 80 - 30], 'remove called with the right range');
});

QUnit.test('cleans up the buffer based on currentTime when loading a live segment if' +
Copy link
Contributor

Choose a reason for hiding this comment

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

This test seems pretty complicated to set up. Maybe it would be better if we could make the trim function call another function with the right passed properties to get the trim range, so that that function can be tested in isolation.

// cause playback stalls.
const safeRemoveToTimeLimit = currentTime - targetDuration;

// Chrome has a hard limit of 150MB of
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment should probably go in the trimBackBuffer_ function, since we aren't doing any trimming here

// Don't allow removing from the buffer within target duration of current time
// to avoid the possibility of removing the GOP currently being played which could
// cause playback stalls.
const safeRemoveToTimeLimit = currentTime - targetDuration;
Copy link
Contributor

Choose a reason for hiding this comment

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

This line and comment might be better just as part of the Math.min return. We can remove the variable since it just obfuscates the meaning (since it is just a subtraction of two numbers).

@mjneil mjneil merged commit 4a642a6 into videojs:master Oct 19, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants