fix: Add fallback to TextDecoder and TextEncoder #4324
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
littlespex
previously approved these changes
Jun 29, 2022
|
@agajassi It's one thing we've seen helpful when investigating your issue, but it doesn't fix your issue. |
|
Ok understood. Thank you |
agajassi
previously approved these changes
Jun 29, 2022
|
This would remove the need for a TextDecoder/TextEncoder polyfill. Is that the intent? |
|
@joeyparrish The main idea is to reduce the requirements of a polyfill which takes up much more when you know that your streams do not contain invalid UTF-8 characters. This applies to Xbox |
|
Okay, then. Let's do a few more things to make sure we don't have a regression in the future:
|
|
@joeyparrish can you review it again? Thanks! |
joeyparrish
approved these changes
Jul 11, 2022
|
I'm relabeling this as a "fix" and cherry-picking it to new releases. |
joeyparrish
changed the title
joeyparrish
pushed a commit
that referenced
this pull request
Jul 14, 2022
Related to a72a1e9 Backported to v3.2.x
joeyparrish
pushed a commit
that referenced
this pull request
Jul 14, 2022
Related to a72a1e9 Backported to v3.2.x
joeyparrish
pushed a commit
that referenced
this pull request
Jul 14, 2022
joeyparrish
pushed a commit
that referenced
this pull request
Jul 14, 2022
This was referenced Jul 14, 2022
joeyparrish
added a commit
to joeyparrish/shaka-player
that referenced
this pull request
Aug 11, 2022
In PR shaka-project#4324, we lifted the requirement to have a native or polyfilled TextDecoder implementation. However, we forgot to remove the check for it in isBrowserSupported(). This led to tests being skipped entirely on Xbox, as Xbox was determined to be an unsupported platform by Player. Although this is a fix, it's a fix to a feature for v4.2, and we don't want to accidentally cherry-pick it to a branch that _does_ still require TextDecoder. So this change is tagged "feat" as well.
joeyparrish
added a commit
to joeyparrish/shaka-player
that referenced
this pull request
Aug 12, 2022
In PR shaka-project#4324, we lifted the requirement to have a native or polyfilled TextDecoder implementation. However, we forgot to remove the check for it in isBrowserSupported(). This led to tests being skipped entirely on Xbox, as Xbox was determined to be an unsupported platform by Player. To fix this, the check for TextDecoder/TextEncode in isBrowserSupported() has been removed. When the TextDecoder polyfill was removed, we left a reference to it in karma.conf.js. This didn't hurt anything per se, but this has now been cleaned up. Finally, TextDecoder was originally introduced to give us a way to recover from errors instead of throwing. The fallback that was reintroduced in shaka-project#4324 was the original code that throws on error. This led to a test failure on Xbox, which represents a complete subtitle failure in real content with an encoding issue. To fix this, we replace the utf-8 decoding fallback based on decodeURIComponent with a plain JS implementation. To better verify these text decoding features, a test that checked two things has been split into two, comments around the tests have been improved, and an additional test case has been added.
joeyparrish
added a commit
to joeyparrish/shaka-player
that referenced
this pull request
Aug 12, 2022
In PR shaka-project#4324, we lifted the requirement to have a native or polyfilled TextDecoder implementation. However, we forgot to remove the check for it in isBrowserSupported(). This led to tests being skipped entirely on Xbox, as Xbox was determined to be an unsupported platform by Player. To fix this, the check for TextDecoder/TextEncode in isBrowserSupported() has been removed. When the TextDecoder polyfill was removed, we left a reference to it in karma.conf.js. This didn't hurt anything per se, but this has now been cleaned up. Finally, TextDecoder was originally introduced to give us a way to recover from errors instead of throwing. The fallback that was reintroduced in shaka-project#4324 was the original code that throws on error. This led to a test failure on Xbox, which represents a complete subtitle failure in real content with an encoding issue. To fix this, we replace the utf-8 decoding fallback based on decodeURIComponent with a plain JS implementation. This adds only 477 bytes to Shaka Player, which is pretty good compared to the 2315 byte polyfill we used to recommend for this. To better verify these text decoding features, a test that checked two things has been split into two, comments around the tests have been improved, and an additional test case has been added.
joeyparrish
added a commit
that referenced
this pull request
Aug 12, 2022
In PR #4324, we lifted the requirement to have a native or polyfilled TextDecoder implementation. However, we forgot to remove the check for it in isBrowserSupported(). This led to tests being skipped entirely on Xbox, as Xbox was determined to be an unsupported platform by Player. To fix this, the check for TextDecoder/TextEncode in isBrowserSupported() has been removed. When the TextDecoder polyfill was removed, we left a reference to it in karma.conf.js. This didn't hurt anything per se, but this has now been cleaned up. Finally, TextDecoder was originally introduced to give us a way to recover from errors instead of throwing. The fallback that was reintroduced in #4324 was the original code that throws on error. This led to a test failure on Xbox, which represents a complete subtitle failure in real content with an encoding issue. To fix this, we replace the utf-8 decoding fallback based on decodeURIComponent with a plain JS implementation. This adds only 477 bytes to Shaka Player, which is pretty good compared to the 2315 byte polyfill we used to recommend for this. To better verify these text decoding features, a test that checked two things has been split into two, comments around the tests have been improved, and an additional test case has been added.
joeyparrish
added a commit
to joeyparrish/shaka-player
that referenced
this pull request
Aug 13, 2022
…t#4403) In PR shaka-project#4324, we lifted the requirement to have a native or polyfilled TextDecoder implementation. However, we forgot to remove the check for it in isBrowserSupported(). This led to tests being skipped entirely on Xbox, as Xbox was determined to be an unsupported platform by Player. To fix this, the check for TextDecoder/TextEncode in isBrowserSupported() has been removed. When the TextDecoder polyfill was removed, we left a reference to it in karma.conf.js. This didn't hurt anything per se, but this has now been cleaned up. Finally, TextDecoder was originally introduced to give us a way to recover from errors instead of throwing. The fallback that was reintroduced in shaka-project#4324 was the original code that throws on error. This led to a test failure on Xbox, which represents a complete subtitle failure in real content with an encoding issue. To fix this, we replace the utf-8 decoding fallback based on decodeURIComponent with a plain JS implementation. This adds only 477 bytes to Shaka Player, which is pretty good compared to the 2315 byte polyfill we used to recommend for this. To better verify these text decoding features, a test that checked two things has been split into two, comments around the tests have been improved, and an additional test case has been added.
joeyparrish
added a commit
to joeyparrish/shaka-player
that referenced
this pull request
Aug 13, 2022
…t#4403) In PR shaka-project#4324, we lifted the requirement to have a native or polyfilled TextDecoder implementation. However, we forgot to remove the check for it in isBrowserSupported(). This led to tests being skipped entirely on Xbox, as Xbox was determined to be an unsupported platform by Player. To fix this, the check for TextDecoder/TextEncode in isBrowserSupported() has been removed. When the TextDecoder polyfill was removed, we left a reference to it in karma.conf.js. This didn't hurt anything per se, but this has now been cleaned up. Finally, TextDecoder was originally introduced to give us a way to recover from errors instead of throwing. The fallback that was reintroduced in shaka-project#4324 was the original code that throws on error. This led to a test failure on Xbox, which represents a complete subtitle failure in real content with an encoding issue. To fix this, we replace the utf-8 decoding fallback based on decodeURIComponent with a plain JS implementation. This adds only 477 bytes to Shaka Player, which is pretty good compared to the 2315 byte polyfill we used to recommend for this. To better verify these text decoding features, a test that checked two things has been split into two, comments around the tests have been improved, and an additional test case has been added.
joeyparrish
added a commit
to joeyparrish/shaka-player
that referenced
this pull request
Aug 13, 2022
…t#4403) In PR shaka-project#4324, we lifted the requirement to have a native or polyfilled TextDecoder implementation. However, we forgot to remove the check for it in isBrowserSupported(). This led to tests being skipped entirely on Xbox, as Xbox was determined to be an unsupported platform by Player. To fix this, the check for TextDecoder/TextEncode in isBrowserSupported() has been removed. When the TextDecoder polyfill was removed, we left a reference to it in karma.conf.js. This didn't hurt anything per se, but this has now been cleaned up. Finally, TextDecoder was originally introduced to give us a way to recover from errors instead of throwing. The fallback that was reintroduced in shaka-project#4324 was the original code that throws on error. This led to a test failure on Xbox, which represents a complete subtitle failure in real content with an encoding issue. To fix this, we replace the utf-8 decoding fallback based on decodeURIComponent with a plain JS implementation. This adds only 477 bytes to Shaka Player, which is pretty good compared to the 2315 byte polyfill we used to recommend for this. To better verify these text decoding features, a test that checked two things has been split into two, comments around the tests have been improved, and an additional test case has been added.
joeyparrish
added a commit
to joeyparrish/shaka-player
that referenced
this pull request
Aug 13, 2022
…t#4403) In PR shaka-project#4324, we lifted the requirement to have a native or polyfilled TextDecoder implementation. However, we forgot to remove the check for it in isBrowserSupported(). This led to tests being skipped entirely on Xbox, as Xbox was determined to be an unsupported platform by Player. To fix this, the check for TextDecoder/TextEncode in isBrowserSupported() has been removed. When the TextDecoder polyfill was removed, we left a reference to it in karma.conf.js. This didn't hurt anything per se, but this has now been cleaned up. Finally, TextDecoder was originally introduced to give us a way to recover from errors instead of throwing. The fallback that was reintroduced in shaka-project#4324 was the original code that throws on error. This led to a test failure on Xbox, which represents a complete subtitle failure in real content with an encoding issue. To fix this, we replace the utf-8 decoding fallback based on decodeURIComponent with a plain JS implementation. This adds only 477 bytes to Shaka Player, which is pretty good compared to the 2315 byte polyfill we used to recommend for this. To better verify these text decoding features, a test that checked two things has been split into two, comments around the tests have been improved, and an additional test case has been added.
joeyparrish
added a commit
that referenced
this pull request
Aug 16, 2022
In PR #4324, we lifted the requirement to have a native or polyfilled TextDecoder implementation. However, we forgot to remove the check for it in isBrowserSupported(). This led to tests being skipped entirely on Xbox, as Xbox was determined to be an unsupported platform by Player. To fix this, the check for TextDecoder/TextEncode in isBrowserSupported() has been removed. When the TextDecoder polyfill was removed, we left a reference to it in karma.conf.js. This didn't hurt anything per se, but this has now been cleaned up. Finally, TextDecoder was originally introduced to give us a way to recover from errors instead of throwing. The fallback that was reintroduced in #4324 was the original code that throws on error. This led to a test failure on Xbox, which represents a complete subtitle failure in real content with an encoding issue. To fix this, we replace the utf-8 decoding fallback based on decodeURIComponent with a plain JS implementation. This adds only 477 bytes to Shaka Player, which is pretty good compared to the 2315 byte polyfill we used to recommend for this. To better verify these text decoding features, a test that checked two things has been split into two, comments around the tests have been improved, and an additional test case has been added.
joeyparrish
added a commit
that referenced
this pull request
Aug 16, 2022
In PR #4324, we lifted the requirement to have a native or polyfilled TextDecoder implementation. However, we forgot to remove the check for it in isBrowserSupported(). This led to tests being skipped entirely on Xbox, as Xbox was determined to be an unsupported platform by Player. To fix this, the check for TextDecoder/TextEncode in isBrowserSupported() has been removed. When the TextDecoder polyfill was removed, we left a reference to it in karma.conf.js. This didn't hurt anything per se, but this has now been cleaned up. Finally, TextDecoder was originally introduced to give us a way to recover from errors instead of throwing. The fallback that was reintroduced in #4324 was the original code that throws on error. This led to a test failure on Xbox, which represents a complete subtitle failure in real content with an encoding issue. To fix this, we replace the utf-8 decoding fallback based on decodeURIComponent with a plain JS implementation. This adds only 477 bytes to Shaka Player, which is pretty good compared to the 2315 byte polyfill we used to recommend for this. To better verify these text decoding features, a test that checked two things has been split into two, comments around the tests have been improved, and an additional test case has been added.
joeyparrish
added a commit
that referenced
this pull request
Aug 16, 2022
In PR #4324, we lifted the requirement to have a native or polyfilled TextDecoder implementation. However, we forgot to remove the check for it in isBrowserSupported(). This led to tests being skipped entirely on Xbox, as Xbox was determined to be an unsupported platform by Player. To fix this, the check for TextDecoder/TextEncode in isBrowserSupported() has been removed. When the TextDecoder polyfill was removed, we left a reference to it in karma.conf.js. This didn't hurt anything per se, but this has now been cleaned up. Finally, TextDecoder was originally introduced to give us a way to recover from errors instead of throwing. The fallback that was reintroduced in #4324 was the original code that throws on error. This led to a test failure on Xbox, which represents a complete subtitle failure in real content with an encoding issue. To fix this, we replace the utf-8 decoding fallback based on decodeURIComponent with a plain JS implementation. This adds only 477 bytes to Shaka Player, which is pretty good compared to the 2315 byte polyfill we used to recommend for this. To better verify these text decoding features, a test that checked two things has been split into two, comments around the tests have been improved, and an additional test case has been added.
joeyparrish
added a commit
that referenced
this pull request
Aug 16, 2022
In PR #4324, we lifted the requirement to have a native or polyfilled TextDecoder implementation. However, we forgot to remove the check for it in isBrowserSupported(). This led to tests being skipped entirely on Xbox, as Xbox was determined to be an unsupported platform by Player. To fix this, the check for TextDecoder/TextEncode in isBrowserSupported() has been removed. When the TextDecoder polyfill was removed, we left a reference to it in karma.conf.js. This didn't hurt anything per se, but this has now been cleaned up. Finally, TextDecoder was originally introduced to give us a way to recover from errors instead of throwing. The fallback that was reintroduced in #4324 was the original code that throws on error. This led to a test failure on Xbox, which represents a complete subtitle failure in real content with an encoding issue. To fix this, we replace the utf-8 decoding fallback based on decodeURIComponent with a plain JS implementation. This adds only 477 bytes to Shaka Player, which is pretty good compared to the 2315 byte polyfill we used to recommend for this. To better verify these text decoding features, a test that checked two things has been split into two, comments around the tests have been improved, and an additional test case has been added.
github-actions
bot
added
the
status: archived
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Related to a72a1e9
BEGIN_COMMIT_OVERRIDE
fix: Add fallback to TextDecoder and TextEncoder #4324
END_COMMIT_OVERRIDE