From 4895bc42ff656ba77b604d8c7c93cba64733cc7a Mon Sep 17 00:00:00 2001 From: tonihei Date: Wed, 7 Dec 2022 10:19:17 +0000 Subject: [PATCH] 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 --- .../media3/common/SimpleBasePlayer.java | 79 ++++++--- .../media3/common/SimpleBasePlayerTest.java | 151 ++++++++++++++++++ 2 files changed, 208 insertions(+), 22 deletions(-) diff --git a/libraries/common/src/main/java/androidx/media3/common/SimpleBasePlayer.java b/libraries/common/src/main/java/androidx/media3/common/SimpleBasePlayer.java index 893968b3b0..731dca5630 100644 --- a/libraries/common/src/main/java/androidx/media3/common/SimpleBasePlayer.java +++ b/libraries/common/src/main/java/androidx/media3/common/SimpleBasePlayer.java @@ -1937,6 +1937,7 @@ public abstract class SimpleBasePlayer extends BasePlayer { private final Timeline.Period period; private @MonotonicNonNull State state; + private boolean released; /** * Creates the base class. @@ -1999,7 +2000,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( @@ -2053,7 +2054,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( @@ -2091,7 +2092,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( @@ -2111,7 +2112,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( @@ -2167,7 +2168,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( @@ -2187,7 +2188,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( @@ -2212,8 +2213,25 @@ public abstract class SimpleBasePlayer extends BasePlayer { @Override public final void release() { - // TODO: implement. - throw new IllegalStateException(); + verifyApplicationThreadAndInitState(); + // 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 @@ -2233,7 +2251,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( @@ -2259,7 +2277,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( @@ -2360,7 +2378,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( @@ -2379,7 +2397,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) { @@ -2397,7 +2415,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) { @@ -2415,7 +2433,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) { @@ -2436,7 +2454,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) { @@ -2484,7 +2502,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( @@ -2533,7 +2551,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( @@ -2546,7 +2564,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( @@ -2560,7 +2578,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( @@ -2574,7 +2592,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( @@ -2593,7 +2611,7 @@ public abstract class SimpleBasePlayer extends BasePlayer { */ protected final void invalidateState() { verifyApplicationThreadAndInitState(); - if (!pendingOperations.isEmpty()) { + if (!pendingOperations.isEmpty() || released) { return; } updateStateAndInformListeners(getState()); @@ -2672,6 +2690,18 @@ public abstract class SimpleBasePlayer extends BasePlayer { throw new IllegalStateException(); } + /** + * 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}. * @@ -2844,6 +2874,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) { @@ -3088,7 +3123,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()); } }, diff --git a/libraries/common/src/test/java/androidx/media3/common/SimpleBasePlayerTest.java b/libraries/common/src/test/java/androidx/media3/common/SimpleBasePlayerTest.java index 0ef67a9e7d..5c53e5e27b 100644 --- a/libraries/common/src/test/java/androidx/media3/common/SimpleBasePlayerTest.java +++ b/libraries/common/src/test/java/androidx/media3/common/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; @@ -50,6 +51,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; @@ -2162,6 +2164,155 @@ public class SimpleBasePlayerTest { assertThat(callForwarded.get()).isFalse(); } + @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 =