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.

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

Add a 'loadeddata' event on TextTracks, which fixes issue #1864. #1949

Closed
wants to merge 1 commit into from

Conversation

OwenEdwards
Copy link
Member

Propose this useful feature for Text Track handling in general, which also fixes issue #1864.

Note that this needs broader testing, and a specific unit test for the fix; @gkatsev, you seem to be the expert on this? :-)

@gkatsev
Copy link
Member

gkatsev commented Mar 14, 2015

Awesome, thanks. I'll take a look over the weekend.

@gkatsev
Copy link
Member

gkatsev commented Mar 14, 2015

I'm not sure I like that we're adding a loadeddata event, given that these are supposed to be very much in-line with the spec. Maybe we need to add an actual shim for a element since that's what we can listen to for load events in native tracks.
And it would mean we could return an actual "track" object from addRemoteTextTrack that you could listen to events on.

@OwenEdwards
Copy link
Member Author

Oh, right, now I'm looking more closely at the WhatWG spec, in section sourcing-out-of-band-text-tracks, it mentions:

If the fetching algorithm does not fail, and the file was successfully processed, then the final task that is queued by the networking task source, after it has finished parsing the data, must change the text track readiness state to loaded, and fire a simple event named load at the track element.

I don't see the TextTrack element has a text-track-readiness-state, but you're right, really we should return an actual "track" object and fire a "load" event on that. I was working from the HTML5 spec, and this seemed reasonable!

@gkatsev
Copy link
Member

gkatsev commented Mar 15, 2015

The Tracks that videojs currently has are actual TextTracks as opposed to the HTMLTrackElement that you linked. I thought that only the former would be sufficient, but seems like even though it gives us 95% of what we need, the other 5% are pretty important too.

@gkatsev
Copy link
Member

gkatsev commented Mar 16, 2015

Once we have the HTMLTrackElement shim thing, we probably would still need this event but it just shouldn't be officially exported (like modechange).

@OwenEdwards
Copy link
Member Author

I see that the VideoJS TextTracks are not an HTML TrackElement. It's the process of calling addRemoteTrack, calling createTrackHelper, and taking src as an option (which I don't see documented?) which blurs the line between the two. There's the XHR call, and we need to know when that's completed (I'm looking to use it elsewhere). Otherwise, I'd XHR it myself, but then I could only pass it to TextTracks by parsing it myself and feeding it in as Cues, right?

@chemoish
Copy link
Member

@OwenEdwards Did you ever encounter race conditions with chapters because of this timeout?

@gkatsev
Copy link
Member

gkatsev commented Nov 24, 2015

I think that #2804 incorporates and supersedes this one. Closing.

@gkatsev gkatsev closed this Nov 24, 2015
chemoish pushed a commit to chemoish/video.js that referenced this pull request Dec 7, 2015
…in videojs#1949.

- Refactor `parseCue` to be managed by `loadTrack` so that `onflush` event can trigger parse complete (emulate load)
- Increased superficial load delay, to something more reliable, but it shouldn't be a problem if vtt.js is embedded in dist.
@OwenEdwards OwenEdwards deleted the bugfix-issue-1864 branch April 14, 2016 00:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants