Skip to content

Commit

Permalink
revert: "fix: Use middleware and a wrapped function for seeking inste…
Browse files Browse the repository at this point in the history
…ad of relying on unreliable 'seeking' events (#161)" (#777)

This reverts commit 6c68761.

This helps address #378 and videojs/video.js#6444.

We still need seekTo for seekToProgramTime.
  • Loading branch information
gkatsev authored Mar 26, 2020
1 parent 90a0215 commit 1a4fc1e
Show file tree
Hide file tree
Showing 4 changed files with 36 additions and 27 deletions.
14 changes: 6 additions & 8 deletions src/master-playlist-controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -104,9 +104,8 @@ export class MasterPlaylistController extends videojs.EventTarget {
useCueTags,
blacklistDuration,
enableLowInitialPlaylist,
sourceType,
seekTo,
cacheEncryptionKeys
cacheEncryptionKeys,
sourceType
} = options;

if (!url) {
Expand All @@ -118,7 +117,6 @@ export class MasterPlaylistController extends videojs.EventTarget {
this.withCredentials = withCredentials;
this.tech_ = tech;
this.hls_ = tech.hls;
this.seekTo_ = seekTo;
this.sourceType_ = sourceType;
this.useCueTags_ = useCueTags;
this.blacklistDuration = blacklistDuration;
Expand Down Expand Up @@ -610,7 +608,7 @@ export class MasterPlaylistController extends videojs.EventTarget {
}

if (this.tech_.ended()) {
this.seekTo_(0);
this.tech_.setCurrentTime(0);
}

if (this.hasPlayed_) {
Expand All @@ -623,7 +621,7 @@ export class MasterPlaylistController extends videojs.EventTarget {
// seek forward to the live point
if (this.tech_.duration() === Infinity) {
if (this.tech_.currentTime() < seekable.start(0)) {
return this.seekTo_(seekable.end(seekable.length - 1));
return this.tech_.setCurrentTime(seekable.end(seekable.length - 1));
}
}
}
Expand Down Expand Up @@ -660,7 +658,7 @@ export class MasterPlaylistController extends videojs.EventTarget {
// readyState is 0, so it must be delayed until the tech fires loadedmetadata.
this.tech_.one('loadedmetadata', () => {
this.trigger('firstplay');
this.seekTo_(seekable.end(0));
this.tech_.setCurrentTime(seekable.end(0));
this.hasPlayed_ = true;
});

Expand All @@ -670,7 +668,7 @@ export class MasterPlaylistController extends videojs.EventTarget {
// trigger firstplay to inform the source handler to ignore the next seek event
this.trigger('firstplay');
// seek to the live point
this.seekTo_(seekable.end(0));
this.tech_.setCurrentTime(seekable.end(0));
}

this.hasPlayed_ = true;
Expand Down
11 changes: 5 additions & 6 deletions src/playback-watcher.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ export default class PlaybackWatcher {
constructor(options) {
this.tech_ = options.tech;
this.seekable = options.seekable;
this.seekTo = options.seekTo;
this.allowSeeksWithinUnsafeLiveWindow = options.allowSeeksWithinUnsafeLiveWindow;
this.media = options.media;

Expand Down Expand Up @@ -190,7 +189,7 @@ export default class PlaybackWatcher {
`seekable range ${Ranges.printableRange(seekable)}. Seeking to ` +
`${seekTo}.`);

this.seekTo(seekTo);
this.tech_.setCurrentTime(seekTo);
return true;
}

Expand Down Expand Up @@ -222,7 +221,7 @@ export default class PlaybackWatcher {
// to avoid triggering an `unknownwaiting` event when the network is slow.
if (currentRange.length && currentTime + 3 <= currentRange.end(0)) {
this.cancelTimer_();
this.seekTo(currentTime);
this.tech_.setCurrentTime(currentTime);

this.logger_(`Stopped at ${currentTime} while inside a buffered region ` +
`[${currentRange.start(0)} -> ${currentRange.end(0)}]. Attempting to resume ` +
Expand Down Expand Up @@ -262,7 +261,7 @@ export default class PlaybackWatcher {
this.logger_(`Fell out of live window at time ${currentTime}. Seeking to ` +
`live point (seekable end) ${livePoint}`);
this.cancelTimer_();
this.seekTo(livePoint);
this.tech_.setCurrentTime(livePoint);

// live window resyncs may be useful for monitoring QoS
this.tech_.trigger({type: 'usage', name: 'hls-live-resync'});
Expand All @@ -278,7 +277,7 @@ export default class PlaybackWatcher {
// allows the video to catch up to the audio position without losing any audio
// (only suffering ~3 seconds of frozen video and a pause in audio playback).
this.cancelTimer_();
this.seekTo(currentTime);
this.tech_.setCurrentTime(currentTime);

// video underflow may be useful for monitoring QoS
this.tech_.trigger({type: 'usage', name: 'hls-video-underflow'});
Expand Down Expand Up @@ -376,7 +375,7 @@ export default class PlaybackWatcher {
'nextRange start:', nextRange.start(0));

// only seek if we still have not played
this.seekTo(nextRange.start(0) + Ranges.TIME_FUDGE_FACTOR);
this.tech_.setCurrentTime(nextRange.start(0) + Ranges.TIME_FUDGE_FACTOR);

this.tech_.trigger({type: 'usage', name: 'hls-gap-skip'});
}
Expand Down
22 changes: 13 additions & 9 deletions src/videojs-http-streaming.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,6 @@ import {
comparePlaylistResolution
} from './playlist-selectors.js';
import { version } from '../package.json';
// import needed to register middleware
import './middleware-set-current-time';
import { isAudioCodec, isVideoCodec, parseContentType } from './util/codecs';

const Hls = {
Expand Down Expand Up @@ -383,6 +381,7 @@ class HlsHandler extends Component {
this.tech_ = tech;
this.source_ = source;
this.stats = {};
this.ignoreNextSeekingEvent_ = false;
this.setOptions_();

if (this.options_.overrideNative &&
Expand Down Expand Up @@ -414,11 +413,13 @@ class HlsHandler extends Component {
}
});

// Handle seeking when looping - middleware doesn't handle this seek event from the tech
this.on(this.tech_, 'seeking', function() {
if (this.tech_.currentTime() === 0 && this.tech_.player_.loop()) {
this.setCurrentTime(0);
if (this.ignoreNextSeekingEvent_) {
this.ignoreNextSeekingEvent_ = false;
return;
}

this.setCurrentTime(this.tech_.currentTime());
});

this.on(this.tech_, 'error', function() {
Expand Down Expand Up @@ -508,12 +509,9 @@ class HlsHandler extends Component {
this.options_.tech = this.tech_;
this.options_.externHls = Hls;
this.options_.sourceType = simpleTypeFromSourceType(type);
// Whenever we seek internally, we should update both the tech and call our own
// setCurrentTime function. This is needed because "seeking" events aren't always
// reliable. External seeks (via the player object) are handled via middleware.
// Whenever we seek internally, we should update the tech
this.options_.seekTo = (time) => {
this.tech_.setCurrentTime(time);
this.setCurrentTime(time);
};

this.masterPlaylistController_ = new MasterPlaylistController(this.options_);
Expand Down Expand Up @@ -716,6 +714,12 @@ class HlsHandler extends Component {
this.tech_.trigger('progress');
});

// In the live case, we need to ignore the very first `seeking` event since
// that will be the result of the seek-to-live behavior
this.on(this.masterPlaylistController_, 'firstplay', function() {
this.ignoreNextSeekingEvent_ = true;
});

this.setupQualityLevels_();

// do nothing if the tech has been disposed already
Expand Down
16 changes: 12 additions & 4 deletions test/playback-watcher.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,9 @@ QUnit.test('skips over gap in Chrome due to video underflow', function(assert) {

let seeks = [];

this.player.vhs.setCurrentTime = (time) => seeks.push(time);
this.player.tech_.setCurrentTime = (time) => {
seeks.push(time);
};

this.player.tech_.trigger('waiting');

Expand Down Expand Up @@ -436,9 +438,11 @@ QUnit.test('fixes bad seeks', function(assert) {
playbackWatcher.tech_ = {
off: () => {},
seeking: () => seeking,
setCurrentTime: (time) => {
seeks.push(time);
},
currentTime: () => currentTime
};
this.player.vhs.setCurrentTime = (time) => seeks.push(time);

currentTime = 50;
seekable = videojs.createTimeRanges([[1, 45]]);
Expand Down Expand Up @@ -487,12 +491,14 @@ QUnit.test('corrects seek outside of seekable', function(assert) {
playbackWatcher.tech_ = {
off: () => {},
seeking: () => seeking,
setCurrentTime: (time) => {
seeks.push(time);
},
currentTime: () => currentTime,
// mocked out
paused: () => false,
buffered: () => videojs.createTimeRanges()
};
this.player.vhs.setCurrentTime = (time) => seeks.push(time);

// waiting

Expand Down Expand Up @@ -575,12 +581,14 @@ function(assert) {
playbackWatcher.tech_ = {
off: () => {},
seeking: () => seeking,
setCurrentTime: (time) => {
seeks.push(time);
},
currentTime: () => currentTime,
// mocked out
paused: () => false,
buffered: () => videojs.createTimeRanges()
};
this.player.vhs.setCurrentTime = (time) => seeks.push(time);

playbackWatcher.allowSeeksWithinUnsafeLiveWindow = true;

Expand Down

0 comments on commit 1a4fc1e

Please sign in to comment.