Load text tracks from dash into video.js #135

justinanastos · 2017-01-17T21:40:10Z

As of video.js v5.14, video.js no longer supports native subtitles on anything besides Safari. For Chrome, this means there is no subtitle support. This will wait for dash.js to load all the text tracks and then add them to video.js if we're using a version that does not attempt to use native subtitles. Audio tracks will also be added.

Todo

Replace.map with .forEach Load text tracks from dash into video.js #135 (review)
Remove WeakMap Load text tracks from dash into video.js #135 (review)
Make sure to clean everything up when source is changed. See Start using remote TextTracks because they can be properly removed videojs-contrib-media-sources#118 (review)
Don't use Map
Fix Safari
Update README.md
Audio tracks looks good to me but I'm not seeing the text track cues populate for http://vm2.dashif.org/dash/vod/testpic_2s/multi_subs.mpd
~~Location for automaticTextTrackSelection option~~

~~Wait for videojs/video.js#4131 to be merged. Add that version as an explicit dependency (maybe change it to a peerDependency and devDependency since we don't actually need it here?~~

This fix depends on videojs@^v5.18

chemoish · 2017-01-18T01:47:43Z

I might look into generating cues before calling addRemoteTextTrack, not sure if this will effect anything, but last time I was in TextTrack I did notice a few setTimeout and Events that caused race conditions (just to be aware).

Somethings to consider in this implementation:

How to manage the selected track? (haven't done a deep dive with managing initial track selection modes, but I did notice that embedded captions were being defaulted by the dash.js reference player)
Apparently there are a few types of dash.js tracks that can be embedded, they can also have different roles, I am not sure how that would effect this implementation (since this looks like it only manages captions)—fyi, there isn't a 1:1 list of TextTracks from dash.js to HTML5 spec.

justinanastos · 2017-01-18T20:22:52Z

@chemoish

I might look into generating cues before calling addRemoteTextTrack, not sure if this will effect anything, but last time I was in TextTrack I did notice a few setTimeout and Events that caused race conditions (just to be aware).

I'm new to working with video.js's implementation, but the addRemoteTextTrack function appears to create the HTMLTrackElement element and I need that to add the cues.

Stepping through the Html5 tech implementation related to addRemoteTextTrack, (https://github.com/videojs/video.js/blob/master/src/js/tech/html5.js#L712-L738), I didn't see any Events or setTimeouts (at least not in v15.1). addRemoteTextTrack accepts an options object that is fed directly to the native HTMLTrackElement which will optionally applykind, label, srclang, default, and src; if any of those exist.

How to manage the selected track? (haven't done a deep dive with managing initial track selection modes, but I did notice that embedded captions were being defaulted by the dash.js reference player)

I updated how I handle this. If I apply the default option, then both the defaulted track and "captions off" will be selected. I will loop through all the tracks after they have been generated and set the mode attribute to "showing" for default: true and "hidden" otherwise.

Apparently there are a few types of dash.js tracks that can be embedded, they can also have different roles, I am not sure how that would effect this implementation (since this looks like it only manages captions)—fyi, there isn't a 1:1 list of TextTracks from dash.js to HTML5 spec.

Agreed, the source for TextSourceBugger.js appears to show that dash.js only supports captions and subtitles. Using the kind property that comes from the dash parsing and feeding that kind into video.js using this implementation will show two separate (and functional) captions and subtitles menus.

justinanastos · 2017-01-19T17:42:59Z

I've reviewed all the samples in #103. This PR in it's current state does not match the functionality of the dash reference player. I'll keep working on it.

justinanastos · 2017-01-20T21:40:10Z

With this update, it's worked with everything I can throw at it @chemoish .

justinanastos · 2017-01-23T18:37:56Z

I found what I perceive to be a flaw in the text track selection logic. This assumes a 1:1 relationship between dash text tracks and videojs's DOM track list. This is not guaranteed because the user can manipulate the videojs text tracks however they like (or not even use them), so this needs to be modified.

chemoish · 2017-01-23T23:56:57Z

src/js/setup-text-tracks.js

+      // Example:
+      // https://storage.googleapis.com/shaka-demo-assets/sintel-mp4-wvtt/dash.mpd
+
+      // Return `track` so we can continue chaning.


chaining + does this require chaining still?

@chemoish No, it doesn't :)

possibly update to .each and remove comment/return.

chemoish · 2017-01-24T00:07:04Z

src/js/videojs-dash.js

+
+    // When `dashjs` finishes loading metadata, create audio tracks for
+    // `video.js`.
+    this.mediaPlayer_.on(


minor, any reason why the event handler is bound inline rather than within the setup (different implementation than textTrack)?

No good reason @chemoish , I will update it to make them consistent.

justinanastos · 2017-01-24T21:55:54Z

Updates pushed with the following changes:

Text track handling is no longer async because I am passing tech into setupTextTracks and it can then check for tech.featuresNativeTextTracks.
setupTextTracks is called in the same fashion as setupAudioTracks
Text track handling is no longer dependent on the dash text track list being a 1:1 match for the videojs track list because users can destroy or create text tracks at will. This uses a WeakMap as a dictionary.

gkatsev · 2017-01-24T22:41:53Z

src/js/setup-text-tracks.js

+function attachDashTextTracksToVideojs(player, tracks) {
+  const mediaPlayer = player.dash.mediaPlayer;
+
+  const trackDictionaryKeyedByTextTrack = new WeakMap();


I don't think it's a good idea to use WeakMap yet, still. To support the widest field of browsers possible, we can't use WeakMap and WeakMap isn't really polyfillable.

I'll replace it with a Map()

We might not want to either Map either depending on which browsers we're targeting for this project. We wouldn't want to require es6-shim for Map() support if we expect to be able to run this package in a browser that supports MSE+EME but not Map().

@forbesjo do need to worry about this?

My mistake, I didn't check to see how the babel transforms were handled. Map can't be polyfilled so I agree it's unacceptable. I'll replace it.

gesinger · 2017-01-26T18:48:07Z

src/js/setup-text-tracks.js

+  tracks
+    // Map input data to match HTMLTrackElement spec
+    // https://developer.mozilla.org/en-US/docs/Web/API/HTMLTrackElement
+    .map((track) => Object.assign(


Any specific reason to assign on top of the dash.js track rather than just create a new object containing only the properties needed by the video.js track?

No good reason, I can axe the track reference.

gesinger · 2017-01-26T18:48:12Z

src/js/setup-text-tracks.js

+    const textTracks = this.textTracks();
+    let activeTextTrackIndex = -1;
+
+    function findMatchingTrack(dashTrack) {


Since this is only used in once spot, it may be easier to read if it is just inlined.

That was my original plan, but jshint didn't like me creating functions within a loop.

gesinger · 2017-01-26T18:48:15Z

src/js/setup-text-tracks.js

+    // Iterate through the tracks and find the one marked as showing.
+    // If none are showing, `activeTextTrackIndex` will be set to
+    // `-1`, disabling text tracks.
+    for (let i = 0; i < textTracks.length; i += 1) {


i++ just for consistent style

gesinger · 2017-01-26T18:55:40Z

src/js/setup-text-tracks.js

+    // Add track to videojs track list
+    .map((track) => {
+      const remoteTextTrack = player.addRemoteTextTrack(track, true);
+      trackDictionaryKeyedByTextTrack.set(remoteTextTrack.track, track);


Might make sense to match the original dash.js track (instead of the new reference) to make it easier down below (can skip trying to find the reference based on index matching, since we have the reference).

I'm not sure I understand. Later on I use index matching because I have to loop through player.textTracks() to find the active track. After that, I use that track to figure out the dash track. dash.setTextTrack() expects an index so I have to first find it, then figure out it's index. I must be missing something?

Great point. When we're using remote time text files .getTracksFor('text') will return an empty array so we have no choice but to use the tracks passed in the event handler.

gesinger · 2017-01-26T19:02:41Z

src/js/setup-text-tracks.js

+  */
+  const textTracks = player.textTracks();
+
+  for (let i = 0; i < textTracks.length; i += 1) {


i++ just for consistent style

gesinger · 2017-01-26T19:14:43Z

src/js/setup-text-tracks.js

+  mediaPlayer.updateDashTextTrack();
+
+  // Update dash when videojs's selected text track changes. Use an arrow function so
+  // `mediaPlayer.updateDashTextTrack` can be overriden exterally.


I'm not sure if we are ready yet to provide an external function that people can override. If we do though, we may want to add it onto player.dash, since mediaPlayer is technically a dash.js object that we don't have control over.

This was a mistake, I had used this logic elsewhere and forgot to remove it. I'll remove it here.

justinanastos · 2017-01-27T23:08:23Z

Updated @gesinger , @gkatsev , and @chemoish .

justinanastos · 2017-01-28T16:44:48Z

Use of Map has been removed.

justinanastos · 2017-01-28T21:19:33Z

The audio track manipulation breaks dash playback in Safari 10.0.3. I am investigating.

forbesjo · 2017-01-30T15:35:55Z

src/js/videojs-dash.js

@@ -89,6 +91,12 @@ class Html5DashJS {
    this.mediaPlayer_.setProtectionData(this.keySystemOptions_);
    this.mediaPlayer_.attachSource(manifestSource);

+    // Setup text tracks
+    setupTextTracks.call(null, this.player, tech);


Should these be called before attachSource()?

My concern is that dashjs.MediaPlayer.events.TEXT_TRACKS_ADDED/dashjs.MediaPlayer.events.PLAYBACK_METADATA_LOADED may fire between attachSource() and the event listener setup functions

This makes sense. If the user can fetch a source and attach it, this might become a sync call and the events might get missed. I've moved the calls to be before attachSource().

justinanastos · 2017-01-30T22:55:28Z

I have a question and could use some advice. I'm having trouble bypassing native captions in Safari because of this line:

https://github.com/videojs/video.js/blob/master/src/js/tech/html5.js#L798

I have been unsuccessful in finding a smooth way to disable native text tracks other than globally disabling it in the Html5 tech when videojs-contrib-dash is included, but this might break other techs/plugins. The only solution I have found that seems to work is to force the user to pass in {html: {nativeCaptions: false}} in their options when instantiating videojs.

There has to be a better way, can anyone give me some advice on where to look or what to do?

As of video.js v5.14, video.js no longer supports native subtitles on anything besides Safari. For Chrome, this means there is *no* subtitle support with mpeg-dash. This will wait for dash.js to load all the text tracks and then add them to video.js if we're using a version that does not attempt to use native subtitles. When the video.js menu is used to change captions, we'll set the corresponding active text track in dash. We *cannot* dynamically add text track cues to video.js beacuse fragmented text tracks are loaded on the fly and there's no event fired when they are loaded. Therefore, we must has dash to natively display captions.

The v5.18.0 release of video.js includes a fix for videojs/video.js#4093 with videojs/video.js#4131. This is neccessary for text tracks to work in Chrome.

- Only set `textTrack.mode` for `captions` and `subtitles. - Don't call the initial `updateActiveDashTextTrack` until after the `textTrack.mode`s have been set.

justinanastos · 2017-05-02T21:44:54Z

Any updates on this @forbesjo ?

squarebracket · 2017-05-04T04:10:07Z

FYI, I downloaded the patch of this PR, and noticed that changing the caption display settings doesn't actually change them.

justinanastos · 2017-05-04T19:57:54Z

@squarebracket

FYI, I downloaded the patch of this PR, and noticed that changing the caption display settings doesn't actually change them.

That's correct. The captions will be displayed natively by dash.js.

forbesjo · 2017-05-08T14:47:40Z

@justinanastos testing this now

squarebracket · 2017-05-08T15:42:42Z

@justinanastos Why would that preclude video.js from setting the caption style? Aren't the dash.js captions being spit out as VTTCues?

gkatsev · 2017-05-08T15:56:20Z

Dash.js renders the captions natively rather than using Video.js's rendering capabilities. That means that the styling that Video.js can provide via the caption settings menu isn't (and potentially can't be) applied.
We probably should disable that menu item in this case.

squarebracket · 2017-05-08T16:23:59Z

Can't videojs just set CSS classes or something though?

forbesjo · 2017-05-08T18:52:02Z

@justinanastos your PR works for me but as has been discussed the captions style cannot be changed since dash.js is doing the rendering and not video.js. Looking into this

forbesjo · 2017-05-08T19:51:29Z

It doesn't look like there's a mechanism to disable the dash.js display so I agree with @gkatsev that the caption settings menu item should be disabled.

squarebracket · 2017-05-09T15:08:52Z

If they're going to be left completely to dash.js, then may I suggest doing something this in Html5DashJS:

    // Attach the source with any protection data
    this.mediaPlayer_.setProtectionData(this.keySystemOptions_);
    this.mediaPlayer_.attachSource(manifestSource);
    // Use the normal text track display to render TTMLs into
    this.mediaPlayer_.attachTTMLRenderingDiv($('.vjs-text-track-display')[0]);

    this.tech_.triggerReady();

The only addition there is calling attachTTMLRenderingDiv.

Their TTML rendering is much better than their VTTCue rendering. For instance, it can do this:

And also supports vertical placement which is important for things like news shows:

squarebracket · 2017-05-09T18:09:26Z

In case anyone was testing media with CEA608 captions, I just had some fixes merged into dash.js. They should now work properly. (For reference, here here and here.)

forbesjo · 2017-05-11T14:16:25Z

@squarebracket I'm not sure if it's a great idea to reuse video.js's text track display for TTML rendering. Instead this script can add a new component div to the player specifically for TTML. That can be done in a separate PR.

@gkatsev what's the recommended way of disabling menu items like the captions settings item? Thinking about this a little more we would still want to display caption settings if there are side-loaded captions (captions not coming from the source manifest). Though when a mix of in-manifest and external captions are present in the player a view might be confused when some captions can be customized while others cannot.

forbesjo · 2017-05-11T18:13:18Z

I think we should leave the captions settings alone, merge this PR in and add other enhancements later

justinanastos · 2017-05-12T19:59:48Z

@squarebracket I believe native cue rendering can do the same thing you're showing, right? There can be multiple cues on the screen at once and their position is defined by the vtt file.

squarebracket · 2017-05-15T15:16:05Z

@justinanastos that is correct, however that information is dropped in the dash.js VTTCue rendering library, but is rendered for the TTML rendering library.

justinanastos force-pushed the use-emulated-text-tracks branch from 7f3cfc8 to d774648 Compare January 17, 2017 22:39

justinanastos force-pushed the use-emulated-text-tracks branch 2 times, most recently from c74b11b to cfe82ae Compare January 18, 2017 20:21

justinanastos changed the title ~~Use emulated text tracks~~ Load text and audio tracks from dash into video.js Jan 18, 2017

justinanastos changed the title ~~Load text and audio tracks from dash into video.js~~ WIP: Load text and audio tracks from dash into video.js Jan 19, 2017

justinanastos force-pushed the use-emulated-text-tracks branch from cfe82ae to be9aa24 Compare January 20, 2017 20:02

justinanastos changed the title ~~WIP: Load text and audio tracks from dash into video.js~~ Load text and audio tracks from dash into video.js Jan 20, 2017

justinanastos changed the title ~~Load text and audio tracks from dash into video.js~~ WIP: Load text and audio tracks from dash into video.js Jan 23, 2017

chemoish reviewed Jan 23, 2017

View reviewed changes

chemoish reviewed Jan 24, 2017

View reviewed changes

justinanastos force-pushed the use-emulated-text-tracks branch 2 times, most recently from 5067eb4 to 10fc47d Compare January 24, 2017 21:53

justinanastos changed the title ~~WIP: Load text and audio tracks from dash into video.js~~ Load text and audio tracks from dash into video.js Jan 24, 2017

justinanastos force-pushed the use-emulated-text-tracks branch from 10fc47d to f0a38b5 Compare January 24, 2017 22:18

gkatsev reviewed Jan 24, 2017

View reviewed changes

gesinger reviewed Jan 26, 2017

View reviewed changes

justinanastos force-pushed the use-emulated-text-tracks branch from f0a38b5 to 82eba76 Compare January 27, 2017 23:07

justinanastos force-pushed the use-emulated-text-tracks branch from 82eba76 to 91ae1b5 Compare January 28, 2017 16:44

justinanastos force-pushed the use-emulated-text-tracks branch from 91ae1b5 to 981cdb2 Compare January 28, 2017 17:50

forbesjo reviewed Jan 30, 2017

View reviewed changes

justinanastos added 4 commits April 14, 2017 16:36

Update README.md to describe nativeCaptions usage requirements

1f9b228

Require [email protected]

b3b2614

The v5.18.0 release of video.js includes a fix for videojs/video.js#4093 with videojs/video.js#4131. This is neccessary for text tracks to work in Chrome.

fixup! Use dash controlled text tracks

f69fab8

- Only set `textTrack.mode` for `captions` and `subtitles. - Don't call the initial `updateActiveDashTextTrack` until after the `textTrack.mode`s have been set.

justinanastos force-pushed the use-emulated-text-tracks branch from e815d1e to f69fab8 Compare April 14, 2017 20:36

justinanastos changed the title ~~WIP: Load text tracks from dash into video.js~~ Load text tracks from dash into video.js Apr 14, 2017

forbesjo added the needs testing label May 8, 2017

forbesjo removed the needs testing label May 8, 2017

imbcmdth added the needs testing label May 9, 2017

forbesjo removed the needs testing label May 11, 2017

forbesjo merged commit 5bf6c6a into videojs:master May 11, 2017

justinanastos deleted the use-emulated-text-tracks branch May 12, 2017 19:58

justinanastos restored the use-emulated-text-tracks branch May 12, 2017 19:58

squarebracket mentioned this pull request Jun 2, 2017

Captions don't render on IE11/Edge #191

Open

squarebracket mentioned this pull request Jan 12, 2018

Attach the VideoJS text track display DOM element to display subtitles #226

Closed

Load text tracks from dash into video.js #135

Load text tracks from dash into video.js #135

Conversation

justinanastos commented Jan 17, 2017 • edited Loading

Todo

chemoish commented Jan 18, 2017 • edited Loading

justinanastos commented Jan 18, 2017

justinanastos commented Jan 19, 2017

justinanastos commented Jan 20, 2017

justinanastos commented Jan 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chemoish Jan 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinanastos commented Jan 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinanastos Jan 28, 2017 • edited Loading

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

Choose a reason for hiding this comment

justinanastos Jan 27, 2017 • edited Loading

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

justinanastos commented Jan 27, 2017

justinanastos commented Jan 28, 2017

justinanastos commented Jan 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinanastos commented Jan 30, 2017 • edited Loading

justinanastos commented May 2, 2017

squarebracket commented May 4, 2017

justinanastos commented May 4, 2017

forbesjo commented May 8, 2017

squarebracket commented May 8, 2017

gkatsev commented May 8, 2017

squarebracket commented May 8, 2017

forbesjo commented May 8, 2017

forbesjo commented May 8, 2017

squarebracket commented May 9, 2017 • edited Loading

squarebracket commented May 9, 2017

forbesjo commented May 11, 2017

forbesjo commented May 11, 2017

justinanastos commented May 12, 2017

squarebracket commented May 15, 2017 • edited Loading

justinanastos commented Jan 17, 2017 •

edited

Loading

chemoish commented Jan 18, 2017 •

edited

Loading

chemoish Jan 24, 2017 •

edited

Loading

justinanastos Jan 28, 2017 •

edited

Loading

justinanastos Jan 27, 2017 •

edited

Loading

justinanastos commented Jan 28, 2017 •

edited

Loading

justinanastos commented Jan 30, 2017 •

edited

Loading

squarebracket commented May 9, 2017 •

edited

Loading

squarebracket commented May 15, 2017 •

edited

Loading