Skip to content

SDK / exoplayer

Go to a project
Commit d3b5f71f by ibaker Committed by Rohit Singh
Options

Consider shuffle order in Timeline.equals() 

Previously two timelines that differed only in shuffle order were
considered equal, which resulted in no call to
Player.Listener.onTimelineChanged when calling
ExoPlayer.setShuffleOrder. This in turn resulted in no call to
MediaControllerCompat.Callback.onQueueChanged.

Also make a small fix inside ExoPlayerImpl.setShuffleOrder, to ensure
that the new shuffle order is used when constructing the masked
timeline.

Issue: google/ExoPlayer#9889
#minor-release
PiperOrigin-RevId: 457703727
(cherry picked from commit 5c7ec13e)
parent f3a350d6
Inline Side-by-side
Showing with 175 additions and 13 deletions
library/common/build.gradle
...@@ -53,6 +53,7 @@ dependencies { ...@@ -53,6 +53,7 @@ dependencies {
testImplementation 'junit:junit:' + junitVersion testImplementation 'junit:junit:' + junitVersion
testImplementation 'com.google.truth:truth:' + truthVersion testImplementation 'com.google.truth:truth:' + truthVersion
testImplementation 'org.robolectric:robolectric:' + robolectricVersion testImplementation 'org.robolectric:robolectric:' + robolectricVersion
testImplementation project(modulePrefix + 'library-core')
testImplementation project(modulePrefix + 'testutils') testImplementation project(modulePrefix + 'testutils')
} }
......
library/common/src/main/java/com/google/android/exoplayer2/Timeline.java
...@@ -1340,6 +1340,27 @@ public abstract class Timeline implements Bundleable { ...@@ -1340,6 +1340,27 @@ public abstract class Timeline implements Bundleable {
return false; return false;
} }
} }
// Check shuffled order
int windowIndex = getFirstWindowIndex(/* shuffleModeEnabled= */ true);
if (windowIndex != other.getFirstWindowIndex(/* shuffleModeEnabled= */ true)) {
return false;
}
int lastWindowIndex = getLastWindowIndex(/* shuffleModeEnabled= */ true);
if (lastWindowIndex != other.getLastWindowIndex(/* shuffleModeEnabled= */ true)) {
return false;
}
while (windowIndex != lastWindowIndex) {
int nextWindowIndex =
getNextWindowIndex(windowIndex, Player.REPEAT_MODE_OFF, /* shuffleModeEnabled= */ true);
if (nextWindowIndex
!= other.getNextWindowIndex(
windowIndex, Player.REPEAT_MODE_OFF, /* shuffleModeEnabled= */ true)) {
return false;
}
windowIndex = nextWindowIndex;
}
return true; return true;
} }
...@@ -1356,6 +1377,13 @@ public abstract class Timeline implements Bundleable { ...@@ -1356,6 +1377,13 @@ public abstract class Timeline implements Bundleable {
for (int i = 0; i < getPeriodCount(); i++) { for (int i = 0; i < getPeriodCount(); i++) {
result = 31 * result + getPeriod(i, period, /* setIds= */ true).hashCode(); result = 31 * result + getPeriod(i, period, /* setIds= */ true).hashCode();
} }
for (int windowIndex = getFirstWindowIndex(true);
windowIndex != C.INDEX_UNSET;
windowIndex = getNextWindowIndex(windowIndex, Player.REPEAT_MODE_OFF, true)) {
result = 31 * result + windowIndex;
}
return result; return result;
} }
......
library/common/src/test/java/com/google/android/exoplayer2/TimelineTest.java
...@@ -20,6 +20,7 @@ import static com.google.common.truth.Truth.assertThat; ...@@ -20,6 +20,7 @@ import static com.google.common.truth.Truth.assertThat;
import androidx.annotation.Nullable; import androidx.annotation.Nullable;
import androidx.test.ext.junit.runners.AndroidJUnit4; import androidx.test.ext.junit.runners.AndroidJUnit4;
import com.google.android.exoplayer2.MediaItem.LiveConfiguration; import com.google.android.exoplayer2.MediaItem.LiveConfiguration;
import com.google.android.exoplayer2.source.ShuffleOrder.DefaultShuffleOrder;
import com.google.android.exoplayer2.source.ads.AdPlaybackState; import com.google.android.exoplayer2.source.ads.AdPlaybackState;
import com.google.android.exoplayer2.testutil.FakeTimeline; import com.google.android.exoplayer2.testutil.FakeTimeline;
import com.google.android.exoplayer2.testutil.FakeTimeline.TimelineWindowDefinition; import com.google.android.exoplayer2.testutil.FakeTimeline.TimelineWindowDefinition;
...@@ -66,6 +67,50 @@ public class TimelineTest { ...@@ -66,6 +67,50 @@ public class TimelineTest {
} }
@Test @Test
public void timelineEquals() {
ImmutableList<TimelineWindowDefinition> timelineWindowDefinitions =
ImmutableList.of(
new TimelineWindowDefinition(/* periodCount= */ 1, /* id= */ 111),
new TimelineWindowDefinition(/* periodCount= */ 2, /* id= */ 222),
new TimelineWindowDefinition(/* periodCount= */ 3, /* id= */ 333));
Timeline timeline1 =
new FakeTimeline(timelineWindowDefinitions.toArray(new TimelineWindowDefinition[0]));
Timeline timeline2 =
new FakeTimeline(timelineWindowDefinitions.toArray(new TimelineWindowDefinition[0]));
assertThat(timeline1).isEqualTo(timeline2);
assertThat(timeline1.hashCode()).isEqualTo(timeline2.hashCode());
}
@Test
public void timelineEquals_includesShuffleOrder() {
ImmutableList<TimelineWindowDefinition> timelineWindowDefinitions =
ImmutableList.of(
new TimelineWindowDefinition(/* periodCount= */ 1, /* id= */ 111),
new TimelineWindowDefinition(/* periodCount= */ 2, /* id= */ 222),
new TimelineWindowDefinition(/* periodCount= */ 3, /* id= */ 333));
Timeline timeline =
new FakeTimeline(
new Object[0],
new DefaultShuffleOrder(timelineWindowDefinitions.size(), /* randomSeed= */ 5),
timelineWindowDefinitions.toArray(new TimelineWindowDefinition[0]));
Timeline timelineWithEquivalentShuffleOrder =
new FakeTimeline(
new Object[0],
new DefaultShuffleOrder(timelineWindowDefinitions.size(), /* randomSeed= */ 5),
timelineWindowDefinitions.toArray(new TimelineWindowDefinition[0]));
Timeline timelineWithDifferentShuffleOrder =
new FakeTimeline(
new Object[0],
new DefaultShuffleOrder(timelineWindowDefinitions.size(), /* randomSeed= */ 3),
timelineWindowDefinitions.toArray(new TimelineWindowDefinition[0]));
assertThat(timeline).isEqualTo(timelineWithEquivalentShuffleOrder);
assertThat(timeline.hashCode()).isEqualTo(timelineWithEquivalentShuffleOrder.hashCode());
assertThat(timeline).isNotEqualTo(timelineWithDifferentShuffleOrder);
}
@Test
public void windowEquals() { public void windowEquals() {
MediaItem mediaItem = new MediaItem.Builder().setUri("uri").setTag(new Object()).build(); MediaItem mediaItem = new MediaItem.Builder().setUri("uri").setTag(new Object()).build();
Timeline.Window window = new Timeline.Window(); Timeline.Window window = new Timeline.Window();
......
library/core/src/main/java/com/google/android/exoplayer2/ExoPlayerImpl.java
...@@ -699,6 +699,7 @@ import java.util.concurrent.TimeoutException; ...@@ -699,6 +699,7 @@ import java.util.concurrent.TimeoutException;
@Override @Override
public void setShuffleOrder(ShuffleOrder shuffleOrder) { public void setShuffleOrder(ShuffleOrder shuffleOrder) {
verifyApplicationThread(); verifyApplicationThread();
this.shuffleOrder = shuffleOrder;
Timeline timeline = createMaskingTimeline(); Timeline timeline = createMaskingTimeline();
PlaybackInfo newPlaybackInfo = PlaybackInfo newPlaybackInfo =
maskTimelineAndPosition( maskTimelineAndPosition(
...@@ -707,7 +708,6 @@ import java.util.concurrent.TimeoutException; ...@@ -707,7 +708,6 @@ import java.util.concurrent.TimeoutException;
maskWindowPositionMsOrGetPeriodPositionUs( maskWindowPositionMsOrGetPeriodPositionUs(
timeline, getCurrentMediaItemIndex(), getCurrentPosition())); timeline, getCurrentMediaItemIndex(), getCurrentPosition()));
pendingOperationAcks++; pendingOperationAcks++;
this.shuffleOrder = shuffleOrder;
internalPlayer.setShuffleOrder(shuffleOrder); internalPlayer.setShuffleOrder(shuffleOrder);
updatePlaybackInfo( updatePlaybackInfo(
newPlaybackInfo, newPlaybackInfo,
......
library/core/src/test/java/com/google/android/exoplayer2/ExoPlayerTest.java
...@@ -113,6 +113,7 @@ import com.google.android.exoplayer2.source.MediaPeriod; ...@@ -113,6 +113,7 @@ import com.google.android.exoplayer2.source.MediaPeriod;
import com.google.android.exoplayer2.source.MediaSource; import com.google.android.exoplayer2.source.MediaSource;
import com.google.android.exoplayer2.source.MediaSource.MediaPeriodId; import com.google.android.exoplayer2.source.MediaSource.MediaPeriodId;
import com.google.android.exoplayer2.source.MediaSourceEventListener; import com.google.android.exoplayer2.source.MediaSourceEventListener;
import com.google.android.exoplayer2.source.ShuffleOrder;
import com.google.android.exoplayer2.source.SinglePeriodTimeline; import com.google.android.exoplayer2.source.SinglePeriodTimeline;
import com.google.android.exoplayer2.source.TrackGroup; import com.google.android.exoplayer2.source.TrackGroup;
import com.google.android.exoplayer2.source.TrackGroupArray; import com.google.android.exoplayer2.source.TrackGroupArray;
...@@ -6502,6 +6503,53 @@ public final class ExoPlayerTest { ...@@ -6502,6 +6503,53 @@ public final class ExoPlayerTest {
} }
@Test @Test
public void setShuffleOrder_notifiesTimelineChanged() throws Exception {
ExoPlayer player =
new TestExoPlayerBuilder(context)
.setClock(new FakeClock(/* isAutoAdvancing= */ true))
.build();
// No callback expected for this call, because the (empty) timeline doesn't change. We start
// with a deterministic shuffle order, to ensure when we call setShuffleOrder again below the
// order is definitely different (otherwise the test is flaky when the existing shuffle order
// matches the shuffle order passed in below).
player.setShuffleOrder(new FakeShuffleOrder(0));
player.setMediaSources(
ImmutableList.of(new FakeMediaSource(), new FakeMediaSource(), new FakeMediaSource()));
Player.Listener mockListener = mock(Player.Listener.class);
player.addListener(mockListener);
player.prepare();
TestPlayerRunHelper.playUntilPosition(player, /* mediaItemIndex= */ 0, /* positionMs= */ 5000);
player.play();
ShuffleOrder.DefaultShuffleOrder newShuffleOrder =
new ShuffleOrder.DefaultShuffleOrder(player.getMediaItemCount(), /* randomSeed= */ 5);
player.setShuffleOrder(newShuffleOrder);
TestPlayerRunHelper.runUntilPlaybackState(player, Player.STATE_ENDED);
player.release();
ArgumentCaptor<Timeline> timelineCaptor = ArgumentCaptor.forClass(Timeline.class);
verify(mockListener)
.onTimelineChanged(
timelineCaptor.capture(), eq(Player.TIMELINE_CHANGE_REASON_PLAYLIST_CHANGED));
Timeline capturedTimeline = Iterables.getOnlyElement(timelineCaptor.getAllValues());
List<Integer> newShuffleOrderIndexes = new ArrayList<>(newShuffleOrder.getLength());
for (int i = newShuffleOrder.getFirstIndex();
i != C.INDEX_UNSET;
i = newShuffleOrder.getNextIndex(i)) {
newShuffleOrderIndexes.add(i);
}
List<Integer> capturedTimelineShuffleIndexes = new ArrayList<>(newShuffleOrder.getLength());
for (int i = capturedTimeline.getFirstWindowIndex(/* shuffleModeEnabled= */ true);
i != C.INDEX_UNSET;
i =
capturedTimeline.getNextWindowIndex(
i, Player.REPEAT_MODE_OFF, /* shuffleModeEnabled= */ true)) {
capturedTimelineShuffleIndexes.add(i);
}
assertThat(capturedTimelineShuffleIndexes).isEqualTo(newShuffleOrderIndexes);
}
@Test
public void setMediaSources_empty_whenEmpty_correctMaskingMediaItemIndex() throws Exception { public void setMediaSources_empty_whenEmpty_correctMaskingMediaItemIndex() throws Exception {
final int[] currentMediaItemIndices = {C.INDEX_UNSET, C.INDEX_UNSET, C.INDEX_UNSET}; final int[] currentMediaItemIndices = {C.INDEX_UNSET, C.INDEX_UNSET, C.INDEX_UNSET};
ActionSchedule actionSchedule = ActionSchedule actionSchedule =
......
testutils/src/main/java/com/google/android/exoplayer2/testutil/FakeTimeline.java
...@@ -25,6 +25,7 @@ import com.google.android.exoplayer2.C; ...@@ -25,6 +25,7 @@ import com.google.android.exoplayer2.C;
import com.google.android.exoplayer2.MediaItem; import com.google.android.exoplayer2.MediaItem;
import com.google.android.exoplayer2.Player; import com.google.android.exoplayer2.Player;
import com.google.android.exoplayer2.Timeline; import com.google.android.exoplayer2.Timeline;
import com.google.android.exoplayer2.source.ShuffleOrder;
import com.google.android.exoplayer2.source.ads.AdPlaybackState; import com.google.android.exoplayer2.source.ads.AdPlaybackState;
import com.google.android.exoplayer2.util.Assertions; import com.google.android.exoplayer2.util.Assertions;
import com.google.android.exoplayer2.util.Util; import com.google.android.exoplayer2.util.Util;
...@@ -273,7 +274,7 @@ public final class FakeTimeline extends Timeline { ...@@ -273,7 +274,7 @@ public final class FakeTimeline extends Timeline {
private final TimelineWindowDefinition[] windowDefinitions; private final TimelineWindowDefinition[] windowDefinitions;
private final Object[] manifests; private final Object[] manifests;
private final int[] periodOffsets; private final int[] periodOffsets;
private final FakeShuffleOrder fakeShuffleOrder; private final ShuffleOrder shuffleOrder;
/** /**
* Returns an ad playback state with the specified number of ads in each of the specified ad * Returns an ad playback state with the specified number of ads in each of the specified ad
...@@ -393,6 +394,19 @@ public final class FakeTimeline extends Timeline { ...@@ -393,6 +394,19 @@ public final class FakeTimeline extends Timeline {
* @param windowDefinitions A list of {@link TimelineWindowDefinition}s. * @param windowDefinitions A list of {@link TimelineWindowDefinition}s.
*/ */
public FakeTimeline(Object[] manifests, TimelineWindowDefinition... windowDefinitions) { public FakeTimeline(Object[] manifests, TimelineWindowDefinition... windowDefinitions) {
this(manifests, new FakeShuffleOrder(windowDefinitions.length), windowDefinitions);
}
/**
* Creates a fake timeline with the given window definitions and {@link
* com.google.android.exoplayer2.source.ShuffleOrder}.
*
* @param windowDefinitions A list of {@link TimelineWindowDefinition}s.
*/
public FakeTimeline(
Object[] manifests,
ShuffleOrder shuffleOrder,
TimelineWindowDefinition... windowDefinitions) {
this.manifests = new Object[windowDefinitions.length]; this.manifests = new Object[windowDefinitions.length];
System.arraycopy(manifests, 0, this.manifests, 0, min(this.manifests.length, manifests.length)); System.arraycopy(manifests, 0, this.manifests, 0, min(this.manifests.length, manifests.length));
this.windowDefinitions = windowDefinitions; this.windowDefinitions = windowDefinitions;
...@@ -401,7 +415,7 @@ public final class FakeTimeline extends Timeline { ...@@ -401,7 +415,7 @@ public final class FakeTimeline extends Timeline {
for (int i = 0; i < windowDefinitions.length; i++) { for (int i = 0; i < windowDefinitions.length; i++) {
periodOffsets[i + 1] = periodOffsets[i] + windowDefinitions[i].periodCount; periodOffsets[i + 1] = periodOffsets[i] + windowDefinitions[i].periodCount;
} }
fakeShuffleOrder = new FakeShuffleOrder(windowDefinitions.length); this.shuffleOrder = shuffleOrder;
} }
@Override @Override
...@@ -420,7 +434,7 @@ public final class FakeTimeline extends Timeline { ...@@ -420,7 +434,7 @@ public final class FakeTimeline extends Timeline {
? getFirstWindowIndex(shuffleModeEnabled) ? getFirstWindowIndex(shuffleModeEnabled)
: C.INDEX_UNSET; : C.INDEX_UNSET;
} }
return shuffleModeEnabled ? fakeShuffleOrder.getNextIndex(windowIndex) : windowIndex + 1; return shuffleModeEnabled ? shuffleOrder.getNextIndex(windowIndex) : windowIndex + 1;
} }
@Override @Override
...@@ -434,20 +448,20 @@ public final class FakeTimeline extends Timeline { ...@@ -434,20 +448,20 @@ public final class FakeTimeline extends Timeline {
? getLastWindowIndex(shuffleModeEnabled) ? getLastWindowIndex(shuffleModeEnabled)
: C.INDEX_UNSET; : C.INDEX_UNSET;
} }
return shuffleModeEnabled ? fakeShuffleOrder.getPreviousIndex(windowIndex) : windowIndex - 1; return shuffleModeEnabled ? shuffleOrder.getPreviousIndex(windowIndex) : windowIndex - 1;
} }
@Override @Override
public int getLastWindowIndex(boolean shuffleModeEnabled) { public int getLastWindowIndex(boolean shuffleModeEnabled) {
return shuffleModeEnabled return shuffleModeEnabled
? fakeShuffleOrder.getLastIndex() ? shuffleOrder.getLastIndex()
: super.getLastWindowIndex(/* shuffleModeEnabled= */ false); : super.getLastWindowIndex(/* shuffleModeEnabled= */ false);
} }
@Override @Override
public int getFirstWindowIndex(boolean shuffleModeEnabled) { public int getFirstWindowIndex(boolean shuffleModeEnabled) {
return shuffleModeEnabled return shuffleModeEnabled
? fakeShuffleOrder.getFirstIndex() ? shuffleOrder.getFirstIndex()
: super.getFirstWindowIndex(/* shuffleModeEnabled= */ false); : super.getFirstWindowIndex(/* shuffleModeEnabled= */ false);
} }
......
testutils/src/main/java/com/google/android/exoplayer2/testutil/TestUtil.java
...@@ -198,8 +198,12 @@ public class TestUtil { ...@@ -198,8 +198,12 @@ public class TestUtil {
/** /**
* Asserts that the actual timelines are the same to the expected timelines. This assert differs * Asserts that the actual timelines are the same to the expected timelines. This assert differs
* from testing equality by not comparing period ids which may be different due to id mapping of * from testing equality by not comparing:
* child source period ids. *
* <ul>
* <li>Period IDs, which may be different due to ID mapping of child source period IDs.
* <li>Shuffle order, which by default is random and non-deterministic.
* </ul>
* *
* @param actualTimelines A list of actual {@link Timeline timelines}. * @param actualTimelines A list of actual {@link Timeline timelines}.
* @param expectedTimelines A list of expected {@link Timeline timelines}. * @param expectedTimelines A list of expected {@link Timeline timelines}.
...@@ -216,10 +220,11 @@ public class TestUtil { ...@@ -216,10 +220,11 @@ public class TestUtil {
/** /**
* Returns true if {@code thisTimeline} is equal to {@code thatTimeline}, ignoring {@link * Returns true if {@code thisTimeline} is equal to {@code thatTimeline}, ignoring {@link
* Timeline.Window#uid} and {@link Timeline.Period#uid} values. * Timeline.Window#uid} and {@link Timeline.Period#uid} values, and shuffle order.
*/ */
public static boolean timelinesAreSame(Timeline thisTimeline, Timeline thatTimeline) { public static boolean timelinesAreSame(Timeline thisTimeline, Timeline thatTimeline) {
return new NoUidTimeline(thisTimeline).equals(new NoUidTimeline(thatTimeline)); return new NoUidOrShufflingTimeline(thisTimeline)
.equals(new NoUidOrShufflingTimeline(thatTimeline));
} }
/** /**
...@@ -503,11 +508,11 @@ public class TestUtil { ...@@ -503,11 +508,11 @@ public class TestUtil {
return list; return list;
} }
private static final class NoUidTimeline extends Timeline { private static final class NoUidOrShufflingTimeline extends Timeline {
private final Timeline delegate; private final Timeline delegate;
public NoUidTimeline(Timeline timeline) { public NoUidOrShufflingTimeline(Timeline timeline) {
this.delegate = timeline; this.delegate = timeline;
} }
...@@ -517,6 +522,27 @@ public class TestUtil { ...@@ -517,6 +522,27 @@ public class TestUtil {
} }
@Override @Override
public int getNextWindowIndex(int windowIndex, int repeatMode, boolean shuffleModeEnabled) {
return delegate.getNextWindowIndex(windowIndex, repeatMode, /* shuffleModeEnabled= */ false);
}
@Override
public int getPreviousWindowIndex(int windowIndex, int repeatMode, boolean shuffleModeEnabled) {
return delegate.getPreviousWindowIndex(
windowIndex, repeatMode, /* shuffleModeEnabled= */ false);
}
@Override
public int getLastWindowIndex(boolean shuffleModeEnabled) {
return delegate.getLastWindowIndex(/* shuffleModeEnabled= */ false);
}
@Override
public int getFirstWindowIndex(boolean shuffleModeEnabled) {
return delegate.getFirstWindowIndex(/* shuffleModeEnabled= */ false);
}
@Override
public Window getWindow(int windowIndex, Window window, long defaultPositionProjectionUs) { public Window getWindow(int windowIndex, Window window, long defaultPositionProjectionUs) {
delegate.getWindow(windowIndex, window, defaultPositionProjectionUs); delegate.getWindow(windowIndex, window, defaultPositionProjectionUs);
window.uid = 0; window.uid = 0;
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or sign in to comment