Captions #1736

gkatsev · 2014-12-10T21:08:45Z

This is the new all encompassing captions PR.
It's still have some issues, like cuechange firing unconditionally and some issues with the captions displaying but there won't be much architectural changes to this.

I'd like at some point to take all the TextTrack stuff out of videojs and include it as a dependency similar to vttjs.

Things left to do:

A whole bunch more tests.
cuechange needs to be updated to only fire once per change. It currently fires a couple of times per change.
When adding a cue to a track programmatically, it needs to remove that cue from other text tracks if it existed in any other track.
The menus don't work great on mobile devices. Particularly, with respect to hiding the menu after usage.

gkatsev · 2014-12-10T23:00:19Z

Currently, the captions only show up if selected via the captions menu.

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.

heff · 2014-12-15T21:09:46Z

src/js/lib.js

@@ -357,6 +357,13 @@ vjs.removeClass = function(element, classToRemove){
 * @private
 */
 vjs.TEST_VID = vjs.createEl('video');
+(function() {


Is the main reason we need to add IIFEs like this because the dev version doesn't get automatically wrapped in one?

Yes but also to make a new context.

heff · 2014-12-16T00:13:47Z

I think I'm starting to understand the reason why there's no removeTextTrack API for the video element. The most common use case for removing tracks is going to be switching sources. When switching sources you would also be removing all source elements from the video element and replacing them with new source elements, so it makes sense to handle tracks the same way. There's no videoEl.sources property like videoEl.textTracks, so this isn't an issue with sources.

So the primary control mechanism for tracks is really the DOM interface, and addTextTrack is a nice to have add-on that's really meant to allow for other dynamic interactions, not captions (even though it's possible). This kind of explains the lack of the src option in addTextTrack.

So then the key issue here is that if the browser was to provide a removeTextTrack API, when you removed a track from videoEl.textTracks it would have to find it's corresponding track element from the video element and remove it. I expect that modifying DOM elements like that is something browsers try to avoid, at least outside of explicit APIs like removeChild.

Looking at it from that perspective (specifically that the DOM interface is the primary one) I think it gives more weight towards @dmlap's suggestions around mimicking that interface, outlined in #1728 part 2.

heff · 2014-12-16T00:33:18Z

src/js/media/media.js

@@ -29,6 +33,59 @@ vjs.MediaTechController = vjs.Component.extend({
    }

    this.initControlsListeners();
+
+    if (!this['featuresTextTracks']) {
+      textTrackDisplay = player.addChild('textTrackDisplay');


Another option here would be to have textTrackDisplay always be a child of player, and have it disable itself whenever nativeTextTracks are used. That might be a little cleaner than having a child of player inject a sibling.

As is there might be a bug here. It doesn't look like textTrackDisplay gets removed when this tech is disposed, so the next tech will get loaded and add a second textTrackDisplay.

I believe that the default component functionality takes care of disposing the textTrackDisplay.

A component will automatically dispose all its children, but textTrackDisplay isn't being added as a child of this tech. When we switch techs the tech gets disposed but the player doesn't, so the textTrackDisplay wouldn't be disposed in that case.

Fixed in 7d702c9

heff · 2014-12-16T01:31:46Z

I'm getting this compiler error when I try to build this locally. It looks like that's what's tripping up travis.

build/files/combined.video.js:8588: ERROR - Variable getProp first declared in build/files/combined.video.js
var getProp = function(obj, prop) {

heff · 2014-12-16T01:36:03Z

src/js/tracks/player-api.js

+ * Descriptions (not supported yet) - audio descriptions that are read back to the user by a screen reading device
+ */
+
+var getProp = function(obj, prop) {


I think this can be removed? It's duplicated here https://github.com/videojs/video.js/pull/1736/files#diff-8169d53aa7eee6cab5f85b6641ef3117R1

getProp should just be removed altogether.

Removed in 33417f6

Clean up text track related listeners.

dmlap · 2014-12-16T23:48:28Z

src/js/tracks/text-track.js

+
+  options = options || {};
+
+  player = options.player || {


Why is this optional? Can this realistically be omitted?

Um... probably not, actually.

Addressed here 78087de, currently throwing but open to other suggestions.

heff · 2014-12-17T00:55:44Z

RE: Captions Settings

I think it'd be better if the color selection actually showed the color instead of the word. I like the PBS version best, but the BC color dropdown could work too.
It'd be nice if you could see what the captions look like as you're changing the settings. It shouldn't be too difficult to create an example caption above the settings.

gkatsev · 2014-12-17T04:51:22Z

Yeah, captions settings stuff needs to be re-written from scratch.

gkatsev · 2014-12-23T01:18:53Z

all current comments have been addressed (other than the settings menu). Closing this now. We should move forward to the new PR #1749.

gkatsev mentioned this pull request Dec 11, 2014

TextTrackList and TextTrack and TextTrackCueList mozilla/vtt.js#335

Open

gkatsev added 28 commits December 11, 2014 18:01

Re-add text tracks on 'movingMediaElementInDOM'

55efd27

Use custom tracks if no native textTracks support

1a06519

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.

Use setAttribute and 'in' so IE8 doesn't feak out.

354bc51

Firefox gets custom captions for now

aefc333

Fix tests. Assume native text tracks, intially

5ad1a3e

Add tracks to texttrackdisplay on ready

60f8fbb

Add tracks buttons async

64b9a91

Create custom tracks with the correct player

76cabc3

Move addTextTrack fully into techs.

a32c878

Load up textTrackDisplay synchronously but have it do no work until the player is ready (see the previous commit).

Move controlbar addition into player

5011e9c

In Html5, support tech, support non native tracks

7de599a

Add a check that delegates to MediaTechController's methods if we're in Html5 but we are not using native tracks.

Add an 'update' method to menu

7b9f388

Move tracks buttons to controlbar children

10abed2

Update tracks buttons if needed

f8dea9f

Stub out ready and addChild for player

4cb5a16

A whole bunch of tests

61bb044

Appease jshint.

e2b7781

reset prototype value in media test

b9b6036

appease phantomjs

d080061

Extern/export addTextTrack

3c95af5

Update tests to work when minified

c7f5d6f

have vjs do the "addChild" "magic"

336f2a7

Rename featuresNativeTracks to featuresTextTracks

84afdf8

Move textTracks_ into tech

4e7a9dc

Update text track btns based on add/remove events

dc108fb

Update tests

86bd6a4

heff mentioned this pull request Dec 15, 2014

Formalize tech "features" #1474

Open

heff reviewed Dec 15, 2014
View reviewed changes

heff reviewed Dec 16, 2014
View reviewed changes

Remove event listeners on dispose

4e02998

Clean up text track related listeners.

dmlap reviewed Dec 16, 2014
View reviewed changes

gkatsev mentioned this pull request Dec 17, 2014

Captions #1749

Closed

3 tasks

gkatsev closed this Dec 23, 2014

heff mentioned this pull request Jul 17, 2015

Handle switching between sets of text tracks #2369

Open

gkatsev deleted the text-tracks-api branch August 12, 2015 18:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Captions #1736

Captions #1736

gkatsev commented Dec 10, 2014

gkatsev commented Dec 10, 2014

heff Dec 15, 2014

gkatsev Dec 19, 2014

heff commented Dec 16, 2014

heff Dec 16, 2014

gkatsev Dec 16, 2014

heff Dec 16, 2014

gkatsev Dec 22, 2014

heff commented Dec 16, 2014

heff Dec 16, 2014

gkatsev Dec 19, 2014

gkatsev Dec 19, 2014

dmlap Dec 16, 2014

gkatsev Dec 17, 2014

gkatsev Dec 19, 2014

heff commented Dec 17, 2014

gkatsev commented Dec 17, 2014

gkatsev commented Dec 23, 2014

Captions #1736

Captions #1736

Conversation

gkatsev commented Dec 10, 2014

gkatsev commented Dec 10, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heff commented Dec 16, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heff commented Dec 16, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heff commented Dec 17, 2014

gkatsev commented Dec 17, 2014

gkatsev commented Dec 23, 2014