Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Jellyfin] Old browsers #112

Merged
merged 5 commits into from
Sep 29, 2021
Merged

[Jellyfin] Old browsers #112

merged 5 commits into from
Sep 29, 2021

Conversation

dmitrylyzo
Copy link
Contributor

This PR improves compatibility with older browsers.
Jellyfin is focused on supporting webOS 1.2 and Tizen 2.3, which use ancient web engines (WebKit).

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

List of commits

82822b2
6444522
9299074
Merge pull request #15 from JustAMan/update-build
other commits are new

Copy link
Member

@TheOneric TheOneric left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your PRs! Assuming you spot-checled this PR doesn't break anything with JSO's example suit (the gh_pages branch); just a few nits:

Normally I'd like to see fixups done by amending the faulty/outdated patch-commit rather than adding yet another commit; howver since this is porting from JustAMan's jellyfin patches there's some value in keeping the commits as unchanged as possible. But your last two Remove unnecessary flags (already in GLOBAL_CFLAGS) and Remove deprecated flag (PR review) commits should imho be squashed into one Drop superfluous and deprecated flags commit.
Also it would be nice if you could add Cherry-pciked from: https://github.com/jellyfin/JavascriptSubtitlesOctopus/commit/<commit-hash> to the description of the cherry-picked commits.


If you haven't yet checked the examples: In order to test your changes, checkout the gh_pages branch in a seperate folder; update the assets/jsfolder to what building your PR yields and run a local server. Eg create a symlink named JavascriptSubtitlesOctopus in and to the root folder of the gh_pages repo and run this Python script: run-server.py. Your tests will be available at http://localhost:8000/JavscriptSubtitlesOctopus

You'll want to at least play the Video.JS sample until 01:10 and skip a bit through the Railgun S Karaoke Test, Attack on Titan Stress Karaoke Test and the Brotli Compressed Subtitle File examples and make sure the subtitles are rendered and not obviously borked.

@dmitrylyzo
Copy link
Contributor Author

Done. Also, simplified from polyfills a bit.

Tested Video.JS, Railgun S Karaoke Test, Attack on Titan Stress Karaoke Test and Brotli Compressed Subtitle File - no issues except these errors in the console (the same for the JSO master):
jso-err1
Fontconfig error occurs when cache is disabled.

All these polyfills should probably be fixed in emscripten itself (emscripten-core/emscripten#11008 and emscripten-core/emscripten#11011)

@TheOneric
Copy link
Member

Thanks again!
Looks good afaict and since Thiago also already approved, I'll merge it shortly.

no issues except these errors in the console (the same for the JSO master)

Yeah, the MIMe and following “ArrayBuffer instatiation” can be safely ignored, they are only because the test server setup doesn't know about the wasm-MIME type (and emscripten will fall back to fetching it anyway via “ArrayBuffer instatiation”) and well we don't have a favicon either. Not sure about the Fontconfig error, but it's not a regression and iinm will degrade font-lookup performance, but not break anything.

All these polyfills should probably be fixed in emscripten itself (emscripten-core/emscripten#11008 and emscripten-core/emscripten#11011)

Having emscripten provide the necessary polyfills would indeed be ideal.

@TheOneric TheOneric merged commit 483b282 into libass:master Sep 29, 2021
@dmitrylyzo dmitrylyzo deleted the old-browsers branch September 30, 2021 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants