fix: Fix TextDecoder fallback and browser support check #4403
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
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.
avelad
reviewed
Aug 12, 2022
| throw new shaka.util.Error( | ||
| shaka.util.Error.Severity.CRITICAL, shaka.util.Error.Category.TEXT, | ||
| shaka.util.Error.Code.BAD_ENCODING); | ||
| // Homebrewed UTF-8 decoder based on |
There was a problem hiding this comment.
One question, why only do it in the case of TextDecoder? A few lines further down there is similar logic for TextEncoder.
There was a problem hiding this comment.
Because in the case of converting UTF-8 to a string, we need error recovery. Not every input is valid. That was the original issue we solved by introducing TextDecoder. However, for converting from string to UTF-8, all input strings are valid AFAICT. So the original fallback is good enough.
avelad
approved these changes
Aug 12, 2022
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.
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.