Skip to content

Commit

Permalink
Fix Chapters.
Browse files Browse the repository at this point in the history
Return null for cues and activeCues until the cues have loaded. This mimics how chrome does it.
Add a property called loaded_ on textTracks to know whether we have loaded it yet or not.
  • Loading branch information
gkatsev committed Jan 24, 2015
1 parent e202f9b commit 83a53af
Show file tree
Hide file tree
Showing 2 changed files with 24 additions and 12 deletions.
24 changes: 13 additions & 11 deletions src/js/tracks/text-track-controls.js
Original file line number Diff line number Diff line change
Expand Up @@ -478,16 +478,18 @@ vjs.ChaptersButton.prototype.createItems = function(){
vjs.ChaptersButton.prototype.createMenu = function(){
var tracks = this.player_.textTracks() || [],
i = 0,
j = tracks.length,
l = tracks.length,
track, chaptersTrack,
items = this.items = [];

for (; i < j; i++) {
for (; i < l; i++) {
track = tracks[i];
if (track['kind'] == this.kind_) {
if (track.readyState() === 0) {
track.load();
track.on('loaded', vjs.bind(this, this.createMenu));
if (!track.cues) {
track['mode'] = 'hidden';
window.setTimeout(vjs.bind(this, function() {

This comment has been minimized.

Copy link
@gkatsev

gkatsev Jan 24, 2015

Author Owner

I wasn't sure how best to handle this. We need some way to delay this until after the cues have loaded but textTracks themselves don't have any 'load' events or anything.
@dmlap @heff thoughts? I guess the main issue here is that if the player gets disposed during the timeout but we could listen to dispose or use the new component.setTimeout. Also, 100ms might be too high.

This comment has been minimized.

Copy link
@heff

heff Jan 24, 2015

So this is chapters specific. We could potentially assume (require that) chapters start at zero and don't have gaps, and then listen for a cuechange event here, which should be triggered as soon as the cues are loaded and the first cue becomes active.

This comment has been minimized.

Copy link
@gkatsev

gkatsev Feb 12, 2015

Author Owner

No, haven't had a better idea yet. I don't think that waiting for cuechange is good because the whole idea is that you have the chapters before playback starts so you could go directly to a chapter.

This comment has been minimized.

Copy link
@gkatsev

gkatsev Feb 12, 2015

Author Owner

@dmlap do you have any thoughts?

This comment has been minimized.

Copy link
@gkatsev

gkatsev Feb 13, 2015

Author Owner

I'll add a TODO and leave this as-in.

This comment has been minimized.

Copy link
@heff

heff Feb 13, 2015

Does this break chapters? If so we can't leave this unfixed.

This comment has been minimized.

Copy link
@gkatsev

gkatsev Feb 13, 2015

Author Owner

No, it works. It's just weird.

This comment has been minimized.

Copy link
@heff

heff Feb 13, 2015

Ha, ok cool.

this.createMenu();
}), 100);
} else {
chaptersTrack = track;
break;
Expand All @@ -506,11 +508,11 @@ vjs.ChaptersButton.prototype.createMenu = function(){
}

if (chaptersTrack) {
var cues = chaptersTrack.cues_, cue, mi;
var cues = chaptersTrack.cues, cue, mi;
i = 0;
j = cues.length;
l = cues.length;

for (;i<j;i++) {
for (; i < l; i++) {
cue = cues[i];

mi = new vjs.ChaptersTrackMenuItem(this.player_, {
Expand Down Expand Up @@ -545,10 +547,10 @@ vjs.ChaptersTrackMenuItem = vjs.MenuItem.extend({

// Modify options for parent MenuItem class's init.
options['label'] = cue.text;
options['selected'] = (cue.startTime <= currentTime && currentTime < cue.endTime);
options['selected'] = (cue['startTime'] <= currentTime && currentTime < cue['endTime']);
vjs.MenuItem.call(this, player, options);

track.on('cuechange', vjs.bind(this, this.update));
track.addEventListener('cuechange', vjs.bind(this, this.update));
}
});

Expand All @@ -563,6 +565,6 @@ vjs.ChaptersTrackMenuItem.prototype.update = function(){
currentTime = this.player_.currentTime();

// vjs.log(currentTime, cue.startTime);
this.selected(cue.startTime <= currentTime && currentTime < cue.endTime);
this.selected(cue['startTime'] <= currentTime && currentTime < cue['endTime']);
};
})();
12 changes: 11 additions & 1 deletion src/js/tracks/text-track.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ vjs.TextTrack = function(options) {
language = options['language'] || options['srclang'] || '';
id = options['id'] || 'vjs_text_track_' + vjs.guid++;

if (kind === 'metadata') {
if (kind === 'metadata' || kind === 'chapters') {
mode = 'hidden';
}

Expand Down Expand Up @@ -116,6 +116,10 @@ vjs.TextTrack = function(options) {

Object.defineProperty(tt, 'cues', {
get: function() {
if (!this.loaded_) {
return null;
}

return cues;
},
set: Function.prototype
Expand All @@ -125,6 +129,10 @@ vjs.TextTrack = function(options) {
get: function() {
var i, l, active, ct, cue;

if (!this.loaded_) {
return null;
}

if (this['cues'].length === 0) {
return activeCues; // nothing to do
}
Expand Down Expand Up @@ -228,6 +236,8 @@ loadTrack = function(src, track) {
return vjs.log.error(err);
}


track.loaded_ = true;
parseCues(responseBody, track);
}));
};
Expand Down

0 comments on commit 83a53af

Please sign in to comment.