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

Add outlineWidth to Cue #1840

Open
wants to merge 5 commits into
base: release
Choose a base branch
from
Open

Conversation

LagradOst
Copy link

Fixes #1834, by adding an extra field to Cue to inform the painter about the outline width of the subtitle.

Copy link

google-cla bot commented Oct 28, 2024

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.

@LagradOst
Copy link
Author

This is not ready for review yet, due to the confusing painting in updateWebView that I am unsure how to update. @icbaker could you clarify how to add Cue specific outlines when convertCaptionStyleToCssTextShadow is only called once per list of cues. I dont see a way to make it cue specific in a clean way.

/**
* The size of the text outline stroke in pixels or {@link #DIMEN_UNSET} for default width.
*/
public final @Px float outlineWidth;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This applies to the whole Cue

But for google/ExoPlayer#9391 I think we will need to be able to apply different outlining to different parts of text within a cue (in order to properly implement the TTML spec).

I think this points towards to using a custom styling span instead (google/ExoPlayer#9391 (comment)).

With a styling span, the answer to your WebView question then becomes to modify this class I think: https://github.com/androidx/media/blob/main/libraries/ui/src/main/java/androidx/media3/ui/SpannedToHtmlConverter.java

Copy link
Author

Choose a reason for hiding this comment

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

Bruh, that makes it a lot more complicated. However I assume that no extra paramater is needed in Cue then, as SpannableString is a CharSequence and outlinewidth can therefore piggyback from CharSequence text. That just makes the drawing a pita as we need to loop all spans.

Would something in the style of https://stackoverflow.com/questions/43275159/how-to-draw-a-spanned-string-with-canvas-drawtext-in-android work for SubtitlePainter? If so, what properties would this custom span have? Just outline width, or would I also need to support additional properties like color, or would that be a separate span that also would need support?

@icbaker icbaker self-assigned this Oct 29, 2024
@LagradOst LagradOst marked this pull request as ready for review November 1, 2024 18:34
@LagradOst
Copy link
Author

@icbaker I believe I added everything needed for text outline using a span. However I an unable to test the webview text, so I added what I think will work.

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.

Custom subtitle outline width
2 participants