Skip to content

Fix RTL punctuation marks issues with mixed BiDi subtitles - #2553

Merged
copybara-service[bot] merged 6 commits into
androidx:mainfrom
orenskl:main
Nov 6, 2025
Merged

copybara-service[bot] merged 6 commits into
androidx:mainfrom
orenskl:main

Conversation

@orenskl

@orenskl orenskl commented Jun 21, 2025 •

Copy link
Copy Markdown
Contributor

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

@google-cla

google-cla Bot commented Jun 21, 2025

Copy link
Copy Markdown

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.

@orenskl orenskl changed the title Fix RTL punctuation issues with mixed BiDi subtitles Fix RTL punctuation marks issues with mixed BiDi subtitles Jun 21, 2025
@tonihei tonihei assigned tonihei and icbaker and unassigned tonihei Jun 27, 2025

@icbaker icbaker left a comment

Copy link
Copy Markdown
Collaborator

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.

Comment thread libraries/ui/src/main/java/androidx/media3/ui/SubtitlePainter.java Outdated
Comment thread libraries/ui/src/main/java/androidx/media3/ui/SubtitlePainter.java Outdated
Comment thread libraries/ui/src/main/java/androidx/media3/ui/SubtitlePainter.java Outdated
@orenskl

orenskl commented Jul 7, 2025

Copy link
Copy Markdown
Contributor Author

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):

subtitles.txt

Here are some examples:

These lines are displayed as:

Screenshot 2025-07-07 at 22 21 24

should be displayed as:

Screenshot 2025-07-07 at 22 51 06

These lines are displayed as:

Screenshot 2025-07-07 at 22 21 47

Should be displayed as:

Screenshot 2025-07-07 at 22 50 38

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 :-)

@icbaker

icbaker commented Jul 8, 2025

Copy link
Copy Markdown
Collaborator

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 .vtt suffix):
vtt-2553.txt

Compare before your change (styling visible, punctuation in the wrong place):

android-screenshot-2025-07-08T113424

android-screenshot-2025-07-08T113602

With after (styling lost, punctuation in the correct place):

android-screenshot-2025-07-08T113753

android-screenshot-2025-07-08T113803


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.

@orenskl

orenskl commented Jul 8, 2025

Copy link
Copy Markdown
Contributor Author

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.

Comment thread libraries/ui/src/main/java/androidx/media3/ui/SubtitlePainter.java Outdated
Comment thread libraries/ui/src/main/java/androidx/media3/ui/SubtitlePainter.java Outdated
Comment thread libraries/ui/src/main/java/androidx/media3/ui/SubtitlePainter.java Outdated
Comment thread libraries/ui/src/main/java/androidx/media3/ui/SubtitlePainter.java Outdated
@orenskl
orenskl requested a review from icbaker October 21, 2025 05:43
@orenskl

orenskl commented Oct 21, 2025

Copy link
Copy Markdown
Contributor Author

Hi, is there anything I can do to help you merge this PR ?

@icbaker

icbaker commented Oct 21, 2025

Copy link
Copy Markdown
Collaborator

Thanks for pinging, I was waiting for the tests and didn't realise they'd already been added (I think just 'resolving' a conversation on GitHub maybe doesn't trigger an email - so I didn't know to look back here) - I'll take another look.

@icbaker

icbaker commented Nov 5, 2025

Copy link
Copy Markdown
Collaborator

I'm going to send this for internal review now. You may see some more commits being added as I make changes in response to review feedback. Please refrain from pushing any more substantive changes as it will complicate the internal review - thanks!

@icbaker
icbaker force-pushed the main branch 5 times, most recently from f09e6a5 to 679cec6 Compare November 5, 2025 16:28
Also add a release note and some nullness fixes
@copybara-service
copybara-service Bot merged commit 62051ae into androidx:main Nov 6, 2025
1 check passed
@androidx androidx locked and limited conversation to collaborators Jan 5, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants