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"
(initial release of the feature).

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 print a deprecation notice.

     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.

  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 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 both the deprecation notices and the fact
that another rustc flag (-C target=mvp) didn't have the expected effect.

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 1, 2024
1 parent d134224 commit 280df78
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@
"build:dev": "./scripts/generate_build.mjs --dev-mode",
"build:all": "npm run build:wasm:release && npm run bundle && npm run bundle:min && npm run build",
"build:wasm:debug": "mkdir -p dist && cd ./src/parsers/manifest/dash/wasm-parser && cargo build --target wasm32-unknown-unknown && cp target/wasm32-unknown-unknown/debug/mpd_node_parser.wasm ../../../../../dist/mpd-parser.wasm",
"build:wasm:release": "mkdir -p dist && cd ./src/parsers/manifest/dash/wasm-parser && cargo build --target wasm32-unknown-unknown --release && node ../../../../../scripts/wasm-optimize.mjs target/wasm32-unknown-unknown/release/mpd_node_parser.wasm ../../../../../dist/mpd-parser.wasm && cd ../../../../../ && npm run wasm-strip",
"build:wasm:release": "mkdir -p dist && cd ./src/parsers/manifest/dash/wasm-parser && cargo build --target wasm32-unknown-unknown --release && wasm-opt target/wasm32-unknown-unknown/release/mpd_node_parser.wasm --signext-lowering -O4 -o ../../../../../dist/mpd-parser.wasm && cd ../../../../../ && npm run wasm-strip",
"bundle": "webpack --progress --config webpack.config.mjs --env production",
"bundle:min": "webpack --progress --config webpack.config.mjs --env minify --env production",
"bundle:min:watch": "webpack --progress --config webpack.config.mjs -w --env production --env minify",
Expand Down

0 comments on commit 280df78

Please sign in to comment.