-
Notifications
You must be signed in to change notification settings - Fork 574
Fix RTL punctuation marks issues with mixed BiDi subtitles #2553
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
base: main
Are you sure you want to change the base?
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
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.
Thanks for the PR! I've added some comments. I was also wondering if you'd be able to provide an example stream or subtitle file that demonstrates the problem you're fixing here? So I can run the demo app without this patch, see the problem, then run it again and see it fixed.
I would also be interested to see what happens with span styling when this code is triggered - because atm I'm worried it might all be lost.
libraries/ui/src/main/java/androidx/media3/ui/SubtitlePainter.java
Outdated
Show resolved
Hide resolved
libraries/ui/src/main/java/androidx/media3/ui/SubtitlePainter.java
Outdated
Show resolved
Hide resolved
libraries/ui/src/main/java/androidx/media3/ui/SubtitlePainter.java
Outdated
Show resolved
Hide resolved
Hi, thanks for the comments. This is a standard SRT file that can be used to reproduce this issue (just rename to srt if needed): Here are some examples: These lines are displayed as: should be displayed as: These lines are displayed as: Should be displayed as: You can notice that the punctuation marks are displayed to the right of the sentences and should be displayed to the left (as in all RTL languages). Hope you are good with Hebrew :-) |
Thank you for the test file and screenshots, that's really helpful. I took your SRT file and adapted it to WebVTT so I could add some styling (borrowed from one of our test files), and confirmed that this PR currently strips styling info. Using this file (just rename to Compare before your change (styling visible, punctuation in the wrong place): With after (styling lost, punctuation in the correct place): Please take a look and see if you can tweak this PR so it puts the punctuation in the correct place while preserving (or restoring) the span styling. |
I was able to reproduce the styling issues with the VTT file on my side, I think the best option here will be to manipulate the cues and then restore the original spans with the indices fixed. I am working on this. |
Spans are preserved when wrapping RTL subtitles
libraries/ui/src/main/java/androidx/media3/ui/SubtitlePainter.java
Outdated
Show resolved
Hide resolved
libraries/ui/src/main/java/androidx/media3/ui/SubtitlePainter.java
Outdated
Show resolved
Hide resolved
libraries/ui/src/main/java/androidx/media3/ui/SubtitlePainter.java
Outdated
Show resolved
Hide resolved
libraries/ui/src/main/java/androidx/media3/ui/SubtitlePainter.java
Outdated
Show resolved
Hide resolved
- Added support for non Spanned cues
- Added Unit Tests
This PR fixes the punctuation marks issues with subtitles text containing a mix of RTL and LTR languages.
google/ExoPlayer#11214
jellyfin/jellyfin-androidtv#4296