From 2f242a05933f3a3a0a7747d90fbe305f7d856993 Mon Sep 17 00:00:00 2001 From: nift4 Date: Fri, 25 Jul 2025 17:36:05 +0200 Subject: [PATCH 1/2] Add failing unit test --- .../session/MediaSessionServiceTest.java | 69 +++++++++++++++++++ 1 file changed, 69 insertions(+) diff --git a/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaSessionServiceTest.java b/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaSessionServiceTest.java index 96fbee105a..f95496ac6b 100644 --- a/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaSessionServiceTest.java +++ b/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaSessionServiceTest.java @@ -28,14 +28,17 @@ import android.content.Context; import android.os.Bundle; import android.os.Handler; +import android.os.HandlerThread; import android.os.Looper; import android.service.notification.StatusBarNotification; import android.support.v4.media.session.MediaControllerCompat; import android.support.v4.media.session.MediaSessionCompat; import android.support.v4.media.session.PlaybackStateCompat; import androidx.annotation.Nullable; +import androidx.core.content.ContextCompat; import androidx.media3.common.ForwardingPlayer; import androidx.media3.common.MediaItem; +import androidx.media3.common.PlaybackParameters; import androidx.media3.common.Player; import androidx.media3.common.util.ConditionVariable; import androidx.media3.exoplayer.ExoPlayer; @@ -51,6 +54,8 @@ import androidx.test.filters.MediumTest; import com.google.common.collect.ImmutableList; import com.google.common.collect.Lists; +import com.google.common.util.concurrent.FutureCallback; +import com.google.common.util.concurrent.Futures; import com.google.common.util.concurrent.ListenableFuture; import com.google.common.util.concurrent.SettableFuture; import java.util.ArrayList; @@ -248,6 +253,70 @@ public ListenableFuture> onAddMediaItems( service.blockUntilAllControllersUnbind(TIMEOUT_MS); } + @Test + public void onPlayRequested_doesNotCauseDeadlock_ifPlaybackThreadIsNotMain() throws Exception { + HandlerThread ht = new HandlerThread("MSSTest:PlaybackThread"); + ht.start(); + TestServiceRegistry testServiceRegistry = TestServiceRegistry.getInstance(); + ConditionVariable playerToldToSpeedUp = new ConditionVariable(); + AtomicReference mediaController = new AtomicReference<>(); + AtomicReference mediaSession = new AtomicReference<>(); + testServiceRegistry.setOnGetSessionHandler( + controllerInfo -> { + MockMediaSessionService service = + (MockMediaSessionService) testServiceRegistry.getServiceInstance(); + Player player = new ExoPlayer.Builder(service).setLooper(ht.getLooper()).build(); + player.addListener( + new Player.Listener() { + @Override + public void onPlaybackParametersChanged(PlaybackParameters playbackParameters) { + if (playbackParameters.speed == 2f) { + playerToldToSpeedUp.open(); + } + } + }); + mediaSession.set(new MediaSession.Builder(service, player).build()); + return mediaSession.get(); + }); + TestHandler handler = new TestHandler(Looper.getMainLooper()); + TestHandler bgHandler = new TestHandler(ht.getLooper()); + handler.post( + () -> { + ListenableFuture controllerFuture = + new MediaController.Builder(context, token).buildAsync(); + Futures.addCallback( + controllerFuture, + new FutureCallback() { + @Override + public void onSuccess(MediaController controller) { + controller.addMediaItem(new MediaItem.Builder().setMediaId("media_id").build()); + controller.play(); + bgHandler.post( + () -> + handler.post( + () -> { + controller.setPlaybackSpeed(2f); + mediaController.set(controller); + })); + } + + @Override + public void onFailure(Throwable t) { + throw new RuntimeException(t); + } + }, + ContextCompat.getMainExecutor(context)); + }); + playerToldToSpeedUp.block(TIMEOUT_MS); + if (!playerToldToSpeedUp.isOpen()) { + ht.interrupt(); // avoid deadlocking the test forever. + } + assertThat(playerToldToSpeedUp.isOpen()).isTrue(); + handler.postAndSync(() -> mediaController.get().release()); + mediaSession.get().release(); + ht.quitSafely(); + } + @Test public void onCreate_withCustomLayout_correctSessionStateFromOnConnect() throws Exception { SessionCommand command1 = new SessionCommand("command1", Bundle.EMPTY); From d70bfc0f1fad6a447c4bf8099079d94043220e4d Mon Sep 17 00:00:00 2001 From: nift4 Date: Sat, 22 Mar 2025 11:48:37 +0100 Subject: [PATCH 2/2] session: fix a deadlock due to onPlayRequested() Issue: #1197 --- .../media3/session/MediaSessionImpl.java | 190 ++++++++++-------- .../session/MediaSessionLegacyStub.java | 24 ++- .../media3/session/MediaSessionStub.java | 13 +- 3 files changed, 131 insertions(+), 96 deletions(-) diff --git a/libraries/session/src/main/java/androidx/media3/session/MediaSessionImpl.java b/libraries/session/src/main/java/androidx/media3/session/MediaSessionImpl.java index 82a7654ed7..9ed82974f1 100644 --- a/libraries/session/src/main/java/androidx/media3/session/MediaSessionImpl.java +++ b/libraries/session/src/main/java/androidx/media3/session/MediaSessionImpl.java @@ -1099,20 +1099,23 @@ protected MediaSessionServiceLegacyStub getLegacyBrowserService() { }); } - /* package */ boolean onPlayRequested() { + /* package */ ListenableFuture onPlayRequested() { if (Looper.myLooper() != Looper.getMainLooper()) { SettableFuture playRequested = SettableFuture.create(); - mainHandler.post(() -> playRequested.set(onPlayRequested())); - try { - return playRequested.get(); - } catch (InterruptedException | ExecutionException e) { - throw new IllegalStateException(e); - } + mainHandler.post( + () -> { + try { + playRequested.set(onPlayRequested().get()); + } catch (ExecutionException | InterruptedException e) { + playRequested.setException(new IllegalStateException(e)); + } + }); + return playRequested; } if (this.mediaSessionListener != null) { - return this.mediaSessionListener.onPlayRequested(instance); + return Futures.immediateFuture(this.mediaSessionListener.onPlayRequested(instance)); } - return true; + return Futures.immediateFuture(true); } /** @@ -1123,84 +1126,103 @@ protected MediaSessionServiceLegacyStub getLegacyBrowserService() { * * @param controller The controller requesting to play. */ - /* package */ void handleMediaControllerPlayRequest( + /* package */ ListenableFuture handleMediaControllerPlayRequest( ControllerInfo controller, boolean callOnPlayerInteractionFinished) { - if (!onPlayRequested()) { - // Request denied, e.g. due to missing foreground service abilities. - return; - } - boolean hasCurrentMediaItem = - playerWrapper.isCommandAvailable(Player.COMMAND_GET_CURRENT_MEDIA_ITEM) - && playerWrapper.getCurrentMediaItem() != null; - boolean canAddMediaItems = - playerWrapper.isCommandAvailable(COMMAND_SET_MEDIA_ITEM) - || playerWrapper.isCommandAvailable(COMMAND_CHANGE_MEDIA_ITEMS); - ControllerInfo controllerForRequest = resolveControllerInfoForCallback(controller); - Player.Commands playCommand = - new Player.Commands.Builder().add(Player.COMMAND_PLAY_PAUSE).build(); - if (hasCurrentMediaItem || !canAddMediaItems) { - // No playback resumption needed or possible. - if (!hasCurrentMediaItem) { - Log.w( - TAG, - "Play requested without current MediaItem, but playback resumption prevented by" - + " missing available commands"); - } - Util.handlePlayButtonAction(playerWrapper); - if (callOnPlayerInteractionFinished) { - onPlayerInteractionFinishedOnHandler(controllerForRequest, playCommand); - } - } else { - @Nullable - ListenableFuture future = - checkNotNull( - callback.onPlaybackResumption( - instance, controllerForRequest, /* isForPlayback= */ true), - "Callback.onPlaybackResumption must return a non-null future"); - Futures.addCallback( - future, - new FutureCallback() { - @Override - public void onSuccess(MediaItemsWithStartPosition mediaItemsWithStartPosition) { - callWithControllerForCurrentRequestSet( - controllerForRequest, - () -> { - MediaUtils.setMediaItemsWithStartIndexAndPosition( - playerWrapper, mediaItemsWithStartPosition); - Util.handlePlayButtonAction(playerWrapper); - if (callOnPlayerInteractionFinished) { - onPlayerInteractionFinishedOnHandler(controllerForRequest, playCommand); - } - }) - .run(); + SettableFuture sessionFuture = SettableFuture.create(); + ListenableFuture playRequestedFuture = onPlayRequested(); + playRequestedFuture.addListener( + () -> { + boolean playRequested; + try { + playRequested = playRequestedFuture.get(); + } catch (ExecutionException | InterruptedException e) { + sessionFuture.setException(new IllegalStateException(e)); + return; + } + if (!playRequested) { + // Request denied, e.g. due to missing foreground service abilities. + sessionFuture.set(new SessionResult(SessionResult.RESULT_ERROR_UNKNOWN)); + return; + } + boolean hasCurrentMediaItem = + playerWrapper.isCommandAvailable(Player.COMMAND_GET_CURRENT_MEDIA_ITEM) + && playerWrapper.getCurrentMediaItem() != null; + boolean canAddMediaItems = + playerWrapper.isCommandAvailable(COMMAND_SET_MEDIA_ITEM) + || playerWrapper.isCommandAvailable(COMMAND_CHANGE_MEDIA_ITEMS); + ControllerInfo controllerForRequest = resolveControllerInfoForCallback(controller); + Player.Commands playCommand = + new Player.Commands.Builder().add(Player.COMMAND_PLAY_PAUSE).build(); + if (hasCurrentMediaItem || !canAddMediaItems) { + // No playback resumption needed or possible. + if (!hasCurrentMediaItem) { + Log.w( + TAG, + "Play requested without current MediaItem, but playback resumption prevented by" + + " missing available commands"); } - - @Override - public void onFailure(Throwable t) { - if (t instanceof UnsupportedOperationException) { - Log.w( - TAG, - "UnsupportedOperationException: Make sure to implement" - + " MediaSession.Callback.onPlaybackResumption() if you add a" - + " media button receiver to your manifest or if you implement the recent" - + " media item contract with your MediaLibraryService.", - t); - } else { - Log.e( - TAG, - "Failure calling MediaSession.Callback.onPlaybackResumption(): " - + t.getMessage(), - t); - } - // Play as requested even if playback resumption fails. - Util.handlePlayButtonAction(playerWrapper); - if (callOnPlayerInteractionFinished) { - onPlayerInteractionFinishedOnHandler(controllerForRequest, playCommand); - } + Util.handlePlayButtonAction(playerWrapper); + sessionFuture.set(new SessionResult(SessionResult.RESULT_SUCCESS)); + if (callOnPlayerInteractionFinished) { + onPlayerInteractionFinishedOnHandler(controllerForRequest, playCommand); } - }, - this::postOrRunOnApplicationHandler); - } + } else { + @Nullable + ListenableFuture future = + checkNotNull( + callback.onPlaybackResumption( + instance, controllerForRequest, /* isForPlayback= */ true), + "Callback.onPlaybackResumption must return a non-null future"); + Futures.addCallback( + future, + new FutureCallback() { + @Override + public void onSuccess(MediaItemsWithStartPosition mediaItemsWithStartPosition) { + callWithControllerForCurrentRequestSet( + controllerForRequest, + () -> { + MediaUtils.setMediaItemsWithStartIndexAndPosition( + playerWrapper, mediaItemsWithStartPosition); + Util.handlePlayButtonAction(playerWrapper); + sessionFuture.set(new SessionResult(SessionResult.RESULT_SUCCESS)); + if (callOnPlayerInteractionFinished) { + onPlayerInteractionFinishedOnHandler( + controllerForRequest, playCommand); + } + }) + .run(); + } + + @Override + public void onFailure(Throwable t) { + if (t instanceof UnsupportedOperationException) { + Log.w( + TAG, + "UnsupportedOperationException: Make sure to implement" + + " MediaSession.Callback.onPlaybackResumption() if you add a media" + + " button receiver to your manifest or if you implement the recent" + + " media item contract with your MediaLibraryService.", + t); + } else { + Log.e( + TAG, + "Failure calling MediaSession.Callback.onPlaybackResumption(): " + + t.getMessage(), + t); + } + // Play as requested even if playback resumption fails. + Util.handlePlayButtonAction(playerWrapper); + sessionFuture.set(new SessionResult(SessionResult.RESULT_SUCCESS)); + if (callOnPlayerInteractionFinished) { + onPlayerInteractionFinishedOnHandler(controllerForRequest, playCommand); + } + } + }, + this::postOrRunOnApplicationHandler); + } + }, + this::postOrRunOnApplicationHandler); + return sessionFuture; } /* package */ void triggerPlayerInfoUpdate() { diff --git a/libraries/session/src/main/java/androidx/media3/session/MediaSessionLegacyStub.java b/libraries/session/src/main/java/androidx/media3/session/MediaSessionLegacyStub.java index dd118c105b..c6ffdf9cc1 100644 --- a/libraries/session/src/main/java/androidx/media3/session/MediaSessionLegacyStub.java +++ b/libraries/session/src/main/java/androidx/media3/session/MediaSessionLegacyStub.java @@ -607,9 +607,27 @@ public void onPrepareFromUri(@Nullable Uri mediaUri, @Nullable Bundle extras) { public void onPlay() { dispatchSessionTaskWithPlayerCommand( COMMAND_PLAY_PAUSE, - controller -> - sessionImpl.handleMediaControllerPlayRequest( - controller, /* callOnPlayerInteractionFinished= */ true), + controller -> { + ListenableFuture resultFuture = + sessionImpl.handleMediaControllerPlayRequest( + controller, /* callOnPlayerInteractionFinished= */ true); + Futures.addCallback( + resultFuture, + new FutureCallback() { + @Override + public void onSuccess(SessionResult result) { + if (result.resultCode != RESULT_SUCCESS) { + Log.w(TAG, "onPlay() failed: " + result + " (from: " + controller + ")"); + } + } + + @Override + public void onFailure(Throwable t) { + Log.e(TAG, "Unexpected exception in onPlay() of " + controller, t); + } + }, + MoreExecutors.directExecutor()); + }, sessionCompat.getCurrentControllerInfo(), /* callOnPlayerInteractionFinished= */ false); } diff --git a/libraries/session/src/main/java/androidx/media3/session/MediaSessionStub.java b/libraries/session/src/main/java/androidx/media3/session/MediaSessionStub.java index 6fe89ce3ac..a2faff6a4d 100644 --- a/libraries/session/src/main/java/androidx/media3/session/MediaSessionStub.java +++ b/libraries/session/src/main/java/androidx/media3/session/MediaSessionStub.java @@ -761,15 +761,10 @@ public void playForControllerInfo(ControllerInfo controller, int sequenceNumber) controller, sequenceNumber, COMMAND_PLAY_PAUSE, - sendSessionResultSuccess( - player -> { - @Nullable MediaSessionImpl impl = sessionImpl.get(); - if (impl == null || impl.isReleased()) { - return; - } - impl.handleMediaControllerPlayRequest( - controller, /* callOnPlayerInteractionFinished= */ false); - })); + sendSessionResultWhenReady( + (session, theController, sequenceId) -> + session.handleMediaControllerPlayRequest( + theController, /* callOnPlayerInteractionFinished= */ false))); } @Override