WASM: Temporarily rollback on wasm-opt dependency for release builds to make it work on some LG + Samsung TVs #1372
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
peaBerberian
added
Compatibility
|
Why do you consider that change is temporary ? If the binaryien npm module is not used anymore, shouldn't we remove it from the There was also a documentation about installing binaryen, that was removed when we moved on the npm module. |
I'm not satisfied with having to rely on a dependency initially purposed for optimizations to act as a sort of babeljs for wasm (unless it was one of its announced goal). I would be much more comfortable if this was done by the compiler as it seems to me to be its role. Doing this through the compiler today is full of "unstable" notices however, so for now I just wait until the issue come to bite us again (e.g. until the produced WebAssembly contains yet another newer feature, either due to a code change on our side or due to a compiler change on their side) to see if we can come back to relying on the compiler.
You're right, I forgot ! Will do.
Yes, will also do thanks! |
peaBerberian
force-pushed
the
fix/remove-sign-extensions-features
branch
from
c60081f
to
b5217ee
Compare
|
Done |
|
Hm it seems that the binaryen's #1373 may fix this on the future if we rely on it |
peaBerberian
force-pushed
the
fix/remove-sign-extensions-features
branch
8 times, most recently
from
a2d4883
to
f1e52ee
Compare
peaBerberian
added a commit
that referenced
this pull request
Feb 5, 2024
Last week, we've seen an issue on some Samsung and LG TVs when relying on the RxPlayer new experimental `MULTI_THREAD` feature due to specific opcodes found in our WebAssembly files which were not compatible with some of those TVs' browser. Though we isolated and fixed the issue in #1372, it might be better to also find a longer term solution to rollback the `MULTI_THREAD` feature when an issue is detected with it preventing us from playing. This could be done in several ways, from throwing errors, to new events, to just return a rejecting Promise in the `attachWorker` method. I chose to go with the latter of those solutions now because it appears logical API-wise and implementation-wise to have that method return a Promise which resolves only if we're able to communicate with a WebWorker (and reject either if the browser does not support it, if a security policy prevent from running the worker, if the request for the worker's code fail or if the code evualation itself fails). I've also added a specialized error just for that API to give more context about what failed (missing feature? etc.). I was afraid that relying on this new Promise to indicate an issue at WebAssembly-compilation-time for our MPD parser would bother us in the future if we ever add other WebAssembly modules (e.g. a smooth parser), which could also independently fail (should we reject the Promise when either compilation fails? Even if we could theoretically still play DASH contents? How would we mix this way with a potentially lazy-loading of features where we wouldn't be compiling right away? and so on...), but after exposing all the potential future paths I could see this `MULTI_THREAD` feature taking, I was able to find an adapted solution still compatible with returning a Promise on the `attachWorker` API. I also tried to automatically fallback from a "multithread mode" to the regular monothread one inside the RxPlayer but doing this was complex. So for now, if `attachWorker` fails, the RxPlayer will remove the worker from its state (new `loadVideo` calls won't depend on it) but it is the responsibility of the application to reload if a content was loaded in "multithread mode" was loaded in the meantime. If an application doesn't want to handle that supplementary complexity, it can just await the Promise returned by `attachWorker` before loading the first content (and catching eventual errors). As the RxPlayer automatically removes the worker if its initialization fails, this will lead to automatically fallback on main thread. At the cost of some time compared to load and initialize the worker parallely.
peaBerberian
added
the
Priority: 0 (Very high)
peaBerberian
added a commit
that referenced
this pull request
Feb 5, 2024
Last week, we've seen an issue on some Samsung and LG TVs when relying on the RxPlayer new experimental `MULTI_THREAD` feature due to specific opcodes found in our WebAssembly files which were not compatible with some of those TVs' browser. Though we isolated and fixed the issue in #1372, it might be better to also find a longer term solution to rollback the `MULTI_THREAD` feature when an issue is detected with it preventing us from playing. This could be done in several ways, from throwing errors, to new events, to just return a rejecting Promise in the `attachWorker` method. I chose to go with the latter of those solutions now because it appears logical API-wise and implementation-wise to have that method return a Promise which resolves only if we're able to communicate with a WebWorker (and reject either if the browser does not support it, if a security policy prevent from running the worker, if the request for the worker's code fail or if the code evualation itself fails). I've also added a specialized error just for that API to give more context about what failed (missing feature? etc.). I was afraid that relying on this new Promise to indicate an issue at WebAssembly-compilation-time for our MPD parser would bother us in the future if we ever add other WebAssembly modules (e.g. a smooth parser), which could also independently fail (should we reject the Promise when either compilation fails? Even if we could theoretically still play DASH contents? How would we mix this way with a potentially lazy-loading of features where we wouldn't be compiling right away? and so on...), but after exposing all the potential future paths I could see this `MULTI_THREAD` feature taking, I was able to find an adapted solution still compatible with returning a Promise on the `attachWorker` API. I also tried to automatically fallback from a "multithread mode" to the regular monothread one inside the RxPlayer but doing this was complex. So for now, if `attachWorker` fails, the RxPlayer will remove the worker from its state (new `loadVideo` calls won't depend on it) but it is the responsibility of the application to reload if a content was loaded in "multithread mode" was loaded in the meantime. If an application doesn't want to handle that supplementary complexity, it can just await the Promise returned by `attachWorker` before loading the first content (and catching eventual errors). As the RxPlayer automatically removes the worker if its initialization fails, this will lead to automatically fallback on main thread. At the cost of some time compared to load and initialize the worker parallely.
Merged
peaBerberian
force-pushed
the
fix/remove-sign-extensions-features
branch
2 times, most recently
from
a325c44
to
7e484e2
Compare
…to make it work on some lg TVs After testing at large scale our `MULTI_THREAD` feature, we noticed that some LG and samsung TVs had issues running it. The problem in question was due to an unrecognized WebAssembly Op Code by the device linked to the "sign-extensions" proposal (https://github.com/WebAssembly/sign-extension-ops/blob/master/proposals/sign-extension-ops/Overview.md) which was not introduced initially with the so-called "WebAssembly MVP" (the initial release of WebAssembly). It turns out that rustc (the Rust compiler) targets a WebAssembly version that is a little further than the MVP by default (it seems that this decision is actually taken by the LLVM project from what I understand from an answers in rust-lang/rust#109807) and amongst the included features is the sign-extensions feature (so the rest of included feature is totally unclear and I could not find any resource on the web listing them. From there, I was able to remove the sign-extensions Op Code from the built WebAssembly file by doing either one of these two ways: 1. Through a rustc flag (`-C target-feature=-sign-ext`). It should be noted that the inclusion of this flag prints a warning notice indicating that the flag is "unstable" Interestingly, a `-C target-cpu=mvp` flag, which looks like it is exactly what we want, is also listed in that page yet it doesn't seem to remove the sign-extensions opcode for me. Maybe because it takes an already-compiled stdlib and the problematic opcode is from there? We could re-build stdlib through another flag but this latter one is marked as "unstable" so I didn't want to adventure too far into this theory and I didn't either understand why the other flag would have an effect in that case. 2. By adding the `--signext-lowering` flag to binaryen's wasm-opt tool which is documented at: > lower sign-ext operations to wasm mvp and disable the sign > extension feature That solution is nice but I couldn't rely on that flag with the [binaryen npm module](https://www.npmjs.com/package/binaryen) we now rely on (because RxPlayer developers most likely have npm installed so it was seen as a simpler dependency in the project than binaries brought by a binaryen project that has to be installed separately). This means that to add this feature, I have to bring back the full `binaryen` package as a dependency of the RxPlayer which isn't nice. Even with its drawbacks, I chose to go with the second solution here because I was afraid due to its warning notice, the fact that the other rustc flag (-C target=mvp) didn't have the expected effect and the hint that this might not work when the feature is in stdlib. We may come back to this in the future. Still, this only fix the issue with the `sign-extensions` opcode, and not the real larger issue which is to either properly handle all devices supporting WebAssembly or to be able to detect and fallback when a device fails to compile it. This hypotetical scenario (for now, though may be more common in the future), will be handled in a future PR. It only acts on that feature. Future WebAssembly builds could break if other new features are not handled by the device.
peaBerberian
force-pushed
the
fix/remove-sign-extensions-features
branch
from
7e484e2
to
631ff14
Compare
|
NOTE: #1373 was now merged, and it already includes the fix. |
Merged
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.
After testing at large scale our
MULTI_THREADfeature, we noticed that some LG and samsung TVs had issues running it.The problem in question was due to an unrecognized WebAssembly Op Code by the device linked to the "sign-extensions" proposal (https://github.com/WebAssembly/sign-extension-ops/blob/master/proposals/sign-extension-ops/Overview.md) which was not introduced initially with the so-called "WebAssembly MVP" (the initial release of WebAssembly).
It turns out that rustc (the Rust compiler) targets a WebAssembly version that is a little further than the MVP by default (it seems that this decision is actually taken by the LLVM project from what I understand from an answers in
rust-lang/rust#109807) and amongst the included features is the sign-extensions feature (so the rest of included features is totally unclear to me and I could not find any resource on the web listing them.
From there, I was able to remove the sign-extensions Op Code from the built WebAssembly file by doing either one of these two ways:
Through a rustc flag (
-C target-feature=-sign-ext). It should be noted that the inclusion of this flag prints a warning notice indicating that the flag is "unstable"Interestingly, a
-C target-cpu=mvpflag, which looks like it is exactly what we want, should also be available yet it doesn't seem to remove the sign-extensions opcode for me.Maybe because it takes an already-compiled stdlib and the problematic opcode is from there? We could re-build stdlib through another flag but this latter one is also marked as "unstable" so I didn't want to adventure too far into this theory and I didn't either understand why the first
-sign-extflag would have an effect in that case.By adding the
--signext-loweringflag to binaryen's wasm-opt tool which is documented at:That solution is nice but I couldn't rely on that flag with the binaryen npm module we now rely on (because RxPlayer developers most likely have npm installed so it was seen as a simpler dependency in the project than binaries brought by a binaryen project that has to be installed separately).
This means that to add this feature, I have to bring back the full
binaryenpackage as a dependency of the RxPlayer.Even with its drawbacks, I chose to go with the second solution here because I was afraid of the first one due to its warning notice, the fact that the other rustc flag (-C target=mvp) didn't have the expected effect and the hint that this might not work when the corresponding source code was found in stdlib (unless some other unstable flag is set). We may come back to this in the future.
Still, this only fixes the issue with the
sign-extensionsopcode, and not the real larger issue which is to either properly handle all devices supporting WebAssembly or to be able to detect and fallback when a device fails to compile our WebAssembly file (doing both would probably be best).It only acts on that "signext" feature. Future WebAssembly builds could break if other new features are not handled by the device.
This hypothetical scenario (for now, though may be more common in the future), will be handled in a future PR.