From 6840569f448b673e4efbb81aa0514263bdf8600b Mon Sep 17 00:00:00 2001 From: brandonocasey Date: Mon, 7 Dec 2020 15:15:48 -0500 Subject: [PATCH 1/4] fix: prevent dispose error and text track duplicate listeners --- src/js/tracks/text-track.js | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/src/js/tracks/text-track.js b/src/js/tracks/text-track.js index f7093538a4..782869d98f 100644 --- a/src/js/tracks/text-track.js +++ b/src/js/tracks/text-track.js @@ -184,7 +184,10 @@ class TextTrack extends Track { const activeCues = new TextTrackCueList(this.activeCues_); let changed = false; const timeupdateHandler = Fn.bind(this, function() { + if (!this.tech_.isReady_ || this.tech_.isDisposed()) { + return; + } // Accessing this.activeCues for the side-effects of updating itself // due to its nature as a getter function. Do not remove or cues will // stop updating! @@ -196,10 +199,14 @@ class TextTrack extends Track { } }); + const disposeHandler = () => { + this.tech_.off('timeupdate', timeupdateHandler); + this.tech_.off('dispose', disposeHandler); + }; + + this.tech_.on('dispose', disposeHandler); if (mode !== 'disabled') { - this.tech_.ready(() => { - this.tech_.on('timeupdate', timeupdateHandler); - }, true); + this.tech_.on('timeupdate', timeupdateHandler); } Object.defineProperties(this, { @@ -241,12 +248,10 @@ class TextTrack extends Track { // On-demand load. loadTrack(this.src, this); } + this.tech_.off('timeupdate', timeupdateHandler); + if (mode !== 'disabled') { - this.tech_.ready(() => { - this.tech_.on('timeupdate', timeupdateHandler); - }, true); - } else { - this.tech_.off('timeupdate', timeupdateHandler); + this.tech_.on('timeupdate', timeupdateHandler); } /** * An event that fires when mode changes on this track. This allows From 3d958438902190a1161378d99dc269f1b6724e97 Mon Sep 17 00:00:00 2001 From: brandonocasey Date: Mon, 7 Dec 2020 15:58:31 -0500 Subject: [PATCH 2/4] add on to fake tech --- test/unit/tech/tech.test.js | 6 +++-- test/unit/tracks/html-track-element.test.js | 2 +- .../tracks/text-track-list-converter.test.js | 25 +++++++++++++------ 3 files changed, 23 insertions(+), 10 deletions(-) diff --git a/test/unit/tech/tech.test.js b/test/unit/tech/tech.test.js index 020fabab51..4dc4e51b04 100644 --- a/test/unit/tech/tech.test.js +++ b/test/unit/tech/tech.test.js @@ -142,8 +142,9 @@ QUnit.test('dispose() should stop time tracking', function(assert) { QUnit.test('dispose() should clear all tracks that are passed when its created', function(assert) { const audioTracks = new AudioTrackList([new AudioTrack(), new AudioTrack()]); const videoTracks = new VideoTrackList([new VideoTrack(), new VideoTrack()]); - const textTracks = new TextTrackList([new TextTrack({tech: {}}), - new TextTrack({tech: {}})]); + const pretech = new Tech(); + const textTracks = new TextTrackList([new TextTrack({tech: pretech}), + new TextTrack({tech: pretech})]); assert.equal(audioTracks.length, 2, 'should have two audio tracks at the start'); assert.equal(videoTracks.length, 2, 'should have two video tracks at the start'); @@ -167,6 +168,7 @@ QUnit.test('dispose() should clear all tracks that are passed when its created', 'should hold text tracks that we passed' ); + pretech.dispose(); tech.dispose(); assert.equal(audioTracks.length, 0, 'should have zero audio tracks after dispose'); diff --git a/test/unit/tracks/html-track-element.test.js b/test/unit/tracks/html-track-element.test.js index 472b8b4381..b46b0721d7 100644 --- a/test/unit/tracks/html-track-element.test.js +++ b/test/unit/tracks/html-track-element.test.js @@ -65,7 +65,7 @@ QUnit.test('fires loadeddata when track cues become populated', function(assert) changes++; }; const htmlTrackElement = new HTMLTrackElement({ - tech() {} + tech: this.tech }); htmlTrackElement.addEventListener('load', loadHandler); diff --git a/test/unit/tracks/text-track-list-converter.test.js b/test/unit/tracks/text-track-list-converter.test.js index beb9b76055..486eb18127 100644 --- a/test/unit/tracks/text-track-list-converter.test.js +++ b/test/unit/tracks/text-track-list-converter.test.js @@ -48,7 +48,8 @@ if (Html5.supportsNativeTextTracks()) { tech: { crossOrigin() { return null; - } + }, + on() {} } }); @@ -75,6 +76,7 @@ if (Html5.supportsNativeTextTracks()) { } }; }, + on() {}, crossOrigin() { return null; }, @@ -108,7 +110,8 @@ if (Html5.supportsNativeTextTracks()) { tech: { crossOrigin() { return null; - } + }, + on() {} } }); @@ -140,6 +143,7 @@ if (Html5.supportsNativeTextTracks()) { crossOrigin() { return null; }, + on() {}, textTracks() { return tt; }, @@ -169,7 +173,8 @@ QUnit.test('trackToJson_ produces correct representation for emulated track obje tech: { crossOrigin() { return null; - } + }, + on() {} } }); @@ -191,7 +196,8 @@ QUnit.test('textTracksToJson produces good json output for emulated only', funct tech: { crossOrigin() { return null; - } + }, + on() {} } }); @@ -203,7 +209,8 @@ QUnit.test('textTracksToJson produces good json output for emulated only', funct tech: { crossOrigin() { return null; - } + }, + on() {} } }); @@ -227,6 +234,7 @@ QUnit.test('textTracksToJson produces good json output for emulated only', funct crossOrigin() { return null; }, + on() {}, textTracks() { return tt; } @@ -259,7 +267,8 @@ QUnit.test('jsonToTextTracks calls addRemoteTextTrack on the tech with emulated tech: { crossOrigin() { return null; - } + }, + on() {} } }); @@ -271,7 +280,8 @@ QUnit.test('jsonToTextTracks calls addRemoteTextTrack on the tech with emulated tech: { crossOrigin() { return null; - } + }, + on() {} } }); @@ -296,6 +306,7 @@ QUnit.test('jsonToTextTracks calls addRemoteTextTrack on the tech with emulated crossOrigin() { return null; }, + on() {}, textTracks() { return tt; }, From e6dda7c065305fd12679852bda9fbac06695b8be Mon Sep 17 00:00:00 2001 From: brandonocasey Date: Thu, 21 Jan 2021 16:05:31 -0500 Subject: [PATCH 3/4] cr --- src/js/tracks/text-track.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/js/tracks/text-track.js b/src/js/tracks/text-track.js index 782869d98f..094fa4afcb 100644 --- a/src/js/tracks/text-track.js +++ b/src/js/tracks/text-track.js @@ -201,10 +201,9 @@ class TextTrack extends Track { const disposeHandler = () => { this.tech_.off('timeupdate', timeupdateHandler); - this.tech_.off('dispose', disposeHandler); }; - this.tech_.on('dispose', disposeHandler); + this.tech_.one('dispose', disposeHandler); if (mode !== 'disabled') { this.tech_.on('timeupdate', timeupdateHandler); } @@ -243,6 +242,10 @@ class TextTrack extends Track { if (!TextTrackMode[newMode]) { return; } + if (mode === newMode) { + return; + } + mode = newMode; if (!this.preload_ && mode !== 'disabled' && this.cues.length === 0) { // On-demand load. From c9209ca3c5db3e4c939f47ae484706c0c9733899 Mon Sep 17 00:00:00 2001 From: brandonocasey Date: Thu, 21 Jan 2021 16:27:24 -0500 Subject: [PATCH 4/4] add tech.one --- .../tracks/html-track-element-list.test.js | 1 + .../tracks/text-track-list-converter.test.js | 23 +++++++++++++------ test/unit/tracks/text-tracks.test.js | 2 +- 3 files changed, 18 insertions(+), 8 deletions(-) diff --git a/test/unit/tracks/html-track-element-list.test.js b/test/unit/tracks/html-track-element-list.test.js index 0278797dea..da25d36cde 100644 --- a/test/unit/tracks/html-track-element-list.test.js +++ b/test/unit/tracks/html-track-element-list.test.js @@ -6,6 +6,7 @@ const defaultTech = { textTracks() {}, on() {}, off() {}, + one() {}, currentTime() {} }; diff --git a/test/unit/tracks/text-track-list-converter.test.js b/test/unit/tracks/text-track-list-converter.test.js index 486eb18127..11fa3dc25b 100644 --- a/test/unit/tracks/text-track-list-converter.test.js +++ b/test/unit/tracks/text-track-list-converter.test.js @@ -49,7 +49,8 @@ if (Html5.supportsNativeTextTracks()) { crossOrigin() { return null; }, - on() {} + on() {}, + one() {} } }); @@ -111,7 +112,8 @@ if (Html5.supportsNativeTextTracks()) { crossOrigin() { return null; }, - on() {} + on() {}, + one() {} } }); @@ -174,7 +176,8 @@ QUnit.test('trackToJson_ produces correct representation for emulated track obje crossOrigin() { return null; }, - on() {} + on() {}, + one() {} } }); @@ -197,7 +200,8 @@ QUnit.test('textTracksToJson produces good json output for emulated only', funct crossOrigin() { return null; }, - on() {} + on() {}, + one() {} } }); @@ -210,7 +214,8 @@ QUnit.test('textTracksToJson produces good json output for emulated only', funct crossOrigin() { return null; }, - on() {} + on() {}, + one() {} } }); @@ -235,6 +240,7 @@ QUnit.test('textTracksToJson produces good json output for emulated only', funct return null; }, on() {}, + one() {}, textTracks() { return tt; } @@ -268,7 +274,8 @@ QUnit.test('jsonToTextTracks calls addRemoteTextTrack on the tech with emulated crossOrigin() { return null; }, - on() {} + on() {}, + one() {} } }); @@ -281,7 +288,8 @@ QUnit.test('jsonToTextTracks calls addRemoteTextTrack on the tech with emulated crossOrigin() { return null; }, - on() {} + on() {}, + one() {} } }); @@ -307,6 +315,7 @@ QUnit.test('jsonToTextTracks calls addRemoteTextTrack on the tech with emulated return null; }, on() {}, + one() {}, textTracks() { return tt; }, diff --git a/test/unit/tracks/text-tracks.test.js b/test/unit/tracks/text-tracks.test.js index cbfbcdc8c8..042e106abc 100644 --- a/test/unit/tracks/text-tracks.test.js +++ b/test/unit/tracks/text-tracks.test.js @@ -343,7 +343,7 @@ QUnit.test('removes cuechange event when text track is hidden for emulated track numTextTrackChanges++; }); - tt.mode = 'showing'; + tt.mode = 'disabled'; this.clock.tick(1); assert.equal( numTextTrackChanges, 1,