Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: Wait for chapters track to be loaded #4228

Merged
merged 2 commits into from May 16, 2022
Merged

fix: Wait for chapters track to be loaded #4228

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
77a68f3
fix: Wait for chapters track to be loaded
joeyparrish May 16, 2022
74d570b
Fix test failures by adding to the FakeVideo implementation
joeyparrish May 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
78 changes: 65 additions & 13 deletions lib/player.js
View file
Open in desktop
Expand Up @@ -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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean that, if we unload and load an asset with text tracks over and over again, it will leave orphaned event listeners? Since we don't unlisten to any events on the video, on unload.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooh, good question. I see now that in unload() or detach(), we do a lot of unlisten() calls for various events.

This pattern was already in place around track events for src=. If this is the only issue with the PR, how about we let the fix stand as is, and I'll follow-up immediately with a full audit of listen()/unlisten() in Player?

Probably this pattern was never actually necessary, but whoever added it wasn't aware of the way we were cleaning up other event listeners in the load graph. I mean, even I wasn't aware, clearly.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fine, yeah.

}

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.
Expand Down Expand Up @@ -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
Expand All @@ -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(
Expand Down Expand Up @@ -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;
}
Expand All @@ -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(() => {
Expand Down Expand Up @@ -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=');
Expand Down Expand Up @@ -4622,6 +4669,7 @@ shaka.Player = class extends shaka.util.FakeEventTarget {
* @param {string} mimeType
* @param {string} label
* @param {!Array.<!shaka.extern.AdCuePoint>} adCuePoints
* @return {!Promise.<!HTMLTrackElement>}
* @private
*/
async addSrcTrackElement_(uri, language, kind, mimeType, label,
Expand All @@ -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
Expand All @@ -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;
}

/**
Expand Down
7 changes: 7 additions & 0 deletions lib/util/error.js
View file
Open in desktop
Expand Up @@ -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.
Expand Down
16 changes: 6 additions & 10 deletions test/player_integration.js
View file
Open in desktop
Expand Up @@ -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];
Expand All @@ -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];
Expand Down Expand Up @@ -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];
Expand All @@ -1152,5 +1148,5 @@ describe('Player', () => {
expect(chapter3.startTime).toBe(30);
expect(chapter3.endTime).toBe(61.349);
});
}); // describe('chapters')
}); // describe('addChaptersTrack')
});