From 1a4fc1e7a32747c92c48cac95076aaaadf4aecd2 Mon Sep 17 00:00:00 2001 From: Gary Katsevman Date: Thu, 26 Mar 2020 16:56:43 -0400 Subject: [PATCH] revert: "fix: Use middleware and a wrapped function for seeking instead of relying on unreliable 'seeking' events (#161)" (#777) This reverts commit 6c68761b5d06d951d7ed20cbd5bb63f324ad2c47. This helps address #378 and videojs/video.js#6444. We still need seekTo for seekToProgramTime. --- src/master-playlist-controller.js | 14 ++++++-------- src/playback-watcher.js | 11 +++++------ src/videojs-http-streaming.js | 22 +++++++++++++--------- test/playback-watcher.test.js | 16 ++++++++++++---- 4 files changed, 36 insertions(+), 27 deletions(-) diff --git a/src/master-playlist-controller.js b/src/master-playlist-controller.js index a4ac76ae7..815a9887d 100644 --- a/src/master-playlist-controller.js +++ b/src/master-playlist-controller.js @@ -104,9 +104,8 @@ export class MasterPlaylistController extends videojs.EventTarget { useCueTags, blacklistDuration, enableLowInitialPlaylist, - sourceType, - seekTo, - cacheEncryptionKeys + cacheEncryptionKeys, + sourceType } = options; if (!url) { @@ -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; @@ -610,7 +608,7 @@ export class MasterPlaylistController extends videojs.EventTarget { } if (this.tech_.ended()) { - this.seekTo_(0); + this.tech_.setCurrentTime(0); } if (this.hasPlayed_) { @@ -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)); } } } @@ -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; }); @@ -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; diff --git a/src/playback-watcher.js b/src/playback-watcher.js index 692194975..747892ef4 100644 --- a/src/playback-watcher.js +++ b/src/playback-watcher.js @@ -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; @@ -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; } @@ -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 ` + @@ -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'}); @@ -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'}); @@ -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'}); } diff --git a/src/videojs-http-streaming.js b/src/videojs-http-streaming.js index c717a48fd..2f663374c 100644 --- a/src/videojs-http-streaming.js +++ b/src/videojs-http-streaming.js @@ -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 = { @@ -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 && @@ -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() { @@ -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_); @@ -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 diff --git a/test/playback-watcher.test.js b/test/playback-watcher.test.js index 9b217b762..083a9e04c 100644 --- a/test/playback-watcher.test.js +++ b/test/playback-watcher.test.js @@ -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'); @@ -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]]); @@ -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 @@ -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;