feat: Add multibyte character support #398
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alex-barstow
force-pushed
the
multibyte-708-support
branch
from
4c9f4b8
to
091ad78
Compare
gkatsev
reviewed
Sep 3, 2021
Co-authored-by: Gary Katsevman <git@gkatsev.com>
Co-authored-by: Gary Katsevman <git@gkatsev.com>
…x.js into multibyte-708-support
| }); | ||
| } else { | ||
| try { | ||
| this.textDecoder_ = new TextDecoder(encoding); |
There was a problem hiding this comment.
This appears to throw if the encoding was incorrect. We may want to concat the caught error onto our warning below.
| // Use the TextDecoder if one was created for this service | ||
| if (service.textDecoder_ && !isExtended) { | ||
| if (isMultiByte) { | ||
| charCodeArray = [currentByte, nextByte]; |
There was a problem hiding this comment.
We should subarray here instead to prevent the creation of a new Uint8Array below.
// for multi-byte (can they be more than two characters?)
charCodeArray = packetData.subarray(i, i + 1)
// for single
charCodeArray = packetData.subarray(i)
There was a problem hiding this comment.
@brandonocasey Actually, packetData isn't a typed array so we do need to create a Uint8Array here. And to address your other question, from the spec:
When a decoder encounters the P16 code, it uses the succeeding two bytes to
address characters in a 16-bit code set.
gkatsev
reviewed
Sep 14, 2021
Co-authored-by: Gary Katsevman <git@gkatsev.com>
…x.js into multibyte-708-support
brandonocasey
approved these changes
Sep 20, 2021
gkatsev
reviewed
Sep 20, 2021
test/caption-stream.test.js
Outdated
Comment on lines
2797
to
2825
| QUnit.test('Creates TextDecoder only if valid encoding value is provided', function(assert) { | ||
| var secondCea708Stream; | ||
|
|
||
| cea708Stream = new m2ts.Cea708Stream({ | ||
| captionServices: { | ||
| SERVICE1: { | ||
| encoding: 'euc-kr' | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| cc708Korean.forEach(cea708Stream.push, cea708Stream); | ||
| cea708Stream.flushDisplayed(4721138662, cea708Stream.services[1]); | ||
|
|
||
| assert.ok(cea708Stream.services[1].textDecoder_, 'TextDecoder created successfully when encoding is valid'); | ||
|
|
||
| secondCea708Stream = new m2ts.Cea708Stream({ | ||
| captionServices: { | ||
| SERVICE1: { | ||
| encoding: 'invalid' | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| cc708Korean.forEach(secondCea708Stream.push, secondCea708Stream); | ||
| secondCea708Stream.flushDisplayed(4721138662, secondCea708Stream.services[1]); | ||
|
|
||
| assert.notOk(secondCea708Stream.services[1].textDecoder_, 'TextDecoder not created when encoding is invalid'); | ||
| }); |
There was a problem hiding this comment.
this test needs to check for IE11 or if TextDecoder is present.
gkatsev
approved these changes
Sep 20, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This adds multibyte character support to the 708 implementation via a new
encodingoption that can be passed to theCaptionStreamconstructor. The intent is to permit legacy multi-byte encodings for Chinese, Japanese, Korean, and others. The value of the option is used to create aTextDecoderinstance (assuming the current browser supports it, only IE doesn't), so the value can be any "valid [encoding] label".I'm waiting to commit the test I wrote until I have an open-source content sample to use