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.

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 #10169

Open
wants to merge 6 commits into
base: dev-v2
Choose a base branch
from

Conversation

szaboa
Copy link
Contributor

@szaboa szaboa commented Apr 9, 2022

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 icbaker self-requested a review April 11, 2022 10:37
@icbaker icbaker self-assigned this Apr 11, 2022
@icbaker
Copy link
Collaborator

icbaker commented Apr 13, 2022

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
Copy link
Contributor Author

szaboa commented Apr 17, 2022

@icbaker Ready to be reviewed again. :)

@Rootax
Copy link

Rootax commented May 8, 2022

@icbaker Ready to be reviewed again. :)

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

@Rootax
Copy link

Rootax commented May 22, 2022

@icbaker Ready to be reviewed again. :)

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

@szaboa szaboa requested a review from icbaker May 27, 2022 12:04
@icbaker
Copy link
Collaborator

icbaker commented Jun 1, 2022

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.

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
&& styleOverrides.position == null
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@szaboa szaboa Jun 1, 2022

Choose a reason for hiding this comment

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

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

@icbaker
Copy link
Collaborator

icbaker commented Jun 1, 2022

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

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:

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
Copy link
Contributor Author

szaboa commented Jun 1, 2022

@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
Copy link

Rootax commented Jul 18, 2022

Is this ready to be merged ?

@sjiawjbssj
Copy link

@icbaker could you please take a look at this

@szaboa
Copy link
Contributor Author

szaboa commented Sep 5, 2023

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

@FongMi
Copy link

FongMi commented Nov 15, 2023

@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
Copy link

@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
Copy link

sjiawjbssj commented Aug 1, 2024

@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
Copy link
Contributor Author

szaboa commented Oct 17, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants