From 219967c5a3ff8d8957a07e39c04ea27b657cab92 Mon Sep 17 00:00:00 2001 From: tonihei Date: Wed, 5 Apr 2023 10:26:31 +0100 Subject: [PATCH] Allow duplicated MediaItems in a legacy session MediaItems are not meant to be unique in a playlist. If a legacy session publishes multiple items that get converted to equal MediaItems, the current code fails because we look up queue ids in a Map (that doesn't allow duplicate entries). Fix this by storing a simple list of items with additional data. #minor-release Issue: androidx/media#290 PiperOrigin-RevId: 521993802 --- RELEASENOTES.md | 4 + .../session/MediaControllerImplLegacy.java | 12 +- .../media3/session/QueueTimeline.java | 166 +++++++++--------- ...aControllerWithMediaSessionCompatTest.java | 36 ++++ 4 files changed, 132 insertions(+), 86 deletions(-) diff --git a/RELEASENOTES.md b/RELEASENOTES.md index 6e81af2448..b2b0eb80ad 100644 --- a/RELEASENOTES.md +++ b/RELEASENOTES.md @@ -28,6 +28,10 @@ instead. Note that even for the deprecated variants, the offset is not anymore added to `startTimeUs` and `endTimeUs` of the `MediaLoadData` objects that are dispatched by the dispatcher. +* Session: + * Fix bug where multiple identical queue items published by a legacy + `MediaSessionCompat` result in an exception in `MediaController` + ([#290](https://github.com/androidx/media/issues/290)). * Audio: * Fix bug where some playbacks fail when tunneling is enabled and `AudioProcessors` are active, e.g. for gapless trimming diff --git a/libraries/session/src/main/java/androidx/media3/session/MediaControllerImplLegacy.java b/libraries/session/src/main/java/androidx/media3/session/MediaControllerImplLegacy.java index 1b39a04b85..6dff57d43c 100644 --- a/libraries/session/src/main/java/androidx/media3/session/MediaControllerImplLegacy.java +++ b/libraries/session/src/main/java/androidx/media3/session/MediaControllerImplLegacy.java @@ -1796,7 +1796,7 @@ import org.checkerframework.checker.nullness.compatqual.NullableType; currentTimeline = isQueueChanged ? QueueTimeline.create(newLegacyPlayerInfo.queue) - : new QueueTimeline((QueueTimeline) oldControllerInfo.playerInfo.timeline); + : ((QueueTimeline) oldControllerInfo.playerInfo.timeline).copy(); boolean isMetadataCompatChanged = oldLegacyPlayerInfo.mediaMetadataCompat != newLegacyPlayerInfo.mediaMetadataCompat @@ -1988,8 +1988,6 @@ import org.checkerframework.checker.nullness.compatqual.NullableType; Integer mediaItemTransitionReason; boolean isOldTimelineEmpty = oldControllerInfo.playerInfo.timeline.isEmpty(); boolean isNewTimelineEmpty = newControllerInfo.playerInfo.timeline.isEmpty(); - int newCurrentMediaItemIndex = - newControllerInfo.playerInfo.sessionPositionInfo.positionInfo.mediaItemIndex; if (isOldTimelineEmpty && isNewTimelineEmpty) { // Still empty Timelines. discontinuityReason = null; @@ -2001,13 +1999,13 @@ import org.checkerframework.checker.nullness.compatqual.NullableType; } else { MediaItem oldCurrentMediaItem = checkStateNotNull(oldControllerInfo.playerInfo.getCurrentMediaItem()); - int oldCurrentMediaItemIndexInNewTimeline = - ((QueueTimeline) newControllerInfo.playerInfo.timeline).indexOf(oldCurrentMediaItem); - if (oldCurrentMediaItemIndexInNewTimeline == C.INDEX_UNSET) { + boolean oldCurrentMediaItemExistsInNewTimeline = + ((QueueTimeline) newControllerInfo.playerInfo.timeline).contains(oldCurrentMediaItem); + if (!oldCurrentMediaItemExistsInNewTimeline) { // Old current item is removed. discontinuityReason = Player.DISCONTINUITY_REASON_REMOVE; mediaItemTransitionReason = Player.MEDIA_ITEM_TRANSITION_REASON_PLAYLIST_CHANGED; - } else if (oldCurrentMediaItemIndexInNewTimeline == newCurrentMediaItemIndex) { + } else if (oldCurrentMediaItem.equals(newControllerInfo.playerInfo.getCurrentMediaItem())) { // Current item is the same. long oldCurrentPosition = MediaUtils.convertToCurrentPositionMs( diff --git a/libraries/session/src/main/java/androidx/media3/session/QueueTimeline.java b/libraries/session/src/main/java/androidx/media3/session/QueueTimeline.java index a7dc94c511..2901789217 100644 --- a/libraries/session/src/main/java/androidx/media3/session/QueueTimeline.java +++ b/libraries/session/src/main/java/androidx/media3/session/QueueTimeline.java @@ -16,7 +16,6 @@ package androidx.media3.session; import static androidx.media3.common.util.Assertions.checkArgument; -import static androidx.media3.common.util.Assertions.checkNotNull; import android.support.v4.media.MediaMetadataCompat; import android.support.v4.media.session.MediaSessionCompat.QueueItem; @@ -27,11 +26,8 @@ import androidx.media3.common.Timeline; import androidx.media3.common.util.Util; import com.google.common.base.Objects; import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableMap; import java.util.ArrayList; -import java.util.HashMap; import java.util.List; -import java.util.Map; /** * An immutable class to represent the current {@link Timeline} backed by {@linkplain QueueItem @@ -45,42 +41,33 @@ import java.util.Map; /* package */ final class QueueTimeline extends Timeline { public static final QueueTimeline DEFAULT = - new QueueTimeline(ImmutableList.of(), ImmutableMap.of(), /* fakeMediaItem= */ null); + new QueueTimeline(ImmutableList.of(), /* fakeMediaItem= */ null); private static final Object FAKE_WINDOW_UID = new Object(); - private final ImmutableList mediaItems; - private final ImmutableMap mediaItemToQueueIdMap; + private final ImmutableList queuedMediaItems; @Nullable private final MediaItem fakeMediaItem; - /** Creates a new instance. */ - public QueueTimeline(QueueTimeline queueTimeline) { - this.mediaItems = queueTimeline.mediaItems; - this.mediaItemToQueueIdMap = queueTimeline.mediaItemToQueueIdMap; - this.fakeMediaItem = queueTimeline.fakeMediaItem; - } - private QueueTimeline( - ImmutableList mediaItems, - ImmutableMap mediaItemToQueueIdMap, - @Nullable MediaItem fakeMediaItem) { - this.mediaItems = mediaItems; - this.mediaItemToQueueIdMap = mediaItemToQueueIdMap; + ImmutableList queuedMediaItems, @Nullable MediaItem fakeMediaItem) { + this.queuedMediaItems = queuedMediaItems; this.fakeMediaItem = fakeMediaItem; } /** Creates a {@link QueueTimeline} from a list of {@linkplain QueueItem queue items}. */ public static QueueTimeline create(List queue) { - ImmutableList.Builder mediaItemsBuilder = new ImmutableList.Builder<>(); - ImmutableMap.Builder mediaItemToQueueIdMap = new ImmutableMap.Builder<>(); + ImmutableList.Builder queuedMediaItemsBuilder = new ImmutableList.Builder<>(); for (int i = 0; i < queue.size(); i++) { QueueItem queueItem = queue.get(i); MediaItem mediaItem = MediaUtils.convertToMediaItem(queueItem); - mediaItemsBuilder.add(mediaItem); - mediaItemToQueueIdMap.put(mediaItem, queueItem.getQueueId()); + queuedMediaItemsBuilder.add(new QueuedMediaItem(mediaItem, queueItem.getQueueId())); } - return new QueueTimeline( - mediaItemsBuilder.build(), mediaItemToQueueIdMap.buildOrThrow(), /* fakeMediaItem= */ null); + return new QueueTimeline(queuedMediaItemsBuilder.build(), /* fakeMediaItem= */ null); + } + + /** Returns a copy of the current queue timeline. */ + public QueueTimeline copy() { + return new QueueTimeline(queuedMediaItems, fakeMediaItem); } /** @@ -91,9 +78,9 @@ import java.util.Map; * @return The corresponding queue ID or {@link QueueItem#UNKNOWN_ID} if not known. */ public long getQueueId(int mediaItemIndex) { - MediaItem mediaItem = getMediaItemAt(mediaItemIndex); - @Nullable Long queueId = mediaItemToQueueIdMap.get(mediaItem); - return queueId == null ? QueueItem.UNKNOWN_ID : queueId; + return mediaItemIndex >= 0 && mediaItemIndex < queuedMediaItems.size() + ? queuedMediaItems.get(mediaItemIndex).queueId + : QueueItem.UNKNOWN_ID; } /** @@ -103,7 +90,7 @@ import java.util.Map; * @return A new {@link QueueTimeline} reflecting the update. */ public QueueTimeline copyWithFakeMediaItem(@Nullable MediaItem fakeMediaItem) { - return new QueueTimeline(mediaItems, mediaItemToQueueIdMap, fakeMediaItem); + return new QueueTimeline(queuedMediaItems, fakeMediaItem); } /** @@ -115,23 +102,17 @@ import java.util.Map; */ public QueueTimeline copyWithNewMediaItem(int replaceIndex, MediaItem newMediaItem) { checkArgument( - replaceIndex < mediaItems.size() - || (replaceIndex == mediaItems.size() && fakeMediaItem != null)); - if (replaceIndex == mediaItems.size()) { - return new QueueTimeline(mediaItems, mediaItemToQueueIdMap, newMediaItem); + replaceIndex < queuedMediaItems.size() + || (replaceIndex == queuedMediaItems.size() && fakeMediaItem != null)); + if (replaceIndex == queuedMediaItems.size()) { + return new QueueTimeline(queuedMediaItems, newMediaItem); } - MediaItem oldMediaItem = mediaItems.get(replaceIndex); - // Create the new play list. - ImmutableList.Builder newMediaItemsBuilder = new ImmutableList.Builder<>(); - newMediaItemsBuilder.addAll(mediaItems.subList(0, replaceIndex)); - newMediaItemsBuilder.add(newMediaItem); - newMediaItemsBuilder.addAll(mediaItems.subList(replaceIndex + 1, mediaItems.size())); - // Update the map of items to queue IDs accordingly. - Map newMediaItemToQueueIdMap = new HashMap<>(mediaItemToQueueIdMap); - Long queueId = checkNotNull(newMediaItemToQueueIdMap.remove(oldMediaItem)); - newMediaItemToQueueIdMap.put(newMediaItem, queueId); - return new QueueTimeline( - newMediaItemsBuilder.build(), ImmutableMap.copyOf(newMediaItemToQueueIdMap), fakeMediaItem); + long queueId = queuedMediaItems.get(replaceIndex).queueId; + ImmutableList.Builder queuedItemsBuilder = new ImmutableList.Builder<>(); + queuedItemsBuilder.addAll(queuedMediaItems.subList(0, replaceIndex)); + queuedItemsBuilder.add(new QueuedMediaItem(newMediaItem, queueId)); + queuedItemsBuilder.addAll(queuedMediaItems.subList(replaceIndex + 1, queuedMediaItems.size())); + return new QueueTimeline(queuedItemsBuilder.build(), fakeMediaItem); } /** @@ -143,11 +124,13 @@ import java.util.Map; * @return A new {@link QueueTimeline} reflecting the update. */ public QueueTimeline copyWithNewMediaItems(int index, List newMediaItems) { - ImmutableList.Builder newMediaItemsBuilder = new ImmutableList.Builder<>(); - newMediaItemsBuilder.addAll(mediaItems.subList(0, index)); - newMediaItemsBuilder.addAll(newMediaItems); - newMediaItemsBuilder.addAll(mediaItems.subList(index, mediaItems.size())); - return new QueueTimeline(newMediaItemsBuilder.build(), mediaItemToQueueIdMap, fakeMediaItem); + ImmutableList.Builder queuedItemsBuilder = new ImmutableList.Builder<>(); + queuedItemsBuilder.addAll(queuedMediaItems.subList(0, index)); + for (int i = 0; i < newMediaItems.size(); i++) { + queuedItemsBuilder.add(new QueuedMediaItem(newMediaItems.get(i), QueueItem.UNKNOWN_ID)); + } + queuedItemsBuilder.addAll(queuedMediaItems.subList(index, queuedMediaItems.size())); + return new QueueTimeline(queuedItemsBuilder.build(), fakeMediaItem); } /** @@ -158,10 +141,10 @@ import java.util.Map; * @return A new {@link QueueTimeline} reflecting the update. */ public QueueTimeline copyWithRemovedMediaItems(int fromIndex, int toIndex) { - ImmutableList.Builder newMediaItemsBuilder = new ImmutableList.Builder<>(); - newMediaItemsBuilder.addAll(mediaItems.subList(0, fromIndex)); - newMediaItemsBuilder.addAll(mediaItems.subList(toIndex, mediaItems.size())); - return new QueueTimeline(newMediaItemsBuilder.build(), mediaItemToQueueIdMap, fakeMediaItem); + ImmutableList.Builder queuedItemsBuilder = new ImmutableList.Builder<>(); + queuedItemsBuilder.addAll(queuedMediaItems.subList(0, fromIndex)); + queuedItemsBuilder.addAll(queuedMediaItems.subList(toIndex, queuedMediaItems.size())); + return new QueueTimeline(queuedItemsBuilder.build(), fakeMediaItem); } /** @@ -173,50 +156,45 @@ import java.util.Map; * @return A new {@link QueueTimeline} reflecting the update. */ public QueueTimeline copyWithMovedMediaItems(int fromIndex, int toIndex, int newIndex) { - List list = new ArrayList<>(mediaItems); + List list = new ArrayList<>(queuedMediaItems); Util.moveItems(list, fromIndex, toIndex, newIndex); - return new QueueTimeline( - new ImmutableList.Builder().addAll(list).build(), - mediaItemToQueueIdMap, - fakeMediaItem); + return new QueueTimeline(ImmutableList.copyOf(list), fakeMediaItem); } - /** - * Returns the media item index of the given media item in the timeline, or {@link C#INDEX_UNSET} - * if the item is not part of this timeline. - * - * @param mediaItem The media item of interest. - * @return The index of the item or {@link C#INDEX_UNSET} if the item is not part of the timeline. - */ - public int indexOf(MediaItem mediaItem) { + /** Returns whether the timeline contains the given {@link MediaItem}. */ + public boolean contains(MediaItem mediaItem) { if (mediaItem.equals(fakeMediaItem)) { - return mediaItems.size(); + return true; } - int mediaItemIndex = mediaItems.indexOf(mediaItem); - return mediaItemIndex == -1 ? C.INDEX_UNSET : mediaItemIndex; + for (int i = 0; i < queuedMediaItems.size(); i++) { + if (mediaItem.equals(queuedMediaItems.get(i).mediaItem)) { + return true; + } + } + return false; } @Nullable public MediaItem getMediaItemAt(int mediaItemIndex) { - if (mediaItemIndex >= 0 && mediaItemIndex < mediaItems.size()) { - return mediaItems.get(mediaItemIndex); + if (mediaItemIndex >= 0 && mediaItemIndex < queuedMediaItems.size()) { + return queuedMediaItems.get(mediaItemIndex).mediaItem; } - return (mediaItemIndex == mediaItems.size()) ? fakeMediaItem : null; + return (mediaItemIndex == queuedMediaItems.size()) ? fakeMediaItem : null; } @Override public int getWindowCount() { - return mediaItems.size() + ((fakeMediaItem == null) ? 0 : 1); + return queuedMediaItems.size() + ((fakeMediaItem == null) ? 0 : 1); } @Override public Window getWindow(int windowIndex, Window window, long defaultPositionProjectionUs) { // TODO(b/149713425): Set duration if it's available from MediaMetadataCompat. MediaItem mediaItem; - if (windowIndex == mediaItems.size() && fakeMediaItem != null) { + if (windowIndex == queuedMediaItems.size() && fakeMediaItem != null) { mediaItem = fakeMediaItem; } else { - mediaItem = mediaItems.get(windowIndex); + mediaItem = queuedMediaItems.get(windowIndex).mediaItem; } return getWindow(window, mediaItem, windowIndex); } @@ -257,14 +235,13 @@ import java.util.Map; return false; } QueueTimeline other = (QueueTimeline) obj; - return Objects.equal(mediaItems, other.mediaItems) - && Objects.equal(mediaItemToQueueIdMap, other.mediaItemToQueueIdMap) + return Objects.equal(queuedMediaItems, other.queuedMediaItems) && Objects.equal(fakeMediaItem, other.fakeMediaItem); } @Override public int hashCode() { - return Objects.hashCode(mediaItems, mediaItemToQueueIdMap, fakeMediaItem); + return Objects.hashCode(queuedMediaItems, fakeMediaItem); } private static Window getWindow(Window window, MediaItem mediaItem, int windowIndex) { @@ -285,4 +262,35 @@ import java.util.Map; /* positionInFirstPeriodUs= */ 0); return window; } + + private static final class QueuedMediaItem { + + public final MediaItem mediaItem; + public final long queueId; + + public QueuedMediaItem(MediaItem mediaItem, long queueId) { + this.mediaItem = mediaItem; + this.queueId = queueId; + } + + @Override + public boolean equals(@Nullable Object o) { + if (this == o) { + return true; + } + if (!(o instanceof QueuedMediaItem)) { + return false; + } + QueuedMediaItem that = (QueuedMediaItem) o; + return queueId == that.queueId && mediaItem.equals(that.mediaItem); + } + + @Override + public int hashCode() { + int result = 7; + result = 31 * result + (int) (queueId ^ (queueId >>> 32)); + result = 31 * result + mediaItem.hashCode(); + return result; + } + } } diff --git a/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaControllerWithMediaSessionCompatTest.java b/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaControllerWithMediaSessionCompatTest.java index 18442014a7..6e9d4363f7 100644 --- a/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaControllerWithMediaSessionCompatTest.java +++ b/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaControllerWithMediaSessionCompatTest.java @@ -79,6 +79,7 @@ import androidx.test.ext.junit.runners.AndroidJUnit4; import androidx.test.ext.truth.os.BundleSubject; import androidx.test.filters.MediumTest; import com.google.common.collect.ImmutableList; +import com.google.common.collect.Iterables; import com.google.common.collect.Range; import com.google.common.util.concurrent.Futures; import com.google.common.util.concurrent.ListenableFuture; @@ -415,6 +416,41 @@ public class MediaControllerWithMediaSessionCompatTest { assertThat(timelineRef.get().getPeriodCount()).isEqualTo(0); } + @Test + public void setQueue_withDuplicatedMediaItems_updatesAndNotifiesTimeline() throws Exception { + MediaController controller = controllerTestRule.createController(session.getSessionToken()); + CountDownLatch latch = new CountDownLatch(1); + AtomicReference timelineFromParamRef = new AtomicReference<>(); + AtomicReference timelineFromGetterRef = new AtomicReference<>(); + AtomicInteger reasonRef = new AtomicInteger(); + Player.Listener listener = + new Player.Listener() { + @Override + public void onTimelineChanged( + Timeline timeline, @Player.TimelineChangeReason int reason) { + timelineFromParamRef.set(timeline); + timelineFromGetterRef.set(controller.getCurrentTimeline()); + reasonRef.set(reason); + latch.countDown(); + } + }; + threadTestRule.getHandler().postAndSync(() -> controller.addListener(listener)); + + List mediaItems = MediaTestUtils.createMediaItems(/* size= */ 2); + Timeline testTimeline = + MediaTestUtils.createTimeline( + ImmutableList.copyOf(Iterables.concat(mediaItems, mediaItems))); + List testQueue = + MediaTestUtils.convertToQueueItemsWithoutBitmap( + MediaUtils.convertToMediaItemList(testTimeline)); + session.setQueue(testQueue); + + assertThat(latch.await(TIMEOUT_MS, MILLISECONDS)).isTrue(); + MediaTestUtils.assertMediaIdEquals(testTimeline, timelineFromParamRef.get()); + MediaTestUtils.assertMediaIdEquals(testTimeline, timelineFromGetterRef.get()); + assertThat(reasonRef.get()).isEqualTo(Player.TIMELINE_CHANGE_REASON_PLAYLIST_CHANGED); + } + @Test public void setQueue_withDescription_notifiesTimelineWithMetadata() throws Exception { CountDownLatch latch = new CountDownLatch(1);