Skip to content

Commit

Permalink
fix(player): cache_.currentTime is not updated when the current time …
Browse files Browse the repository at this point in the history
…is set (#8285)

Updating cache_.currentTime as soon as the currentTime is set avoids having to wait for the timeupdate event, which results in:

- making cache_.currentTime more reliable
- updating the progress bar on mouse up after dragging when the media is paused.

See also: #6232, #6234, #6370, #6372
  • Loading branch information
amtins authored Jun 1, 2023
1 parent 7e85324 commit b782cb0
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 20 deletions.
45 changes: 25 additions & 20 deletions src/js/player.js
Original file line number Diff line number Diff line change
Expand Up @@ -2462,29 +2462,34 @@ class Player extends Component {
* - Nothing when setting
*/
currentTime(seconds) {
if (typeof seconds !== 'undefined') {
if (seconds < 0) {
seconds = 0;
}
if (!this.isReady_ || this.changingSrc_ || !this.tech_ || !this.tech_.isReady_) {
this.cache_.initTime = seconds;
this.off('canplay', this.boundApplyInitTime_);
this.one('canplay', this.boundApplyInitTime_);
return;
}
this.techCall_('setCurrentTime', seconds);
this.cache_.initTime = 0;
if (seconds === undefined) {
// cache last currentTime and return. default to 0 seconds
//
// Caching the currentTime is meant to prevent a massive amount of reads on the tech's
// currentTime when scrubbing, but may not provide much performance benefit after all.
// Should be tested. Also something has to read the actual current time or the cache will
// never get updated.
this.cache_.currentTime = (this.techGet_('currentTime') || 0);
return this.cache_.currentTime;
}

if (seconds < 0) {
seconds = 0;
}

if (!this.isReady_ || this.changingSrc_ || !this.tech_ || !this.tech_.isReady_) {
this.cache_.initTime = seconds;
this.off('canplay', this.boundApplyInitTime_);
this.one('canplay', this.boundApplyInitTime_);
return;
}

// cache last currentTime and return. default to 0 seconds
//
// Caching the currentTime is meant to prevent a massive amount of reads on the tech's
// currentTime when scrubbing, but may not provide much performance benefit after all.
// Should be tested. Also something has to read the actual current time or the cache will
// never get updated.
this.cache_.currentTime = (this.techGet_('currentTime') || 0);
return this.cache_.currentTime;
this.techCall_('setCurrentTime', seconds);
this.cache_.initTime = 0;

if (isFinite(seconds)) {
this.cache_.currentTime = Number(seconds);
}
}

/**
Expand Down
14 changes: 14 additions & 0 deletions test/unit/player.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -2699,6 +2699,20 @@ QUnit.test('Should accept multiple calls to currentTime after player initializat
assert.equal(player.currentTime(), 800, 'The last value passed is stored as the currentTime value');
});

QUnit.test('Should be able to set the cache currentTime after player initialization as soon the canplay event is fired', function(assert) {
const player = TestHelpers.makePlayer({});

player.src('xyz.mp4');
player.currentTime(500);

assert.strictEqual(player.getCache().currentTime, 0, 'cache currentTime value was not changed');

this.clock.tick(100);
player.trigger('canplay');

assert.strictEqual(player.getCache().currentTime, 500, 'cache currentTime value is the one passed after initialization');
});

QUnit.test('Should fire debugon event when debug mode is enabled', function(assert) {
const player = TestHelpers.makePlayer({});
const debugOnSpy = sinon.spy();
Expand Down

0 comments on commit b782cb0

Please sign in to comment.