From dec3d9c50505a6a7707232f595acbe1cfea8c3a0 Mon Sep 17 00:00:00 2001 From: Joey Parrish Date: Mon, 16 May 2022 16:10:44 -0700 Subject: [PATCH] fix: Wait for chapters track to be loaded (#4228) addChaptersTrack() was already an async method, but it did not wait for the chapter data to be loaded by the browser. The solution is to wait for the `load` event on the `` element we create. To accomplish this, some cleanup and refactoring was done in how tracks are managed. Summary of changes: - The `addtrack` event, which triggered management of tracks in src= playback, is now used for all types of playback. In src= mode, it manages all tracks, and in MSE mode, it only manages chapters tracks (which are added to the video element as in src= mode). - `processChaptersTrack_()` has been renamed to `activateChaptersTrack_()`, since its only job is to set the track's mode field to make the browser load it. - `activateChaptersTrack_()` is now only ever called via the `addtrack` event. - `activateChaptersTrack_()` no longer loops over all chapter tracks on a timer, and instead only touches the single track it was called for. - `addSrcTrackElement_()` now returns the HTML `` element it creates. - `addChaptersTrack()` now awaits a `load` or `error` event to complete (or fail) the operation. - Existing tests for addChaptersTrack had long delays to work around this issue; these delays have simply been removed. Fixes #4186 --- lib/player.js | 78 ++++++++++-- lib/util/error.js | 7 ++ test/player_integration.js | 16 +-- test/player_src_equals_integration.js | 164 +++++++++++++------------- test/test/util/simple_fakes.js | 22 ++++ 5 files changed, 181 insertions(+), 106 deletions(-) diff --git a/lib/player.js b/lib/player.js index e668782ca3..81c97c5e59 100644 --- a/lib/player.js +++ b/lib/player.js @@ -1953,6 +1953,35 @@ shaka.Player = class extends shaka.util.FakeEventTarget { // "streaming" so that they can access internal information. this.loadMode_ = shaka.Player.LoadMode.MEDIA_SOURCE; + // This flag is used below in track management to check if this load was + // canceled before the necessary events fired. + let unloaded = false; + this.cleanupOnUnload_.push(() => { + unloaded = true; + }); + + if (this.video_.textTracks) { + this.eventManager_.listen(this.video_.textTracks, 'addtrack', (e) => { + // If we have moved on to another piece of content while waiting for + // the above event, we should not process tracks here. + if (unloaded) { + return; + } + + const trackEvent = /** @type {!TrackEvent} */(e); + if (trackEvent.track) { + const track = trackEvent.track; + goog.asserts.assert(track instanceof TextTrack, 'Wrong track type!'); + + switch (track.kind) { + case 'chapters': + this.activateChaptersTrack_(track); + break; + } + } + }); + } + // The event must be fired after we filter by restrictions but before the // active stream is picked to allow those listening for the "streaming" // event to make changes before streaming starts. @@ -2251,6 +2280,7 @@ shaka.Player = class extends shaka.util.FakeEventTarget { this.eventManager_.listen( this.video_.audioTracks, 'change', () => this.onTracksChanged_()); } + if (this.video_.textTracks) { this.eventManager_.listen(this.video_.textTracks, 'addtrack', (e) => { // If we have moved on to another piece of content while waiting for @@ -2263,19 +2293,23 @@ shaka.Player = class extends shaka.util.FakeEventTarget { if (trackEvent.track) { const track = trackEvent.track; goog.asserts.assert(track instanceof TextTrack, 'Wrong track type!'); + switch (track.kind) { case 'metadata': this.processTimedMetadataSrcEqls_(track); break; + case 'chapters': - this.processChaptersTrack_(track); + this.activateChaptersTrack_(track); break; + default: this.onTracksChanged_(); break; } } }); + this.eventManager_.listen( this.video_.textTracks, 'removetrack', () => this.onTracksChanged_()); this.eventManager_.listen( @@ -2532,12 +2566,12 @@ shaka.Player = class extends shaka.util.FakeEventTarget { } /** - * We're looking for chapters tracks to process the chapters. + * Set the mode on a chapters track so that it loads. * * @param {?TextTrack} track * @private */ - processChaptersTrack_(track) { + activateChaptersTrack_(track) { if (!track || track.kind != 'chapters') { return; } @@ -2550,10 +2584,7 @@ shaka.Player = class extends shaka.util.FakeEventTarget { // this process to be repeated several times to ensure that it has been put // in the correct mode. const timer = new shaka.util.Timer(() => { - const chaptersTracks = this.getChaptersTracks_(); - for (const chaptersTrack of chaptersTracks) { - chaptersTrack.mode = 'hidden'; - } + track.mode = 'hidden'; }).tickNow().tickAfter(0.5); this.cleanupOnUnload_.push(() => { @@ -4537,25 +4568,41 @@ shaka.Player = class extends shaka.util.FakeEventTarget { if (!mimeType) { mimeType = await this.getTextMimetype_(uri); } + let adCuePoints = []; if (this.adManager_) { try { adCuePoints = this.adManager_.getServerSideCuePoints(); } catch (error) {} } - await this.addSrcTrackElement_(uri, language, /* kind= */ 'chapters', - mimeType, /* label= */ '', adCuePoints); + + /** @type {!HTMLTrackElement} */ + const trackElement = await this.addSrcTrackElement_( + uri, language, /* kind= */ 'chapters', mimeType, /* label= */ '', + adCuePoints); + const chaptersTracks = this.getChaptersTracks(); const chaptersTrack = chaptersTracks.find((t) => { return t.language == language; }); + if (chaptersTrack) { - const html5ChaptersTracks = this.getChaptersTracks_(); - for (const html5ChaptersTrack of html5ChaptersTracks) { - this.processChaptersTrack_(html5ChaptersTrack); - } + await new Promise((resolve, reject) => { + // The chapter data isn't available until the 'load' event fires, and + // that won't happen until the chapters track is activated by the + // process method. + this.eventManager_.listenOnce(trackElement, 'load', resolve); + this.eventManager_.listenOnce(trackElement, 'error', (event) => { + reject(new shaka.util.Error( + shaka.util.Error.Severity.RECOVERABLE, + shaka.util.Error.Category.TEXT, + shaka.util.Error.Code.CHAPTERS_TRACK_FAILED)); + }); + }); + return chaptersTrack; } + // This should not happen, but there are browser implementations that may // not support the Track element. shaka.log.error('Cannot add this text when loaded with src='); @@ -4609,6 +4656,7 @@ shaka.Player = class extends shaka.util.FakeEventTarget { * @param {string} mimeType * @param {string} label * @param {!Array.} adCuePoints + * @return {!Promise.} * @private */ async addSrcTrackElement_(uri, language, kind, mimeType, label, @@ -4624,12 +4672,14 @@ shaka.Player = class extends shaka.util.FakeEventTarget { uri = shaka.media.MediaSourceEngine.createObjectURL(blob); mimeType = 'text/vtt'; } + const trackElement = /** @type {!HTMLTrackElement} */(document.createElement('track')); trackElement.src = this.cmcdManager_.appendTextTrackData(uri); trackElement.label = label; trackElement.kind = kind; trackElement.srclang = language; + // Because we're pulling in the text track file via Javascript, the // same-origin policy applies. If you'd like to have a player served // from one domain, but the text track served from another, you'll @@ -4639,7 +4689,9 @@ shaka.Player = class extends shaka.util.FakeEventTarget { if (!this.video_.getAttribute('crossorigin')) { this.video_.setAttribute('crossorigin', 'anonymous'); } + this.video_.appendChild(trackElement); + return trackElement; } /** diff --git a/lib/util/error.js b/lib/util/error.js index 8185ced008..518c027723 100644 --- a/lib/util/error.js +++ b/lib/util/error.js @@ -345,6 +345,13 @@ shaka.util.Error.Code = { */ 'MISSING_TEXT_PLUGIN': 2014, + /** + * The chapters track failed to load. The browser does not provide any + * information in this case to identify why it failed, but there may be + * details in the JavaScript console. + */ + 'CHAPTERS_TRACK_FAILED': 2015, + /** * Some component tried to read past the end of a buffer. The segment index, * init segment, or PSSH may be malformed. diff --git a/test/player_integration.js b/test/player_integration.js index 22f513b478..01eeb8b06e 100644 --- a/test/player_integration.js +++ b/test/player_integration.js @@ -1069,16 +1069,16 @@ describe('Player', () => { }); }); // describe('unloading') - describe('chapters', () => { - it('add external chapters in vtt format', async () => { + describe('addChaptersTrack', () => { + it('adds external chapters in vtt format', async () => { await player.load('test:sintel_no_text_compiled'); const locationUri = new goog.Uri(location.href); const partialUri1 = new goog.Uri('/base/test/test/assets/chapters.vtt'); const absoluteUri1 = locationUri.resolve(partialUri1); await player.addChaptersTrack(absoluteUri1.toString(), 'en'); - await shaka.test.Util.delay(1.5); - + // Data should be available as soon as addChaptersTrack resolves. + // See https://github.com/shaka-project/shaka-player/issues/4186 const chapters = player.getChapters('en'); expect(chapters.length).toBe(3); const chapter1 = chapters[0]; @@ -1098,8 +1098,6 @@ describe('Player', () => { const absoluteUri2 = locationUri.resolve(partialUri2); await player.addChaptersTrack(absoluteUri2.toString(), 'en'); - await shaka.test.Util.delay(1.5); - const chaptersUpdated = player.getChapters('en'); expect(chaptersUpdated.length).toBe(6); const chapterUpdated1 = chaptersUpdated[0]; @@ -1128,15 +1126,13 @@ describe('Player', () => { expect(chapterUpdated6.endTime).toBe(61.349); }); - it('add external chapters in srt format', async () => { + it('adds external chapters in srt format', async () => { await player.load('test:sintel_no_text_compiled'); const locationUri = new goog.Uri(location.href); const partialUri = new goog.Uri('/base/test/test/assets/chapters.srt'); const absoluteUri = locationUri.resolve(partialUri); await player.addChaptersTrack(absoluteUri.toString(), 'es'); - await shaka.test.Util.delay(1.5); - const chapters = player.getChapters('es'); expect(chapters.length).toBe(3); const chapter1 = chapters[0]; @@ -1152,5 +1148,5 @@ describe('Player', () => { expect(chapter3.startTime).toBe(30); expect(chapter3.endTime).toBe(61.349); }); - }); // describe('chapters') + }); // describe('addChaptersTrack') }); diff --git a/test/player_src_equals_integration.js b/test/player_src_equals_integration.js index 4e3e8a00df..196280836d 100644 --- a/test/player_src_equals_integration.js +++ b/test/player_src_equals_integration.js @@ -319,90 +319,88 @@ describe('Player Src Equals', () => { expect(newTrack).toBeTruthy(); }); - it('add external chapters in vtt format', async () => { - await loadWithSrcEquals(SMALL_MP4_CONTENT_URI, /* startTime= */ null); - - const locationUri = new goog.Uri(location.href); - const partialUri1 = new goog.Uri('/base/test/test/assets/chapters.vtt'); - const absoluteUri1 = locationUri.resolve(partialUri1); - await player.addChaptersTrack(absoluteUri1.toString(), 'en'); - - await shaka.test.Util.delay(1.5); - - const chapters = player.getChapters('en'); - expect(chapters.length).toBe(3); - const chapter1 = chapters[0]; - expect(chapter1.title).toBe('Chapter 1'); - expect(chapter1.startTime).toBe(0); - expect(chapter1.endTime).toBe(5); - const chapter2 = chapters[1]; - expect(chapter2.title).toBe('Chapter 2'); - expect(chapter2.startTime).toBe(5); - expect(chapter2.endTime).toBe(10); - const chapter3 = chapters[2]; - expect(chapter3.title).toBe('Chapter 3'); - expect(chapter3.startTime).toBe(10); - expect(chapter3.endTime).toBe(20); - - const partialUri2 = new goog.Uri('/base/test/test/assets/chapters2.vtt'); - const absoluteUri2 = locationUri.resolve(partialUri2); - await player.addChaptersTrack(absoluteUri2.toString(), 'en'); - - await shaka.test.Util.delay(1.5); - - const chaptersUpdated = player.getChapters('en'); - expect(chaptersUpdated.length).toBe(6); - const chapterUpdated1 = chaptersUpdated[0]; - expect(chapterUpdated1.title).toBe('Chapter 1'); - expect(chapterUpdated1.startTime).toBe(0); - expect(chapterUpdated1.endTime).toBe(5); - const chapterUpdated2 = chaptersUpdated[1]; - expect(chapterUpdated2.title).toBe('Chapter 2'); - expect(chapterUpdated2.startTime).toBe(5); - expect(chapterUpdated2.endTime).toBe(10); - const chapterUpdated3 = chaptersUpdated[2]; - expect(chapterUpdated3.title).toBe('Chapter 3'); - expect(chapterUpdated3.startTime).toBe(10); - expect(chapterUpdated3.endTime).toBe(20); - const chapterUpdated4 = chaptersUpdated[3]; - expect(chapterUpdated4.title).toBe('Chapter 4'); - expect(chapterUpdated4.startTime).toBe(20); - expect(chapterUpdated4.endTime).toBe(30); - const chapterUpdated5 = chaptersUpdated[4]; - expect(chapterUpdated5.title).toBe('Chapter 5'); - expect(chapterUpdated5.startTime).toBe(30); - expect(chapterUpdated5.endTime).toBe(40); - const chapterUpdated6 = chaptersUpdated[5]; - expect(chapterUpdated6.title).toBe('Chapter 6'); - expect(chapterUpdated6.startTime).toBe(40); - expect(chapterUpdated6.endTime).toBe(61.349); - }); - - it('add external chapters in srt format', async () => { - await loadWithSrcEquals(SMALL_MP4_CONTENT_URI, /* startTime= */ null); - - const locationUri = new goog.Uri(location.href); - const partialUri = new goog.Uri('/base/test/test/assets/chapters.srt'); - const absoluteUri = locationUri.resolve(partialUri); - await player.addChaptersTrack(absoluteUri.toString(), 'es'); - - await shaka.test.Util.delay(1.5); + describe('addChaptersTrack', () => { + it('adds external chapters in vtt format', async () => { + await loadWithSrcEquals(SMALL_MP4_CONTENT_URI, /* startTime= */ null); + + const locationUri = new goog.Uri(location.href); + const partialUri1 = new goog.Uri('/base/test/test/assets/chapters.vtt'); + const absoluteUri1 = locationUri.resolve(partialUri1); + await player.addChaptersTrack(absoluteUri1.toString(), 'en'); + + // Data should be available as soon as addChaptersTrack resolves. + // See https://github.com/shaka-project/shaka-player/issues/4186 + const chapters = player.getChapters('en'); + expect(chapters.length).toBe(3); + const chapter1 = chapters[0]; + expect(chapter1.title).toBe('Chapter 1'); + expect(chapter1.startTime).toBe(0); + expect(chapter1.endTime).toBe(5); + const chapter2 = chapters[1]; + expect(chapter2.title).toBe('Chapter 2'); + expect(chapter2.startTime).toBe(5); + expect(chapter2.endTime).toBe(10); + const chapter3 = chapters[2]; + expect(chapter3.title).toBe('Chapter 3'); + expect(chapter3.startTime).toBe(10); + expect(chapter3.endTime).toBe(20); + + const partialUri2 = new goog.Uri('/base/test/test/assets/chapters2.vtt'); + const absoluteUri2 = locationUri.resolve(partialUri2); + await player.addChaptersTrack(absoluteUri2.toString(), 'en'); + + const chaptersUpdated = player.getChapters('en'); + expect(chaptersUpdated.length).toBe(6); + const chapterUpdated1 = chaptersUpdated[0]; + expect(chapterUpdated1.title).toBe('Chapter 1'); + expect(chapterUpdated1.startTime).toBe(0); + expect(chapterUpdated1.endTime).toBe(5); + const chapterUpdated2 = chaptersUpdated[1]; + expect(chapterUpdated2.title).toBe('Chapter 2'); + expect(chapterUpdated2.startTime).toBe(5); + expect(chapterUpdated2.endTime).toBe(10); + const chapterUpdated3 = chaptersUpdated[2]; + expect(chapterUpdated3.title).toBe('Chapter 3'); + expect(chapterUpdated3.startTime).toBe(10); + expect(chapterUpdated3.endTime).toBe(20); + const chapterUpdated4 = chaptersUpdated[3]; + expect(chapterUpdated4.title).toBe('Chapter 4'); + expect(chapterUpdated4.startTime).toBe(20); + expect(chapterUpdated4.endTime).toBe(30); + const chapterUpdated5 = chaptersUpdated[4]; + expect(chapterUpdated5.title).toBe('Chapter 5'); + expect(chapterUpdated5.startTime).toBe(30); + expect(chapterUpdated5.endTime).toBe(40); + const chapterUpdated6 = chaptersUpdated[5]; + expect(chapterUpdated6.title).toBe('Chapter 6'); + expect(chapterUpdated6.startTime).toBe(40); + expect(chapterUpdated6.endTime).toBe(61.349); + }); - const chapters = player.getChapters('es'); - expect(chapters.length).toBe(3); - const chapter1 = chapters[0]; - expect(chapter1.title).toBe('Chapter 1'); - expect(chapter1.startTime).toBe(0); - expect(chapter1.endTime).toBe(5); - const chapter2 = chapters[1]; - expect(chapter2.title).toBe('Chapter 2'); - expect(chapter2.startTime).toBe(5); - expect(chapter2.endTime).toBe(30); - const chapter3 = chapters[2]; - expect(chapter3.title).toBe('Chapter 3'); - expect(chapter3.startTime).toBe(30); - expect(chapter3.endTime).toBe(61.349); - }); + it('add external chapters in srt format', async () => { + await loadWithSrcEquals(SMALL_MP4_CONTENT_URI, /* startTime= */ null); + + const locationUri = new goog.Uri(location.href); + const partialUri = new goog.Uri('/base/test/test/assets/chapters.srt'); + const absoluteUri = locationUri.resolve(partialUri); + await player.addChaptersTrack(absoluteUri.toString(), 'es'); + + const chapters = player.getChapters('es'); + expect(chapters.length).toBe(3); + const chapter1 = chapters[0]; + expect(chapter1.title).toBe('Chapter 1'); + expect(chapter1.startTime).toBe(0); + expect(chapter1.endTime).toBe(5); + const chapter2 = chapters[1]; + expect(chapter2.title).toBe('Chapter 2'); + expect(chapter2.startTime).toBe(5); + expect(chapter2.endTime).toBe(30); + const chapter3 = chapters[2]; + expect(chapter3.title).toBe('Chapter 3'); + expect(chapter3.startTime).toBe(30); + expect(chapter3.endTime).toBe(61.349); + }); + }); // describe('addChaptersTrack') // Since we are not in-charge of streaming, calling |retryStreaming| should // have no effect. diff --git a/test/test/util/simple_fakes.js b/test/test/util/simple_fakes.js index 4cfb33de05..f22ed4ad3a 100644 --- a/test/test/util/simple_fakes.js +++ b/test/test/util/simple_fakes.js @@ -152,9 +152,27 @@ shaka.test.FakeVideo = class { constructor(currentTime) { /** @const {!Object.} */ this.on = {}; // event listeners + /** @type {!Array.} */ this.textTracks = []; + // In a real video element, textTracks is an event target. + // Since Player listens to events on textTracks, we need to fake that + // interface. + this.textTracksEventTarget = new shaka.util.FakeEventTarget(); + this.textTracks.addEventListener = + // eslint-disable-next-line no-restricted-syntax + this.textTracksEventTarget.addEventListener.bind( + this.textTracksEventTarget); + this.textTracks.removeEventListener = + // eslint-disable-next-line no-restricted-syntax + this.textTracksEventTarget.removeEventListener.bind( + this.textTracksEventTarget); + this.textTracks.dispatchEvent = + // eslint-disable-next-line no-restricted-syntax + this.textTracksEventTarget.dispatchEvent.bind( + this.textTracksEventTarget); + this.currentTime = currentTime || 0; this.readyState = 0; this.playbackRate = 1; @@ -173,6 +191,10 @@ shaka.test.FakeVideo = class { jasmine.createSpy('addTextTrack').and.callFake((kind, id) => { const track = new shaka.test.FakeTextTrack(); this.textTracks.push(track); + + const trackEvent = new shaka.util.FakeEvent('addtrack', {track}); + this.textTracksEventTarget.dispatchEvent(trackEvent); + return track; });