[PLAYER-4216] (PR 3 of 3) MainHtml5 closed captions refactor #333

jpfranco · 2018-09-07T00:59:47Z

Rewrote most of the closed captions implementation from scratch. The main issues that were addressed by these changes are the following:

Allowing captions to be switched via the native fullscreen UI on iOS
Allowing captions to be turned off via the native fullscreen UI on iOS
Keeping the player's UI in sync with the native fullscreen UI on iOS

These changes will also fix the following:

In-manifest/In-stream captions for VOD will now be available on supported platforms
Metadata text tracks will no longer show up in the UI
Text tracks will no longer be removed and re-added on each call to setClosedCaptions
VTT files will no longer be requested on each call to setClosedCaptions and when pausing and then resuming
More robust handling of multi closed captions for live

Note: I will create a separate PR for new unit tests, for now I only fixed the existing ones.

…ollision

jpfranco · 2018-09-07T01:02:19Z

.babelrc

+      "@babel/preset-env", {
+        "targets": {
+          "ie": 11


By default Babel will transpile es6 syntax but it will not automatically add polyfills for APIs such as Array.prototype.filter or Object.assign. Eventually we should look for a way to load all polyfills in the core so that they are available for all plugins without including them on each script

jpfranco · 2018-09-07T01:02:59Z

src/main/js/constants/constants.js

@@ -0,0 +1,21 @@
+
+const CONSTANTS = {


Deliberately left this out of html5-common since only MainHtml5 should be concerned with these values

jpfranco · 2018-09-07T01:05:03Z

tests/unit/main_html5/wrapper-api-tests.js

    expect(element.textTracks[0].mode).to.eql(OO.CONSTANTS.CLOSED_CAPTIONS.SHOWING);
  });

-  it('should replace French text tracks by English text tracks for iOS versions < 10 ', function(){


This workaround was removed so these tests are no longer relevant

jpfranco · 2018-09-07T01:05:20Z

tests/unit/main_html5/wrapper-event-tests.js

@@ -607,60 +598,6 @@ describe('main_html5 wrapper tests', function () {
    expect(vtc.notifyParameters).to.eql([vtc.interface.EVENTS.CLOSED_CAPTION_CUE_CHANGED, ""]);
  });

-  it('should notify CLOSED_CAPTION_CUE_CHANGED on \'timeupdate\' event in Firefox', function(){


This Firefox workaround was removed so these tests are no longer relevant

aeng7 · 2018-09-07T21:05:50Z

src/main/js/text_track/text_track_map.js

+   * @param {Object} metadata An object containing the properties to be merged with the existing object
+   * @return {Object} The updated metadata entry or undefined if there were no matches
+   */
+  tryUpdateEntry(searchOptions, metadata = {}) {


Does this update the entry in this.textTracks? If not, should this function be renamed?

Yes, the matched object reference from this.textTracks will be updated.

aeng7 · 2018-09-07T21:11:45Z

tests/unit/main_html5/wrapper-api-tests.js

    wrapper.setClosedCaptions(language, closedCaptions, params);
+    element.textTracks.onaddtrack();


Do we have a listener for onaddtrack in our video plugin? I'm just curious how this is used for the unit tests.

This is set up on the loadedmetadata event

aeng7 · 2018-09-07T21:14:59Z

src/main/js/text_track/text_track_helper.js

+   * @param {TextTrackMap} textTrackMap A TextTrackMap that contains metadata for all of the video's TextTrack objects.
+   * @return {TextTrack} The first TextTrack object that matches the given key or undefined if there are no matches.
+   */
+  findTrackByKey(languageOrId, textTrackMap = new TextTrackMap()) {


Is it possible for two text tracks to have the same exact language key (like English for hard of hearing vs English subtitles)? If so, should this return all the tracks that have that key? Also, who is using this function. I tried searching this PR for findTrackByKey and coulnd't find a usage.

Edit: Nevermind, found out that the main_html5 source was minimized.

Yes, good catch. I will have to tweak this in order to account for that scenario. This is called by executeSetClosedCaptions btw

aeng7 · 2018-09-07T21:22:21Z

src/main/js/text_track/text_track_map.js

+   * @return {Boolean} True if all tracks have 'disabled' mode, false otherwise
+   */
+  areAllDisabled() {
+    const allDisabled = this.textTracks.reduce((result, trackMetadata) =>


Would it be simpler to iterate through the textTracks and return once a non-disabled text track is found? I think .reduce will keep iterating through the textTracks even after a non-disabled text track is found. Though this is minor all things considered since textTracks won't be large.

Given the small size of the data set I think reduce is the way to do this with the least amount of code. I can change this if you want, but I like this one-liner

Nope, I'm good. This is fine

aeng7 · 2018-09-07T21:23:19Z

src/main/js/text_track/text_track_map.js

+      let isFound = true;
+
+      for (let property in searchOptions) {
+        if (searchOptions[property] !== currentTrack[property]) {


Do we need to do any hasOwnProperty checks?

Yes, will add this but I don't think it's strictly necessary. The class controls the object creation and if someone passes a searchOptions object that inherits from something unexpected then that in itself would be a bigger issue.

aeng7 · 2018-09-07T21:40:15Z

src/main/js/main_html5.js

+          id: textTrack.label
+        });
+
+        if (trackMetadata) {


What happens if the trackMetadata isn't found?

This would just mean that either all external tracks have already been mapped or the addtrack event was triggered by an in-stream track addition. This call is meant to be idempotent so that it can be executed at any point in time

aeng7 · 2018-09-11T21:55:09Z

LGTM

dallen42

LGTM

jpfranco81 added 19 commits August 17, 2018 14:02

[PLAYER-4216] Closed captions refactor WIP

f6e03a6

[PLAYER-4216] Closed captions refactor WIP

fa9aa4d

[PLAYER-4216] Closed captions refactor WIP

8025edd

[PLAYER-4216] Closed captions refactor WIP

5bf8d89

[PLAYER-4216] Allowing native UI to switch closed captions

d2062e1

[PLAYER-4216] Giving priority to external CC's when there is a lang c…

7702250

…ollision

[PLAYER-4216] Switching to a single text track map

4a7e286

[PLAYER-4216] Now reacting to captions being turned off from native UI

74fd452

[PLAYER-4216] Workaround for Edge not propagating DOM ids to TextTracks

1abea50

[PLAYER-4216] CC refactor improvements

e0969d5

[PLAYER-4216] Cleanup WIP

b073e98

[PLAYER-4216] Cleanup WIP

973adf0

[PLAYER-4216] Adding babel polyfill for IE11

71c2308

[PLAYER-4216] Tweaked set crossorigin logic

3893bc6

[PLAYER-4216] Removed unnecessary removetrack handler

c8550e4

[PLAYER-4216] Firefox and Safari fixes

d934c06

[PLAYER-4216] Fixed unit tests

8f3796d

[PLAYER-4216] Console log messages cleanup

151b400

[PLAYER-4216] Edge case fixes

2bc3be8

jpfranco requested review from dallen42, pilievOoyala and aeng7 September 7, 2018 00:59

jpfranco commented Sep 7, 2018

View reviewed changes

aeng7 reviewed Sep 7, 2018

View reviewed changes

jpfranco81 and others added 6 commits September 10, 2018 12:26

[PLAYER-4216] CR updates and fixes for Edge

e70b5c4

[PLAYER-4216] More Safari fixes

4347480

update html5-common

9ff36df

[PLAYER-4216] More Safari fixes

841f272

[PLAYER-4216] Clearing stuck cues on language change

3d45734

Merge branch 'master' into PLAYER-4216

1935286

dallen42 approved these changes Sep 12, 2018

View reviewed changes

jpfranco merged commit c891805 into master Sep 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PLAYER-4216] (PR 3 of 3) MainHtml5 closed captions refactor #333

[PLAYER-4216] (PR 3 of 3) MainHtml5 closed captions refactor #333

jpfranco commented Sep 7, 2018 •

edited

Loading

jpfranco Sep 7, 2018

jpfranco Sep 7, 2018

jpfranco Sep 7, 2018

jpfranco Sep 7, 2018

aeng7 Sep 7, 2018

jpfranco Sep 7, 2018

aeng7 Sep 7, 2018

jpfranco Sep 7, 2018

aeng7 Sep 7, 2018 •

edited

Loading

jpfranco Sep 7, 2018

aeng7 Sep 7, 2018

jpfranco Sep 7, 2018 •

edited

Loading

aeng7 Sep 7, 2018

aeng7 Sep 7, 2018

jpfranco Sep 7, 2018

aeng7 Sep 7, 2018

jpfranco Sep 7, 2018

aeng7 commented Sep 11, 2018

dallen42 left a comment

		wrapper.setClosedCaptions(language, closedCaptions, params);
		element.textTracks.onaddtrack();

[PLAYER-4216] (PR 3 of 3) MainHtml5 closed captions refactor #333

[PLAYER-4216] (PR 3 of 3) MainHtml5 closed captions refactor #333

Conversation

jpfranco commented Sep 7, 2018 • 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

aeng7 Sep 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpfranco Sep 7, 2018 • 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

aeng7 commented Sep 11, 2018

dallen42 left a comment

Choose a reason for hiding this comment

jpfranco commented Sep 7, 2018 •

edited

Loading

aeng7 Sep 7, 2018 •

edited

Loading

jpfranco Sep 7, 2018 •

edited

Loading