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 #1749

Closed
wants to merge 236 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@gkatsev
Member

gkatsev commented Dec 17, 2014

This PR supercedes the other PRs (#1736)

  • fix up one last test
  • more tests
  • fixup settings menu styles (prettier/reworked should come as a separate PR)

gkatsev and others added some commits Nov 11, 2014

Support native captions in html5 tech.
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.
Use custom tracks if no native textTracks support
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.
Move as much as possible into techs.
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.
Move addTextTrack fully into techs.
Load up textTrackDisplay synchronously but have it do no work until the
player is ready (see the previous commit).
In Html5, support tech, support non native tracks
Add a check that delegates to MediaTechController's methods if we're in
Html5 but we are not using native tracks.
Rename volumeMenuButton's update to volumeUpdate
MenuButton's new 'update' method conflicted with volumeMenuButton's
update method which was used to update volume levels.
Use vtt.js for track element emulation
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.
Finish moving lazy load
vtt.js is now loaded at the first text track creation so don't load it in the media tech controller as well.
Fix styling.
vtt.js handles captions styling so don't bother with text track styles in video-js.less anymore.
Remove obsolete cue parsing test
Since vtt.js is parsing captions now, we don't need a unit test for captions parsing.
Allow overriding vtt.js URL
Put the option to override the URL to vtt.js somewhere that it can be easily accessed.
@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Jan 24, 2015

Member

Re label-less: I like falling back to srclang and then to "unknown". If they leave both off it's probably unintentional or that they don't understand what's needed. Showing "unknown" would be easier to debug than having them not show up at all. At least I can't think of any use case for hiding captions from the captions button.

Member

heff commented Jan 24, 2015

Re label-less: I like falling back to srclang and then to "unknown". If they leave both off it's probably unintentional or that they don't understand what's needed. Showing "unknown" would be easier to debug than having them not show up at all. At least I can't think of any use case for hiding captions from the captions button.

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Jan 24, 2015

Member

Would it be just "unknown" or "unknown {{kind}}"?

Member

gkatsev commented Jan 24, 2015

Would it be just "unknown" or "unknown {{kind}}"?

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Jan 24, 2015

Member

Kind seems unnecessary in the captions menu. "unknown language" might be more appropriate, but I think I'd stick with just unknown.

Member

heff commented Jan 24, 2015

Kind seems unnecessary in the captions menu. "unknown language" might be more appropriate, but I think I'd stick with just unknown.

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Jan 24, 2015

Member

This would apply to subtitles too. We can have it say "unknown subtitles" and "unknown captions". Just unknown is fine too.

Member

gkatsev commented Jan 24, 2015

This would apply to subtitles too. We can have it say "unknown subtitles" and "unknown captions". Just unknown is fine too.

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Jan 24, 2015

Member

Yeah, I still like unknown. When they are labeled it's typically just "English" not "English Captions"

Member

heff commented Jan 24, 2015

Yeah, I still like unknown. When they are labeled it's typically just "English" not "English Captions"

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Jan 24, 2015

Member

Right, makes sense. I think iOS does "unknown captions" because I think they have only one menu for all text tracks.

Member

gkatsev commented Jan 24, 2015

Right, makes sense. I think iOS does "unknown captions" because I think they have only one menu for all text tracks.

gkatsev added some commits Jan 26, 2015

Merge branch 'master' into tt-with-style
Conflicts:
	package.json
	src/css/video-js.less
	src/js/exports.js
Don't hide tracks with no label.
Instead, default to langauge or "Unknown";
Move text track display to before big play button.
IE10 and below don't support pointer-events:none, so, we want to make
sure that the big play button will be clickable.
@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Feb 10, 2015

Instead of checking instanceof is there a property that's on both versions of the lists that we could check here so that this works in IE8 too?

heff commented on test/unit/tracks/text-track.js in e202f9b Feb 10, 2015

Instead of checking instanceof is there a property that's on both versions of the lists that we could check here so that this works in IE8 too?

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Feb 10, 2015

Owner

I could duck type these.

Owner

gkatsev replied Feb 10, 2015

I could duck type these.

This comment has been minimized.

Show comment
Hide comment
@heff

heff Feb 10, 2015

Yeah, that seems more valuable than just turning it off for ie8.

heff replied Feb 10, 2015

Yeah, that seems more valuable than just turning it off for ie8.

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Feb 13, 2015

Owner

Done.

Owner

gkatsev replied Feb 13, 2015

Done.

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Feb 10, 2015

I don't think that just saying this breaks in IE8 is a good enough reason to turn it off for ie8. :-P

Why isn't it working in IE8? And if the goal is to have tracks that work everywhere, why is this test still valuable otherwise?

heff commented on test/unit/tracks/text-track-controls.js in e202f9b Feb 10, 2015

I don't think that just saying this breaks in IE8 is a good enough reason to turn it off for ie8. :-P

Why isn't it working in IE8? And if the goal is to have tracks that work everywhere, why is this test still valuable otherwise?

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Feb 10, 2015

Owner

This test specifically tests a issue that ios7 has. So, even if it doesn't run on IE8, it's still valuable to have.
I do not remember why it was working on IE8 or if I investigated it.

Owner

gkatsev replied Feb 10, 2015

This test specifically tests a issue that ios7 has. So, even if it doesn't run on IE8, it's still valuable to have.
I do not remember why it was working on IE8 or if I investigated it.

This comment has been minimized.

Show comment
Hide comment
@heff

heff Feb 10, 2015

Ok, cool. I think we either need a much more clear comment or to scope it to vjs.IS_IOS instead so it's more obvious what the test is limited for.

heff replied Feb 10, 2015

Ok, cool. I think we either need a much more clear comment or to scope it to vjs.IS_IOS instead so it's more obvious what the test is limited for.

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Feb 13, 2015

Owner

Decided to leave a more elaborate comment and open an issue: videojs#1861

Owner

gkatsev replied Feb 13, 2015

Decided to leave a more elaborate comment and open an issue: videojs#1861

@@ -21,7 +21,8 @@
},
"main": "./dist/video-js/video.js",
"dependencies": {
"videojs-swf": "4.5.3"
"videojs-swf": "4.5.3",
"vtt.js": "git+https://github.com/gkatsev/vtt.js.git#shim-build"

This comment has been minimized.

@heff

heff Feb 10, 2015

Member

Do the recent merges mean we can change this to the main lib yet?

@heff

heff Feb 10, 2015

Member

Do the recent merges mean we can change this to the main lib yet?

This comment has been minimized.

@gkatsev

gkatsev Feb 10, 2015

Member

Unfortunately, not yet. All the IE8 changes went in but the shim build stuff still need to get tweaked first.

@gkatsev

gkatsev Feb 10, 2015

Member

Unfortunately, not yet. All the IE8 changes went in but the shim build stuff still need to get tweaked first.

Show outdated Hide outdated src/css/video-js.less
if (supportsTextTracks && vjs.TEST_VID.textTracks.length > 0) {
supportsTextTracks = typeof vjs.TEST_VID.textTracks[0]['mode'] !== 'number';
}
if (supportsTextTracks && vjs.IS_FIREFOX) {

This comment has been minimized.

@heff

heff Feb 10, 2015

Member

It feels like a shame to disable native captions in firefox completely. I feel like we should submit a bug, or see if one already exists that we can track for this.

@heff

heff Feb 10, 2015

Member

It feels like a shame to disable native captions in firefox completely. I feel like we should submit a bug, or see if one already exists that we can track for this.

This comment has been minimized.

@gkatsev

gkatsev Feb 10, 2015

Member

Yeah. I just didn't have time to figure out what was going on. Still not certain where the root of the problem is.

@gkatsev

gkatsev Feb 10, 2015

Member

Yeah. I just didn't have time to figure out what was going on. Still not certain where the root of the problem is.

This comment has been minimized.

@heff

heff Feb 10, 2015

Member

Ok, if that's gonna take work, can we add a TODO comment here for now so we can move forward?

@heff

heff Feb 10, 2015

Member

Ok, if that's gonna take work, can we add a TODO comment here for now so we can move forward?

This comment has been minimized.

@gkatsev

gkatsev Feb 13, 2015

Member

Done.

@gkatsev

gkatsev Feb 13, 2015

Member

Done.

var player = this.player_,
tracks,
textTrackListChanges = function() {
var textTrackDisplay = player.getChild('textTrackDisplay'),

This comment has been minimized.

@heff

heff Feb 10, 2015

Member

Was there a specific reason we couldn't let the textTrackDisplay handle updating itself instead of requiring the tech to update it?

@heff

heff Feb 10, 2015

Member

Was there a specific reason we couldn't let the textTrackDisplay handle updating itself instead of requiring the tech to update it?

This comment has been minimized.

@gkatsev

gkatsev Feb 10, 2015

Member

Not certain. It's possible that this function can be moved to the text track display.

@gkatsev

gkatsev Feb 10, 2015

Member

Not certain. It's possible that this function can be moved to the text track display.

This comment has been minimized.

@heff

heff Feb 10, 2015

Member

Same here as far as adding a TODO comment.

@heff

heff Feb 10, 2015

Member

Same here as far as adding a TODO comment.

This comment has been minimized.

@gkatsev

gkatsev Feb 13, 2015

Member

Done.

@gkatsev

gkatsev Feb 13, 2015

Member

Done.

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Feb 12, 2015

Member

@gkatsev I'll work on some of the remaining cleanup tasks.

I don't see a resolution to the last chapters related question. Were you able to figure that out?

Member

heff commented Feb 12, 2015

@gkatsev I'll work on some of the remaining cleanup tasks.

I don't see a resolution to the last chapters related question. Were you able to figure that out?

@gkatsev gkatsev modified the milestone: Captions Feb 13, 2015

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Feb 13, 2015

Member

@heff pushed TODOs. Would you want me to merge master into this branch so it's easier to merge this back into master?

Member

gkatsev commented Feb 13, 2015

@heff pushed TODOs. Would you want me to merge master into this branch so it's easier to merge this back into master?

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Feb 13, 2015

Member

Thanks for handling those! Yeah, if you have a minute to merge master you're in a better place to understand merge conflicts, but otherwise I can take care of it.

Member

heff commented Feb 13, 2015

Thanks for handling those! Yeah, if you have a minute to merge master you're in a better place to understand merge conflicts, but otherwise I can take care of it.

Merge branch 'master' into tt-with-style
Conflicts:
	src/js/media/html5.js
	src/js/player.js
@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Feb 13, 2015

Member

@heff merged in.

Member

gkatsev commented Feb 13, 2015

@heff merged in.

@gkatsev gkatsev closed this in 4e5c28c Feb 13, 2015

@gkatsev gkatsev deleted the gkatsev:tt-with-style branch Aug 12, 2015

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