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

New: Auto-generated captions #816

Merged
merged 1 commit into from
Jul 9, 2018
Merged

Conversation

tonyjin
Copy link
Contributor

@tonyjin tonyjin commented Jul 8, 2018

Note: This depends on video intelligence being enabled for your enterprise. See https://www.box.com/skills for details.

This patch detects if Box Skills transcript cards are loaded and if so, converts them to Shaka Text Cues to be used for captioning. This patch doesn't support switching between native subtitles and auto-generated subtitles. If there are subtitles already within the video, we do not attempt to load auto-generated captions.

@boxcla
Copy link

boxcla commented Jul 8, 2018

Verified that @tonyjin has signed the CLA. Thanks for the pull request!

this.autoCaptionDisplayer.setTextVisibility(true);
this.emit('subtitlechange', __('auto_generated'));

// Valid non-auto-generated index ==> turn specified text track on
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move these comments into their blocks?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like having the comment above the if statement to describe the condition going into the block as well

this.textTracks = this.player.getTextTracks().sort((track1, track2) => track1.id - track2.id);
if (this.textTracks.length > 0) {
this.mediaControls.initSubtitles(
this.textTracks.map((track) => getLanguageName(track.language) || track.language),
getLanguageName(this.options.location.locale.substring(0, 2))
);
return;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the behavior for now is only try to load autogenerated captions if there are no baked in subtitles on the video?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, it's going to be more complicated to support both use cases and I figure the intersection of videos with their own subtitles + those we auto-translate is likely to be small for now.

*
* @return {boolean} Whether or not there are any auto-generated captions
*/
loadAutoGeneratedCaptions() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this need to return a boolean?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was originally going to do auto-generated captions first and then fall back to baked in subtitles and then realized it's probably better the other way. Wasn't sure if we needed to keep this boolean for later, but I can remove it now, add back as needed later.

Note: This depends on video intelligence being enabled for your enterprise. See https://www.box.com/skills for details.

This patch detects if Box Skills transcript cards are loaded and if so, converts them to Shaka Text Cues to be used for captioning. This patch doesn't support switching between native subtitles and auto-generated subtitles. If there are subtitles already within the video, we do not attempt to load auto-generated captions.
@tonyjin tonyjin force-pushed the auto-generated-captions branch from a624d80 to 2036a88 Compare July 9, 2018 19:03
@tonyjin tonyjin merged commit a5cd36f into box:master Jul 9, 2018
@tonyjin tonyjin deleted the auto-generated-captions branch July 9, 2018 20:02
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