From ee209690cb54fc800cf3a213f5075c9aaf51463c Mon Sep 17 00:00:00 2001 From: tonihei Date: Thu, 21 Jul 2022 10:10:22 +0000 Subject: [PATCH] Ensure pending commands are still sent in MediaController.release() We currently clear all pending messages, including the one that flushes pending commands to the MediaSession. To ensure all commands that have been called before controller.release() are still sent, we can manually trigger the flush message from the release call. Related to handling the final flush because disconnecting the controller, MediaSessionStub didn't post the removal of the controller to the session thread, creating a race condition between removing the controller and actually handling the flush. Issue: androidx/media#99 PiperOrigin-RevId: 462342860 --- RELEASENOTES.md | 2 + .../session/MediaControllerImplBase.java | 45 ++++++++++++------- .../media3/session/MediaSessionStub.java | 8 +++- .../MediaSessionAndControllerTest.java | 23 ++++++++++ 4 files changed, 61 insertions(+), 17 deletions(-) diff --git a/RELEASENOTES.md b/RELEASENOTES.md index 0a2d83b742..590bf23c0e 100644 --- a/RELEASENOTES.md +++ b/RELEASENOTES.md @@ -25,6 +25,8 @@ channel name used by the provider. Also, add method `DefaultNotificationProvider.setSmallIcon(int)` to set the notifications small icon ([#104](https://github.com/androidx/media/issues/104)). + * Ensure commands sent before `MediaController.release()` are not dropped + ([#99](https://github.com/androidx/media/issues/99)). ### 1.0.0-beta02 (2022-07-15) 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 46652a4036..df7d2e5b00 100644 --- a/libraries/session/src/main/java/androidx/media3/session/MediaControllerImplBase.java +++ b/libraries/session/src/main/java/androidx/media3/session/MediaControllerImplBase.java @@ -320,7 +320,7 @@ import org.checkerframework.checker.nullness.qual.NonNull; serviceConnection = null; } connectedToken = null; - flushCommandQueueHandler.removeCallbacksAndMessages(/* token= */ null); + flushCommandQueueHandler.release(); this.iSession = null; controllerStub.destroy(); if (iSession != null) { @@ -3070,30 +3070,43 @@ import org.checkerframework.checker.nullness.qual.NonNull; } } - private class FlushCommandQueueHandler extends Handler { + private class FlushCommandQueueHandler { private static final int MSG_FLUSH_COMMAND_QUEUE = 1; - public FlushCommandQueueHandler(Looper looper) { - super(looper); - } + private final Handler handler; - @Override - public void handleMessage(Message msg) { - if (msg.what == MSG_FLUSH_COMMAND_QUEUE) { - try { - iSession.flushCommandQueue(controllerStub); - } catch (RemoteException e) { - Log.w(TAG, "Error in sending flushCommandQueue"); - } - } + public FlushCommandQueueHandler(Looper looper) { + handler = new Handler(looper, /* callback= */ this::handleMessage); } public void sendFlushCommandQueueMessage() { - if (iSession != null && !hasMessages(MSG_FLUSH_COMMAND_QUEUE)) { + if (iSession != null && !handler.hasMessages(MSG_FLUSH_COMMAND_QUEUE)) { // Send message to notify the end of the transaction. It will be handled when the current // looper iteration is over. - sendEmptyMessage(MSG_FLUSH_COMMAND_QUEUE); + handler.sendEmptyMessage(MSG_FLUSH_COMMAND_QUEUE); + } + } + + public void release() { + if (handler.hasMessages(MSG_FLUSH_COMMAND_QUEUE)) { + flushCommandQueue(); + } + handler.removeCallbacksAndMessages(/* token= */ null); + } + + private boolean handleMessage(Message msg) { + if (msg.what == MSG_FLUSH_COMMAND_QUEUE) { + flushCommandQueue(); + } + return true; + } + + private void flushCommandQueue() { + try { + iSession.flushCommandQueue(controllerStub); + } catch (RemoteException e) { + Log.w(TAG, "Error in sending flushCommandQueue"); } } } 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 1f40a89885..4915c09de5 100644 --- a/libraries/session/src/main/java/androidx/media3/session/MediaSessionStub.java +++ b/libraries/session/src/main/java/androidx/media3/session/MediaSessionStub.java @@ -552,7 +552,13 @@ import java.util.concurrent.ExecutionException; } long token = Binder.clearCallingIdentity(); try { - connectedControllersManager.removeController(caller.asBinder()); + @Nullable MediaSessionImpl sessionImpl = this.sessionImpl.get(); + if (sessionImpl == null || sessionImpl.isReleased()) { + return; + } + postOrRun( + sessionImpl.getApplicationHandler(), + () -> connectedControllersManager.removeController(caller.asBinder())); } finally { Binder.restoreCallingIdentity(token); } 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 a31ea62039..c4def47aad 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,4 +225,27 @@ public class MediaSessionAndControllerTest { assertThat(latch.await(TIMEOUT_MS, MILLISECONDS)).isTrue(); assertThat(playWhenReadyRef.get()).isEqualTo(testPlayWhenReady); } + + @Test + public void commandBeforeControllerRelease_handledBySession() throws Exception { + MockPlayer player = + new MockPlayer.Builder().setApplicationLooper(Looper.getMainLooper()).build(); + MediaSession session = + sessionTestRule.ensureReleaseAfterTest( + new MediaSession.Builder(context, player).setId(TAG).build()); + MediaController controller = controllerTestRule.createController(session.getToken()); + + threadTestRule + .getHandler() + .postAndSync( + () -> { + controller.prepare(); + controller.play(); + controller.release(); + }); + + // Assert these methods are called without timing out. + player.awaitMethodCalled(MockPlayer.METHOD_PREPARE, TIMEOUT_MS); + player.awaitMethodCalled(MockPlayer.METHOD_PLAY, TIMEOUT_MS); + } }