Add support for SSA (V4+) PrimaryColour style

[szaboa]

@icbaker This is an initial PR for SSA (V4+) PrimaryColour style.

A question in general, just to make sure we start this style support in the right way:

• Parsing of the style attributes should be done in the SsaStyle so then this class will have all the styles prepared to be used directly from the SsaDecoder, or the SsaStyle should be just a holder and the parsing needs to be done on the decoder side?

Open point:

• Not sure what is the code of the the primary colour style override, as the spec on this from Matroska is pretty confusing, and it is not aligned with this other spec. I'll try to figure it out by taking VLC for reference.

[icbaker]

Thanks for the PR - looks like a great start! I added some specific comments in the code, but only after that did I spot this part of the moodub spec (emphasis mine):

Field 4: PrimaryColour. A long integer BGR (blue-green-red) value. ie. the byte order in the hexadecimal equivelent of this number is BBGGRR

That sounds like these values will actually be (64-bit?) numbers. Combined with this line:

Field 4-7: The color format contains the alpha channel, too. (AABBGGRR)

This seems weird. Does this mean that in v4 there are 3 colors (B G R) and each one has 64/3=21 bits (with 3 unused bits)?

It makes a bit more sense in v4+ where it's clear that there are 4 colors, so each one can have 64/4 = 16 bits.

And then the fileformats.fandom spec you linked to has no mention of this numeric representation at all and just says this:

Color values are expressed in hexadecimal BGR format as &HBBGGRR& or ABGR (with alpha channel) as &HAABBGGRR&. Transparency (alpha) can be expressed as &HAA&. Note that in the alpha channel, 00 is opaque and FF is transparent.

I wonder if to be conservative we need to handle both? And presumably also be robust against missing leading zeroes (same as for the override format below).

So I think we should handle both files like this:

[V4+ Styles]
Format: Name, PrimaryColour
Style: Default,&H00FFFFFF

And like this:

[V4+ Styles]
Format: Name, PrimaryColour
Style: Default,16777215

---

• Parsing of the style attributes should be done in the SsaStyle so then this class will have all the styles prepared to be used directly from the SsaDecoder, or the SsaStyle should be just a holder and the parsing needs to be done on the decoder side?

Since the alignment parsing logic is already inside SsaStyle, let's keep following that pattern - so what you've done (add new static methods) is perfect.

• Not sure what is the code of the the primary colour style override, as the spec on this from Matroska is pretty confusing, and it is not aligned with this other spec. I'll try to figure it out by taking VLC for reference.

I think the two specs you linked are aligned. It seems the number before the c is optional - although neither spec seems to make it clear what it means if it's missing - presumably it means the foreground color? That would seem like a sensible default to me. The moodub doc says "\alpha defaults to \1a" so I think we can extrapolate that "\c defaults to \1c"

So you can either use {\c...} or {\1c...}

There's some trickiness in the moodub doc which I think we should handle:

Leading zeroes are not required. e.g.

• {\c&HFF&} This is pure, full intensity red
• {\c&HFF00&} This is pure, full intensity Green
• {\c&HFF0000&} This is pure, full intensity Blue
• {\c&HFFFFFF&} This is White
• {\c&HA0A0A&} This is dark grey

It seems reasonable to split the Format line support away from the override support, so each PR can stay small and focussed. Do you want to add a new column to your table indicating whether ExoPlayer supports the property in Format lines vs in overrides? Then imo let's keep this PR focussed on the Format line, and once we've got that hammered out you can then raise a second one to add override support.

[icbaker]

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.

[szaboa]

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

[szaboa]

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?

[szaboa]

I wonder if to be conservative we need to handle both? And presumably also be robust against missing leading zeroes (same as for the override format below).

Yep, let's handle both of them.

Do you want to add a new column to your table indicating whether ExoPlayer supports the property in Format lines vs in overrides? Then imo let's keep this PR focussed on the Format line, and once we've got that hammered out you can then raise a second one to add override support.

Yep, I'll add a new column to the table for the overrides, and agree with you that we should first focus on the Format line.

I'll update the PR when everything is ready :)

[szaboa]

Handling both the hex- and decimal format of the colors has been added. Note that the original alpha value needs to be inverted, because 0xFF indicates full transparency in case of SSA (V4+) - this is based on the tests performed on VLC and also fileformats.fandom specs states this. And because we decided to handle the leading zeros, the V4 hex colors are also supported out of the box.

[icbaker]

Looks good! If you can add the testdata + test then I'll work on getting this merged.

Thanks for this, it's great :)

[icbaker]

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 :)

[szaboa]

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 :)

[icbaker]

(I'm not sure this is a good idea, just a thought)

What about making these private and having an int getColor() method that throws an exception if isSet == false? There would also be a boolean isSet() method. This is similar to Optional.get().

(in fact this class probably wouldn't be needed if we could use Java 8's OptionalInt, but I think our minSdk level is still too low for that)

[szaboa]

I like this idea, I've changed the code the way you described.

[icbaker]

should this use rgbaStringBuilder?

[szaboa]

of course it should use that, sorry it was too late yesterday :) I'll fix it.

[szaboa]

Testdata + test added, ready for review.

[icbaker]

Field 4: PrimaryColour. A long integer BGR (blue-green-red) value. ie. the byte order in the hexadecimal equivelent of this number is BBGGRR

That sounds like these values will actually be (64-bit?) numbers. Combined with this line:

Field 4-7: The color format contains the alpha channel, too. (AABBGGRR)

This seems weird. Does this mean that in v4 there are 3 colors (B G R) and each one has 64/3=21 bits (with 3 unused bits)?

It makes a bit more sense in v4+ where it's clear that there are 4 colors, so each one can have 64/4 = 16 bits.

I've spent a bit more time thinking about this, and playing with your sample file in VLC (thanks!). I think the "long integer" doesn't mean 64 bits, it means an unsigned 32-bit integer. And (obviously?) in the 3-color case only the lowest 24 bits are used (top 8 bits are just always zero).

Your parsing code seems correct but I realised a couple of things:

1. You don't need to manually add zeroes to the string before parsing, they're implicit when you parse to int (i.e. Long.parseLong("123") == Long.parseLong("000123"), and equally Long.parseLong("ABC", 16) == Long.parseLong("000ABC", 16)
2. It's probably safer to just use a long for storing the bits, to avoid hitting any weird java behaviour around automatic propagation of negative values if the 32nd bit of the input is high.

So I'm going to make those tweaks as I merge it.

[szaboa]

I've spent a bit more time thinking about this, and playing with your sample file in VLC (thanks!). I think the "long integer" doesn't mean 64 bits, it means an unsigned 32-bit integer. And (obviously?) in the 3-color case only the lowest 24 bits are used (top 8 bits are just always zero).

Yes, I came to the same conclusion when I was testing out different color representations in VLC.

You don't need to manually add zeroes to the string before parsing

You are right, good catch :)