feat: caption refactor + support cvaa & default audio/text tracks #118

OrenMe · 2017-09-19T13:58:28Z

Description of the Changes

player handles synthetic VTT cue and displays them with a derived HTML Dom element that is created from this cue
currently player support cues from native video element which are created via the adapters. Adapters themselves parse the VTT from whatever source(VTT segmented file, VTT in header, DFXP etc) and use the native textTrack API to add them to video and sync them to timeline.
HTML5 layer gets the cuechange event and creates a synthetic VTT cue to normalize the VTTCue and TextTrackCue models to align with VTTCue one(thanks Edge and IE11) so this means we don't support VTT attributes fully in those browsers yet
expose API to set text style and text cue display settings

CheckLists

changes have been done against master branch, and PR does not conflict
new unit / functional tests have been added (whenever applicable)
test are passing in local environment
Travis tests are passing (or test results are not worse than on master branch :))
Docs have been updated

contains models for VTT cue and text track style settings and egine to port VTT cue model to DOM element which is a port of VTT.js project

allow application to create test style object to apply on player via API

* player handles synthetic VTT cue and displays them with a derived HTML Dom element that is created from this cue. * currently support cues from native video element which are created via the adapters. Adapters themselves parse the VTT from whatever source(VTT segmented file, VTT in header, DFXP etc) and use the native textTrack API to add them to video and sync them to timeline. html5 layer gets the cuechange event and creates a synthetic VTT cue to normalize the VTTCue and TextTrackCue models to VTTCue one(thanks Edge and IE11) so this means we don't support VTT attributes fully in those browsers yet * expose API to set text style and text cue display settings

OrenMe · 2017-09-19T14:01:01Z

Still needs to:

handle default text tracks from manifest
handle auto textLanguage option from config
external VTT files
support VTT attributes from VTT file on edge and IE11
use default texttrack API on iOS for native playback
tests
sync with UI elements, e.g. the control bar and possibly ads - need to change the text position when they appear

… into captionRefactor

use following options in descending importance to determine default text/audio language: 1. `playback.textLanguage` and `playback.audioLanguage` 2. default track from manifest

For browser that don't support native VTTCue create a VTTCue from the TextTrackCue. This meand that for Edge and IE11 we won't have full support for all VTTCue attributes.

If special config "auto" is set in config the player will try to deterimne the default language according to OS locale. language string are ISO-693-1

this can be used whenever, but will be used for supporting iOS native text tracks for example where we can't create our own div in the native player for captions display

OrenMe · 2017-09-27T14:41:58Z

@dan-ziv changing this to a controller is indeed the right way but it will require some changes that we will cover in other PR, that will handle adding some life cycle methods we currently lack.

… into captionRefactor

yairans · 2017-09-27T20:21:33Z

src/engines/html5/html5.js

+   * @type {Array<boolean>}
+   * @private
+   */
+  _showTextTrackfirstTime: { [number]: boolean } = {};


this is an object or map. no array as typed in jsdoc

also, you have to clean this map in destroy

No need to clean it, after a text track is shown for first time it is reused, and no need for this hack

change media?

yairans · 2017-09-27T20:32:35Z

src/engines/html5/html5.js

+
+  /**
+   * Get currently selected text track
+   * @returns {*} - returns the active text track element if available


jsdoc supports {?TextTrack} http://usejsdoc.org/tags-type.html

yairans · 2017-09-27T20:33:46Z

src/engines/html5/html5.js

+    const textTracks = this._el.textTracks;
+    for (let track in textTracks) {
+      if (textTracks.hasOwnProperty(track)) {
+        if (textTracks[parseInt(track)] && textTracks[parseInt(track)].mode !== "disabled") {


.mode !== "disabled" could be "hidden" also. i guess you rely on native adapter which makes the selected to 'hidden'. but track could be 'hidden' also from the browser and then is unselected.
note the the 'not showing' mode could be browser depended. e.g in safari by default is "disabled" while in chrome is "hidden".
maybe you have to change all 'hidden' to 'disabled' in advance.
also we have to change dash and hls adapters to hide track by 'disabled'. currently is 'hidden'.
best to implement hideTextTrack in baseAdapter

better to let track = textTracks[parseInt(track)] once instead of calculate it 3 times.

Regarding the mode, I added in the _setDefaultTracks a call to hideTextTrack so it will take care of setting them to disabled.

yairans · 2017-09-27T21:10:20Z

src/engines/html5/html5.js

+      let textTrackEl: TextTrack = this._getSelectedTextTrack();
+      if (textTrackEl) {
+        textTrackEl.oncuechange = null;
+      }


just call this. _removeCueChangeListener()

yairans · 2017-09-27T21:15:50Z

src/engines/html5/html5.js

@@ -194,13 +212,85 @@ export default class Html5 extends FakeEventTarget implements IEngine {

  /**
   * Select a new text track.
-   * @param {TextTrack} textTrack - The text track object to set.
+   * @param {PKTextTrack} textTrack - The text track object to set.


The playkit text track object to set.

yairans · 2017-09-27T21:16:22Z

src/engines/html5/html5.js

+
+  /**
+   * Add cuechange listener to active textTrack
+   * @param {PKTextTrack} textTrack - player text track


playkit text track

yairans · 2017-09-27T21:16:59Z

src/engines/html5/html5.js

+
+  /**
+   * Remove cuechange listener to active textTrack
+   * @param {PKTextTrack} textTrack - player text track


playkit text track

ooopps - I don't even use it so no ned to define it

yairans · 2017-09-27T21:20:20Z

src/engines/html5/media-source/adapters/native-adapter.js

@@ -440,15 +440,16 @@ export default class NativeAdapter extends BaseMediaSourceAdapter {
  /**
   * Select a text track
   * @function selectTextTrack
-   * @param {TextTrack} textTrack - the track to select
+   * @param {PKTextTrack} textTrack - the track to select


the playkit text track to select

yairans · 2017-09-27T21:44:09Z

remove dist

yairans · 2017-09-27T21:57:50Z

src/player.js

+ * @type {string}
+ * @const
+ */
+const SUBTITLES_STYLE_NAME: string = 'playkit-subtitles-style';


SUBTITLES_STYLE_ID ?

yairans · 2017-09-27T21:58:52Z

src/player.js

+  _playerId: string;
+  /**
+   * The player last updated text cues list
+   * @type {any}


{Array<any>}

yairans · 2017-09-27T22:00:22Z

src/player.js

@@ -227,6 +267,90 @@ export default class Player extends FakeEventTarget {
  }

  /**
+   * Sets style attributes for text tracks.
+   * @param {Object} style - text styling settings


{TextStyle}

yairans · 2017-09-27T22:03:24Z

src/player.js

+    }
+
+    if (this._config.playback.useNativeTextTrack) {
+      sheet.insertRule(`video.${ENGINE_CLASS_NAME}::cue { ${style.toCSS()} }`, 0);


is it valid in ie11?

check if this._config.playback and style is exists and has toCSS() method

this._config. playback will always exist cause we set it on constructor.

it's not valid on ie11, but we use it only on iOS and the actual setup is done in application level that should check when it wants to use it and then just set useNativeTextTrack: true, otherwise use the DIV with CSS styling

yairans · 2017-09-27T22:23:28Z

src/player.js

+   * @type {any}
+   * @private
+   */
+  _activeTextCues: Array<any> = [];


should clean on destroy

not only destroy, more like on change media 👍

yairans · 2017-09-27T22:48:52Z

src/player.js

+  setTextDisplaySettings(settings: Object): void {
+    this._textDisplaySettings = settings;
+    this._updateCueDisplaySettings();
+    for (let i = 0; i < this._activeTextCues.length; i++) {


better to check _activeTextCues existence. i know you init as [] in html5 _onCueChange but still....

I think that if we init it and enforce FLOW typings then it is safe

yairans · 2017-09-27T22:51:11Z

src/player.js

+   * @type {HTMLDivElement}
+   * @private
+   */
+  _textDisplayEl: HTMLDivElement;


should clean on destroy ? or can rely vid element destroy?

No need, we clean the content on reset and this is just the placeholder

yairans · 2017-09-27T22:56:05Z

src/player.js

+
+  /**
+   * handle text cue change
+   * @param {Event} event - the cue change event payload


{FakeEvent}

yairans · 2017-09-27T22:56:42Z

src/player.js

+   * @private
+   * @returns {void}
+   */
+  _updateCueDisplaySettings(){


yairans · 2017-09-27T22:58:19Z

src/player.js

+   * @private
+   * @returns {void}
+   */
+  _updateTextDisplay(cues: any): void {


should be Array<Cue>. also in jsdoc

yairans · 2017-09-28T12:23:30Z

src/player.js

+      Utils.Dom.addClassName(this._textDisplayEl, SUBTITLES_CLASS_NAME);
+      Utils.Dom.appendChild(this._el, this._textDisplayEl);
+    }
+    processCues(window, cues, this._textDisplayEl);


maybe add try/catch around processCues

why? it's a managed method, the processCues has it's own protections internally

OrenMe · 2017-09-29T11:43:02Z

@dan-ziv @yairans please review following review fixes

approved

OrenMe · 2017-10-02T02:24:43Z

src/engines/html5/html5.js

@@ -282,7 +288,7 @@ export default class Html5 extends FakeEventTarget implements IEngine {
   * @returns {?TextTrack} - returns the active text track element if available
   * @private
   */
-  _getSelectedTextTrack(): ?TextTrack {
+  _getSelectedTextTrackElement(): ?TextTrack {


It's not an element, but a DOM object.
if it was of the format then I would agree.

<video> <track>...</track> </video>

And even for the above, you can still get the element as an object via video.textTracks and via DOM API selector.

textTrackEl was a convention copy-pasted from the previous implementation so I just kept the naming.

OrenMe · 2017-10-02T05:47:21Z

added tests

OrenMe added 3 commits September 19, 2017 14:03

add vtt cue model and style model

e112642

contains models for VTT cue and text track style settings and egine to port VTT cue model to DOM element which is a port of VTT.js project

expose text style API

61db637

allow application to create test style object to apply on player via API

OrenMe added Priority: Medium Type: Feature labels Sep 19, 2017

OrenMe self-assigned this Sep 19, 2017

OrenMe requested review from yairans, dan-ziv and odedhutzler September 19, 2017 13:58

OrenMe added 15 commits September 19, 2017 17:01

Merge branch 'master' into captionRefactor

9397b2e

refactor VTTCue and add flow annotations

3d197e8

Merge branch 'captionRefactor' of https://github.com/kaltura/playkit-js…

540520f

… into captionRefactor

handle default track handling

c6f0793

use following options in descending importance to determine default text/audio language: 1. `playback.textLanguage` and `playback.audioLanguage` 2. default track from manifest

trigger TEXT_TRACK_CHANGED on hide track

b49edf4

comments and code cleanup

21e19cb

noramlize TextTrackCue to VTTCue

4e20d9c

For browser that don't support native VTTCue create a VTTCue from the TextTrackCue. This meand that for Edge and IE11 we won't have full support for all VTTCue attributes.

fix call setDefaultTracks

2f48933

Merge branch 'master' into captionRefactor

c15ee85

fix merge issue

ea202b0

fix method return and typings

63bdfa3

fix tests

744b7dc

fix tests

db6846a

fix explicit type checking

27bc749

fix equality check

878cecc

OrenMe mentioned this pull request Sep 27, 2017

feat: use off caption from playkit kaltura/playkit-js-ui#60

Merged

5 tasks

OrenMe added 4 commits September 27, 2017 10:38

add off caption track

d8479a7

auto choose text language accoring to locale

78ee186

If special config "auto" is set in config the player will try to deterimne the default language according to OS locale. language string are ISO-693-1

support using native text track via config

3db0625

this can be used whenever, but will be used for supporting iOS native text tracks for example where we can't create our own div in the native player for captions display

add default useNativeTextTrack config

1df03f0

OrenMe added 2 commits September 27, 2017 17:36

CR fix

f5d6dc0

CR fix

6224c90

OrenMe added 2 commits September 27, 2017 17:43

Merge branch 'captionRefactor' of https://github.com/kaltura/playkit-js…

c9fe6a4

… into captionRefactor

fix tests

e7edaac

yairans suggested changes Sep 27, 2017

View reviewed changes

yairans previously requested changes Sep 28, 2017

View reviewed changes

PR fixes

b27282a

OrenMe and others added 3 commits September 29, 2017 14:43

Merge branch 'master' into captionRefactor

65b31c6

Pr fixes

6a631da

Add IE11 protection

f9f0eff

Fix eslint

2979630

OrenMe commented Oct 2, 2017

View reviewed changes

OrenMe added 3 commits October 1, 2017 22:46

change textStyle to getter/setter

f1caf0c

add clone method to text-style

8aa0d5f

add tests to new APIs

d50613b

Dan Ziv added 2 commits October 2, 2017 11:53

Apply lowercase 'should' convention on tests

70acb41

Fix eslint in test

ed28f36

dan-ziv changed the title ~~feat: caption refactor~~ feat: caption refactor suppot cvaa & default audio/text tracks) Oct 2, 2017

dan-ziv changed the title ~~feat: caption refactor suppot cvaa & default audio/text tracks)~~ feat: caption refactor support cvaa & default audio/text tracks Oct 2, 2017

dan-ziv changed the title ~~feat: caption refactor support cvaa & default audio/text tracks~~ feat: caption refactor + support cvaa & default audio/text tracks Oct 2, 2017

dan-ziv approved these changes Oct 2, 2017

View reviewed changes

dan-ziv merged commit 187cf78 into master Oct 2, 2017

dan-ziv deleted the captionRefactor branch October 2, 2017 09:16

feat: caption refactor + support cvaa & default audio/text tracks #118

feat: caption refactor + support cvaa & default audio/text tracks #118

Conversation

OrenMe commented Sep 19, 2017 • edited Loading

Description of the Changes

CheckLists

OrenMe commented Sep 19, 2017 • edited Loading

OrenMe commented Sep 27, 2017

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

yairans Sep 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

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

yairans commented Sep 27, 2017

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

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

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

OrenMe commented Sep 29, 2017

OrenMe Oct 2, 2017 • edited Loading

Choose a reason for hiding this comment

OrenMe commented Oct 2, 2017

OrenMe commented Sep 19, 2017 •

edited

Loading

OrenMe commented Sep 19, 2017 •

edited

Loading

yairans Sep 27, 2017 •

edited

Loading

OrenMe Oct 2, 2017 •

edited

Loading