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

CEA-608 Decoder #2731

Merged
merged 33 commits into from
Aug 7, 2020
Merged

Conversation

muhammadharis
Copy link
Contributor

@muhammadharis muhammadharis commented Jul 15, 2020

This pertains to #2648. A CEA-608 decoder that decodes CEA-608 (Line 21) data from User Data Registered by Rec. ITU-T T.35 SEI messages and returns them as cues in Shaka's internal cue format. This allows us to separately maintain our own decoder, and removes our dependency on the Mux.js caption decoder.

Format:
Cues are emitted in Shaka's internal format (lib/text/cue.js). This decoder makes use of nested cues. The top level cue is always a blank cue with no text, and each nested cue inside it contains text, as well as a specific style, or linebreak cues to facilitate line breaks. This also allows for inline style (color, italics, underline) changes.

  • Basic North American, Special North American, and Extended European Charsets supported.

  • Underlines, colors, and Italics supported, set as a property on each nested cue.

  • Text mode not emitted.

  • Positioning and alignment not supported.

removed useless logs

Added color support

fixed up background attribute logic

trim new lines on row ends and styling

fix logic on clearing rollup captions when moving window

hotfixes on decoder and tests

fixed logic and added support for all charsets

minor syntax fix

renaming constants

improve commenting, remove redundant comments

move important hex into constants

broke hex values in unit tests into constants

comment

fixing constant suffixes

cleaned impl for streams/channel
clarification in comments
@muhammadharis muhammadharis changed the title CEA-608 Decoder WIP: CEA-608 Decoder Jul 28, 2020
@muhammadharis muhammadharis changed the title WIP: CEA-608 Decoder CEA-608 Decoder Jul 29, 2020
Copy link
Member

@joeyparrish joeyparrish left a comment

Choose a reason for hiding this comment

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

I need to spend more time reading the decoder class and its tests, but I've been over the rest thoroughly enough, I think. I'll go ahead and send the comments I've got so far. Let me know if you want to discuss any of it in person.

lib/cea/atsc_decoder.js Outdated Show resolved Hide resolved
lib/cea/i_caption_decoder.js Show resolved Hide resolved
lib/media/closed_caption_parser.js Outdated Show resolved Hide resolved
lib/text/text_engine.js Show resolved Hide resolved
test/cea/cea608_memory_unit.js Outdated Show resolved Hide resolved
lib/cea/cea608_memory.js Outdated Show resolved Hide resolved
lib/cea/cea608_memory.js Outdated Show resolved Hide resolved
lib/cea/cea_decoder.js Outdated Show resolved Hide resolved
lib/cea/cea_decoder.js Outdated Show resolved Hide resolved
lib/cea/cea_decoder.js Outdated Show resolved Hide resolved
Copy link
Member

@joeyparrish joeyparrish left a comment

Choose a reason for hiding this comment

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

I need to do one more detailed pass on CeaDecoder, CeaUtils, and the decoder unit tests. But here are a few comments to get you going again.

lib/cea/cea608_data_channel.js Outdated Show resolved Hide resolved
lib/cea/cea608_data_channel.js Outdated Show resolved Hide resolved
lib/cea/cea_decoder.js Outdated Show resolved Hide resolved
lib/cea/cea_decoder.js Show resolved Hide resolved
lib/cea/cea608_data_channel.js Show resolved Hide resolved
lib/media/closed_caption_parser.js Outdated Show resolved Hide resolved
test/cea/cea608_memory_unit.js Outdated Show resolved Hide resolved
test/cea/cea608_memory_unit.js Outdated Show resolved Hide resolved
test/cea/cea608_memory_unit.js Show resolved Hide resolved
test/cea/cea_decoder_unit.js Outdated Show resolved Hide resolved
Copy link
Member

@joeyparrish joeyparrish left a comment

Choose a reason for hiding this comment

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

Okay, this time I've been over absolutely everything. Thanks so much for the revisions and all your hard work on this!

lib/cea/cea608_data_channel.js Show resolved Hide resolved
lib/cea/cea608_data_channel.js Outdated Show resolved Hide resolved
test/cea/cea_decoder_unit.js Outdated Show resolved Hide resolved
test/cea/cea_decoder_unit.js Outdated Show resolved Hide resolved
test/cea/cea_decoder_unit.js Outdated Show resolved Hide resolved
lib/cea/cea_decoder.js Outdated Show resolved Hide resolved
lib/cea/cea_decoder.js Outdated Show resolved Hide resolved
lib/cea/cea_decoder.js Outdated Show resolved Hide resolved
Comment on lines +227 to +228
ccPacket.ccData1 &= 0x7f;
ccPacket.ccData2 &= 0x7f;
Copy link
Member

Choose a reason for hiding this comment

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

Are these packets completely discarded after decode() is complete? If so, this should be fine. But if they were ever re-processed, this would cause half your packets to fail the parity check on a second pass.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, once the decode is complete the packets are all cleared. There is no reprocessing of packets after.

@joeyparrish joeyparrish merged commit 1c00b4c into shaka-project:master Aug 7, 2020
@muhammadharis muhammadharis deleted the mp4-cea-decoder branch August 8, 2020 20:44
@github-actions github-actions bot added the status: archived Archived and locked; will not be updated label Jul 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: archived Archived and locked; will not be updated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants