Skip to content

feat: Cea708 caption decoder#346

Merged
gkatsev merged 8 commits into
videojs:mainfrom
iamstickfigure:cea708-decoder
Dec 1, 2020
Merged

feat: Cea708 caption decoder#346
gkatsev merged 8 commits into
videojs:mainfrom
iamstickfigure:cea708-decoder

Conversation

@iamstickfigure

@iamstickfigure iamstickfigure commented Jul 8, 2020

Copy link
Copy Markdown
Contributor

This adds another caption decoder for 708 captions along side the existing 608 caption decoder. It isn't fully featured, but it has almost as many features as the existing 608 decoder.

Like the 608 decoder, this does not support positioning.
Unlike the 608 decoder, this does not support italics and underline.

Added functions for all commands, even if they aren't fully implemented.
Added doc comments.
@gkatsev

gkatsev commented Jul 8, 2020

Copy link
Copy Markdown
Member

Thank you so much! We're swamped trying to get VHS 2.1 out, but we'll review as soon as we can!

@gkatsev

gkatsev commented Jul 8, 2020

Copy link
Copy Markdown
Member

Also, would be awesome to get some tests added.

@iamstickfigure

Copy link
Copy Markdown
Contributor Author

Regarding tests, I actually have a test harness set up locally that I've been using while iterating on my code. However, it relies on the existence of reading in large TS files. I took a look at the 608 decoder tests, and those tests do not read in source video files, so I'm not entirely sure how I would adapt my test harness in a way that avoids having large TS files in the repo.

Looking again though, I see a file already present called mixed-608-708-captions.js. I suppose I could use that assuming it actually has 708 captions in the data. I'll take a closer look at that.

@iamstickfigure

iamstickfigure commented Jul 30, 2020

Copy link
Copy Markdown
Contributor Author

Also, would be awesome to get some tests added.

Tests added

@gkatsev

gkatsev commented Oct 7, 2020

Copy link
Copy Markdown
Member

Hey, just wanted to post an update since it's been a while. We've been swamped but this is on our plan to look into really soon.

@iamstickfigure

iamstickfigure commented Oct 8, 2020

Copy link
Copy Markdown
Contributor Author

Awesome. No problem. It gave us some extra time to do a lot more testing and discover/fix a few bugs. I added some improvements and implemented a bit more of the spec like roll-up captions.

@gkatsev

gkatsev commented Oct 8, 2020

Copy link
Copy Markdown
Member

Speaking of roll-up captions, I just finished implementing region support in vtt.js videojs/vtt.js#50, which was added to webvtt specifically for 608/708 rollup support.

@iamstickfigure

Copy link
Copy Markdown
Contributor Author

Awesome. For now, my roll-up implementation just involves replacing the first row with the second row, and repeat. So it's like a jumpy version of roll-up captions, but it did the trick. It might be worth converting over to using vtt regions at some point in a separate PR. I assume this enables some sort of animated scrolling between rows?

@gkatsev

gkatsev commented Oct 10, 2020

Copy link
Copy Markdown
Member

yup, exactly. It transitions each cue up when they are in the same region.

@gkatsev gkatsev added this to the 5.7 milestone Nov 17, 2020
@gkatsev
gkatsev merged commit 309f8d8 into videojs:main Dec 1, 2020
@gkatsev

gkatsev commented Dec 1, 2020

Copy link
Copy Markdown
Member

Thanks @iamstickfigure. We'll get it into a release soon!

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.

5 participants