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

mjneil · 2017-10-05T19:27:03Z

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

OshinKaramian

Mostly a wide series of nitpicks and test suggestions.

OshinKaramian · 2017-10-13T15:16:36Z

src/playlist.js

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


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

OshinKaramian · 2017-10-13T15:21:35Z

src/playlist.js

@@ -485,6 +510,7 @@ export const estimateSegmentRequestTime = function(segmentDuration,

 Playlist.duration = duration;


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

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

Which would be cleaner.

OshinKaramian · 2017-10-13T15:25:08Z

test/playlist.test.js

@@ -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),


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.

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.

OshinKaramian · 2017-10-13T15:30:14Z

test/playlist.test.js

  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) {


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.

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

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.

…tion

OshinKaramian · 2017-10-17T18:36:25Z

src/playlist.js

+ * @param {Object} playlist
+ *        a media playlist object;
+ * @return {Number}
+ *         The media index of the segment


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

…ent time

OshinKaramian · 2017-10-18T19:08:29Z

src/master-playlist-controller.js

@@ -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;


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.

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.

gesinger · 2017-10-18T19:06:35Z

src/master-playlist-controller.js

@@ -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;


Since it is used in multiple places, may be worth creating a variable like Ranges.SAFE_END_DELTA and putting the comment there.

I think this one still needs to be changed.

gesinger · 2017-10-18T19:31:37Z

src/master-playlist-controller.js

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


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

gesinger · 2017-10-18T19:35:56Z

src/playlist-loader.js

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


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.

gesinger · 2017-10-18T19:37:23Z

src/playlist-loader.js

+
+      // 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) {


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?

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

gesinger · 2017-10-18T19:37:46Z

src/playlist-loader.js

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


Preference, but this can be one line.

gesinger · 2017-10-18T19:44:13Z

src/playlist.js

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


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?

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

gesinger · 2017-10-18T19:49:25Z

src/playlist.js

+  let distanceFromEnd = playlist.segments[i].duration || playlist.targetDuration;
+  const safeDistance = distanceFromEnd + playlist.targetDuration * 2;
+
+  while (i--) {


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

gesinger · 2017-10-18T19:52:30Z

src/segment-loader.js

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


Is this from a different PR?

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.

gesinger · 2017-10-18T19:56:25Z

test/playlist.test.js

+               // 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');


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

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

does that make sense? any ideas on how i can make that more clear in the comment?

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

gesinger · 2017-10-18T19:57:48Z

test/playlist.test.js

+    ]
+  };
+
+  const expected = 3;


Preference, but for this (and below) the variables don't add much to just making the call in a single line.

gesinger · 2017-10-18T22:50:54Z

src/master-playlist-controller.js

@@ -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;


I think this one still needs to be changed.

gesinger · 2017-10-18T22:51:12Z

src/master-playlist-controller.js

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


I'd say either caps Return or remove period

gesinger · 2017-10-18T23:05:25Z

src/playlist-loader.js

      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];


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

gesinger · 2017-10-18T23:08:26Z

src/ranges.js

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


Maybe we should just use TIME_FUDGE_FACTOR * 3?

gesinger · 2017-10-18T23:11:47Z

test/playlist.test.js

+               // 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');


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

gesinger · 2017-10-18T23:15:17Z

test/videojs-contrib-hls.test.js

@@ -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' +


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.

gesinger · 2017-10-19T16:26:46Z

src/segment-loader.js

+  // cause playback stalls.
+  const safeRemoveToTimeLimit = currentTime - targetDuration;
+
+  // Chrome has a hard limit of 150MB of


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

gesinger · 2017-10-19T16:28:40Z

src/segment-loader.js

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


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 added the in progress label Oct 5, 2017

mjneil force-pushed the fix/safe-live-point branch from bad769e to d032819 Compare October 13, 2017 14:19

OshinKaramian suggested changes Oct 13, 2017

View reviewed changes

OshinKaramian approved these changes Oct 13, 2017

View reviewed changes

OshinKaramian mentioned this pull request Oct 13, 2017

Fine tune blacklist handling #1269

Merged

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

mjneil force-pushed the fix/safe-live-point branch from 108e218 to d389cbb Compare October 16, 2017 20:24

mjneil added 5 commits October 17, 2017 14:38

use 3*targetDuration for safe live point instead of 3 segments

0918559

cleanup/comments

ee2e4fc

break up tests

8050a02

use the new spec requirement of last segment duration + 2 target dura…

3917852

…tion

oops

b781765

mjneil force-pushed the fix/safe-live-point branch from de7e607 to b781765 Compare October 17, 2017 18:38

OshinKaramian approved these changes Oct 17, 2017

View reviewed changes

mjneil added 4 commits October 17, 2017 14:54

update comment

54ea94f

do not let back buffer trimming remove within target duration of curr…

3881c2e

…ent time

add unit test

2e31dd6

increase threshold for stuck playlist checking

ab60fd7

OshinKaramian reviewed Oct 18, 2017

View reviewed changes

clarifying comment

fb9fbc9

gesinger reviewed Oct 18, 2017

View reviewed changes

review fixes

3a77d68

gesinger reviewed Oct 18, 2017

View reviewed changes

refactor some things

94d9d91

gesinger reviewed Oct 19, 2017

View reviewed changes

move some comments

1b69a71

gesinger approved these changes Oct 19, 2017

View reviewed changes

mjneil merged commit 4a642a6 into videojs:master Oct 19, 2017

mjneil removed the in progress label Oct 19, 2017

		@@ -485,6 +510,7 @@ export const estimateSegmentRequestTime = function(segmentDuration,

		Playlist.duration = duration;

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

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

Conversation

mjneil commented Oct 5, 2017 • edited Loading

Description

Changes

OshinKaramian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OshinKaramian Oct 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjneil commented Oct 5, 2017 •

edited

Loading

OshinKaramian Oct 13, 2017 •

edited

Loading