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.

Sign up for GitHub

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+) MarginL, MarginR, MarginV style #2008

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+385 −74
Open

Add support for SSA (v4+) MarginL, MarginR, MarginV style #2008

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
91f25f1
Add support for SSA (v4+) MarginL, MarginR, MarginV style
szaboa Dec 30, 2024
f7c58e0
Revert formatting
szaboa Dec 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions demos/main/src/main/assets/media.exolist.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -690,6 +690,13 @@
"subtitle_mime_type": "text/x-ssa",
"subtitle_language": "en"
},
{
"name": "SubStation Alpha margin",
"uri": "https://storage.googleapis.com/exoplayer-test-media-1/gen-3/screens/dash-vod-single-segment/video-avc-baseline-480.mp4",
"subtitle_uri": "https://gist.githubusercontent.com/szaboa/bca7cdc90c1492eb747032f267a5de19/raw/8985eeb544641e174da22a1dafe50cd87393512f/test-subs-margin.ass",
"subtitle_mime_type": "text/x-ssa",
"subtitle_language": "en"
},
{
"name": "MPEG-4 Timed Text",
"uri": "https://storage.googleapis.com/exoplayer-test-media-1/mp4/dizzy-with-tx3g.mp4"
Expand Down
41 changes: 37 additions & 4 deletions libraries/extractor/src/main/java/androidx/media3/extractor/text/ssa/SsaDialogueFormat.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,14 +36,27 @@
public final int endTimeIndex;
public final int styleIndex;
public final int textIndex;
public final int marginLeftIndex;
public final int marginRightIndex;
public final int marginVerticalIndex;
public final int length;

private SsaDialogueFormat(
int startTimeIndex, int endTimeIndex, int styleIndex, int textIndex, int length) {
int startTimeIndex,
int endTimeIndex,
int styleIndex,
int textIndex,
int marginLeftIndex,
int marginRightIndex,
int marginVerticalIndex,
int length) {
this.startTimeIndex = startTimeIndex;
this.endTimeIndex = endTimeIndex;
this.styleIndex = styleIndex;
this.textIndex = textIndex;
this.marginLeftIndex = marginLeftIndex;
this.marginRightIndex = marginRightIndex;
this.marginVerticalIndex = marginVerticalIndex;
this.length = length;
}

Expand All @@ -58,6 +71,9 @@ public static SsaDialogueFormat fromFormatLine(String formatLine) {
int endTimeIndex = C.INDEX_UNSET;
int styleIndex = C.INDEX_UNSET;
int textIndex = C.INDEX_UNSET;
int marginLeftIndex = C.INDEX_UNSET;
int marginRightIndex = C.INDEX_UNSET;
int marginVerticalIndex = C.INDEX_UNSET;
Assertions.checkArgument(formatLine.startsWith(FORMAT_LINE_PREFIX));
String[] keys = TextUtils.split(formatLine.substring(FORMAT_LINE_PREFIX.length()), ",");
for (int i = 0; i < keys.length; i++) {
Expand All @@ -74,12 +90,29 @@ public static SsaDialogueFormat fromFormatLine(String formatLine) {
case "text":
textIndex = i;
break;
case "marginl":
marginLeftIndex = i;
break;
case "marginr":
marginRightIndex = i;
break;
case "marginv":
marginVerticalIndex = i;
break;
}
}
return (startTimeIndex != C.INDEX_UNSET
&& endTimeIndex != C.INDEX_UNSET
&& textIndex != C.INDEX_UNSET)
? new SsaDialogueFormat(startTimeIndex, endTimeIndex, styleIndex, textIndex, keys.length)
&& endTimeIndex != C.INDEX_UNSET
&& textIndex != C.INDEX_UNSET)
? new SsaDialogueFormat(
startTimeIndex,
endTimeIndex,
styleIndex,
textIndex,
marginLeftIndex,
marginRightIndex,
marginVerticalIndex,
keys.length)
: null;
}
}
170 changes: 114 additions & 56 deletions libraries/extractor/src/main/java/androidx/media3/extractor/text/ssa/SsaParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -351,7 +351,23 @@ private void parseDialogueLine(
.replace("\\N", "\n")
.replace("\\n", "\n")
.replace("\\h", "\u00A0");
Cue cue = createCue(text, style, styleOverrides, screenWidth, screenHeight);

float dialogueMarginLeft = format.marginLeftIndex != C.INDEX_UNSET
? SsaStyle.parseMargin(lineValues[format.marginLeftIndex]) : 0f;
float dialogueMarginRight = format.marginRightIndex != C.INDEX_UNSET
? SsaStyle.parseMargin(lineValues[format.marginRightIndex]) : 0f;
float dialogueMarginVertical = format.marginVerticalIndex != C.INDEX_UNSET
? SsaStyle.parseMargin(lineValues[format.marginVerticalIndex]) : 0f;

Cue cue = createCue(
text,
style,
styleOverrides,
dialogueMarginLeft,
dialogueMarginRight,
dialogueMarginVertical,
screenWidth,
screenHeight);

int startTimeIndex = addCuePlacerholderByTime(startTimeUs, cueTimesUs, cues);
int endTimeIndex = addCuePlacerholderByTime(endTimeUs, cueTimesUs, cues);
Expand Down Expand Up @@ -384,65 +400,14 @@ private static Cue createCue(
String text,
@Nullable SsaStyle style,
SsaStyle.Overrides styleOverrides,
float dialogueMarginLeft,
float dialogueMarginRight,
float dialogueMarginVertical,
float screenWidth,
float screenHeight) {
SpannableString spannableText = new SpannableString(text);
Cue.Builder cue = new Cue.Builder().setText(spannableText);

if (style != null) {
if (style.primaryColor != null) {
spannableText.setSpan(
new ForegroundColorSpan(style.primaryColor),
/* start= */ 0,
/* end= */ spannableText.length(),
SpannableString.SPAN_EXCLUSIVE_EXCLUSIVE);
}
if (style.borderStyle == SsaStyle.SSA_BORDER_STYLE_BOX && style.outlineColor != null) {
spannableText.setSpan(
new BackgroundColorSpan(style.outlineColor),
/* start= */ 0,
/* end= */ spannableText.length(),
SpannableString.SPAN_EXCLUSIVE_EXCLUSIVE);
}
if (style.fontSize != Cue.DIMEN_UNSET && screenHeight != Cue.DIMEN_UNSET) {
cue.setTextSize(
style.fontSize / screenHeight, Cue.TEXT_SIZE_TYPE_FRACTIONAL_IGNORE_PADDING);
}
if (style.bold && style.italic) {
spannableText.setSpan(
new StyleSpan(Typeface.BOLD_ITALIC),
/* start= */ 0,
/* end= */ spannableText.length(),
SpannableString.SPAN_EXCLUSIVE_EXCLUSIVE);
} else if (style.bold) {
spannableText.setSpan(
new StyleSpan(Typeface.BOLD),
/* start= */ 0,
/* end= */ spannableText.length(),
SpannableString.SPAN_EXCLUSIVE_EXCLUSIVE);
} else if (style.italic) {
spannableText.setSpan(
new StyleSpan(Typeface.ITALIC),
/* start= */ 0,
/* end= */ spannableText.length(),
SpannableString.SPAN_EXCLUSIVE_EXCLUSIVE);
}
if (style.underline) {
spannableText.setSpan(
new UnderlineSpan(),
/* start= */ 0,
/* end= */ spannableText.length(),
SpannableString.SPAN_EXCLUSIVE_EXCLUSIVE);
}
if (style.strikeout) {
spannableText.setSpan(
new StrikethroughSpan(),
/* start= */ 0,
/* end= */ spannableText.length(),
SpannableString.SPAN_EXCLUSIVE_EXCLUSIVE);
}
}

@SsaStyle.SsaAlignment int alignment;
if (styleOverrides.alignment != SsaStyle.SSA_ALIGNMENT_UNKNOWN) {
alignment = styleOverrides.alignment;
Expand All @@ -461,11 +426,104 @@ private static Cue createCue(
cue.setPosition(styleOverrides.position.x / screenWidth);
cue.setLine(styleOverrides.position.y / screenHeight, LINE_TYPE_FRACTION);
} else {
// TODO: Read the MarginL, MarginR and MarginV values from the Style & Dialogue lines.
cue.setPosition(computeDefaultLineOrPosition(cue.getPositionAnchor()));
cue.setLine(computeDefaultLineOrPosition(cue.getLineAnchor()), LINE_TYPE_FRACTION);
}

// Apply margins if there are no overrides and we have valid positions.
if (styleOverrides.alignment == SsaStyle.SSA_ALIGNMENT_UNKNOWN
Copy link
Collaborator

Choose a reason for hiding this comment

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

why do we check specifically styleOverrides.alignment here when we've already resolved alignment from both styleOverrides and style on L411?

(related comment below)

&& styleOverrides.position == null
&& cue.getPosition() != Cue.DIMEN_UNSET
&& cue.getLine() != Cue.DIMEN_UNSET) {

// Margin from Dialogue lines takes precedence over margin from Style line.
float marginLeft = dialogueMarginLeft != 0f
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: there's a bit of naming confusion here imo

dialogueMarginLeft and style.marginLeft are values parsed pretty directly from the SSA file, so they are 'absolute' values in pixels.

But this local marginLeft value takes these and divides them by screenWidth, so it ends up being more 'fractional.

This distinction is important, and fiddly, so I think we should make it clear with the variable naming - probably by tweaking the name of this local to distinguish it from the other two?

? dialogueMarginLeft / screenWidth
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: imo double-nested ternaries are quite hard to read

I would suggest unrolling this to:

float marginLeft;
if (dialogueMarginLeft != 0f) {
  marginLeft = dialogueMarginLeft / screenWidth;
} else if (style != null) {
  marginLeft = style.marginLeft / screenWidth;
} else {
  marginLeft = 0f;
}

: style != null ? style.marginLeft / screenWidth : 0f;
float marginRight = dialogueMarginRight != 0f
? dialogueMarginRight / screenWidth
: style != null ? style.marginRight / screenWidth : 0f;

// Apply margin left, margin right.
if (SsaStyle.hasLeftAlignment(style)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't you use the alignment local from L411 here, rather than just the alignment derived from the style?

(related to comment above)

cue.setPosition(cue.getPosition() + marginLeft);
cue.setSize(1 - marginRight - marginLeft);
} else if (SsaStyle.hasRightAlignment(style)) {
cue.setPosition(cue.getPosition() - marginRight);
cue.setSize(1 - marginRight - marginLeft);
} else {
// Center alignment or unknown.
cue.setPosition(cue.getPosition() + (marginLeft - marginRight) / 2);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure I understand the maths here (or for the hasRightAlignment case above).

Cue.position always measures from the left hand side of the screen (for horizontal text cues), no matter what value Cue.textAlignment has. So shouldn't it always be increased by marginLeft?

In general applying these margins after the size/position has already been calculated seems hard to reason about - why not apply them as part of computing the size and position?

cue.setSize(1 - marginRight - marginLeft);
Copy link
Collaborator

Choose a reason for hiding this comment

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

this line is the same is all 3 if blocks - could probably pull it out?

}

// Apply margin vertical, ignore it when alignment is middle.
if (!SsaStyle.hasMiddleAlignment(style)) {
float marginVertical = dialogueMarginVertical != 0f ? dialogueMarginVertical / screenHeight
: style != null ? style.marginVertical / screenHeight : 0f;
cue.setLine(
cue.getLine() - (SsaStyle.hasTopAlignment(style) ? -marginVertical : marginVertical),
LINE_TYPE_FRACTION);
}
}

if (style == null) {
return cue.build();
}

// Apply rest of the styles.
if (style.primaryColor != null) {
spannableText.setSpan(
new ForegroundColorSpan(style.primaryColor),
/* start= */ 0,
/* end= */ spannableText.length(),
SpannableString.SPAN_EXCLUSIVE_EXCLUSIVE);
}
if (style.borderStyle == SsaStyle.SSA_BORDER_STYLE_BOX && style.outlineColor != null) {
spannableText.setSpan(
new BackgroundColorSpan(style.outlineColor),
/* start= */ 0,
/* end= */ spannableText.length(),
SpannableString.SPAN_EXCLUSIVE_EXCLUSIVE);
}
if (style.fontSize != Cue.DIMEN_UNSET && screenHeight != Cue.DIMEN_UNSET) {
cue.setTextSize(
style.fontSize / screenHeight, Cue.TEXT_SIZE_TYPE_FRACTIONAL_IGNORE_PADDING);
}
if (style.bold && style.italic) {
spannableText.setSpan(
new StyleSpan(Typeface.BOLD_ITALIC),
/* start= */ 0,
/* end= */ spannableText.length(),
SpannableString.SPAN_EXCLUSIVE_EXCLUSIVE);
} else if (style.bold) {
spannableText.setSpan(
new StyleSpan(Typeface.BOLD),
/* start= */ 0,
/* end= */ spannableText.length(),
SpannableString.SPAN_EXCLUSIVE_EXCLUSIVE);
} else if (style.italic) {
spannableText.setSpan(
new StyleSpan(Typeface.ITALIC),
/* start= */ 0,
/* end= */ spannableText.length(),
SpannableString.SPAN_EXCLUSIVE_EXCLUSIVE);
}
if (style.underline) {
spannableText.setSpan(
new UnderlineSpan(),
/* start= */ 0,
/* end= */ spannableText.length(),
SpannableString.SPAN_EXCLUSIVE_EXCLUSIVE);
}
if (style.strikeout) {
spannableText.setSpan(
new StrikethroughSpan(),
/* start= */ 0,
/* end= */ spannableText.length(),
SpannableString.SPAN_EXCLUSIVE_EXCLUSIVE);
}

return cue.build();
}

Expand Down
Loading