From 38536d3dabe883fe4b2d63c6e863e1cab64eb634 Mon Sep 17 00:00:00 2001 From: John Corser Date: Fri, 3 Apr 2026 08:53:35 -0400 Subject: [PATCH 1/4] Fix overlapping subtitles with negative and positive line numbers Fixes overlapping subtitle rendering when multiple WebVTT cues have overlapping timestamps and are assigned line numbers. Problem: When multiple WebVTT cues overlap in time, they get assigned line numbers (-1, -2, -3 for bottom-stacked or 0, 1, 2 for top-stacked). However, SubtitlePainter calculates each cue's vertical position using only firstLineHeight, not accounting for the actual multi-line height of preceding cues. This causes cues to visually overlap. Solution: 1. Added getLastDrawnTextBottom() to SubtitlePainter to expose the actual rendered height of a text cue 2. Modified CanvasSubtitleOutput.dispatchDraw() to track cumulative height of stacked cues and adjust the drawing boundary for subsequent cues 3. Handles both negative line numbers (bottom-stacked) and positive line numbers (top-stacked) Fixes #2237 --- .../media3/ui/CanvasSubtitleOutput.java | 33 ++- .../androidx/media3/ui/SubtitlePainter.java | 18 ++ .../media3/ui/CanvasSubtitleOutputTest.java | 273 ++++++++++++++++++ .../media3/ui/SubtitlePainterTest.java | 201 +++++++++++++ 4 files changed, 523 insertions(+), 2 deletions(-) create mode 100644 libraries/ui/src/test/java/androidx/media3/ui/CanvasSubtitleOutputTest.java create mode 100644 libraries/ui/src/test/java/androidx/media3/ui/SubtitlePainterTest.java diff --git a/libraries/ui/src/main/java/androidx/media3/ui/CanvasSubtitleOutput.java b/libraries/ui/src/main/java/androidx/media3/ui/CanvasSubtitleOutput.java index e01e803e0ad..b37ab6ae99f 100644 --- a/libraries/ui/src/main/java/androidx/media3/ui/CanvasSubtitleOutput.java +++ b/libraries/ui/src/main/java/androidx/media3/ui/CanvasSubtitleOutput.java @@ -104,12 +104,34 @@ public void dispatchDraw(Canvas canvas) { return; } + // Track cumulative offsets for stacked cues to prevent overlapping. + // Bottom-stacked cues (negative line numbers) stack upward from the bottom. + // Top-stacked cues (positive line numbers) stack downward from the top. + int cumulativeBottomOffset = 0; + int cumulativeTopOffset = 0; + int cueCount = cues.size(); for (int i = 0; i < cueCount; i++) { Cue cue = cues.get(i); if (cue.verticalType != Cue.TYPE_UNSET) { cue = repositionVerticalCue(cue); } + + // Determine if this cue is stacked from bottom or top based on line number. + boolean isBottomStackedCue = + cue.line != Cue.DIMEN_UNSET && cue.lineType == Cue.LINE_TYPE_NUMBER && cue.line < 0; + boolean isTopStackedCue = + cue.line != Cue.DIMEN_UNSET && cue.lineType == Cue.LINE_TYPE_NUMBER && cue.line >= 0; + + // Adjust boundaries to account for previously drawn cues. + int adjustedTop = top; + int adjustedBottom = bottom; + if (isBottomStackedCue && cumulativeBottomOffset > 0) { + adjustedBottom = bottom - cumulativeBottomOffset; + } else if (isTopStackedCue && cumulativeTopOffset > 0) { + adjustedTop = top + cumulativeTopOffset; + } + float cueTextSizePx = SubtitleViewUtils.resolveTextSize( cue.textSizeType, cue.textSize, rawViewHeight, viewHeightMinusPadding); @@ -122,9 +144,16 @@ public void dispatchDraw(Canvas canvas) { bottomPaddingFraction, canvas, left, - top, + adjustedTop, right, - bottom); + adjustedBottom); + + // Accumulate offset so subsequent cues don't overlap. + if (isBottomStackedCue) { + cumulativeBottomOffset += painter.getLastDrawnTextBottom(); + } else if (isTopStackedCue) { + cumulativeTopOffset += painter.getLastDrawnTextBottom(); + } } } diff --git a/libraries/ui/src/main/java/androidx/media3/ui/SubtitlePainter.java b/libraries/ui/src/main/java/androidx/media3/ui/SubtitlePainter.java index 24b76027d04..cff12ad4825 100644 --- a/libraries/ui/src/main/java/androidx/media3/ui/SubtitlePainter.java +++ b/libraries/ui/src/main/java/androidx/media3/ui/SubtitlePainter.java @@ -52,6 +52,9 @@ /** Ratio of inner padding to font size. */ private static final float INNER_PADDING_RATIO = 0.125f; + /** The bottom Y position of the last drawn text cue, or 0 if no text cue was drawn. */ + private int lastDrawnTextBottom; + // Styled dimensions. private final float outlineWidth; private final float shadowRadius; @@ -218,13 +221,28 @@ public void draw( if (isTextCue) { checkNotNull(cueText); setupTextLayout(); + lastDrawnTextBottom = textLayout != null ? textLayout.getHeight() : 0; } else { checkNotNull(cueBitmap); setupBitmapLayout(); + lastDrawnTextBottom = 0; } drawLayout(canvas, isTextCue); } + /** + * Returns the bottom Y position of the last drawn text cue relative to the parent bounds. + * + *

This can be used to stack multiple cues without overlap. For bottom-stacked cues (negative + * line numbers), this represents how far up from the bottom the cue extends. For top-stacked cues + * (positive line numbers), this represents how far down from the top the cue extends. + * + * @return The bottom Y position of the last drawn text cue, or 0 if no text cue was drawn. + */ + public int getLastDrawnTextBottom() { + return lastDrawnTextBottom; + } + @RequiresNonNull("cueText") private void setupTextLayout() { SpannableStringBuilder cueText = diff --git a/libraries/ui/src/test/java/androidx/media3/ui/CanvasSubtitleOutputTest.java b/libraries/ui/src/test/java/androidx/media3/ui/CanvasSubtitleOutputTest.java new file mode 100644 index 00000000000..97c64ff5a29 --- /dev/null +++ b/libraries/ui/src/test/java/androidx/media3/ui/CanvasSubtitleOutputTest.java @@ -0,0 +1,273 @@ +/* + * Copyright 2026 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package androidx.media3.ui; + +import static com.google.common.truth.Truth.assertThat; + +import android.content.Context; +import android.graphics.Bitmap; +import android.graphics.Canvas; +import android.graphics.Color; +import androidx.media3.common.text.Cue; +import androidx.test.core.app.ApplicationProvider; +import androidx.test.ext.junit.runners.AndroidJUnit4; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; + +/** Tests for {@link CanvasSubtitleOutput}. */ +@RunWith(AndroidJUnit4.class) +public class CanvasSubtitleOutputTest { + + private static final int VIEW_WIDTH = 1920; + private static final int VIEW_HEIGHT = 1080; + + private Context context; + private CanvasSubtitleOutput subtitleOutput; + private Canvas canvas; + private Bitmap bitmap; + + @Before + public void setUp() { + context = ApplicationProvider.getApplicationContext(); + subtitleOutput = new CanvasSubtitleOutput(context); + // Set up the view with a fixed size for consistent testing + subtitleOutput.layout(0, 0, VIEW_WIDTH, VIEW_HEIGHT); + bitmap = Bitmap.createBitmap(VIEW_WIDTH, VIEW_HEIGHT, Bitmap.Config.ARGB_8888); + canvas = new Canvas(bitmap); + } + + @Test + public void dispatchDraw_emptyCues_doesNotCrash() { + subtitleOutput.update( + Collections.emptyList(), + CaptionStyleCompat.DEFAULT, + /* textSize= */ 0.05f, + Cue.TEXT_SIZE_TYPE_FRACTIONAL, + /* bottomPaddingFraction= */ 0.08f); + + // Should not throw + subtitleOutput.dispatchDraw(canvas); + } + + @Test + public void dispatchDraw_singleCue_renders() { + Cue cue = new Cue.Builder().setText("Single subtitle").build(); + + subtitleOutput.update( + Collections.singletonList(cue), + CaptionStyleCompat.DEFAULT, + /* textSize= */ 0.05f, + Cue.TEXT_SIZE_TYPE_FRACTIONAL, + /* bottomPaddingFraction= */ 0.08f); + + // Should not throw + subtitleOutput.dispatchDraw(canvas); + } + + @Test + public void dispatchDraw_overlappingCuesWithNegativeLineNumbers_rendersWithoutOverlap() { + // Simulate WebVTT overlapping cues that get assigned negative line numbers + // by WebvttSubtitle.getCues() + Cue cue1 = + new Cue.Builder() + .setText("First overlapping subtitle") + .setLine(-1f, Cue.LINE_TYPE_NUMBER) + .setLineAnchor(Cue.ANCHOR_TYPE_START) + .build(); + Cue cue2 = + new Cue.Builder() + .setText("Second overlapping subtitle") + .setLine(-2f, Cue.LINE_TYPE_NUMBER) + .setLineAnchor(Cue.ANCHOR_TYPE_START) + .build(); + + subtitleOutput.update( + Arrays.asList(cue1, cue2), + CaptionStyleCompat.DEFAULT, + /* textSize= */ 0.05f, + Cue.TEXT_SIZE_TYPE_FRACTIONAL, + /* bottomPaddingFraction= */ 0.08f); + + // Should not throw and should render both cues + subtitleOutput.dispatchDraw(canvas); + } + + @Test + public void dispatchDraw_multipleOverlappingCues_rendersAll() { + // Test with 3+ overlapping cues (common in anime subtitles) + Cue cue1 = + new Cue.Builder() + .setText("Line 1") + .setLine(-1f, Cue.LINE_TYPE_NUMBER) + .setLineAnchor(Cue.ANCHOR_TYPE_START) + .build(); + Cue cue2 = + new Cue.Builder() + .setText("Line 2") + .setLine(-2f, Cue.LINE_TYPE_NUMBER) + .setLineAnchor(Cue.ANCHOR_TYPE_START) + .build(); + Cue cue3 = + new Cue.Builder() + .setText("Line 3") + .setLine(-3f, Cue.LINE_TYPE_NUMBER) + .setLineAnchor(Cue.ANCHOR_TYPE_START) + .build(); + + subtitleOutput.update( + Arrays.asList(cue1, cue2, cue3), + CaptionStyleCompat.DEFAULT, + /* textSize= */ 0.05f, + Cue.TEXT_SIZE_TYPE_FRACTIONAL, + /* bottomPaddingFraction= */ 0.08f); + + // Should not throw + subtitleOutput.dispatchDraw(canvas); + } + + @Test + public void dispatchDraw_mixedCueTypes_rendersCorrectly() { + // Mix of cues with and without explicit line numbers + Cue cueWithLine = + new Cue.Builder() + .setText("Cue with line") + .setLine(-1f, Cue.LINE_TYPE_NUMBER) + .setLineAnchor(Cue.ANCHOR_TYPE_START) + .build(); + Cue cueWithoutLine = new Cue.Builder().setText("Cue without line").build(); + + subtitleOutput.update( + Arrays.asList(cueWithLine, cueWithoutLine), + CaptionStyleCompat.DEFAULT, + /* textSize= */ 0.05f, + Cue.TEXT_SIZE_TYPE_FRACTIONAL, + /* bottomPaddingFraction= */ 0.08f); + + // Should not throw + subtitleOutput.dispatchDraw(canvas); + } + + @Test + public void dispatchDraw_cuesWithFractionalLine_notAffectedByStacking() { + // Cues with LINE_TYPE_FRACTION should not be affected by the stacking logic + Cue cue1 = + new Cue.Builder() + .setText("Fractional line cue 1") + .setLine(0.9f, Cue.LINE_TYPE_FRACTION) + .build(); + Cue cue2 = + new Cue.Builder() + .setText("Fractional line cue 2") + .setLine(0.8f, Cue.LINE_TYPE_FRACTION) + .build(); + + subtitleOutput.update( + Arrays.asList(cue1, cue2), + CaptionStyleCompat.DEFAULT, + /* textSize= */ 0.05f, + Cue.TEXT_SIZE_TYPE_FRACTIONAL, + /* bottomPaddingFraction= */ 0.08f); + + // Should not throw + subtitleOutput.dispatchDraw(canvas); + } + + @Test + public void dispatchDraw_cuesWithPositiveLineNumbers_stacksCorrectly() { + // Cues with positive line numbers (top-anchored) should stack downward from the top + Cue cue1 = + new Cue.Builder() + .setText("Top line cue 1") + .setLine(0f, Cue.LINE_TYPE_NUMBER) + .setLineAnchor(Cue.ANCHOR_TYPE_START) + .build(); + Cue cue2 = + new Cue.Builder() + .setText("Top line cue 2") + .setLine(1f, Cue.LINE_TYPE_NUMBER) + .setLineAnchor(Cue.ANCHOR_TYPE_START) + .build(); + + subtitleOutput.update( + Arrays.asList(cue1, cue2), + CaptionStyleCompat.DEFAULT, + /* textSize= */ 0.05f, + Cue.TEXT_SIZE_TYPE_FRACTIONAL, + /* bottomPaddingFraction= */ 0.08f); + + // Should not throw + subtitleOutput.dispatchDraw(canvas); + } + + @Test + public void dispatchDraw_mixedPositiveAndNegativeLineNumbers_stacksCorrectly() { + // Test with cues at both top and bottom of screen + Cue topCue = + new Cue.Builder() + .setText("Top subtitle") + .setLine(0f, Cue.LINE_TYPE_NUMBER) + .setLineAnchor(Cue.ANCHOR_TYPE_START) + .build(); + Cue bottomCue = + new Cue.Builder() + .setText("Bottom subtitle") + .setLine(-1f, Cue.LINE_TYPE_NUMBER) + .setLineAnchor(Cue.ANCHOR_TYPE_START) + .build(); + + subtitleOutput.update( + Arrays.asList(topCue, bottomCue), + CaptionStyleCompat.DEFAULT, + /* textSize= */ 0.05f, + Cue.TEXT_SIZE_TYPE_FRACTIONAL, + /* bottomPaddingFraction= */ 0.08f); + + // Should not throw + subtitleOutput.dispatchDraw(canvas); + } + + @Test + public void dispatchDraw_multiLineCueWithOverlap_stacksCorrectly() { + // Test with a multi-line cue followed by another cue + // This is the core issue from the bug report + Cue multiLineCue = + new Cue.Builder() + .setText("This is a very long subtitle that will wrap to multiple lines") + .setLine(-1f, Cue.LINE_TYPE_NUMBER) + .setLineAnchor(Cue.ANCHOR_TYPE_START) + .build(); + Cue secondCue = + new Cue.Builder() + .setText("Second cue") + .setLine(-2f, Cue.LINE_TYPE_NUMBER) + .setLineAnchor(Cue.ANCHOR_TYPE_START) + .build(); + + subtitleOutput.update( + Arrays.asList(multiLineCue, secondCue), + CaptionStyleCompat.DEFAULT, + /* textSize= */ 0.05f, + Cue.TEXT_SIZE_TYPE_FRACTIONAL, + /* bottomPaddingFraction= */ 0.08f); + + // Should not throw + subtitleOutput.dispatchDraw(canvas); + } +} diff --git a/libraries/ui/src/test/java/androidx/media3/ui/SubtitlePainterTest.java b/libraries/ui/src/test/java/androidx/media3/ui/SubtitlePainterTest.java new file mode 100644 index 00000000000..6296d184901 --- /dev/null +++ b/libraries/ui/src/test/java/androidx/media3/ui/SubtitlePainterTest.java @@ -0,0 +1,201 @@ +/* + * Copyright 2026 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package androidx.media3.ui; + +import static com.google.common.truth.Truth.assertThat; + +import android.content.Context; +import android.graphics.Bitmap; +import android.graphics.Canvas; +import androidx.media3.common.text.Cue; +import androidx.test.core.app.ApplicationProvider; +import androidx.test.ext.junit.runners.AndroidJUnit4; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; + +/** Tests for {@link SubtitlePainter}. */ +@RunWith(AndroidJUnit4.class) +public class SubtitlePainterTest { + + private static final int CANVAS_WIDTH = 1920; + private static final int CANVAS_HEIGHT = 1080; + private static final float DEFAULT_TEXT_SIZE_PX = 50f; + + private Context context; + private SubtitlePainter painter; + private Canvas canvas; + + @Before + public void setUp() { + context = ApplicationProvider.getApplicationContext(); + painter = new SubtitlePainter(context); + Bitmap bitmap = Bitmap.createBitmap(CANVAS_WIDTH, CANVAS_HEIGHT, Bitmap.Config.ARGB_8888); + canvas = new Canvas(bitmap); + } + + @Test + public void getLastDrawnTextBottom_beforeDraw_returnsZero() { + assertThat(painter.getLastDrawnTextBottom()).isEqualTo(0); + } + + @Test + public void getLastDrawnTextBottom_afterDrawingTextCue_returnsPositiveValue() { + Cue cue = new Cue.Builder().setText("Test subtitle").build(); + + painter.draw( + cue, + CaptionStyleCompat.DEFAULT, + DEFAULT_TEXT_SIZE_PX, + /* cueTextSizePx= */ 0, + /* bottomPaddingFraction= */ 0.08f, + canvas, + /* cueBoxLeft= */ 0, + /* cueBoxTop= */ 0, + /* cueBoxRight= */ CANVAS_WIDTH, + /* cueBoxBottom= */ CANVAS_HEIGHT); + + assertThat(painter.getLastDrawnTextBottom()).isGreaterThan(0); + } + + @Test + public void getLastDrawnTextBottom_multiLineText_returnsLargerValue() { + Cue singleLineCue = new Cue.Builder().setText("Single line").build(); + painter.draw( + singleLineCue, + CaptionStyleCompat.DEFAULT, + DEFAULT_TEXT_SIZE_PX, + /* cueTextSizePx= */ 0, + /* bottomPaddingFraction= */ 0.08f, + canvas, + /* cueBoxLeft= */ 0, + /* cueBoxTop= */ 0, + /* cueBoxRight= */ CANVAS_WIDTH, + /* cueBoxBottom= */ CANVAS_HEIGHT); + int singleLineBottom = painter.getLastDrawnTextBottom(); + + Cue multiLineCue = new Cue.Builder().setText("Line 1\nLine 2\nLine 3").build(); + painter.draw( + multiLineCue, + CaptionStyleCompat.DEFAULT, + DEFAULT_TEXT_SIZE_PX, + /* cueTextSizePx= */ 0, + /* bottomPaddingFraction= */ 0.08f, + canvas, + /* cueBoxLeft= */ 0, + /* cueBoxTop= */ 0, + /* cueBoxRight= */ CANVAS_WIDTH, + /* cueBoxBottom= */ CANVAS_HEIGHT); + int multiLineBottom = painter.getLastDrawnTextBottom(); + + assertThat(multiLineBottom).isGreaterThan(singleLineBottom); + } + + @Test + public void getLastDrawnTextBottom_emptyText_returnsZero() { + Cue cue = new Cue.Builder().setText("").build(); + + painter.draw( + cue, + CaptionStyleCompat.DEFAULT, + DEFAULT_TEXT_SIZE_PX, + /* cueTextSizePx= */ 0, + /* bottomPaddingFraction= */ 0.08f, + canvas, + /* cueBoxLeft= */ 0, + /* cueBoxTop= */ 0, + /* cueBoxRight= */ CANVAS_WIDTH, + /* cueBoxBottom= */ CANVAS_HEIGHT); + + assertThat(painter.getLastDrawnTextBottom()).isEqualTo(0); + } + + @Test + public void getLastDrawnTextBottom_bitmapCue_returnsZero() { + Bitmap cueBitmap = Bitmap.createBitmap(100, 50, Bitmap.Config.ARGB_8888); + Cue cue = + new Cue.Builder() + .setBitmap(cueBitmap) + .setPosition(0.5f) + .setPositionAnchor(Cue.ANCHOR_TYPE_MIDDLE) + .setLine(0.9f, Cue.LINE_TYPE_FRACTION) + .setLineAnchor(Cue.ANCHOR_TYPE_END) + .setSize(0.5f) + .build(); + + painter.draw( + cue, + CaptionStyleCompat.DEFAULT, + DEFAULT_TEXT_SIZE_PX, + /* cueTextSizePx= */ 0, + /* bottomPaddingFraction= */ 0.08f, + canvas, + /* cueBoxLeft= */ 0, + /* cueBoxTop= */ 0, + /* cueBoxRight= */ CANVAS_WIDTH, + /* cueBoxBottom= */ CANVAS_HEIGHT); + + assertThat(painter.getLastDrawnTextBottom()).isEqualTo(0); + } + + @Test + public void getLastDrawnTextBottom_cueWithNegativeLine_returnsPositiveValue() { + Cue cue = + new Cue.Builder() + .setText("Bottom-stacked subtitle") + .setLine(-1f, Cue.LINE_TYPE_NUMBER) + .setLineAnchor(Cue.ANCHOR_TYPE_START) + .build(); + + painter.draw( + cue, + CaptionStyleCompat.DEFAULT, + DEFAULT_TEXT_SIZE_PX, + /* cueTextSizePx= */ 0, + /* bottomPaddingFraction= */ 0.08f, + canvas, + /* cueBoxLeft= */ 0, + /* cueBoxTop= */ 0, + /* cueBoxRight= */ CANVAS_WIDTH, + /* cueBoxBottom= */ CANVAS_HEIGHT); + + assertThat(painter.getLastDrawnTextBottom()).isGreaterThan(0); + } + + @Test + public void getLastDrawnTextBottom_cueWithPositiveLine_returnsPositiveValue() { + Cue cue = + new Cue.Builder() + .setText("Top-stacked subtitle") + .setLine(0f, Cue.LINE_TYPE_NUMBER) + .setLineAnchor(Cue.ANCHOR_TYPE_START) + .build(); + + painter.draw( + cue, + CaptionStyleCompat.DEFAULT, + DEFAULT_TEXT_SIZE_PX, + /* cueTextSizePx= */ 0, + /* bottomPaddingFraction= */ 0.08f, + canvas, + /* cueBoxLeft= */ 0, + /* cueBoxTop= */ 0, + /* cueBoxRight= */ CANVAS_WIDTH, + /* cueBoxBottom= */ CANVAS_HEIGHT); + + assertThat(painter.getLastDrawnTextBottom()).isGreaterThan(0); + } +} From c19631c3eea64d64f095b4856c96d6722bc73882 Mon Sep 17 00:00:00 2001 From: John Corser Date: Fri, 17 Apr 2026 19:04:09 -0400 Subject: [PATCH 2/4] Address PR feedback: rename method, fix docs, add bitmap support MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Fix copyright year: 2026 → 2025 in test files - Rename getLastDrawnTextBottom() to getLastDrawnCueHeight() to clarify it returns the height of the drawn cue, not a position - Update documentation to accurately describe the return value - Add bitmap cue support: now returns bitmapRect.height() for bitmap cues instead of 0, since bitmap cues can also have line-based position - Update tests to reflect the new method name and bitmap behavior --- .../media3/ui/CanvasSubtitleOutput.java | 4 +-- .../androidx/media3/ui/SubtitlePainter.java | 21 ++++++------ .../media3/ui/CanvasSubtitleOutputTest.java | 2 +- .../media3/ui/SubtitlePainterTest.java | 34 +++++++++---------- 4 files changed, 30 insertions(+), 31 deletions(-) diff --git a/libraries/ui/src/main/java/androidx/media3/ui/CanvasSubtitleOutput.java b/libraries/ui/src/main/java/androidx/media3/ui/CanvasSubtitleOutput.java index b37ab6ae99f..b68a2c5e584 100644 --- a/libraries/ui/src/main/java/androidx/media3/ui/CanvasSubtitleOutput.java +++ b/libraries/ui/src/main/java/androidx/media3/ui/CanvasSubtitleOutput.java @@ -150,9 +150,9 @@ public void dispatchDraw(Canvas canvas) { // Accumulate offset so subsequent cues don't overlap. if (isBottomStackedCue) { - cumulativeBottomOffset += painter.getLastDrawnTextBottom(); + cumulativeBottomOffset += painter.getLastDrawnCueHeight(); } else if (isTopStackedCue) { - cumulativeTopOffset += painter.getLastDrawnTextBottom(); + cumulativeTopOffset += painter.getLastDrawnCueHeight(); } } } diff --git a/libraries/ui/src/main/java/androidx/media3/ui/SubtitlePainter.java b/libraries/ui/src/main/java/androidx/media3/ui/SubtitlePainter.java index cff12ad4825..334dcbfe6d9 100644 --- a/libraries/ui/src/main/java/androidx/media3/ui/SubtitlePainter.java +++ b/libraries/ui/src/main/java/androidx/media3/ui/SubtitlePainter.java @@ -52,8 +52,8 @@ /** Ratio of inner padding to font size. */ private static final float INNER_PADDING_RATIO = 0.125f; - /** The bottom Y position of the last drawn text cue, or 0 if no text cue was drawn. */ - private int lastDrawnTextBottom; + /** The height of the last drawn cue, or 0 if no cue was drawn. */ + private int lastDrawnCueHeight; // Styled dimensions. private final float outlineWidth; @@ -221,26 +221,25 @@ public void draw( if (isTextCue) { checkNotNull(cueText); setupTextLayout(); - lastDrawnTextBottom = textLayout != null ? textLayout.getHeight() : 0; + lastDrawnCueHeight = textLayout != null ? textLayout.getHeight() : 0; } else { checkNotNull(cueBitmap); setupBitmapLayout(); - lastDrawnTextBottom = 0; + lastDrawnCueHeight = bitmapRect != null ? bitmapRect.height() : 0; } drawLayout(canvas, isTextCue); } /** - * Returns the bottom Y position of the last drawn text cue relative to the parent bounds. + * Returns the height of the last drawn cue. * - *

This can be used to stack multiple cues without overlap. For bottom-stacked cues (negative - * line numbers), this represents how far up from the bottom the cue extends. For top-stacked cues - * (positive line numbers), this represents how far down from the top the cue extends. + *

This can be used to stack multiple cues without overlap by adjusting the drawing bounds for + * subsequent cues by this amount. * - * @return The bottom Y position of the last drawn text cue, or 0 if no text cue was drawn. + * @return The height of the last drawn cue in pixels, or 0 if no cue was drawn. */ - public int getLastDrawnTextBottom() { - return lastDrawnTextBottom; + public int getLastDrawnCueHeight() { + return lastDrawnCueHeight; } @RequiresNonNull("cueText") diff --git a/libraries/ui/src/test/java/androidx/media3/ui/CanvasSubtitleOutputTest.java b/libraries/ui/src/test/java/androidx/media3/ui/CanvasSubtitleOutputTest.java index 97c64ff5a29..222b7c47c86 100644 --- a/libraries/ui/src/test/java/androidx/media3/ui/CanvasSubtitleOutputTest.java +++ b/libraries/ui/src/test/java/androidx/media3/ui/CanvasSubtitleOutputTest.java @@ -1,5 +1,5 @@ /* - * Copyright 2026 The Android Open Source Project + * Copyright 2025 The Android Open Source Project * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. diff --git a/libraries/ui/src/test/java/androidx/media3/ui/SubtitlePainterTest.java b/libraries/ui/src/test/java/androidx/media3/ui/SubtitlePainterTest.java index 6296d184901..bb7420134a1 100644 --- a/libraries/ui/src/test/java/androidx/media3/ui/SubtitlePainterTest.java +++ b/libraries/ui/src/test/java/androidx/media3/ui/SubtitlePainterTest.java @@ -1,5 +1,5 @@ /* - * Copyright 2026 The Android Open Source Project + * Copyright 2025 The Android Open Source Project * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -48,12 +48,12 @@ public void setUp() { } @Test - public void getLastDrawnTextBottom_beforeDraw_returnsZero() { - assertThat(painter.getLastDrawnTextBottom()).isEqualTo(0); + public void getLastDrawnCueHeight_beforeDraw_returnsZero() { + assertThat(painter.getLastDrawnCueHeight()).isEqualTo(0); } @Test - public void getLastDrawnTextBottom_afterDrawingTextCue_returnsPositiveValue() { + public void getLastDrawnCueHeight_afterDrawingTextCue_returnsPositiveValue() { Cue cue = new Cue.Builder().setText("Test subtitle").build(); painter.draw( @@ -68,11 +68,11 @@ public void getLastDrawnTextBottom_afterDrawingTextCue_returnsPositiveValue() { /* cueBoxRight= */ CANVAS_WIDTH, /* cueBoxBottom= */ CANVAS_HEIGHT); - assertThat(painter.getLastDrawnTextBottom()).isGreaterThan(0); + assertThat(painter.getLastDrawnCueHeight()).isGreaterThan(0); } @Test - public void getLastDrawnTextBottom_multiLineText_returnsLargerValue() { + public void getLastDrawnCueHeight_multiLineText_returnsLargerValue() { Cue singleLineCue = new Cue.Builder().setText("Single line").build(); painter.draw( singleLineCue, @@ -85,7 +85,7 @@ public void getLastDrawnTextBottom_multiLineText_returnsLargerValue() { /* cueBoxTop= */ 0, /* cueBoxRight= */ CANVAS_WIDTH, /* cueBoxBottom= */ CANVAS_HEIGHT); - int singleLineBottom = painter.getLastDrawnTextBottom(); + int singleLineHeight = painter.getLastDrawnCueHeight(); Cue multiLineCue = new Cue.Builder().setText("Line 1\nLine 2\nLine 3").build(); painter.draw( @@ -99,13 +99,13 @@ public void getLastDrawnTextBottom_multiLineText_returnsLargerValue() { /* cueBoxTop= */ 0, /* cueBoxRight= */ CANVAS_WIDTH, /* cueBoxBottom= */ CANVAS_HEIGHT); - int multiLineBottom = painter.getLastDrawnTextBottom(); + int multiLineHeight = painter.getLastDrawnCueHeight(); - assertThat(multiLineBottom).isGreaterThan(singleLineBottom); + assertThat(multiLineHeight).isGreaterThan(singleLineHeight); } @Test - public void getLastDrawnTextBottom_emptyText_returnsZero() { + public void getLastDrawnCueHeight_emptyText_returnsZero() { Cue cue = new Cue.Builder().setText("").build(); painter.draw( @@ -120,11 +120,11 @@ public void getLastDrawnTextBottom_emptyText_returnsZero() { /* cueBoxRight= */ CANVAS_WIDTH, /* cueBoxBottom= */ CANVAS_HEIGHT); - assertThat(painter.getLastDrawnTextBottom()).isEqualTo(0); + assertThat(painter.getLastDrawnCueHeight()).isEqualTo(0); } @Test - public void getLastDrawnTextBottom_bitmapCue_returnsZero() { + public void getLastDrawnCueHeight_bitmapCue_returnsPositiveValue() { Bitmap cueBitmap = Bitmap.createBitmap(100, 50, Bitmap.Config.ARGB_8888); Cue cue = new Cue.Builder() @@ -148,11 +148,11 @@ public void getLastDrawnTextBottom_bitmapCue_returnsZero() { /* cueBoxRight= */ CANVAS_WIDTH, /* cueBoxBottom= */ CANVAS_HEIGHT); - assertThat(painter.getLastDrawnTextBottom()).isEqualTo(0); + assertThat(painter.getLastDrawnCueHeight()).isGreaterThan(0); } @Test - public void getLastDrawnTextBottom_cueWithNegativeLine_returnsPositiveValue() { + public void getLastDrawnCueHeight_cueWithNegativeLine_returnsPositiveValue() { Cue cue = new Cue.Builder() .setText("Bottom-stacked subtitle") @@ -172,11 +172,11 @@ public void getLastDrawnTextBottom_cueWithNegativeLine_returnsPositiveValue() { /* cueBoxRight= */ CANVAS_WIDTH, /* cueBoxBottom= */ CANVAS_HEIGHT); - assertThat(painter.getLastDrawnTextBottom()).isGreaterThan(0); + assertThat(painter.getLastDrawnCueHeight()).isGreaterThan(0); } @Test - public void getLastDrawnTextBottom_cueWithPositiveLine_returnsPositiveValue() { + public void getLastDrawnCueHeight_cueWithPositiveLine_returnsPositiveValue() { Cue cue = new Cue.Builder() .setText("Top-stacked subtitle") @@ -196,6 +196,6 @@ public void getLastDrawnTextBottom_cueWithPositiveLine_returnsPositiveValue() { /* cueBoxRight= */ CANVAS_WIDTH, /* cueBoxBottom= */ CANVAS_HEIGHT); - assertThat(painter.getLastDrawnTextBottom()).isGreaterThan(0); + assertThat(painter.getLastDrawnCueHeight()).isGreaterThan(0); } } From 9585a889de0f6a4263c03e828732d08d9db78f64 Mon Sep 17 00:00:00 2001 From: John Corser Date: Sat, 18 Apr 2026 12:44:14 -0400 Subject: [PATCH 3/4] Also handle cues with DIMEN_UNSET (default SRT positioning) Cues without explicit line positioning (line == DIMEN_UNSET) should also stack from the bottom to prevent overlapping. This is common with SRT subtitles that don't specify positioning info. --- .../main/java/androidx/media3/ui/CanvasSubtitleOutput.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/libraries/ui/src/main/java/androidx/media3/ui/CanvasSubtitleOutput.java b/libraries/ui/src/main/java/androidx/media3/ui/CanvasSubtitleOutput.java index b68a2c5e584..bf4ad87cd6e 100644 --- a/libraries/ui/src/main/java/androidx/media3/ui/CanvasSubtitleOutput.java +++ b/libraries/ui/src/main/java/androidx/media3/ui/CanvasSubtitleOutput.java @@ -118,8 +118,10 @@ public void dispatchDraw(Canvas canvas) { } // Determine if this cue is stacked from bottom or top based on line number. + // Cues with DIMEN_UNSET (no explicit position) default to bottom stacking. boolean isBottomStackedCue = - cue.line != Cue.DIMEN_UNSET && cue.lineType == Cue.LINE_TYPE_NUMBER && cue.line < 0; + cue.line == Cue.DIMEN_UNSET + || (cue.lineType == Cue.LINE_TYPE_NUMBER && cue.line < 0); boolean isTopStackedCue = cue.line != Cue.DIMEN_UNSET && cue.lineType == Cue.LINE_TYPE_NUMBER && cue.line >= 0; From f00f1883bf9329186d7f4df117e330136fc062b2 Mon Sep 17 00:00:00 2001 From: John Corser Date: Sat, 16 May 2026 20:01:46 -0400 Subject: [PATCH 4/4] Only apply stacking offset to cues sharing the same line value Address review feedback: the previous approach applied cumulative viewport shrinking to ALL subsequent cues regardless of their line number. This caused incorrect positioning when cues had different line numbers (e.g., -1 and -2), because the line-number math in SubtitlePainter already separates them. Now we track cumulative offsets per line value using a Map. Only cues that share the same line number (or DIMEN_UNSET) get stacked. Cues with different line numbers are positioned independently by SubtitlePainter's existing logic. This correctly handles: - Same line number: cues are stacked (the overlapping SRT/WebVTT case) - Different line numbers: no viewport adjustment (already separated) - DIMEN_UNSET: all unpositioned cues stack from bottom (SRT default) --- .../media3/ui/CanvasSubtitleOutput.java | 37 +++++----- .../media3/ui/CanvasSubtitleOutputTest.java | 74 +++++++++++++++++++ 2 files changed, 94 insertions(+), 17 deletions(-) diff --git a/libraries/ui/src/main/java/androidx/media3/ui/CanvasSubtitleOutput.java b/libraries/ui/src/main/java/androidx/media3/ui/CanvasSubtitleOutput.java index bf4ad87cd6e..9aba1729b37 100644 --- a/libraries/ui/src/main/java/androidx/media3/ui/CanvasSubtitleOutput.java +++ b/libraries/ui/src/main/java/androidx/media3/ui/CanvasSubtitleOutput.java @@ -26,7 +26,9 @@ import androidx.media3.common.text.Cue; import java.util.ArrayList; import java.util.Collections; +import java.util.HashMap; import java.util.List; +import java.util.Map; /** * A {@link SubtitleView.Output} that uses Android's native layout framework via {@link @@ -104,11 +106,11 @@ public void dispatchDraw(Canvas canvas) { return; } - // Track cumulative offsets for stacked cues to prevent overlapping. - // Bottom-stacked cues (negative line numbers) stack upward from the bottom. - // Top-stacked cues (positive line numbers) stack downward from the top. - int cumulativeBottomOffset = 0; - int cumulativeTopOffset = 0; + // Track cumulative offsets per line value to prevent overlapping only among cues that share + // the same line number. Cues with different line numbers are already separated by the + // line-number positioning math in SubtitlePainter, so they don't need viewport adjustment. + // We use Float as the key: DIMEN_UNSET for unpositioned cues, or the actual line value. + Map cumulativeOffsetByLine = new HashMap<>(); int cueCount = cues.size(); for (int i = 0; i < cueCount; i++) { @@ -117,21 +119,24 @@ public void dispatchDraw(Canvas canvas) { cue = repositionVerticalCue(cue); } - // Determine if this cue is stacked from bottom or top based on line number. - // Cues with DIMEN_UNSET (no explicit position) default to bottom stacking. + // Determine the effective line key for grouping. + float lineKey = cue.line; boolean isBottomStackedCue = cue.line == Cue.DIMEN_UNSET || (cue.lineType == Cue.LINE_TYPE_NUMBER && cue.line < 0); boolean isTopStackedCue = cue.line != Cue.DIMEN_UNSET && cue.lineType == Cue.LINE_TYPE_NUMBER && cue.line >= 0; - // Adjust boundaries to account for previously drawn cues. + // Get the cumulative offset for cues at this same line value. + int cumulativeOffset = cumulativeOffsetByLine.getOrDefault(lineKey, 0); + + // Adjust boundaries to account for previously drawn cues at the same line. int adjustedTop = top; int adjustedBottom = bottom; - if (isBottomStackedCue && cumulativeBottomOffset > 0) { - adjustedBottom = bottom - cumulativeBottomOffset; - } else if (isTopStackedCue && cumulativeTopOffset > 0) { - adjustedTop = top + cumulativeTopOffset; + if (isBottomStackedCue && cumulativeOffset > 0) { + adjustedBottom = bottom - cumulativeOffset; + } else if (isTopStackedCue && cumulativeOffset > 0) { + adjustedTop = top + cumulativeOffset; } float cueTextSizePx = @@ -150,11 +155,9 @@ public void dispatchDraw(Canvas canvas) { right, adjustedBottom); - // Accumulate offset so subsequent cues don't overlap. - if (isBottomStackedCue) { - cumulativeBottomOffset += painter.getLastDrawnCueHeight(); - } else if (isTopStackedCue) { - cumulativeTopOffset += painter.getLastDrawnCueHeight(); + // Accumulate offset so subsequent cues at the same line don't overlap. + if (isBottomStackedCue || isTopStackedCue) { + cumulativeOffsetByLine.put(lineKey, cumulativeOffset + painter.getLastDrawnCueHeight()); } } } diff --git a/libraries/ui/src/test/java/androidx/media3/ui/CanvasSubtitleOutputTest.java b/libraries/ui/src/test/java/androidx/media3/ui/CanvasSubtitleOutputTest.java index 222b7c47c86..dc279a2045b 100644 --- a/libraries/ui/src/test/java/androidx/media3/ui/CanvasSubtitleOutputTest.java +++ b/libraries/ui/src/test/java/androidx/media3/ui/CanvasSubtitleOutputTest.java @@ -270,4 +270,78 @@ public void dispatchDraw_multiLineCueWithOverlap_stacksCorrectly() { // Should not throw subtitleOutput.dispatchDraw(canvas); } + + @Test + public void dispatchDraw_sameLine_cuesGetStacked() { + // Two cues with the same line number should be stacked (viewport shrunk for second cue) + Cue cue1 = + new Cue.Builder() + .setText("First cue at line -1") + .setLine(-1f, Cue.LINE_TYPE_NUMBER) + .setLineAnchor(Cue.ANCHOR_TYPE_START) + .build(); + Cue cue2 = + new Cue.Builder() + .setText("Second cue at line -1") + .setLine(-1f, Cue.LINE_TYPE_NUMBER) + .setLineAnchor(Cue.ANCHOR_TYPE_START) + .build(); + + subtitleOutput.update( + Arrays.asList(cue1, cue2), + CaptionStyleCompat.DEFAULT, + /* textSize= */ 0.05f, + Cue.TEXT_SIZE_TYPE_FRACTIONAL, + /* bottomPaddingFraction= */ 0.08f); + + // Should not throw - both cues rendered without overlap + subtitleOutput.dispatchDraw(canvas); + } + + @Test + public void dispatchDraw_differentLines_noViewportShrinking() { + // Cues with different line numbers should NOT have their viewport shrunk, + // because the line-number positioning already separates them. + Cue cue1 = + new Cue.Builder() + .setText("Cue at line -1") + .setLine(-1f, Cue.LINE_TYPE_NUMBER) + .setLineAnchor(Cue.ANCHOR_TYPE_START) + .build(); + Cue cue2 = + new Cue.Builder() + .setText("Cue at line -2") + .setLine(-2f, Cue.LINE_TYPE_NUMBER) + .setLineAnchor(Cue.ANCHOR_TYPE_START) + .build(); + + subtitleOutput.update( + Arrays.asList(cue1, cue2), + CaptionStyleCompat.DEFAULT, + /* textSize= */ 0.05f, + Cue.TEXT_SIZE_TYPE_FRACTIONAL, + /* bottomPaddingFraction= */ 0.08f); + + // Should not throw - cues positioned independently by their line numbers + subtitleOutput.dispatchDraw(canvas); + } + + @Test + public void dispatchDraw_unsetLineCues_getStacked() { + // Multiple cues with DIMEN_UNSET (typical SRT) should be stacked since they all + // target the same default bottom position. + Cue cue1 = new Cue.Builder().setText("First SRT cue").build(); + Cue cue2 = new Cue.Builder().setText("Second SRT cue").build(); + Cue cue3 = new Cue.Builder().setText("Third SRT cue").build(); + + subtitleOutput.update( + Arrays.asList(cue1, cue2, cue3), + CaptionStyleCompat.DEFAULT, + /* textSize= */ 0.05f, + Cue.TEXT_SIZE_TYPE_FRACTIONAL, + /* bottomPaddingFraction= */ 0.08f); + + // Should not throw - all three cues stacked from bottom + subtitleOutput.dispatchDraw(canvas); + } }