Fix cue render order for ass/ssa. #2137
Conversation
|
@icbaker Could you take a look? Thanks! |
| * An {@link Ordering} which sorts cues in ascending layer priority | ||
| */ | ||
| private static final Ordering<Cue> CUES_LAYER_PRIORITY_COMPARATOR = | ||
| Ordering.<Integer>natural().onResultOf(c -> c.layer); |
There was a problem hiding this comment.
I think this logic should be part of CUES_DISPLAY_PRIORITY_COMPARATOR rather than a separate Ordering object
There was a problem hiding this comment.
CUES_DISPLAY_PRIORITY_COMPARATOR with CuesWithTiming, but the layer is in Cue, not in CuesWithTiming.
| /** | ||
| * The layer for cue, the larger cue will render above the smaller cue. | ||
| */ | ||
| public final int layer; |
There was a problem hiding this comment.
nit: TTML (which has a much more robust & unambiguous spec than SSA) uses the name zIndex for this. It seems to have the same "higher index renders above lower index" semantics.
Shall we use that nomenclature here? So Cue.zIndex
We should keep the layer terminology everywhere in the ssa package.
There was a problem hiding this comment.
I'm not familiar with TTML, but I can change the layer to zIndex, if you think it is more stand.
| * Gets the layer for this Cue. | ||
| */ | ||
| @Pure | ||
| public @VerticalType int getLayer() { |
There was a problem hiding this comment.
If this isn't needed I suggest we remove it
There was a problem hiding this comment.
If this isn't needed I suggest we remove it
Yes, I forget to delete the type annotation.
| try { | ||
| return Integer.parseInt(intString.trim()); | ||
| } catch (Exception exception) { | ||
| return C.LENGTH_UNSET; |
There was a problem hiding this comment.
I think we should default to 0 here for layer, same as Cue
That probably means you need to either return null here, or take a default value as a parameter, or don't define this general purpose function and just in-line it for the layer case.
I'd suggest in-lining
There was a problem hiding this comment.
parseInt is a general method, and I will set layer to 0 when the result is unset in line 333.
| @@ -184,7 +185,8 @@ public void draw( | |||
| && this.parentLeft == cueBoxLeft | |||
| && this.parentTop == cueBoxTop | |||
| && this.parentRight == cueBoxRight | |||
| && this.parentBottom == cueBoxBottom) { | |||
| && this.parentBottom == cueBoxBottom | |||
| && this.cueLayer == cue.layer) { | |||
There was a problem hiding this comment.
What happens if you remove this comparison? It seems a bit surprising you need to store cueLayer into a field and then only use it in this comparison. I think all the other fields being compared here are then later used to compute the layout (hence they need to be checked to see if the layout needs to be invalidated).
There was a problem hiding this comment.
Due to your explain, it seems no need to compair the layer.
| @@ -68,7 +75,7 @@ public class CuesWithTiming { | |||
|
|
|||
| /** Creates an instance. */ | |||
| public CuesWithTiming(List<Cue> cues, long startTimeUs, long durationUs) { | |||
| this.cues = ImmutableList.copyOf(cues); | |||
| this.cues = ImmutableList.sortedCopyOf(CUES_LAYER_PRIORITY_COMPARATOR, cues); | |||
There was a problem hiding this comment.
CuesWithTiming.cues has no documented ordering behaviour wrt layout on screen.
CuesGroup.cue does document how ordering affects layout.
Is this reordering needed here? It seems it shouldn't be, since the field has no ordering guarantee. Maybe this should be moved to a point where a CueGroup is being created?
There was a problem hiding this comment.
Yes, you are right. That's a good way. and it seems there is no need to modify the MergingCuesResolver.
This should fix #2124