Ensure masking PlayerInfos are not merged into real PlayerInfo updates

The masking process in MediaController sends pending actions and creates
placeholder PlayerInfo objects. Once all pending actions are received
and we got a final new PlayerInfo from the session, the masking stops
and we should continue using the real PlayerInfo from session again.

The current tracking wasn't quite accurate, leading to some cases
where we accidentally merge a masking PlayerInfo with a real one, which
may cause exceptions since they may not be compatible.

To solve this, we can
 - Clearly start the masking process by setting pendingPlayerInfo to
   the currently known PlayerInfo (not just when receiving intermediate
   updates). This has the nice side effect that we no longer need the
   pendingBundlingExclusions as we start off with no exlusions already
   and never have to deal with a pending state that has values excluded.
 - When ending the masking process, clearly copy over the last real
   PlayerInfo (without any exclusions), so that no merging with any
   placeholder/masking PlayerInfo object happens.

There is a remaining bug where no-op updated never send a real PlayerInfo,
meaning the controller stays in its masking state forever. This causes
some of the test flakiness, which we can temporarily work around by sleeping
to ensure the order of events in the remote process is guaranteed.

PiperOrigin-RevId: 776629604

Author: tonihei

RELEASENOTES.md

@@ -25,6 +25,8 @@
 *   Muxers:
 *   IMA extension:
 *   Session:
+    *   Fix bug where some controller changes that are not handled by the
+        session may cause `IllegalStateExceptions`.
 *   UI:
     *   Fix bug where `PlayerSurface` inside re-usable components like
         `LazyColumn` didn't work correctly

libraries/session/src/main/java/androidx/media3/session/MediaControllerImplBase.java

@@ -46,7 +46,6 @@
 import android.os.Process;
 import android.os.RemoteException;
 import android.os.SystemClock;
-import android.util.Pair;
 import android.view.Surface;
 import android.view.SurfaceHolder;
 import android.view.SurfaceView;
@@ -144,7 +143,6 @@
   private long currentPositionMs;
   private long lastSetPlayWhenReadyCalledTimeMs;
   @Nullable private PlayerInfo pendingPlayerInfo;
-  @Nullable private BundlingExclusions pendingBundlingExclusions;
   private Bundle sessionExtras;
 
   public MediaControllerImplBase(
@@ -386,6 +384,10 @@ private ListenableFuture<SessionResult> dispatchRemoteSessionTask(
               new SessionResult(SessionResult.RESULT_INFO_SKIPPED));
       int sequenceNumber = result.getSequenceNumber();
       if (addToPendingMaskingOperations) {
+        if (pendingMaskingSequencedFutureNumbers.isEmpty()) {
+          // First pending operation, start masking PlayerInfo.
+          pendingPlayerInfo = playerInfo;
+        }
         pendingMaskingSequencedFutureNumbers.add(sequenceNumber);
       }
       try {
@@ -2765,36 +2767,29 @@ void onPlayerInfoChanged(PlayerInfo newPlayerInfo, BundlingExclusions bundlingEx
     if (!isConnected()) {
       return;
     }
-    if (pendingPlayerInfo != null && pendingBundlingExclusions != null) {
-      Pair<PlayerInfo, BundlingExclusions> mergedPlayerInfoUpdate =
+    if (pendingPlayerInfo != null) {
+      pendingPlayerInfo =
           mergePlayerInfo(
-              pendingPlayerInfo,
-              pendingBundlingExclusions,
-              newPlayerInfo,
-              bundlingExclusions,
-              intersectedPlayerCommands);
-      newPlayerInfo = mergedPlayerInfoUpdate.first;
-      bundlingExclusions = mergedPlayerInfoUpdate.second;
-    }
-    pendingPlayerInfo = null;
-    pendingBundlingExclusions = null;
-    if (!pendingMaskingSequencedFutureNumbers.isEmpty()) {
-      // We are still waiting for all pending masking operations to be handled.
-      pendingPlayerInfo = newPlayerInfo;
-      pendingBundlingExclusions = bundlingExclusions;
-      return;
+              pendingPlayerInfo, newPlayerInfo, bundlingExclusions, intersectedPlayerCommands);
+      if (pendingMaskingSequencedFutureNumbers.isEmpty()) {
+        // Finish masking.
+        newPlayerInfo = pendingPlayerInfo;
+        bundlingExclusions = BundlingExclusions.NONE;
+        pendingPlayerInfo = null;
+      } else {
+        // We are still waiting for all pending masking operations to be handled.
+        return;
+      }
     }
     PlayerInfo oldPlayerInfo = playerInfo;
     // Assigning class variable now so that all getters called from listeners see the updated value.
     // But we need to use a local final variable to ensure listeners get consistent parameters.
     playerInfo =
         mergePlayerInfo(
-                oldPlayerInfo,
-                /* oldBundlingExclusions= */ BundlingExclusions.NONE,
-                newPlayerInfo,
-                /* newBundlingExclusions= */ bundlingExclusions,
-                intersectedPlayerCommands)
-            .first;
+            oldPlayerInfo,
+            newPlayerInfo,
+            /* newBundlingExclusions= */ bundlingExclusions,
+            intersectedPlayerCommands);
     PlayerInfo finalPlayerInfo = playerInfo;
 
     @Nullable

libraries/session/src/main/java/androidx/media3/session/MediaUtils.java

@@ -23,7 +23,6 @@
 import android.os.Parcelable;
 import android.os.SystemClock;
 import android.text.TextUtils;
-import android.util.Pair;
 import androidx.annotation.Nullable;
 import androidx.media3.common.C;
 import androidx.media3.common.Player;
@@ -128,39 +127,28 @@ public static Commands intersect(@Nullable Commands commands1, @Nullable Command
    * previousPlayerInfo} and taking into account the passed available commands.
    *
    * @param oldPlayerInfo The old {@link PlayerInfo}.
-   * @param oldBundlingExclusions The bundling exclusions in the old {@link PlayerInfo}.
    * @param newPlayerInfo The new {@link PlayerInfo}.
    * @param newBundlingExclusions The bundling exclusions in the new {@link PlayerInfo}.
    * @param availablePlayerCommands The available commands to take into account when merging.
-   * @return A pair with the resulting {@link PlayerInfo} and {@link BundlingExclusions}.
+   * @return The resulting merged {@link PlayerInfo}.
    */
-  public static Pair<PlayerInfo, BundlingExclusions> mergePlayerInfo(
+  public static PlayerInfo mergePlayerInfo(
       PlayerInfo oldPlayerInfo,
-      BundlingExclusions oldBundlingExclusions,
       PlayerInfo newPlayerInfo,
       BundlingExclusions newBundlingExclusions,
       Commands availablePlayerCommands) {
     PlayerInfo mergedPlayerInfo = newPlayerInfo;
-    BundlingExclusions mergedBundlingExclusions = newBundlingExclusions;
     if (newBundlingExclusions.isTimelineExcluded
-        && availablePlayerCommands.contains(Player.COMMAND_GET_TIMELINE)
-        && !oldBundlingExclusions.isTimelineExcluded) {
+        && availablePlayerCommands.contains(Player.COMMAND_GET_TIMELINE)) {
       // Use the previous timeline if it is excluded in the most recent update.
       mergedPlayerInfo = mergedPlayerInfo.copyWithTimeline(oldPlayerInfo.timeline);
-      mergedBundlingExclusions =
-          new BundlingExclusions(
-              /* isTimelineExcluded= */ false, mergedBundlingExclusions.areCurrentTracksExcluded);
     }
     if (newBundlingExclusions.areCurrentTracksExcluded
-        && availablePlayerCommands.contains(Player.COMMAND_GET_TRACKS)
-        && !oldBundlingExclusions.areCurrentTracksExcluded) {
+        && availablePlayerCommands.contains(Player.COMMAND_GET_TRACKS)) {
       // Use the previous tracks if it is excluded in the most recent update.
       mergedPlayerInfo = mergedPlayerInfo.copyWithCurrentTracks(oldPlayerInfo.currentTracks);
-      mergedBundlingExclusions =
-          new BundlingExclusions(
-              mergedBundlingExclusions.isTimelineExcluded, /* areCurrentTracksExcluded= */ false);
     }
-    return new Pair<>(mergedPlayerInfo, mergedBundlingExclusions);
+    return mergedPlayerInfo;
   }
 
   /** Generates an array of {@code n} indices. */

libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaControllerStateMaskingTest.java

@@ -58,6 +58,7 @@
 import org.junit.After;
 import org.junit.Before;
 import org.junit.ClassRule;
+import org.junit.Ignore;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.RuleChain;
@@ -3189,8 +3190,6 @@ public void onEvents(Player player, Player.Events events) {
           }
         };
     threadTestRule.getHandler().postAndSync(() -> controller.addListener(listener));
-
-    // Step 1: Report a discontinuity from item 0 to item 1 in the session.
     PositionInfo oldPositionInfo =
         new PositionInfo(
             /* windowUid= */ timeline.getWindow(/* windowIndex= */ 0, new Window()).uid,
@@ -3217,13 +3216,24 @@ public void onEvents(Player player, Player.Events events) {
             /* contentPositionMs= */ 0,
             /* adGroupIndex= */ C.INDEX_UNSET,
             /* adIndexInAdGroup= */ C.INDEX_UNSET);
-    remoteSession.getMockPlayer().setCurrentMediaItemIndex(1);
-    remoteSession
-        .getMockPlayer()
-        .notifyPositionDiscontinuity(
-            oldPositionInfo, newPositionInfo, Player.DISCONTINUITY_REASON_AUTO_TRANSITION);
-    // Step 2: Before step 1 can be handled by the controller, remove item 1.
-    threadTestRule.getHandler().postAndSync(() -> controller.removeMediaItem(/* index= */ 1));
+
+    threadTestRule
+        .getHandler()
+        .postAndSync(
+            () -> {
+              // Step 1: Report a discontinuity from item 0 to item 1 in the session.
+              remoteSession.getMockPlayer().setCurrentMediaItemIndex(1);
+              remoteSession
+                  .getMockPlayer()
+                  .notifyPositionDiscontinuity(
+                      oldPositionInfo,
+                      newPositionInfo,
+                      Player.DISCONTINUITY_REASON_AUTO_TRANSITION);
+              // Step 2: Before step 1 can be handled by the controller, remove item 1.
+              controller.removeMediaItem(/* index= */ 1);
+            });
+    Thread.sleep(100); // TODO: b/428144538 - Remove sleep once we are guaranteed to get an update
+    // Send player updates for item removal.
     remoteSession.getMockPlayer().setCurrentMediaItemIndex(0);
     remoteSession.getMockPlayer().setTimeline(MediaTestUtils.createTimeline(/* windowCount= */ 1));
     remoteSession.getMockPlayer().notifyPlaybackStateChanged(Player.STATE_ENDED);
@@ -3235,6 +3245,98 @@ public void onEvents(Player player, Player.Events events) {
     assertThat(reportedStateChangeToEndedAtSameTimeAsDiscontinuity.get()).isTrue();
   }
 
+  @Ignore // TODO: b/428144538 - Fix the logic to make this test possible
+  @Test
+  public void timelineUpdatesDuringMasking_withNoPlayerInfoUpdateFromSession_areResolvedCorrectly()
+      throws Exception {
+    Timeline timeline = MediaTestUtils.createTimeline(/* windowCount= */ 5);
+    remoteSession.getMockPlayer().setTimeline(timeline);
+    remoteSession.getMockPlayer().setCurrentMediaItemIndex(4);
+    remoteSession
+        .getMockPlayer()
+        .notifyTimelineChanged(Player.TIMELINE_CHANGE_REASON_PLAYLIST_CHANGED);
+    MediaController controller = controllerTestRule.createController(remoteSession.getToken());
+    CountDownLatch timelineChangeReported = new CountDownLatch(2);
+    ArrayList<Timeline> reportedTimelineChanges = new ArrayList<>();
+    ArrayList<Integer> reportedCurrentIndex = new ArrayList<>();
+    Player.Listener listener =
+        new Player.Listener() {
+          @Override
+          public void onEvents(Player player, Player.Events events) {
+            if (events.contains(Player.EVENT_TIMELINE_CHANGED)) {
+              reportedTimelineChanges.add(player.getCurrentTimeline());
+              reportedCurrentIndex.add(player.getCurrentMediaItemIndex());
+              timelineChangeReported.countDown();
+            }
+          }
+        };
+    threadTestRule.getHandler().postAndSync(() -> controller.addListener(listener));
+
+    // Change masked Timeline in controller by assigning new single item, but don't change Timeline
+    // of the remote session. This means the timeline change needs to be reversed once the update is
+    // handled even if the remote session doesn't send any further update.
+    threadTestRule
+        .getHandler()
+        .postAndSync(
+            () ->
+                controller.setMediaItem(
+                    new MediaItem.Builder().setMediaId("placeholder_id").build()));
+
+    assertThat(timelineChangeReported.await(TIMEOUT_MS, MILLISECONDS)).isTrue();
+    assertThat(reportedTimelineChanges.get(0).getWindowCount()).isEqualTo(1);
+    assertThat(reportedTimelineChanges.get(1).getWindowCount()).isEqualTo(5);
+    assertThat(reportedCurrentIndex.get(0)).isEqualTo(0);
+    assertThat(reportedCurrentIndex.get(1)).isEqualTo(4);
+  }
+
+  @Test
+  public void
+      timelineUpdatesDuringMasking_withPlayerInfoUpdateExcludingTimeline_areResolvedCorrectly()
+          throws Exception {
+    Timeline timeline = MediaTestUtils.createTimeline(/* windowCount= */ 5);
+    remoteSession.getMockPlayer().setPlaybackState(Player.STATE_READY);
+    remoteSession.getMockPlayer().setTimeline(timeline);
+    remoteSession.getMockPlayer().setCurrentMediaItemIndex(4);
+    remoteSession
+        .getMockPlayer()
+        .notifyTimelineChanged(Player.TIMELINE_CHANGE_REASON_PLAYLIST_CHANGED);
+    MediaController controller = controllerTestRule.createController(remoteSession.getToken());
+    CountDownLatch timelineChangeReported = new CountDownLatch(2);
+    ArrayList<Timeline> reportedTimelineChanges = new ArrayList<>();
+    ArrayList<Integer> reportedCurrentIndex = new ArrayList<>();
+    Player.Listener listener =
+        new Player.Listener() {
+          @Override
+          public void onEvents(Player player, Player.Events events) {
+            if (events.contains(Player.EVENT_TIMELINE_CHANGED)) {
+              reportedTimelineChanges.add(player.getCurrentTimeline());
+              reportedCurrentIndex.add(player.getCurrentMediaItemIndex());
+              timelineChangeReported.countDown();
+            }
+          }
+        };
+    threadTestRule.getHandler().postAndSync(() -> controller.addListener(listener));
+
+    // Change masked Timeline in controller by assigning new single item, but don't change Timeline
+    // of the remote session. This means the timeline change needs to be reversed once the update is
+    // handled even if the remote session doesn't send any further Timeline updates.
+    threadTestRule
+        .getHandler()
+        .postAndSync(
+            () ->
+                controller.setMediaItem(
+                    new MediaItem.Builder().setMediaId("placeholder_id").build()));
+    Thread.sleep(100); // TODO: b/428144538 - Remove sleep once we are guaranteed to get an update
+    // Update session with new PlayerInfo, without changing Timeline.
+    remoteSession.getMockPlayer().notifyPlaybackStateChanged(Player.STATE_BUFFERING);
+
+    assertThat(timelineChangeReported.await(TIMEOUT_MS, MILLISECONDS)).isTrue();
+    assertThat(reportedTimelineChanges.get(0).getWindowCount()).isEqualTo(1);
+    assertThat(reportedTimelineChanges.get(1).getWindowCount()).isEqualTo(5);
+    assertThat(reportedCurrentIndex.get(0)).isEqualTo(0);
+    assertThat(reportedCurrentIndex.get(1)).isEqualTo(4);
+  }
+
   @Test
   public void seekTo_indexLargerThanPlaylist_isIgnored() throws Exception {
     MediaController controller = controllerTestRule.createController(remoteSession.getToken());