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

feat(subtitles): settings & custom element for external subtitles #2360

Merged
merged 31 commits into from
Sep 12, 2024

Conversation

seanmcbroom
Copy link
Contributor

This pull request introduces new features and improvements related to subtitle settings and displaying subtitle tracks. The main changes include:

Subtitle Settings Page:

  • Added a new settings page dedicated to subtitle customization.
  • Users can now select their prefered subtitle font from a list of available fonts.
  • Font size and position from the bottom of the screen can be adjusted.
  • Options to enable or disable subtitle backdrop and stroke.

Custom Subtitle Track Element:

  • Integrated a custom track element to handle external subtitle files, supporting both VTT and ASS/SSA formats.
  • Added logic to parse and sanitize subtitle files, ensuring harmful tags are removed and specific tags are replaced with styled <span> tags for safe rendering

@jellyfin-bot
Copy link

jellyfin-bot commented Jun 5, 2024

Cloudflare Pages deployment

Latest commit aa797ae
Status ✅ Deployed!
Preview URL https://f941f7f1.jf-vue.pages.dev
Type 🔀 Preview

View build logs

@seanmcbroom seanmcbroom changed the title Master feat(subtitles): settings & custom element for external subtitles Jun 5, 2024
Copy link
Member

@ferferga ferferga left a comment

Choose a reason for hiding this comment

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

Thank you very much for your conntribution ❤️❤️❤️!

Beforehand, please bear with me because, as I point out in one of my comments, I'm not fully knowledgeable of all the subtitle flow we need to handle in Jellyfin and I might be naive in some aspects.

Regardless of the refactoring bits, I'd like to have some input from @erikbucik as well. Although current web client provides with typography customization options, I don't think this is a good idea. While in the surface it might seem better to provide the same options as the current web client and a huge amount of customization, I don't think being feature complete === repeat the same mistakes/UX quirks of the current web client. This is my reasoning:

  • Users expect consistency across an application. This means that, by default, subtitles should use the same typography that we use everywhere, right? Are we on the same page on this?
  • Introducing these kind of customization options might appeal at first, because we're catering for more user tastes. But, regardless what we do, in this case it will be impossible to cater to all tastes and, at the end of the day, we're offering a closed typography selection. If I'd want another typography, I'd need to rebuild the client with the typography of my liking, figure out all this logic, add it to all this logic and profit! At this point, we're making more difficult changing it, which is something that I could also do right now (add font, add CSS, build and profit!). A counter-example of this would be the playback speed selection): it's integrated in a way that it's flexible enough to cater all the endless likings of an user without compromises, it's not complex and there are good defaults for those who don't want the extra customization (the selection menu on click).

Hopefully I explained myself right, since I'm in the middle of some exams I'm reviewing this in the quick way, sorry!

Thank you very much again for your contribution!

frontend/src/components/Playback/PlayerElement.vue Outdated Show resolved Hide resolved
frontend/locales/en.json Outdated Show resolved Hide resolved
frontend/locales/en.json Outdated Show resolved Hide resolved
frontend/locales/en.json Outdated Show resolved Hide resolved
frontend/src/components/Playback/SubtitleTrack.vue Outdated Show resolved Hide resolved
frontend/src/utils/subtitles.ts Outdated Show resolved Hide resolved
frontend/src/utils/subtitles.ts Outdated Show resolved Hide resolved
frontend/src/utils/subtitles.ts Outdated Show resolved Hide resolved
frontend/src/store/player-element.ts Outdated Show resolved Hide resolved
frontend/src/store/client-settings.ts Outdated Show resolved Hide resolved
frontend/src/utils/subtitles.ts Fixed Show fixed Hide fixed
frontend/src/utils/subtitles.ts Fixed Show fixed Hide fixed
frontend/src/utils/subtitles.ts Fixed Show fixed Hide fixed
frontend/src/utils/subtitles.ts Fixed Show fixed Hide fixed
frontend/src/utils/subtitles.ts Fixed Show fixed Hide fixed
frontend/src/utils/subtitles.ts Fixed Show fixed Hide fixed
frontend/src/utils/subtitles.ts Fixed Show fixed Hide fixed
frontend/src/utils/subtitles.ts Fixed Show fixed Hide fixed
frontend/src/pages/settings/subtitles.vue Fixed Show fixed Hide fixed
@seanmcbroom
Copy link
Contributor Author

seanmcbroom commented Jun 9, 2024

First of all, thank you for taking the time to review the pull request. I know you've been busy and I wish you good luck on your exams!

I've already responded to a few of the discussions. I couldn't get to all of them today but I'll try to get to them done within the next few days. 👍

Copy link

sonarcloud bot commented Jun 9, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

Copy link
Member

@ferferga ferferga left a comment

Choose a reason for hiding this comment

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

A few extra comments, now I think I fully understand the scope of this much better, thank you!

Answered the non resolved conversations of my previous comments and I'm also adding some small things that I noticed in this second pass.

Re-tagging @erikbucik for his input regarding my first review comment and whether we should give typography choices or not.

Also, FYI, I might merge these days a few PRs of mine: they're part of some work I've been doing for my uni thesis (the reason why I'm so busy lately, since it needs to be delivered next week :P) and I had them locally for a while, and for my defense it's good to have them upstreamed already. Don't take it as I'm neglecting this PR please, definitely good stuff here!

Also, if you can, merge the latest changes locally and run npm ci so you have the linter working again.

frontend/src/components/Playback/SubtitleTrack.vue Outdated Show resolved Hide resolved
frontend/src/utils/subtitles.ts Outdated Show resolved Hide resolved
frontend/src/utils/subtitles.ts Outdated Show resolved Hide resolved
frontend/src/utils/subtitles.ts Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Not sure how much does this takes for an average movie, but perhaps, in order to avoid blocking the main loop, we can move this into a WebWorker? In case you want to take a look at how it's done, check BlurhashCanvas.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's already run asynchronously, so it shouldn't be blocking the main loop anyway. What benefits does WebWorker provide besides being run asynchronously?

Copy link
Member

Choose a reason for hiding this comment

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

@seanmcbroom Being async doesn't mean it doesn't block the main loop. async/await is just syntatic sugar over Promises. Promises themselves don't block the main thread while they're being resolved, the event loop can defer them in the background... But if you fire wrap in a promise a while loop that resolves at the 1 millionth iteration, you'll see how, as soon as the event loop starts processing the promise, the whole thread will be blocked because the loop itself is blocking.

asynchronous code still runs in the main thread, although in a deferred way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, sorry about my lack of knowledge regarding how javascript is run on a hardware level. I'll look into it and update you.

@erikbucik
Copy link
Member

I think it's good to let the user choose a different font. If not purely because of personal preferences, then for better readability.
Our current UI font (Figtree) may not be the easiest to read quickly (smaller x-height, wider and geometric-like letterforms, etc.), therefore allowing a font change for subtitles is a good idea.
The subtitle font servers one purpose (aside from fully supporting the language the subs are in) => helping the watcher read & understand the subs really quickly so that they can look at the video for as long as possible.

The other customization options (position, stroke, shadow, etc.) are also nice to have.

@ferferga
Copy link
Member

ferferga commented Jun 19, 2024

@seanmcbroom After further chat with Erik, we came up with the following conclusions as well (though these are more technical related):

  • The displayed fonts should be the system's ones. You can get the current installed fonts using this API: https://developer.mozilla.org/en-US/docs/Web/API/Window/queryLocalFonts
  • By using the system fonts, we're sure the text can be displayed and we don't need to increase the bundle with a lot of fonts.
  • That API is currently just supported by Chromium browsers. It entered draft at W3C just 2 weeks ago (7 Jun 2024), so I expect it will take a while to reach the other browsers still, but it should come at some point.
  • Given this is a niche feature and Chromium is the most widely used browser, it's not critical to think of a workaround and this could be left as is. If the API is not supported the selector must be disabled.
  • That API requires permission. Use https://vueuse.org/core/usePermission/#usepermission to reactively track if permission is given and disable/enable the selector with that.
  • Use https://vueuse.org/core/useSupported/#usage to reactively check if the API is supported
  • Given the API it's not widely supported, we need to display informational messages:

Custom fonts are currently not supported by your device

And the following buttons/links:

Learn more: When not supported, linking to the CanIUse page
Allow permission: When the API is supported but permission is not yet granted.

  • The font selector must be in its own component, so it could be reused for changing Books typography or even the whole UI.
  • The font selector should display each font in the specific font:

image

  • I'd say that the VSelect structure should be something like this:
<VListItem>*Current font* (Default)</VListItem>
<VDivider />
...Rest of fonts (also wrapped in VListItem)
  • Current Font must be replaced by the current's font name of course. It would be great though if, instead of hardcoding it, we could query the fon'ts name from CSS and computedStyle or using Font API. That way, if we change the font later, somehow CSS stylesheet is overriden by the user (because he modified the html file, for example) or allow for font customization in appearance settings for the whole client, the real selected font is displayed.

What do you think of these points?

@seanmcbroom
Copy link
Contributor Author

I agree with all your points and proposed changes to the font selection. I wasn't even aware of the queryLocalFonts method and the timing for it being passed into chromium is almost perfect, so I guess it was meant to be haha.

@jellyfin-bot jellyfin-bot added merge conflict Something has merge conflicts and removed merge conflict Something has merge conflicts labels Jul 12, 2024
@jellyfin-bot jellyfin-bot added the merge conflict Something has merge conflicts label Aug 11, 2024
@ferferga ferferga force-pushed the master branch 11 times, most recently from bc4e4a7 to 0233c77 Compare August 11, 2024 10:44
seanmcbroom and others added 25 commits September 12, 2024 02:43
Signed-off-by: Fernando Fernández <[email protected]>
* Reduce unnecessary verbosity of the stores
* Extract the current typography of the application as a CSS variable
* Make font selector truly generic and also able to change the typography of the whole app

There are now 3 internal values:
- auto: for following app's font
- system: for following system's font
- default: default app's font

This way subtitles can be truly configurable independently from the app.

App typography follows the same schema, but without 'auto' since it's not applicable there.

Signed-off-by: Fernando Fernández <[email protected]>
Signed-off-by: Fernando Fernández <[email protected]>
Signed-off-by: Fernando Fernández <[email protected]>
Signed-off-by: Fernando Fernández <[email protected]>
Copy link

sonarcloud bot commented Sep 12, 2024

@ferferga
Copy link
Member

@seanmcbroom Okey, let's ship this!
Thank you very much for all your knowledge about subtitles and patience while getting this in! ❤️❤️🚀🚀🚀

@ferferga ferferga merged commit f3a6836 into jellyfin:master Sep 12, 2024
14 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
vue Pull requests that edit or add Vue files
Projects
Development

Successfully merging this pull request may close these issues.

4 participants