-
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
Add support for SSA (v4+) FontSize style #8615
Add support for SSA (v4+) FontSize style #8615
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.
- If the "fontSize" can't be parsed, should we return
null
orCue.DIMEN_UNSET
fromparseFontSize
? We could avoid an unboxing withCue.DIMEN_UNSET
but in the other hand, returningnull
would be consistent with the primary color parsing.
I think DIMEN_UNSET is perfect here - since there is a value we can use that isn't a valid font size (which is the problem we hit with color).
- It is OK to do the size calculation (
style.fontSize / screenHeight
) on decoder side or should be placed in theSsaStyle
? If the latter, then need to passscreenWidth
andscreenHeight
toSsaStyle
.
Where you've put the calculation looks fine to me.
- In case of missing
PlayResY
, the "fontSize" still has an effect in VLC, but not sure based on what. Should we try to figure this out or just ignore the "fontSize" ifPlayResY
is not specified?
Hypothesis (untested): VLC assumes a PlayResY of 288 if it's missing. This seems to be a default assumed in a couple of places in libass, e.g.:
https://github.com/libass/libass/blob/59eb317aaa495ad5331c9efdf8d7bf3d860c2992/libass/ass.c#L1545-L1549
If that aligns with your observations then I think we should add this default logic to all usages of screenWidth
and screenHeight
, probably here, and probably in a separate PR (and so keep the existing logic of ignoring FontSize if PlayResY is missing for now):
ExoPlayer/library/core/src/main/java/com/google/android/exoplayer2/text/ssa/SsaDecoder.java
Lines 88 to 89 in b100094
screenWidth = Cue.DIMEN_UNSET; | |
screenHeight = Cue.DIMEN_UNSET; |
library/core/src/main/java/com/google/android/exoplayer2/text/ssa/SsaStyle.java
Outdated
Show resolved
Hide resolved
Applied the suggested changes, ready for review. I'll look into the missing "PlayResY" case and create a separate PR for that as you suggested. |
Thanks, looks good! I'll work on getting this merged. |
@@ -114,7 +120,8 @@ public static SsaStyle fromStyleLine(String styleLine, Format format) { | |||
return new SsaStyle( | |||
styleValues[format.nameIndex].trim(), | |||
parseAlignment(styleValues[format.alignmentIndex].trim()), | |||
parseColor(styleValues[format.primaryColorIndex].trim())); | |||
parseColor(styleValues[format.primaryColorIndex].trim()), | |||
parseFontSize(styleValues[format.fontSizeIndex].trim())); |
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.
Is it better to use Cue.DIMEN_UNSET
here if fontSizeIndex == C.INDEX_UNSET
? More specifically, all valid styles should not be discarded if some other styles are not found for whatever reason.
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.
Yeah, that's a good point, seems the intention of this code is to be robust against both different Format line orderings and presence or absence of fields, but it's currently only defensive against the ordering and breaks if fields we expect to see are absent. This is obviously getting worse as we add support for more style properties.
I don't think this PR is the right place to fix this though - i'll send a separate change. Thanks for flagging!
PiperOrigin-RevId: 359244236
Suggested in a comment on PR Issue: #8615 PiperOrigin-RevId: 359522217
Suggested in a comment on PR Issue: #8615 PiperOrigin-RevId: 359522217
Suggested in a comment on PR Issue: google#8615 PiperOrigin-RevId: 359522217
@icbaker
This is the initial PR for the "FontSize" style (SSA v4+). I'll remove the entry from the
media.exolist.json
file and add tests on the decoder side after resolving any upcoming comments.However, I am bit confused of this style, because the specs are not mentioning the unit (in the override section the "point size" mentioned though). Based on my tests with VLC, the font size is relative to the specified
PlayResY
in the[Script Info]
, but its value doesn't seem to be represent pixels exactly and couldn't find a constant coefficient either, however visually this implementation seems to be proportional with the VLC one :)Furthermore, I wasn't sure about these small things:
If the "fontSize" can't be parsed, should we return
null
orCue.DIMEN_UNSET
fromparseFontSize
? We could avoid an unboxing withCue.DIMEN_UNSET
but in the other hand, returningnull
would be consistent with the primary color parsing.It is OK to do the size calculation (
style.fontSize / screenHeight
) on decoder side or should be placed in theSsaStyle
? If the latter, then need to passscreenWidth
andscreenHeight
toSsaStyle
.In case of missing
PlayResY
, the "fontSize" still has an effect in VLC, but not sure based on what. Should we try to figure this out or just ignore the "fontSize" ifPlayResY
is not specified?