Fix sending custom commands with a media browser

When sending a custom command with `browser.sendCustomCommand` when connected to a legacy browser service, the custom command was delivered to `MediaSessionCompat.Callback.onCustomAction` instead of the service method `onCustomAction`. The difference is that the service version can return an async response with a bundle, while the session callback version doesn't have a return value. Hence, the service method was never called and it wasn't possible to send a reponse or signal an error back to the browser. The resulting `ListanableFuture` simply always immediately resolved to a success. This change overrides `ListenableFuture<SessionResult> sendCustomCommand(SessionCommand command, Bundle args)` in `MediaBrowserImplLegacy` to use the `MediaBrowserCompat` method to send instead of the `MediaControlleCompat` method that was used by the subclass `MediaControllerImplLegacy`. This involves the service callback instead of the session callback and enables `MediaBrowser` to get the actual return value from the legacy service. Issue: #1474 #cherrypick PiperOrigin-RevId: 676519314
androidx · Sep 19, 2024 · 6bda0da · 6bda0da
1 parent 3c5e764
commit 6bda0da
Show file tree

Hide file tree

Showing 6 changed files with 173 additions and 0 deletions.
diff --git a/RELEASENOTES.md b/RELEASENOTES.md
@@ -206,6 +206,18 @@ This release includes the following changes since the
         playback can't be suppressed without the system crashing the service
         with a `ForegroundServiceDidNotStartInTimeException`
         ([#1528](https://github.com/google/ExoPlayer/issues/1528)).
+    *   Fix bug that caused custom commands sent from a `MediaBrowser` being
+        dispatched to the `MediaSessionCompat.Callback` instead of the
+        `MediaBrowserServiceCompat` variant of the method when connected to a
+        legacy service. This prevented the `MediaBrowser` to receive the actual
+        return value sent back by the legacy service
+        ([#1474](https://github.com/androidx/media/issues/1474)).
+*   UI:
+*   Downloads:
+*   OkHttp Extension:
+*   Cronet Extension:
+*   RTMP Extension:
+*   HLS Extension:
 *   DASH Extension:
     *   Add support for periods starting in the middle of a segment
         ([#1440](https://github.com/androidx/media/issues/1440)).

diff --git a/libraries/session/src/main/java/androidx/media3/session/MediaBrowserImplLegacy.java b/libraries/session/src/main/java/androidx/media3/session/MediaBrowserImplLegacy.java
@@ -291,6 +291,35 @@ public void onError(String query, @Nullable Bundle extrasSent) {
     return future;
   }
 
+  @Override
+  public ListenableFuture<SessionResult> sendCustomCommand(SessionCommand command, Bundle args) {
+    MediaBrowserCompat browserCompat = getBrowserCompat();
+    if (browserCompat != null && instance.isSessionCommandAvailable(command)) {
+      SettableFuture<SessionResult> settable = SettableFuture.create();
+      browserCompat.sendCustomAction(
+          command.customAction,
+          args,
+          new MediaBrowserCompat.CustomActionCallback() {
+            @Override
+            public void onResult(
+                String action, @Nullable Bundle extras, @Nullable Bundle resultData) {
+              Bundle mergedBundles = new Bundle(extras);
+              mergedBundles.putAll(resultData);
+              settable.set(new SessionResult(SessionResult.RESULT_SUCCESS, mergedBundles));
+            }
+
+            @Override
+            public void onError(String action, @Nullable Bundle extras, @Nullable Bundle data) {
+              Bundle mergedBundles = new Bundle(extras);
+              mergedBundles.putAll(data);
+              settable.set(new SessionResult(SessionResult.RESULT_ERROR_UNKNOWN, mergedBundles));
+            }
+          });
+      return settable;
+    }
+    return super.sendCustomCommand(command, args);
+  }
+
   private MediaBrowserCompat getBrowserCompat(LibraryParams extras) {
     return browserCompats.get(extras);
   }

diff --git a/...ssion_common/src/main/java/androidx/media3/test/session/common/MediaBrowserConstants.java b/...ssion_common/src/main/java/androidx/media3/test/session/common/MediaBrowserConstants.java
@@ -29,6 +29,8 @@ public class MediaBrowserConstants {
   public static final String ROOT_EXTRAS_KEY = "root_extras_key";
   public static final int ROOT_EXTRAS_VALUE = 4321;
 
+  public static final String COMMAND_ACTION_PLAYLIST_ADD = "androidx.media3.actions.playlist_add";
+
   public static final String MEDIA_ID_GET_BROWSABLE_ITEM = "media_id_get_browsable_item";
   public static final String MEDIA_ID_GET_PLAYABLE_ITEM = "media_id_get_playable_item";
   public static final String MEDIA_ID_GET_ITEM_WITH_METADATA = "media_id_get_item_with_metadata";

diff --git a/...src/main/java/androidx/media3/test/session/common/MediaBrowserServiceCompatConstants.java b/...src/main/java/androidx/media3/test/session/common/MediaBrowserServiceCompatConstants.java
@@ -29,6 +29,7 @@ public class MediaBrowserServiceCompatConstants {
       "getLibraryRoot_fatalAuthenticationError_receivesPlaybackException";
   public static final String TEST_GET_CHILDREN_NON_FATAL_AUTHENTICATION_ERROR =
       "getLibraryRoot_nonFatalAuthenticationError_receivesPlaybackException";
+  public static final String TEST_SEND_CUSTOM_COMMAND = "sendCustomCommand";
 
   private MediaBrowserServiceCompatConstants() {}
 }
diff --git a/...t/java/androidx/media3/session/MediaBrowserListenerWithMediaBrowserServiceCompatTest.java b/...t/java/androidx/media3/session/MediaBrowserListenerWithMediaBrowserServiceCompatTest.java
@@ -32,8 +32,10 @@
 import static androidx.media3.test.session.common.MediaBrowserServiceCompatConstants.TEST_GET_CHILDREN_WITH_NULL_LIST;
 import static androidx.media3.test.session.common.MediaBrowserServiceCompatConstants.TEST_GET_LIBRARY_ROOT;
 import static androidx.media3.test.session.common.MediaBrowserServiceCompatConstants.TEST_ON_CHILDREN_CHANGED_SUBSCRIBE_AND_UNSUBSCRIBE;
+import static androidx.media3.test.session.common.MediaBrowserServiceCompatConstants.TEST_SEND_CUSTOM_COMMAND;
 import static androidx.media3.test.session.common.TestUtils.TIMEOUT_MS;
 import static com.google.common.truth.Truth.assertThat;
+import static com.google.common.util.concurrent.MoreExecutors.directExecutor;
 import static java.util.concurrent.TimeUnit.MILLISECONDS;
 import static org.junit.Assert.assertThrows;
 
@@ -46,14 +48,19 @@
 import androidx.media3.common.Player;
 import androidx.media3.session.MediaLibraryService.LibraryParams;
 import androidx.media3.test.session.common.HandlerThreadTestRule;
+import androidx.media3.test.session.common.MediaBrowserConstants;
 import androidx.test.core.app.ApplicationProvider;
 import androidx.test.ext.junit.runners.AndroidJUnit4;
 import androidx.test.filters.LargeTest;
 import com.google.common.collect.ImmutableList;
+import com.google.common.util.concurrent.FutureCallback;
+import com.google.common.util.concurrent.Futures;
+import com.google.common.util.concurrent.ListenableFuture;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.ExecutionException;
+import java.util.concurrent.atomic.AtomicReference;
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Rule;
@@ -336,4 +343,80 @@ public void onError(MediaController controller, SessionError sessionError) {
     assertThat(sessionErrors.get(0).code)
         .isEqualTo(PlaybackException.ERROR_CODE_AUTHENTICATION_EXPIRED);
   }
+
+  @Test
+  public void sendCustomCommand_success_correctAsyncResult() throws Exception {
+    remoteService.setProxyForTest(TEST_SEND_CUSTOM_COMMAND);
+    MediaBrowser browser = createBrowser(/* listener= */ null);
+    CountDownLatch latch = new CountDownLatch(/* count= */ 1);
+    AtomicReference<SessionResult> sessionResultRef = new AtomicReference<>();
+
+    ListenableFuture<SessionResult> resultFuture =
+        threadTestRule
+            .getHandler()
+            .postAndSync(
+                () ->
+                    browser.sendCustomCommand(
+                        new SessionCommand(
+                            MediaBrowserConstants.COMMAND_ACTION_PLAYLIST_ADD,
+                            /* extras= */ Bundle.EMPTY),
+                        /* args= */ Bundle.EMPTY));
+    Futures.addCallback(
+        resultFuture,
+        new FutureCallback<SessionResult>() {
+          @Override
+          public void onSuccess(SessionResult result) {
+            sessionResultRef.set(result);
+            latch.countDown();
+          }
+
+          @Override
+          public void onFailure(Throwable t) {}
+        },
+        directExecutor());
+
+    assertThat(latch.await(TIMEOUT_MS, MILLISECONDS)).isTrue();
+    assertThat(sessionResultRef.get()).isNotNull();
+    assertThat(sessionResultRef.get().resultCode).isEqualTo(SessionResult.RESULT_SUCCESS);
+    assertThat(sessionResultRef.get().extras.getString("key-1")).isEqualTo("success-from-service");
+  }
+
+  @Test
+  public void sendCustomCommand_failure_correctAsyncResult() throws Exception {
+    remoteService.setProxyForTest(TEST_SEND_CUSTOM_COMMAND);
+    MediaBrowser browser = createBrowser(/* listener= */ null);
+    CountDownLatch latch = new CountDownLatch(/* count= */ 1);
+    AtomicReference<SessionResult> sessionResultRef = new AtomicReference<>();
+    Bundle args = new Bundle();
+    args.putBoolean("request_error", true);
+
+    ListenableFuture<SessionResult> resultFuture =
+        threadTestRule
+            .getHandler()
+            .postAndSync(
+                () ->
+                    browser.sendCustomCommand(
+                        new SessionCommand(
+                            MediaBrowserConstants.COMMAND_ACTION_PLAYLIST_ADD,
+                            /* extras= */ Bundle.EMPTY),
+                        args));
+    Futures.addCallback(
+        resultFuture,
+        new FutureCallback<SessionResult>() {
+          @Override
+          public void onSuccess(SessionResult result) {
+            sessionResultRef.set(result);
+            latch.countDown();
+          }
+
+          @Override
+          public void onFailure(Throwable t) {}
+        },
+        directExecutor());
+
+    assertThat(latch.await(TIMEOUT_MS, MILLISECONDS)).isTrue();
+    assertThat(sessionResultRef.get()).isNotNull();
+    assertThat(sessionResultRef.get().resultCode).isEqualTo(SessionResult.RESULT_ERROR_UNKNOWN);
+    assertThat(sessionResultRef.get().extras.getString("key-1")).isEqualTo("error-from-service");
+  }
 }
diff --git a/..._session_current/src/main/java/androidx/media3/session/MockMediaBrowserServiceCompat.java b/..._session_current/src/main/java/androidx/media3/session/MockMediaBrowserServiceCompat.java
@@ -30,6 +30,7 @@
 import static androidx.media3.test.session.common.MediaBrowserServiceCompatConstants.TEST_GET_CHILDREN_WITH_NULL_LIST;
 import static androidx.media3.test.session.common.MediaBrowserServiceCompatConstants.TEST_GET_LIBRARY_ROOT;
 import static androidx.media3.test.session.common.MediaBrowserServiceCompatConstants.TEST_ON_CHILDREN_CHANGED_SUBSCRIBE_AND_UNSUBSCRIBE;
+import static androidx.media3.test.session.common.MediaBrowserServiceCompatConstants.TEST_SEND_CUSTOM_COMMAND;
 
 import android.content.Intent;
 import android.net.Uri;
@@ -46,6 +47,7 @@
 import androidx.annotation.Nullable;
 import androidx.media.MediaBrowserServiceCompat;
 import androidx.media3.test.session.common.IRemoteMediaBrowserServiceCompat;
+import androidx.media3.test.session.common.MediaBrowserConstants;
 import androidx.media3.test.session.common.MediaBrowserServiceCompatConstants;
 import com.google.common.collect.ImmutableList;
 import java.lang.reflect.Method;
@@ -271,6 +273,9 @@ public void setProxyForTest(String testName) throws RemoteException {
         case TEST_GET_CHILDREN_NON_FATAL_AUTHENTICATION_ERROR:
           getChildren_authenticationError_receivesPlaybackException(session, /* isFatal= */ false);
           break;
+        case TEST_SEND_CUSTOM_COMMAND:
+          setProxyForTestSendCustomCommand();
+          break;
         default:
           throw new IllegalArgumentException("Unknown testName: " + testName);
       }
@@ -374,6 +379,47 @@ public void onLoadChildren(
           });
     }
 
+    private void setProxyForTestSendCustomCommand() {
+      setMediaBrowserServiceProxy(
+          new MockMediaBrowserServiceCompat.Proxy() {
+            @Override
+            public BrowserRoot onGetRoot(
+                String clientPackageName, int clientUid, Bundle rootHints) {
+              session.setPlaybackState(
+                  new PlaybackStateCompat.Builder()
+                      .setState(
+                          PlaybackStateCompat.STATE_PLAYING,
+                          /* position= */ 123L,
+                          /* playbackSpeed= */ 1.0f)
+                      .addCustomAction(
+                          new PlaybackStateCompat.CustomAction.Builder(
+                                  MediaBrowserConstants.COMMAND_ACTION_PLAYLIST_ADD,
+                                  "Add to playlist",
+                                  CommandButton.ICON_PLAYLIST_ADD)
+                              .build())
+                      .build());
+
+              return new BrowserRoot(ROOT_ID, Bundle.EMPTY);
+            }
+
+            @Override
+            public void onCustomAction(String action, Bundle extras, Result<Bundle> result) {
+              Bundle resultBundle = new Bundle();
+              if (action.equals(MediaBrowserConstants.COMMAND_ACTION_PLAYLIST_ADD)) {
+                if (extras.getBoolean("request_error", /* defaultValue= */ false)) {
+                  resultBundle.putString("key-1", "error-from-service");
+                  result.sendError(resultBundle);
+                } else {
+                  resultBundle.putString("key-1", "success-from-service");
+                  result.sendResult(resultBundle);
+                }
+              } else {
+                result.sendError(resultBundle);
+              }
+            }
+          });
+    }
+
     private void setProxyForTestGetLibraryRoot_correctExtraKeyAndValue() {
       setMediaBrowserServiceProxy(
           new MockMediaBrowserServiceCompat.Proxy() {