From 91d3792e9643150cb15aea6c63e3d9f067642c68 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81lvaro=20Velad=20Galv=C3=A1n?= Date: Wed, 8 May 2024 11:16:40 +0200 Subject: [PATCH] fix: Re-add setting playbackRate to 0 to control buffering state (#6546) Fixes https://github.com/shaka-project/shaka-player/issues/6527 Fixes https://github.com/shaka-project/shaka-player/issues/6355 This reverts commit https://github.com/shaka-project/shaka-player/commit/6156dced6bddc5e2cd0cc52071295cff63cadfcd. --- lib/media/play_rate_controller.js | 44 ++++++++++++++++++----- lib/media/stall_detector.js | 3 ++ lib/player.js | 19 ++++++++++ test/media/play_rate_controller_unit.js | 48 +++++++++++++++++++++++++ test/player_src_equals_integration.js | 2 +- 5 files changed, 107 insertions(+), 9 deletions(-) diff --git a/lib/media/play_rate_controller.js b/lib/media/play_rate_controller.js index f0917d117c..326b1ca4dd 100644 --- a/lib/media/play_rate_controller.js +++ b/lib/media/play_rate_controller.js @@ -6,6 +6,7 @@ goog.provide('shaka.media.PlayRateController'); +goog.require('goog.asserts'); goog.require('shaka.log'); goog.require('shaka.util.IReleasable'); goog.require('shaka.util.Timer'); @@ -27,6 +28,9 @@ shaka.media.PlayRateController = class { /** @private {?shaka.media.PlayRateController.Harness} */ this.harness_ = harness; + /** @private {boolean} */ + this.isBuffering_ = false; + /** @private {number} */ this.rate_ = this.harness_.getRate(); @@ -51,15 +55,25 @@ shaka.media.PlayRateController = class { } /** - * Set the playback rate. + * 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. * * @param {number} rate */ set(rate) { - if (this.rate_ != rate) { - this.rate_ = rate; - this.apply_(); - } + goog.asserts.assert(rate != 0, 'Should never set rate of 0 explicitly!'); + this.rate_ = rate; + this.apply_(); } /** @@ -93,11 +107,14 @@ shaka.media.PlayRateController = class { // Always stop the timer. We may not start it again. this.timer_.stop(); - shaka.log.v1('Changing effective playback rate to', this.rate_); + /** @type {number} */ + const rate = this.calculateCurrentRate_(); + + shaka.log.v1('Changing effective playback rate to', rate); - if (this.rate_ >= 0) { + if (rate >= 0) { try { - this.applyRate_(this.rate_); + this.applyRate_(rate); return; } catch (e) { // Fall through to the next clause. @@ -117,6 +134,17 @@ 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 1dbdefa411..6fca50825d 100644 --- a/lib/media/stall_detector.js +++ b/lib/media/stall_detector.js @@ -174,6 +174,9 @@ 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 86521af3a8..28412180ee 100644 --- a/lib/player.js +++ b/lib/player.js @@ -622,6 +622,12 @@ 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; @@ -1228,6 +1234,11 @@ 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_) { @@ -3101,6 +3112,10 @@ shaka.Player = class extends shaka.util.FakeEventTarget { !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); @@ -3110,6 +3125,9 @@ shaka.Player = class extends shaka.util.FakeEventTarget { this.updateBufferingSettings_(rebufferingGoal); this.updateBufferState_(); + 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', @@ -5833,6 +5851,7 @@ 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); } diff --git a/test/media/play_rate_controller_unit.js b/test/media/play_rate_controller_unit.js index 916166fc6e..b7c6028565 100644 --- a/test/media/play_rate_controller_unit.js +++ b/test/media/play_rate_controller_unit.js @@ -57,6 +57,54 @@ 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 124562a8a6..623307605b 100644 --- a/test/player_src_equals_integration.js +++ b/test/player_src_equals_integration.js @@ -177,7 +177,7 @@ describe('Player Src Equals', () => { // Let playback run for a little. await video.play(); - await waiter.waitForMovementOrFailOnTimeout(video, /* timeout= */10); + await waiter.waitForMovementOrFailOnTimeout(video, /* timeout= */ 10); // Enabling trick play should change our playback rate to the same rate. player.trickPlay(2);