Update session position info on timeline change

This fixes an inconsistent state of the `PlayerInfo` when the index of the playing media item is changed by a playlist modification. In this inconsistent state, calling `Playerinfo.getCurrentMediaItem` can produce an `ArrayIndexOutOfBoundException` (see stack trace in GH issue). This change takes the following measurements: - always update sessionPosition and timeline of the PlayerInfo together in `MediaSessionImpl.PlayerListener` where the PlayerInfo originates from - add an assertion to avoid building a `PlayerInfo` instance in an inconsistent state - reduce the window of opportunity for concurrent access to `mediaSessionImpl.playerInfo` when dispatching player info changes in `MediaSessionImpl` Issue: #51 PiperOrigin-RevId: 444812661
androidx · May 9, 2022 · dee83cc · dee83cc
1 parent 9369348
commit dee83cc
Show file tree

Hide file tree

Showing 4 changed files with 39 additions and 17 deletions.
diff --git a/RELEASENOTES.md b/RELEASENOTES.md
@@ -80,6 +80,9 @@
 *   Session:
     *   Fix NPE in MediaControllerImplLegacy
         ([#59](https://github.com/androidx/media/pull/59))
+    *   Update session position info on timeline
+        change([#51](https://github.com/androidx/media/issues/51))
+
 *   Data sources:
     *   Rename `DummyDataSource` to `PlaceHolderDataSource`.
 *   Remove deprecated symbols:

diff --git a/libraries/session/src/main/java/androidx/media3/session/MediaControllerImplLegacy.java b/libraries/session/src/main/java/androidx/media3/session/MediaControllerImplLegacy.java
@@ -753,7 +753,8 @@ public void addMediaItems(int index, List<MediaItem> mediaItems) {
     int newCurrentMediaItemIndex =
         calculateCurrentItemIndexAfterAddItems(currentMediaItemIndex, index, mediaItems.size());
     PlayerInfo maskedPlayerInfo =
-        controllerInfo.playerInfo.copyWithTimeline(newQueueTimeline, newCurrentMediaItemIndex);
+        controllerInfo.playerInfo.copyWithTimelineAndMediaItemIndex(
+            newQueueTimeline, newCurrentMediaItemIndex);
     ControllerInfo maskedControllerInfo =
         new ControllerInfo(
             maskedPlayerInfo,
@@ -801,7 +802,8 @@ public void removeMediaItems(int fromIndex, int toIndex) {
               + " new current item");
     }
     PlayerInfo maskedPlayerInfo =
-        controllerInfo.playerInfo.copyWithTimeline(newQueueTimeline, newCurrentMediaItemIndex);
+        controllerInfo.playerInfo.copyWithTimelineAndMediaItemIndex(
+            newQueueTimeline, newCurrentMediaItemIndex);
 
     ControllerInfo maskedControllerInfo =
         new ControllerInfo(
@@ -861,7 +863,8 @@ public void moveMediaItems(int fromIndex, int toIndex, int newIndex) {
     QueueTimeline newQueueTimeline =
         queueTimeline.copyWithMovedMediaItems(fromIndex, toIndex, newIndex);
     PlayerInfo maskedPlayerInfo =
-        controllerInfo.playerInfo.copyWithTimeline(newQueueTimeline, newCurrentMediaItemIndex);
+        controllerInfo.playerInfo.copyWithTimelineAndMediaItemIndex(
+            newQueueTimeline, newCurrentMediaItemIndex);
 
     ControllerInfo maskedControllerInfo =
         new ControllerInfo(

diff --git a/libraries/session/src/main/java/androidx/media3/session/MediaSessionImpl.java b/libraries/session/src/main/java/androidx/media3/session/MediaSessionImpl.java
@@ -366,7 +366,7 @@ public void broadcastCustomCommand(SessionCommand command, Bundle args) {
         (controller, seq) -> controller.sendCustomCommand(seq, command, args));
   }
 
-  private void dispatchOnPlayerInfoChanged(boolean excludeTimeline) {
+  private void dispatchOnPlayerInfoChanged(PlayerInfo playerInfo, boolean excludeTimeline) {
 
     List<ControllerInfo> controllers =
         sessionStub.getConnectedControllersManager().getConnectedControllers();
@@ -910,7 +910,9 @@ public void onTimelineChanged(Timeline timeline, @Player.TimelineChangeReason in
       if (player == null) {
         return;
       }
-      session.playerInfo = session.playerInfo.copyWithTimeline(timeline);
+      session.playerInfo =
+          session.playerInfo.copyWithTimelineAndSessionPositionInfo(
+              timeline, player.createSessionPositionInfoForBundling());
       session.onPlayerInfoChangedHandler.sendPlayerInfoChangedMessage(/* excludeTimeline= */ false);
       session.dispatchRemoteControllerTaskToLegacyStub(
           (callback, seq) -> callback.onTimelineChanged(seq, timeline, reason));
@@ -1177,9 +1179,10 @@ public PlayerInfoChangedHandler(Looper looper) {
     public void handleMessage(Message msg) {
       if (msg.what == MSG_PLAYER_INFO_CHANGED) {
         playerInfo =
-            playerInfo.copyWithSessionPositionInfo(
+            playerInfo.copyWithTimelineAndSessionPositionInfo(
+                getPlayerWrapper().getCurrentTimeline(),
                 getPlayerWrapper().createSessionPositionInfoForBundling());
-        dispatchOnPlayerInfoChanged(excludeTimeline);
+        dispatchOnPlayerInfoChanged(playerInfo, excludeTimeline);
         excludeTimeline = true;
       } else {
         throw new IllegalStateException("Invalid message what=" + msg.what);

diff --git a/libraries/session/src/main/java/androidx/media3/session/PlayerInfo.java b/libraries/session/src/main/java/androidx/media3/session/PlayerInfo.java
@@ -44,6 +44,7 @@
 import androidx.media3.common.TrackSelectionParameters;
 import androidx.media3.common.VideoSize;
 import androidx.media3.common.text.Cue;
+import androidx.media3.common.util.Assertions;
 import androidx.media3.common.util.BundleableUtil;
 import com.google.common.collect.ImmutableList;
 import java.lang.annotation.Documented;
@@ -271,6 +272,9 @@ public Builder setTrackSelectionParameters(TrackSelectionParameters parameters)
     }
 
     public PlayerInfo build() {
+      Assertions.checkState(
+          timeline.isEmpty()
+              || sessionPositionInfo.positionInfo.mediaItemIndex < timeline.getWindowCount());
       return new PlayerInfo(
           playerError,
           mediaItemTransitionReason,
@@ -344,7 +348,7 @@ public PlayerInfo build() {
           MediaMetadata.EMPTY,
           /* seekBackIncrementMs= */ 0,
           /* seekForwardIncrementMs= */ 0,
-          /* maxSeekToPreviousPosition= */ 0,
+          /* maxSeekToPreviousPositionMs= */ 0,
           TrackSelectionParameters.DEFAULT_WITHOUT_CONTEXT);
 
   @Nullable public final PlaybackException playerError;
@@ -429,11 +433,6 @@ public PlayerInfo copyWithPlayerError(PlaybackException playerError) {
     return new Builder(this).setPlayerError(playerError).build();
   }
 
-  @CheckResult
-  public PlayerInfo copyWithSessionPositionInfo(SessionPositionInfo sessionPositionInfo) {
-    return new Builder(this).setSessionPositionInfo(sessionPositionInfo).build();
-  }
-
   @CheckResult
   public PlayerInfo copyWithPlaybackState(
       @Player.State int playbackState, @Nullable PlaybackException playerError) {
@@ -454,6 +453,11 @@ public PlayerInfo copyWithIsLoading(boolean isLoading) {
     return new Builder(this).setIsLoading(isLoading).build();
   }
 
+  @CheckResult
+  public PlayerInfo copyWithPlaybackParameters(PlaybackParameters playbackParameters) {
+    return new Builder(this).setPlaybackParameters(playbackParameters).build();
+  }
+
   @CheckResult
   public PlayerInfo copyWithPositionInfos(
       PositionInfo oldPositionInfo,
@@ -467,8 +471,8 @@ public PlayerInfo copyWithPositionInfos(
   }
 
   @CheckResult
-  public PlayerInfo copyWithPlaybackParameters(PlaybackParameters playbackParameters) {
-    return new Builder(this).setPlaybackParameters(playbackParameters).build();
+  public PlayerInfo copyWithSessionPositionInfo(SessionPositionInfo sessionPositionInfo) {
+    return new Builder(this).setSessionPositionInfo(sessionPositionInfo).build();
   }
 
   @CheckResult
@@ -477,14 +481,23 @@ public PlayerInfo copyWithTimeline(Timeline timeline) {
   }
 
   @CheckResult
-  public PlayerInfo copyWithTimeline(Timeline timeline, int windowIndex) {
+  public PlayerInfo copyWithTimelineAndSessionPositionInfo(
+      Timeline timeline, SessionPositionInfo sessionPositionInfo) {
+    return new Builder(this)
+        .setTimeline(timeline)
+        .setSessionPositionInfo(sessionPositionInfo)
+        .build();
+  }
+
+  @CheckResult
+  public PlayerInfo copyWithTimelineAndMediaItemIndex(Timeline timeline, int mediaItemIndex) {
     return new Builder(this)
         .setTimeline(timeline)
         .setSessionPositionInfo(
             new SessionPositionInfo(
                 new PositionInfo(
                     sessionPositionInfo.positionInfo.windowUid,
-                    windowIndex,
+                    mediaItemIndex,
                     sessionPositionInfo.positionInfo.mediaItem,
                     sessionPositionInfo.positionInfo.periodUid,
                     sessionPositionInfo.positionInfo.periodIndex,