From d3b5f71f25a3e8a78656f652a5b250b3215bf26e Mon Sep 17 00:00:00 2001 From: ibaker Date: Tue, 28 Jun 2022 12:15:54 +0000 Subject: [PATCH] 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 5c7ec13e85d32bdb464082069d462c740bc7cd55) --- library/common/build.gradle | 1 + .../google/android/exoplayer2/Timeline.java | 28 +++++++++++ .../android/exoplayer2/TimelineTest.java | 45 +++++++++++++++++ .../android/exoplayer2/ExoPlayerImpl.java | 2 +- .../android/exoplayer2/ExoPlayerTest.java | 48 +++++++++++++++++++ .../exoplayer2/testutil/FakeTimeline.java | 26 +++++++--- .../android/exoplayer2/testutil/TestUtil.java | 38 ++++++++++++--- 7 files changed, 175 insertions(+), 13 deletions(-) diff --git a/library/common/build.gradle b/library/common/build.gradle index e0eebdba49..dfe6ab9596 100644 --- a/library/common/build.gradle +++ b/library/common/build.gradle @@ -53,6 +53,7 @@ dependencies { testImplementation 'junit:junit:' + junitVersion testImplementation 'com.google.truth:truth:' + truthVersion testImplementation 'org.robolectric:robolectric:' + robolectricVersion + testImplementation project(modulePrefix + 'library-core') testImplementation project(modulePrefix + 'testutils') } diff --git a/library/common/src/main/java/com/google/android/exoplayer2/Timeline.java b/library/common/src/main/java/com/google/android/exoplayer2/Timeline.java index 91c2f95241..14bb2c387d 100644 --- a/library/common/src/main/java/com/google/android/exoplayer2/Timeline.java +++ b/library/common/src/main/java/com/google/android/exoplayer2/Timeline.java @@ -1340,6 +1340,27 @@ public abstract class Timeline implements Bundleable { 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; } @@ -1356,6 +1377,13 @@ public abstract class Timeline implements Bundleable { for (int i = 0; i < getPeriodCount(); i++) { 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; } diff --git a/library/common/src/test/java/com/google/android/exoplayer2/TimelineTest.java b/library/common/src/test/java/com/google/android/exoplayer2/TimelineTest.java index 8567b919f2..3e5bc2f75c 100644 --- a/library/common/src/test/java/com/google/android/exoplayer2/TimelineTest.java +++ b/library/common/src/test/java/com/google/android/exoplayer2/TimelineTest.java @@ -20,6 +20,7 @@ import static com.google.common.truth.Truth.assertThat; import androidx.annotation.Nullable; import androidx.test.ext.junit.runners.AndroidJUnit4; 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.testutil.FakeTimeline; import com.google.android.exoplayer2.testutil.FakeTimeline.TimelineWindowDefinition; @@ -65,6 +66,50 @@ public class TimelineTest { TimelineAsserts.assertNextWindowIndices(timeline, Player.REPEAT_MODE_ALL, false, 0); } + @Test + public void timelineEquals() { + ImmutableList 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 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() { MediaItem mediaItem = new MediaItem.Builder().setUri("uri").setTag(new Object()).build(); diff --git a/library/core/src/main/java/com/google/android/exoplayer2/ExoPlayerImpl.java b/library/core/src/main/java/com/google/android/exoplayer2/ExoPlayerImpl.java index 603a0660ac..9e64bea0c6 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/ExoPlayerImpl.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/ExoPlayerImpl.java @@ -699,6 +699,7 @@ import java.util.concurrent.TimeoutException; @Override public void setShuffleOrder(ShuffleOrder shuffleOrder) { verifyApplicationThread(); + this.shuffleOrder = shuffleOrder; Timeline timeline = createMaskingTimeline(); PlaybackInfo newPlaybackInfo = maskTimelineAndPosition( @@ -707,7 +708,6 @@ import java.util.concurrent.TimeoutException; maskWindowPositionMsOrGetPeriodPositionUs( timeline, getCurrentMediaItemIndex(), getCurrentPosition())); pendingOperationAcks++; - this.shuffleOrder = shuffleOrder; internalPlayer.setShuffleOrder(shuffleOrder); updatePlaybackInfo( newPlaybackInfo, diff --git a/library/core/src/test/java/com/google/android/exoplayer2/ExoPlayerTest.java b/library/core/src/test/java/com/google/android/exoplayer2/ExoPlayerTest.java index ca1bd604a7..3b66b47802 100644 --- a/library/core/src/test/java/com/google/android/exoplayer2/ExoPlayerTest.java +++ b/library/core/src/test/java/com/google/android/exoplayer2/ExoPlayerTest.java @@ -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.MediaPeriodId; 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.TrackGroup; import com.google.android.exoplayer2.source.TrackGroupArray; @@ -6501,6 +6502,53 @@ public final class ExoPlayerTest { assertThat(positionAfterSetShuffleOrder.get()).isAtLeast(5000); } + @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 timelineCaptor = ArgumentCaptor.forClass(Timeline.class); + verify(mockListener) + .onTimelineChanged( + timelineCaptor.capture(), eq(Player.TIMELINE_CHANGE_REASON_PLAYLIST_CHANGED)); + + Timeline capturedTimeline = Iterables.getOnlyElement(timelineCaptor.getAllValues()); + List newShuffleOrderIndexes = new ArrayList<>(newShuffleOrder.getLength()); + for (int i = newShuffleOrder.getFirstIndex(); + i != C.INDEX_UNSET; + i = newShuffleOrder.getNextIndex(i)) { + newShuffleOrderIndexes.add(i); + } + List 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 { final int[] currentMediaItemIndices = {C.INDEX_UNSET, C.INDEX_UNSET, C.INDEX_UNSET}; diff --git a/testutils/src/main/java/com/google/android/exoplayer2/testutil/FakeTimeline.java b/testutils/src/main/java/com/google/android/exoplayer2/testutil/FakeTimeline.java index 6e14864da1..8eb6cca922 100644 --- a/testutils/src/main/java/com/google/android/exoplayer2/testutil/FakeTimeline.java +++ b/testutils/src/main/java/com/google/android/exoplayer2/testutil/FakeTimeline.java @@ -25,6 +25,7 @@ import com.google.android.exoplayer2.C; import com.google.android.exoplayer2.MediaItem; import com.google.android.exoplayer2.Player; 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.util.Assertions; import com.google.android.exoplayer2.util.Util; @@ -273,7 +274,7 @@ public final class FakeTimeline extends Timeline { private final TimelineWindowDefinition[] windowDefinitions; private final Object[] manifests; 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 @@ -393,6 +394,19 @@ public final class FakeTimeline extends Timeline { * @param windowDefinitions A list of {@link TimelineWindowDefinition}s. */ 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]; System.arraycopy(manifests, 0, this.manifests, 0, min(this.manifests.length, manifests.length)); this.windowDefinitions = windowDefinitions; @@ -401,7 +415,7 @@ public final class FakeTimeline extends Timeline { for (int i = 0; i < windowDefinitions.length; i++) { periodOffsets[i + 1] = periodOffsets[i] + windowDefinitions[i].periodCount; } - fakeShuffleOrder = new FakeShuffleOrder(windowDefinitions.length); + this.shuffleOrder = shuffleOrder; } @Override @@ -420,7 +434,7 @@ public final class FakeTimeline extends Timeline { ? getFirstWindowIndex(shuffleModeEnabled) : C.INDEX_UNSET; } - return shuffleModeEnabled ? fakeShuffleOrder.getNextIndex(windowIndex) : windowIndex + 1; + return shuffleModeEnabled ? shuffleOrder.getNextIndex(windowIndex) : windowIndex + 1; } @Override @@ -434,20 +448,20 @@ public final class FakeTimeline extends Timeline { ? getLastWindowIndex(shuffleModeEnabled) : C.INDEX_UNSET; } - return shuffleModeEnabled ? fakeShuffleOrder.getPreviousIndex(windowIndex) : windowIndex - 1; + return shuffleModeEnabled ? shuffleOrder.getPreviousIndex(windowIndex) : windowIndex - 1; } @Override public int getLastWindowIndex(boolean shuffleModeEnabled) { return shuffleModeEnabled - ? fakeShuffleOrder.getLastIndex() + ? shuffleOrder.getLastIndex() : super.getLastWindowIndex(/* shuffleModeEnabled= */ false); } @Override public int getFirstWindowIndex(boolean shuffleModeEnabled) { return shuffleModeEnabled - ? fakeShuffleOrder.getFirstIndex() + ? shuffleOrder.getFirstIndex() : super.getFirstWindowIndex(/* shuffleModeEnabled= */ false); } diff --git a/testutils/src/main/java/com/google/android/exoplayer2/testutil/TestUtil.java b/testutils/src/main/java/com/google/android/exoplayer2/testutil/TestUtil.java index ea9b018da0..18bed1670a 100644 --- a/testutils/src/main/java/com/google/android/exoplayer2/testutil/TestUtil.java +++ b/testutils/src/main/java/com/google/android/exoplayer2/testutil/TestUtil.java @@ -198,8 +198,12 @@ public class TestUtil { /** * 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 - * child source period ids. + * from testing equality by not comparing: + * + *
    + *
  • Period IDs, which may be different due to ID mapping of child source period IDs. + *
  • Shuffle order, which by default is random and non-deterministic. + *
* * @param actualTimelines A list of actual {@link Timeline timelines}. * @param expectedTimelines A list of expected {@link Timeline timelines}. @@ -216,10 +220,11 @@ public class TestUtil { /** * 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) { - return new NoUidTimeline(thisTimeline).equals(new NoUidTimeline(thatTimeline)); + return new NoUidOrShufflingTimeline(thisTimeline) + .equals(new NoUidOrShufflingTimeline(thatTimeline)); } /** @@ -503,11 +508,11 @@ public class TestUtil { return list; } - private static final class NoUidTimeline extends Timeline { + private static final class NoUidOrShufflingTimeline extends Timeline { private final Timeline delegate; - public NoUidTimeline(Timeline timeline) { + public NoUidOrShufflingTimeline(Timeline timeline) { this.delegate = timeline; } @@ -516,6 +521,27 @@ public class TestUtil { return delegate.getWindowCount(); } + @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) { delegate.getWindow(windowIndex, window, defaultPositionProjectionUs);