Don't emit a CuesWithTiming for zero-duration Subtitle events

It's a bit arguable whether the `Subtitle` implementation supports zero-duration events, since `getEventTimeCount` is documented as effectively "the number of times the cues returns by `getCues(long)` changes", and zero-duration events violate that. However, the current `WebvttSubtitle` impl **does** produce zero-duration events, so it seems safer to handle them gracefully here and then, as a possible follow-up, fix the `WebvttSubtitle` impl (or remove it completely). Issue: #1177 #minor-release PiperOrigin-RevId: 616095798
androidx · Mar 15, 2024 · e9ed874 · e9ed874
1 parent f39fe82
commit e9ed874
Show file tree

Hide file tree

Showing 5 changed files with 148 additions and 1 deletion.
diff --git a/RELEASENOTES.md b/RELEASENOTES.md
@@ -48,6 +48,9 @@
         tunneling even if the device does not do this automatically as required
         by the API ([#1169](https://github.com/androidx/media/issues/1169)).
 *   Text:
+    *   WebVTT: Prevent directly consecutive cues from creating spurious
+        additional `CuesWithTiming` instances from `WebvttParser.parse`
+        ([#1177](https://github.com/androidx/media/issues/1177)).
 *   Metadata:
 *   Image:
 *   DRM:

diff --git a/libraries/extractor/src/main/java/androidx/media3/extractor/text/LegacySubtitleUtil.java b/libraries/extractor/src/main/java/androidx/media3/extractor/text/LegacySubtitleUtil.java
@@ -101,6 +101,8 @@ private static void outputSubtitleEvent(
     // It's safe to inspect element i+1, because we already exited the loop above if
     // i == getEventTimeCount() - 1.
     long durationUs = subtitle.getEventTime(eventIndex + 1) - subtitle.getEventTime(eventIndex);
-    output.accept(new CuesWithTiming(cuesForThisStartTime, startTimeUs, durationUs));
+    if (durationUs > 0) {
+      output.accept(new CuesWithTiming(cuesForThisStartTime, startTimeUs, durationUs));
+    }
   }
 }
diff --git a/...tor/src/test/java/androidx/media3/extractor/text/webvtt/LegacySubtitleUtilWebvttTest.java b/...tor/src/test/java/androidx/media3/extractor/text/webvtt/LegacySubtitleUtilWebvttTest.java
@@ -52,6 +52,18 @@ public class LegacySubtitleUtilWebvttTest {
                   /* startTimeUs= */ 3_000_000,
                   /* endTimeUs= */ 4_000_000)));
 
+  private static final WebvttSubtitle CONSECUTIVE_SUBTITLE =
+      new WebvttSubtitle(
+          Arrays.asList(
+              new WebvttCueInfo(
+                  WebvttCueParser.newCueForText(FIRST_SUBTITLE_STRING),
+                  /* startTimeUs= */ 1_000_000,
+                  /* endTimeUs= */ 2_000_000),
+              new WebvttCueInfo(
+                  WebvttCueParser.newCueForText(SECOND_SUBTITLE_STRING),
+                  /* startTimeUs= */ 2_000_000,
+                  /* endTimeUs= */ 4_000_000)));
+
   private static final WebvttSubtitle OVERLAPPING_SUBTITLE =
       new WebvttSubtitle(
           Arrays.asList(
@@ -82,6 +94,24 @@ public void toCuesWithTiming_allCues_simpleSubtitle() {
         .containsExactly(SECOND_SUBTITLE_STRING);
   }
 
+  @Test
+  public void toCuesWithTiming_allCues_consecutiveSubtitle() {
+    ImmutableList<CuesWithTiming> cuesWithTimingsList =
+        toCuesWithTimingList(CONSECUTIVE_SUBTITLE, SubtitleParser.OutputOptions.allCues());
+
+    assertThat(cuesWithTimingsList).hasSize(2);
+    assertThat(cuesWithTimingsList.get(0).startTimeUs).isEqualTo(1_000_000);
+    assertThat(cuesWithTimingsList.get(0).durationUs).isEqualTo(1_000_000);
+    assertThat(cuesWithTimingsList.get(0).endTimeUs).isEqualTo(2_000_000);
+    assertThat(Lists.transform(cuesWithTimingsList.get(0).cues, c -> c.text))
+        .containsExactly(FIRST_SUBTITLE_STRING);
+    assertThat(cuesWithTimingsList.get(1).startTimeUs).isEqualTo(2_000_000);
+    assertThat(cuesWithTimingsList.get(1).durationUs).isEqualTo(2_000_000);
+    assertThat(cuesWithTimingsList.get(1).endTimeUs).isEqualTo(4_000_000);
+    assertThat(Lists.transform(cuesWithTimingsList.get(1).cues, c -> c.text))
+        .containsExactly(SECOND_SUBTITLE_STRING);
+  }
+
   @Test
   public void toCuesWithTiming_allCues_overlappingSubtitle() {
     ImmutableList<CuesWithTiming> cuesWithTimingsList =
@@ -168,6 +198,40 @@ public void toCuesWithTiming_onlyEmitCuesAfterStartTime_startInMiddleOfCue_simpl
         .containsExactly(SECOND_SUBTITLE_STRING);
   }
 
+  @Test
+  public void toCuesWithTiming_onlyEmitCuesAfterStartTime_startBetweenCues_consecutiveSubtitle() {
+    ImmutableList<CuesWithTiming> cuesWithTimingsList =
+        toCuesWithTimingList(
+            CONSECUTIVE_SUBTITLE, SubtitleParser.OutputOptions.onlyCuesAfter(2_000_000));
+
+    assertThat(cuesWithTimingsList).hasSize(1);
+    assertThat(cuesWithTimingsList.get(0).startTimeUs).isEqualTo(2_000_000);
+    assertThat(cuesWithTimingsList.get(0).durationUs).isEqualTo(2_000_000);
+    assertThat(cuesWithTimingsList.get(0).endTimeUs).isEqualTo(4_000_000);
+    assertThat(Lists.transform(cuesWithTimingsList.get(0).cues, c -> c.text))
+        .containsExactly(SECOND_SUBTITLE_STRING);
+  }
+
+  @Test
+  public void toCuesWithTiming_onlyEmitCuesAfterStartTime_startInMiddleOfCue_consecutiveSubtitle() {
+    ImmutableList<CuesWithTiming> cuesWithTimingsList =
+        toCuesWithTimingList(
+            CONSECUTIVE_SUBTITLE, SubtitleParser.OutputOptions.onlyCuesAfter(1_500_000));
+
+    assertThat(cuesWithTimingsList).hasSize(2);
+    // First cue is truncated to start at OutputOptions.startTimeUs
+    assertThat(cuesWithTimingsList.get(0).startTimeUs).isEqualTo(1_500_000);
+    assertThat(cuesWithTimingsList.get(0).durationUs).isEqualTo(500_000);
+    assertThat(cuesWithTimingsList.get(0).endTimeUs).isEqualTo(2_000_000);
+    assertThat(Lists.transform(cuesWithTimingsList.get(0).cues, c -> c.text))
+        .containsExactly(FIRST_SUBTITLE_STRING);
+    assertThat(cuesWithTimingsList.get(1).startTimeUs).isEqualTo(2_000_000);
+    assertThat(cuesWithTimingsList.get(1).durationUs).isEqualTo(2_000_000);
+    assertThat(cuesWithTimingsList.get(1).endTimeUs).isEqualTo(4_000_000);
+    assertThat(Lists.transform(cuesWithTimingsList.get(1).cues, c -> c.text))
+        .containsExactly(SECOND_SUBTITLE_STRING);
+  }
+
   @Test
   public void toCuesWithTiming_onlyEmitCuesAfterStartTime_overlappingSubtitle() {
     ImmutableList<CuesWithTiming> cuesWithTimingsList =
@@ -259,6 +323,55 @@ public void toCuesWithTiming_onlyEmitCuesAfterStartTime_overlappingSubtitle() {
         .containsExactly(FIRST_SUBTITLE_STRING);
   }
 
+  @Test
+  public void
+      toCuesWithTiming_emitCuesAfterStartTimeThenThoseBefore_startBetweenCues_consecutiveSubtitle() {
+    ImmutableList<CuesWithTiming> cuesWithTimingsList =
+        toCuesWithTimingList(
+            CONSECUTIVE_SUBTITLE,
+            SubtitleParser.OutputOptions.cuesAfterThenRemainingCuesBefore(2_000_000));
+
+    assertThat(cuesWithTimingsList).hasSize(2);
+    assertThat(cuesWithTimingsList.get(0).startTimeUs).isEqualTo(2_000_000);
+    assertThat(cuesWithTimingsList.get(0).durationUs).isEqualTo(2_000_000);
+    assertThat(cuesWithTimingsList.get(0).endTimeUs).isEqualTo(4_000_000);
+    assertThat(Lists.transform(cuesWithTimingsList.get(0).cues, c -> c.text))
+        .containsExactly(SECOND_SUBTITLE_STRING);
+    assertThat(cuesWithTimingsList.get(1).startTimeUs).isEqualTo(1_000_000);
+    assertThat(cuesWithTimingsList.get(1).durationUs).isEqualTo(1_000_000);
+    assertThat(cuesWithTimingsList.get(1).endTimeUs).isEqualTo(2_000_000);
+    assertThat(Lists.transform(cuesWithTimingsList.get(1).cues, c -> c.text))
+        .containsExactly(FIRST_SUBTITLE_STRING);
+  }
+
+  @Test
+  public void
+      toCuesWithTiming_emitCuesAfterStartTimeThenThoseBefore_startInMiddleOfCue_consecutiveSubtitle() {
+    ImmutableList<CuesWithTiming> cuesWithTimingsList =
+        toCuesWithTimingList(
+            CONSECUTIVE_SUBTITLE,
+            SubtitleParser.OutputOptions.cuesAfterThenRemainingCuesBefore(1_500_000));
+
+    assertThat(cuesWithTimingsList).hasSize(3);
+    // First event is truncated to start at OutputOptions.startTimeUs.
+    assertThat(cuesWithTimingsList.get(0).startTimeUs).isEqualTo(1_500_000);
+    assertThat(cuesWithTimingsList.get(0).durationUs).isEqualTo(500_000);
+    assertThat(cuesWithTimingsList.get(0).endTimeUs).isEqualTo(2_000_000);
+    assertThat(Lists.transform(cuesWithTimingsList.get(0).cues, c -> c.text))
+        .containsExactly(FIRST_SUBTITLE_STRING);
+    assertThat(cuesWithTimingsList.get(1).startTimeUs).isEqualTo(2_000_000);
+    assertThat(cuesWithTimingsList.get(1).durationUs).isEqualTo(2_000_000);
+    assertThat(cuesWithTimingsList.get(1).endTimeUs).isEqualTo(4_000_000);
+    assertThat(Lists.transform(cuesWithTimingsList.get(1).cues, c -> c.text))
+        .containsExactly(SECOND_SUBTITLE_STRING);
+    // Final event is the part of the 'first event' that is before OutputOptions.startTimeUs
+    assertThat(cuesWithTimingsList.get(2).startTimeUs).isEqualTo(1_000_000);
+    assertThat(cuesWithTimingsList.get(2).durationUs).isEqualTo(500_000);
+    assertThat(cuesWithTimingsList.get(2).endTimeUs).isEqualTo(1_500_000);
+    assertThat(Lists.transform(cuesWithTimingsList.get(2).cues, c -> c.text))
+        .containsExactly(FIRST_SUBTITLE_STRING);
+  }
+
   @Test
   public void toCuesWithTiming_emitCuesAfterStartTimeThenThoseBefore_overlappingSubtitle() {
     ImmutableList<CuesWithTiming> cuesWithTimingsList =

diff --git a/...aries/extractor/src/test/java/androidx/media3/extractor/text/webvtt/WebvttParserTest.java b/...aries/extractor/src/test/java/androidx/media3/extractor/text/webvtt/WebvttParserTest.java
@@ -50,6 +50,8 @@ public class WebvttParserTest {
   private static final String TYPICAL_WITH_IDS_FILE = "media/webvtt/typical_with_identifiers";
   private static final String TYPICAL_WITH_COMMENTS_FILE = "media/webvtt/typical_with_comments";
   private static final String WITH_POSITIONING_FILE = "media/webvtt/with_positioning";
+  private static final String WITH_CONSECUTIVE_TIMESTAMPS_FILE =
+      "media/webvtt/with_consecutive_cues";
   private static final String WITH_OVERLAPPING_TIMESTAMPS_FILE =
       "media/webvtt/with_overlapping_timestamps";
   private static final String WITH_VERTICAL_FILE = "media/webvtt/with_vertical";
@@ -334,6 +336,26 @@ public void parseWithPositioning() throws Exception {
     assertThat(eighthCue.positionAnchor).isEqualTo(Cue.ANCHOR_TYPE_END);
   }
 
+  // https://github.com/androidx/media/issues/1177
+  @Test
+  public void parseWithConsecutiveTimestamps() throws Exception {
+    ImmutableList<CuesWithTiming> allCues = getCuesForTestAsset(WITH_CONSECUTIVE_TIMESTAMPS_FILE);
+
+    assertThat(allCues).hasSize(2);
+
+    assertThat(allCues.get(0).startTimeUs).isEqualTo(0L);
+    assertThat(allCues.get(0).durationUs).isEqualTo(1_234_000L);
+    assertThat(allCues.get(0).endTimeUs).isEqualTo(1_234_000L);
+    Cue firstCue = Iterables.getOnlyElement(allCues.get(0).cues);
+    assertThat(firstCue.text.toString()).isEqualTo("This is the first subtitle.");
+
+    assertThat(allCues.get(1).startTimeUs).isEqualTo(1_234_000L);
+    assertThat(allCues.get(1).durationUs).isEqualTo(3_456_000L - 1_234_000L);
+    assertThat(allCues.get(1).endTimeUs).isEqualTo(3_456_000L);
+    Cue secondCue = Iterables.getOnlyElement(allCues.get(1).cues);
+    assertThat(secondCue.text.toString()).isEqualTo("This is the second subtitle.");
+  }
+
   @Test
   public void parseWithOverlappingTimestamps() throws Exception {
     List<CuesWithTiming> allCues = getCuesForTestAsset(WITH_OVERLAPPING_TIMESTAMPS_FILE);

diff --git a/libraries/test_data/src/test/assets/media/webvtt/with_consecutive_cues b/libraries/test_data/src/test/assets/media/webvtt/with_consecutive_cues
@@ -0,0 +1,7 @@
+WEBVTT
+
+00:00.000 --> 00:01.234
+This is the first subtitle.
+
+00:01.234 --> 00:03.456
+This is the second subtitle.