Fix/more llhls fixes #1201

brandonocasey · 2021-09-15T20:11:29Z

Description

Based on #1195 for now, but we can pull it out if we want as only one of the fixes requires it.

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

brandonocasey · 2021-09-15T20:13:52Z

src/master-playlist-controller.js

@@ -1431,7 +1431,7 @@ export class MasterPlaylistController extends videojs.EventTarget {
  onSyncInfoUpdate_() {
    let audioSeekable;

-    if (!this.masterPlaylistLoader_) {
+    if (!this.masterPlaylistLoader_ || this.sourceUpdater_.hasCreatedSourceBuffers()) {


This was causing us to:

As soon as audio/video segment loader fires this we come in here

Since only one of the loaders is actually ready at this point, we get seekable from just that one

We seek to the end of this invalid seekable range

We come back in here again and this time we get an intersection between the two and a valid seekable

We seek to the valid seekable end via playback watcher

This causes content to be removes and things to be downloaded/appended again.

Would be worth adding a small comment above stating that. Something like: // if only one source buffer has been created, the seekable range will be invalid

brandonocasey · 2021-09-15T20:14:37Z

src/playback-watcher.js

@@ -350,7 +350,9 @@ export default class PlaybackWatcher {

    // verify that at least two segment durations have been
    // appended before checking for a gap.
-    const twoSegmentDurations = (this.media().targetDuration - Ranges.TIME_FUDGE_FACTOR) * 2;
+    const targetDuration = this.media().partTargetDuration || this.media().targetDuration;


This helps skip gaps faster in llhls, mostly this is helping with our bad segment choice at the start of a playlist. It is likely something we want to do anyway though, as we don't want to wait very long to skip gaps in llhls.

brandonocasey · 2021-09-15T20:16:48Z

src/playlist-loader.js

@@ -257,7 +257,8 @@ const getAllSegments = function(media) {
 export const isPlaylistUnchanged = (a, b) => a === b ||
  (a.segments && b.segments && a.segments.length === b.segments.length &&
   a.endList === b.endList &&
-   a.mediaSequence === b.mediaSequence);
+   a.mediaSequence === b.mediaSequence &&
+   (a.preloadSegment && b.preloadSegment && a.preloadSegment === b.preloadSegment));


Without this line we can trigger playlistunchaned for llhls playlists that have changed.

brandonocasey · 2021-09-15T20:17:18Z

src/playlist.js

@@ -117,7 +117,19 @@ const backwardDuration = function(playlist, endSequence) {
      return { result: result + segment.end, precise: true };
    }

-    result += segment.duration;
+    if (segment.preload) {


This prevents us from considering the full segment duration for preload segments that do not have all of their parts yet.

should we have a helper like segmentDurationWithParts(segment) which does this piece and below?

brandonocasey · 2021-09-15T20:18:23Z

src/playlist.js

@@ -321,19 +333,19 @@ export const playlistEnd = function(playlist, expired, useSafeLiveEnd, liveEdgeP

  expired = expired || 0;

-  let lastSegmentTime = intervalDuration(
+  let lastSegmentEndTime = intervalDuration(


A more descriptive name as it isn't the start time, but the end time we are getting here.

src/playlist.js

brandonocasey · 2021-09-15T20:19:11Z

src/segment-loader.js

@@ -1984,7 +1984,7 @@ export default class SegmentLoader extends videojs.EventTarget {
    this.setTimeMapping_(segmentInfo.timeline);

    // for tracking overall stats
-    this.updateMediaSecondsLoaded_(segmentInfo.segment);
+    this.updateMediaSecondsLoaded_(segmentInfo.part || segmentInfo.segment);


src/sync-controller.js

brandonocasey · 2021-09-16T20:03:58Z

src/playlist-loader.js

@@ -662,6 +665,8 @@ export default class PlaylistLoader extends EventTarget {
      this.trigger('mediachanging');
    }

+    this.pendingMedia_ = playlist;


Added this to prevent switching to the same playlist twice at the start of playback.

Where does it get used?

masterPlaylistLoader_.shouldSwitchToMedia_()

brandonocasey · 2021-09-17T18:21:17Z

src/master-playlist-controller.js

-      buffered.end(buffered.length - 1) - this.tech_.currentTime() : 0;
-
+    const currentTime = this.tech_.currentTime();
+    const forwardBuffer = Ranges.timeAheadOf(buffered, currentTime);


Use timeAheadOf so that we consider gaps in our forward buffer and don't try to switch when we have enough buffer but have gaps.

brandonocasey · 2021-09-17T18:21:52Z

src/master-playlist-controller.js


    return shouldSwitchToMedia({
+      isBuffered,


We don't really want to switch at the start of playback for live streams before we have actually started. It almost doubles are time to first playback.

brandonocasey · 2021-09-17T18:22:39Z

src/playback-watcher.js

    // appended before checking for a gap.
-    const twoSegmentDurations = (this.media().targetDuration - Ranges.TIME_FUDGE_FACTOR) * 2;
+    const durations = media.partTargetDuration ? media.partTargetDuration : media.targetDuration * 2;


I lowered this to one part target duration. parts are so tiny that its almost impossible not be be off by a few ms.

brandonocasey · 2021-09-20T20:38:26Z

src/sync-controller.js

@@ -46,35 +46,33 @@ export const syncPointStrategies = [
        const datetimeMapping =
          syncController.timelineToDatetimeMappings[segment.timeline];

-        if (!datetimeMapping) {
+        if (!datetimeMapping || !segment.dateTimeObject) {


short circuit earlier

codecov · 2021-09-20T20:44:44Z

Codecov Report

Merging #1201 (db2da0f) into main (1fe2df1) will increase coverage by 0.08%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1201      +/-   ##
==========================================
+ Coverage   86.33%   86.42%   +0.08%     
==========================================
  Files          39       39              
  Lines        9701     9743      +42     
  Branches     2243     2263      +20     
==========================================
+ Hits         8375     8420      +45     
+ Misses       1326     1323       -3

Impacted Files	Coverage Δ
src/master-playlist-controller.js	`94.78% <100.00%> (+0.16%)`	⬆️
src/playback-watcher.js	`98.80% <100.00%> (+<0.01%)`	⬆️
src/playlist-loader.js	`95.04% <100.00%> (+0.33%)`	⬆️
src/playlist.js	`94.56% <100.00%> (+0.23%)`	⬆️
src/segment-loader.js	`96.31% <100.00%> (+0.07%)`	⬆️
src/sync-controller.js	`97.44% <100.00%> (+0.49%)`	⬆️

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 1fe2df1...db2da0f. Read the comment docs.

package.json

gkatsev · 2021-10-14T17:44:50Z

src/playlist.js

@@ -117,7 +117,19 @@ const backwardDuration = function(playlist, endSequence) {
      return { result: result + segment.end, precise: true };
    }

-    result += segment.duration;
+    if (segment.preload) {


should we have a helper like segmentDurationWithParts(segment) which does this piece and below?

src/segment-loader.js

gkatsev · 2021-10-14T17:46:58Z

src/segment-loader.js

        // we must "resync" the segment loader when we switch renditions and
        // the segment loader is already synced to the previous rendition
-        this.resyncLoader();
+        this.resetLoader();


How come we're switching from resync to reset here? Also, the comment would need to be changed.

The only extra thing that reset does that resync does not is fetchAtBuffer_ = false. I think that we cannot use the end of the buffer for live stream segment selection reliably. This was especially true when changing renditions near the start of playback, as it would put us behind. Non-independent segments also don't change the buffer by themselves so if we switch while appending them we would be parts behind. We could change this so that it only resets during a live stream, otherwise it resyncs.

Might be nice to do this only for live streams, though, if @gesinger thinks it's fine to always do it, I won't have any objections.

changed it to only reset during live streams.

src/segment-loader.js

brandonocasey · 2021-10-18T20:02:50Z

Added tests for segmentInfoString and added segmentDurationWithParts + tests

gkatsev

LGTM and improves start-up time a lot.

gesinger · 2021-10-19T14:35:29Z

src/master-playlist-controller.js

@@ -47,6 +47,7 @@ const sumLoaderStat = function(stat) {
 };
 const shouldSwitchToMedia = function({
  currentPlaylist,
+  isBuffered,


It took me a little to figure out what this reflected. It might make sense for shouldSwitchToMedia to take in the overall buffer and do the calculations on its own internally.

gesinger · 2021-10-19T14:37:18Z

src/master-playlist-controller.js

@@ -78,6 +79,10 @@ const shouldSwitchToMedia = function({
  // playlist, and if `BUFFER_LOW_WATER_LINE` is greater than the duration availble
  // in those segments, a viewer will never experience a rendition upswitch.
  if (!currentPlaylist.endList) {
+    if (!isBuffered && typeof currentPlaylist.partTargetDuration === 'number') {
+      log(`not ${sharedLogLine} as current playlist is live llhls, but currentTime isn't in buffered.`);


We should add the comment you made in github below above here.

gesinger · 2021-10-19T14:39:41Z

src/master-playlist-controller.js

@@ -1431,7 +1431,7 @@ export class MasterPlaylistController extends videojs.EventTarget {
  onSyncInfoUpdate_() {
    let audioSeekable;

-    if (!this.masterPlaylistLoader_) {
+    if (!this.masterPlaylistLoader_ || this.sourceUpdater_.hasCreatedSourceBuffers()) {


Would be worth adding a small comment above stating that. Something like: // if only one source buffer has been created, the seekable range will be invalid

gesinger · 2021-10-19T15:08:58Z

src/playback-watcher.js

+
+    // verify that at least two segment durations or one part duration have been
+    // appended before checking for a gap.
+    const durations = media.partTargetDuration ? media.partTargetDuration :


Maybe we can rename this to something like minAppendedDuration.

gesinger · 2021-10-19T15:10:12Z

src/playlist-loader.js

@@ -662,6 +665,8 @@ export default class PlaylistLoader extends EventTarget {
      this.trigger('mediachanging');
    }

+    this.pendingMedia_ = playlist;


Where does it get used?

gesinger · 2021-10-19T15:18:03Z

src/segment-loader.js

        // the segment loader is already synced to the previous rendition
-        this.resyncLoader();
+
+        // for live streams reset, for vod resync


It may be worth adding why we reset for live vs resync for VOD.

gesinger · 2021-10-19T15:31:18Z

test/loader-common.js

+
+        loader.playlist(newPlaylist);
+
+        assert.true(resyncCalled, 'resync was called');


Since reset also calls resync, we'll want to check reset as well.

gesinger · 2021-10-19T15:33:07Z

test/loader-common.js

+      playlist.segments[4].parts[1].independent = true;
+      const segmentInfo = loader.chooseNextRequest_();
+
+      assert.equal(segmentInfo.partIndex, 1, 'previous part');


Doesn't this match the partIndex above?

it does, but we also don't have anything buffered so it tries to choose sepmentIndex 4, partIndex 2 but that isn't independent so we choose the current partIndex. Let me see if I can find something better than this though.

gesinger · 2021-10-19T15:33:46Z

test/master-playlist-controller.test.js

+  mpc.masterPlaylistLoader_.media = () => ({id: 'foo', endList: false, partTargetDuration: 5});
+  const nextPlaylist = {id: 'bar', endList: false, partTargetDuration: 5};
+
+  assert.notOk(mpc.shouldSwitchToMedia_(nextPlaylist), 'should switch without currentPlaylist');


Should this be "shouldn't switch when video not buffered"?

gesinger · 2021-10-19T15:34:46Z

test/master-playlist-controller.test.js

+  mpc.masterPlaylistLoader_.media = () => ({id: 'foo', endList: false, partTargetDuration: 5});
+  const nextPlaylist = {id: 'bar', endList: false, partTargetDuration: 5};
+
+  assert.ok(mpc.shouldSwitchToMedia_(nextPlaylist), 'should switch without currentPlaylist');


There should be a currentPlaylists here I imagine?

src/master-playlist-controller.js

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

brandonocasey and others added 4 commits September 8, 2021 11:45

fix: Only check for bad seeks after seeking and an append

1feddea

Apply suggestions from code review

96d5e9f

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

Merge branch 'main' into fix/use-seeking

b071028

Merge branch 'main' into fix/use-seeking

c6144ad

brandonocasey commented Sep 15, 2021

View reviewed changes

fix: more llhls fixes

ff350fa

brandonocasey force-pushed the fix/more-llhls-fixes branch from 897da8c to ff350fa Compare September 15, 2021 20:16

brandonocasey commented Sep 15, 2021

View reviewed changes

do the same for forward duration

9e74eae

brandonocasey commented Sep 15, 2021

View reviewed changes

src/playlist.js Outdated Show resolved Hide resolved

brandonocasey commented Sep 15, 2021

View reviewed changes

src/sync-controller.js Outdated Show resolved Hide resolved

do not switch to the same media twice

c42b1e1

brandonocasey commented Sep 16, 2021

View reviewed changes

brandonocasey added 6 commits September 17, 2021 10:55

bad part guess and reset loader rather than resync

e5fd656

update video.js

a3b9f09

small segment choice fixes

1704056

independent and timeAheadOf fixes

cf340b0

import ranges once

33052bb

less time for llhls to skip bad seeks

b40a28c

brandonocasey commented Sep 17, 2021

View reviewed changes

brandonocasey commented Sep 20, 2021

View reviewed changes

add/update tests

5fd0ff3

add more tests

21463d4

Base automatically changed from fix/use-seeking to main September 22, 2021 19:10

gkatsev reviewed Sep 22, 2021

View reviewed changes

package.json Outdated Show resolved Hide resolved

brandonocasey and others added 2 commits September 22, 2021 15:44

Merge remote-tracking branch 'origin/main' into fix/more-llhls-fixes

5387192

Merge branch 'main' into fix/more-llhls-fixes

c7765ba

gkatsev mentioned this pull request Sep 28, 2021

Support For Low Latency HLS #721

Open

gkatsev reviewed Oct 14, 2021

View reviewed changes

brandonocasey and others added 3 commits October 18, 2021 15:29

Merge branch 'main' into fix/more-llhls-fixes

2927cd8

add helper for preload segment duration

9b2395b

segmentInfoString tests

124fcc9

brandonocasey added 2 commits October 18, 2021 16:26

reset for live and resync for vod

bd6adce

add test

64d513f

gkatsev previously approved these changes Oct 18, 2021

View reviewed changes

gesinger reviewed Oct 19, 2021

View reviewed changes

code review

fe2c2a6

brandonocasey dismissed gkatsev’s stale review via fe2c2a6 October 19, 2021 17:44

move isBuffered and forwardBuffer to shouldSwitch

feabe07

gesinger reviewed Oct 20, 2021

View reviewed changes

src/master-playlist-controller.js Outdated Show resolved Hide resolved

src/master-playlist-controller.js Outdated Show resolved Hide resolved

brandonocasey and others added 2 commits October 20, 2021 12:15

Update src/master-playlist-controller.js

42fd68c

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

Update src/master-playlist-controller.js

db2da0f

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

gesinger approved these changes Oct 20, 2021

View reviewed changes

gkatsev approved these changes Oct 20, 2021

View reviewed changes

brandonocasey merged commit bf4a458 into main Oct 20, 2021

brandonocasey deleted the fix/more-llhls-fixes branch October 20, 2021 16:42

dzianis-dashkevich mentioned this pull request Feb 28, 2022

FeatureRequest: Add configuration for a fast quality switching #1255

Closed

adrums86 mentioned this pull request Jul 25, 2023

fix(live): only reset playlist loader for LLHLS #1410

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/more llhls fixes #1201

Fix/more llhls fixes #1201

brandonocasey commented Sep 15, 2021

brandonocasey Sep 15, 2021

gesinger Oct 19, 2021

brandonocasey Sep 15, 2021

brandonocasey Sep 15, 2021

brandonocasey Sep 15, 2021

gkatsev Oct 14, 2021

brandonocasey Sep 15, 2021

brandonocasey Sep 15, 2021

brandonocasey Sep 16, 2021

gesinger Oct 19, 2021

brandonocasey Oct 19, 2021

brandonocasey Sep 17, 2021

brandonocasey Sep 17, 2021

brandonocasey Sep 17, 2021

brandonocasey Sep 20, 2021

codecov bot commented Sep 20, 2021 •

edited

Loading

gkatsev Oct 14, 2021

gkatsev Oct 14, 2021

brandonocasey Oct 18, 2021

gkatsev Oct 18, 2021

brandonocasey Oct 18, 2021

brandonocasey commented Oct 18, 2021

gkatsev left a comment

gesinger Oct 19, 2021

gesinger Oct 19, 2021

gesinger Oct 19, 2021

gesinger Oct 19, 2021

gesinger Oct 19, 2021

gesinger Oct 19, 2021

gesinger Oct 19, 2021

gesinger Oct 19, 2021

brandonocasey Oct 19, 2021

gesinger Oct 19, 2021

gesinger Oct 19, 2021


		loader.playlist(newPlaylist);

		assert.true(resyncCalled, 'resync was called');

Fix/more llhls fixes #1201

Fix/more llhls fixes #1201

Conversation

brandonocasey commented Sep 15, 2021

Description

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

codecov bot commented Sep 20, 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

brandonocasey commented Oct 18, 2021

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 20, 2021 •

edited

Loading