fix: playback watcher should fix seeking into a gap. #1192

brandonocasey · 2021-08-31T20:03:03Z

Description

playback watcher should seek us out of a gap that was seeked into.

src/playback-watcher.js

codecov · 2021-08-31T20:06:54Z

Codecov Report

Merging #1192 (375c4ff) into main (9aeb77b) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1192      +/-   ##
==========================================
+ Coverage   86.55%   86.59%   +0.03%     
==========================================
  Files          39       39              
  Lines        9619     9645      +26     
  Branches     2225     2233       +8     
==========================================
+ Hits         8326     8352      +26     
  Misses       1293     1293

Impacted Files	Coverage Δ
src/playback-watcher.js	`99.19% <100.00%> (+0.03%)`	⬆️
src/playlist-loader.js	`94.31% <100.00%> (+0.01%)`	⬆️
src/ranges.js	`99.34% <100.00%> (+0.06%)`	⬆️

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 9aeb77b...375c4ff. Read the comment docs.

src/playback-watcher.js

brandonocasey · 2021-09-01T20:26:12Z

src/playback-watcher.js

@@ -253,7 +267,7 @@ export default class PlaybackWatcher {
   * @private
   */
  checkCurrentTime_() {
-    if (this.tech_.seeking() && this.fixesBadSeeks_()) {
+    if (this.fixesBadSeeks_()) {


fixesBadSeeks_ already returns false if seeking is false on tech so checking seeking is redundant.

brandonocasey · 2021-09-01T20:26:17Z

src/playback-watcher.js

@@ -426,7 +445,7 @@ export default class PlaybackWatcher {
    const seekable = this.seekable();
    const currentTime = this.tech_.currentTime();

-    if (this.tech_.seeking() && this.fixesBadSeeks_()) {
+    if (this.fixesBadSeeks_()) {


fixesBadSeeks_ already returns false if seeking is false on tech so checking seeking is redundant.

brandonocasey · 2021-09-01T20:26:57Z

src/playlist-loader.js

@@ -340,7 +340,8 @@ export const updateMaster = (master, newMedia, unchangedCheck = isPlaylistUnchan
 *         The time in ms to wait before refreshing the live playlist
 */
 export const refreshDelay = (media, update) => {
-  const lastSegment = media.segments[media.segments.length - 1];
+  const segments = media.segments || [];


Fixes an issue I was seeing in our multi period source where sometimes media does not have segments. I can make this a different pull request, but it was easy enough.

brandonocasey · 2021-09-01T20:28:23Z

src/ranges.js

@@ -445,3 +445,32 @@ export const lastBufferedEnd = function(a) {

  return a.end(a.length - 1);
 };
+
+export const timeAheadOf = function(range, startTime) {
+  let time = 0;


This function allows us to keep the previous logic where we made sure that two segment appends had happened while the player is seeking. The problem with the previous logic is that it did not consider all of the buffer after currentTime. This code adds up all of the buffer after currentTime gaps and all.

brandonocasey · 2021-09-01T20:31:03Z

src/playback-watcher.js

        targetDuration: this.media().targetDuration,
        currentTime
      })
    ) {
-      seekTo = buffered.start(0) + Ranges.SAFE_TIME_DELTA;
-      this.logger_(`Buffered region starts (${buffered.start(0)}) ` +
+      const nextRange = Ranges.findNextRange(buffered, currentTime);


Example values for why we need this:

buffered = [[0, 10], [21, 40]] currentTime = 20

Previously we would just take buffered.start(0) which is in this case is before currentTime. Now we find the TimeRange that is after currentTime and use that.

brandonocasey · 2021-09-01T20:33:15Z

src/playback-watcher.js

-  }
+export const closeToBufferedContent = ({ buffered, audioBuffered, videoBuffered, targetDuration, currentTime }) => {
+  const twoSegmentDurations = (targetDuration - Ranges.TIME_FUDGE_FACTOR) * 2;
+  const bufferedToCheck = [audioBuffered, videoBuffered];


I wasn't sure of a better way to fix this. Sometimes we will have one or the other and sometimes we will have both. We need to make sure that both buffers have two segment durations appended ahead of current time before we do anything. We do this because fixesBadSeeks on a setTimeout whenever tech.seeking() is true. Ideally we would go back to what I had using events and never using a timer, but that would require too much testing.

brandonocasey · 2021-09-01T20:34:15Z

src/playback-watcher.js

-  if (currentTime > buffered.start(0)) {
+  // default to using buffered, but if we don't have one
+  // use video or audio buffered
+  buffered = buffered && buffered.length ? buffered : null;


I saw a case were we had no bufferIntersection but we did have a video/audio buffer. Seeking to the start of the next videoBuffer fixed it for us.

brandonocasey · 2021-09-01T20:36:15Z

src/playback-watcher.js

    return false;
  }

  // Since target duration generally represents the max (or close to max) duration of a
  // segment, if the buffer is within a segment of the current time, the gap probably
  // won't be closed, and current time should be considered close to buffered content.
-  return buffered.start(0) - currentTime < targetDuration;
+  return nextRange.start(0) - currentTime < (targetDuration + Ranges.TIME_FUDGE_FACTOR);


Added time fudge factor here because segment durations are never exact.

brandonocasey · 2021-09-01T20:36:41Z

test/playback-watcher.test.js

@@ -1006,6 +1006,12 @@ QUnit.test('jumps to buffered content if seeking just before', function(assert)
    currentTime: () => currentTime,
    buffered: () => buffered
  };
+
+  Object.assign(playbackWatcher.masterPlaylistController_.sourceUpdater_, {


had to update this test to pass videoBuffered, other than that all tests continue to work.

src/ranges.js

gesinger · 2021-09-01T20:34:15Z

src/ranges.js

+export const timeAheadOf = function(range, startTime) {
+  let time = 0;
+
+  if (!range || !range.length || !range.end) {


Is !range.end necessary?

not sure we do it in other functions in this file.

Maybe it was a mistake? I think end is a method.

src/playback-watcher.js

test/playback-watcher.test.js

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

src/playback-watcher.js

Co-authored-by: Gary Katsevman <[email protected]>

src/playback-watcher.js

Co-authored-by: Gary Katsevman <[email protected]>

src/playback-watcher.js

gesinger · 2021-09-03T14:54:38Z

src/ranges.js

+export const timeAheadOf = function(range, startTime) {
+  let time = 0;
+
+  if (!range || !range.length || !range.end) {


Maybe it was a mistake? I think end is a method.

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

brandonocasey added 2 commits August 31, 2021 16:00

fix: playback watcher should fix seeking into a gap.

1e33964

Merge remote-tracking branch 'origin/main' into fix/handle-seek-to-gap

055d78f

brandonocasey commented Aug 31, 2021

View reviewed changes

src/playback-watcher.js Outdated Show resolved Hide resolved

gesinger reviewed Aug 31, 2021

View reviewed changes

src/playback-watcher.js Outdated Show resolved Hide resolved

src/playback-watcher.js Outdated Show resolved Hide resolved

brandonocasey added 4 commits August 31, 2021 16:30

use segment loader events

57bda63

switch to using fixesBadSeeks

68834d1

fix playlists without segments

2d67064

refactor

acade5c

brandonocasey commented Sep 1, 2021

View reviewed changes

gesinger reviewed Sep 1, 2021

View reviewed changes

Apply suggestions from code review

e098076

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

gkatsev reviewed Sep 1, 2021

View reviewed changes

src/playback-watcher.js Outdated Show resolved Hide resolved

src/playback-watcher.js Show resolved Hide resolved

brandonocasey and others added 2 commits September 1, 2021 17:01

add jsdocs

d25e405

Update src/playback-watcher.js

67d1519

Co-authored-by: Gary Katsevman <[email protected]>

gkatsev reviewed Sep 2, 2021

View reviewed changes

src/playback-watcher.js Outdated Show resolved Hide resolved

Update src/playback-watcher.js

4923506

Co-authored-by: Gary Katsevman <[email protected]>

gesinger reviewed Sep 3, 2021

View reviewed changes

brandonocasey and others added 3 commits September 3, 2021 11:00

Update src/playback-watcher.js

56cbece

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

cr

b71a47a

spacing

375c4ff

gkatsev approved these changes Sep 3, 2021

View reviewed changes

gesinger approved these changes Sep 3, 2021

View reviewed changes

brandonocasey merged commit 61b8eef into main Sep 3, 2021

brandonocasey deleted the fix/handle-seek-to-gap branch September 3, 2021 17:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: playback watcher should fix seeking into a gap. #1192

fix: playback watcher should fix seeking into a gap. #1192

brandonocasey commented Aug 31, 2021

codecov bot commented Aug 31, 2021 •

edited

Loading

brandonocasey Sep 1, 2021

brandonocasey Sep 1, 2021

brandonocasey Sep 1, 2021

brandonocasey Sep 1, 2021

brandonocasey Sep 1, 2021

brandonocasey Sep 1, 2021

brandonocasey Sep 1, 2021

brandonocasey Sep 1, 2021

brandonocasey Sep 1, 2021

gesinger Sep 1, 2021

brandonocasey Sep 1, 2021

gesinger Sep 3, 2021

gesinger Sep 3, 2021

fix: playback watcher should fix seeking into a gap. #1192

fix: playback watcher should fix seeking into a gap. #1192

Conversation

brandonocasey commented Aug 31, 2021

Description

codecov bot commented Aug 31, 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

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 Aug 31, 2021 •

edited

Loading