feat: add position data to captions #434
Conversation
| @@ -35,7 +35,7 @@ var CoalesceStream = function(options) { | |||
| this.push = function(output) { | |||
| // buffer incoming captions until the associated video segment | |||
| // finishes | |||
| if (output.text) { | |||
| if (output.content || output.text) { | |||
There was a problem hiding this comment.
I believe other types of captions (708) could still use text as of now, so both content and text are included.
There was a problem hiding this comment.
A couple questions about applying the offset and indent to this.displayed_. Also have we tried this with 708? Curious if we get the positioning there as well, I can dig up some media or maybe we can create a unit test to try it out. Overall this looks great though, nice work!
| @@ -35,7 +35,7 @@ var CoalesceStream = function(options) { | |||
| this.push = function(output) { | |||
| // buffer incoming captions until the associated video segment | |||
| // finishes | |||
| if (output.text) { | |||
| if (output.content || output.text) { | |||
package-lock.json
Outdated
There was a problem hiding this comment.
I think we may want to exclude lock changes from this PR.
lib/m2ts/caption-stream.js
Outdated
| @@ -1231,10 +1233,12 @@ var ROWS = [0x1100, 0x1120, 0x1200, 0x1220, 0x1500, 0x1520, 0x1600, 0x1620, | |||
|
|
|||
| // CEA-608 captions are rendered onto a 34x15 matrix of character | |||
| // cells. The "bottom" row is the last element in the outer array. | |||
| // We keep track of positioning infomration as we go by storing the | |||
There was a problem hiding this comment.
nit: comment spelling
| // We keep track of positioning infomration as we go by storing the | |
| // We keep track of positioning information as we go by storing the |
|
|
||
| this.column_ = indentations * 4; | ||
| // add to the number of indentations for positioning | ||
| this.nonDisplayed_[this.row_].indent += indentations; |
There was a problem hiding this comment.
Do we need to add the indent to the this.displayed_ buffer here as well?
There was a problem hiding this comment.
Good question, and I don't believe so. I think the this.nonDisplayed_ buffer is basically where the caption data is manipulated until the end of a caption. At that instance, we swap everything over to this.displayed_.
|
|
||
| // For an offest value 1-3, set the offset for that caption | ||
| // in the non-displayed array. | ||
| this.nonDisplayed_[this.row_].offset = offset; |
There was a problem hiding this comment.
Do we need to add the offset to the this.displayed_ buffer?
@adrums86 Great question! As of now, these changes will not affect the position of 708 captions. I do however think we could add this logic to the
|
NOTE: This will require a major version update, as this changes how the caption data is sent to VHS. This was necessary, as we were previously combining text that can have different positioning data on these messages. This will require changes in VHS and video.js once this is released.
Previously for CEA-608 captions, we were completely ignoring positioning data and just centering all captions at the bottom of the screen. This change allows us to take advantage of our parsing and including this positioning data. The 'line' field represents the row on which to display the text from 1-15, and the
positionfield represents where the caption should be displayed horizontally on a scale from 1-100 (although the spec states we should lock it between 10-80).See: https://dvcs.w3.org/hg/text-tracks/raw-file/default/608toVTT/608toVTT.html#positioning-in-cea-608
Testing instructions will be included once I have a PR out with the VHS changes to consume this change.