From 0fa66534fe0739715954cacaf03d2ec426b25aab Mon Sep 17 00:00:00 2001 From: ibaker Date: Tue, 18 Jul 2023 10:27:40 +0100 Subject: [PATCH] Use `CuesWithTiming.durationUs` in `SubripParser` instead of empty cues This fixes two things in one go: 1. In order to indicate 'end of a cue' **without** relying on `CuesWithTiming.durationUs`, `SubripParser` should have been emitting an empty `List` instead of the current behaviour of a single-item list with `cue.text=""`. 2. There's no need for the empty cue (or cue list), we can use `durationUs` to indicate the end of each list of cues. There's no real need to ever have a `Cue` with `text=""`, so also deprecate `Cue.EMPTY`. PiperOrigin-RevId: 548938874 --- api.txt | 2 +- .../java/androidx/media3/common/text/Cue.java | 7 +- .../extractor/text/subrip/SubripParser.java | 29 +++---- .../text/subrip/SubripParserTest.java | 82 +++++++++---------- 4 files changed, 57 insertions(+), 63 deletions(-) diff --git a/api.txt b/api.txt index 48953e6dc8..163431944a 100644 --- a/api.txt +++ b/api.txt @@ -1188,7 +1188,7 @@ package androidx.media3.common.text { field public static final int ANCHOR_TYPE_MIDDLE = 1; // 0x1 field public static final int ANCHOR_TYPE_START = 0; // 0x0 field public static final float DIMEN_UNSET = -3.4028235E38f; - field public static final androidx.media3.common.text.Cue EMPTY; + field @Deprecated public static final androidx.media3.common.text.Cue EMPTY; field public static final int LINE_TYPE_FRACTION = 0; // 0x0 field public static final int LINE_TYPE_NUMBER = 1; // 0x1 field public static final int TEXT_SIZE_TYPE_ABSOLUTE = 2; // 0x2 diff --git a/libraries/common/src/main/java/androidx/media3/common/text/Cue.java b/libraries/common/src/main/java/androidx/media3/common/text/Cue.java index d957c9b80a..d5d4b0148f 100644 --- a/libraries/common/src/main/java/androidx/media3/common/text/Cue.java +++ b/libraries/common/src/main/java/androidx/media3/common/text/Cue.java @@ -50,8 +50,11 @@ import org.checkerframework.dataflow.qual.Pure; // information around in a sidecar object. public final class Cue implements Bundleable { - /** The empty cue. */ - public static final Cue EMPTY = new Cue.Builder().setText("").build(); + /** + * @deprecated There's no general need for a cue with an empty text string. If you need one, + * create it yourself. + */ + @Deprecated public static final Cue EMPTY = new Cue.Builder().setText("").build(); /** An unset position, width or size. */ // Note: We deliberately don't use Float.MIN_VALUE because it's positive & very close to zero. diff --git a/libraries/extractor/src/main/java/androidx/media3/extractor/text/subrip/SubripParser.java b/libraries/extractor/src/main/java/androidx/media3/extractor/text/subrip/SubripParser.java index ca99741fc4..ff4b05006a 100644 --- a/libraries/extractor/src/main/java/androidx/media3/extractor/text/subrip/SubripParser.java +++ b/libraries/extractor/src/main/java/androidx/media3/extractor/text/subrip/SubripParser.java @@ -22,11 +22,9 @@ import android.text.Spanned; import android.text.TextUtils; import androidx.annotation.Nullable; import androidx.annotation.VisibleForTesting; -import androidx.media3.common.C; import androidx.media3.common.text.Cue; import androidx.media3.common.util.Assertions; import androidx.media3.common.util.Log; -import androidx.media3.common.util.LongArray; import androidx.media3.common.util.ParsableByteArray; import androidx.media3.common.util.UnstableApi; import androidx.media3.common.util.Util; @@ -82,8 +80,7 @@ public final class SubripParser implements SubtitleParser { @Nullable @Override public ImmutableList parse(byte[] data, int offset, int length) { - ArrayList cues = new ArrayList<>(); - LongArray startTimesUs = new LongArray(); + ImmutableList.Builder cues = new ImmutableList.Builder<>(); if (dataScratch.length < length) { dataScratch = new byte[length]; @@ -115,10 +112,12 @@ public final class SubripParser implements SubtitleParser { break; } + long startTimeUs; + long endTimeUs; Matcher matcher = SUBRIP_TIMING_LINE.matcher(currentLine); if (matcher.matches()) { - startTimesUs.add(parseTimecode(matcher, /* groupOffset= */ 1)); - startTimesUs.add(parseTimecode(matcher, /* groupOffset= */ 6)); + startTimeUs = parseTimecode(matcher, /* groupOffset= */ 1); + endTimeUs = parseTimecode(matcher, /* groupOffset= */ 6); } else { Log.w(TAG, "Skipping invalid timing: " + currentLine); continue; @@ -147,21 +146,13 @@ public final class SubripParser implements SubtitleParser { break; } } - cues.add(buildCue(text, alignmentTag)); - cues.add(Cue.EMPTY); - } - - ImmutableList.Builder cuesWithStartTimeAndDuration = ImmutableList.builder(); - for (int i = 0; i < cues.size(); i++) { - cuesWithStartTimeAndDuration.add( + cues.add( new CuesWithTiming( - /* cues= */ ImmutableList.of(cues.get(i)), - /* startTimeUs= */ startTimesUs.get(i), - /* durationUs= */ i == cues.size() - 1 - ? C.TIME_UNSET - : startTimesUs.get(i + 1) - startTimesUs.get(i))); + ImmutableList.of(buildCue(text, alignmentTag)), + startTimeUs, + /* durationUs= */ endTimeUs - startTimeUs)); } - return cuesWithStartTimeAndDuration.build(); + return cues.build(); } @Override diff --git a/libraries/extractor/src/test/java/androidx/media3/extractor/text/subrip/SubripParserTest.java b/libraries/extractor/src/test/java/androidx/media3/extractor/text/subrip/SubripParserTest.java index f887af9fa9..31771ecae1 100644 --- a/libraries/extractor/src/test/java/androidx/media3/extractor/text/subrip/SubripParserTest.java +++ b/libraries/extractor/src/test/java/androidx/media3/extractor/text/subrip/SubripParserTest.java @@ -65,10 +65,10 @@ public final class SubripParserTest { List allCues = parser.parse(bytes); - assertThat(allCues).hasSize(6); + assertThat(allCues).hasSize(3); assertTypicalCue1(allCues.get(0)); - assertTypicalCue2(allCues.get(2)); - assertTypicalCue3(allCues.get(4)); + assertTypicalCue2(allCues.get(1)); + assertTypicalCue3(allCues.get(2)); } @Test @@ -78,11 +78,11 @@ public final class SubripParserTest { ImmutableList allCues = parser.parse(bytes, 10, bytes.length - 15); - assertThat(allCues).hasSize(4); + assertThat(allCues).hasSize(2); // Because of the offset, we skip the first line of dialogue assertTypicalCue2(allCues.get(0)); // Because of the length restriction, we only partially parse the third line of dialogue - Cue thirdCue = allCues.get(2).cues.get(0); + Cue thirdCue = allCues.get(1).cues.get(0); assertThat(thirdCue.text.toString()).isEqualTo("This is the third subti"); } @@ -95,10 +95,10 @@ public final class SubripParserTest { List allCues = parser.parse(bytes); - assertThat(allCues).hasSize(6); + assertThat(allCues).hasSize(3); assertTypicalCue1(allCues.get(0)); - assertTypicalCue2(allCues.get(2)); - assertTypicalCue3(allCues.get(4)); + assertTypicalCue2(allCues.get(1)); + assertTypicalCue3(allCues.get(2)); } @Test @@ -110,10 +110,10 @@ public final class SubripParserTest { List allCues = parser.parse(bytes); - assertThat(allCues).hasSize(6); + assertThat(allCues).hasSize(3); assertTypicalCue1(allCues.get(0)); - assertTypicalCue2(allCues.get(2)); - assertTypicalCue3(allCues.get(4)); + assertTypicalCue2(allCues.get(1)); + assertTypicalCue3(allCues.get(2)); } @Test @@ -126,9 +126,9 @@ public final class SubripParserTest { List allCues = parser.parse(bytes); - assertThat(allCues).hasSize(4); + assertThat(allCues).hasSize(2); assertTypicalCue1(allCues.get(0)); - assertTypicalCue3(allCues.get(2)); + assertTypicalCue3(allCues.get(1)); } @Test @@ -141,9 +141,9 @@ public final class SubripParserTest { List allCues = parser.parse(bytes); - assertThat(allCues).hasSize(4); + assertThat(allCues).hasSize(2); assertTypicalCue1(allCues.get(0)); - assertTypicalCue3(allCues.get(2)); + assertTypicalCue3(allCues.get(1)); } @Test @@ -156,7 +156,7 @@ public final class SubripParserTest { List allCues = parser.parse(bytes); - assertThat(allCues).hasSize(2); + assertThat(allCues).hasSize(1); assertTypicalCue3(allCues.get(0)); } @@ -169,9 +169,9 @@ public final class SubripParserTest { List allCues = parser.parse(bytes); - assertThat(allCues).hasSize(4); + assertThat(allCues).hasSize(2); assertTypicalCue1(allCues.get(0)); - assertTypicalCue2(allCues.get(2)); + assertTypicalCue2(allCues.get(1)); } @Test @@ -182,10 +182,10 @@ public final class SubripParserTest { List allCues = parser.parse(bytes); - assertThat(allCues).hasSize(6); + assertThat(allCues).hasSize(3); assertTypicalCue1(allCues.get(0)); - assertTypicalCue2(allCues.get(2)); - assertTypicalCue3(allCues.get(4)); + assertTypicalCue2(allCues.get(1)); + assertTypicalCue3(allCues.get(2)); } @Test @@ -196,10 +196,10 @@ public final class SubripParserTest { List allCues = parser.parse(bytes); - assertThat(allCues).hasSize(6); + assertThat(allCues).hasSize(3); assertTypicalCue1(allCues.get(0)); - assertTypicalCue2(allCues.get(2)); - assertTypicalCue3(allCues.get(4)); + assertTypicalCue2(allCues.get(1)); + assertTypicalCue3(allCues.get(2)); } @Test @@ -212,22 +212,22 @@ public final class SubripParserTest { assertThat(allCues).isNotNull(); assertThat(allCues.get(0).cues.get(0).text.toString()).isEqualTo("This is the first subtitle."); - assertThat(allCues.get(2).cues.get(0).text.toString()) + assertThat(allCues.get(1).cues.get(0).text.toString()) .isEqualTo("This is the second subtitle.\nSecond subtitle with second line."); - assertThat(allCues.get(4).cues.get(0).text.toString()).isEqualTo("This is the third subtitle."); - assertThat(allCues.get(6).cues.get(0).text.toString()) + assertThat(allCues.get(2).cues.get(0).text.toString()).isEqualTo("This is the third subtitle."); + assertThat(allCues.get(3).cues.get(0).text.toString()) .isEqualTo("This { \\an2} is not a valid tag due to the space after the opening bracket."); - assertThat(allCues.get(8).cues.get(0).text.toString()) + assertThat(allCues.get(4).cues.get(0).text.toString()) .isEqualTo("This is the fifth subtitle with multiple valid tags."); - assertAlignmentCue(allCues.get(10), Cue.ANCHOR_TYPE_END, Cue.ANCHOR_TYPE_START); // {/an1} - assertAlignmentCue(allCues.get(12), Cue.ANCHOR_TYPE_END, Cue.ANCHOR_TYPE_MIDDLE); // {/an2} - assertAlignmentCue(allCues.get(14), Cue.ANCHOR_TYPE_END, Cue.ANCHOR_TYPE_END); // {/an3} - assertAlignmentCue(allCues.get(16), Cue.ANCHOR_TYPE_MIDDLE, Cue.ANCHOR_TYPE_START); // {/an4} - assertAlignmentCue(allCues.get(18), Cue.ANCHOR_TYPE_MIDDLE, Cue.ANCHOR_TYPE_MIDDLE); // {/an5} - assertAlignmentCue(allCues.get(20), Cue.ANCHOR_TYPE_MIDDLE, Cue.ANCHOR_TYPE_END); // {/an6} - assertAlignmentCue(allCues.get(22), Cue.ANCHOR_TYPE_START, Cue.ANCHOR_TYPE_START); // {/an7} - assertAlignmentCue(allCues.get(24), Cue.ANCHOR_TYPE_START, Cue.ANCHOR_TYPE_MIDDLE); // {/an8} - assertAlignmentCue(allCues.get(26), Cue.ANCHOR_TYPE_START, Cue.ANCHOR_TYPE_END); // {/an9} + assertAlignmentCue(allCues.get(5), Cue.ANCHOR_TYPE_END, Cue.ANCHOR_TYPE_START); // {/an1} + assertAlignmentCue(allCues.get(6), Cue.ANCHOR_TYPE_END, Cue.ANCHOR_TYPE_MIDDLE); // {/an2} + assertAlignmentCue(allCues.get(7), Cue.ANCHOR_TYPE_END, Cue.ANCHOR_TYPE_END); // {/an3} + assertAlignmentCue(allCues.get(8), Cue.ANCHOR_TYPE_MIDDLE, Cue.ANCHOR_TYPE_START); // {/an4} + assertAlignmentCue(allCues.get(9), Cue.ANCHOR_TYPE_MIDDLE, Cue.ANCHOR_TYPE_MIDDLE); // {/an5} + assertAlignmentCue(allCues.get(10), Cue.ANCHOR_TYPE_MIDDLE, Cue.ANCHOR_TYPE_END); // {/an6} + assertAlignmentCue(allCues.get(11), Cue.ANCHOR_TYPE_START, Cue.ANCHOR_TYPE_START); // {/an7} + assertAlignmentCue(allCues.get(12), Cue.ANCHOR_TYPE_START, Cue.ANCHOR_TYPE_MIDDLE); // {/an8} + assertAlignmentCue(allCues.get(13), Cue.ANCHOR_TYPE_START, Cue.ANCHOR_TYPE_END); // {/an9} } @Test @@ -239,11 +239,11 @@ public final class SubripParserTest { List allCues = parser.parse(bytes); - assertThat(allCues).hasSize(6); + assertThat(allCues).hasSize(3); assertTypicalCue1(allCues.get(0)); - assertThat(allCues.get(2).startTimeUs).isEqualTo(2_000_000); - assertThat(allCues.get(3).startTimeUs).isEqualTo(3_000_000); - assertTypicalCue3(allCues.get(4)); + assertThat(allCues.get(1).startTimeUs).isEqualTo(2_000_000); + assertThat(allCues.get(1).durationUs).isEqualTo(1_000_000); + assertTypicalCue3(allCues.get(2)); } private static void assertTypicalCue1(CuesWithTiming cuesWithTiming) {