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

[szaboa]

This PR is about adding support for MarginL, MarginR, MarginV from both the Style and Dialogue lines. I've used VLC as reference.

• Non-zero margins defined in Dialogue lines takes priority over the margins in Style lines.
• In case we have a {\pos} override, then no margins will be applied neither from Dialogue or Style (same as in VLC).
• In case we have a {\an} override, then VLC ignores the Dialogue margin but applies the Style margin which makes no sense for me, so in our case I've just ignored both Dialogue and Style margins (same as {\pos} case).

Suggestion: maybe we could update the https://storage.googleapis.com/exoplayer-test-media-1/ssa/test-subs-position.ass subtitle file with these new lines, then no need for a new media in media.exolist.json.

[icbaker]

Suggestion: maybe we could update the https://storage.googleapis.com/exoplayer-test-media-1/ssa/test-subs-position.ass subtitle file with these new lines, then no need for a new media in media.exolist.json.

Yep, sounds good - I'll update the file as part of merging this.

[szaboa]

@icbaker Ready to be reviewed again. :)

[Rootax]

@icbaker Ready to be reviewed again. :)

Thx you, we've missed .ass/ssa support for years... I hope this can be merged soon.

[Rootax]

@icbaker Ready to be reviewed again. :)

It's a pretty important merge, I hope this can be reviewed soon...

[icbaker]

Apologies for the delay on this - I'm looking at it now.

I've patched it internally and I'm going to make some changes before sending it for internal review, so please don't upload any more commits here. One thing I'm going to change is reshuffle the logic in SsaDecoder to avoid setting the cue.position and cue.line values without considering the margins, and then changing them later. Instead I plan to just calculate them once, considering the margin values from the beginning (if set). I think this will be easier to reason about.

[icbaker]

Is this logic implying that the margin values are completely ignored if the cue contains \pos position overrides? Is that the expected behaviour?

Ditto for alignment overrides - why should they cause the margins to be ignored?

[szaboa]

I am not sure if this is the expected behaviour but I took VLC for reference, where it is ignored.

[icbaker]

Another question: We currently use a default margin value of 5%:

ExoPlayer/library/extractor/src/main/java/com/google/android/exoplayer2/text/ssa/SsaDecoder.java

Line 59 in c386644

private static final float DEFAULT_MARGIN = 0.05f;

ExoPlayer/library/extractor/src/main/java/com/google/android/exoplayer2/text/ssa/SsaDecoder.java

Lines 462 to 474 in c386644

	private static float computeDefaultLineOrPosition(@Cue.AnchorType int anchor) {
	switch (anchor) {
	case Cue.ANCHOR_TYPE_START:
	return DEFAULT_MARGIN;
	case Cue.ANCHOR_TYPE_MIDDLE:
	return 0.5f;
	case Cue.ANCHOR_TYPE_END:
	return 1.0f - DEFAULT_MARGIN;
	case Cue.TYPE_UNSET:
	default:
	return Cue.DIMEN_UNSET;
	}
	}

This value is used if we don't know PlayResX or PlayResX or if the cue doesn't have a position override:

ExoPlayer/library/extractor/src/main/java/com/google/android/exoplayer2/text/ssa/SsaDecoder.java

Lines 381 to 390 in c386644

	if (styleOverrides.position != null
	&& screenHeight != Cue.DIMEN_UNSET
	&& screenWidth != Cue.DIMEN_UNSET) {
	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);
	}

I believe your current implementation still uses this default value as a starting point, and adds any margin from the SSA file on top. This doesn't really seem right (shouldn't we just use the file-specified margin directly?). But equally, removing this default completely would result in a file that doesn't specify a margin at all changing from 'nicely' indented cues to cues that are hard against the edge of the screen. This kinda seems 'correct' too - what do you think?

The other option is that we use the default margin when the file doesn't specify a margin (in neither style nor dialogue line).

[szaboa]

@icbaker

I believe your current implementation still uses this default value as a starting point, and adds any margin from the SSA file on top.

This is right.

If we want to keep strictly to the standard then removing the default margin is the way to go (VLC does the same, if no margin is specified then the subtitle is rendered on the edges). However probably a couple of devs/customers got used to the default margin, so I'd say we can go with this:

The other option is that we use the default margin when the file doesn't specify a margin (in neither style nor dialogue line).

Note that if you decide to remove the default margin in the end, then the unit tests needs to be corrected. :)

[Rootax]

Is this ready to be merged ?

[sjiawjbssj]

@icbaker could you please take a look at this

[szaboa]

@icbaker what's up with this? should I resolve the conflicts or this is too outdated already? :)

[FongMi]

@szaboa How to fix the problem ?
My subtitleView is setApplyEmbeddedFontSizes(false);
So can adjust ass text size.
If size too large, still overlapping.

Screen_recording_20231115_155328.mp4

[GlassedSilver]

@icbaker what's up with this? should I resolve the conflicts or this is too outdated already? :)

I assume it's worth trying to fix the conflicts since at the very least it'd show implementing this much needed feature that a lot of other apps would immediately profit from wouldn't fail to implement due to lack of developer initiative.

Google certainly feels the need to keep up with media playback needs, that's obvious otherwise they wouldn't have just announced to switch to VideoLAN's AV1 software decoder for devices dating back to Android 12 even.

(they are switching away from their own developed decoder since it didn't reach VideoLAN's efficiency)

[sjiawjbssj]

@szaboa not sure if you've already read it but @icbaker said they are willing to take another look if this PR is re opened in the androidx media repository and the conflicts are resolved.
See: androidx/media#1482 (comment)

[szaboa]

@sjiawjbssj I've missed this, i'll try to find some time to re-open the PR in androidx/media. But no promises when that's going to happen :)