diff --git a/lib/media/play_rate_controller.js b/lib/media/play_rate_controller.js index 1e2f591d2d..204974405c 100644 --- a/lib/media/play_rate_controller.js +++ b/lib/media/play_rate_controller.js @@ -6,7 +6,6 @@ goog.provide('shaka.media.PlayRateController'); -goog.require('goog.asserts'); goog.require('shaka.log'); goog.require('shaka.util.IReleasable'); goog.require('shaka.util.Timer'); @@ -17,8 +16,6 @@ goog.require('shaka.util.Timer'); * the playback rate on the media element can change outside of the controller, * the playback controller will need to be updated to stay in-sync. * - * TODO: Try not to manage buffering above the browser with playbackRate=0. - * * @implements {shaka.util.IReleasable} * @final */ @@ -30,9 +27,6 @@ shaka.media.PlayRateController = class { /** @private {?shaka.media.PlayRateController.Harness} */ this.harness_ = harness; - /** @private {boolean} */ - this.isBuffering_ = false; - /** @private {number} */ this.rate_ = this.harness_.getRate(); @@ -56,25 +50,15 @@ shaka.media.PlayRateController = class { } /** - * Sets the buffering flag, which controls the effective playback rate. - * - * @param {boolean} isBuffering If true, forces playback rate to 0 internally. - */ - setBuffering(isBuffering) { - this.isBuffering_ = isBuffering; - this.apply_(); - } - - /** - * Set the playback rate. This rate will only be used as provided when the - * player is not buffering. You should never set the rate to 0. + * Set the playback rate. * * @param {number} rate */ set(rate) { - goog.asserts.assert(rate != 0, 'Should never set rate of 0 explicitly!'); - this.rate_ = rate; - this.apply_(); + if (this.rate_ != rate) { + this.rate_ = rate; + this.apply_(); + } } /** @@ -108,14 +92,11 @@ shaka.media.PlayRateController = class { // Always stop the timer. We may not start it again. this.timer_.stop(); - /** @type {number} */ - const rate = this.calculateCurrentRate_(); + shaka.log.v1('Changing effective playback rate to', this.rate_); - shaka.log.v1('Changing effective playback rate to', rate); - - if (rate >= 0) { + if (this.rate_ >= 0) { try { - this.applyRate_(rate); + this.applyRate_(this.rate_); return; } catch (e) { // Fall through to the next clause. @@ -135,17 +116,6 @@ shaka.media.PlayRateController = class { this.applyRate_(0); } - /** - * Calculate the rate that the controller wants the media element to have - * based on the current state of the controller. - * - * @return {number} - * @private - */ - calculateCurrentRate_() { - return this.isBuffering_ ? 0 : this.rate_; - } - /** * If the new rate is different than the media element's playback rate, this * will change the playback rate. If the rate does not need to change, it will diff --git a/lib/media/stall_detector.js b/lib/media/stall_detector.js index 6fca50825d..1dbdefa411 100644 --- a/lib/media/stall_detector.js +++ b/lib/media/stall_detector.js @@ -174,9 +174,6 @@ shaka.media.StallDetector.MediaElementImplementation = class { if (this.mediaElement_.paused) { return false; } - if (this.mediaElement_.playbackRate == 0) { - return false; - } // If we have don't have enough content, we are not stalled, we are // buffering. diff --git a/lib/player.js b/lib/player.js index 0fc54fcca5..f422b78c50 100644 --- a/lib/player.js +++ b/lib/player.js @@ -540,12 +540,6 @@ shaka.Player = class extends shaka.util.FakeEventTarget { */ this.playRateController_ = null; - // We use the buffering observer and timer to track when we move from having - // enough buffered content to not enough. They only exist when content has - // been loaded and are not re-used between loads. - /** @private {shaka.util.Timer} */ - this.bufferPoller_ = null; - /** @private {shaka.media.BufferingObserver} */ this.bufferObserver_ = null; @@ -1610,11 +1604,6 @@ shaka.Player = class extends shaka.util.FakeEventTarget { this.playheadObservers_ = null; } - if (this.bufferPoller_) { - this.bufferPoller_.stop(); - this.bufferPoller_ = null; - } - // Stop the parser early. Since it is at the start of the pipeline, it // should be start early to avoid is pushing new data downstream. if (this.parser_) { @@ -2253,7 +2242,7 @@ shaka.Player = class extends shaka.util.FakeEventTarget { // being initialized. const rebufferThreshold = Math.max( this.manifest_.minBufferTime, this.config_.streaming.rebufferingGoal); - this.startBufferManagement_(rebufferThreshold); + this.startBufferManagement_(mediaElement, rebufferThreshold); // Now we can switch to the initial variant. if (!activeVariant) { @@ -2527,7 +2516,7 @@ shaka.Player = class extends shaka.util.FakeEventTarget { // set the initial buffering state and that depends on other components // being initialized. const rebufferThreshold = this.config_.streaming.rebufferingGoal; - this.startBufferManagement_(rebufferThreshold); + this.startBufferManagement_(mediaElement, rebufferThreshold); // Add all media element listeners. const updateStateHistory = () => this.updateStateHistory_(); @@ -3050,18 +3039,15 @@ shaka.Player = class extends shaka.util.FakeEventTarget { * Initialize and start the buffering system (observer and timer) so that we * can monitor our buffer lead during playback. * + * @param {!HTMLMediaElement} mediaElement * @param {number} rebufferingGoal * @private */ - startBufferManagement_(rebufferingGoal) { + startBufferManagement_(mediaElement, rebufferingGoal) { goog.asserts.assert( !this.bufferObserver_, 'No buffering observer should exist before initialization.'); - goog.asserts.assert( - !this.bufferPoller_, - 'No buffer timer should exist before initialization.'); - // Give dummy values, will be updated below. this.bufferObserver_ = new shaka.media.BufferingObserver(1, 2); @@ -3071,12 +3057,14 @@ shaka.Player = class extends shaka.util.FakeEventTarget { this.updateBufferingSettings_(rebufferingGoal); this.updateBufferState_(); - // TODO: We should take some time to look into the effects of our - // quarter-second refresh practice. We often use a quarter-second - // but we have no documentation about why. - this.bufferPoller_ = new shaka.util.Timer(() => { - this.pollBufferState_(); - }).tickEvery(/* seconds= */ 0.25); + this.loadEventManager_.listen(mediaElement, 'waiting', + (e) => this.pollBufferState_()); + this.loadEventManager_.listen(mediaElement, 'stalled', + (e) => this.pollBufferState_()); + this.loadEventManager_.listen(mediaElement, 'canplaythrough', + (e) => this.pollBufferState_()); + this.loadEventManager_.listen(mediaElement, 'progress', + (e) => this.pollBufferState_()); } /** @@ -5652,7 +5640,6 @@ shaka.Player = class extends shaka.util.FakeEventTarget { const loaded = this.stats_ && this.bufferObserver_ && this.playhead_; if (loaded) { - this.playRateController_.setBuffering(isBuffering); if (this.cmcdManager_) { this.cmcdManager_.setBuffering(isBuffering); } @@ -5796,7 +5783,7 @@ shaka.Player = class extends shaka.util.FakeEventTarget { 'Updating playbackRate to ' + liveSyncPlaybackRate); this.trickPlay(liveSyncPlaybackRate); } - } else if (playbackRate !== 1 && playbackRate !== 0) { + } else if (playbackRate !== this.playRateController_.getDefaultRate()) { this.cancelTrickPlay(); } } @@ -6901,8 +6888,6 @@ shaka.Player = class extends shaka.util.FakeEventTarget { // Because Safari's native HLS reports slightly inaccurate values for // bufferEnd here, we use a fudge factor. Without this, we can end up in a // buffering state at the end of the stream. See issue #2117. - // TODO: Try to remove the fudge here once we no longer manage buffering - // state above the browser with playbackRate=0. const fudge = 1; // 1000 ms return bufferEnd != null && bufferEnd >= this.video_.duration - fudge; } diff --git a/test/media/play_rate_controller_unit.js b/test/media/play_rate_controller_unit.js index b7c6028565..916166fc6e 100644 --- a/test/media/play_rate_controller_unit.js +++ b/test/media/play_rate_controller_unit.js @@ -57,54 +57,6 @@ describe('PlayRateController', () => { expect(setPlayRateSpy).toHaveBeenCalledWith(0); }); - it('buffering state sets rate to zero', () => { - controller.setBuffering(true); - expect(setPlayRateSpy).toHaveBeenCalledWith(0); - - setPlayRateSpy.calls.reset(); - - controller.setBuffering(false); - expect(setPlayRateSpy).toHaveBeenCalledWith(1); - }); - - it('entering buffering state twice has no effect', () => { - controller.setBuffering(true); - expect(setPlayRateSpy).toHaveBeenCalledWith(0); - - // Reset the calls so that we can make sure it was not called again. - setPlayRateSpy.calls.reset(); - - controller.setBuffering(true); - expect(setPlayRateSpy).not.toHaveBeenCalled(); - }); - - it('leaving buffering state twice has no effect', () => { - controller.setBuffering(true); - controller.setBuffering(false); - - // Reset the calls so that we can make sure it was not called again. - setPlayRateSpy.calls.reset(); - - controller.setBuffering(false); - expect(setPlayRateSpy).not.toHaveBeenCalled(); - }); - - // When we set the rate while in a buffering state, we should see the new - // rate be used once we leave the buffering state. - it('set takes effect after buffering state ends', () => { - controller.setBuffering(true); - expect(setPlayRateSpy).toHaveBeenCalledWith(0); - - // Reset so that we can make sure it was not called after we call |set(4)|. - setPlayRateSpy.calls.reset(); - - controller.set(4); - expect(setPlayRateSpy).not.toHaveBeenCalled(); - - controller.setBuffering(false); - expect(setPlayRateSpy).toHaveBeenCalledWith(4); - }); - // Make sure that when the playback rate set, if the new rate matches the // current rate, the controller will not set the rate on the media element. it('does not redundently set the playrate', () => { diff --git a/test/player_src_equals_integration.js b/test/player_src_equals_integration.js index ebf3e6d909..fd117970d8 100644 --- a/test/player_src_equals_integration.js +++ b/test/player_src_equals_integration.js @@ -165,10 +165,6 @@ describe('Player Src Equals', () => { it('can control trick play rate', async () => { await loadWithSrcEquals(SMALL_MP4_CONTENT_URI, /* startTime= */ null); - // Let playback run for a little. - await video.play(); - await waiter.waitForMovementOrFailOnTimeout(video, /* timeout= */10); - let videoRateChange = false; let playerRateChange = false; eventManager.listen(video, 'ratechange', () => { @@ -178,6 +174,10 @@ describe('Player Src Equals', () => { playerRateChange = true; }); + // Let playback run for a little. + await video.play(); + await waiter.waitForMovementOrFailOnTimeout(video, /* timeout= */10); + // Enabling trick play should change our playback rate to the same rate. player.trickPlay(2); expect(video.playbackRate).toBe(2);