fix: more accurate segment choices and logging #1127

brandonocasey · 2021-05-05T20:24:56Z

Description

This pull request refactors two functions that are the cause of latency drift in live streams during rendition changes. It also vastly improves the segmentInfoString logging logic.

`SegmentLoader#getSyncSegmentCandidate`

What we do right now is choose a segment that will never be the one we want, still append it to the source buffer and then ignore the final "end result" (saving the media/part index as the current one).

I have improved this logic in the following ways:

Try to choose an accurate segment that will be appended
If the segment isn't accurate, save the timing info but don't append saving us a lot of source buffer latency.
If the segment is accurate append it and save it's media/part index as the current media/part index.

`Playlist#getMediaInfoForTime`

Currently we add TIME_FUDGE_FACTOR for each segment and this causes us to choose the wrong segment more often then not for playlists with > 15 segments as we will be off by more than half a second in those cases. Instead we should only add TIME_FUDGE_FACTOR a single time when comparing against the time we want. We might even want to consider not using TIME_FUDGE_FACTOR here.

codecov · 2021-05-05T20:26:39Z

Codecov Report

Merging #1127 (1d74f5d) into main (1c7a63b) will decrease coverage by 0.07%.
The diff coverage is 86.48%.

❗ Current head 1d74f5d differs from pull request most recent head 504f434. Consider uploading reports for the commit 504f434 to get more accurate results

@@            Coverage Diff             @@
##             main    #1127      +/-   ##
==========================================
- Coverage   86.20%   86.13%   -0.08%     
==========================================
  Files          39       39              
  Lines        9289     9272      -17     
  Branches     2127     2123       -4     
==========================================
- Hits         8008     7986      -22     
- Misses       1281     1286       +5

Impacted Files	Coverage Δ
src/segment-loader.js	`95.39% <84.84%> (-0.42%)`	⬇️
src/playlist.js	`94.50% <100.00%> (ø)`
src/playlist-loader.js	`91.40% <0.00%> (-0.23%)`	⬇️
src/manifest.js	`97.67% <0.00%> (-0.18%)`	⬇️
src/media-groups.js	`98.90% <0.00%> (-0.07%)`	⬇️

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 458be2c...504f434. Read the comment docs.

brandonocasey · 2021-05-06T19:56:59Z

src/playlist.js


-        if (time > 0) {
+        if ((time + TIME_FUDGE_FACTOR) > 0) {


Should we keep TIME_FUDGE_FACTOR at all?

question: What is TIME_FUDGE_FACTOR used for here? Is using it causing timing issues down the line?

I think this may be the wrong place for us to use time fudging. We should probably only ever fudge time in one place: when we are trying to determine the next segment to request and are not simply walking the playlist (mediaIndex++). In that case, all requests to get media info/segment info should be as precise as possible given the current segment information, then, once the final segment is determined, we should decide whether we want to fudge the time at all to conservatively request a segment (i.e., whether to request back 1 segment, or, as an alternative, get media info for time with a fudge factor included).

I think this change basically encompasses what you are saying. We only use getMediaIndexForTime when we are not incrementing mediaIndex/partIndex. This change makes it so that cumulatively we can only be off by TIME_FUDGE_FACTOR rather than every single part/segments duration being incremented or decremented by TIME_FUDGE_FACTOR. For the sake of time I think that we might want to add a TODO here to see if we should even use TIME_FUDGE_FACTOR at all. I lean towards not using it, but it will require a bit more testing and could easily enough be an isolated change.

👍 a TODO would be good for now. This definitely is better than before.

Probably worth making an issue/a note in our major/refactoring epic

brandonocasey · 2021-05-06T19:58:25Z

test/loader-common.js

@@ -870,7 +870,7 @@ export const LoaderCommonFactory = ({
      });

      QUnit.test('drops partIndex if playlist update drops parts', function(assert) {
-        assert.timeout(100000000000000000000);
+        loader.duration_ = () => Infinity;


ie 11 has the duration as NaN here, so sync-controller gets a sync point of 1 for a vod video.

gkatsev

Trying out https://conventionalcomments.org/ again.

gkatsev · 2021-05-24T21:15:26Z

src/playlist.js


-        if (time > 0) {
+        if ((time + TIME_FUDGE_FACTOR) > 0) {


question: What is TIME_FUDGE_FACTOR used for here? Is using it causing timing issues down the line?

src/segment-loader.js

gkatsev · 2021-05-24T21:22:26Z

test/segment-loader.test.js

@@ -2967,6 +2954,65 @@ QUnit.module('SegmentLoader', function(hooks) {
      });
    });

+    QUnit.test('sync request can be thrown away', function(assert) {


issue: looks like this test is failing under IE11. https://github.com/videojs/http-streaming/pull/1127/checks?check_run_id=2659611689#step:13:478

gesinger · 2021-05-25T19:41:00Z

src/playlist.js


-        if (time > 0) {
+        if ((time + TIME_FUDGE_FACTOR) > 0) {


I think this may be the wrong place for us to use time fudging. We should probably only ever fudge time in one place: when we are trying to determine the next segment to request and are not simply walking the playlist (mediaIndex++). In that case, all requests to get media info/segment info should be as precise as possible given the current segment information, then, once the final segment is determined, we should decide whether we want to fudge the time at all to conservatively request a segment (i.e., whether to request back 1 segment, or, as an alternative, get media info for time with a fudge factor included).

src/segment-loader.js

gesinger · 2021-05-25T19:59:18Z

src/segment-loader.js

+    // throw away the isSyncRequest segment if
+    // it wouldn't have been the next segment we request.
+    if (segmentInfo.isSyncRequest) {
+      this.syncController_.saveSegmentTimingInfo({


We may have to be careful here, since we're losing some of our timing logic, which may lead to inaccurate next requests:

http-streaming/src/segment-loader.js

Lines 2860 to 2863 in 1c7a63b

// Now that the end of the segment has been reached, we can set the end time. It's

// best to wait until all appends are done so we're sure that the primary media is

// finished (and we have its end time).

this.updateTimingInfoEnd_(segmentInfo);

Should we call this function within this block? From a brief glance it seems like it only estimates the segment end point using start/duration unless we have an end. Seems like we should call it here.

I think we should probably call it. On that note we may also want to consider adding a property to the segment whether the last time we requested that segment it was a sync request (and clear it if it's requested on a non sync request). It may help us for debugging, particularly now that it's no longer being appended.

test/segment-loader.test.js

Co-authored-by: Garrett Singer <[email protected]>

src/playlist.js

…oice

gesinger · 2021-05-26T20:49:05Z

src/segment-loader.js


  return `${name} [${seq + index}/${seq + segmentLen}]` +
-    (hasPartIndex ? ` part [${partIndex}/${zeroBasedPartCount}]` : '') +
+    (hasPartIndex ? ` part [${partIndex}/${totalParts}]` : '') +


Should this take into account the 0 based indexing vs total. Maybe we can just say `[index 0, 3 total]

I think we should keep the name zeroBasedPartCount here I guess. I do subtract 1 to make it zero based.

brandonocasey commented May 6, 2021

View reviewed changes

fix: more accurate segment choices and logging

a4a45df

brandonocasey force-pushed the fix/better-segment-choice branch from bdd1e83 to a4a45df Compare May 10, 2021 16:43

brandonocasey added the needs: review label May 12, 2021

Merge branch 'main' into fix/better-segment-choice

d2d4aab

gkatsev reviewed May 24, 2021

View reviewed changes

ie 11 fix

127e075

gesinger reviewed May 25, 2021

View reviewed changes

brandonocasey and others added 7 commits May 25, 2021 16:37

pass segments to getSyncSegmentCandidate

6ddcf4c

Update src/segment-loader.js

fa97a95

Co-authored-by: Garrett Singer <[email protected]>

Update src/segment-loader.js

eae4f41

Co-authored-by: Garrett Singer <[email protected]>

inccorrect assertion text

17dda63

fix trailing space

ad088c5

time fudge factor comment

8ebbc50

call updateTimingInfoEnd and add some comments

c6f36e4

gesinger reviewed May 26, 2021

View reviewed changes

src/playlist.js Outdated Show resolved Hide resolved

better comment

2bd3f53

gesinger previously approved these changes May 26, 2021

View reviewed changes

Merge remote-tracking branch 'origin/main' into fix/better-segment-ch…

dd29591

…oice

brandonocasey dismissed gesinger’s stale review via dd29591 May 26, 2021 20:25

add PART-TARGET

a971ff0

gesinger reviewed May 26, 2021

View reviewed changes

totalParts -> zeroBasedPartCount

504f434

gesinger approved these changes May 26, 2021

View reviewed changes

brandonocasey merged commit ce03f66 into main May 26, 2021

brandonocasey deleted the fix/better-segment-choice branch May 26, 2021 20:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: more accurate segment choices and logging #1127

fix: more accurate segment choices and logging #1127

brandonocasey commented May 5, 2021

codecov bot commented May 5, 2021 •

edited

Loading

brandonocasey May 6, 2021

gkatsev May 24, 2021

gesinger May 25, 2021

brandonocasey May 25, 2021

gesinger May 26, 2021

gkatsev May 26, 2021

brandonocasey May 26, 2021

brandonocasey May 6, 2021

gkatsev left a comment

gkatsev May 24, 2021

gkatsev May 24, 2021

gesinger May 25, 2021

gesinger May 25, 2021

brandonocasey May 25, 2021

gesinger May 26, 2021

brandonocasey May 26, 2021

gesinger May 26, 2021

brandonocasey May 26, 2021

	// Now that the end of the segment has been reached, we can set the end time. It's
	// best to wait until all appends are done so we're sure that the primary media is
	// finished (and we have its end time).
	this.updateTimingInfoEnd_(segmentInfo);

fix: more accurate segment choices and logging #1127

fix: more accurate segment choices and logging #1127

Conversation

brandonocasey commented May 5, 2021

Description

SegmentLoader#getSyncSegmentCandidate

Playlist#getMediaInfoForTime

codecov bot commented May 5, 2021 • edited Loading

Codecov Report

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

gkatsev 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

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

`SegmentLoader#getSyncSegmentCandidate`

`Playlist#getMediaInfoForTime`

codecov bot commented May 5, 2021 •

edited

Loading