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

Jump to bottom

Add support for SSA (V4+) PrimaryColour style #8490

Merged
merged 5 commits into from
Feb 1, 2021
Merged

Add support for SSA (V4+) PrimaryColour style #8490

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0ead2af
Add support for SSA (V4+) PrimaryColour style
szaboa Jan 21, 2021
1364c01
Improve support of SSA (V4+) PrimaryColour style
szaboa Jan 25, 2021
2241320
Remove unused static variable
szaboa Jan 25, 2021
f8a47bc
Modify the SsaColor to be more similar to the Optional class.
szaboa Jan 26, 2021
28a3921
Add color decoding tests to SsaDecoderTest, remove SubStation Alpha c…
szaboa Jan 26, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
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 @@ -506,6 +506,13 @@
"subtitle_mime_type": "text/x-ssa",
"subtitle_language": "en"
},
{
"name": "SubStation Alpha colors",
"uri": "https://storage.googleapis.com/exoplayer-test-media-1/gen-3/screens/dash-vod-single-segment/video-avc-baseline-480.mp4",
"subtitle_uri": "https://drive.google.com/uc?export=download&id=13EdW4Qru-vQerUlwS_Ht5Cely_Tn0tQe",
"subtitle_mime_type": "text/x-ssa",
"subtitle_language": "en"
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we probably won't add this to the demo app, and instead update the test-subs-position.ass file above to include a range of SSA/ASS features.

If you'd like to have a go at crafting that file (would probably make sense to include features we don't support yet too) that would be cool - you can send a standalone PR for it and I'll upload it to the storage bucket.

This colors-only file you've crafted is still handy - I think we should turn it into automated test data in this PR. You can put it here:
https://github.com/google/ExoPlayer/tree/dev-v2/testdata/src/test/assets/media/ssa

And then add a test using it here:
https://github.com/google/ExoPlayer/blob/dev-v2/library/core/src/test/java/com/google/android/exoplayer2/text/ssa/SsaDecoderTest.java

That will help make sure future editors don't mess up the careful bit twiddling :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the entry from media.exolist.json. And of course, I can try to craft a file with more features - but I would wait a bit until we add 1-2 more style support just to get more familiar with it :)

{
"name": "MPEG-4 Timed Text",
"uri": "https://storage.googleapis.com/exoplayer-test-media-1/mp4/dizzy-with-tx3g.mp4"
Expand Down
15 changes: 13 additions & 2 deletions library/core/src/main/java/com/google/android/exoplayer2/text/ssa/SsaDecoder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
import static com.google.android.exoplayer2.util.Util.castNonNull;

import android.text.Layout;
import android.text.SpannableString;
import android.text.style.ForegroundColorSpan;
import androidx.annotation.Nullable;
import com.google.android.exoplayer2.C;
import com.google.android.exoplayer2.text.Cue;
Expand Down Expand Up @@ -301,8 +303,17 @@ private static Cue createCue(
SsaStyle.Overrides styleOverrides,
float screenWidth,
float screenHeight) {
Cue.Builder cue = new Cue.Builder().setText(text);

SpannableString spannableText = new SpannableString(text);
Cue.Builder cue = new Cue.Builder().setText(spannableText);

// Apply primary color.
if (style != null) {
if (style.primaryColor != SsaStyle.SSA_COLOR_UNKNOWN) {
spannableText.setSpan(new ForegroundColorSpan(style.primaryColor),
0, spannableText.length(), SpannableString.SPAN_EXCLUSIVE_EXCLUSIVE);
}
}
// Apply alignment.
@SsaStyle.SsaAlignment int alignment;
if (styleOverrides.alignment != SsaStyle.SSA_ALIGNMENT_UNKNOWN) {
alignment = styleOverrides.alignment;
Expand Down
34 changes: 30 additions & 4 deletions library/core/src/main/java/com/google/android/exoplayer2/text/ssa/SsaStyle.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,12 @@

import android.graphics.PointF;
import android.text.TextUtils;
import androidx.annotation.ColorInt;
import androidx.annotation.IntDef;
import androidx.annotation.Nullable;
import com.google.android.exoplayer2.C;
import com.google.android.exoplayer2.util.Assertions;
import com.google.android.exoplayer2.util.ColorParser;
import com.google.android.exoplayer2.util.Log;
import com.google.android.exoplayer2.util.Util;
import java.lang.annotation.Documented;
Expand Down Expand Up @@ -83,12 +85,16 @@
public static final int SSA_ALIGNMENT_TOP_CENTER = 8;
public static final int SSA_ALIGNMENT_TOP_RIGHT = 9;

public static final int SSA_COLOR_UNKNOWN = -1;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unfortunately I don't think you can do this, because the Android ColorInt representation uses all 32 bits to represent the 4 ARGB channels (8 bits each), so there are no 'reserved' invalid values.

Specifically -1 is 0xFFFFFFFF, i.e. fully transparent black - which means if someone writes a SSA file with a format line with primaryColour="&HFFFFFFFF" then your code in SsaDecoder will ignore this (thinking it's unset).

Arguably 100% transparent colors are silly, but i think it's still confusing to arbitrarily declare a valid part of the color-space to be our token 'invalid' value.

I think you probably need to track "primary color set" as a separate boolean - see Cue.windowColor and windowColorSet for somewhere we've had to do a similar thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I missed it.. big time :) I'll fix it similarly how the windowColorSet is solved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the end I've decided to go with a dedicated SsaColor class which holds this state, because we'll have multiple color attributes in the style which would lead to having a lot of xyColorSet flags. What do you think?


public final String name;
@SsaAlignment public final int alignment;
@ColorInt public int primaryColor;

private SsaStyle(String name, @SsaAlignment int alignment) {
private SsaStyle(String name, @SsaAlignment int alignment, @ColorInt int primaryColor) {
this.name = name;
this.alignment = alignment;
this.primaryColor = primaryColor;
}

@Nullable
Expand All @@ -105,7 +111,9 @@ public static SsaStyle fromStyleLine(String styleLine, Format format) {
}
try {
return new SsaStyle(
styleValues[format.nameIndex].trim(), parseAlignment(styleValues[format.alignmentIndex]));
styleValues[format.nameIndex].trim(),
parseAlignment(styleValues[format.alignmentIndex]),
parsePrimaryColor(styleValues[format.primaryColorIndex]));
} catch (RuntimeException e) {
Log.w(TAG, "Skipping malformed 'Style:' line: '" + styleLine + "'", e);
return null;
Expand Down Expand Up @@ -144,6 +152,16 @@ private static boolean isValidAlignment(@SsaAlignment int alignment) {
}
}

@ColorInt
private static int parsePrimaryColor(String primaryColorStr) {
try {
return ColorParser.parseSsaColor(primaryColorStr);
} catch (IllegalArgumentException ex) {
Log.w(TAG, "Failed parsing color value: " + primaryColorStr);
}
return SSA_COLOR_UNKNOWN;
}

/**
* Represents a {@code Format:} line from the {@code [V4+ Styles]} section
*
Expand All @@ -154,11 +172,13 @@ private static boolean isValidAlignment(@SsaAlignment int alignment) {

public final int nameIndex;
public final int alignmentIndex;
public final int primaryColorIndex;
public final int length;

private Format(int nameIndex, int alignmentIndex, int length) {
private Format(int nameIndex, int alignmentIndex, int primaryColorIndex, int length) {
this.nameIndex = nameIndex;
this.alignmentIndex = alignmentIndex;
this.primaryColorIndex = primaryColorIndex;
this.length = length;
}

Expand All @@ -171,6 +191,7 @@ private Format(int nameIndex, int alignmentIndex, int length) {
public static Format fromFormatLine(String styleFormatLine) {
int nameIndex = C.INDEX_UNSET;
int alignmentIndex = C.INDEX_UNSET;
int primaryColorIndex = C.INDEX_UNSET;
String[] keys =
TextUtils.split(styleFormatLine.substring(SsaDecoder.FORMAT_LINE_PREFIX.length()), ",");
for (int i = 0; i < keys.length; i++) {
Expand All @@ -181,9 +202,14 @@ public static Format fromFormatLine(String styleFormatLine) {
case "alignment":
alignmentIndex = i;
break;
case "primarycolour":
primaryColorIndex = i;
icbaker marked this conversation as resolved.
Show resolved Hide resolved
break;
}
}
return nameIndex != C.INDEX_UNSET ? new Format(nameIndex, alignmentIndex, keys.length) : null;
return nameIndex != C.INDEX_UNSET
? new Format(nameIndex, alignmentIndex, primaryColorIndex, keys.length)
: null;
}
}

Expand Down
21 changes: 21 additions & 0 deletions library/core/src/main/java/com/google/android/exoplayer2/util/ColorParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,27 @@ public static int parseCssColor(String colorExpression) {
return parseColorInternal(colorExpression, true);
}

/**
* Parses a SSA V4+ color expression.
*
* @param colorExpression The color expression.
* @return The parsed ARGB color.
*/
@ColorInt
public static int parseSsaColor(String colorExpression) {
// SSA V4+ color format is &HAABBGGRR.
if (colorExpression.length() != 10 || !"&H".equals(colorExpression.substring(0, 2))) {
icbaker marked this conversation as resolved.
Show resolved Hide resolved
throw new IllegalArgumentException();
}
// Convert &HAABBGGRR to #RRGGBBAA.
String rgba = new StringBuilder()
.append(colorExpression.substring(2))
.append("#")
.reverse()
.toString();
return parseColorInternal(rgba, true);
}

@ColorInt
private static int parseColorInternal(String colorExpression, boolean alphaHasFloatFormat) {
Assertions.checkArgument(!TextUtils.isEmpty(colorExpression));
Expand Down
5 changes: 5 additions & 0 deletions library/core/src/test/java/com/google/android/exoplayer2/util/ColorParserTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,10 @@
import static android.graphics.Color.BLACK;
import static android.graphics.Color.RED;
import static android.graphics.Color.WHITE;
import static android.graphics.Color.YELLOW;
import static android.graphics.Color.argb;
import static android.graphics.Color.parseColor;
import static com.google.android.exoplayer2.util.ColorParser.parseSsaColor;
import static com.google.android.exoplayer2.util.ColorParser.parseTtmlColor;
import static com.google.common.truth.Truth.assertThat;

Expand Down Expand Up @@ -64,6 +66,9 @@ public void hexCodeParsing() {
// Hex colors in ColorParser are RGBA, where-as {@link Color#parseColor} takes ARGB.
assertThat(parseTtmlColor("#FFFFFF00")).isEqualTo(parseColor("#00FFFFFF"));
assertThat(parseTtmlColor("#12345678")).isEqualTo(parseColor("#78123456"));
// SSA colors are in &HAABBGGRR format.
assertThat(parseSsaColor("&HFF0000FF")).isEqualTo(RED);
assertThat(parseSsaColor("&HFF00FFFF")).isEqualTo(YELLOW);
}

@Test
Expand Down