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.

Sign up for GitHub

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
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1feddea
fix: Only check for bad seeks after seeking and an append
brandonocasey Sep 8, 2021
96d5e9f
Apply suggestions from code review
brandonocasey Sep 14, 2021
b071028
Merge branch 'main' into fix/use-seeking
brandonocasey Sep 15, 2021
c6144ad
Merge branch 'main' into fix/use-seeking
brandonocasey Sep 15, 2021
f256728
Merge branch 'main' into fix/use-seeking
brandonocasey Sep 22, 2021
e50c413
add TODO
brandonocasey Sep 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
161 changes: 73 additions & 88 deletions src/playback-watcher.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,64 +21,6 @@ const timerCancelEvents = [
'error'
];

/**
* Returns whether or not the current time should be considered close to buffered content,
* taking into consideration whether there's enough buffered content for proper playback.
*
* @param {Object} options
* Options object
* @param {TimeRange} options.buffered
* Current buffer
* @param {TimeRange} options.videoBuffered
* Current buffered from the media source's video buffer
* @param {TimeRange} options.audioBuffered
* Current buffered from the media source's audio buffer
* @param {number} options.targetDuration
* The active playlist's target duration
* @param {number} options.currentTime
* The current time of the player
* @return {boolean}
* Whether the current time should be considered close to the buffer
*/
export const closeToBufferedContent = ({ buffered, audioBuffered, videoBuffered, targetDuration, currentTime }) => {
const twoSegmentDurations = (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;
}

const timeAhead = Ranges.timeAheadOf(bufferedToCheck[i], currentTime);

// if we are less than two video/audio segment durations behind,
// we haven't append enough to be close to buffered content.
if (timeAhead < twoSegmentDurations) {
return false;
}
}

// default to using buffered, but if we don't have one
// use video or audio buffered
if (!buffered || buffered.length === 0) {
buffered = videoBuffered || audioBuffered;
}

const nextRange = Ranges.findNextRange(buffered, currentTime);

// if we don't have a next buffered range, we cannot be close to
// buffered content.
if (!nextRange.length) {
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 nextRange.start(0) - currentTime < (targetDuration + Ranges.TIME_FUDGE_FACTOR);
};

/**
* @class PlaybackWatcher
*/
Expand Down Expand Up @@ -109,7 +51,6 @@ export default class PlaybackWatcher {
const canPlayHandler = () => this.monitorCurrentTime_();
const waitingHandler = () => this.techWaiting_();
const cancelTimerHandler = () => this.cancelTimer_();
const fixesBadSeeksHandler = () => this.fixesBadSeeks_();

const mpc = this.masterPlaylistController_;

Expand All @@ -134,7 +75,38 @@ export default class PlaybackWatcher {
this.tech_.on(['seeked', 'seeking'], loaderChecks[type].reset);
});

this.tech_.on('seekablechanged', fixesBadSeeksHandler);
/**
* We check if a seek was into a gap through the following steps:
* 1. We get a seeking event and we do not get a seeked event. This means that
* a seek was attempted but not completed.
* 2. We run `fixesBadSeeks_` on segment loader appends. This means that we already
* removed everything from our buffer and appended a segment, and should be ready
* to check for gaps.
*/
const setSeekingHandlers = (fn) => {
['main', 'audio'].forEach((type) => {
mpc[`${type}SegmentLoader_`][fn]('appended', this.seekingAppendCheck_);
brandonocasey marked this conversation as resolved.
Show resolved Hide resolved
});
};

this.seekingAppendCheck_ = () => {
if (this.fixesBadSeeks_()) {
this.consecutiveUpdates = 0;
this.lastRecordedTime = this.tech_.currentTime();
setSeekingHandlers('off');
}
};

this.clearSeekingAppendCheck_ = () => setSeekingHandlers('off');

this.watchForBadSeeking_ = () => {
this.clearSeekingAppendCheck_();
setSeekingHandlers('on');
};

this.tech_.on('seeked', this.clearSeekingAppendCheck_);
this.tech_.on('seeking', this.watchForBadSeeking_);

this.tech_.on('waiting', waitingHandler);
this.tech_.on(timerCancelEvents, cancelTimerHandler);
this.tech_.on('canplay', canPlayHandler);
Expand All @@ -154,12 +126,14 @@ export default class PlaybackWatcher {

// Define the dispose function to clean up our events
this.dispose = () => {
this.clearSeekingAppendCheck_();
this.logger_('dispose');
this.tech_.off('seekablechanged', fixesBadSeeksHandler);
this.tech_.off('waiting', waitingHandler);
this.tech_.off(timerCancelEvents, cancelTimerHandler);
this.tech_.off('canplay', canPlayHandler);
this.tech_.off('play', playHandler);
this.tech_.off('seeking', this.watchForBadSeeking_);
this.tech_.off('seeked', this.clearSeekingAppendCheck_);

loaderTypes.forEach((type) => {
mpc[`${type}SegmentLoader_`].off('appendsdone', loaderChecks[type].updateend);
Expand Down Expand Up @@ -272,12 +246,6 @@ export default class PlaybackWatcher {
* @private
*/
checkCurrentTime_() {
if (this.fixesBadSeeks_()) {
this.consecutiveUpdates = 0;
this.lastRecordedTime = this.tech_.currentTime();
return;
}

if (this.tech_.paused() || this.tech_.seeking()) {
return;
}
Expand Down Expand Up @@ -338,6 +306,10 @@ export default class PlaybackWatcher {
return false;
}

// TODO: It's possible that these seekable checks should be moved out of this function
// and into a function that runs on seekablechange. It's also possible that we only need
// afterSeekableWindow as the buffered check at the bottom is good enough to handle before
// seekable range.
const seekable = this.seekable();
const currentTime = this.tech_.currentTime();
const isAfterSeekableRange = this.afterSeekableWindow_(
Expand Down Expand Up @@ -377,27 +349,45 @@ export default class PlaybackWatcher {

const sourceUpdater = this.masterPlaylistController_.sourceUpdater_;
const buffered = this.tech_.buffered();
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;
}
Comment on lines +352 to 381
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.


return false;
seekTo = nextRange.start(0) + Ranges.SAFE_TIME_DELTA;

this.logger_(`Buffered region starts (${nextRange.start(0)}) ` +
` just beyond seek point (${currentTime}). Seeking to ${seekTo}.`);

this.tech_.setCurrentTime(seekTo);

return true;
}

/**
Expand Down Expand Up @@ -450,11 +440,6 @@ export default class PlaybackWatcher {
const seekable = this.seekable();
const currentTime = this.tech_.currentTime();

if (this.fixesBadSeeks_()) {
// Tech is seeking or bad seek fixed, no action needed
return true;
}

if (this.tech_.seeking() || this.timer_ !== null) {
// Tech is seeking or already waiting on another action, no action needed
return true;
Expand Down
Loading