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

Captions #1691

Closed
wants to merge 41 commits into from
Closed

Captions #1691

wants to merge 41 commits into from

Conversation

gkatsev
Copy link
Member

@gkatsev gkatsev commented Nov 25, 2014

This includes the beginning of vtt.js work and supersedes/is a continuation of #1656 but includes fancy captions rather than just native captions like the other PR does.

Don't auto-add the textTrackDisplay component.
Add a 'featuresNativeTrack'. Currently hardcoded to false for everything
except the html5 tech.
Add textTracks and addTextTrack methods to html5 tech.
Proxy player level methods to tech if we're using native tracks.
Fix up track menu items to work with spec tracks or vjs tracks.
Also, if text tracks are old and use numerical mode values, use custom
tracks.
If an html tech is using custom tracks, remove track elements so we
don't accidentally show both custom and native captions.
Clean up showTextTrack slightly.
textTracks is completely inside of techs but needs to be called manually
because techGet requires the tech to be ready.
addTextTrack, unfortunately, currently forks. If when called, it doesn't
have a tech, it assumes that it's a custom implementation and does the
same work that MediaTechController#addTextTrack does.
Don't create a flash getter for textTracks.
Load up textTrackDisplay synchronously but have it do no work until the
player is ready (see the previous commit).
Add a check that delegates to MediaTechController's methods if we're in
Html5 but we are not using native tracks.
MenuButton's new 'update' method conflicted with volumeMenuButton's
update method which was used to update volume levels.
gkatsev and others added 11 commits November 25, 2014 16:07
Remove video.js's track element emulation and pull in vtt.js to do that work instead. Add an option to force vtt.js-based captions to be used even if native caption support is present. Currently, vtt.js is loaded when the first TextTrack component is created. If vtt.js is not available when it's time to begin parsing cues, poll until it finishes downloading.
vtt.js is now loaded at the first text track creation so don't load it in the media tech controller as well.
vtt.js handles captions styling so don't bother with text track styles in video-js.less anymore.
Since vtt.js is parsing captions now, we don't need a unit test for captions parsing.
Put the option to override the URL to vtt.js somewhere that it can be easily accessed.
@heff
Copy link
Member

heff commented Nov 26, 2014

This looks like it's still mostly just the #1656 code so far. Make a note when you want this to be reviewed.

@gkatsev
Copy link
Member Author

gkatsev commented Nov 26, 2014

@heff yep, there's a lot more work to be done here, specifically around the TextTrack emulation. This PR is our WIP for getting it working in our player.

@gkatsev
Copy link
Member Author

gkatsev commented Dec 3, 2014

Closing this for now. We'll re-open a new PR with the latest changes for captions once they are ready.

@gkatsev gkatsev closed this Dec 3, 2014
@heff
Copy link
Member

heff commented Dec 8, 2014

Are the latest changes going to be added to this branch or do you have a new branch for it? If it's just going to be added we might want to reopen this since we're pointing a bunch of people to it.

@gkatsev
Copy link
Member Author

gkatsev commented Dec 8, 2014

There will be a new branch. That's why I'm pointing people just at #1655. I guess I should edit that.

@gkatsev
Copy link
Member Author

gkatsev commented Dec 8, 2014

Ah, actually, I'm not pointing at this one.

@gkatsev
Copy link
Member Author

gkatsev commented Dec 8, 2014

@heff I think today or tomorrow, I can open a new PR with all the changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants