Skip to content

Commit

Permalink
WASM: Temporarily rollback on wasm-opt dependency for release builds …
Browse files Browse the repository at this point in the history
…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.
  • Loading branch information
peaBerberian committed Feb 5, 2024
1 parent fee73d0 commit 7e484e2
Show file tree
Hide file tree
Showing 5 changed files with 8 additions and 85 deletions.
1 change: 1 addition & 0 deletions .github/workflows/checks.yml
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ jobs:
cache: 'npm'
# needed for integration & memory tests codecs support
- run: sudo add-apt-repository multiverse && sudo apt update && sudo apt install -y ubuntu-restricted-extras
- run: sudo apt-get -y install binaryen
- run: npm install
- run: npm run build
# Firefox seems to have issue with integration tests on GitHub actions only
Expand Down
17 changes: 0 additions & 17 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 0 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,6 @@
"@typescript-eslint/parser": "6.16.0",
"arraybuffer-loader": "1.0.8",
"babel-loader": "9.1.3",
"binaryen": "116.0.0",
"chai": "4.3.10",
"core-js": "3.34.0",
"esbuild": "0.19.10",
Expand Down
63 changes: 0 additions & 63 deletions scripts/wasm-optimize.mjs

This file was deleted.

11 changes: 7 additions & 4 deletions src/parsers/manifest/dash/wasm-parser/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -251,12 +251,13 @@ fear that they may change in the future, in which case this documentation could
easily be outdated.

To be able to call those scripts, you will need to have the Rust compiler
toolchain installed and ready to compile to WebAssembly.
toolchain installed and ready to compile to WebAssembly as well as `binaryen`, a
WebAssembly-specialized toolbox.

To do this, the easiest way would be to rely on `rustup`, a tool to install and
update Rust toolchains:
There are several ways this can be done, with the easiest way generally being:

1. Install [rustup](https://rustup.rs/)
1. Install [rustup](https://rustup.rs/), which is a tool intended to
facilitate the installation of Rust toolchains

2. Install and rely on the stable toolchain:
```sh
Expand All @@ -268,4 +269,6 @@ update Rust toolchains:
rustup target add wasm32-unknown-unknown
```

4. Now install [binaryen](https://github.com/WebAssembly/binaryen)

That should be it!

0 comments on commit 7e484e2

Please sign in to comment.