fix: Keep media update timeout alive so live playlists can recover #1176

brandonocasey · 2021-07-29T19:46:06Z

Description

Right now for live hls streams we can be stuck buffering even though we should have recovered because we don't attempt to retry requests. You can reproduce this:

Select Unified Streaming Live HLS
Play
Set network to offline in dev tools
Wait for the player to stall
Set network back to normal in dev tools
Player will never recover

This happens because we exclude and pause the current playlist loader when it fails a request. Then we switch to same playlist again and since it is the same playlist we don't try to request it again. So now we have no mediaUpdateTimeout and we did not request the playlist again.

The fix

We need a way to make sure that we start a mediaUpdateTimeout in media
We need mediaUpdateTimeouts to create media update timeouts
We do not need to always return true in shouldSwitchToMedia for live playlists if the playlist is the same. This gave us state we have now as we were relying on abr to refresh live playlists in some scenarios.

codecov · 2021-07-29T19:47:36Z

Codecov Report

Merging #1176 (5f2c279) into main (e8230a9) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1176      +/-   ##
==========================================
+ Coverage   86.51%   86.54%   +0.03%     
==========================================
  Files          39       39              
  Lines        9602     9619      +17     
  Branches     2219     2222       +3     
==========================================
+ Hits         8307     8325      +18     
+ Misses       1295     1294       -1

Impacted Files	Coverage Δ
src/master-playlist-controller.js	`94.62% <100.00%> (+0.02%)`	⬆️
src/playlist-loader.js	`94.29% <100.00%> (+0.56%)`	⬆️

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 e8230a9...5f2c279. Read the comment docs.

brandonocasey · 2021-07-29T19:47:56Z

src/playlist-loader.js

    const media = this.media();

    if (shouldDelay) {
      const delay = media ? ((media.partTargetDuration || media.targetDuration) / 2) * 1000 : 5 * 1000;

-      this.mediaUpdateTimeout = window.setTimeout(() => this.load(), delay);
+      this.mediaUpdateTimeout = window.setTimeout(() => {


imo this should be called something else, but I think we should save it for the playlist-loader base class refactor as dash-playlist-loader does the same thing.

gkatsev

Haven't tested yet but changes make sense to me.

gesinger · 2021-07-30T15:13:23Z

src/master-playlist-controller.js

@@ -67,7 +67,7 @@ const shouldSwitchToMedia = function({
  // This is because in LIVE, the player plays 3 segments from the end of the
  // 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 || !currentPlaylist.endList) {
+  if (!currentPlaylist) {
    log(`${sharedLogLine} as current playlist ` + (!currentPlaylist ? 'is not set' : 'is live'));


Per the comment above, will this mean that we should have a way of ignoring the low water for live in the rest of this function?

And we may need to update this line and the comment above

gesinger · 2021-07-30T15:16:27Z

src/playlist-loader.js

@@ -619,6 +613,8 @@ export default class PlaylistLoader extends EventTarget {
      return;
    }

+    this.updateMediaUpdateTimeout_(refreshDelay(playlist, true));


May be worth a comment above here saying why we update the timeout even if it's switching to the active playlist

gesinger · 2021-07-30T15:23:28Z

test/master-playlist-controller.test.js

-    endList = false;
-    currentTime = 100;
-    currentPlaylistBandwidth = 1000;
-    nextPlaylistBandwidth = 1001;
-    buffered = [];
-    this.masterPlaylistController.mainSegmentLoader_.trigger('bandwidthupdate');
-    assert.equal(
-      mediaChanges.length,
-      1,
-      'changes live media when no buffer and higher bandwidth playlist'
-    );
-    buffered = [[0, 100], [100, 109]];
-    this.masterPlaylistController.mainSegmentLoader_.trigger('bandwidthupdate');
-    assert.equal(
-      mediaChanges.length,
-      2,
-      'changes live media when insufficient forward buffer and higher ' +
-               'bandwidth playlist'
-    );
-    buffered = [[0, 100], [100, 130]];
-    this.masterPlaylistController.mainSegmentLoader_.trigger('bandwidthupdate');
-    assert.equal(
-      mediaChanges.length,
-      3,
-      'changes live media when sufficient forward buffer and higher ' +
-               'bandwidth playlist'
-    );
-
-    mediaChanges.length = 0;
-


Do we want to remove these tests?

These only tested that live content always switches as far as I could tell. We might have to add one more back in depending on changes that I have to make in shouldSwitchToMedia.

brandonocasey · 2021-08-06T15:14:53Z

src/master-playlist-controller.js

@@ -351,7 +356,7 @@ export class MasterPlaylistController extends videojs.EventTarget {
  checkABR_() {
    const nextPlaylist = this.selectPlaylist();

-    if (this.shouldSwitchToMedia_(nextPlaylist)) {
+    if (nextPlaylist && this.shouldSwitchToMedia_(nextPlaylist)) {


minor fix here. I saw that we throw an error if nextPlaylist is undefined.

gkatsev

Can confirm that this fixes the issue of going offline and coming back!

src/playlist-loader.js

brandonocasey added 2 commits July 29, 2021 15:25

fix: Keep media update timeout alive so live playlists can recover

0f5ed77

consistency

378234a

brandonocasey commented Jul 29, 2021

View reviewed changes

typo

e4aa070

gkatsev previously approved these changes Jul 30, 2021

View reviewed changes

gesinger reviewed Jul 30, 2021

View reviewed changes

brandonocasey added 2 commits August 5, 2021 16:38

add comment

9ecba52

move live playlist check down, bring back tests

9ea139f

brandonocasey dismissed gkatsev’s stale review via 9ea139f August 6, 2021 15:13

brandonocasey commented Aug 6, 2021

View reviewed changes

gkatsev reviewed Aug 17, 2021

View reviewed changes

src/playlist-loader.js Show resolved Hide resolved

gkatsev and others added 3 commits August 17, 2021 13:30

Merge branch 'main' into fix/live-media-update-timeout

a600a69

Merge branch 'main' into fix/live-media-update-timeout

a812826

add a test for load delay

5f2c279

gkatsev approved these changes Aug 17, 2021

View reviewed changes

gesinger approved these changes Aug 17, 2021

View reviewed changes

brandonocasey merged commit 8b3533c into main Aug 17, 2021

brandonocasey deleted the fix/live-media-update-timeout branch August 17, 2021 19:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Keep media update timeout alive so live playlists can recover #1176

fix: Keep media update timeout alive so live playlists can recover #1176

brandonocasey commented Jul 29, 2021 •

edited

Loading

codecov bot commented Jul 29, 2021 •

edited

Loading

brandonocasey Jul 29, 2021

gkatsev left a comment

gesinger Jul 30, 2021

gesinger Jul 30, 2021

brandonocasey Aug 6, 2021

gesinger Jul 30, 2021

brandonocasey Jul 30, 2021

brandonocasey Aug 6, 2021

gkatsev left a comment

fix: Keep media update timeout alive so live playlists can recover #1176

fix: Keep media update timeout alive so live playlists can recover #1176

Conversation

brandonocasey commented Jul 29, 2021 • edited Loading

Description

The fix

codecov bot commented Jul 29, 2021 • edited Loading

Codecov Report

brandonocasey Jul 29, 2021

Choose a reason for hiding this comment

gkatsev left a comment

Choose a reason for hiding this comment

gesinger Jul 30, 2021

Choose a reason for hiding this comment

gesinger Jul 30, 2021

Choose a reason for hiding this comment

brandonocasey Aug 6, 2021

Choose a reason for hiding this comment

gesinger Jul 30, 2021

Choose a reason for hiding this comment

brandonocasey Jul 30, 2021

Choose a reason for hiding this comment

brandonocasey Aug 6, 2021

Choose a reason for hiding this comment

gkatsev left a comment

Choose a reason for hiding this comment

brandonocasey commented Jul 29, 2021 •

edited

Loading

codecov bot commented Jul 29, 2021 •

edited

Loading