Skip to content

feat: Add subtitle sync ui for easy alignment #6735

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

Open
wants to merge 28 commits into
base: master
Choose a base branch
from

Conversation

armanckeser
Copy link

@armanckeser armanckeser commented Apr 10, 2025

Changes

  • Chrome reactive change
chrome_reactive.mp4
  • Firefox before and after (pressed playback and stopped for the actual subtitles to re-render)

Screenshot 2025-04-17 204703
Screenshot 2025-04-17 204719

  • WebOS Browser

webos-dev-tmp-674d30ba-3ec7-4d78-bad5-42802e6794d3

  • Adds a dynamic subtitle sync rendering. Users can now stop where they would like a subtitle to start, align the time with the view and continue
  • Adds api from the html player and playback manager to get the subtitles.

Issues

#4364

@armanckeser armanckeser requested a review from a team as a code owner April 10, 2025 00:26
@jellyfin-bot
Copy link
Collaborator

jellyfin-bot commented Apr 10, 2025

Cloudflare Pages deployment

Latest commit 98b4af8
Status ✅ Deployed!
Preview URL https://ca82cfe2.jellyfin-web.pages.dev
Type 🔀 Preview

View build logs

Copy link
Contributor

@dmitrylyzo dmitrylyzo left a comment

Choose a reason for hiding this comment

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

  1. This only works with "custom subtitle element" (doesn't work in Chrome) and obviously only for text (SRT) subtitles.
  2. We probably should update the subtitles timeline on timeupdate.
  3. IMO, the timeline is too close to the offset slider, which makes you think they are tied.
  4. Too long for mobile.

@armanckeser
Copy link
Author

Do you have recommendations for 1?

I am also open to some guidance on 2. I tried a bunch of things, but I feel like there is an efficient solution my lack of FE knowledge eludes me from.

@dmitrylyzo
Copy link
Contributor

Do you have recommendations for 1?

When we use native API: https://developer.mozilla.org/en-US/docs/Web/API/HTMLMediaElement/textTracks
I think for SSA/ASS and PGS we should hide the timeline.

I am also open to some guidance on 2. I tried a bunch of things, but I feel like there is an efficient solution my lack of FE knowledge eludes me from.

Ideally, we should use PlaybackSubscriber.
But something like this could work:

function bindToPlayer(player) {
if (player === currentPlayer) {
return;
}
releaseCurrentPlayer();
currentPlayer = player;
if (!player) {
return;
}
Events.on(player, 'timeupdate', onTimeUpdate);
Events.on(player, 'playbackstart', onPlaybackStart);
Events.on(player, 'playbackstop', onPlaybackStop);
}
function releaseCurrentPlayer() {
const player = currentPlayer;
if (player) {
Events.off(player, 'timeupdate', onTimeUpdate);
Events.off(player, 'playbackstart', onPlaybackStart);
Events.off(player, 'playbackstop', onPlaybackStop);
currentPlayer = null;
}
}

Copy link
Author

@armanckeser armanckeser left a comment

Choose a reason for hiding this comment

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

Hmm, does this mean that platforms like WebOS wouldn't be able to use this implementation? I presume WebOS uses a chromiom browser.

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.

@armanckeser
Copy link
Author

Still have to think a bit about the realtime updates, the problem is getting a smooth timeline. Updating current implementation on every timeupdate results in the timeline moving choppily every second. I need to increase the resolution for the percentage calculations or add a css transition or something, but I am not sure yet

…or improved playback event handling and timeline updates
…t methods and utilizing onPlayerTimeUpdate for visualization
@armanckeser
Copy link
Author

I tried to implement a smooth scroll, but I just give up. I think this provides the value I would like it to provide. And even though the playback rendering is a bit janky, probably better than not having it at all.

@armanckeser armanckeser requested a review from dmitrylyzo April 12, 2025 02:50
…ubtitle event retrieval for improved clarity and functionality in SubtitleTimeline class
@armanckeser
Copy link
Author

@dmitrylyzo thoughts?

@Gauvino
Copy link

Gauvino commented Apr 15, 2025

Absolutely incredible

@armanckeser
Copy link
Author

armanckeser commented Apr 18, 2025

@dmitrylyzo Added screenshots from Firefox, Chrome, WebOS browser. Added realtime update ability and a video from Chrome showing that the changes are accurate with the realtime rendering. Changed the styling to better match Jellyfin Web style. I believe with SSA/ASS and PGS, the subtitle offset menu does not even show up (I tried with PGS and DVD SUB, neither showed the menu), but if it did, the timeline would be hidden.

@armanckeser
Copy link
Author

@dmitrylyzo Would love to learn what types of tests or proof I could add to make this easier to review for the team.

Copy link
Contributor

@dmitrylyzo dmitrylyzo left a comment

Choose a reason for hiding this comment

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

  1. As before, I think the timeline is too close to the offset slider, which makes you think they are tied.
    Close:
    timeline-old
    Split:
    timeline

  2. My biggest concern right now is intermediate refactoring. I prefer to do refactoring first and then add the feature. Ideally, we should make commits atomic. If no one has any objections to the current commit sequence, so be it.

@armanckeser
Copy link
Author

armanckeser commented Apr 28, 2025

As before, I think the timeline is too close to the offset slider, which makes you think they are tied.

Ah, thanks for the clarification. I thought you meant that the current time marker itself was too close to the button (which it was), but "split" looks much better

My biggest concern right now is intermediate refactoring. I prefer to do refactoring first and then add the feature. Ideally, we should make commits atomic. If no one has any objections to the current commit sequence, so be it.

I see. Let me try a stacked approach, I am thinking a PR (or commit?) to refactor the original into OffsetController and the SubtitleSync object, and another one to add the timeline view. Does that sound good to you?

armanckeser and others added 4 commits April 29, 2025 21:57
…f 0 for improved compatibility with Firefox element attachment.
…eline for improved code clarity and maintainability. The new method consolidates the logic for fetching subtitle events from both Jellyfin and VTT cues.
…responsiveness. Adjusted margins, padding, and dimensions for better alignment and user experience.
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