Skip to content

Commit b312b30

Browse files
johnpcjohnpc
committed
Address PR review comments
- Update copyright year to 2026 in test files - Rename getLastDrawnTextHeight() to getLastDrawnTextBottom() with clearer docs - Handle positive line numbers (top-stacked cues) in addition to negative - Add tests for positive line numbers and mixed top/bottom cues
1 parent e0dd3fd commit b312b30

4 files changed

Lines changed: 523 additions & 2 deletions

File tree

‎libraries/ui/src/main/java/androidx/media3/ui/CanvasSubtitleOutput.java‎

Lines changed: 31 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -104,12 +104,34 @@ public void dispatchDraw(Canvas canvas) {
104104
return;
105105
}
106106

107+
// Track cumulative offsets for stacked cues to prevent overlapping.
108+
// Bottom-stacked cues (negative line numbers) stack upward from the bottom.
109+
// Top-stacked cues (positive line numbers) stack downward from the top.
110+
int cumulativeBottomOffset = 0;
111+
int cumulativeTopOffset = 0;
112+
107113
int cueCount = cues.size();
108114
for (int i = 0; i < cueCount; i++) {
109115
Cue cue = cues.get(i);
110116
if (cue.verticalType != Cue.TYPE_UNSET) {
111117
cue = repositionVerticalCue(cue);
112118
}
119+
120+
// Determine if this cue is stacked from bottom or top based on line number.
121+
boolean isBottomStackedCue =
122+
cue.line != Cue.DIMEN_UNSET && cue.lineType == Cue.LINE_TYPE_NUMBER && cue.line < 0;
123+
boolean isTopStackedCue =
124+
cue.line != Cue.DIMEN_UNSET && cue.lineType == Cue.LINE_TYPE_NUMBER && cue.line >= 0;
125+
126+
// Adjust boundaries to account for previously drawn cues.
127+
int adjustedTop = top;
128+
int adjustedBottom = bottom;
129+
if (isBottomStackedCue && cumulativeBottomOffset > 0) {
130+
adjustedBottom = bottom - cumulativeBottomOffset;
131+
} else if (isTopStackedCue && cumulativeTopOffset > 0) {
132+
adjustedTop = top + cumulativeTopOffset;
133+
}
134+
113135
float cueTextSizePx =
114136
SubtitleViewUtils.resolveTextSize(
115137
cue.textSizeType, cue.textSize, rawViewHeight, viewHeightMinusPadding);
@@ -122,9 +144,16 @@ public void dispatchDraw(Canvas canvas) {
122144
bottomPaddingFraction,
123145
canvas,
124146
left,
125-
top,
147+
adjustedTop,
126148
right,
127-
bottom);
149+
adjustedBottom);
150+
151+
// Accumulate offset so subsequent cues don't overlap.
152+
if (isBottomStackedCue) {
153+
cumulativeBottomOffset += painter.getLastDrawnTextBottom();
154+
} else if (isTopStackedCue) {
155+
cumulativeTopOffset += painter.getLastDrawnTextBottom();
156+
}
128157
}
129158
}
130159

‎libraries/ui/src/main/java/androidx/media3/ui/SubtitlePainter.java‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,9 @@
5252
/** Ratio of inner padding to font size. */
5353
private static final float INNER_PADDING_RATIO = 0.125f;
5454

55+
/** The bottom Y position of the last drawn text cue, or 0 if no text cue was drawn. */
56+
private int lastDrawnTextBottom;
57+
5558
// Styled dimensions.
5659
private final float outlineWidth;
5760
private final float shadowRadius;
@@ -218,13 +221,28 @@ public void draw(
218221
if (isTextCue) {
219222
checkNotNull(cueText);
220223
setupTextLayout();
224+
lastDrawnTextBottom = textLayout != null ? textLayout.getHeight() : 0;
221225
} else {
222226
checkNotNull(cueBitmap);
223227
setupBitmapLayout();
228+
lastDrawnTextBottom = 0;
224229
}
225230
drawLayout(canvas, isTextCue);
226231
}
227232

233+
/**
234+
* Returns the bottom Y position of the last drawn text cue relative to the parent bounds.
235+
*
236+
* <p>This can be used to stack multiple cues without overlap. For bottom-stacked cues (negative
237+
* line numbers), this represents how far up from the bottom the cue extends. For top-stacked cues
238+
* (positive line numbers), this represents how far down from the top the cue extends.
239+
*
240+
* @return The bottom Y position of the last drawn text cue, or 0 if no text cue was drawn.
241+
*/
242+
public int getLastDrawnTextBottom() {
243+
return lastDrawnTextBottom;
244+
}
245+
228246
@RequiresNonNull("cueText")
229247
private void setupTextLayout() {
230248
SpannableStringBuilder cueText =

‎libraries/ui/src/test/java/androidx/media3/ui/CanvasSubtitleOutputTest.java‎

Lines changed: 273 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,273 @@
1+
/*
2+
* Copyright 2026 The Android Open Source Project
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
package androidx.media3.ui;
17+
18+
import static com.google.common.truth.Truth.assertThat;
19+
20+
import android.content.Context;
21+
import android.graphics.Bitmap;
22+
import android.graphics.Canvas;
23+
import android.graphics.Color;
24+
import androidx.media3.common.text.Cue;
25+
import androidx.test.core.app.ApplicationProvider;
26+
import androidx.test.ext.junit.runners.AndroidJUnit4;
27+
import java.util.Arrays;
28+
import java.util.Collections;
29+
import java.util.List;
30+
import org.junit.Before;
31+
import org.junit.Test;
32+
import org.junit.runner.RunWith;
33+
34+
/** Tests for {@link CanvasSubtitleOutput}. */
35+
@RunWith(AndroidJUnit4.class)
36+
public class CanvasSubtitleOutputTest {
37+
38+
private static final int VIEW_WIDTH = 1920;
39+
private static final int VIEW_HEIGHT = 1080;
40+
41+
private Context context;
42+
private CanvasSubtitleOutput subtitleOutput;
43+
private Canvas canvas;
44+
private Bitmap bitmap;
45+
46+
@Before
47+
public void setUp() {
48+
context = ApplicationProvider.getApplicationContext();
49+
subtitleOutput = new CanvasSubtitleOutput(context);
50+
// Set up the view with a fixed size for consistent testing
51+
subtitleOutput.layout(0, 0, VIEW_WIDTH, VIEW_HEIGHT);
52+
bitmap = Bitmap.createBitmap(VIEW_WIDTH, VIEW_HEIGHT, Bitmap.Config.ARGB_8888);
53+
canvas = new Canvas(bitmap);
54+
}
55+
56+
@Test
57+
public void dispatchDraw_emptyCues_doesNotCrash() {
58+
subtitleOutput.update(
59+
Collections.emptyList(),
60+
CaptionStyleCompat.DEFAULT,
61+
/* textSize= */ 0.05f,
62+
Cue.TEXT_SIZE_TYPE_FRACTIONAL,
63+
/* bottomPaddingFraction= */ 0.08f);
64+
65+
// Should not throw
66+
subtitleOutput.dispatchDraw(canvas);
67+
}
68+
69+
@Test
70+
public void dispatchDraw_singleCue_renders() {
71+
Cue cue = new Cue.Builder().setText("Single subtitle").build();
72+
73+
subtitleOutput.update(
74+
Collections.singletonList(cue),
75+
CaptionStyleCompat.DEFAULT,
76+
/* textSize= */ 0.05f,
77+
Cue.TEXT_SIZE_TYPE_FRACTIONAL,
78+
/* bottomPaddingFraction= */ 0.08f);
79+
80+
// Should not throw
81+
subtitleOutput.dispatchDraw(canvas);
82+
}
83+
84+
@Test
85+
public void dispatchDraw_overlappingCuesWithNegativeLineNumbers_rendersWithoutOverlap() {
86+
// Simulate WebVTT overlapping cues that get assigned negative line numbers
87+
// by WebvttSubtitle.getCues()
88+
Cue cue1 =
89+
new Cue.Builder()
90+
.setText("First overlapping subtitle")
91+
.setLine(-1f, Cue.LINE_TYPE_NUMBER)
92+
.setLineAnchor(Cue.ANCHOR_TYPE_START)
93+
.build();
94+
Cue cue2 =
95+
new Cue.Builder()
96+
.setText("Second overlapping subtitle")
97+
.setLine(-2f, Cue.LINE_TYPE_NUMBER)
98+
.setLineAnchor(Cue.ANCHOR_TYPE_START)
99+
.build();
100+
101+
subtitleOutput.update(
102+
Arrays.asList(cue1, cue2),
103+
CaptionStyleCompat.DEFAULT,
104+
/* textSize= */ 0.05f,
105+
Cue.TEXT_SIZE_TYPE_FRACTIONAL,
106+
/* bottomPaddingFraction= */ 0.08f);
107+
108+
// Should not throw and should render both cues
109+
subtitleOutput.dispatchDraw(canvas);
110+
}
111+
112+
@Test
113+
public void dispatchDraw_multipleOverlappingCues_rendersAll() {
114+
// Test with 3+ overlapping cues (common in anime subtitles)
115+
Cue cue1 =
116+
new Cue.Builder()
117+
.setText("Line 1")
118+
.setLine(-1f, Cue.LINE_TYPE_NUMBER)
119+
.setLineAnchor(Cue.ANCHOR_TYPE_START)
120+
.build();
121+
Cue cue2 =
122+
new Cue.Builder()
123+
.setText("Line 2")
124+
.setLine(-2f, Cue.LINE_TYPE_NUMBER)
125+
.setLineAnchor(Cue.ANCHOR_TYPE_START)
126+
.build();
127+
Cue cue3 =
128+
new Cue.Builder()
129+
.setText("Line 3")
130+
.setLine(-3f, Cue.LINE_TYPE_NUMBER)
131+
.setLineAnchor(Cue.ANCHOR_TYPE_START)
132+
.build();
133+
134+
subtitleOutput.update(
135+
Arrays.asList(cue1, cue2, cue3),
136+
CaptionStyleCompat.DEFAULT,
137+
/* textSize= */ 0.05f,
138+
Cue.TEXT_SIZE_TYPE_FRACTIONAL,
139+
/* bottomPaddingFraction= */ 0.08f);
140+
141+
// Should not throw
142+
subtitleOutput.dispatchDraw(canvas);
143+
}
144+
145+
@Test
146+
public void dispatchDraw_mixedCueTypes_rendersCorrectly() {
147+
// Mix of cues with and without explicit line numbers
148+
Cue cueWithLine =
149+
new Cue.Builder()
150+
.setText("Cue with line")
151+
.setLine(-1f, Cue.LINE_TYPE_NUMBER)
152+
.setLineAnchor(Cue.ANCHOR_TYPE_START)
153+
.build();
154+
Cue cueWithoutLine = new Cue.Builder().setText("Cue without line").build();
155+
156+
subtitleOutput.update(
157+
Arrays.asList(cueWithLine, cueWithoutLine),
158+
CaptionStyleCompat.DEFAULT,
159+
/* textSize= */ 0.05f,
160+
Cue.TEXT_SIZE_TYPE_FRACTIONAL,
161+
/* bottomPaddingFraction= */ 0.08f);
162+
163+
// Should not throw
164+
subtitleOutput.dispatchDraw(canvas);
165+
}
166+
167+
@Test
168+
public void dispatchDraw_cuesWithFractionalLine_notAffectedByStacking() {
169+
// Cues with LINE_TYPE_FRACTION should not be affected by the stacking logic
170+
Cue cue1 =
171+
new Cue.Builder()
172+
.setText("Fractional line cue 1")
173+
.setLine(0.9f, Cue.LINE_TYPE_FRACTION)
174+
.build();
175+
Cue cue2 =
176+
new Cue.Builder()
177+
.setText("Fractional line cue 2")
178+
.setLine(0.8f, Cue.LINE_TYPE_FRACTION)
179+
.build();
180+
181+
subtitleOutput.update(
182+
Arrays.asList(cue1, cue2),
183+
CaptionStyleCompat.DEFAULT,
184+
/* textSize= */ 0.05f,
185+
Cue.TEXT_SIZE_TYPE_FRACTIONAL,
186+
/* bottomPaddingFraction= */ 0.08f);
187+
188+
// Should not throw
189+
subtitleOutput.dispatchDraw(canvas);
190+
}
191+
192+
@Test
193+
public void dispatchDraw_cuesWithPositiveLineNumbers_stacksCorrectly() {
194+
// Cues with positive line numbers (top-anchored) should stack downward from the top
195+
Cue cue1 =
196+
new Cue.Builder()
197+
.setText("Top line cue 1")
198+
.setLine(0f, Cue.LINE_TYPE_NUMBER)
199+
.setLineAnchor(Cue.ANCHOR_TYPE_START)
200+
.build();
201+
Cue cue2 =
202+
new Cue.Builder()
203+
.setText("Top line cue 2")
204+
.setLine(1f, Cue.LINE_TYPE_NUMBER)
205+
.setLineAnchor(Cue.ANCHOR_TYPE_START)
206+
.build();
207+
208+
subtitleOutput.update(
209+
Arrays.asList(cue1, cue2),
210+
CaptionStyleCompat.DEFAULT,
211+
/* textSize= */ 0.05f,
212+
Cue.TEXT_SIZE_TYPE_FRACTIONAL,
213+
/* bottomPaddingFraction= */ 0.08f);
214+
215+
// Should not throw
216+
subtitleOutput.dispatchDraw(canvas);
217+
}
218+
219+
@Test
220+
public void dispatchDraw_mixedPositiveAndNegativeLineNumbers_stacksCorrectly() {
221+
// Test with cues at both top and bottom of screen
222+
Cue topCue =
223+
new Cue.Builder()
224+
.setText("Top subtitle")
225+
.setLine(0f, Cue.LINE_TYPE_NUMBER)
226+
.setLineAnchor(Cue.ANCHOR_TYPE_START)
227+
.build();
228+
Cue bottomCue =
229+
new Cue.Builder()
230+
.setText("Bottom subtitle")
231+
.setLine(-1f, Cue.LINE_TYPE_NUMBER)
232+
.setLineAnchor(Cue.ANCHOR_TYPE_START)
233+
.build();
234+
235+
subtitleOutput.update(
236+
Arrays.asList(topCue, bottomCue),
237+
CaptionStyleCompat.DEFAULT,
238+
/* textSize= */ 0.05f,
239+
Cue.TEXT_SIZE_TYPE_FRACTIONAL,
240+
/* bottomPaddingFraction= */ 0.08f);
241+
242+
// Should not throw
243+
subtitleOutput.dispatchDraw(canvas);
244+
}
245+
246+
@Test
247+
public void dispatchDraw_multiLineCueWithOverlap_stacksCorrectly() {
248+
// Test with a multi-line cue followed by another cue
249+
// This is the core issue from the bug report
250+
Cue multiLineCue =
251+
new Cue.Builder()
252+
.setText("This is a very long subtitle that will wrap to multiple lines")
253+
.setLine(-1f, Cue.LINE_TYPE_NUMBER)
254+
.setLineAnchor(Cue.ANCHOR_TYPE_START)
255+
.build();
256+
Cue secondCue =
257+
new Cue.Builder()
258+
.setText("Second cue")
259+
.setLine(-2f, Cue.LINE_TYPE_NUMBER)
260+
.setLineAnchor(Cue.ANCHOR_TYPE_START)
261+
.build();
262+
263+
subtitleOutput.update(
264+
Arrays.asList(multiLineCue, secondCue),
265+
CaptionStyleCompat.DEFAULT,
266+
/* textSize= */ 0.05f,
267+
Cue.TEXT_SIZE_TYPE_FRACTIONAL,
268+
/* bottomPaddingFraction= */ 0.08f);
269+
270+
// Should not throw
271+
subtitleOutput.dispatchDraw(canvas);
272+
}
273+
}

0 commit comments

Comments
 (0)