Skip to content

Commit

Permalink
fix: Use timeupdate as well as rvfc/raf for cues (#7918)
Browse files Browse the repository at this point in the history
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).
  • Loading branch information
mister-ben committed Sep 9, 2022
1 parent 2810507 commit 9b81afe
Show file tree
Hide file tree
Showing 2 changed files with 43 additions and 12 deletions.
14 changes: 11 additions & 3 deletions src/js/tracks/text-track.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand All @@ -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);
}

});

Expand Down Expand Up @@ -368,14 +372,18 @@ 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() {
if (this.rvf_) {
this.tech_.cancelVideoFrameCallback(this.rvf_);
this.rvf_ = undefined;
}
this.tech_.off('timeupdate', this.timeupdateHandler);
}

/**
Expand Down
41 changes: 32 additions & 9 deletions test/unit/tracks/text-track.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand All @@ -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();
Expand All @@ -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();
Expand Down Expand Up @@ -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');

Expand All @@ -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');

Expand Down

0 comments on commit 9b81afe

Please sign in to comment.