Make attachWorker return a promise
#1374
Conversation
peaBerberian
added
Priority: 0 (Very high)
peaBerberian
changed the title
attachWorker return a promise by defaultattachWorker return a promise
There was a problem hiding this comment.
I am wondering about the relevance of having attachWorker a public method.
An application should always call the following attachWorker with the same arguments if it intents to play in multithread mode.
this.player.attachWorker({
workerUrl: EMBEDDED_WORKER,
dashWasmUrl: EMBEDDED_DASH_WASM,
});Maybe we could include this step into the addFeature(MULTI_THREAD) or in the loadVideo(), to make the API simpler to use.
However doing this change would not let the opportunity for the application to
get the attachWorker promise result. And so, the automatic fallback on main thread you were talking about seems to be required if we decide to do this change.
src/core/api/public_api.ts
Outdated
| if (typeof Worker !== "function") { | ||
| log.warn("API: Cannot rely on a WebWorker: Worker API unavailable"); | ||
| return rej(new WorkerInitializationError("INCOMPATIBLE_ERROR", | ||
| "Worker unavailable")); | ||
| } else if (!hasWebassembly) { | ||
| log.warn("API: Cannot rely on a WebWorker: WebAssembly unavailable"); | ||
| return rej(new WorkerInitializationError("INCOMPATIBLE_ERROR", | ||
| "WebAssembly unavailable")); | ||
| } else { | ||
| const blobUrl = URL.createObjectURL(workerSettings.workerUrl); | ||
| this._priv_worker = new Worker(blobUrl); | ||
| URL.revokeObjectURL(blobUrl); | ||
| } | ||
|
|
||
| this._priv_worker.onerror = (evt: ErrorEvent) => { | ||
| this._priv_worker = null; | ||
| log.error("Unexpected worker error", | ||
| evt.error instanceof Error ? evt.error : undefined); | ||
| }; | ||
| log.debug("---> Sending To Worker:", MainThreadMessageType.Init); | ||
| this._priv_worker.postMessage({ | ||
| type: MainThreadMessageType.Init, | ||
| value: { | ||
| dashWasmUrl: workerSettings.dashWasmUrl, | ||
| logLevel: log.getLevel(), | ||
| sendBackLogs: isDebugModeEnabled(), | ||
| date: Date.now(), | ||
| timestamp: getMonotonicTimeStamp(), | ||
| hasVideo: this.videoElement?.nodeName.toLowerCase() === "video", | ||
| hasMseInWorker, | ||
| }, | ||
| }); | ||
| log.addEventListener("onLogLevelChange", (logLevel) => { | ||
| if (this._priv_worker === null) { | ||
| return; | ||
| if (typeof workerSettings.workerUrl === "string") { | ||
| this._priv_worker = new Worker(workerSettings.workerUrl); |
There was a problem hiding this comment.
there can be one less nesting level here because the if and else if act as early return
An application only has to do it once per instance. And it allows to have a Worker per instance, which I thought mapped nicely. Doing it at Doing it on Overall, I find the |
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
force-pushed
the
feat/attach-worker-prom
branch
from
77a389a
to
b2c8716
Compare
You are right, it need to be done one time by instance of the class. What about doing it in the constructor |
If both were added as feature, yes. This is how it works:
So the only difference in multithread mode today is that we will also reject the Promise of |
This would mean that the feature has to be added before the construction and that you could not lazily add a worker, which I thought was too restrictive. |
|
All other RxPlayer features today don't have to be added before construction. They just need to be added before you rely on them (e.g. you could only add a Even with |
Ok thanks, I understand better why it's done that way, changing that was not the scope of that PR anyway :) |
Last week, we've seen an issue on some Samsung and LG TVs when relying on the RxPlayer new experimental
MULTI_THREADfeature 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_THREADfeature 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
attachWorkermethod.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_THREADfeature taking, I was able to find an adapted solution still compatible with returning a Promise on theattachWorkerAPI.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
attachWorkerfails, the RxPlayer will remove the worker from its state (newloadVideocalls 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
attachWorkerbefore 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.