Skip to content

Commit

Permalink
Return true from CuesResolver.addCues if the output changed
Browse files Browse the repository at this point in the history 
This belongs in the resolver, because it depends on the resolution
algorithm (and therefore the logic can't live in `TextRenderer`).

This also fixes a bug in `TextRenderer` where we were doing arithmetic
with `cues.durationUs` without checking if it was `TIME_UNSET` first.

#minor-release

PiperOrigin-RevId: 571332750
  • Loading branch information
@icbaker @copybara-github
icbaker authored and copybara-github committed Oct 6, 2023
1 parent 78f403a commit 2724287
Show file tree
Hide file tree
Showing 6 changed files with 173 additions and 40 deletions.
7 changes: 5 additions & 2 deletions libraries/exoplayer/src/main/java/androidx/media3/exoplayer/text/CuesResolver.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,11 @@
*/
/* package */ interface CuesResolver {

/** Adds cues to this instance. */
void addCues(CuesWithTiming cues);
/**
* Adds {@code cues} to this instance, returning whether this changes the cues displayed at {@code
* currentPositionUs}.
*/
boolean addCues(CuesWithTiming cues, long currentPositionUs);

/**
* Returns the {@linkplain Cue cues} that should be shown at time {@code timeUs}.
Expand Down
7 changes: 5 additions & 2 deletions libraries/exoplayer/src/main/java/androidx/media3/exoplayer/text/MergingCuesResolver.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,16 +60,19 @@ public MergingCuesResolver() {
}

@Override
public void addCues(CuesWithTiming cues) {
public boolean addCues(CuesWithTiming cues, long currentPositionUs) {
checkArgument(cues.startTimeUs != C.TIME_UNSET);
checkArgument(cues.durationUs != C.TIME_UNSET);
boolean cuesAreShownAtCurrentTime =
cues.startTimeUs <= currentPositionUs && currentPositionUs < cues.endTimeUs;
for (int i = cuesWithTimingList.size() - 1; i >= 0; i--) {
if (cues.startTimeUs >= cuesWithTimingList.get(i).startTimeUs) {
cuesWithTimingList.add(i + 1, cues);
return;
return cuesAreShownAtCurrentTime;
}
}
cuesWithTimingList.add(0, cues);
return cuesAreShownAtCurrentTime;
}

@Override
Expand Down
15 changes: 13 additions & 2 deletions libraries/exoplayer/src/main/java/androidx/media3/exoplayer/text/ReplacingCuesResolver.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
*/
package androidx.media3.exoplayer.text;

import static androidx.media3.common.util.Assertions.checkArgument;

import androidx.media3.common.C;
import androidx.media3.common.text.Cue;
import androidx.media3.extractor.text.CuesWithTiming;
Expand Down Expand Up @@ -43,14 +45,23 @@ public ReplacingCuesResolver() {
}

@Override
public void addCues(CuesWithTiming cues) {
public boolean addCues(CuesWithTiming cues, long currentPositionUs) {
checkArgument(cues.startTimeUs != C.TIME_UNSET);
boolean cuesAreShownAtCurrentTime =
cues.startTimeUs <= currentPositionUs
&& (cues.endTimeUs == C.TIME_UNSET || currentPositionUs < cues.endTimeUs);
for (int i = cuesWithTimingList.size() - 1; i >= 0; i--) {
if (cues.startTimeUs >= cuesWithTimingList.get(i).startTimeUs) {
cuesWithTimingList.add(i + 1, cues);
return;
return cuesAreShownAtCurrentTime;
} else if (cuesWithTimingList.get(i).startTimeUs <= currentPositionUs) {
// There's a cue that starts after the new cues, but before the current time, meaning
// the new cues will not be displayed at the current time.
cuesAreShownAtCurrentTime = false;
}
}
cuesWithTimingList.add(0, cues);
return cuesAreShownAtCurrentTime;
}

@Override
Expand Down
7 changes: 1 addition & 6 deletions libraries/exoplayer/src/main/java/androidx/media3/exoplayer/text/TextRenderer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -312,12 +312,7 @@ private boolean readAndDecodeCuesWithTiming(long positionUs) {
cueData.limit());
cueDecoderInputBuffer.clear();

cuesResolver.addCues(cuesWithTiming);

// Return whether the CuesWithTiming we added to CuesMerger changes the subtitles that
// should be on-screen *now*.
return cuesWithTiming.startTimeUs <= positionUs
&& positionUs < cuesWithTiming.startTimeUs + cuesWithTiming.durationUs;
return cuesResolver.addCues(cuesWithTiming, positionUs);
case C.RESULT_FORMAT_READ:
case C.RESULT_NOTHING_READ:
default:
Expand Down
82 changes: 67 additions & 15 deletions ...aries/exoplayer/src/test/java/androidx/media3/exoplayer/text/MergingCuesResolverTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,8 @@ public void nonOverlappingCues() {
new CuesWithTiming(SECOND_CUES, /* startTimeUs= */ 6_000_000, /* durationUs= */ 1_000_000);

// Reverse the addCues call to check everything still works (it should).
mergingCuesResolver.addCues(secondCuesWithTiming);
mergingCuesResolver.addCues(firstCuesWithTiming);
mergingCuesResolver.addCues(secondCuesWithTiming, /* currentPositionUs= */ 0);
mergingCuesResolver.addCues(firstCuesWithTiming, /* currentPositionUs= */ 0);

assertCuesStartAt(mergingCuesResolver, 3_000_000);

Expand All @@ -82,8 +82,8 @@ public void overlappingCues() {
CuesWithTiming secondCuesWithTiming =
new CuesWithTiming(SECOND_CUES, /* startTimeUs= */ 2_000_000, /* durationUs= */ 4_000_000);

mergingCuesResolver.addCues(firstCuesWithTiming);
mergingCuesResolver.addCues(secondCuesWithTiming);
mergingCuesResolver.addCues(firstCuesWithTiming, /* currentPositionUs= */ 0);
mergingCuesResolver.addCues(secondCuesWithTiming, /* currentPositionUs= */ 0);

assertCuesStartAt(mergingCuesResolver, 1_000_000);
assertCueTextBetween(mergingCuesResolver, 1_000_000, 2_000_000, "first cue");
Expand All @@ -109,8 +109,8 @@ public void overlappingCues_matchingStartTimes() {
CuesWithTiming secondCuesWithTiming =
new CuesWithTiming(SECOND_CUES, /* startTimeUs= */ 1_000_000, /* durationUs= */ 3_000_000);

mergingCuesResolver.addCues(firstCuesWithTiming);
mergingCuesResolver.addCues(secondCuesWithTiming);
mergingCuesResolver.addCues(firstCuesWithTiming, /* currentPositionUs= */ 0);
mergingCuesResolver.addCues(secondCuesWithTiming, /* currentPositionUs= */ 0);

assertCuesStartAt(mergingCuesResolver, 1_000_000);
// secondCuesWithTiming has a shorter duration than firstCuesWithTiming, so should appear later
Expand All @@ -134,8 +134,8 @@ public void overlappingCues_matchingEndTimes() {
CuesWithTiming secondCuesWithTiming =
new CuesWithTiming(SECOND_CUES, /* startTimeUs= */ 2_000_000, /* durationUs= */ 3_000_000);

mergingCuesResolver.addCues(firstCuesWithTiming);
mergingCuesResolver.addCues(secondCuesWithTiming);
mergingCuesResolver.addCues(firstCuesWithTiming, /* currentPositionUs= */ 0);
mergingCuesResolver.addCues(secondCuesWithTiming, /* currentPositionUs= */ 0);

assertCuesStartAt(mergingCuesResolver, 1_000_000);
// secondCuesWithTiming has a shorter duration than firstCuesWithTiming, so should appear later
Expand All @@ -158,7 +158,59 @@ public void unsetDuration_unsupported() {
new CuesWithTiming(
FIRST_CUES, /* startTimeUs= */ 3_000_000, /* durationUs= */ C.TIME_UNSET);

assertThrows(IllegalArgumentException.class, () -> mergingCuesResolver.addCues(cuesWithTiming));
assertThrows(
IllegalArgumentException.class,
() -> mergingCuesResolver.addCues(cuesWithTiming, /* currentPositionUs= */ 0));
}

@Test
public void addCues_cuesStartAfterCurrentPosition() {
MergingCuesResolver mergingCuesResolver = new MergingCuesResolver();
CuesWithTiming cuesWithTiming =
new CuesWithTiming(FIRST_CUES, /* startTimeUs= */ 3_000_000, /* durationUs= */ 2_000_000);

assertThat(mergingCuesResolver.addCues(cuesWithTiming, /* currentPositionUs= */ 1_000_000))
.isFalse();
}

@Test
public void addCues_cuesStartAtCurrentPosition() {
MergingCuesResolver mergingCuesResolver = new MergingCuesResolver();
CuesWithTiming cuesWithTiming =
new CuesWithTiming(FIRST_CUES, /* startTimeUs= */ 3_000_000, /* durationUs= */ 2_000_000);

assertThat(mergingCuesResolver.addCues(cuesWithTiming, /* currentPositionUs= */ 3_000_000))
.isTrue();
}

@Test
public void addCues_cuesDisplayedAtCurrentPosition() {
MergingCuesResolver mergingCuesResolver = new MergingCuesResolver();
CuesWithTiming cuesWithTiming =
new CuesWithTiming(FIRST_CUES, /* startTimeUs= */ 3_000_000, /* durationUs= */ 2_000_000);

assertThat(mergingCuesResolver.addCues(cuesWithTiming, /* currentPositionUs= */ 4_000_000))
.isTrue();
}

@Test
public void addCues_cuesEndBeforeCurrentPosition() {
MergingCuesResolver mergingCuesResolver = new MergingCuesResolver();
CuesWithTiming cuesWithTiming =
new CuesWithTiming(FIRST_CUES, /* startTimeUs= */ 3_000_000, /* durationUs= */ 2_000_000);

assertThat(mergingCuesResolver.addCues(cuesWithTiming, /* currentPositionUs= */ 6_000_000))
.isFalse();
}

@Test
public void addCues_cuesEndAtCurrentPosition() {
MergingCuesResolver mergingCuesResolver = new MergingCuesResolver();
CuesWithTiming cuesWithTiming =
new CuesWithTiming(FIRST_CUES, /* startTimeUs= */ 3_000_000, /* durationUs= */ 2_000_000);

assertThat(mergingCuesResolver.addCues(cuesWithTiming, /* currentPositionUs= */ 5_000_000))
.isFalse();
}

@Test
Expand All @@ -171,9 +223,9 @@ public void discardCuesBeforeTimeUs() {
CuesWithTiming thirdCuesWithTiming =
new CuesWithTiming(THIRD_CUES, /* startTimeUs= */ 8_000_000, /* durationUs= */ 4_000_000);

mergingCuesResolver.addCues(firstCuesWithTiming);
mergingCuesResolver.addCues(secondCuesWithTiming);
mergingCuesResolver.addCues(thirdCuesWithTiming);
mergingCuesResolver.addCues(firstCuesWithTiming, /* currentPositionUs= */ 0);
mergingCuesResolver.addCues(secondCuesWithTiming, /* currentPositionUs= */ 0);
mergingCuesResolver.addCues(thirdCuesWithTiming, /* currentPositionUs= */ 0);

// Remove only firstCuesWithTiming (secondCuesWithTiming should be kept because it ends after
// this time).
Expand All @@ -195,9 +247,9 @@ public void clear_clearsAllCues() {
CuesWithTiming thirdCuesWithTiming =
new CuesWithTiming(THIRD_CUES, /* startTimeUs= */ 8_000_000, /* durationUs= */ 4_000_000);

mergingCuesResolver.addCues(firstCuesWithTiming);
mergingCuesResolver.addCues(secondCuesWithTiming);
mergingCuesResolver.addCues(thirdCuesWithTiming);
mergingCuesResolver.addCues(firstCuesWithTiming, /* currentPositionUs= */ 0);
mergingCuesResolver.addCues(secondCuesWithTiming, /* currentPositionUs= */ 0);
mergingCuesResolver.addCues(thirdCuesWithTiming, /* currentPositionUs= */ 0);

mergingCuesResolver.clear();

Expand Down
95 changes: 82 additions & 13 deletions ...ies/exoplayer/src/test/java/androidx/media3/exoplayer/text/ReplacingCuesResolverTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,8 @@ public void unsetDuration() {
SECOND_CUES, /* startTimeUs= */ 6_000_000, /* durationUs= */ C.TIME_UNSET);

// Reverse the addCues call to check everything still works (it should).
replacingCuesResolver.addCues(secondCuesWithTiming);
replacingCuesResolver.addCues(cuesWithTiming);
replacingCuesResolver.addCues(secondCuesWithTiming, /* currentPositionUs= */ 0);
replacingCuesResolver.addCues(cuesWithTiming, /* currentPositionUs= */ 0);

assertCuesStartAt(replacingCuesResolver, 3_000_000);
assertCueTextBetween(replacingCuesResolver, 3_000_000, 6_000_000, "first cue");
Expand All @@ -81,8 +81,8 @@ public void nonOverlappingCues() {
CuesWithTiming secondCuesWithTiming =
new CuesWithTiming(SECOND_CUES, /* startTimeUs= */ 6_000_000, /* durationUs= */ 1_000_000);

replacingCuesResolver.addCues(firstCuesWithTiming);
replacingCuesResolver.addCues(secondCuesWithTiming);
replacingCuesResolver.addCues(firstCuesWithTiming, /* currentPositionUs= */ 0);
replacingCuesResolver.addCues(secondCuesWithTiming, /* currentPositionUs= */ 0);

assertCuesStartAt(replacingCuesResolver, 3_000_000);

Expand All @@ -101,8 +101,8 @@ public void overlappingCues_secondReplacesFirst() {
CuesWithTiming secondCuesWithTiming =
new CuesWithTiming(SECOND_CUES, /* startTimeUs= */ 2_000_000, /* durationUs= */ 4_000_000);

replacingCuesResolver.addCues(firstCuesWithTiming);
replacingCuesResolver.addCues(secondCuesWithTiming);
replacingCuesResolver.addCues(firstCuesWithTiming, /* currentPositionUs= */ 0);
replacingCuesResolver.addCues(secondCuesWithTiming, /* currentPositionUs= */ 0);

assertCuesStartAt(replacingCuesResolver, 1_000_000);
assertCueTextBetween(replacingCuesResolver, 1_000_000, 2_000_000, "first cue");
Expand All @@ -119,15 +119,84 @@ public void overlappingCues_matchingStartTimes_onlySecondEmitted() {
CuesWithTiming secondCuesWithTiming =
new CuesWithTiming(SECOND_CUES, /* startTimeUs= */ 1_000_000, /* durationUs= */ 3_000_000);

replacingCuesResolver.addCues(firstCuesWithTiming);
replacingCuesResolver.addCues(secondCuesWithTiming);
replacingCuesResolver.addCues(firstCuesWithTiming, /* currentPositionUs= */ 0);
replacingCuesResolver.addCues(secondCuesWithTiming, /* currentPositionUs= */ 0);

assertCuesStartAt(replacingCuesResolver, 1_000_000);
assertCueTextBetween(
replacingCuesResolver, 1_000_000, 4_000_000, "second group: cue1", "second group: cue2");
assertCuesEndAt(replacingCuesResolver, 4_000_000);
}

@Test
public void addCues_cuesStartAfterCurrentPosition() {
ReplacingCuesResolver replacingCuesResolver = new ReplacingCuesResolver();
CuesWithTiming cuesWithTiming =
new CuesWithTiming(FIRST_CUES, /* startTimeUs= */ 3_000_000, /* durationUs= */ 2_000_000);

assertThat(replacingCuesResolver.addCues(cuesWithTiming, /* currentPositionUs= */ 1_000_000))
.isFalse();
}

@Test
public void addCues_cuesStartAtCurrentPosition() {
ReplacingCuesResolver replacingCuesResolver = new ReplacingCuesResolver();
CuesWithTiming cuesWithTiming =
new CuesWithTiming(FIRST_CUES, /* startTimeUs= */ 3_000_000, /* durationUs= */ 2_000_000);

assertThat(replacingCuesResolver.addCues(cuesWithTiming, /* currentPositionUs= */ 3_000_000))
.isTrue();
}

@Test
public void addCues_cuesDisplayedAtCurrentPosition() {
ReplacingCuesResolver replacingCuesResolver = new ReplacingCuesResolver();
CuesWithTiming cuesWithTiming =
new CuesWithTiming(FIRST_CUES, /* startTimeUs= */ 3_000_000, /* durationUs= */ 2_000_000);

assertThat(replacingCuesResolver.addCues(cuesWithTiming, /* currentPositionUs= */ 4_000_000))
.isTrue();
}

@Test
public void addCues_cuesDisplayedAtCurrentPosition_butAlreadyReplacedByLaterCues() {
ReplacingCuesResolver replacingCuesResolver = new ReplacingCuesResolver();
CuesWithTiming firstCuesWithTiming =
new CuesWithTiming(FIRST_CUES, /* startTimeUs= */ 3_000_000, /* durationUs= */ 3_000_000);
CuesWithTiming secondCuesWithTiming =
new CuesWithTiming(SECOND_CUES, /* startTimeUs= */ 4_000_000, /* durationUs= */ 3_000_000);
CuesWithTiming thirdCuesWithTiming =
new CuesWithTiming(THIRD_CUES, /* startTimeUs= */ 5_000_000, /* durationUs= */ 3_000_000);
replacingCuesResolver.addCues(thirdCuesWithTiming, /* currentPositionUs= */ 0);

assertThat(
replacingCuesResolver.addCues(firstCuesWithTiming, /* currentPositionUs= */ 5_000_000))
.isFalse();
assertThat(
replacingCuesResolver.addCues(secondCuesWithTiming, /* currentPositionUs= */ 5_500_000))
.isFalse();
}

@Test
public void addCues_cuesEndBeforeCurrentPosition() {
ReplacingCuesResolver replacingCuesResolver = new ReplacingCuesResolver();
CuesWithTiming cuesWithTiming =
new CuesWithTiming(FIRST_CUES, /* startTimeUs= */ 3_000_000, /* durationUs= */ 2_000_000);

assertThat(replacingCuesResolver.addCues(cuesWithTiming, /* currentPositionUs= */ 6_000_000))
.isFalse();
}

@Test
public void addCues_cuesEndAtCurrentPosition() {
ReplacingCuesResolver replacingCuesResolver = new ReplacingCuesResolver();
CuesWithTiming cuesWithTiming =
new CuesWithTiming(FIRST_CUES, /* startTimeUs= */ 3_000_000, /* durationUs= */ 2_000_000);

assertThat(replacingCuesResolver.addCues(cuesWithTiming, /* currentPositionUs= */ 5_000_000))
.isFalse();
}

@Test
public void discardCuesBeforeTimeUs() {
ReplacingCuesResolver replacingCuesResolver = new ReplacingCuesResolver();
Expand All @@ -136,8 +205,8 @@ public void discardCuesBeforeTimeUs() {
CuesWithTiming secondCuesWithTiming =
new CuesWithTiming(SECOND_CUES, /* startTimeUs= */ 6_000_000, /* durationUs= */ 1_000_000);

replacingCuesResolver.addCues(firstCuesWithTiming);
replacingCuesResolver.addCues(secondCuesWithTiming);
replacingCuesResolver.addCues(firstCuesWithTiming, /* currentPositionUs= */ 0);
replacingCuesResolver.addCues(secondCuesWithTiming, /* currentPositionUs= */ 0);

// Remove firstCuesWithTiming
replacingCuesResolver.discardCuesBeforeTimeUs(5_500_000);
Expand All @@ -158,9 +227,9 @@ public void clear_clearsAllCues() {
CuesWithTiming thirdCuesWithTiming =
new CuesWithTiming(THIRD_CUES, /* startTimeUs= */ 8_000_000, /* durationUs= */ 4_000_000);

replacingCuesResolver.addCues(firstCuesWithTiming);
replacingCuesResolver.addCues(secondCuesWithTiming);
replacingCuesResolver.addCues(thirdCuesWithTiming);
replacingCuesResolver.addCues(firstCuesWithTiming, /* currentPositionUs= */ 0);
replacingCuesResolver.addCues(secondCuesWithTiming, /* currentPositionUs= */ 0);
replacingCuesResolver.addCues(thirdCuesWithTiming, /* currentPositionUs= */ 0);

replacingCuesResolver.clear();

Expand Down

0 comments on commit 2724287

Please sign in to comment.