From 8feeafbf004b5c23e3fa9e90b5e1f05f0347f428 Mon Sep 17 00:00:00 2001 From: Matt Luedke Date: Mon, 8 Jan 2018 15:07:32 -0500 Subject: [PATCH 1/2] Refs #16 Fix video does not resume playing after quality change When the HTML5 preload attribute is set to 'none' or when using Safari (even when the preload attribute is not 'none'), the video does not resume playing after the quality is changed using the quality selector menu. The quality selector plugin was listening for the 'loadeddata' event in order to know when to resume playback, but the 'loadeddata' event does not fire when the preload attribute is set to 'none', and Safari does not fetch enough data to emit a 'loadeddata' event. --- package.json | 2 +- src/js/index.js | 33 +++++++++-- src/js/middleware/SourceInterceptor.js | 4 ++ src/js/util/SafeSeek.js | 80 ++++++++++++++++++++++++++ 4 files changed, 112 insertions(+), 7 deletions(-) create mode 100644 src/js/util/SafeSeek.js diff --git a/package.json b/package.json index 58d4b19..9ef928a 100644 --- a/package.json +++ b/package.json @@ -26,7 +26,6 @@ "homepage": "https://github.com/silvermine/videojs-quality-selector#readme", "devDependencies": { "autoprefixer": "7.1.1", - "class.extend": "0.9.2", "coveralls": "2.13.1", "eslint": "4.0.0", "eslint-config-silvermine": "1.3.0", @@ -51,6 +50,7 @@ "video.js": "6.x" }, "dependencies": { + "class.extend": "0.9.2", "underscore": "1.8.3" } } diff --git a/src/js/index.js b/src/js/index.js index d1de183..998f0b1 100644 --- a/src/js/index.js +++ b/src/js/index.js @@ -3,7 +3,8 @@ var _ = require('underscore'), events = require('./events'), qualitySelectorFactory = require('./components/QualitySelector'), - sourceInterceptorFactory = require('./middleware/SourceInterceptor'); + sourceInterceptorFactory = require('./middleware/SourceInterceptor'), + SafeSeek = require('./util/SafeSeek'); module.exports = function(videojs) { videojs = videojs || window.videojs; @@ -12,8 +13,7 @@ module.exports = function(videojs) { sourceInterceptorFactory(videojs); videojs.hook('setup', function(player) { - // Add handler to switch sources when the user requests a change - player.on(events.QUALITY_REQUESTED, function(event, newSource) { + function changeQuality(event, newSource) { var sources = player.currentSources(), currentTime = player.currentTime(), isPaused = player.paused(), @@ -29,15 +29,36 @@ module.exports = function(videojs) { // following updates the original object in `sources`. selectedSource.selected = true; + if (player._qualitySelectorSafeSeek) { + player._qualitySelectorSafeSeek.onQualitySelectionChange(); + } + player.src(sources); - player.one('loadeddata', function() { - player.currentTime(currentTime); + player.ready(function() { + if (!player._qualitySelectorSafeSeek || player._qualitySelectorSafeSeek.hasFinished()) { + // Either we don't have a pending seek action or the one that we have is no + // longer applicable. This block must be within a `player.ready` callback + // because the call to `player.src` above is asynchronous, and so not + // having it within this `ready` callback would cause the SourceInterceptor + // to execute after this block instead of before. + // + // We save the `currentTime` within the SafeSeek instance because if + // multiple QUALITY_REQUESTED events are received before the SafeSeek + // operation finishes, the player's `currentTime` will be `0` if the + // player's `src` is updated but the player's `currentTime` has not yet + // been set by the SafeSeek operation. + player._qualitySelectorSafeSeek = new SafeSeek(player, currentTime); + } + if (!isPaused) { player.play(); } }); - }); + } + + // Add handler to switch sources when the user requests a change + player.on(events.QUALITY_REQUESTED, changeQuality); }); }; diff --git a/src/js/middleware/SourceInterceptor.js b/src/js/middleware/SourceInterceptor.js index f302f22..099d367 100644 --- a/src/js/middleware/SourceInterceptor.js +++ b/src/js/middleware/SourceInterceptor.js @@ -13,6 +13,10 @@ module.exports = function(videojs) { var sources = player.currentSources(), userSelectedSource, chosenSource; + if (player._qualitySelectorSafeSeek) { + player._qualitySelectorSafeSeek.onPlayerSourcesChange(); + } + // There are generally two source options, the one that videojs // auto-selects and the one that a "user" of this plugin has // supplied via the `selected` property. `selected` can come from diff --git a/src/js/util/SafeSeek.js b/src/js/util/SafeSeek.js new file mode 100644 index 0000000..3feb0b2 --- /dev/null +++ b/src/js/util/SafeSeek.js @@ -0,0 +1,80 @@ +'use strict'; + +var Class = require('class.extend'); + +module.exports = Class.extend({ + init: function(player, seekToTime) { + this._player = player; + this._seekToTime = seekToTime; + this._hasFinished = false; + this._keepThisInstanceWhenPlayerSourcesChange = false; + this._seekWhenSafe(); + }, + + _seekWhenSafe: function() { + var HAVE_FUTURE_DATA = 3; + + // `readyState` in Video.js is the same as the HTML5 Media element's `readyState` + // property. + // + // `readyState` is an enum of 5 values (0-4), each of which represent a state of + // readiness to play. The meaning of the values range from HAVE_NOTHING (0), meaning + // no data is available to HAVE_ENOUGH_DATA (4), meaning all data is loaded and the + // video can be played all the way through. + // + // In order to seek successfully, the `readyState` must be at least HAVE_FUTURE_DATA + // (3). + // + // @see http://docs.videojs.com/player#readyState + // @see https://developer.mozilla.org/en-US/docs/Web/API/HTMLMediaElement/readyState + // @see https://dev.w3.org/html5/spec-preview/media-elements.html#seek-the-media-controller + if (this._player.readyState() < HAVE_FUTURE_DATA) { + this._seekFn = this._seek.bind(this); + // The `canplay` event means that the `readyState` is at least HAVE_FUTURE_DATA. + this._player.one('canplay', this._seekFn); + } else { + this._seek(); + } + }, + + onPlayerSourcesChange: function() { + if (this._keepThisInstanceWhenPlayerSourcesChange) { + // By setting this to `false`, we know that if the player sources change again + // the change did not originate from a quality selection change, the new sources + // are likely different from the old sources, and so this pending seek no longer + // applies. + this._keepThisInstanceWhenPlayerSourcesChange = false; + } else { + this.cancel(); + } + }, + + onQualitySelectionChange: function() { + // `onPlayerSourcesChange` will cancel this pending seek unless we tell it not to. + // We need to reuse this same pending seek instance because when the player is + // paused, the `preload` attribute is set to `none`, and the user selects one + // quality option and then another, the player cannot seek until the player has + // enough data to do so (and the `canplay` event is fired) and thus on the second + // selection the player's `currentTime()` is `0` and when the video plays we would + // seek to `0` instead of the correct time. + if (!this.hasFinished()) { + this._keepThisInstanceWhenPlayerSourcesChange = true; + } + }, + + _seek: function() { + this._player.currentTime(this._seekToTime); + this._keepThisInstanceWhenPlayerSourcesChange = false; + this._hasFinished = true; + }, + + hasFinished: function() { + return this._hasFinished; + }, + + cancel: function() { + this._player.off('canplay', this._seekFn); + this._keepThisInstanceWhenPlayerSourcesChange = false; + this._hasFinished = true; + }, +}); From 73ada268643e6024cb5c9f232e92841d06b671a9 Mon Sep 17 00:00:00 2001 From: Matt Luedke Date: Tue, 9 Jan 2018 15:50:43 -0500 Subject: [PATCH 2/2] Update CHANGELOG for 1.1.2 --- CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 87a9545..d261fa5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,12 @@ In general, this project adheres to [Semantic Versioning](http://semver.org/). If for some reason we do something that's not strictly semantic, it will be clearly called out below. +## 1.1.2 + + * Fixed a bug where selecting a quality menu item while a video was playing did not resume + playback after the source changed. Affected Safari and players whose `preload` attribute + was `none` (8feeafb Fixes #16). + ## 1.1.1 * Reference underscore as a dependency since we depend on it (931d8a4 See #12)