fix: better time to first frame for live playlists #1105

brandonocasey · 2021-03-25T19:58:43Z

Does the following:

~~Fixes some off by 1 errors~~
Loops backwards over segments, rather than forwards when looking for a sync-point in live videos
Actually uses live delay timing values as live delays, rather than doing it in a round-about fashion.

src/playlist.js

brandonocasey · 2021-03-25T20:05:34Z

src/playlist.js

@@ -293,13 +293,18 @@ export const playlistEnd = function(playlist, expired, useSafeLiveEnd, liveEdgeP

  expired = expired || 0;

-  const endSequence = useSafeLiveEnd ? safeLiveIndex(playlist, liveEdgePadding) : playlist.segments.length;


It doesn't really make sense to get a safe live index, based on how many seconds we want to start behind the playback head. Then get how many seconds that index starts at from behind the end of the playlist. We should instead get the timing information for the end of the playlist. Then subtract the live edge padding from that.

This was causing us to download 3/4 (but it could be any number really, especially for a fast connection.) segments before a frame of video would even be shown, because we were setting seekableEnd to an exact segment end point. We would then download a segment that should start -0.001s before the start of seekable end but when we append it the buffered region is actually +0.001s ahead of seekable end. Eventually playback-watcher would seek us into the buffered range.

This fix is better because:

The spec stats that we should start 3 target durations behind live

We will almost always start in the middle of a segment as targetDuration is almost never exact, preventing any rounding errors from stalling playback.

brandonocasey · 2021-03-25T20:08:22Z

src/sync-controller.js

+const getSegmentIndex = (i, playlist, currentTime = 0) => {
+  const segments = playlist.segments;
+
+  return (playlist.endList || currentTime === 0) ? i : segments.length - (i + 1);


Loop from the back of the playlist when looking for a live sync point, as we are much more likely to be closer to the end.

src/playlist.js

gesinger · 2021-03-26T14:04:28Z

src/sync-controller.js

@@ -38,7 +44,8 @@ export const syncPointStrategies = [
      currentTime = currentTime || 0;

      for (let i = 0; i < segments.length; i++) {
-        const segment = segments[i];
+        const segmentIndex = getSegmentIndex(i, playlist, currentTime);


Since ProgramDateTime is rare to see in VOD, the VOD strategy should be used more often for VOD, and, in theory, this code should not need to run very often for VOD (unless there's heavy rendition switching going on), we may be better off just always starting from the end, and keeping the code simpler.

I don't think it's that much code and we share this bit between the two loops. I think that we will end up with more code if we change this to looping backwards.

codecov · 2021-03-26T14:56:54Z

Codecov Report

Merging #1105 (ff70d0b) into main (1b990f1) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1105      +/-   ##
==========================================
- Coverage   86.08%   86.06%   -0.02%     
==========================================
  Files          38       38              
  Lines        8931     8923       -8     
  Branches     2001     1998       -3     
==========================================
- Hits         7688     7680       -8     
  Misses       1243     1243

Impacted Files	Coverage Δ
src/playlist.js	`93.47% <100.00%> (-0.57%)`	⬇️
src/sync-controller.js	`96.72% <100.00%> (+0.09%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1b990f1...ff70d0b. Read the comment docs.

gkatsev · 2021-03-29T22:57:55Z

I have to say I'm a bit leery of removing the safeLiveIndex, though, it's probably fine. Would like to hear back from @gesinger if he has more thoughts on the comment he brought up.

brandonocasey · 2021-03-30T15:39:27Z

So the reason I removed safeLiveIndex is because we only used it to get the index of a segment based on a period of time that we want to be behind. Then we use that index to calculate the duration at which that segment would start, which will put seekableEnd at an estimated segment boundary. If that estimated segment boundary is wrong we will create a gap.
Example:

playlist is 3000s
targetDuration is 6
last segment durations from the manifest are -1 = 5.597, -2 = 5.5, -3 = 5.9, -4 = 5.5`.

safeLiveDelay will be 17.9s behind live (targetDuration * 2, plus last segment duration)
safeLiveIndex will return segment at index -4
intervalDuration will return 2977.503
this will be a nearly exact segment boundary for the start to segment at index -4

With this code just start at the specific distance away from the current end of the playlist that we want to be.
Example:

playlist is 3000s
targetDuration is 6
last segment durations from the manifest are -1 = 5.597, -2 = 5.5, -3 = 5.9, -4 = 5.5`.

We get the duration of the entire playlist, 3000s
We subtract our safeLiveDistance of 18s (targetDuration * 3) to get 2982s
This will be most of the way through the segment at index -4 but not on a boundary.

src/playlist.js

brandonocasey · 2021-03-30T16:22:27Z

test/playlist.test.js

-    assert.equal(playlistEnd, 148.5, 'playlist end at the last segment end');
-  }
-);
+  QUnit.test('playlistEnd uses default live edge padding with useSafeLiveEnd true', function(assert) {


The new tests are here

brandonocasey · 2021-03-30T17:23:56Z

The test code should be reviewed with whitespace differences turned off as I moved things around so that running all the playlist tests isn't so difficult.

gesinger · 2021-04-01T14:42:30Z

test/playlist.test.js


-QUnit.module('Playlist estimateSegmentRequestTime');
+  QUnit.test('playlistEnd uses given live edge padding with useSafeLiveEnd true', function(assert) {


Looks like this test is included twice.

brandonocasey force-pushed the fix/better-ttff branch 2 times, most recently from 7477bd8 to e0d34d4 Compare March 25, 2021 20:02

brandonocasey commented Mar 25, 2021

View reviewed changes

src/playlist.js Outdated Show resolved Hide resolved

brandonocasey commented Mar 25, 2021

View reviewed changes

brandonocasey force-pushed the fix/better-ttff branch from e0d34d4 to b0adf1a Compare March 25, 2021 20:06

fix: correct playlist end logic for live playlists

3cc24ca

brandonocasey force-pushed the fix/better-ttff branch from b0adf1a to 3cc24ca Compare March 25, 2021 20:07

brandonocasey commented Mar 25, 2021

View reviewed changes

gesinger reviewed Mar 26, 2021

View reviewed changes

test fixes

31a4fc7

brandonocasey and others added 3 commits March 26, 2021 10:58

cr

cef33d7

remove safeLiveIndex

86c1c1e

Merge branch 'main' into fix/better-ttff

02f025c

Merge branch 'main' into fix/better-ttff

9677a53

gesinger reviewed Mar 30, 2021

View reviewed changes

src/playlist.js Show resolved Hide resolved

add tests and organize

f1ba676

brandonocasey commented Mar 30, 2021

View reviewed changes

Merge branch 'main' into fix/better-ttff

124d9cf

brandonocasey added the needs: review label Mar 30, 2021

gesinger reviewed Apr 1, 2021

View reviewed changes

remove duplicate test

ff70d0b

gesinger approved these changes Apr 5, 2021

View reviewed changes

brandonocasey merged commit 1e94680 into main Apr 5, 2021

brandonocasey deleted the fix/better-ttff branch April 5, 2021 15:53

brandonocasey mentioned this pull request Apr 5, 2021

feat: use serverControl and preloadSegment llhls features behind a flag #1078

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: better time to first frame for live playlists #1105

fix: better time to first frame for live playlists #1105

brandonocasey commented Mar 25, 2021 •

edited

Loading

brandonocasey Mar 25, 2021 •

edited

Loading

brandonocasey Mar 25, 2021

gesinger Mar 26, 2021

brandonocasey Mar 26, 2021

codecov bot commented Mar 26, 2021 •

edited

Loading

gkatsev commented Mar 29, 2021

brandonocasey commented Mar 30, 2021

brandonocasey Mar 30, 2021

brandonocasey commented Mar 30, 2021

gesinger Apr 1, 2021

		@@ -293,13 +293,18 @@ export const playlistEnd = function(playlist, expired, useSafeLiveEnd, liveEdgeP

		expired = expired \|\| 0;

		const endSequence = useSafeLiveEnd ? safeLiveIndex(playlist, liveEdgePadding) : playlist.segments.length;


		QUnit.module('Playlist estimateSegmentRequestTime');
		QUnit.test('playlistEnd uses given live edge padding with useSafeLiveEnd true', function(assert) {

fix: better time to first frame for live playlists #1105

fix: better time to first frame for live playlists #1105

Conversation

brandonocasey commented Mar 25, 2021 • edited Loading

brandonocasey Mar 25, 2021 • edited Loading

Choose a reason for hiding this comment

brandonocasey Mar 25, 2021

Choose a reason for hiding this comment

gesinger Mar 26, 2021

Choose a reason for hiding this comment

brandonocasey Mar 26, 2021

Choose a reason for hiding this comment

codecov bot commented Mar 26, 2021 • edited Loading

Codecov Report

gkatsev commented Mar 29, 2021

brandonocasey commented Mar 30, 2021

brandonocasey Mar 30, 2021

Choose a reason for hiding this comment

brandonocasey commented Mar 30, 2021

gesinger Apr 1, 2021

Choose a reason for hiding this comment

brandonocasey commented Mar 25, 2021 •

edited

Loading

brandonocasey Mar 25, 2021 •

edited

Loading

codecov bot commented Mar 26, 2021 •

edited

Loading