Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 6 commits into from
Sep 22, 2021

Conversation

brandonocasey
Copy link
Contributor

@brandonocasey brandonocasey commented Sep 8, 2021

Description

In our current code we run fixesBadSeeks_() on tech waiting and our emulated tech waiting. This is problematic because at the time of tech waiting we won't have removed content from our segment loaders buffers. This means that any seek backwards within targetDuration will be skipped as a gap.

Example:

1. seek to 300 tech waiting fires, but there is nothing in the buffer at this time.
2. seek back to 290, tech waiting fires and we have buffer starting at 299s
3. playback watcher calls `fixedBadSeeks_` and determines that there is a gap from 290-299 as we have yet to append content.
4. playback watcher skips to 299

Previously we check to see if an append had happened by checking the buffer, but that isn't specific enough as large target durations will have issues as seen above. Now we use seeking events.

@codecov
Copy link

codecov bot commented Sep 8, 2021

Codecov Report

Merging #1195 (e50c413) into main (324af10) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1195      +/-   ##
==========================================
- Coverage   86.61%   86.60%   -0.01%     
==========================================
  Files          39       39              
  Lines        9651     9652       +1     
  Branches     2235     2231       -4     
==========================================
  Hits         8359     8359              
- Misses       1292     1293       +1     
Impacted Files Coverage Δ
src/playback-watcher.js 98.79% <100.00%> (-0.40%) ⬇️

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 324af10...e50c413. Read the comment docs.

src/playback-watcher.js Outdated Show resolved Hide resolved
src/playback-watcher.js Outdated Show resolved Hide resolved
src/playback-watcher.js Show resolved Hide resolved
Comment on lines +348 to 377
const audioBuffered = sourceUpdater.audioBuffer ? sourceUpdater.audioBuffered() : null;
const videoBuffered = sourceUpdater.videoBuffer ? sourceUpdater.videoBuffered() : null;

// 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 bufferedToCheck = [audioBuffered, videoBuffered];

for (let i = 0; i < bufferedToCheck.length; i++) {
// skip null buffered
if (!bufferedToCheck[i]) {
continue;
}

if (
closeToBufferedContent({
buffered,
audioBuffered: sourceUpdater.audioBuffer ? sourceUpdater.audioBuffered() : null,
videoBuffered: sourceUpdater.videoBuffer ? sourceUpdater.videoBuffered() : null,
targetDuration: this.media().targetDuration,
currentTime
})
) {
const nextRange = Ranges.findNextRange(buffered, currentTime);
const timeAhead = Ranges.timeAheadOf(bufferedToCheck[i], currentTime);

seekTo = nextRange.start(0) + Ranges.SAFE_TIME_DELTA;
this.logger_(`Buffered region starts (${nextRange.start(0)}) ` +
` just beyond seek point (${currentTime}). Seeking to ${seekTo}.`);
// if we are less than two video/audio segment durations behind,
// we haven't appended enough to call this a bad seek.
if (timeAhead < twoSegmentDurations) {
return false;
}
}

this.tech_.setCurrentTime(seekTo);
return true;
const nextRange = Ranges.findNextRange(buffered, currentTime);

// we have appended enough content, but we don't have anything buffered
// to seek over the gap
if (nextRange.length === 0) {
return false;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any specific reason for moving all of this out of a separate function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After reducing the function to just the twoSegmentDurations check and the nextRange check it seemed silly to call findNextRange in the helper function and then directly afterwards since we need access to it. So I think it made more sense to move the helper function in here.

test/playback-watcher.test.js Outdated Show resolved Hide resolved
@gkatsev
Copy link
Member

gkatsev commented Sep 17, 2021

We have another test stream that this PR seems to fix #1202

@harvister
Copy link

harvister commented Sep 18, 2021

Hi, I was having a similar issue over at #1202. I built and tested your most recent commit c6144ad, as @gkatsev mentioned it could also fix my issue.

I can confirm that it does indeed fix the dramatic several second skip-back and/or infinite looping that I was experiencing, so thank you for that.

However, it still does not play as cleanly as all the other players I mentioned in my post. After your fix, I now experience a stutter instead of the jump-back, basically a half second or less pause in both the video and the audio which is still quite disruptive for the user as you can see the pause and you hear a staticy audio artifact. Much better though, thanks.

@brandonocasey brandonocasey merged commit 9d6505a into main Sep 22, 2021
@brandonocasey brandonocasey deleted the fix/use-seeking branch September 22, 2021 19:10
@ashiskumarsahu
Copy link

Hi,
On which version of videojs, the fix is available?
I am using following:
https://vjs.zencdn.net/7.10.2/video.js
https://vjs.zencdn.net/7.17.1/video.js

But issue still persist.
Actually I am generating a playout stream having 1200 segments in playlist.
I am trying to play old chunks based on user plays the content with some timing calculation logic:

player.currentTime(900); //Like this

But if network fluctuates, it playing segments from backward(old chunks).

How it should behave:
suppose current loaded segment: 1080/seg-549.ts
when network switch it should load from 720/549.ts or 720/550.ts
Currently its loading 720/200.ts

Thank You

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants