[Jellyfin] Render ahead (oneshot render)

[dmitrylyzo]

Original author: @JustAMan

This PR adds a Render ahead mode that improves the accuracy of event timing by rendering a set of images in advance so that the frame is ready in time.
Jellyfin uses this mode.

I cherry-picked related commits, skipping merge-commits.

List of commits

848a686
65fbfec
5430cc9
4e42502
90910bd
21c84cc
6980deb
6d3a5c7
7fc34cf
cea2dc9
88d2df4
bd8bcab
ecd47dd
67947f3
9dd6eb2
e1604cb
Merge pull request #6 from JustAMan/improve-timing-accuracy
a8d670e
d3bc472
90d4848
a66e797
971a997
a166606
Merge pull request #8 from JustAMan/lite-mode
825c999
345d701
4c5f018
bcf4b5f
5256b63
Merge pull request #11 from JustAMan/limit-canvas-size
cc9575c
93f4d0f
0722559
Merge pull request #12 from JustAMan/fix-resize
7eefe74
595d0e2
Merge pull request #15 from JustAMan/update-build
c66e7b1
Merge pull request #19 from Teranode/master
0e9e792
9eac714

Changes
Add Render ahead mode.

[ThaUnknown]

@dmitrylyzo could you resolve conflicts so we can get this moving?

additionally, documenting some examples of using renderahead would be nice, maybe in the pages showcases?

[TheOneric]

Thanks again for working on this! Here are some remarks after just a first cursory look:

• Please indicate the original commits this was cherry-picked from in the commit-message as done in [Jellyfin] Old browsers #112.
• Needs to be rebased
• This seems to do much more than just "prerendering frames for later deployment"; it isn't immediately clear in all cases if those other changes (limiting ca
  nvas size, dropping animations, ...) are part of the oneshot render mode, or when they are active
• Can you clarify if the new rendering modes are also droppng frames (like lossyRender) or blending in WASM (like blendRender) instead of JS?
• All those modes and new options need to be documented in the README, but none currently are
  (tbf, the blendRender mode also wasn't documented until recently, but now it is: 9424e8f)
• At first glance this seens t limit the canvas size by default; I'm not sure we should do that by default.

[dmitrylyzo]

This seems to do much more than just "prerendering frames for later deployment"; it isn't immediately clear in all cases if those other changes (limiting canvas size, dropping animations, ...) are part of the oneshot render mode, or when they are active

iirc, it was tested on heavy animated subtitles and these features were born in between. They are too dependent on the oneshot render code base.
You can see the List of commits in the first post. I marked merge-commits in our fork.

Can you clarify if the new rendering modes are also droppng frames (like lossyRender) or blending in WASM (like blendRender) instead of JS?

This is a single mode and it uses blendRender.

JavascriptSubtitlesOctopus/src/post-worker.js

Line 260 in 61c8997

rendered = self.blendRenderTiming(eventStart, true);

It can erase animation tags, but I haven't tested that. Jellyfin doesn't use it. So it was probably only tested in the dev cycle.

Current Jellyfin options

https://github.com/jellyfin/jellyfin-web/blob/f4536f4865a6d3110db885159a95befbf0f14dfe/src/plugins/htmlVideoPlayer/plugin.js#L1075-L1085

                // new octopus options; override all, even defaults
                renderMode: 'blend',
                dropAllAnimations: false,
                libassMemoryLimit: 40,
                libassGlyphLimit: 40,
                targetFps: 24,
                prescaleTradeoff: 0.8,
                softHeightLimit: 1080,
                hardHeightLimit: 2160,
                resizeVariation: 0.2,
                renderAhead: 90

At first glance this seens t limit the canvas size by default; I'm not sure we should do that by default.

This?

JavascriptSubtitlesOctopus/src/subtitles-octopus.js

Lines 23 to 25 in 61c8997

	self.prescaleTradeoff = options.prescaleTradeoff || null; // render subtitles less than viewport when less than 1.0 to improve speed, render to more than 1.0 to improve quality; set to null to disable scaling
	self.softHeightLimit = options.softHeightLimit || 1080; // don't apply prescaleTradeoff < 1 when viewport height is less that this limit
	self.hardHeightLimit = options.hardHeightLimit || 2160; // don't ever go above this limit

if prescaleTradeoff is null (by default), hardHeightLimit is only used.

JavascriptSubtitlesOctopus/src/subtitles-octopus.js

Lines 705 to 708 in 61c8997

	if (height > self.hardHeightLimit) {
	width = width * self.hardHeightLimit / height;
	height = self.hardHeightLimit;
	}

This is probably done to avoid wasting time rendering too high quality subtitles.

[ThaUnknown]

I've been busy, so I wasn't able to leave feedback on this.

Is there any reason to force the use of blend render? It performs a lot worse than fast render since it can't benefit from hardware acceleration. I think it should inherit the rendering mode which was user specified on init, rather than force one.

Also from my testing SO performed worse with pre-rendering on releases that had >300 bitmaps per frame, but I'm not too sure if I made some sort of mistake, or if it simply can't handle it.

Shouldn't all the pre-render logic be handled on the worker? Doing it on the main thread seems very wasteful.

IMO, the default hard size cap should be set to 2k, for retina displays.

[TheOneric]

This? […] if prescaleTradeoff is null (by default), hardHeightLimit is only used.

Yes, this. Imho by default it should just use the physical canvas size without any cappping as this is the most intuituve behaviour.

They are too dependent on the oneshot render code base.

But the settings themselves do not depend on oneshotRender being used, right?

[ThaUnknown]

As this adds multiple options and changes&adds a lot of code, it would be great if I could get another opinion from a JSO-maintainer on this. A summary can be found below.

As a note for testing: Opening the browser-console makes rendering much slower due to the debug prints, which may trigger the (documented and planned to be improved) subtitle-freezes with renderAhead.

The bulk of the changes are backported jellyfin-commits, those add three separate things: 1.) renderAhead to prerender 2.) An option to remove all animating tags 3.) Options to bound the size (in pixels) of the rendering canvas (which will then be scaled by the browser to fit the physical size)

To my understanding it is difficult to split these up because the original commits are interspersed and depend on each other and the commit messages are not always accurately describing what it actually contains and they are definitely not always atomic. (but there's some worth in keeping close to the original commits as they are used in the jellyfin-fork for 1+ year). All of this makes them not only hard to split up but also a major pain to review. The backports are followed up by new commits fixing various issues found in testing and glancing over the code. It appears to pass all our samples from the gh-pages branch.

.

Future todos, probably fine for after this has been merged (in no particular order):

• Add a renderAhead sample to our example suit (possibly duplicate the AoT one so it's possible to compare renderAhead+blend with lossy)

• investigate whether we can make the WASM-blendRenderer the new default renderer; it seems as it should always be faster than the current default and afaik there are no frame-dropping or browser-incompatibility concerns with it unlike with the lossy "fast"-mode or renderAhead

• improve renderAhead's behaviour when falling behind the video (the goal is to not be worse than using a plain blend redererer)

• check if renderAhead can be made to work with other rendering-modes

• tiled blending patches from jellyfin

• DataHoarder's font loading improvements

• low-prio: simplify and improve the resizing logic for the prerendering cache

  • after that: maybe consolidate the (remaining) renderAhead related options into one obejct

• WASM blend render isn't worth it on high end hardware, it can't benefit from hardware accelerated canvas blending, so while it might offer insane results on low end hardware, it probably wont on high-end hardware like mine, I tested this on subtitles which had ~600-1k bitmaps per frame, which performed... okay on my threaded render, but almost not working on blend render
• a bit off-topic, but with how stuff is currently going in web, streaming should be a high priority, so we should probably investigate how this behaves on adding events, speaking of which, completely aren't functional createEvent broken by mutating strings #77 and if I remember correctly, renderAhead didn't work well if events were added after it thought there are no more events to pre-render

[dmitrylyzo]

if I remember correctly, renderAhead didn't work well if events were added after it thought there are no more events to pre-render

Yes, it won't restart until you drop the end marker (assuming events are appended).
Also, rescanAllAnimations or some more delicate function should be called to detect animation in the new event.

[ThaUnknown]

if I remember correctly, renderAhead didn't work well if events were added after it thought there are no more events to pre-render

Yes, it won't restart until you drop the end marker (assuming events are appended). Also, rescanAllAnimations or some more delicate function should be called to detect animation in the new event.

yeah, with how wonderful streaming is, it might not necessarily be appended, so even that wouldn't work, every time you add an event you'd need to reset the entire cache, which is counter productive

[TheOneric]

investigate whether we can make the WASM-blendRenderer the new default renderer; it seems as it should always be faster than the current default

WASM blend render isn't worth it on high end hardware, it can't benefit from hardware accelerated canvas blending, so while it might offer insane results on low end hardware, it probably wont on high-end hardware like mine […]

Defaults are supposed to work well everywhere, not just on some special hardware+software combination.

[ThaUnknown]

investigate whether we can make the WASM-blendRenderer the new default renderer; it seems as it should always be faster than the current default

WASM blend render isn't worth it on high end hardware, it can't benefit from hardware accelerated canvas blending, so while it might offer insane results on low end hardware, it probably wont on high-end hardware like mine […]

Defaults are supposed to work well everywhere, not just on some special hardware+software combination.

yes, that is what I'm saying, WASM blend won't work for high intensity TS

[TheOneric]

investigate whether we can make the WASM-blendRenderer the new default renderer; it seems as it should always be faster than the current default

WASM blend render isn't worth it on high end hardware, it can't benefit from hardware accelerated canvas blending, so while it might offer insane results on low end hardware, it probably wont on high-end hardware like mine […]

Defaults are supposed to work well everywhere, not just on some special hardware+software combination.

yes, that is what I'm saying, WASM blend won't work for high intensity TS

Maybe I should've been more clear: Defaults should work reasonably well on all devices and engines. WASM-Blending does and so far seems to always be better then the current default. fast meanwhile has serious problems on many devices and engines dropping frames and in some cases even being slower than the current default.

[ThaUnknown]

investigate whether we can make the WASM-blendRenderer the new default renderer; it seems as it should always be faster than the current default

WASM blend render isn't worth it on high end hardware, it can't benefit from hardware accelerated canvas blending, so while it might offer insane results on low end hardware, it probably wont on high-end hardware like mine […]

Defaults are supposed to work well everywhere, not just on some special hardware+software combination.

yes, that is what I'm saying, WASM blend won't work for high intensity TS

Maybe I should've been more clear: Defaults should work reasonably well on all devices and engines. WASM-Blending does and so far seems to always be better then the current default. fast meanwhile has serious problems on many devices and engines dropping frames and in some cases even being slower than the current default.

lossy performs well on 70% of cases, according to 2021's browser market shares [chrome, edge, firefox], from my testing WASM blending had much more mixed results than fast or even normal, I'd have to re-run these tests after my performance improvements vs this new blend, because the current normal or fast performance is a joke

Edit: the way I do it in my version of SO, is default to threaded offscreen fast, with fallbacks:
fast offscreen > fast > blend
offscreen blend > blend
offscreen normal > normal

Edit2: admittedly this data is biased, but it should be up to the developer to analyze their target audience, and choose the best mode based on that, but by default, we should target the biggest userbase, especially considering how big of an issue heavy TS is for this library, and what kind of reputation it gave this library [blame CR]

[ThaUnknown]

so what's currently stalling this?

[JustAMan]

lossy performs well on 70% of cases, according to 2021's browser market shares [chrome, edge, firefox], from my testing WASM blending had much more mixed results than fast or even normal, I'd have to re-run these tests after my performance improvements vs this new blend, because the current normal or fast performance is a joke

As an original author of some of these things, I would say that WASM blend should perform much better than browser-based on the cases I was mainly optimising it for - older browsers and funny engines like WebOS. I was specifically trying to make it fast on ancient Chrome like 27 or so hoping that new hardware and software would do better anyway 🙃

That said, I would suggest to default to WASM blend on some old-ish browsers (like, those shipped earlier than, say, 2019 or smth) or when hardware acceleration is turned off.

Ideally it should be decoupled from rendering ahead, though, so be able to render subs in sync with video (instead of hoping that rendering one frame would be so fast no one would notice a lag).

And thanks a million to @dmitrylyzo to taking this up!

[ThaUnknown]

A weird thing I also noticed with fast render, is while total render time [not sum of all frame time, but last frame end time - first frame start time ] is faster, single frame render is in many cases slower, most likely because of async? I'm not too sure

[heyaco]

Merging?

[TheOneric]

[re. WASM-blending as possible new default]

As an original author of some of these things, I would say that WASM blend should perform much better than browser-based on the cases I was mainly optimising it for - older browsers and funny engines like WebOS. I was specifically trying to make it fast on ancient Chrome like 27 or so hoping that new hardware and software would do better anyway 🙃

Thanks for the reply!
On on modern engines WASM-blending should also be faster or equal to than the current default of JS-blending, right? At least my very limited test so far suggests that; have you noticed any indication otherwise?

And thanks a million to @dmitrylyzo to taking this up!

Thanks to both you for originally writing this and dmitrylyzo for porting and fixing it. :)

[TheOneric]

I'm waiting for @rcombs to take a look at the patches, after which this may either need more mending, be merged or need to be split up more afterall. Be patient, as this is quite a lot and additionally hard to review. (Now also marking this in the GitHub-UI)

[rcombs]

This PR remains too large to review practically in a single shot. Furthermore, some of the later commits are fixes for bugs introduced in previous commits (e.g. 10c35c5 appears to fix a bug introduced in or after 95bff90), meaning that the repo state is broken between commits, which makes this unsuitable for merge as-is. Never break functionality in one commit and then fix it in another within the same PR; just rebase and fix the broken commit.

Some changes from this are not actually relevant to this branch, and can be broken up into smaller PRs for separate review and merge review; for instance:

• All style cleanup (whitespace changes, etc) is separable from actual code changes
• Changes to default settings (e.g. frame rate 30->24) are separable from everything else

Furthermore, some of these commits refactor code that was added in previous commits within the same PR. This makes commit-by-commit review effectively useless. For instance, f38fc7e deletes large amounts of code added in 69906fc. Please squash your commits down appropriately.

Meanwhile, many of these commits add both base-level infrastructure and calls into it in the same commit. This makes review more difficult, and also would make future bisects harder. Implement new C++ functionality in one commit, then call it from JS in another, with each split up into as many commits as appropriate (for instance, adding a new class should usually be a commit on its own).

[dmitrylyzo]

Most of the commits are not mine - I am just porting a feature from another repo and trying to keep the original authorship of the code.

FPS change was added after #111 (comment)

[ThaUnknown]

Most of the commits are not mine - I am just porting a feature from another repo and trying to keep the original authorship of the code.

FPS change was added after #111 (comment)

I think the original commit structure doesn't matter, just what what does, as long as you put justaman as a co-author it should be fine, but from what I talked about with rcombs, a feature/functionality shouldn't be split into more than 1 commit, or more simply the same code can't be changed in more than 1 commit, and unfortunately this won't be merged until those requirements are met ;-;

so if I was you I wouldn't worry that much about the original commit structure, just focus on readability

[JustAMan]

I'm fine with any editing of the commits, I can recede the "code ownership/authorship" to anyone wanting to maintain this if this helps going further.

I did this in the first place "for the greater good", not for the fame or anything.

[TheOneric]

Most of the commits are not mine - I am just porting a feature from another repo and trying to keep the original authorship of the code.

There's some value in keeping a close mapping to the original history since it was already deployed publicly for over a year; that's why this form wasn't rejected immediately. However, the non-atomicity and interleaving of multiple changes is too detrimental to the maintainability and bisectability in the long term, while in the short term it makes it very hard to properly review (as the long response times show), making bugs slipping in more likely (we already identified & fixed several bugs, possibly there are even more) and hampering a merge.
Thus, it was decided to forgo preservation of the original history and insist on the general requirements for patches also here.
As for the matter of authorship, there's the widely recognised Co-Authored-By: commit message trailer which can be used to indicate that multiple people worked on a change, with the original or largest contributor for a particular commit being granted the primary authorship. (I just noticed, you already used this in one commit.)

.

I think, now that we're no longer trying to emulated the original jellyfin-history, this can be split into at least four series (in whatever order is most convenient):

• renderAhead mode; possibly also including the proposed changes to its behaviour when falling behind ( + a reminder or patch to add it to the sample suite)
• the option to bound the canvas size
• the option to drop animations
• change fps default, make the WASM-blendRenderer the new default mode and other default changes if there were any

Thanks again for your work on this JustAMan and dmitrylyzo!

---

Just so it isn't buried under all the comments posted since, here are again the future todos previously identified. The first three (as noted above) and the consolidation of renderAhead-options would probably be best integrated into the newly split series. I don't recall otoh what the resize logic improvements were about, if needed they can be another series before the renderAhead one, to avoid introducing option that change shortly after.

Future todos, [...] (in no particular order):

• Add a renderAhead sample to our example suit (possibly duplicate the AoT one so it's possible to compare renderAhead+blend with lossy)

• investigate whether we can make the WASM-blendRenderer the new default renderer; it seems as it should always be faster than the current default and afaik there are no frame-dropping or browser-incompatibility concerns with it unlike with the lossy "fast"-mode or renderAhead

• improve renderAhead's behaviour when falling behind the video (the goal is to not be worse than using a plain blend redererer)

• check if renderAhead can be made to work with other rendering-modes

• tiled blending patches from jellyfin

• DataHoarder's font loading improvements

• low-prio: simplify and improve the resizing logic for the prerendering cache

  • after that: maybe consolidate the (remaining) renderAhead related options into one obejct

[dmitrylyzo]

After squashing, you lose all messages of intermediate commits and probably info about why is that. just to warn

Bisecting: there is probably not much difference between a single (squashed) commit and a merge commit when using the --first-parent option (git 2.29).

The Co-Authored-By: doesn't specify the exact part of the code that the co-author touched. I need to know who to blame (even if it's me).
The one that exists is because TheOneric reviewed the PR and I just squashed a new commits.

dropAnimation feature is a bonus to the animation detector (parser).

Since I will port using the sameoriginal, then fixes pattern, I think you won't accept the PR with the canvas bounding feature as well.
f2c1fea
35efbb9
a602fd7
c2b711a
part of 40da83b

I think you need someone with a more flexible mind (to port all of this).

EDIT:
I can squash some minor commits to where the relevant code appeared first time or an error was introduced, but only if the author is the same.
3be4c2b
f7e0be8
2e93f95
7fefc36
87a7249
f54f72a

[TheOneric]

dropAnimation feature is a bonus to the animation detector (parser).

Adding the animation detection and the option first and then later reusing the detection for renderAhead brings the same end result, but as a bonus takes some bulk away from the renderAhead series.

Bisecting: there is probably not much difference between a single (squashed) commit and a merge commit when using the --first-parent option (git 2.29).

There is a significant difference in all but single-commit merges.
With --first-parent the whole merge is effectively a single (enormous) commit, if the commits making up the merged series are not working properly individually, the series itself cannot be bisected.

A proper patch series consists of well-documented atomic commits, meaning:

• each commit compiles individually
• each commit is, at the time of merge, assumed to be correct by itself
• the state of the whole repo makes sense after each individual commit
• there are no temporary user-visible behaviour changes within a series
• each commit is as concise/small as possible without breaking any other requirements
  (among other that means, refactors are split from the actual code changes)
• each commit with a non-trivial change explains in its commit message what it does, why and what assumptions and/or considerations went into the specific implementation of the change

This not only makes review easier, it also means the patch-series itself can be bisected and when you get a result, the amount of code needed to analysse for the source of the bug is much smaller and you have plenty of explanation for what it wants to do and why it was done the way it is, helping in understanding the code and making it less likely new bugs are introduced in an attempt to "fix" the bug currently at hand.
Using --first-parent enables none of this.

Sometimes this can mean the commit message is longer than the actual code change like here. For an example of how the commits are supposed to be split up (but all commits are almost trivial) see the renderMode option unification I split out of this series as an example: #118.

The Co-Authored-By: doesn't specify the exact part of the code that the co-author touched.

If relevant this info can be added to the commit message.

I need to know who to blame (even if it's me).

Pointing fingers to blame when a bug is found is not productive. Fixing the bug is.
Properly documented atomic commits make it easy to locate regressions and to understand how they came about.
An amalgamation of potentially non-compiling and individually bugged commits, makes all of identifying, understanding and fixing harder. Willingly making the code harder to maintain just to make finger-pointing easier is not merely unproductive but detrimental.

Often having small atomic commits also means there's only one author per commit, and you'd also “know who to blame”. Here we don't as the original implementation does not consist of atomic commits and is known to be buggy. Keeping the code maintainable and as easy to fix and bisect as possible is more important than “know[ing] who to blame”.