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

Smart font streaming for external ASS subtitles #6257

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

p0358
Copy link
Contributor

@p0358 p0358 commented Oct 25, 2024

!! requires jellyfin/jellyfin#12877 -- read that PR for more information and discussion on this feature, to avoid duplicating explanations here

Changes

  • support grabbing the font list and fonts for external ASS subtitles
    • currently it modifies the DeliveryUrl of media stream of subtitles to make it into the fonts endpoint, discuss on the main PR if instead of that, something like FontsUrl should be added next to DeliveryUrl or not
    • don't try to grab fonts if it's not a normal .ass file from the API, and one that's not inside of .mkv (the latter comes from assumption from main PR that for now we don't scan to missing fonts in MKV, since there's no code right now to filter out existing font attachments, and we don't want to duplicate fonts, assuming that usually fonts are embedded in MKV properly)
  • refactor the function to make it more readable and better by making all promises parallel at once, to avoid waiting a few round-trip times (and wasm loading + both font endpoints can all take some time on cold requests, so it's only logical to do them all at once to decrease any potential delay in displaying subtitles)
    • turns 3 round-trips into just one, yay

Issues
Closes #5960 old PR by superseding it with this

(also since this is a draft, I didn't test the code yet, will remove this line after I do)

@jellyfin-bot
Copy link
Collaborator

jellyfin-bot commented Oct 25, 2024

Cloudflare Pages deployment

Latest commit 2c1f90e
Status ✅ Deployed!
Preview URL https://bc033b0f.jellyfin-web.pages.dev
Type 🔀 Preview

View build logs

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

ESLint doesn't pass. Please fix all ESLint issues.

src/plugins/htmlVideoPlayer/plugin.js Show resolved Hide resolved
src/plugins/htmlVideoPlayer/plugin.js Show resolved Hide resolved
src/plugins/htmlVideoPlayer/plugin.js Outdated Show resolved Hide resolved
src/plugins/htmlVideoPlayer/plugin.js Outdated Show resolved Hide resolved
src/plugins/htmlVideoPlayer/plugin.js Outdated Show resolved Hide resolved
src/plugins/htmlVideoPlayer/plugin.js Outdated Show resolved Hide resolved
src/plugins/htmlVideoPlayer/plugin.js Outdated Show resolved Hide resolved
src/plugins/htmlVideoPlayer/plugin.js Outdated Show resolved Hide resolved
src/plugins/htmlVideoPlayer/plugin.js Outdated Show resolved Hide resolved
src/plugins/htmlVideoPlayer/plugin.js Outdated Show resolved Hide resolved
@p0358
Copy link
Contributor Author

p0358 commented Oct 25, 2024

Ah no, I actually messed up with the encodingOptions, I guess one more round trip might be unavoidable... I think I'll refactor it in a way where encodingOptions -> grab fallback fonts will be an independent promise, since those combined could take faster than waiting for wasm loading and other fonts list loading before making the second request (radical performance thinking xd)

@thornbill thornbill added the backend Requires work on the server to finish label Oct 25, 2024
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

ESLint doesn't pass. Please fix all ESLint issues.

src/plugins/htmlVideoPlayer/plugin.js Show resolved Hide resolved
src/plugins/htmlVideoPlayer/plugin.js Show resolved Hide resolved
src/plugins/htmlVideoPlayer/plugin.js Outdated Show resolved Hide resolved
Also refactors the function renderSSaAss to be more parallel and readable and nice...
Copy link

sonarcloud bot commented Oct 26, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
1 Security Hotspot

See analysis details on SonarCloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend Requires work on the server to finish
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants