feat: Stop setting playbackRate to 0 to control buffering state

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,23 +50,11 @@ 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_();
   }
@@ -108,14 +90,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 +114,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

lib/player.js

@@ -520,12 +520,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;
 
@@ -1588,11 +1582,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_) {
@@ -2229,7 +2218,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) {
@@ -2505,7 +2494,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_();
@@ -3028,18 +3017,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);
 
@@ -3049,12 +3035,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_());
   }
 
   /**
@@ -5642,7 +5630,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);
       }

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', () => {