From 1fda0d3fec44b87e20d597c1b600f46eb67ec758 Mon Sep 17 00:00:00 2001 From: Joey Parrish Date: Mon, 16 May 2022 12:23:57 -0700 Subject: [PATCH] fix: Wait for chapters track to be loaded 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 +++++++++++++------------- 4 files changed, 159 insertions(+), 106 deletions(-) diff --git a/lib/player.js b/lib/player.js index 38880f2d8b5..7af43b52079 100644 --- a/lib/player.js +++ b/lib/player.js @@ -1960,6 +1960,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. @@ -2259,6 +2288,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 @@ -2271,19 +2301,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( @@ -2540,12 +2574,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; } @@ -2558,10 +2592,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(() => { @@ -4550,25 +4581,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='); @@ -4622,6 +4669,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, @@ -4637,12 +4685,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 @@ -4652,7 +4702,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 f15eddce1c2..548766c824b 100644 --- a/lib/util/error.js +++ b/lib/util/error.js @@ -351,6 +351,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 22f513b478f..01eeb8b06e4 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 4e3e8a00df1..196280836d5 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.