-
Notifications
You must be signed in to change notification settings - Fork 116
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
Conversation
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
src/lib/viewers/media/DashViewer.js
Outdated
* | ||
* @return {boolean} Whether or not there are any auto-generated captions | ||
*/ | ||
loadAutoGeneratedCaptions() { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
a624d80
to
2036a88
Compare
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.