From 1d26d1891e6e3cd49a4b479723b8aba7be746c1c Mon Sep 17 00:00:00 2001 From: tonihei Date: Thu, 20 Jun 2024 03:55:17 -0700 Subject: [PATCH] Clarify that onPlayWhenReadyChanged can be called again with new reason Sometimes the reason for the current state may change. If we don't report this again, users have no way of knowing that the reason changed. Also adjust ExoPlayerImpl and MediaControllerImplBase accordingly. SimpleBasePlayer already adheres to this logic. #cherrypick PiperOrigin-RevId: 644970236 --- .../java/androidx/media3/common/Player.java | 3 + .../media3/exoplayer/ExoPlayerImpl.java | 7 ++- .../media3/exoplayer/ExoPlayerTest.java | 4 +- .../session/MediaControllerImplBase.java | 3 +- .../session/common/IRemoteMediaSession.aidl | 2 +- ...lerCompatCallbackWithMediaSessionTest.java | 14 ++++- .../session/MediaControllerListenerTest.java | 57 +++++++++++++++++-- .../MediaSessionAndControllerTest.java | 4 +- .../session/MediaSessionKeyEventTest.java | 4 +- .../session/MediaSessionProviderService.java | 8 ++- .../androidx/media3/session/MockPlayer.java | 27 ++++++--- .../media3/session/RemoteMediaSession.java | 7 ++- 12 files changed, 113 insertions(+), 27 deletions(-) diff --git a/libraries/common/src/main/java/androidx/media3/common/Player.java b/libraries/common/src/main/java/androidx/media3/common/Player.java index 54463c2b77..ffccdce19c 100644 --- a/libraries/common/src/main/java/androidx/media3/common/Player.java +++ b/libraries/common/src/main/java/androidx/media3/common/Player.java @@ -922,6 +922,9 @@ public interface Player { /** * Called when the value returned from {@link #getPlayWhenReady()} changes. * + *

The current {@code playWhenReady} value may be re-reported if the {@code reason} for this + * value changes. + * *

{@link #onEvents(Player, Events)} will also be called to report this event along with * other events that happen in the same {@link Looper} message queue iteration. * diff --git a/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/ExoPlayerImpl.java b/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/ExoPlayerImpl.java index c303d9f185..0a16ed745f 100644 --- a/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/ExoPlayerImpl.java +++ b/libraries/exoplayer/src/main/java/androidx/media3/exoplayer/ExoPlayerImpl.java @@ -2154,7 +2154,9 @@ import java.util.concurrent.TimeoutException; Player.EVENT_PLAYBACK_STATE_CHANGED, listener -> listener.onPlaybackStateChanged(newPlaybackInfo.playbackState)); } - if (playWhenReadyChanged) { + if (playWhenReadyChanged + || previousPlaybackInfo.playWhenReadyChangeReason + != newPlaybackInfo.playWhenReadyChangeReason) { listeners.queueEvent( Player.EVENT_PLAY_WHEN_READY_CHANGED, listener -> @@ -2798,7 +2800,8 @@ import java.util.concurrent.TimeoutException; @PlaybackSuppressionReason int playbackSuppressionReason = computePlaybackSuppressionReason(playWhenReady, playerCommand); if (playbackInfo.playWhenReady == playWhenReady - && playbackInfo.playbackSuppressionReason == playbackSuppressionReason) { + && playbackInfo.playbackSuppressionReason == playbackSuppressionReason + && playbackInfo.playWhenReadyChangeReason == playWhenReadyChangeReason) { return; } updatePlaybackInfoForPlayWhenReadyAndSuppressionReasonStates( diff --git a/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/ExoPlayerTest.java b/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/ExoPlayerTest.java index 13cf7335f7..e1ec207519 100644 --- a/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/ExoPlayerTest.java +++ b/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/ExoPlayerTest.java @@ -4392,7 +4392,9 @@ public class ExoPlayerTest { assertThat(playWhenReady).isFalse(); assertThat(suppressionReason).isEqualTo(Player.PLAYBACK_SUPPRESSION_REASON_NONE); verify(listener, never()).onPlaybackSuppressionReasonChanged(anyInt()); - // TODO: Fix behavior and assert that audio focus loss is reported via onPlayWhenReadyChanged. + verify(listener) + .onPlayWhenReadyChanged( + /* playWhenReady= */ false, Player.PLAY_WHEN_READY_CHANGE_REASON_AUDIO_FOCUS_LOSS); } @Test diff --git a/libraries/session/src/main/java/androidx/media3/session/MediaControllerImplBase.java b/libraries/session/src/main/java/androidx/media3/session/MediaControllerImplBase.java index 0bccdce336..3a2cf83181 100644 --- a/libraries/session/src/main/java/androidx/media3/session/MediaControllerImplBase.java +++ b/libraries/session/src/main/java/androidx/media3/session/MediaControllerImplBase.java @@ -2746,7 +2746,8 @@ import org.checkerframework.checker.nullness.qual.NonNull; @Nullable @Player.PlayWhenReadyChangeReason Integer playWhenReadyChangeReason = - oldPlayerInfo.playWhenReady != finalPlayerInfo.playWhenReady + oldPlayerInfo.playWhenReadyChangeReason != finalPlayerInfo.playWhenReadyChangeReason + || oldPlayerInfo.playWhenReady != finalPlayerInfo.playWhenReady ? finalPlayerInfo.playWhenReadyChangeReason : null; diff --git a/libraries/test_session_common/src/main/aidl/androidx/media3/test/session/common/IRemoteMediaSession.aidl b/libraries/test_session_common/src/main/aidl/androidx/media3/test/session/common/IRemoteMediaSession.aidl index e59ed772d2..115e19e91a 100644 --- a/libraries/test_session_common/src/main/aidl/androidx/media3/test/session/common/IRemoteMediaSession.aidl +++ b/libraries/test_session_common/src/main/aidl/androidx/media3/test/session/common/IRemoteMediaSession.aidl @@ -59,7 +59,7 @@ interface IRemoteMediaSession { void increaseDeviceVolume(String sessionId, int flags); void setDeviceMuted(String sessionId, boolean muted, int flags); void notifyPlayerError(String sessionId, in Bundle playerErrorBundle); - void notifyPlayWhenReadyChanged(String sessionId, boolean playWhenReady, int reason); + void notifyPlayWhenReadyChanged(String sessionId, boolean playWhenReady, int playWhenReadyChangeReason, int suppressionReason); void notifyPlaybackStateChanged(String sessionId, int state); void notifyIsLoadingChanged(String sessionId, boolean isLoading); void notifyPositionDiscontinuity(String sessionId, diff --git a/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaControllerCompatCallbackWithMediaSessionTest.java b/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaControllerCompatCallbackWithMediaSessionTest.java index 5b5e27e798..a118c2e391 100644 --- a/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaControllerCompatCallbackWithMediaSessionTest.java +++ b/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaControllerCompatCallbackWithMediaSessionTest.java @@ -789,7 +789,9 @@ public class MediaControllerCompatCallbackWithMediaSessionTest { session .getMockPlayer() .notifyPlayWhenReadyChanged( - /* playWhenReady= */ false, Player.PLAYBACK_SUPPRESSION_REASON_NONE); + /* playWhenReady= */ false, + Player.PLAY_WHEN_READY_CHANGE_REASON_USER_REQUEST, + Player.PLAYBACK_SUPPRESSION_REASON_NONE); assertThat(latch.await(TIMEOUT_MS, MILLISECONDS)).isTrue(); assertThat(playbackStateCompatRef.get().getState()).isEqualTo(PlaybackStateCompat.STATE_PAUSED); @@ -833,7 +835,9 @@ public class MediaControllerCompatCallbackWithMediaSessionTest { session .getMockPlayer() .notifyPlayWhenReadyChanged( - /* playWhenReady= */ true, Player.PLAYBACK_SUPPRESSION_REASON_NONE); + /* playWhenReady= */ true, + Player.PLAY_WHEN_READY_CHANGE_REASON_USER_REQUEST, + Player.PLAYBACK_SUPPRESSION_REASON_NONE); assertThat(latch.await(TIMEOUT_MS, MILLISECONDS)).isTrue(); assertThat(playbackStateCompatRef.get().getState()) @@ -919,6 +923,7 @@ public class MediaControllerCompatCallbackWithMediaSessionTest { .getMockPlayer() .notifyPlayWhenReadyChanged( /* playWhenReady= */ true, + Player.PLAY_WHEN_READY_CHANGE_REASON_USER_REQUEST, Player.PLAYBACK_SUPPRESSION_REASON_TRANSIENT_AUDIO_FOCUS_LOSS); assertThat(latch.await(TIMEOUT_MS, MILLISECONDS)).isTrue(); @@ -969,6 +974,7 @@ public class MediaControllerCompatCallbackWithMediaSessionTest { .getMockPlayer() .notifyPlayWhenReadyChanged( /* playWhenReady= */ true, + Player.PLAY_WHEN_READY_CHANGE_REASON_USER_REQUEST, Player.PLAYBACK_SUPPRESSION_REASON_TRANSIENT_AUDIO_FOCUS_LOSS); assertThat(latch.await(TIMEOUT_MS, MILLISECONDS)).isTrue(); @@ -1015,7 +1021,9 @@ public class MediaControllerCompatCallbackWithMediaSessionTest { session .getMockPlayer() .notifyPlayWhenReadyChanged( - /* playWhenReady= */ true, Player.PLAYBACK_SUPPRESSION_REASON_NONE); + /* playWhenReady= */ true, + Player.PLAY_WHEN_READY_CHANGE_REASON_USER_REQUEST, + Player.PLAYBACK_SUPPRESSION_REASON_NONE); assertThat(latch.await(TIMEOUT_MS, MILLISECONDS)).isTrue(); assertThat(playbackStateCompatRef.get().getState()) diff --git a/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaControllerListenerTest.java b/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaControllerListenerTest.java index 1ba681287b..aa9ee66fe2 100644 --- a/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaControllerListenerTest.java +++ b/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaControllerListenerTest.java @@ -1454,7 +1454,9 @@ public class MediaControllerListenerTest { }; threadTestRule.getHandler().postAndSync(() -> controller.addListener(listener)); - remoteSession.getMockPlayer().notifyPlayWhenReadyChanged(testPlayWhenReady, testReason); + remoteSession + .getMockPlayer() + .notifyPlayWhenReadyChanged(testPlayWhenReady, testReason, testSuppressionReason); assertThat(latch.await(TIMEOUT_MS, MILLISECONDS)).isTrue(); assertThat(playWhenReadyParamRef.get()).isEqualTo(testPlayWhenReady); @@ -1469,6 +1471,44 @@ public class MediaControllerListenerTest { Player.EVENT_PLAY_WHEN_READY_CHANGED, Player.EVENT_PLAYBACK_SUPPRESSION_REASON_CHANGED); } + @Test + public void onPlayWhenReadyReasonChanged_isNotified() throws Exception { + remoteSession + .getMockPlayer() + .setPlayWhenReady( + /* playWhenReady= */ false, + Player.PLAYBACK_SUPPRESSION_REASON_TRANSIENT_AUDIO_FOCUS_LOSS); + MediaController controller = controllerTestRule.createController(remoteSession.getToken()); + CountDownLatch latch = new CountDownLatch(1); + AtomicBoolean playWhenReadyParamRef = new AtomicBoolean(); + AtomicBoolean playWhenReadyGetterRef = new AtomicBoolean(); + AtomicInteger playWhenReadyReasonParamRef = new AtomicInteger(); + Player.Listener listener = + new Player.Listener() { + @Override + public void onPlayWhenReadyChanged(boolean playWhenReady, int reason) { + playWhenReadyParamRef.set(playWhenReady); + playWhenReadyGetterRef.set(controller.getPlayWhenReady()); + playWhenReadyReasonParamRef.set(reason); + latch.countDown(); + } + }; + threadTestRule.getHandler().postAndSync(() -> controller.addListener(listener)); + + remoteSession + .getMockPlayer() + .notifyPlayWhenReadyChanged( + /* playWhenReady= */ false, + Player.PLAY_WHEN_READY_CHANGE_REASON_AUDIO_FOCUS_LOSS, + Player.PLAYBACK_SUPPRESSION_REASON_NONE); + + assertThat(latch.await(TIMEOUT_MS, MILLISECONDS)).isTrue(); + assertThat(playWhenReadyParamRef.get()).isFalse(); + assertThat(playWhenReadyGetterRef.get()).isFalse(); + assertThat(playWhenReadyReasonParamRef.get()) + .isEqualTo(Player.PLAY_WHEN_READY_CHANGE_REASON_AUDIO_FOCUS_LOSS); + } + @Test public void onPlayWhenReadyChanged_updatesGetters() throws Exception { boolean testPlayWhenReady = true; @@ -1550,7 +1590,10 @@ public class MediaControllerListenerTest { remoteSession.getMockPlayer().setTotalBufferedDuration(testTotalBufferedDurationMs); remoteSession.getMockPlayer().setCurrentLiveOffset(testCurrentLiveOffsetMs); remoteSession.getMockPlayer().setContentBufferedPosition(testContentBufferedPositionMs); - remoteSession.getMockPlayer().notifyPlayWhenReadyChanged(testPlayWhenReady, testReason); + remoteSession + .getMockPlayer() + .notifyPlayWhenReadyChanged( + testPlayWhenReady, Player.PLAY_WHEN_READY_CHANGE_REASON_USER_REQUEST, testReason); assertThat(latch.await(TIMEOUT_MS, MILLISECONDS)).isTrue(); assertThat(playWhenReadyRef.get()).isEqualTo(testPlayWhenReady); @@ -1604,7 +1647,10 @@ public class MediaControllerListenerTest { }; threadTestRule.getHandler().postAndSync(() -> controller.addListener(listener)); - remoteSession.getMockPlayer().notifyPlayWhenReadyChanged(testPlayWhenReady, testReason); + remoteSession + .getMockPlayer() + .notifyPlayWhenReadyChanged( + testPlayWhenReady, Player.PLAY_WHEN_READY_CHANGE_REASON_USER_REQUEST, testReason); assertThat(latch.await(TIMEOUT_MS, MILLISECONDS)).isTrue(); assertThat(playbackSuppressionReasonParamRef.get()).isEqualTo(testReason); @@ -1686,7 +1732,10 @@ public class MediaControllerListenerTest { remoteSession.getMockPlayer().setTotalBufferedDuration(testTotalBufferedDurationMs); remoteSession.getMockPlayer().setCurrentLiveOffset(testCurrentLiveOffsetMs); remoteSession.getMockPlayer().setContentBufferedPosition(testContentBufferedPositionMs); - remoteSession.getMockPlayer().notifyPlayWhenReadyChanged(testPlayWhenReady, testReason); + remoteSession + .getMockPlayer() + .notifyPlayWhenReadyChanged( + testPlayWhenReady, Player.PLAY_WHEN_READY_CHANGE_REASON_USER_REQUEST, testReason); assertThat(latch.await(TIMEOUT_MS, MILLISECONDS)).isTrue(); assertThat(playbackSuppressionReasonRef.get()).isEqualTo(testReason); diff --git a/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaSessionAndControllerTest.java b/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaSessionAndControllerTest.java index 280fca9da7..6202482ff0 100644 --- a/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaSessionAndControllerTest.java +++ b/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaSessionAndControllerTest.java @@ -225,7 +225,9 @@ public class MediaSessionAndControllerTest { .postAndSync( () -> player.notifyPlayWhenReadyChanged( - testPlayWhenReady, Player.PLAYBACK_SUPPRESSION_REASON_NONE)); + testPlayWhenReady, + Player.PLAY_WHEN_READY_CHANGE_REASON_USER_REQUEST, + Player.PLAYBACK_SUPPRESSION_REASON_NONE)); assertThat(latch.await(TIMEOUT_MS, MILLISECONDS)).isTrue(); assertThat(playWhenReadyRef.get()).isEqualTo(testPlayWhenReady); diff --git a/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaSessionKeyEventTest.java b/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaSessionKeyEventTest.java index b3429b9c8e..641b2c1213 100644 --- a/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaSessionKeyEventTest.java +++ b/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaSessionKeyEventTest.java @@ -101,7 +101,9 @@ public class MediaSessionKeyEventTest { handler.postAndSync( () -> { player.notifyPlayWhenReadyChanged( - /* playWhenReady= */ true, Player.PLAYBACK_SUPPRESSION_REASON_NONE); + /* playWhenReady= */ true, + Player.PLAY_WHEN_READY_CHANGE_REASON_USER_REQUEST, + Player.PLAYBACK_SUPPRESSION_REASON_NONE); player.notifyPlaybackStateChanged(Player.STATE_READY); }); } else { diff --git a/libraries/test_session_current/src/main/java/androidx/media3/session/MediaSessionProviderService.java b/libraries/test_session_current/src/main/java/androidx/media3/session/MediaSessionProviderService.java index 6557d4ec3c..708f239587 100644 --- a/libraries/test_session_current/src/main/java/androidx/media3/session/MediaSessionProviderService.java +++ b/libraries/test_session_current/src/main/java/androidx/media3/session/MediaSessionProviderService.java @@ -856,13 +856,17 @@ public class MediaSessionProviderService extends Service { @Override public void notifyPlayWhenReadyChanged( - String sessionId, boolean playWhenReady, @Player.PlaybackSuppressionReason int reason) + String sessionId, + boolean playWhenReady, + @Player.PlayWhenReadyChangeReason int playWhenReadyChangeReason, + @Player.PlaybackSuppressionReason int suppressionReason) throws RemoteException { runOnHandler( () -> { MediaSession session = sessionMap.get(sessionId); MockPlayer player = (MockPlayer) session.getPlayer(); - player.notifyPlayWhenReadyChanged(playWhenReady, reason); + player.notifyPlayWhenReadyChanged( + playWhenReady, playWhenReadyChangeReason, suppressionReason); }); } diff --git a/libraries/test_session_current/src/main/java/androidx/media3/session/MockPlayer.java b/libraries/test_session_current/src/main/java/androidx/media3/session/MockPlayer.java index 09a4e812ec..3c38eb587c 100644 --- a/libraries/test_session_current/src/main/java/androidx/media3/session/MockPlayer.java +++ b/libraries/test_session_current/src/main/java/androidx/media3/session/MockPlayer.java @@ -310,6 +310,7 @@ public class MockPlayer implements Player { public int deviceVolume; public boolean deviceMuted; public boolean playWhenReady; + public @PlayWhenReadyChangeReason int playWhenReadyChangeReason; public @PlaybackSuppressionReason int playbackSuppressionReason; public @State int playbackState; public boolean isLoading; @@ -404,7 +405,9 @@ public class MockPlayer implements Player { checkNotNull(conditionVariables.get(METHOD_PLAY)).open(); if (changePlayerStateWithTransportControl) { notifyPlayWhenReadyChanged( - /* playWhenReady= */ true, Player.PLAYBACK_SUPPRESSION_REASON_NONE); + /* playWhenReady= */ true, + Player.PLAY_WHEN_READY_CHANGE_REASON_USER_REQUEST, + Player.PLAYBACK_SUPPRESSION_REASON_NONE); } } @@ -413,7 +416,9 @@ public class MockPlayer implements Player { checkNotNull(conditionVariables.get(METHOD_PAUSE)).open(); if (changePlayerStateWithTransportControl) { notifyPlayWhenReadyChanged( - /* playWhenReady= */ false, Player.PLAYBACK_SUPPRESSION_REASON_NONE); + /* playWhenReady= */ false, + Player.PLAY_WHEN_READY_CHANGE_REASON_USER_REQUEST, + Player.PLAYBACK_SUPPRESSION_REASON_NONE); } } @@ -585,22 +590,26 @@ public class MockPlayer implements Player { * Player.Listener#onIsPlayingChanged} as appropriate. */ public void notifyPlayWhenReadyChanged( - boolean playWhenReady, @PlaybackSuppressionReason int playbackSuppressionReason) { - boolean playWhenReadyChanged = (this.playWhenReady != playWhenReady); + boolean playWhenReady, + @PlayWhenReadyChangeReason int playWhenReadyChangeReason, + @PlaybackSuppressionReason int playbackSuppressionReason) { + boolean playWhenReadyChanged = this.playWhenReady != playWhenReady; + boolean playWhenReadyReasonChanged = + this.playWhenReadyChangeReason != playWhenReadyChangeReason; boolean playbackSuppressionReasonChanged = - (this.playbackSuppressionReason != playbackSuppressionReason); - if (!playWhenReadyChanged && !playbackSuppressionReasonChanged) { + this.playbackSuppressionReason != playbackSuppressionReason; + if (!playWhenReadyChanged && !playbackSuppressionReasonChanged && !playWhenReadyReasonChanged) { return; } boolean wasPlaying = isPlaying(); this.playWhenReady = playWhenReady; + this.playWhenReadyChangeReason = playWhenReadyChangeReason; this.playbackSuppressionReason = playbackSuppressionReason; boolean isPlaying = isPlaying(); for (Listener listener : listeners) { - if (playWhenReadyChanged) { - listener.onPlayWhenReadyChanged( - playWhenReady, Player.PLAY_WHEN_READY_CHANGE_REASON_USER_REQUEST); + if (playWhenReadyChanged || playWhenReadyReasonChanged) { + listener.onPlayWhenReadyChanged(playWhenReady, playWhenReadyChangeReason); } if (playbackSuppressionReasonChanged) { listener.onPlaybackSuppressionReasonChanged(playbackSuppressionReason); diff --git a/libraries/test_session_current/src/main/java/androidx/media3/session/RemoteMediaSession.java b/libraries/test_session_current/src/main/java/androidx/media3/session/RemoteMediaSession.java index 70af785ff7..e7e8abd4b0 100644 --- a/libraries/test_session_current/src/main/java/androidx/media3/session/RemoteMediaSession.java +++ b/libraries/test_session_current/src/main/java/androidx/media3/session/RemoteMediaSession.java @@ -314,9 +314,12 @@ public class RemoteMediaSession { } public void notifyPlayWhenReadyChanged( - boolean playWhenReady, @Player.PlaybackSuppressionReason int reason) + boolean playWhenReady, + @Player.PlayWhenReadyChangeReason int playWhenReadyChangeReason, + @Player.PlaybackSuppressionReason int suppressionReason) throws RemoteException { - binder.notifyPlayWhenReadyChanged(sessionId, playWhenReady, reason); + binder.notifyPlayWhenReadyChanged( + sessionId, playWhenReady, playWhenReadyChangeReason, suppressionReason); } public void notifyPlaybackStateChanged(@Player.State int state) throws RemoteException {