jellyfin / jellyfin-web

Web Client for Jellyfin
https://jellyfin.org
GNU General Public License v2.0
2.42k stars 1.29k forks source link

JASSUB doesn't work in legacy browsers #4794

Closed dmitrylyzo closed 8 months ago

dmitrylyzo commented 1 year ago

Describe The Bug JASSUB doesn't work in legacy browsers: worker fails on webOS 1.2, requiring fetch, Promise (probably more).

Steps To Reproduce

  1. Run app in webOS 1.2 emulator using ares-inspect.
  2. Open ares-inspect address in the Chrome 79 (newer may work too).
  3. Start playback of video with ASS subtitles.
  4. See error in the console.

Expected Behavior No error and see subtitles.

System (please complete the following information):

Additional Context Promise in webOS 2 has a weird implementation, so it also requires testing. JASSUB 1.5.13 works fine, because it was built with compatibility flags (browser version).

ThaUnknown commented 1 year ago

this is.... unusual?

jassub only uses fetch for engines which support streamed WASM compilation, which REQUIRES fetch in the engine to work, as it uses fetch responses, so if fetch doesnt exist, streamed compilation will never run, and never call fetch, so it's a non-issue?

  // hack, we want custom WASM URLs
  const _fetch = fetch
  const wasm = !WebAssembly.instantiateStreaming && read_(data.wasmUrl, true)
  if (WebAssembly.instantiateStreaming) self.fetch = _ => _fetch(data.wasmUrl)
  WASM({ wasm }).then(Module => {
    _malloc = Module._malloc
    // ...

fetch is overwritten because emscripten in their infinite wisdom decided to hardcode the wasm url when using instantiateStreaming so this unwraps it, kinda hacky, but it works really good

instantiateStreaming was added way after fetch so even if fetch doesn't exist, it should never cause an error during runtime as fetch itself wont ever be called

from emscripten's compiled code:

WebAssembly.instantiateStreaming?WebAssembly.instantiateStreaming(fetch("hardcodedURL.wasm"),imports):WebAssembly.instantiate(Module["wasm"],imports)

so fetch will never run on engines which don't support it, this is already checked by jassub and emscripten

promise also shouldn't need to be polyfilled as Promises are only required when using webassembly which was added after promises and on engines which don't support WASM promises aren't used, instead a fake WASM emulation is used:

  try {
    const module = new WebAssembly.Module(Uint8Array.of(0x0, 0x61, 0x73, 0x6d, 0x01, 0x00, 0x00, 0x00))
    if (!(module instanceof WebAssembly.Module) || !(new WebAssembly.Instance(module) instanceof WebAssembly.Instance)) throw new Error('WASM not supported')
  } catch (e) {
    console.warn(e)
    // load WASM2JS code if WASM is unsupported
    eval(read_(data.legacyWasmUrl))
  }

which fakes the promise.then code with [from emscriptens compiled code]:

var
WebAssembly = {
  // Note that we do not use closure quoting (this['buffer'], etc.) on these
  // functions, as they are just meant for internal use. In other words, this is
  // not a fully general polyfill.
  /** @constructor */
  Memory: function(opts) {
    this.buffer = new ArrayBuffer(opts['initial'] * 65536);
  },

  Module: function(binary) {
    // TODO: use the binary and info somehow - right now the wasm2js output is embedded in
    // the main JS
  },

  /** @constructor */
  Instance: function(module, info) {
    // TODO: use the module somehow - right now the wasm2js output is embedded in
    // the main JS
    // This will be replaced by the actual wasm2js code.
    this.exports = (
// EMSCRIPTEN_START_ASM
// ...esm2js code
// EMSCRIPTEN_END_ASM

)(info);
  },

  instantiate: /** @suppress{checkTypes} */ function(binary, info) {
    return {
      then: function(ok) {
        var module = new WebAssembly.Module(binary);
        ok({
          'instance': new WebAssembly.Instance(module, info)
        });
        // Emulate a simple WebAssembly.instantiate(..).then(()=>{}).catch(()=>{}) syntax.
        return { catch: function() {} };
      }
    };
  },

  RuntimeError: Error
};

it does however require Promise inside the main thread, which I think is already polyfilled in JF web so it shouldn't be an issue?

ThaUnknown commented 1 year ago

image also I can't get JF to even play any media to test this :&

ThaUnknown commented 1 year ago

I couldn't get JF-web to work, so I simply copied its webpack config and hosted it on my test page, it works on chrome 76, but I can't really test webOS: image

dmitrylyzo commented 1 year ago

Chrome 76 (2019) is much more modern than the webOS 1.2 (2014) web engine. webOS 1.2 uses old WebKit. In strict mode, it complains about fetch and WebAssembly just because they aren't in global scope. Using self.* helps here (at least with WebAssembly), but then it fails on Promise.

The problem started after WASM and JS workers were combined.

First of all, worker now requires transpilation. A week ago, that wasn't required. :thinking:

I created fix-legacy branch (not ready, but some commits are fine).

but I can't really test webOS

I test webOS 1.2 using LG SDK: https://webostv.developer.lge.com/develop/tools/cli-installation https://webostv.developer.lge.com/develop/tools/emulator-installation

ThaUnknown commented 1 year ago

the worker always required transpiration, this isn't new, I'll look into missing globals when using strict, or I could remove use strict which I think is added by vite [as always causes problems]

also I don't see why it would cry about WebAssembly missing, if it doesn't exist it should error inside the trycatch, and polyfill it, and the next time it's ran it's already defined

I see the issue with promise tho, that's annoying

ThaUnknown commented 1 year ago

I think promise needs to be polyfilled in worker, I tried, but I'm faily sure I failed

ThaUnknown commented 1 year ago

I'm fairly sure I fixed all these issues in 1.7.7

dmitrylyzo commented 1 year ago

I'm fairly sure I fixed all these issues in 1.7.7

JASSUB 1.7.7 doesn't work in Chrome 79

Uncaught (in promise) RangeError: WebAssembly.Table.get(): invalid index 584944 into function table
    at ae (http://.../jellyfin/web/176e75238a0c18a53cdd.js:480:74)
    at Dr (http://.../jellyfin/web/176e75238a0c18a53cdd.js:1142:16)
    at wasm-function[722]:0xa18b4
    at wasm-function[747]:0xa68eb
    at wasm-function[870]:0xea152
    at Ir (http://.../jellyfin/web/176e75238a0c18a53cdd.js:1159:9)
    at http://.../jellyfin/web/176e75238a0c18a53cdd.js:1173:197

It doesn't work on webOS 1.2: it loads jassub-worker.wasm.js, but then it also loads data.wasmUrl, which is jassub-worker.wasm.

ThaUnknown commented 1 year ago

audible confusion

it loading both wasm modules is a side effect but it doesn't really matter, since only the legacy one is used, and the latter is ignored, that table.get is a massive woe however, because it shouldn't even run webassembly code, ever, if it loads the legacy code it shouldn't ever use normal WASM

the stack trace is kinda useless as that's definetly code transpiled by jf's webpack config, so some code snippets or screenshots would be nicer

dmitrylyzo commented 1 year ago

webOS 1.2:

    function Rr(t, r, n) {
      var i = Ce();
      try {
        return ae(t)(r, n);
      } catch (o) {
        if (we(i), o !== o + 0) throw o;
        me(1, 0);
      }
    }
    function Dr(t, r, n, i, o) {
      var u = Ce();
      try {
        return ae(t)(r, n, i, o);    // TypeError: 'undefined' is not a function (evaluating 'ae(t)(r, n, i, o)') 
      } catch (p) {
        if (we(u), p !== p + 0) throw p;
        me(1, 0);
      }
    }
    function Wr(t, r, n, i) {
      var o = Ce();
      try {
        return ae(t)(r, n, i);
      } catch (u) {
        if (we(o), u !== u + 0) throw u;
        me(1, 0);
      }
    }
    f.getTempRet0 = tt, f.setTempRet0 = et;
    function Ir(t) {
      t.M();
    }
    var Ze = {
        a: Ur
      },
      We,
      J,
      Ne,
      me,
      et,
      tt,
      Ce,
      we;
    return (WebAssembly.instantiateStreaming ? WebAssembly.instantiateStreaming(fetch("jassub-worker-modern.wasm"), Ze) : WebAssembly.instantiate(f.wasm, Ze)).then(function (t) {
      C = (t.instance || t).exports, f._malloc = We = C.N, J = C.O, Ne = C.P, f.__embind_initialize_bindings = C.Q, me = C.S, et = C.T, tt = C.U, Ce = C.V, we = C.W, C.X, C.Y, C.Z, C._, ue = C.R, Ir(C), P();
    }), f.ready;
  }(f);

Chrome 79:

    function Lt(t, r, n) {
      f.hasOwnProperty(t) || ge("Replacing nonexistant public symbol"), f[t].overloadTable !== void 0 && n !== void 0 ? f[t].overloadTable[n] = r : (f[t] = r, f[t].argCount = n);
    }
    function Ht(t, r, n) {
      var i = dynCalls[t];
      return n && n.length ? i.apply(null, [r].concat(n)) : i.call(null, r);
    }
    var be = [];
    function ae(t) {
      var r = be[t];
      return r || (t >= be.length && (be.length = t + 1), be[t] = r = ue.get(t)), r;    // RangeError: WebAssembly.Table.get(): invalid index 584944 into function table
    }
    function Bt(t, r, n) {
      if (t.includes("j")) return Ht(t, r, n);
      var i = ae(r).apply(null, n);
      return i;
    }
    function xt(t, r) {
      var n = [];
      return function () {
        return n.length = 0, Object.assign(n, arguments), Bt(t, r, n);
      };
    }
    function q(t, r) {
      t = I(t);
      function n() {
        return t.includes("j") ? xt(t, r) : ae(r);
      }
      var i = n();
      return typeof i != "function" && _("unknown function pointer with signature ".concat(t, ": ").concat(r)), i;
    }

Does JASSUB 1.7.7 work in Chrome 76 for you?

ThaUnknown commented 1 year ago

the issue is some emscripten bug, I've downgraded emscripten and I think it should fix this issue, the chrome 76 bug is worrying, as that implies the C compiled code errored, which hasnt been changed in months

ThaUnknown commented 1 year ago

you can try npm install https://github.com/thaunknown/jassub and see if it errors

dmitrylyzo commented 1 year ago

I tested latest jassub/main (https://github.com/ThaUnknown/jassub/commit/36eb3d76ffa9195aa589673deae6e21312547aa2) in Chrome 79 - works.

Regarding webOS 1.2 and transpilation, there is some warning about using variables when constructing Worker.

Using a variable in the Worker constructor is not supported by webpack. For example, the following code will not work: const url = new URL('./path/to/worker.ts', import.meta.url); const worker = new Worker(url);. This is because webpack cannot analyse the syntax statically. It is important to be aware of this limitation when using Worker syntax with webpack.

Could this be the reason it isn't transpiled?

ThaUnknown commented 1 year ago

The worker is being transpiled by webpack because of the babel loader config as you saw yourself, the function and variable names are mangled, I think it's honestly some stupid emscripten bug, and it's starting to annoy me

ThaUnknown commented 1 year ago

@dmitrylyzo okay, with v1.7.8 I've 100% confirmed that legacy works again, I didn't have time to check if it worked previously or not but I'm sure it works now, if it still doesn't work then it's likely because of the transpilation done by babel

ThaUnknown commented 1 year ago

@dmitrylyzo could you test 1.7.8? if it still errors, any you get any errors, send them, I'm fairly sure it's gonna be caused by the wasm.js worker being broken by babel so I'll simply have to exclude it

dmitrylyzo commented 1 year ago

Worker (1.7.8, 1.7.9) now requires a polyfill for Array.prototype.copyWithin (webOS 1.2).

ThaUnknown commented 1 year ago

wth

ThaUnknown commented 1 year ago

minor oversight, fixed in 1.7.10

dmitrylyzo commented 1 year ago

minor oversight, fixed in 1.7.10

Now the same error as https://github.com/jellyfin/jellyfin-web/issues/4794#issuecomment-1732431567

TypeError: 'undefined' is not a function (evaluating 'ie(t)(r, n, i, o)')

Also, I have different output files when building from source (main branch) - it fails ES check (arrow-functions). It seems the build infra differs.

ThaUnknown commented 1 year ago

okay, so babel is definetly at fault here, if you copy over the worker.wasm.js file from jassub's dist and replace it in jellyfin-web's folder, tho you'd need to rename it to some hash that webpack uses for the file, and it should work

If this still doesn't work, then this is a JS engine error, as it runs correctly on newer chrome engines, and the error you referenced is emitted in the WASM emulation, which means somehow on older engines, the JS is executed differently, because the WASM steps should be the same, giga yikes

did you use docker to compile? the output should be the same

where is the ES check failed? it should fail in worker and main, but not in .wasm.js

dmitrylyzo commented 1 year ago

Tried 1.7.11 on webOS 1.2: TypeError: 'undefined' is not a function (evaluating 'ie(t)(r, n, i, o)')

Tried rebuilding (using run-docker-build.sh): Babel doesn't transpile the worker (arrow functions in the output). And again, the result of JASSUB rebuild differs from what is in the npm store. :man_shrugging:

ThaUnknown commented 1 year ago

I'll have some time this weekend to look into this, but from what you described: this is bad, really bad, in summary the JS engine webOS 1.2 uses evaluates things differently?

JASSUB doesn't do any transpilation, it's on the end developer to transpile the code JASSUB provides, which jellyfin already has a config for and does

at most the JASSUB rebuild should ONLY differ from the npm store with stuff like new lines, and nothing else, I cleaned out my ubuntu install, tried running this again, and I get the same output as what the NPM store has

give me a few hours and I'll look deeper

ThaUnknown commented 1 year ago

either there was something critically wrong with my wsl install, or I lack some basic understanding of how it works but I had a lot of issues with files being cached and not updated, not sure what happened, it's fixed

it's as I feared, babel is transpiling the JS files, and breaking the WASM, which is the root of the issue, I'm however at my wit's end as to how to disable that, static copy doesn't seem to work as expected, and the worker keeps changing, I tried changing this behavior but I keep failing, not sure why: https://github.com/ThaUnknown/jellyfin-web/tree/static-wasm.js left is what's outputed by webpack's static copy, right is original from node_modules image

ThaUnknown commented 1 year ago

TIL, production mode makes Terser run on ALL files even statically copied ones, and since it has dead code removal..... yeah you can imagine the rest

this was up there with some of the least fun things I've had to track down in recent weeks

https://github.com/jellyfin/jellyfin-web/pull/4971

@dmitrylyzo

ThaUnknown commented 1 year ago

fixed in jassub 1.7.13

ThaUnknown commented 12 months ago

@dmitrylyzo is this now resolved on jassub's end? or is there still something I need to fix? asking cuz I want to mark this as complete on my TODO list

dmitrylyzo commented 12 months ago

@dmitrylyzo is this now resolved on jassub's end? or is there still something I need to fix? asking cuz I want to mark this as complete on my TODO list

Yes, it is resolved. 🤞 At least it finally works on webOS 1.2. I am waiting for Renovate to update its PR.

dmitrylyzo commented 11 months ago

webOS 2 (it has a native Promise, but it is buggy - it should still be polyfilled): TypeError: PromiseResolver is not a function (evaluating 'h(l)')

var Module = function Module() {
  var l = arguments.length > 0 && arguments[0] !== undefined ? arguments[0] : {};
  return function (l) {
    var l = l,
      h;
    l.ready = new Promise(function (t, r) {
      h = t;
    });
    var C = function C(t) {
        return console.log(t);
      },
      y = function y(t) {
        return console.error(t);
      };
    function _() {
      h(l);    // TypeError: PromiseResolver is not a function (evaluating 'h(l)')
    }

Tizen 5 and webOS 5: Uncaught (in promise) CompileError: WasmCompile: Wasm decoding failed: unexpected section: Code @+4549

return (WebAssembly.instantiateStreaming ? WebAssembly.instantiateStreaming(fetch("jassub-worker-modern.wasm"), Ze) : WebAssembly.instantiate(l.wasm, Ze)).then(function (t) {

^ loading jassub-worker.wasm

ThaUnknown commented 11 months ago

I added a fix for engines which pretend that promise is supported in .15 [I don't understand how resolve in promise can not be a function], but I was not able to reproduce the webOS 5 error so I'm not sure whats wrong