fix: reBufferingGoal is not respected

lib/media/play_rate_controller.js

@@ -30,6 +30,9 @@ shaka.media.PlayRateController = class {
     /** @private {number} */
     this.rate_ = this.harness_.getRate();
 
+    /** @private {boolean} */
+    this.isBuffering_ = false;
+
     /** @private {number} */
     this.pollRate_ = 0.25;
 
@@ -50,6 +53,16 @@ shaka.media.PlayRateController = class {
     this.harness_ = null;
   }
 
+  /**
+   * 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.
    *
@@ -97,7 +110,7 @@ shaka.media.PlayRateController = class {
 
     if (this.rate_ >= 0) {
       try {
-        this.applyRate_(this.rate_);
+        this.applyRate_(this.isBuffering_ ? 0 : this.rate_);
         return;
       } catch (e) {
         // Fall through to the next clause.

lib/media/stall_detector.js

@@ -180,6 +180,12 @@ shaka.media.StallDetector.MediaElementImplementation = class {
     if (this.mediaElement_.buffered.length == 0) {
       return false;
     }
+    // the playback rate is at 0, the player is not stalled but simply pausing
+    // it's buffering due to not enough buffer to respect reBufferingGoal
+    // or minbufferTime
+    if (this.mediaElement_.playbackRate == 0) {
+      return false;
+    }
 
     return shaka.media.StallDetector.MediaElementImplementation.hasContentFor_(
         this.mediaElement_.buffered,

lib/player.js

@@ -5717,6 +5717,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);
       }

test/media/play_rate_controller_unit.js

@@ -66,4 +66,52 @@ describe('PlayRateController', () => {
     controller.set(1);
     expect(setPlayRateSpy).not.toHaveBeenCalled();
   });
+
+  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);
+  });
 });

test/player_integration.js

@@ -948,6 +948,79 @@ describe('Player', () => {
     });
   });
 
+  describe('rebufferGoal', () => {
+    const startBuffering = jasmine.objectContaining({buffering: true});
+    const endBuffering = jasmine.objectContaining({buffering: false});
+    /** @type {!jasmine.Spy} */
+    const onBuffering = jasmine.createSpy('onBuffering');
+    /** @type {!jasmine.Spy} */
+    const onSeeking = jasmine.createSpy('onSeeking');
+    /** @type {!jasmine.Spy} */
+    const onPlaying = jasmine.createSpy('onPlaying');
+    /** @type {!jasmine.Spy} */
+    const onTimeUpdate = jasmine.createSpy('onTimeUpdate');
+    /** @type {!shaka.test.Waiter} */
+    let waiter;
+
+    beforeEach(() => {
+      player.addEventListener('buffering', Util.spyFunc(onBuffering));
+      eventManager.listen(video, 'seeking', Util.spyFunc(onSeeking));
+      eventManager.listen(video, 'timeupdate', Util.spyFunc(onTimeUpdate));
+      eventManager.listen(video, 'playing', Util.spyFunc(onPlaying));
+      video.autoplay = false;
+      waiter = new shaka.test.Waiter(eventManager)
+          .setPlayer(player)
+          .timeoutAfter(20)
+          .failOnTimeout(true);
+    });
+
+    it('state orchestration and buffer length', async () => {
+      // the expected player behaviours should be the following one
+      // buffering event start
+      // playing event but player is not playing
+      // because of the rebufferingGoal > to the buffer )
+      // buffering event stop
+      // timeupdate
+
+      // create a delay in the request to check the event's order
+      const netEngine = player.getNetworkingEngine();
+      netEngine.registerResponseFilter(
+          async (type, response, context) => {
+            await shaka.test.Util.delay(2);
+          });
+
+      player.configure('streaming.rebufferingGoal', 25);
+      player.configure('streaming.bufferingGoal', 60);
+      player.configure('streaming.stallEnabled', true);
+      player.configure('streaming.stallThreshold', 1);
+      player.configure('manifest.dash.ignoreMinBufferTime', true);
+      video.autoplay = true;
+      await player.load('test:sintel_long_compiled');
+
+      expect(onBuffering).toHaveBeenCalledTimes(1);
+      expect(onBuffering).toHaveBeenCalledWith(startBuffering);
+      expect(onSeeking).not.toHaveBeenCalled();
+      expect(onTimeUpdate).not.toHaveBeenCalled();
+      expect(getBufferedAhead()).toBeLessThanOrEqual(25);
+      onBuffering.calls.reset();
+
+      await waiter.waitForEvent(video, 'playing');
+      expect(getBufferedAhead()).toBeLessThanOrEqual(25);
+      expect(video.currentTime).toBe(0);
+
+      await waiter.waitForEvent(player, 'buffering');
+      await shaka.test.Util.delay(1);
+      expect(onSeeking).not.toHaveBeenCalled();
+      expect(onTimeUpdate).toHaveBeenCalled();
+      expect(onBuffering).toHaveBeenCalledTimes(1);
+      expect(onBuffering).toHaveBeenCalledWith(endBuffering);
+      await waiter.waitForEvent(video, 'timeupdate');
+      // segment duration are 10s meaning
+      // that the expected buffer should be 30..
+      expect(getBufferedAhead()).toBeGreaterThanOrEqual(25);
+    });
+  });
+
   describe('buffering', () => {
     const startBuffering = jasmine.objectContaining({buffering: true});
     const endBuffering = jasmine.objectContaining({buffering: false});
@@ -1066,14 +1139,6 @@ describe('Player', () => {
       expect(getBufferedBehind()).toBeLessThanOrEqual(10);
     });
 
-    function getBufferedAhead() {
-      const end = shaka.media.TimeRangesUtils.bufferEnd(video.buffered);
-      if (end == null) {
-        return 0;
-      }
-      return end - video.currentTime;
-    }
-
     function getBufferedBehind() {
       const start = shaka.media.TimeRangesUtils.bufferStart(video.buffered);
       if (start == null) {
@@ -1083,6 +1148,14 @@ describe('Player', () => {
     }
   });  // describe('buffering')
 
+  function getBufferedAhead() {
+    const end = shaka.media.TimeRangesUtils.bufferEnd(video.buffered);
+    if (end == null) {
+      return 0;
+    }
+    return end - video.currentTime;
+  }
+
   describe('configuration', () => {
     it('has the correct number of arguments in compiled callbacks', () => {
       // Get the default configuration for both the compiled & uncompiled

test/player_unit.js

@@ -2710,6 +2710,7 @@ describe('Player', () => {
     });
 
     it('tracks info about current stream', () => {
+      forceBufferingTo(false);
       let stats = player.getStats();
       // Should have chosen the first of each type of stream.
       expect(stats.width).toBe(100);