Skip to content

Commit

Permalink
Don't emit a CuesWithTiming for zero-duration Subtitle events
Browse files Browse the repository at this point in the history 
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: androidx/media#1177

#minor-release

PiperOrigin-RevId: 616095798
  • Loading branch information
@icbaker @copybara-github
icbaker authored and copybara-github committed Mar 15, 2024
1 parent 9fafa37 commit 1fdb42b
Show file tree
Hide file tree
Showing 4 changed files with 145 additions and 1 deletion.
4 changes: 3 additions & 1 deletion library/extractor/src/main/java/com/google/android/exoplayer2/text/LegacySubtitleUtil.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,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));
}
}
}
113 changes: 113 additions & 0 deletions ...src/test/java/com/google/android/exoplayer2/text/webvtt/LegacySubtitleUtilWebvttTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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 =
Expand Down Expand Up @@ -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 =
Expand Down Expand Up @@ -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 =
Expand Down
22 changes: 22 additions & 0 deletions ...y/extractor/src/test/java/com/google/android/exoplayer2/text/webvtt/WebvttParserTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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);
Expand Down
7 changes: 7 additions & 0 deletions testdata/src/test/assets/media/webvtt/with_consecutive_cues
View file
Original file line number Diff line number Diff line change
@@ -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.

0 comments on commit 1fdb42b

Please sign in to comment.