Modernize library, bugfixes, improvements

[ThaUnknown]

! THIS IS NOT A FINAL PRODUCT !

This PR includes changes from 4 different community branches, unifying this is difficult:

• JustAMan changes: blendRender, oneshotRender, fixes
• jellyfin changes: blendRender, oneshotRender, fixes
• WeebDataHoarder changes: font handling improvements
• my changes:
  • re-wrote entire library to use ES6 class constructors [meaning the base requirement for the lib is now ES6]
  • re-organized structure
  • many, many bugfixes
  • removed unused functionality
  • greatly improved performance on the JS side, mainly the bitmap building
  • optimized/re-wrote pretty much all functions
  • added offscreen rendering to fully render images on canvases on the worker, instead of the main thread

I struggled to get this to compile, since this includes compile configs from 3 different repos, help on that would be nice

[TFSThiagoBR98]

@ThaUnknown thanks for the contribution, you can check my fork for some changes from then.

[ThaUnknown]

@ThaUnknown thanks for the contribution, you can check my fork for some changes from then.

TFSThiagoBR98@3d87bc3
this doesnt actually work correctly, it fails to preload the files and doesn't end up rendering anything

also if you know how to actually get this to compile, feel free to help

[TFSThiagoBR98]

this doesnt actually work correctly, it fails to preload the files and doesn't end up rendering anything

I reverted it, thanks

[TFSThiagoBR98]

also if you know how to actually get this to compile, feel free to help

Your dockerfile is broken, see 128c6f9

[ThaUnknown]

also if you know how to actually get this to compile, feel free to help

Your dockerfile is broken, see 128c6f9

our branches arent inline, also i think its more than just 1 file that's wrong here

I don't really have a way of testing the compilation, I just took datahoarder's compiled worker and swapped my changes in for testing, so in theory compiling this should work just fine

edit: that is defo the case here, it seems like you're still using libass 9? did you even try running your version of SO?

[TheOneric]

I don't really have a way of testing the compilation

You can compile locally or use the CI builds. Eg here you see your PR failing to build: https://github.com/libass/JavascriptSubtitlesOctopus/pull/106/checks?check_run_id=3118340661
However, since you don't yet have any commits in this repo and thanks to the crypto-mining CI-abuse earlier this year, your CI runs on this PR need to be manually approved. So it'd be better if you activate GHA-runs on your own fork-repo (per default disabled for forks) and use this for testing.

our branches arent inline, also i think its more than just 1 file that's wrong here

Yes, just from the commit-messages, there appear to be a lot of redundant and obsolete commits¹ in this patch-series, eg repeated bumps of the same dependency; some things were already merged iinm, or commits adding obsolete workaround for already fixed emscripten bugs.
Bumping the dependencies is likely trivial and can be done at any time, or did you or the other authors encounter issues with this?

[ThaUnknown]

I don't really have a way of testing the compilation

You can compile locally or use the CI builds. Eg here you see your PR failing to build: https://github.com/libass/JavascriptSubtitlesOctopus/pull/106/checks?check_run_id=3118340661
However, since you don't yet have any commits in this repo and thanks to the crypto-mining CI-abuse earlier this year, your CI runs on this PR need to be manually approved. So it'd be better if you activate GHA-runs on your own fork-repo (per default disabled for forks) and use this for testing.

our branches arent inline, also i think its more than just 1 file that's wrong here

Yes, just from the commit-messages, there appear to be a lot of redundant and obsolete commits¹ in this patch-series, eg repeated bumps of the same dependency; some things were already merged iinm, or commits adding obsolete workaround for already fixed emscripten bugs.
Bumping the dependencies is likely trivial and can be done at any time, or did you or the other authors encounter issues with this?

the issue is, this PR includes compile changes/configs from 2 different repos, kinda meshed together, I tried joining it with no avail, but I might just end up reverting all the changes made to the libass repo, or just hope some1 here will fix it themselves XD

these changes work, and there is a lot to gain, but managing all this myself with not very much knowledge to go on, is slowly getting to my sanity, that's why I asked for help with it

again, these are changes from many community branches, unifying which isn't easy, but since no1 was getting around to doing anything towards adding these changes, I did

I'm a web developer, I can write good js, but outside of that I rapidly start loosing confidence in what I'm doing xD

[TheOneric]

this PR includes compile changes/configs from 2 different repos, kinda meshed together, I tried joining it with no avail
[…] managing all this myself with not very much knowledge to go on, is slowly getting to my sanity

Yes, this is a lot of work. Doing this right, all at once is probably too much for any single person due to shear amount of different changes. I'd suggest splitting this into several thematic patch-series one-after-another, eg one for all blendRender-changes, one for the oneShotRenderer, one for all bug fixes, another one for the font-loading-stuff, … and at last (since it depends on the presence of the others) the ES6 rewrite.
This makes each part easier to manage and by decoupling the parts they could also be done by different people with less coordination overhead than multiple people attempting to work on a single enormous patch-series.

[ThaUnknown]

this PR includes compile changes/configs from 2 different repos, kinda meshed together, I tried joining it with no avail
[…] managing all this myself with not very much knowledge to go on, is slowly getting to my sanity

Yes, this is a lot of work. Doing this right, all at once is probably too much for any single person due to shear amount of different changes. I'd suggest splitting this into several thematic patch-series one-after-another, eg one for all blendRender-changes, one for the oneShotRenderer, one for all bug fixes, another one for the font-loading-stuff, … and at last (since it depends on the presence of the others) the ES6 rewrite.
This makes each part easier to manage and by decoupling the parts they could also be done by different people with less coordination overhead than multiple people attempting to work on a single enormous patch-series.

the only thing that doesn't work is the build because of data hoarders changes to font handling, everything else seems fine

[ThaUnknown]

@TheOneric after making a sizable dent in my desk from getting angry at inanimate objects, I decided to revert to the latest working commit which actually built, since I spent 3 days trying to add changes which took you guys less than a day to implement. TLDR it's my way of saying I fucking give up, feel free to clone this to another branch and fix the build changes, since really it's all that's left.

If you wish to test these changes, performance and general handling, you can do so in my player [just play Tears of Steel], and add your own ASS subtitles by running client.subtitleData.renderer.setTrack('Subtitles') in console.
If you have a WRTC compatible torrent client you can also just play back your own torrent. [Keep in mind that site also includes a torrent client and a custom video player alongside the subtitle renderer, so performance might be a bit worse than what you'd expect from a standard video player]

If you want more documented changes about my code, just feel free to ask, tho I've made sure the structure is a bit cleaner than what we had previously so it at least should be somewhat readable.

[ThaUnknown]

@TheOneric guessing I won't get any help with this? should I close it?

[TheOneric]

guessing I won't get any help with this?

Well, I'm not that familiar with advanced JS or the specifics of emscripten, so I couldn't review it anyway.
But as I told you before, this PR is simply too much at once, even if we were to ignore the fact, that it has conflicts with master making it unmergeable, throwing several addition, bugfixes and a complete rewrite together into one patch-series just creates a huge mess that's hard (i.e. near impossible) to properly review or understand. By extension, fixing errors in such a huge series is even harder.
But this doesn't mean it's impossible to get the improvements to master; the well-established solution to this is to split the problem and create several consecutive PRs for each category. This way you won't have to deal with quite as many problems at once and it's easier to understand and help you with it for reviewers. Eg from the listing in your initial post, I'd suggest the following:

• Start with a PR which includes everything (change, addition, bugfix) relating to the blendRenderer from all 4 repositories — excluding stuff that is specific to or relying on the rewrite. But nothing more than that!
• After the previous PR got merged, do the same for oneShotRenderer.
• Then once this is sorted out the same procedure for the font-handling changes.
• Any other bugfixes not specific to or relying on the rewrite can be done now or already earlier in between the above mentioned PRs
• Then finally, the rewrite.

For all PRs do not include changes to dist/ – it no longer exists, only blows up the diff, is a nuisance in review and creates merge conflicts – or include commits updating or fixing the build of $dependecy – we already bumped everything to the current newest releases.
This keeps every PR focused on a single topic making everything much easier and more likely to be helped with, reviewed and merged. (And perhaps if we're lucky, the original authors may then also be willing to help with review and polishing for the PRs corresponding to mostly their own patches)

---

If you wish to test these changes, performance and general handling, you can do so in my player [just play Tears of Steel], and add your own ASS subtitles by running client.subtitleData.renderer.setTrack('Subtitles') in console.

Just testing some mytery binaries, isn't a proper patch review but fyi: Doesn't work for me in Firefox-ESR 78 (which supports all of ES6 except tail-recursion, which according to this table is currently only supported in WebKit-based Browsers like Safari). Not sure if that's due to JSO-changes or some other component. If it is related to JSO, this is a problem.

[ThaUnknown]

guessing I won't get any help with this?

Well, I'm not that familiar with advanced JS or the specifics of emscripten, so I couldn't review it anyway.
But as I told you before, this PR is simply too much at once, even if we were to ignore the fact, that it has conflicts with master making it unmergeable, throwing several addition, bugfixes and a complete rewrite together into one patch-series just creates a huge mess that's hard (i.e. near impossible) to properly review or understand. By extension, fixing errors in such a huge series is even harder.
But this doesn't mean it's impossible to get the improvements to master; the well-established solution to this is to split the problem and create several consecutive PRs for each category. This way you won't have to deal with quite as many problems at once and it's easier to understand and help you with it for reviewers. Eg from the listing in your initial post, I'd suggest the following:

• Start with a PR which includes everything (change, addition, bugfix) relating to the blendRenderer from all 4 repositories — excluding stuff that is specific to or relying on the rewrite. But nothing more than that!
• After the previous PR got merged, do the same for oneShotRenderer.
• Then once this is sorted out the same procedure for the font-handling changes.
• Any other bugfixes not specific to or relying on the rewrite can be done now or already earlier in between the above mentioned PRs
• Then finally, the rewrite.

For all PRs do not include changes to dist/ – it no longer exists, only blows up the diff, is a nuisance in review and creates merge conflicts – or include commits updating or fixing the build of $dependecy – we already bumped everything to the current newest releases.
This keeps every PR focused on a single topic making everything much easier and more likely to be helped with, reviewed and merged. (And perhaps if we're lucky, the original authors may then also be willing to help with review and polishing for the PRs corresponding to mostly their own patches)

If you wish to test these changes, performance and general handling, you can do so in my player [just play Tears of Steel], and add your own ASS subtitles by running client.subtitleData.renderer.setTrack('Subtitles') in console.

Just testing some mytery binaries, isn't a proper patch review but fyi: Doesn't work for me in Firefox-ESR 78 (which supports all of ES6 except tail-recursion, which according to this table is currently only supported in WebKit-based Browsers like Safari). Not sure if that's due to JSO-changes or some other component. If it is related to JSO, this is a problem.

at most you can really only split this into 2 PR's, any more than that and you're just duplicating the work you're gonna do, I'm honestly not willing to do the exact same work 4-5 times, especially for shit like blend render which I didn't even work on, or oneshotrender which I think never actually worked [XD], noone else, not even the original authors were willing to add these changes.

I still don't understand the reasoning for you removing the /dist folder, all it does is make the library close to inaccessible for many web devs, and cdn's, what's worse, your package.json still announces that those files exist, which makes this situation even worse. nvm this exists on npm

What exactly doesn't work for you? I tested everything except safari, even on mobile and didn't run into any issues. If you mean the standalone binaries, then did you configure the instance correctly? Do you mean the compiling? Do you mean the player itself?

[TheOneric]

I still don't understand the reasoning for you removing the /dist folder

It contains binaries not code which doesn't belong into a source repo and only as previously mentioned: blows up diffs to manifold the actual change with superfluous changes, creates pointless merge conflicts, is a nuisance in review. The package.json also specifyies that the build script needs to be run as preparation. After the build the folder does exists and gets pacakaged properly.
Nightly binary builds are available as GH-build-artifacts. There's literally no benefit but many disadvantges to dragging dist/ around in the repo.

The blendRenderer and oneHsotRenderer are independent from each other and the font-stuff, right? Then splitting only reduces your and the review work if you're doing it sequentially as I suggested and not in parallel which would indeed waste effort. But if everything is bundled in one amalgamation then it's no surprise the original authors aren't inclined to help with it (and most of it won't be their work anyway).

---

Doesn't work for me in Firefox-ESR 78
What exactly doesn't work for you?

Nothing happens when I press a button, the console shows some errors.

[ThaUnknown]

I still don't understand the reasoning for you removing the /dist folder

It contains binaries not code which doesn't belong into a source repo and only as previously mentioned: blows up diffs to manifold the actual change with superfluous changes, creates pointless merge conflicts, is a nuisance in review. The package.json also specifyies that the build script needs to be run as preparation. After the build the folder does exists and gets pacakaged properly.
Nightly binary builds are available as GH-build-artifacts. There's literally no benefit but many disadvantges to dragging dist/ around in the repo.

The blendRenderer and oneHsotRenderer are independent from each other and the font-stuff, right? Then splitting only reduces your and the review work if you're doing it sequentially as I suggested and not in parallel which would indeed waste effort. But if everything is bundled in one amalgamation then it's no surprise the original authors aren't inclined to help with it (and most of it won't be their work anyway).

Doesn't work for me in Firefox-ESR 78
What exactly doesn't work for you?

Nothing happens when I press a button, the console shows some errors.

yeah, my bad on the dist folder, I didn't realize it was built for NPM

no not really, both font stuff and blendrender and oneshotrender modify the cpp wrapper, so I'd have to unmerge those changes manually, split them, then re-add them, thats like x3 work

more than "some errors" would be nicer, what exactly are you doing, are you playing your own torrent or one of the built in ones? and the player that's shown there uses ES8 because it has some crazy fancy stuff in it, so if you want to test only SO es6 then you'd probably need to set up something by hand

[ThaUnknown]

also, I think if this was ever to be accepted it should be a new major version, since it changes it so much, I'll try to resolve as many conflicts as I can where I'm sure it won't break anything

[TheOneric]

no not really, both font stuff and blendrender and oneshotrender modify the cpp wrapper, so I'd have to unmerge those changes manually, split them, then re-add them, thats like x3 work

At least the font-related changes and the sum of the two rendere changes prior to "rewrite" are already sequentially in the commit history. Splitting those two is no more work than branching of and a single git-command to rewind. Same goes for "rewrite" and the rest. Potentially there are a few fixes that need to be pulled in out of order, bzt from the commit-titles this appears to be only a few.
If oneShot- and blendRenderer were interleaved in the commit history I guess you could maayybe leave them together, but the final decision on this is for the qualified reviewers (i.e. rcombs and TFSThiago) to make.

btw, some of the changes (already prior to "rewrite") drop features, you'll likely want to drop those commits (as well as "update/fix $dependecy"- and "bump emscripten"-commits).

---

Off-topic for this PR, but:

the player that's shown there uses ES8

It appears Firefox-ESR 78 also fully supports ES8 and also ES7.

more than "some errors" would be nicer, what exactly are you doing

I'm just hitting the [Play Tears of Steel] button exactly as advised by you.
It won't tell you much, so best try for yourself with ESR-Firefox (no addons), but here:

Uncaught TypeError: right-hand side of 'in' should be an object, got undefined      index.js:214:8
   x index.js:214
   <anonymous> (index):135

Uncaught (in promise) DOMException: The operation is insecure.                      bundle.js:2
   x index.js:28
   
Uncaught ReferenceError: can't access lexical declaration 'client' before initialization      webtorrent-player:174:11
   t https://thaunknown.github.io/webtorrent-player/:174
   onclick https://thaunknown.github.io/webtorrent-player/:1

[ThaUnknown]

no not really, both font stuff and blendrender and oneshotrender modify the cpp wrapper, so I'd have to unmerge those changes manually, split them, then re-add them, thats like x3 work

At least the font-related changes and the sum of the two rendere changes prior to "rewrite" are already sequentially in the commit history. Splitting those two is no more work than branching of and a single git-command to rewind. Same goes for "rewrite" and the rest. Potentially there are a few fixes that need to be pulled in out of order, bzt from the commit-titles this appears to be only a few.
If oneShot- and blendRenderer were interleaved in the commit history I guess you could maayybe leave them together, but the final decision on this is for the qualified reviewers (i.e. rcombs and TFSThiago) to make.

btw, some of the changes (already prior to "rewrite") drop features, you'll likely want to drop those commits (as well as "update/fix $dependecy"- and "bump emscripten"-commits).

Off-topic for this PR, but:

the player that's shown there uses ES8

It appears Firefox-ESR 78 also fully supports ES8 and also ES7.

more than "some errors" would be nicer, what exactly are you doing

I'm just hitting the [Play Tears of Steel] button exactly as advised by you.
It won't tell you much, so best try for yourself with ESR-Firefox (no addons), but here:

Uncaught TypeError: right-hand side of 'in' should be an object, got undefined      index.js:214:8
   x index.js:214
   <anonymous> (index):135

Uncaught (in promise) DOMException: The operation is insecure.                      bundle.js:2
   x index.js:28
   
Uncaught ReferenceError: can't access lexical declaration 'client' before initialization      webtorrent-player:174:11
   t https://thaunknown.github.io/webtorrent-player/:174
   onclick https://thaunknown.github.io/webtorrent-player/:1

yeaaaah, just like you're not really sure about JS, I'm not really use about C++ and emscripten, so I'm not 100% sure what's happening in there, I'm aware I drop some functionality, but afaik it's nothing major, one very big change from before [outside of function names etc] functionality wise, is the fallback font is no-longer embedded, I decided to keep this change because the load times were really bad, and in many, many use cases it was never used, this way the fallback font loading is deferred for when it's actually needed

// off topic: the player broke because of a generic misplacement of a single bracket, works now, thanks a lot for finding this, also that stacktrace tells me pretty much everything, man I love sourcemaps

[dmitrylyzo]

I divided @JustAMan's commits into 3 branches:
https://github.com/jellyfin/JavascriptSubtitlesOctopus/commits/oneshot-render (#111)
https://github.com/jellyfin/JavascriptSubtitlesOctopus/commits/smart-blend (depends on oneshot-render)
https://github.com/jellyfin/JavascriptSubtitlesOctopus/commits/old-browsers (#112)
After the merge I got the same source 🤞

[ThaUnknown]

I divided @JustAMan's commits into 3 branches:
https://github.com/jellyfin/JavascriptSubtitlesOctopus/commits/oneshot-render (#111)
https://github.com/jellyfin/JavascriptSubtitlesOctopus/commits/smart-blend (depends on oneshot-render)
https://github.com/jellyfin/JavascriptSubtitlesOctopus/commits/old-browsers (#112)
After the merge I got the same source 🤞

this is great, I might refactor this PR after these are merged, but still probably as some sort of V2 version, this really should be compiled using smth like babel if someone wants backwards compatibility, instead of dropping features

[ThaUnknown]

I still really don't see how we can include datahoarder's font loading optimisations, without him doing it himself, but I doubt that's gonna happen, so I'll most likely just exclude them.

[JustAMan]

Thanks a million Dmitry for taking this up! :) вс, 26 сент. 2021 г., 15:28 Cas ***@***.***>:

…

I still really don't see how we can include datahoarder's font loading optimisations, without him doing it himself, but I doubt that's gonna happen, so I'll most likely just exclude them. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#106 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHW6RQARM4Y2POG7LC3J5TUD4GWDANCNFSM5AWCDEEQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[ThaUnknown]

closed in favor of waiting for #111