Unify renderMode options and add missing documentation

[TheOneric]

The renames are not in line with what is currently used in jellyfin. However fast doesn't make much sense for the lossy mode and normal also isn't a good name for the JS-blending mode, especially since we want to change the default soon.

[TFSThiagoBR98]

LGTM

[dmitrylyzo]

Real default targetFps:

JavascriptSubtitlesOctopus/src/post-worker.js

Line 9 in c7eeea0

self.targetFps = 30;

But it can also be specified here (as in that renderAhead PR):

JavascriptSubtitlesOctopus/src/subtitles-octopus.js

Line 19 in c7eeea0

self.targetFps = options.targetFps || undefined;

just to unify the options and not to clog that feature

[TheOneric]

Real default targetFps:

// src/post-worker.js, line 9:
self.targetFps = 30; 

Can you elaborate on what you think should be changed? The doc-commit does list 30 as the default.

[dmitrylyzo]

Can you elaborate on what you think should be changed? The doc-commit does list 30 as the default.

https://github.com/jellyfin/JavascriptSubtitlesOctopus/blob/f4625ac313b318bd5d2e0ae18679ff516370bae6/src/subtitles-octopus.js#L22

But that's because renderAhead started using targetFps in the subtitles-octopus.js.

[TheOneric]

[Later commits of #111 setting the default already in subtitles-octopus.js.] But that's because renderAhead started using targetFps in the subtitles-octopus.js.

I'm afraid I am still not sure what change you are suggesting. The only commit relating to targetFps here is the documentation-fix. It lists 30 as the default value. While in src/subtitles-octopus.js the fallback is to undefined, the value is not used in src/subtitles-octopus.js (yet) and in src/post-worker.js a value of undefined will be made to fall back to 30, so 30 should indeed be the default value. Did I miss something?

Do you perhaps, want the default value already being set in src/subtitles-octopus.js? If so, I already planned to do this in the follow-up series to this updating the default values for consistency with other options; but this has no place in a commit adding missing documentation.

[dmitrylyzo]

Do you perhaps, want the default value already being set in src/subtitles-octopus.js?

Yes.

-    self.targetFps = options.targetFps || undefined;
+    self.targetFps = options.targetFps || 30;

But it is not currently used in subtitles-octopus.js, so it is probably not necessary to change this now.

If so, I already planned to do this in the follow-up series to this updating the default values for consistency with other options; but this has no place in a commit adding missing documentation.

👍

[dmitrylyzo]

@TheOneric If renderMode is not valid, the renderer runs discontinuously, probably trying to output an error message.
Just noticed this while I was porting the empty fix.
Maybe it would be better to fix the mode in the options block?

You may also be interested in https://github.com/WeebDataHoarder/JavascriptSubtitlesOctopus/blob/ea80135ebe80d3fae85f0a88e5f38801335d563c/src/subtitles-octopus.js#L5-L8
WeebDataHoarder@ea80135

[TheOneric]

If renderMode is not valid, the renderer runs discontinuously, probably trying to output an error message.

When I tested this the error message was only printed on intital page load, resize and playback resume after pause. While playback was active I didn't get any more error messages.

Maybe it would be better to fix the mode in the options block?

That would require duplicating the full list of valid renderModes, which is not great. A simpler way to avoid repeated errors is to just override the stored renderMode after outputting the message. (Or alternatively if changing the renderMode during playback is not supported or planned to be supported: run getRenderMethod only once during init and store the result.)

[dmitrylyzo]

When I tested this the error message was only printed on intital page load, resize and playback resume after pause. While playback was active I didn't get any more error messages.

I didn't have them either - it was quietly lagging. Only 4 times at the start.

That would require duplicating the full list of valid renderModes, which is not great.

How about a lookup?

self.renderModes = {
    'js-blend': self.render,
    'lossy': self.lossyRender,
    'wasm-blend': self.blendRender
};

I think renderMode can also be defined with a custom setter where you can check the mode - if we need to change it on-the-fly.

[TheOneric]

You may also be interested in https://github.com/WeebDataHoarder/JavascriptSubtitlesOctopus/blob/ea80135ebe80d3fae85f0a88e5f38801335d563c/src/subtitles-octopus.js#L5-L8
WeebDataHoarder/JavascriptSubtitlesOctopus@ea80135

The comment claiming Safari not supporting the function doesn't seem to be accurate (anymore), assuming Safari is using up-to-date WebKit. With WebKitGTK 2.34.4 createImageBitmap is a function calling native code:

> createImageBitmap
< function createImageBitmap() {
    [native code]
}
> typeof createImageBitmap
< "function"

Is there another reason lossy doesn't work in Safari (meaning the check would have to be different) or does this only apply to old versions and other BrowserEngines as used eg by SmartTVs? Fwiw with the aforementioned WebkitGTK the Brotli sample using the lossy renderer plays fine for me.

[dmitrylyzo]

https://webostv.developer.lge.com/discover/specifications/web-engine/
webOS 1.2 emulator (WebKit 538.2) - no Promise, no createImageBitmap.
webOS 2.0 emulator (WebKit 537.41) - has Promise (#122 (comment)), no createImageBitmap.
webOS 3.0 emulator (Chrome 38) - has Promise, no createImageBitmap.
webOS 4.0 emulator (Chrome 53) - has Promise, has createImageBitmap.

It is also better to check window.createImageBitmap. Otherwise you get ReferenceError: Can't find variable: createImageBitmap.
Or use try/catch. Using typeof fixes that.