Skip to content
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

Fix caption integration #25

Merged
merged 3 commits into from
May 8, 2017
Merged

Fix caption integration #25

merged 3 commits into from
May 8, 2017

Conversation

tmm1
Copy link
Contributor

@tmm1 tmm1 commented May 5, 2017

This is a rebased version of #19 with additional fixes.

cc @ranuser99 @willkelleher

@tmm1
Copy link
Contributor Author

tmm1 commented May 5, 2017

With this change, I now see a CC button in Chrome with an english option listed. Picking the option renders the captions, however when there are multiple lines they appear upside down.

@tmm1
Copy link
Contributor Author

tmm1 commented May 5, 2017

I can't figure out why the cues are showing up in the wrong order..

This hacky change to video.js fixes it:

diff --git a/src/js/tracks/text-track-display.js b/src/js/tracks/text-track-display.js
index 0097dd67..afd3fce3 100644
--- a/src/js/tracks/text-track-display.js
+++ b/src/js/tracks/text-track-display.js
@@ -242,6 +242,7 @@ class TextTrackDisplay extends Component {
     for (let i = 0; i < track.activeCues.length; i++) {
       cues.push(track.activeCues[i]);
     }
+    cues.reverse();
 
     window.WebVTT.processCues(window, cues, this.el_);
 

@mrbar42
Copy link
Collaborator

mrbar42 commented May 7, 2017

Couldn't reproduce so far, neither the missing button nor the reverse order.
@tmm1 on what browsers are you able to reproduce?

@tmm1
Copy link
Contributor Author

tmm1 commented May 7, 2017

@mrbar42 this issue is specific to a/53 captions (also known CEA608/708 or EIA608) which are embedded in live tv video packets.

See @ranuser99's example stream: https://1-fss-fso35.streamhoster.com/lv_ntv/broadcast1/playlist.m3u8

@tmm1
Copy link
Contributor Author

tmm1 commented May 7, 2017

@mrbar42
Copy link
Collaborator

mrbar42 commented May 8, 2017

My testing of this:
VideoJS <= 5.13.1
on Firefox 53 where text tracks has no native support, this PR fixes the problem
on Chrome 58 where text tracks are natively supported, the button does not appear.

VideoJS > 5.13.1
Both Firefox and Chrome doesn't show the button like described in the issue.

it looks like the problem is that video.js is unaware of the captions.
when setting videoTag.controls = true the captions button is shown on the native controls.

seems like another fix is needed for newer videojs versions (not in the scope of this PR)

@mrbar42 mrbar42 merged commit 664847d into Peer5:master May 8, 2017
@mrbar42 mrbar42 mentioned this pull request May 8, 2017
@willkelleher
Copy link
Contributor

Thanks @tmm1 !

@tmm1 tmm1 deleted the caption-fix-rebase branch May 8, 2017 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants