Skip to content

Commit

Permalink
feat: Stop setting playbackRate to 0 to control buffering state (#5696)
Browse files Browse the repository at this point in the history 
Closes #2093
  • Loading branch information
@avelad
avelad authored Oct 5, 2023
1 parent b93e656 commit 6156dce
Show file tree
Hide file tree
Showing 5 changed files with 25 additions and 121 deletions.
46 changes: 8 additions & 38 deletions lib/media/play_rate_controller.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand All @@ -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
*/
Expand All @@ -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();

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

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

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

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

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

0 comments on commit 6156dce

Please sign in to comment.