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

feat(CaptionStream): add flag to turn off 708 captions #365

Merged
merged 6 commits into from
Jan 20, 2021
Merged

Conversation

gkatsev
Copy link
Member

@gkatsev gkatsev commented Jan 15, 2021

This options flow through the transmuxer into CaptionStream.

This options flow through the transmuxer into CaptionStream.
gkatsev added a commit to videojs/http-streaming that referenced this pull request Jan 15, 2021
Add cc708 as a VHS option to turn off 708 captions.

This PR depends on videojs/mux.js#365

CaptionStream.prototype.init.call(this);

// cc708 flag, default to true
this.cc708_ = typeof options.cc708 === 'boolean' ? options.cc708 : true;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it might be better to add a bit more description. Maybe something like include708Captions or parse708Captions.

Copy link
Contributor

@gesinger gesinger left a comment

Choose a reason for hiding this comment

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

Probably worth adding a test for the option.


CaptionStream.prototype.init.call(this);

// cc708 flag, default to true
this.cc708_ = typeof options.cc708 === 'boolean' ? options.cc708 : true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be worth changing the name to be a bit more descriptive. Maybe include708Captions or parse708Captions.

this.parse708captions_ = typeof options.parse708captions === 'boolean' ?
options.parse708captions :
true;
this.parse708captions_ = true;
Copy link
Member Author

Choose a reason for hiding this comment

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

this is to demonstrate the test failing

@gkatsev gkatsev merged commit 8a7cdb6 into main Jan 20, 2021
@gkatsev gkatsev deleted the 708-flag branch January 20, 2021 16:48
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.

2 participants