Skip to content

Commit

Permalink
fix: Re-add setting playbackRate to 0 to control buffering state (#6546)
Browse files Browse the repository at this point in the history
Fixes #6527
Fixes #6355
This reverts commit
6156dce.
  • Loading branch information
avelad committed May 8, 2024
1 parent 6b62993 commit 91d3792
Show file tree
Hide file tree
Showing 5 changed files with 107 additions and 9 deletions.
44 changes: 36 additions & 8 deletions lib/media/play_rate_controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand All @@ -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();

Expand All @@ -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_();
}

/**
Expand Down Expand Up @@ -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.
Expand All @@ -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
Expand Down
3 changes: 3 additions & 0 deletions lib/media/stall_detector.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
19 changes: 19 additions & 0 deletions lib/player.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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_) {
Expand Down Expand Up @@ -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);

Expand All @@ -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',
Expand Down Expand Up @@ -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);
}
Expand Down
48 changes: 48 additions & 0 deletions test/media/play_rate_controller_unit.js
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down
2 changes: 1 addition & 1 deletion test/player_src_equals_integration.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down

0 comments on commit 91d3792

Please sign in to comment.