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

Fix loading issues with embedded SUBRIP subtitles while using Exoplayer #2743

Closed

Conversation

nathkn
Copy link

@nathkn nathkn commented May 7, 2023

Currently, embedded SRT/SUBRIP subtitles can take a long time to load because it requires the server to use ffmpeg to extract the embedded subtitles out of the video stream in order to return a JSON file of the subtitles so they can be used by the custom overlay. A longer explanation is here. For large videos (such as 4K), this can take minutes to load a single subtitle. After these fixes, they load near instantaneously by Exoplayer.

Changes

  • Enables the SRT/SUBRIP embedded subtitle profiles, which means they will be displayed directly by Exoplayer instead of being shown through the custom overlay fragment
  • In order to keep parity with the custom overlay's ability to change the formatting of the subtitles based on user preferences, this PR also implements subtitle formatting within Exoplayer, since otherwise those settings would be lost when using Exoplayer directly for embedded subtitles
  • Some of these code changes have been influenced by this previous attempt to remedy the issue: Let ExoPlayer and VLC handle external subtitles #1949

Issues
Fixes #145

(Note: I'm not a Java developer and haven't contributed to this project before, so any and all suggestions for how to implement this better are welcomed!)

nathkn added 2 commits May 7, 2023 16:24
There doesn't appear to be a stroke-width equivalent, so it is excluded
here
Copy link
Member

@nielsvanvelzen nielsvanvelzen left a comment

Choose a reason for hiding this comment

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

I think it's fine to replace our custom subtitle rendering with the one provided by ExoPlayer (in fact, this is one of my planned changes with #1057), but we should do that for all (text-)subtitles to make the behavior consistent. That means both embedded and external subtitles should use it and our custom subtitle rendering code must be removed.

@nathkn
Copy link
Author

nathkn commented May 10, 2023

@nielsvanvelzen I definitely agree with that - this PR was more of a quick fix until without needing modify a ton of code that was going to be overhauled with the playback rewrite anyway.

I could take a shot at removing the custom subtitle rendering and have ExoPlayer handle external and embedded subtitles if you wanted? Could also do that after this PR is merged too - let me know how you'd prefer I proceed

@nielsvanvelzen
Copy link
Member

You can update this PR to completely remove the custom subtitle stuff if you want. I won't merge it as-is.

@nielsvanvelzen
Copy link
Member

Hey @nathkn, are you still working on this PR/planning to finish it?

@nielsvanvelzen
Copy link
Member

Closing this PR as abandoned.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge conflict Conflicts prevent merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Subtitles Don't Always Load
3 participants