From a1d7696c87626a43abc4928704faaf942b0e7b93 Mon Sep 17 00:00:00 2001 From: mister-ben Date: Fri, 9 Sep 2022 19:50:21 +0200 Subject: [PATCH] fix: Use timeupdate as well as rvfc/raf for cues (#7918) Use the timeupdate event as well as the rvfc and raf callbacks to check cues. This is a bit overkill for "usual" playback but avoids edge cases. If the more preceise callback trigger first the cue will update but the timeupdate event should catch any that were missed, notwithstanding that timeupdate was always somewhat unpredictable. Fixes #7910 (audio in video els and Samsung being weird) and fixes #7902 (no updates off screen). --- src/js/tracks/text-track.js | 14 +++++++--- test/unit/tracks/text-track.test.js | 41 ++++++++++++++++++++++------- 2 files changed, 43 insertions(+), 12 deletions(-) diff --git a/src/js/tracks/text-track.js b/src/js/tracks/text-track.js index 7cb02c8995..36a9406453 100644 --- a/src/js/tracks/text-track.js +++ b/src/js/tracks/text-track.js @@ -184,13 +184,15 @@ class TextTrack extends Track { const activeCues = new TextTrackCueList(this.activeCues_); let changed = false; - this.timeupdateHandler = Fn.bind(this, function() { + this.timeupdateHandler = Fn.bind(this, function(event = {}) { if (this.tech_.isDisposed()) { return; } if (!this.tech_.isReady_) { - this.rvf_ = this.tech_.requestVideoFrameCallback(this.timeupdateHandler); + if (event.type !== 'timeupdate') { + this.rvf_ = this.tech_.requestVideoFrameCallback(this.timeupdateHandler); + } return; } @@ -205,7 +207,9 @@ class TextTrack extends Track { changed = false; } - this.rvf_ = this.tech_.requestVideoFrameCallback(this.timeupdateHandler); + if (event.type !== 'timeupdate') { + this.rvf_ = this.tech_.requestVideoFrameCallback(this.timeupdateHandler); + } }); @@ -368,7 +372,10 @@ class TextTrack extends Track { } startTracking() { + // More precise cues based on requestVideoFrameCallback with a requestAnimationFram fallback this.rvf_ = this.tech_.requestVideoFrameCallback(this.timeupdateHandler); + // Also listen to timeupdate in case rVFC/rAF stops (window in background, audio in video el) + this.tech_.on('timeupdate', this.timeupdateHandler); } stopTracking() { @@ -376,6 +383,7 @@ class TextTrack extends Track { this.tech_.cancelVideoFrameCallback(this.rvf_); this.rvf_ = undefined; } + this.tech_.off('timeupdate', this.timeupdateHandler); } /** diff --git a/test/unit/tracks/text-track.test.js b/test/unit/tracks/text-track.test.js index 3b8ae94733..474793abbd 100644 --- a/test/unit/tracks/text-track.test.js +++ b/test/unit/tracks/text-track.test.js @@ -279,7 +279,9 @@ QUnit.test('does not fire cuechange before Tech is ready', function(assert) { return 0; }; + // `playing` would trigger rvfc or raf, `timeupdate` for fallback player.tech_.trigger('playing'); + player.tech_.trigger('timeupdate'); assert.equal(changes, 0, 'a cuechange event is not triggered'); player.tech_.on('ready', function() { @@ -292,6 +294,11 @@ QUnit.test('does not fire cuechange before Tech is ready', function(assert) { assert.equal(changes, 2, 'a cuechange event trigger addEventListener and oncuechange'); + player.tech_.trigger('timeupdate'); + clock.tick(1); + + assert.equal(changes, 2, 'a cuechange event trigger not duplicated by timeupdate'); + tt.off(); player.dispose(); clock.restore(); @@ -311,31 +318,45 @@ QUnit.test('fires cuechange when cues become active and inactive', function(asse const cuechangeHandler = function() { changes++; }; + let fakeCurrentTime = 0; + + player.tech_.currentTime = function() { + return fakeCurrentTime; + }; tt.addCue({ id: '1', startTime: 1, endTime: 5 }); + tt.addCue({ + id: '2', + startTime: 11, + endTime: 14 + }); tt.oncuechange = cuechangeHandler; tt.addEventListener('cuechange', cuechangeHandler); - player.tech_.currentTime = function() { - return 2; - }; + fakeCurrentTime = 2; + player.tech_.trigger('playing'); + assert.equal(changes, 2, 'a cuechange event trigger addEventListener and oncuechange (rvfc/raf)'); + + fakeCurrentTime = 7; player.tech_.trigger('playing'); - assert.equal(changes, 2, 'a cuechange event trigger addEventListener and oncuechange'); + assert.equal(changes, 4, 'a cuechange event trigger addEventListener and oncuechange (rvfc/raf)'); - player.tech_.currentTime = function() { - return 7; - }; + fakeCurrentTime = 12; + player.tech_.trigger('timeupdate'); - player.tech_.trigger('playing'); + assert.equal(changes, 6, 'a cuechange event trigger addEventListener and oncuechange (timeupdate)'); + + fakeCurrentTime = 17; + player.tech_.trigger('timeupdate'); - assert.equal(changes, 4, 'a cuechange event trigger addEventListener and oncuechange'); + assert.equal(changes, 8, 'a cuechange event trigger addEventListener and oncuechange (timeupdate)'); tt.off(); player.dispose(); @@ -365,6 +386,7 @@ QUnit.test('enabled and disabled cuechange handler when changing mode to hidden' return 2; }; player.tech_.trigger('playing'); + player.tech_.trigger('timeupdate'); assert.equal(changes, 1, 'a cuechange event trigger'); @@ -376,6 +398,7 @@ QUnit.test('enabled and disabled cuechange handler when changing mode to hidden' return 7; }; player.tech_.trigger('playing'); + player.tech_.trigger('timeupdate'); assert.equal(changes, 0, 'NO cuechange event trigger');