Add support for SSA (v4+) MarginL, MarginR, MarginV style #10169
Conversation
library/extractor/src/main/java/com/google/android/exoplayer2/t
8000
ext/ssa/SsaDecoder.java
Outdated
Show resolved
Hide resolved
library/extractor/src/main/java/com/google/android/exoplayer2/text/ssa/SsaDecoder.java
Outdated
Show resolved
Hide resolved
library/extractor/src/main/java/com/google/android/exoplayer2/text/ssa/SsaStyle.java
Show resolved
Hide resolved
Yep, sounds good - I'll update the file as part of merging this. |
|
@icbaker Ready to be reviewed again. :) |
Thx you, we've missed .ass/ssa support for years... I hope this can be merged soon. |
It's a pretty important merge, I hope this can be reviewed soon... |
|
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 |
| 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I am not sure if this is the expected behaviour but I took VLC for reference, where it is ignored.
|
Another question: We currently use a default margin value of 5%: This value is used if we don't know 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). |
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:
Note that if you decide to remove the default margin in the end, then the unit tests needs to be corrected. :) |
|
Is this ready to be merged ? |
|
@icbaker could you please take a look at this |
|
@icbaker what's up with this? should I resolve the conflicts or this is too outdated already? :) |
|
@szaboa How to fix the problem ? Screen_recording_20231115_155328.mp4 |
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) |
|
@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. |
|
@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 :) |
|
@sjiawjbssj reopened here androidx/media#2008. |
This PR is about adding support for
MarginL,MarginR,MarginVfrom both theStyleandDialoguelines. I've used VLC as reference.Dialoguelines takes priority over the margins inStylelines.{\pos}override, then no margins will be applied neither fromDialogueorStyle(same as in VLC).{\an}override, then VLC ignores theDialoguemargin but applies theStylemargin which makes no sense for me, so in our case I've just ignored bothDialogueandStylemargins (same as{\pos}case).Suggestion: maybe we could update the
https://storage.googleapis.com/exoplayer-test-media-1/ssa/test-subs-position.asssubtitle file with these new lines, then no need for a new media inmedia.exolist.json.