-
Notifications
You must be signed in to change notification settings - Fork 6k
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
Support Webvtt subtitles class colors #4178
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
|
I signed it! |
I thought VTT required the red class to be defined for the style to apply. Would you kindly provide a specification reference for this behavior change? |
Closing due to lack of activity. |
Relevant specs: Once this has a signed CLA and has been rebased/merged into current dev-v2 I can take a more in-depth look at the code. |
I think this will likely also fix #6581 |
# Conflicts: # library/core/src/main/java/com/google/android/exoplayer2/text/webvtt/WebvttCueParser.java
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
int start, int end) { | ||
for (String className : classes) { | ||
if (ColorParser.isNamedColor(className)) { | ||
int color = ColorParser.parseCssColor(className); |
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.
This is neat code re-use, but because the WebVTT list is so much shorter than the list of CSS colors I think it would be clearer to explicitly list the WebVTT default colors. Otherwise if someone defines a custom class called papayawhip
in their style section that maybe has nothing to do with that color, we'll color the cue anyway (which is not in-line with the WebVTT spec).
So I think a private static final Map<String, Integer> DEFAULT_COLORS
inside WebvttCueParser
would be the best approach here. You'll probably have to populate it in a static initializer block the same as in ColorParser
.
That will also make adding background color support less 'different' - we can just add a DEFAULT_BACKGROUND_COLORS
map alongside. If you have the time/inclination, feel free to also add that in this same PR! :)
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.
Sounds good, I'll take a look and update this soon
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.
Updated the strategy by adding the two maps (adds support for background too) in WebvttCueParser
and removing the dependency from the color parser.
Added relevant tests too
@@ -514,6 +515,8 @@ private static void applySpansForTag( | |||
text.setSpan(new UnderlineSpan(), start, end, Spanned.SPAN_EXCLUSIVE_EXCLUSIVE); | |||
break; | |||
case TAG_CLASS: | |||
applySupportedClasses(text, startTag.classes, start, end); |
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.
Can you add some tests to WebvttCueParserTest
for this?
…matching with tests TIL: papayawhip is a color
Thanks! I'll get this merged shortly. |
hi @icbaker what are the next steps on this PR? |
Problem
Colors defined in Webvtt cue Class tags were ignored.
For example in the screenshots below the cue text was
Example asset: http://dash.edgesuite.net/akamai/test/caption_test/ElephantsDream/elephants_dream_480p_heaac5_1.mpd
Solution
WebvttCueParser
already knows about the cue classes, but these were not used. So in case of a color class, we now apply aForegroundColorSpan
to the subtitle text.Screenshots