Add support for SSA (v4+) MarginL, MarginR, MarginV style #2008
Conversation
|
Add ass feature for exoplayer use Java code mabe a huge work。 |
There was a problem hiding this comment.
I only part-reviewed this, because my main comment is about whether we could make the code easier to reason about by applying the margins when initially calculating position/size, rather than trying to apply them in a second pass - and this might result in relatively major changes.
| && cue.getLine() != Cue.DIMEN_UNSET) { | ||
|
|
||
| // Margin from Dialogue lines takes precedence over margin from Style line. | ||
| float marginLeft = dialogueMarginLeft != 0f |
There was a problem hiding this comment.
nit: there's a bit of naming confusion here imo
dialogueMarginLeft and style.marginLeft are values parsed pretty directly from the SSA file, so they are 'absolute' values in pixels.
But this local marginLeft value takes these and divides them by screenWidth, so it ends up being more 'fractional.
This distinction is important, and fiddly, so I think we should make it clear with the variable naming - probably by tweaking the name of this local to distinguish it from the other two?
|
|
||
| // Margin from Dialogue lines takes precedence over margin from Style line. | ||
| float marginLeft = dialogueMarginLeft != 0f | ||
| ? dialogueMarginLeft / screenWidth |
There was a problem hiding this comment.
nit: imo double-nested ternaries are quite hard to read
I would suggest unrolling this to:
float marginLeft;
if (dialogueMarginLeft != 0f) {
marginLeft = dialogueMarginLeft / screenWidth;
} else if (style != null) {
marginLeft = style.marginLeft / screenWidth;
} else {
marginLeft = 0f;
}| : style != null ? style.marginRight / screenWidth : 0f; | ||
|
|
||
| // Apply margin left, margin right. | ||
| if (SsaStyle.hasLeftAlignment(style)) { |
There was a problem hiding this comment.
Shouldn't you use the alignment local from L411 here, rather than just the alignment derived from the style?
(related to comment above)
| 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 |
There was a problem hiding this comment.
why do we check specifically styleOverrides.alignment here when we've already resolved alignment from both styleOverrides and style on L411?
(related comment below)
| } else { | ||
| // Center alignment or unknown. | ||
| cue.setPosition(cue.getPosition() + (marginLeft - marginRight) / 2); | ||
| cue.setSize(1 - marginRight - marginLeft); |
There was a problem hiding this comment.
this line is the same is all 3 if blocks - could probably pull it out?
| cue.setSize(1 - marginRight - marginLeft); | ||
| } else { | ||
| // Center alignment or unknown. | ||
| cue.setPosition(cue.getPosition() + (marginLeft - marginRight) / 2); |
There was a problem hiding this comment.
I'm not sure I understand the maths here (or for the hasRightAlignment case above).
Cue.position always measures from the left hand side of the screen (for horizontal text cues), no matter what value Cue.textAlignment has. So shouldn't it always be increased by marginLeft?
In general applying these margins after the size/position has already been calculated seems hard to reason about - why not apply them as part of computing the size and position?
Reopening google/ExoPlayer#10169.
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.