From 561b83e282dcaf04110ceb14e41393e6a6cb97b9 Mon Sep 17 00:00:00 2001 From: tonihei Date: Thu, 28 Jul 2022 11:10:36 +0000 Subject: [PATCH] Delay unbinding the controller until pending commands are handled. Once a controller bound to a MediaSessionService unbinds, this service may be destroyed immediately if this was the last bound controller and the service isn't started in the foreground. At the time of releasing the controller, there may still be pending commands that need to be handled by the session service. These commands may cause the session service to post a foreground notification to keep it alive. So to avoid the destruction of the service while these commands are still handled, we need to keep the controller bound. We also add a timeout in case the session tasks are never completed by the session implementation. In case the controller is destroyed, the unbinding happens automatically by the system. PiperOrigin-RevId: 463802220 (cherry picked from commit ac42b593cc3280b866d42cdc11d57bb031b39757) --- .../media3/session/MediaController.java | 12 +++- .../session/MediaControllerImplBase.java | 17 +++-- .../media3/session/MediaControllerStub.java | 19 +++--- .../session/SequencedFutureManager.java | 43 +++++++++++- libraries/test_session_current/build.gradle | 1 + .../session/MediaSessionServiceTest.java | 66 +++++++++++++++++++ .../session/MockMediaSessionService.java | 50 ++++++++++++++ 7 files changed, 189 insertions(+), 19 deletions(-) diff --git a/libraries/session/src/main/java/androidx/media3/session/MediaController.java b/libraries/session/src/main/java/androidx/media3/session/MediaController.java index 235ca6553f..7f09bd14ca 100644 --- a/libraries/session/src/main/java/androidx/media3/session/MediaController.java +++ b/libraries/session/src/main/java/androidx/media3/session/MediaController.java @@ -98,8 +98,10 @@ import org.checkerframework.checker.initialization.qual.UnderInitialization; * {@link MediaSession} in it. {@link MediaSessionService} will provide a session to connect. * *

When you're done, use {@link #releaseFuture(Future)} or {@link #release()} to clean up - * resources. This also helps session service to be destroyed when there's no controller associated - * with it. + * resources. This also helps the session service to be destroyed when there's no controller + * associated with it. Releasing the controller will still deliver all pending commands sent to the + * session and only unbind from the session service once these commands have been handled, or after + * a timeout of {@link #RELEASE_UNBIND_TIMEOUT_MS}. * *

Threading Model

* @@ -161,6 +163,12 @@ import org.checkerframework.checker.initialization.qual.UnderInitialization; */ public class MediaController implements Player { + /** + * The timeout for handling pending commands after calling {@link #release()}. If the timeout is + * reached, the controller is unbound from the session service even if commands are still pending. + */ + @UnstableApi public static final long RELEASE_UNBIND_TIMEOUT_MS = 30_000; + private static final String TAG = "MediaController"; private static final String WRONG_THREAD_ERROR_MESSAGE = 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 df7d2e5b00..aadc8a42d6 100644 --- a/libraries/session/src/main/java/androidx/media3/session/MediaControllerImplBase.java +++ b/libraries/session/src/main/java/androidx/media3/session/MediaControllerImplBase.java @@ -162,6 +162,8 @@ import org.checkerframework.checker.nullness.qual.NonNull; public static final String TAG = "MCImplBase"; + private static final long RELEASE_TIMEOUT_MS = 30_000; + private final MediaController instance; protected final SequencedFutureManager sequencedFutureManager; protected final MediaControllerStub controllerStub; @@ -315,14 +317,9 @@ import org.checkerframework.checker.nullness.qual.NonNull; return; } released = true; - if (serviceConnection != null) { - context.unbindService(serviceConnection); - serviceConnection = null; - } connectedToken = null; flushCommandQueueHandler.release(); this.iSession = null; - controllerStub.destroy(); if (iSession != null) { int seq = sequencedFutureManager.obtainNextSequenceNumber(); try { @@ -332,8 +329,16 @@ import org.checkerframework.checker.nullness.qual.NonNull; // No-op. } } - sequencedFutureManager.release(); listeners.release(); + sequencedFutureManager.lazyRelease( + RELEASE_TIMEOUT_MS, + () -> { + if (serviceConnection != null) { + context.unbindService(serviceConnection); + serviceConnection = null; + } + controllerStub.destroy(); + }); } @Override diff --git a/libraries/session/src/main/java/androidx/media3/session/MediaControllerStub.java b/libraries/session/src/main/java/androidx/media3/session/MediaControllerStub.java index 1e27fd8e65..9e9fa0fc62 100644 --- a/libraries/session/src/main/java/androidx/media3/session/MediaControllerStub.java +++ b/libraries/session/src/main/java/androidx/media3/session/MediaControllerStub.java @@ -29,6 +29,7 @@ import androidx.media3.common.util.Log; import androidx.media3.session.MediaLibraryService.LibraryParams; import java.lang.ref.WeakReference; import java.util.List; +import org.checkerframework.checker.nullness.qual.NonNull; /* package */ class MediaControllerStub extends IMediaController.Stub { @@ -41,7 +42,7 @@ import java.util.List; } @Override - public void onSessionResult(int seq, Bundle sessionResultBundle) { + public void onSessionResult(int sequenceNum, Bundle sessionResultBundle) { SessionResult result; try { result = SessionResult.CREATOR.fromBundle(sessionResultBundle); @@ -52,11 +53,11 @@ import java.util.List; // Don't post setting future result so the result can be obtained on the application looper. // For an example, {@code MediaController.setRating(rating).get()} wouldn't return if the // result is posted. - dispatchControllerTask(controller -> controller.setFutureResult(seq, result)); + setControllerFutureResult(sequenceNum, result); } @Override - public void onLibraryResult(int seq, Bundle libraryResultBundle) { + public void onLibraryResult(int sequenceNum, Bundle libraryResultBundle) { LibraryResult result; try { result = LibraryResult.UNKNOWN_TYPE_CREATOR.fromBundle(libraryResultBundle); @@ -67,8 +68,7 @@ import java.util.List; // Don't post setting future result so the result can be obtained on the application looper. // For an example, {@code MediaBrowser.getLibraryRoot(params).get()} wouldn't return if the // result is posted. - dispatchControllerTask( - (ControllerTask) browser -> browser.setFutureResult(seq, result)); + setControllerFutureResult(sequenceNum, result); } @Override @@ -246,16 +246,15 @@ import java.util.List; controller.clear(); } - private void dispatchControllerTask(ControllerTask task) { + private void setControllerFutureResult( + int sequenceNum, T futureResult) { long token = Binder.clearCallingIdentity(); try { @Nullable MediaControllerImplBase controller = this.controller.get(); - if (controller == null || controller.isReleased()) { + if (controller == null) { return; } - @SuppressWarnings("unchecked") - T castedController = (T) controller; - task.run(castedController); + controller.setFutureResult(sequenceNum, futureResult); } finally { Binder.restoreCallingIdentity(token); } diff --git a/libraries/session/src/main/java/androidx/media3/session/SequencedFutureManager.java b/libraries/session/src/main/java/androidx/media3/session/SequencedFutureManager.java index b468bee847..21fe8a0fa6 100644 --- a/libraries/session/src/main/java/androidx/media3/session/SequencedFutureManager.java +++ b/libraries/session/src/main/java/androidx/media3/session/SequencedFutureManager.java @@ -15,10 +15,14 @@ */ package androidx.media3.session; +import static androidx.media3.common.util.Assertions.checkNotNull; + +import android.os.Handler; import androidx.annotation.GuardedBy; import androidx.annotation.Nullable; import androidx.collection.ArrayMap; import androidx.media3.common.util.Log; +import androidx.media3.common.util.Util; import com.google.common.util.concurrent.AbstractFuture; import com.google.common.util.concurrent.ListenableFuture; import java.util.ArrayList; @@ -39,6 +43,17 @@ import org.checkerframework.checker.nullness.qual.NonNull; @GuardedBy("lock") private final ArrayMap> seqToFutureMap; + @GuardedBy("lock") + @Nullable + private Runnable pendingLazyReleaseCallback; + + @GuardedBy("lock") + @Nullable + private Handler releaseCallbackHandler; + + @GuardedBy("lock") + private boolean isReleased; + public SequencedFutureManager() { lock = new Object(); seqToFutureMap = new ArrayMap<>(); @@ -66,7 +81,11 @@ import org.checkerframework.checker.nullness.qual.NonNull; synchronized (lock) { int seq = obtainNextSequenceNumber(); SequencedFuture result = SequencedFuture.create(seq, resultWhenClosed); - seqToFutureMap.put(seq, result); + if (isReleased) { + result.setWithTheValueOfResultWhenClosed(); + } else { + seqToFutureMap.put(seq, result); + } return result; } } @@ -94,20 +113,42 @@ import org.checkerframework.checker.nullness.qual.NonNull; + result.getClass()); } } + if (pendingLazyReleaseCallback != null && seqToFutureMap.isEmpty()) { + release(); + } } } public void release() { List> pendingResults; synchronized (lock) { + isReleased = true; pendingResults = new ArrayList<>(seqToFutureMap.values()); seqToFutureMap.clear(); + if (pendingLazyReleaseCallback != null) { + checkNotNull(releaseCallbackHandler).post(pendingLazyReleaseCallback); + pendingLazyReleaseCallback = null; + releaseCallbackHandler = null; + } } for (SequencedFuture result : pendingResults) { result.setWithTheValueOfResultWhenClosed(); } } + public void lazyRelease(long timeoutMs, Runnable releaseCallback) { + synchronized (lock) { + Handler releaseCallbackHandler = Util.createHandlerForCurrentLooper(); + this.releaseCallbackHandler = releaseCallbackHandler; + pendingLazyReleaseCallback = releaseCallback; + if (seqToFutureMap.isEmpty()) { + release(); + } else { + releaseCallbackHandler.postDelayed(this::release, timeoutMs); + } + } + } + public static final class SequencedFuture extends AbstractFuture { private final int sequenceNumber; diff --git a/libraries/test_session_current/build.gradle b/libraries/test_session_current/build.gradle index 4d7ad668a4..429b89881a 100644 --- a/libraries/test_session_current/build.gradle +++ b/libraries/test_session_current/build.gradle @@ -38,6 +38,7 @@ dependencies { implementation project(modulePrefix + 'lib-session') implementation project(modulePrefix + 'test-session-common') implementation 'androidx.media:media:' + androidxMediaVersion + androidTestImplementation project(modulePrefix + 'lib-exoplayer') androidTestImplementation 'androidx.test.ext:junit:' + androidxTestJUnitVersion androidTestImplementation 'androidx.test.ext:truth:' + androidxTestTruthVersion androidTestImplementation 'androidx.test:core:' + androidxTestCoreVersion 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 fb6c95f615..6f83813127 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 @@ -25,8 +25,12 @@ import static org.junit.Assert.assertThrows; import android.content.ComponentName; import android.content.Context; import android.os.Bundle; +import android.os.Handler; import android.os.Looper; +import androidx.media3.common.MediaItem; import androidx.media3.common.Player; +import androidx.media3.common.util.ConditionVariable; +import androidx.media3.exoplayer.ExoPlayer; import androidx.media3.session.MediaSession.ControllerInfo; import androidx.media3.test.session.common.HandlerThreadTestRule; import androidx.media3.test.session.common.MainLooperTestRule; @@ -34,11 +38,14 @@ import androidx.media3.test.session.common.TestUtils; import androidx.test.core.app.ApplicationProvider; import androidx.test.ext.junit.runners.AndroidJUnit4; import androidx.test.filters.MediumTest; +import com.google.common.collect.ImmutableList; import com.google.common.util.concurrent.ListenableFuture; +import com.google.common.util.concurrent.SettableFuture; import java.util.ArrayList; import java.util.List; import java.util.concurrent.CountDownLatch; import java.util.concurrent.ExecutionException; +import java.util.concurrent.atomic.AtomicBoolean; import org.junit.After; import org.junit.Before; import org.junit.ClassRule; @@ -79,6 +86,65 @@ public class MediaSessionServiceTest { TestServiceRegistry.getInstance().cleanUp(); } + @Test + public void controllerRelease_keepsControllerBoundUntilCommandsHandled() throws Exception { + TestServiceRegistry testServiceRegistry = TestServiceRegistry.getInstance(); + ConditionVariable mediaItemsAdded = new ConditionVariable(); + AtomicBoolean controllerBoundWhenMediaItemsAdded = new AtomicBoolean(false); + testServiceRegistry.setOnGetSessionHandler( + controllerInfo -> { + // Save bound state at the point where media items are added and listeners are informed. + MockMediaSessionService service = + (MockMediaSessionService) testServiceRegistry.getServiceInstance(); + Player player = new ExoPlayer.Builder(service).build(); + player.addListener( + new Player.Listener() { + @Override + public void onEvents(Player player, Player.Events events) { + if (events.contains(Player.EVENT_TIMELINE_CHANGED) + && !player.getCurrentTimeline().isEmpty()) { + controllerBoundWhenMediaItemsAdded.set(service.hasBoundController()); + mediaItemsAdded.open(); + } + } + }); + // Add short delay for resolving media items. + return new MediaSession.Builder(service, player) + .setCallback( + new MediaSession.Callback() { + @Override + public ListenableFuture> onAddMediaItems( + MediaSession mediaSession, + ControllerInfo controller, + List mediaItems) { + SettableFuture> future = SettableFuture.create(); + MediaItem playableItem = + mediaItems.get(0).buildUpon().setUri("https://test.test").build(); + new Handler(Looper.myLooper()) + .postDelayed( + () -> future.set(ImmutableList.of(playableItem)), + /* delayMillis= */ 500); + return future; + } + }) + .build(); + }); + RemoteMediaController controller = + controllerTestRule.createRemoteController( + token, /* waitForConnection= */ true, /* connectionHints= */ null); + MockMediaSessionService service = + (MockMediaSessionService) testServiceRegistry.getServiceInstance(); + + // Add items and release controller immediately. + controller.addMediaItem(new MediaItem.Builder().setMediaId("media_id").build()); + controller.release(); + + // Assert controller is still bound when command is fully handled and unbound after that. + mediaItemsAdded.block(TIMEOUT_MS); + assertThat(controllerBoundWhenMediaItemsAdded.get()).isEqualTo(true); + service.blockUntilAllControllersUnbind(TIMEOUT_MS); + } + /** * Tests whether {@link MediaSessionService#onGetSession(ControllerInfo)} is called when * controller tries to connect, with the proper arguments. diff --git a/libraries/test_session_current/src/main/java/androidx/media3/session/MockMediaSessionService.java b/libraries/test_session_current/src/main/java/androidx/media3/session/MockMediaSessionService.java index 2ccf5acfc1..d3bc2aec82 100644 --- a/libraries/test_session_current/src/main/java/androidx/media3/session/MockMediaSessionService.java +++ b/libraries/test_session_current/src/main/java/androidx/media3/session/MockMediaSessionService.java @@ -17,11 +17,17 @@ package androidx.media3.session; import static androidx.media3.test.session.common.CommonConstants.SUPPORT_APP_PACKAGE_NAME; +import android.content.Intent; import android.os.HandlerThread; +import android.os.IBinder; import android.text.TextUtils; +import androidx.annotation.Nullable; +import androidx.media3.common.util.ConditionVariable; import androidx.media3.common.util.UnstableApi; import androidx.media3.common.util.Util; import androidx.media3.session.MediaSession.ControllerInfo; +import java.util.concurrent.TimeoutException; +import java.util.concurrent.atomic.AtomicInteger; /** A mock MediaSessionService */ @UnstableApi @@ -29,8 +35,37 @@ public class MockMediaSessionService extends MediaSessionService { /** ID of the session that this service will create. */ public static final String ID = "TestSession"; + private final AtomicInteger boundControllerCount; + public MediaSession session; + private HandlerThread handlerThread; + private ConditionVariable allControllersUnbound; + + public MockMediaSessionService() { + boundControllerCount = new AtomicInteger(/* initialValue= */ 0); + allControllersUnbound = new ConditionVariable(); + allControllersUnbound.open(); + } + + /** Returns whether at least one controller is bound to this service. */ + public boolean hasBoundController() { + return !allControllersUnbound.isOpen(); + } + + /** + * Blocks until all bound controllers unbind. + * + * @param timeoutMs The block timeout in milliseconds. + * @throws TimeoutException If the block timed out. + * @throws InterruptedException If the block was interrupted. + */ + public void blockUntilAllControllersUnbind(long timeoutMs) + throws TimeoutException, InterruptedException { + if (!allControllersUnbound.block(timeoutMs)) { + throw new TimeoutException(); + } + } @Override public void onCreate() { @@ -40,6 +75,21 @@ public class MockMediaSessionService extends MediaSessionService { handlerThread.start(); } + @Override + public IBinder onBind(@Nullable Intent intent) { + boundControllerCount.incrementAndGet(); + allControllersUnbound.close(); + return super.onBind(intent); + } + + @Override + public boolean onUnbind(Intent intent) { + if (boundControllerCount.decrementAndGet() == 0) { + allControllersUnbound.open(); + } + return super.onUnbind(intent); + } + @Override public void onDestroy() { super.onDestroy();