CEA-708 Decoder (#2648) #2807

muhammadharis · 2020-08-18T21:00:40Z

This pertains to #2648 (although this is a new feature, not a replacement) and #1404. A CEA-708 decoder that follows the CEA-708-E standard, decodes closed caption data from User Data Registered by Rec. ITU-T T.35 SEI messages, and returns them as cues in Shaka's internal cue format. Furthermore, this pull request fixes and cements some of the logic surrounding CEA-608 and CEA-708 tag parsing on the Dash Manifest Parser.

Format:
Similar to the CEA-608 decoder, 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.

Details:

ASCII (G0), Latin-1 (G1), and CEA-708 specific charsets (G2 and G3) all supported.
Underlines, colors, and Italics supported, set as a property on each nested cue.
Positioning of text is supported. (Exception: In CEA-708 the default positioning is left, in this decoder it is centered.)
Positioning of windows not supported, but relevant fields that could be used to support this are extracted and left as a TODO.

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

made colors array more intuitive

…cea708-decoder

lib/cea/cea_decoder.js

joeyparrish · 2020-08-27T16:55:43Z

lib/cea/cea_decoder.js

+    const stableComparator = (ccPacket1, ccPacket2) => {
+      const diff = ccPacket1.pts - ccPacket2.pts;
+      const isEqual = diff === 0;
+      return isEqual ? ccPacket1.order - ccPacket2.order : diff;
+    };


I think this could condense down to:

const stableComparator = (p1, p2) => (p1.pts - p2.pts) || (p1.order - p2.order);

For me, the compactness makes it easy for me to see at a glance what it's doing (falling back from one value to another). But I'm fine with you leaving it as-is if you feel the long form is more readable.

joeyparrish · 2020-08-27T17:01:32Z

shaka-player.uncompiled.js

 goog.require('shaka.cea.CeaDecoder');
 goog.require('shaka.cea.Cea608DataChannel');
 goog.require('shaka.cea.Cea608Memory');
+goog.require('shaka.cea.Cea708Window');
 goog.require('shaka.cea.Mp4CeaParser');


The classes in this list should only be the ones which are not goog.required anywhere else. If they are required by another class in this list, they don't need to be listed here.

So the only things that appear in this list are plugins which register themselves and top-level classes used by applications directly.

You may be missing some goog.require() statements in lib/cea/, but I think everything in this namespace is depended on directly or indirectly by ClosedCaptionParser.

test/dash/dash_parser_manifest_unit.js

joeyparrish · 2020-08-27T17:19:08Z

ui/text_selection.js

    // will set a text stream if it isn't already set. Consequently, reversing
    // the order of these calls makes two languages display simultaneously
    // if captions are turned off -> on in a different language.
-    this.player.selectTextLanguage(track.language, track.roles[0]);
+    this.player.selectTextTrack(track);


I went back through the history of this file to see how/when this came to be, and I am certain now that it was a mistake. Thanks for fixing it!

joeyparrish · 2020-08-27T18:24:40Z

lib/cea/cea708_window.js

+    const topLevelCue = new shaka.text.Cue(
+        this.startTime_, endTime, /* payload= */ '');
+
+    if (this.justification_ === 0) {


Use the enum here, please.

joeyparrish · 2020-08-27T18:25:16Z

lib/cea/cea708_service.js

+ * @property {!number} type
+ *   Type of the byte. Either 2 or 3, DTVCC Packet Data or a DTVCC Packet Start.
+ * @property {!number} byte The byte containing data relevant to the packet.
+ * @property {(!number)} order


style nit: remove these extra parens

lib/cea/cea708_service.js

joeyparrish · 2020-08-27T18:38:17Z

lib/cea/dtvcc_packet_builder.js

+// Builds packets based on Figure 5 CCP State Table in 5.2 of CEA-708-E.
+// Initially, there is no packet. When a DTVCC_PACKET_START payload is received,
+// a packet begins construction. The packet is considered parsed once all bytes
+// indicated in the header are read, and ignored if a new packet is started
+// before the current packet being processed is parsed.


I'm having trouble with your terminology here. Let's discuss in our scheduled meeting today and see if we can work it out.

joeyparrish

Thank you so much!

joeyparrish · 2020-09-08T16:46:50Z

lib/cea/cea708_service.js

+      controlCode = (controlCode << 16) | extendedControlCodeBlock.value;
+    }
+
+    // Control codes are in 1 of 4 groups: CL, CR, GL, GR.


I'm having a hard time connecting this comment with the handlers below, which seem to be C0-C4 and G0-G4. None of them ends with L or R.

joeyparrish · 2020-09-08T16:54:43Z

lib/cea/cea708_service.js

+   * @private
+   */
+  handleC2_(dtvccPacket, controlCode) {
+    // There are currently no commands on the C2 table as of CEA-708-E, but if


When this code is read years from now, it would be helpful to have a rough date instead of "currently", so the reader knows if it's worth double-checking for changes in specs. I tend to say things like "as of September 2020..."

joeyparrish · 2020-09-08T16:55:39Z

lib/cea/cea708_service.js

+   * @private
+   */
+  handleC3_(dtvccPacket, controlCode) {
+    // There are currently no commands on the C3 table as of CEA-708-E, but if


joeyparrish · 2020-09-08T16:56:49Z

lib/cea/cea708_service.js

+  }
+
+  /**
+   * Yields each non-null window specified in the 8-bit bitmap.


joeyparrish · 2020-09-08T17:07:07Z

lib/cea/cea708_service.js

+   */
+  setPenAttributes_(dtvccPacket) {
+    // Following 2 bytes take the following form:
+    // b1 = |TXT_TAG|OFS|PSZ| , b2 = |I|U|EDTYP|FNTAG|


Though I assume that the shortened names are from the spec, they are not self-explanatory names. Can you explain them in comments?

Also, you don't explain the size of those fields in bits, so it's impossible to compare this comment with the bitwise math below.

A diagram would help (I'm just making up these sizes):

// | 7654| 32| 10| |7|6| 543| 210| // |TXT_TAG|OFS|PSZ| |I|U|EDTYP|FNTAG|

Or maybe just list them with sizes and descriptions:

// TXT_TAG (4 bits): A tag for text, I guess? // OFS (2 bits): family-owned contract furniture manufacturer (ofs.com) // PSZ (2 bits): Pez dispenser // I (1 bit): Italic // U (1 bit): Underline // EDTYP (3 bits): Equine Dystopia // FNTAG (3 bits): Fondue Tag

Maybe also link to https://en.wikipedia.org/wiki/CEA-708#SetPenAttributes_(0x90_+_2_bytes)

:-)

joeyparrish · 2020-09-08T19:56:11Z

test/cea/cea708_window_unit.js

+    }
+    const topLevelCue = new shaka.text.Cue(startTime, endTime, '');
+    topLevelCue.nestedCues = [
+      CeaUtils.createDefaultCue(startTime, endTime, text1+text2),


I'm a little uncertain about this. If the pen starts at column 0, writes four characters, then moves to column 6 and writes more, shouldn't there be 2 spaces between them?

Good catch, I didn't even consider that a null cell should be handled as a space, but that makes perfect sense. I've fixed it. 👍

joeyparrish · 2020-09-08T19:57:10Z

test/cea/cea708_window_unit.js

+
+  it('cuts off text that exceeds the column size on a given row', () => {
+    const text = '0123456789012345678901234567890123'; // this text is 34 chars.
+    const trimmedText = '01234567890123456789012345678901';


For clarity, should this maybe be written as text.substr(0, 32)?

test/dash/dash_parser_manifest_unit.js

joeyparrish · 2020-09-08T20:07:13Z

test/cea/dtvcc_packet_builder_unit.js

+      order: 0,
+    });
+
+    const dtvccStartByte = 0b00000001; // 1 * 2 -1 = 1 data bytes should follow.


The previous tests used hex notation for this. Why switch to binary?

joeyparrish · 2020-09-08T20:08:17Z

test/cea/dtvcc_packet_unit.js

+  beforeEach(() => {
+
+  });


I think you could drop this. :-)

oops, I should have been more attentive while copy/pasting 😳

joeyparrish · 2020-09-09T20:18:47Z

lib/cea/cea708_service.js

@@ -276,7 +277,7 @@ shaka.cea.Cea708Service = class {
   * @private
   */
  handleC3_(dtvccPacket, controlCode) {
-    // There are currently no commands on the C3 table as of CEA-708-E, but if
+    // As of the CEA-708-E spec there are no commands on the C2 table, but if


Did this change to C2 by mistake? The method is still called handleC3_.

joeyparrish

Woohoo!

shaka-bot · 2020-09-09T21:17:23Z

Test Failure:

Generating Closure dependencies...
Linting JavaScript...
Linting CSS...
Invalid option "--disable-globbing".
END-BUILD: FAILURE
Build step 'Execute shell' marked build as failure

joeyparrish · 2020-09-09T21:19:40Z

Test Failure

This looks like a problem with our build bot. I'm looking into it.

shaka-bot · 2020-09-10T00:12:13Z

All tests passed!

shaka-bot · 2020-09-10T00:20:57Z

Test Failure:

Generating Closure dependencies...
Linting JavaScript...
Linting CSS...
Error: Undefined rule unit-allowed-list
at module.exports (/var/lib/jenkins/workspace/Manual PR Test (local-tests)/node_modules/stylelint/lib/utils/configurationError.js:8:28)
at Object.keys.forEach.ruleName (/var/lib/jenkins/workspace/Manual PR Test (local-tests)/node_modules/stylelint/lib/augmentConfig.js:323:13)
at Array.forEach (<anonymous>)
at normalizeAllRuleSettings (/var/lib/jenkins/workspace/Manual PR Test (local-tests)/node_modules/stylelint/lib/augmentConfig.js:316:29)
at augmentConfigBasic.then.then.then.augmentedConfig (/var/lib/jenkins/workspace/Manual PR Test (local-tests)/node_modules/stylelint/lib/augmentConfig.js:94:14)
END-BUILD: FAILURE
Build step 'Execute shell' marked build as failure

…cea708-decoder

shaka-bot · 2020-09-10T16:58:49Z

All tests passed!

joeyparrish · 2020-09-10T20:04:55Z

Thanks, @muhammadharis, for your contributions!

This pertains to shaka-project#2648 (although this is a new feature, not a replacement) and shaka-project#1404. A CEA-708 decoder that follows the CEA-708-E standard, decodes closed caption data from User Data Registered by Rec. ITU-T T.35 SEI messages, and returns them as cues in Shaka's internal cue format. Furthermore, this pull request fixes and cements some of the logic surrounding CEA-608 and CEA-708 tag parsing on the Dash Manifest Parser. Format: Similar to the CEA-608 decoder, 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. Details: - ASCII (G0), Latin-1 (G1), and CEA-708 specific charsets (G2 and G3) all supported. - Underlines, colors, and Italics supported, set as a property on each nested cue. - Positioning of text is supported. (Exception: In CEA-708 the default positioning is left, in this decoder it is centered.) - Positioning of windows not supported, but relevant fields that could be used to support this are extracted and left as a TODO.

muhammadharis added 30 commits July 14, 2020 16:17

removed unnecessary switch statement

b3ed9ae

made all hex lowercase

da868ce

removed log

e27c1e0

linting

047317c

added annotations for constants

18b1356

commenting hotfixes

352d058

moved comment

1ebb6a9

clarification in comments

resolve conflicts

9a9b65e

modularize design and add appropriate unit tests

0a4ae52

made colors array more intuitive

color fix

5302da8

CEA-708 Decoder (shaka-project#2648)

92da874

resolve merge conflict

4165bc7

improve error handling and safety of 708 decoding

10c0a5b

made data flow more intuitive

4933c1a

Merge branch 'master' of https://github.com/google/shaka-player into …

10381b9

…cea708-decoder

add impl for G2 and G3 groups, fix comments, enhance logic

adb467e

add emit logic

63973ee

map 64 colors -> 8 colors to avoid unreadable text

c11c1ed

moved shared constants into utils file

89d8913

unit tests for window

cc3b8b9

added unit tests for cea-708 logic

de499a8

remove extra file

a4bbebf

further unit tests and removing logs

d791022

minor bugfix in unit tests

7420ca9

fix emit logic, and fix type errors in compiler

759961b

tighten types on unit tests

b3579a3

clean dash parsing of CEA-608 and CEA-708

a3f323f

typing fix compiler

e7b0cc7

remove logs

f807210

triple equals

c514e7c

avelad mentioned this pull request Aug 26, 2020

Add note about CEA-708 #2824

Merged

fix merge conflict

72378f0

joeyparrish reviewed Aug 27, 2020

View reviewed changes

address pull request review

a6544fe

muhammadharis requested a review from joeyparrish August 28, 2020 19:31

crypt096 approved these changes Sep 1, 2020

View reviewed changes

remove comments on language_utils

bd0edf0

joeyparrish requested changes Sep 8, 2020

View reviewed changes

address pr review

b72883f

joeyparrish requested changes Sep 9, 2020

View reviewed changes

typo fix and remove unnecessary comment

df901cb

muhammadharis requested a review from joeyparrish September 9, 2020 20:57

joeyparrish approved these changes Sep 9, 2020

View reviewed changes

fix merge conflict

0b4ed72

Merge branch 'master' of https://github.com/google/shaka-player into …

0873830

…cea708-decoder

joeyparrish merged commit 54ff2d8 into shaka-project:master Sep 10, 2020

joeyparrish added the type: enhancement New feature or request label Sep 10, 2020

joeyparrish added this to the v3.1 milestone Sep 10, 2020

github-actions bot added the status: archived Archived and locked; will not be updated label Jul 25, 2023

github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CEA-708 Decoder (#2648) #2807

CEA-708 Decoder (#2648) #2807

muhammadharis commented Aug 18, 2020

joeyparrish Aug 27, 2020

joeyparrish Aug 27, 2020

joeyparrish Aug 27, 2020

joeyparrish Aug 27, 2020

joeyparrish Aug 27, 2020

joeyparrish Aug 27, 2020

joeyparrish left a comment

joeyparrish Sep 8, 2020

joeyparrish Sep 8, 2020

joeyparrish Sep 8, 2020

joeyparrish Sep 8, 2020

joeyparrish Sep 8, 2020

joeyparrish Sep 8, 2020

muhammadharis Sep 9, 2020

joeyparrish Sep 8, 2020

joeyparrish Sep 8, 2020

joeyparrish Sep 8, 2020

muhammadharis Sep 8, 2020

joeyparrish Sep 9, 2020

joeyparrish left a comment

shaka-bot commented Sep 9, 2020

joeyparrish commented Sep 9, 2020

shaka-bot commented Sep 10, 2020

shaka-bot commented Sep 10, 2020

shaka-bot commented Sep 10, 2020

joeyparrish commented Sep 10, 2020

CEA-708 Decoder (#2648) #2807

CEA-708 Decoder (#2648) #2807

Conversation

muhammadharis commented Aug 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joeyparrish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joeyparrish left a comment

Choose a reason for hiding this comment

shaka-bot commented Sep 9, 2020

joeyparrish commented Sep 9, 2020

shaka-bot commented Sep 10, 2020

shaka-bot commented Sep 10, 2020

shaka-bot commented Sep 10, 2020

joeyparrish commented Sep 10, 2020