From b782cb09f4e526d44b9f96b674d60091749613c7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9?= <34163393+amtins@users.noreply.github.com> Date: Thu, 1 Jun 2023 14:50:29 +0200 Subject: [PATCH] fix(player): cache_.currentTime is not updated when the current time 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 --- src/js/player.js | 45 ++++++++++++++++++++++------------------ test/unit/player.test.js | 14 +++++++++++++ 2 files changed, 39 insertions(+), 20 deletions(-) diff --git a/src/js/player.js b/src/js/player.js index 96ed0bed8b..e8b9ae3248 100644 --- a/src/js/player.js +++ b/src/js/player.js @@ -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); + } } /** diff --git a/test/unit/player.test.js b/test/unit/player.test.js index e9529a3148..d8f9534263 100644 --- a/test/unit/player.test.js +++ b/test/unit/player.test.js @@ -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();