Support release in SimpleBasePlayer

This adds support for the release handling. To align with the established behavior in ExoPlayer, the player can only call listeners from within the release methods (and not afterwards) and automatically enforces an IDLE state (without listener call) in case getters of the player are used after release. PiperOrigin-RevId: 493543958

Support release in SimpleBasePlayer
This adds support for the release handling. To align with the established behavior in ExoPlayer, the player can only call listeners from within the release methods (and not afterwards) and automatically enforces an IDLE state (without listener call) in case getters of the player are used after release. PiperOrigin-RevId: 493543958
3a66c28d · tonihei · Ian Baker · 01ff4e37 · 3a66c28d · 3a66c28d
Commit 3a66c28d authored Dec 07, 2022 by tonihei Committed by Ian Baker Dec 12, 2022
Showing with 208 additions and 22 deletions
library/common/src/main/java/com/google/android/exoplayer2/SimpleBasePlayer.java
library/common/src/test/java/com/google/android/exoplayer2/SimpleBasePlayerTest.java
--- a/library/common/src/main/java/com/google/android/exoplayer2/SimpleBasePlayer.java
+++ b/library/common/src/main/java/com/google/android/exoplayer2/SimpleBasePlayer.java
@@ -1940,6 +1940,7 @@ public abstract class SimpleBasePlayer extends BasePlayer {
  private final Timeline.Period period;
  private @MonotonicNonNull State state;
+  private boolean released;
  /**
   * Creates the base class.
@@ -2002,7 +2003,7 @@ public abstract class SimpleBasePlayer extends BasePlayer {
    verifyApplicationThreadAndInitState();
    // Use a local copy to ensure the lambda below uses the current state value.
    State state = this.state;
-    if (!state.availableCommands.contains(Player.COMMAND_PLAY_PAUSE)) {
+    if (!shouldHandleCommand(Player.COMMAND_PLAY_PAUSE)) {
      return;
    }
    updateStateForPendingOperation(
@@ -2056,7 +2057,7 @@ public abstract class SimpleBasePlayer extends BasePlayer {
    verifyApplicationThreadAndInitState();
    // Use a local copy to ensure the lambda below uses the current state value.
    State state = this.state;
-    if (!state.availableCommands.contains(Player.COMMAND_PREPARE)) {
+    if (!shouldHandleCommand(Player.COMMAND_PREPARE)) {
      return;
    }
    updateStateForPendingOperation(
@@ -2094,7 +2095,7 @@ public abstract class SimpleBasePlayer extends BasePlayer {
    verifyApplicationThreadAndInitState();
    // Use a local copy to ensure the lambda below uses the current state value.
    State state = this.state;
-    if (!state.availableCommands.contains(Player.COMMAND_SET_REPEAT_MODE)) {
+    if (!shouldHandleCommand(Player.COMMAND_SET_REPEAT_MODE)) {
      return;
    }
    updateStateForPendingOperation(
@@ -2114,7 +2115,7 @@ public abstract class SimpleBasePlayer extends BasePlayer {
    verifyApplicationThreadAndInitState();
    // Use a local copy to ensure the lambda below uses the current state value.
    State state = this.state;
-    if (!state.availableCommands.contains(Player.COMMAND_SET_SHUFFLE_MODE)) {
+    if (!shouldHandleCommand(Player.COMMAND_SET_SHUFFLE_MODE)) {
      return;
    }
    updateStateForPendingOperation(
@@ -2170,7 +2171,7 @@ public abstract class SimpleBasePlayer extends BasePlayer {
    verifyApplicationThreadAndInitState();
    // Use a local copy to ensure the lambda below uses the current state value.
    State state = this.state;
-    if (!state.availableCommands.contains(Player.COMMAND_SET_SPEED_AND_PITCH)) {
+    if (!shouldHandleCommand(Player.COMMAND_SET_SPEED_AND_PITCH)) {
      return;
    }
    updateStateForPendingOperation(
@@ -2190,7 +2191,7 @@ public abstract class SimpleBasePlayer extends BasePlayer {
    verifyApplicationThreadAndInitState();
    // Use a local copy to ensure the lambda below uses the current state value.
    State state = this.state;
-    if (!state.availableCommands.contains(Player.COMMAND_STOP)) {
+    if (!shouldHandleCommand(Player.COMMAND_STOP)) {
      return;
    }
    updateStateForPendingOperation(
@@ -2215,8 +2216,25 @@ public abstract class SimpleBasePlayer extends BasePlayer {
  @Override
  public final void release() {
-    // TODO: implement.
+    verifyApplicationThreadAndInitState();
-    throw new IllegalStateException();
+    // Use a local copy to ensure the lambda below uses the current state value.
+    State state = this.state;
+    if (released) { // TODO(b/261158047): Replace by !shouldHandleCommand(Player.COMMAND_RELEASE)
+      return;
+    }
+    updateStateForPendingOperation(
+        /* pendingOperation= */ handleRelease(), /* placeholderStateSupplier= */ () -> state);
+    released = true;
+    listeners.release();
+    // Enforce some final state values in case getters are called after release.
+    this.state =
+        this.state
+            .buildUpon()
+            .setPlaybackState(Player.STATE_IDLE)
+            .setTotalBufferedDurationMs(PositionSupplier.ZERO)
+            .setContentBufferedPositionMs(state.contentPositionMsSupplier)
+            .setAdBufferedPositionMs(state.adPositionMsSupplier)
+            .build();
  }
  @Override
@@ -2236,7 +2254,7 @@ public abstract class SimpleBasePlayer extends BasePlayer {
    verifyApplicationThreadAndInitState();
    // Use a local copy to ensure the lambda below uses the current state value.
    State state = this.state;
-    if (!state.availableCommands.contains(Player.COMMAND_SET_TRACK_SELECTION_PARAMETERS)) {
+    if (!shouldHandleCommand(Player.COMMAND_SET_TRACK_SELECTION_PARAMETERS)) {
      return;
    }
    updateStateForPendingOperation(
@@ -2262,7 +2280,7 @@ public abstract class SimpleBasePlayer extends BasePlayer {
    verifyApplicationThreadAndInitState();
    // Use a local copy to ensure the lambda below uses the current state value.
    State state = this.state;
-    if (!state.availableCommands.contains(Player.COMMAND_SET_MEDIA_ITEMS_METADATA)) {
+    if (!shouldHandleCommand(Player.COMMAND_SET_MEDIA_ITEMS_METADATA)) {
      return;
    }
    updateStateForPendingOperation(
@@ -2363,7 +2381,7 @@ public abstract class SimpleBasePlayer extends BasePlayer {
    verifyApplicationThreadAndInitState();
    // Use a local copy to ensure the lambda below uses the current state value.
    State state = this.state;
-    if (!state.availableCommands.contains(Player.COMMAND_SET_VOLUME)) {
+    if (!shouldHandleCommand(Player.COMMAND_SET_VOLUME)) {
      return;
    }
    updateStateForPendingOperation(
@@ -2382,7 +2400,7 @@ public abstract class SimpleBasePlayer extends BasePlayer {
    verifyApplicationThreadAndInitState();
    // Use a local copy to ensure the lambda below uses the current state value.
    State state = this.state;
-    if (!state.availableCommands.contains(Player.COMMAND_SET_VIDEO_SURFACE)) {
+    if (!shouldHandleCommand(Player.COMMAND_SET_VIDEO_SURFACE)) {
      return;
    }
    if (surface == null) {
@@ -2400,7 +2418,7 @@ public abstract class SimpleBasePlayer extends BasePlayer {
    verifyApplicationThreadAndInitState();
    // Use a local copy to ensure the lambda below uses the current state value.
    State state = this.state;
-    if (!state.availableCommands.contains(Player.COMMAND_SET_VIDEO_SURFACE)) {
+    if (!shouldHandleCommand(Player.COMMAND_SET_VIDEO_SURFACE)) {
      return;
    }
    if (surfaceHolder == null) {
@@ -2418,7 +2436,7 @@ public abstract class SimpleBasePlayer extends BasePlayer {
    verifyApplicationThreadAndInitState();
    // Use a local copy to ensure the lambda below uses the current state value.
    State state = this.state;
-    if (!state.availableCommands.contains(Player.COMMAND_SET_VIDEO_SURFACE)) {
+    if (!shouldHandleCommand(Player.COMMAND_SET_VIDEO_SURFACE)) {
      return;
    }
    if (surfaceView == null) {
@@ -2439,7 +2457,7 @@ public abstract class SimpleBasePlayer extends BasePlayer {
    verifyApplicationThreadAndInitState();
    // Use a local copy to ensure the lambda below uses the current state value.
    State state = this.state;
-    if (!state.availableCommands.contains(Player.COMMAND_SET_VIDEO_SURFACE)) {
+    if (!shouldHandleCommand(Player.COMMAND_SET_VIDEO_SURFACE)) {
      return;
    }
    if (textureView == null) {
@@ -2487,7 +2505,7 @@ public abstract class SimpleBasePlayer extends BasePlayer {
    verifyApplicationThreadAndInitState();
    // Use a local copy to ensure the lambda below uses the current state value.
    State state = this.state;
-    if (!state.availableCommands.contains(Player.COMMAND_SET_VIDEO_SURFACE)) {
+    if (!shouldHandleCommand(Player.COMMAND_SET_VIDEO_SURFACE)) {
      return;
    }
    updateStateForPendingOperation(
@@ -2536,7 +2554,7 @@ public abstract class SimpleBasePlayer extends BasePlayer {
    verifyApplicationThreadAndInitState();
    // Use a local copy to ensure the lambda below uses the current state value.
    State state = this.state;
-    if (!state.availableCommands.contains(Player.COMMAND_SET_DEVICE_VOLUME)) {
+    if (!shouldHandleCommand(Player.COMMAND_SET_DEVICE_VOLUME)) {
      return;
    }
    updateStateForPendingOperation(
@@ -2549,7 +2567,7 @@ public abstract class SimpleBasePlayer extends BasePlayer {
    verifyApplicationThreadAndInitState();
    // Use a local copy to ensure the lambda below uses the current state value.
    State state = this.state;
-    if (!state.availableCommands.contains(Player.COMMAND_ADJUST_DEVICE_VOLUME)) {
+    if (!shouldHandleCommand(Player.COMMAND_ADJUST_DEVICE_VOLUME)) {
      return;
    }
    updateStateForPendingOperation(
@@ -2563,7 +2581,7 @@ public abstract class SimpleBasePlayer extends BasePlayer {
    verifyApplicationThreadAndInitState();
    // Use a local copy to ensure the lambda below uses the current state value.
    State state = this.state;
-    if (!state.availableCommands.contains(Player.COMMAND_ADJUST_DEVICE_VOLUME)) {
+    if (!shouldHandleCommand(Player.COMMAND_ADJUST_DEVICE_VOLUME)) {
      return;
    }
    updateStateForPendingOperation(
@@ -2577,7 +2595,7 @@ public abstract class SimpleBasePlayer extends BasePlayer {
    verifyApplicationThreadAndInitState();
    // Use a local copy to ensure the lambda below uses the current state value.
    State state = this.state;
-    if (!state.availableCommands.contains(Player.COMMAND_ADJUST_DEVICE_VOLUME)) {
+    if (!shouldHandleCommand(Player.COMMAND_ADJUST_DEVICE_VOLUME)) {
      return;
    }
    updateStateForPendingOperation(
@@ -2596,7 +2614,7 @@ public abstract class SimpleBasePlayer extends BasePlayer {
   */
  protected final void invalidateState() {
    verifyApplicationThreadAndInitState();
-    if (!pendingOperations.isEmpty()) {
+    if (!pendingOperations.isEmpty() || released) {
      return;
    }
    updateStateAndInformListeners(getState());
@@ -2676,6 +2694,18 @@ public abstract class SimpleBasePlayer extends BasePlayer {
  }
  /**
+   * Handles calls to {@link Player#release}.
+   *
+   * @return A {@link ListenableFuture} indicating the completion of all immediate {@link State}
+   *     changes caused by this call.
+   */
+  // TODO(b/261158047): Add that this method will only be called if COMMAND_RELEASE is available.
+  @ForOverride
+  protected ListenableFuture<?> handleRelease() {
+    throw new IllegalStateException();
+  }
+  /**
   * Handles calls to {@link Player#setRepeatMode}.
   *
   * <p>Will only be called if {@link Player#COMMAND_SET_REPEAT_MODE} is available.
@@ -2847,6 +2877,11 @@ public abstract class SimpleBasePlayer extends BasePlayer {
    throw new IllegalStateException();
  }
+  @RequiresNonNull("state")
+  private boolean shouldHandleCommand(@Player.Command int commandCode) {
+    return !released && state.availableCommands.contains(commandCode);
+  }
  @SuppressWarnings("deprecation") // Calling deprecated listener methods.
  @RequiresNonNull("state")
  private void updateStateAndInformListeners(State newState) {
@@ -3091,7 +3126,7 @@ public abstract class SimpleBasePlayer extends BasePlayer {
          () -> {
            castNonNull(state); // Already checked by method @RequiresNonNull pre-condition.
            pendingOperations.remove(pendingOperation);
-            if (pendingOperations.isEmpty()) {
+            if (pendingOperations.isEmpty() && !released) {
              updateStateAndInformListeners(getState());
            }
          },

--- a/library/common/src/test/java/com/google/android/exoplayer2/SimpleBasePlayerTest.java
+++ b/library/common/src/test/java/com/google/android/exoplayer2/SimpleBasePlayerTest.java
@@ -22,6 +22,7 @@ import static org.mockito.ArgumentMatchers.anyBoolean;
 import static org.mockito.ArgumentMatchers.anyFloat;
 import static org.mockito.ArgumentMatchers.anyInt;
 import static org.mockito.ArgumentMatchers.anyLong;
+import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.verify;
@@ -57,6 +58,7 @@ import java.lang.reflect.Modifier;
 import java.util.ArrayList;
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicReference;
+import org.junit.Ignore;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.robolectric.shadows.ShadowLooper;
@@ -2170,6 +2172,155 @@ public class SimpleBasePlayerTest {
  }
  @Test
+  public void release_immediateHandling_updatesStateInformsListenersAndReturnsIdle() {
+    State state =
+        new State.Builder()
+            .setAvailableCommands(new Commands.Builder().addAllCommands().build())
+            .setPlaybackState(Player.STATE_READY)
+            .setPlaylist(
+                ImmutableList.of(
+                    new SimpleBasePlayer.MediaItemData.Builder(/* uid= */ new Object()).build()))
+            .build();
+    State updatedState = state.buildUpon().setRepeatMode(Player.REPEAT_MODE_ALL).build();
+    SimpleBasePlayer player =
+        new SimpleBasePlayer(Looper.myLooper()) {
+          private State playerState = state;
+          @Override
+          protected State getState() {
+            return playerState;
+          }
+          @Override
+          protected ListenableFuture<?> handleRelease() {
+            playerState = updatedState;
+            return Futures.immediateVoidFuture();
+          }
+        };
+    Listener listener = mock(Listener.class);
+    player.addListener(listener);
+    player.release();
+    assertThat(player.getPlaybackState()).isEqualTo(Player.STATE_IDLE);
+    assertThat(player.getRepeatMode()).isEqualTo(Player.REPEAT_MODE_ALL);
+    verify(listener).onRepeatModeChanged(Player.REPEAT_MODE_ALL);
+    verify(listener).onEvents(eq(player), any());
+    verifyNoMoreInteractions(listener);
+  }
+  @Test
+  public void release_asyncHandling_returnsIdleAndIgnoredAsyncStateUpdate() {
+    State state =
+        new State.Builder()
+            .setAvailableCommands(new Commands.Builder().addAllCommands().build())
+            .setPlaybackState(Player.STATE_READY)
+            .setPlaylist(
+                ImmutableList.of(
+                    new SimpleBasePlayer.MediaItemData.Builder(/* uid= */ new Object()).build()))
+            .build();
+    // Additionally set the repeat mode to see a difference between the placeholder and new state.
+    State updatedState = state.buildUpon().setRepeatMode(Player.REPEAT_MODE_ALL).build();
+    SettableFuture<?> future = SettableFuture.create();
+    SimpleBasePlayer player =
+        new SimpleBasePlayer(Looper.myLooper()) {
+          @Override
+          protected State getState() {
+            return future.isDone() ? updatedState : state;
+          }
+          @Override
+          protected ListenableFuture<?> handleRelease() {
+            return future;
+          }
+        };
+    Listener listener = mock(Listener.class);
+    player.addListener(listener);
+    player.release();
+    // Verify initial change to IDLE without listener call.
+    assertThat(player.getPlaybackState()).isEqualTo(Player.STATE_IDLE);
+    verifyNoMoreInteractions(listener);
+    future.set(null);
+    // Verify no further update happened.
+    assertThat(player.getRepeatMode()).isEqualTo(Player.REPEAT_MODE_OFF);
+    verifyNoMoreInteractions(listener);
+  }
+  @Ignore("b/261158047: Ignore test while Player.COMMAND_RELEASE doesn't exist.")
+  @Test
+  public void release_withoutAvailableCommand_isNotForwarded() {
+    State state =
+        new State.Builder()
+            // TODO(b/261158047): Uncomment once test is no longer ignored.
+            // .setAvailableCommands(
+            //    new Commands.Builder().addAllCommands().remove(Player.COMMAND_RELEASE).build())
+            .build();
+    AtomicBoolean callForwarded = new AtomicBoolean();
+    SimpleBasePlayer player =
+        new SimpleBasePlayer(Looper.myLooper()) {
+          @Override
+          protected State getState() {
+            return state;
+          }
+          @Override
+          protected ListenableFuture<?> handleRelease() {
+            callForwarded.set(true);
+            return Futures.immediateVoidFuture();
+          }
+        };
+    player.release();
+    assertThat(callForwarded.get()).isFalse();
+  }
+  @Test
+  public void release_withSubsequentPlayerAction_ignoresSubsequentAction() {
+    AtomicBoolean releaseCalled = new AtomicBoolean();
+    AtomicBoolean getStateCalledAfterRelease = new AtomicBoolean();
+    AtomicBoolean handlePlayWhenReadyCalledAfterRelease = new AtomicBoolean();
+    SimpleBasePlayer player =
+        new SimpleBasePlayer(Looper.myLooper()) {
+          @Override
+          protected State getState() {
+            if (releaseCalled.get()) {
+              getStateCalledAfterRelease.set(true);
+            }
+            return new State.Builder()
+                .setAvailableCommands(new Commands.Builder().addAllCommands().build())
+                .build();
+          }
+          @Override
+          protected ListenableFuture<?> handleSetPlayWhenReady(boolean playWhenReady) {
+            if (releaseCalled.get()) {
+              handlePlayWhenReadyCalledAfterRelease.set(true);
+            }
+            return Futures.immediateVoidFuture();
+          }
+          @Override
+          protected ListenableFuture<?> handleRelease() {
+            return Futures.immediateVoidFuture();
+          }
+        };
+    player.release();
+    releaseCalled.set(true);
+    // Try triggering a regular player action and to invalidate the state manually.
+    player.setPlayWhenReady(true);
+    player.invalidateState();
+    assertThat(getStateCalledAfterRelease.get()).isFalse();
+    assertThat(handlePlayWhenReadyCalledAfterRelease.get()).isFalse();
+  }
+  @Test
  public void setRepeatMode_immediateHandling_updatesStateAndInformsListeners() {
    State state =
        new State.Builder()