-
-
Notifications
You must be signed in to change notification settings - Fork 509
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
Remove custom subtitle renderer #3825
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
detekt found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only a few magic numbers
Does media3 support subtitle position offset? Tested this commit, subtitles finally load fine, but I really miss the ability to move the subtitles a bit lower on screen. EDIT: have taken a stab in #3969, @nielsvanvelzen ptal |
Our custom subtitle renderer is causing a lot of issues, like timeouts when waiting for subtitle extraction or issues with the rendering because someone decided to put SSA tags in SRT subs (please don't do this). There really isn't much benefit for the current custom renderer anyway.
While we could put some effort in fixing the issues in the current renderer, it's much easier to rely on media3 to render it (for now), this would also make it easier for the initial subtitle support in the playback rewrite. We can always re-introduce our own renderer in the future.
While I did test these changes, there may be issues with track selection or subtitles that are not embedded. As it's nearly impossible to test all edge-cases.
Changes
Issues
Should fix #145 and the many duplicates of that issue