diff --git a/RELEASENOTES.md b/RELEASENOTES.md index 0dd62f79c3..e9fe4b3056 100644 --- a/RELEASENOTES.md +++ b/RELEASENOTES.md @@ -139,6 +139,9 @@ * Fix bug where a `MediaBrowser` connected to a legacy browser service, didn't receive an error sent by the service after the browser has subscribed to a `parentid`. + * Improve interoperability behavior, so that a Media3 browser that is + connected to a legacy `MediaBrowserService` doesn't request the children + of a `parentId` twice when subscribing to a parent. * UI: * Make the stretched/cropped video in `PlayerView`-in-Compose-`AndroidView` workaround opt-in, due to issues diff --git a/libraries/session/src/main/java/androidx/media3/session/MediaBrowserImplLegacy.java b/libraries/session/src/main/java/androidx/media3/session/MediaBrowserImplLegacy.java index 1a852d5216..aab7bfd519 100644 --- a/libraries/session/src/main/java/androidx/media3/session/MediaBrowserImplLegacy.java +++ b/libraries/session/src/main/java/androidx/media3/session/MediaBrowserImplLegacy.java @@ -199,12 +199,54 @@ import org.checkerframework.checker.initialization.qual.UnderInitialization; return Futures.immediateFuture(LibraryResult.ofError(ERROR_SESSION_DISCONNECTED)); } - SettableFuture>> future = SettableFuture.create(); Bundle options = createOptionsWithPagingInfo(params, page, pageSize); - browserCompat.subscribe(parentId, options, new GetChildrenCallback(future, parentId)); + SettableFuture>> future = SettableFuture.create(); + // Try to get the cached children in case this is the first call right after subscribing. + List childrenFromCache = + getChildrenFromSubscription(parentId, page); + // Always evict to avoid memory leaks. We've done what we can. + evictChildrenFromSubscription(parentId); + if (childrenFromCache != null) { + future.set( + LibraryResult.ofItemList( + LegacyConversions.convertBrowserItemListToMediaItemList(childrenFromCache), + new LibraryParams.Builder().setExtras(options).build())); + } else { + GetChildrenCallback getChildrenCallback = new GetChildrenCallback(future, parentId); + browserCompat.subscribe(parentId, options, getChildrenCallback); + } return future; } + @Nullable + private List getChildrenFromSubscription( + String parentId, int page) { + List callbacks = subscribeCallbacks.get(parentId); + if (callbacks == null) { + return null; + } + for (int i = 0; i < callbacks.size(); i++) { + if (callbacks.get(i).canServeGetChildrenRequest(parentId, page)) { + return callbacks.get(i).receivedChildren; + } + } + return null; + } + + private void evictChildrenFromSubscription(String parentId) { + List callbacks = subscribeCallbacks.get(parentId); + if (callbacks == null) { + return; + } + for (int i = 0; i < callbacks.size(); i++) { + if (callbacks.get(i).receivedChildren != null) { + // Evict the first cached children we find. + callbacks.get(i).receivedChildren = null; + return; + } + } + } + @Override public ListenableFuture> getItem(String mediaId) { if (!getInstance().isSessionCommandAvailable(SessionCommand.COMMAND_CODE_LIBRARY_GET_ITEM)) { @@ -471,6 +513,8 @@ import org.checkerframework.checker.initialization.qual.UnderInitialization; private final Bundle subscriptionOptions; private final SettableFuture> future; + @Nullable private List receivedChildren; + public SubscribeCallback( String subscriptionParentId, Bundle subscriptionOptions, @@ -535,24 +579,43 @@ import org.checkerframework.checker.initialization.qual.UnderInitialization; // Browser is closed. return; } - int itemCount; - if (children != null) { - itemCount = children.size(); - } else { - // Currently no way to tell failures in MediaBrowser#subscribe(). + if (children == null) { + // Note this doesn't happen except possibly when someone is using a very old OS (change + // landed in Android tree at 2016-02-23). Recent Android versions turn children=null into an + // error and `onError` of this `SubscriptionCallback` is called instead of + // `onChildrenLoaded` (see `MediaBrowser.onLoadChildren`). We should do the same here to be + // consistent again. + onError(subscriptionParentId, subscriptionOptions); return; } LibraryParams params = LegacyConversions.convertToLibraryParams( context, browserCompat.getNotifyChildrenChangedOptions()); + receivedChildren = children; getInstance() .notifyBrowserListener( listener -> { - listener.onChildrenChanged(getInstance(), parentId, itemCount, params); + listener.onChildrenChanged(getInstance(), parentId, children.size(), params); }); future.set(LibraryResult.ofVoid()); } + + /** + * Returns true if the cached children can be served for a request for the given parent ID and + * paging options. + * + * @param parentId The media ID of the parent of the requested children. + * @param pageIndex The requested page index. + * @return True if the request can be served with the cached children of the subscription + * callback. + */ + public boolean canServeGetChildrenRequest(String parentId, int pageIndex) { + return subscriptionParentId.equals(parentId) + && receivedChildren != null + && pageIndex + == subscriptionOptions.getInt(MediaBrowserCompat.EXTRA_PAGE, /* defaultValue= */ 0); + } } private class GetChildrenCallback extends SubscriptionCallback { diff --git a/libraries/test_session_common/src/main/java/androidx/media3/test/session/common/MediaBrowserServiceCompatConstants.java b/libraries/test_session_common/src/main/java/androidx/media3/test/session/common/MediaBrowserServiceCompatConstants.java index b5d3314e83..f23a3cf181 100644 --- a/libraries/test_session_common/src/main/java/androidx/media3/test/session/common/MediaBrowserServiceCompatConstants.java +++ b/libraries/test_session_common/src/main/java/androidx/media3/test/session/common/MediaBrowserServiceCompatConstants.java @@ -25,6 +25,8 @@ public class MediaBrowserServiceCompatConstants { public static final String TEST_GET_CHILDREN = "getChildren_correctMetadataExtras"; public static final String TEST_GET_CHILDREN_WITH_NULL_LIST = "onChildrenChanged_withNullChildrenListInLegacyService_convertedToSessionError"; + public static final String TEST_GET_CHILDREN_INCREASE_NUMBER_OF_CHILDREN_WITH_EACH_CALL = + "onChildrenChanged_cacheChildrenOfSubscribeCall_serviceCalledOnceOnly"; public static final String TEST_GET_CHILDREN_FATAL_AUTHENTICATION_ERROR = "getLibraryRoot_fatalAuthenticationError_receivesPlaybackException"; public static final String TEST_GET_CHILDREN_NON_FATAL_AUTHENTICATION_ERROR = diff --git a/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaBrowserListenerWithMediaBrowserServiceCompatTest.java b/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaBrowserListenerWithMediaBrowserServiceCompatTest.java index 8892dfd505..10c6367778 100644 --- a/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaBrowserListenerWithMediaBrowserServiceCompatTest.java +++ b/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaBrowserListenerWithMediaBrowserServiceCompatTest.java @@ -29,6 +29,7 @@ import static androidx.media3.test.session.common.MediaBrowserConstants.ROOT_ID_ import static androidx.media3.test.session.common.MediaBrowserServiceCompatConstants.TEST_CONNECT_REJECTED; import static androidx.media3.test.session.common.MediaBrowserServiceCompatConstants.TEST_GET_CHILDREN; import static androidx.media3.test.session.common.MediaBrowserServiceCompatConstants.TEST_GET_CHILDREN_FATAL_AUTHENTICATION_ERROR; +import static androidx.media3.test.session.common.MediaBrowserServiceCompatConstants.TEST_GET_CHILDREN_INCREASE_NUMBER_OF_CHILDREN_WITH_EACH_CALL; import static androidx.media3.test.session.common.MediaBrowserServiceCompatConstants.TEST_GET_CHILDREN_NON_FATAL_AUTHENTICATION_ERROR; 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; @@ -380,6 +381,60 @@ public class MediaBrowserListenerWithMediaBrowserServiceCompatTest { .isEqualTo(SessionError.DEFAULT_ERROR_MESSAGE); } + @Test + public void onChildrenChanged_cacheChildrenOfSubscribeCall_serviceCalledOnceOnly() + throws Exception { + String testParentId = TEST_GET_CHILDREN_INCREASE_NUMBER_OF_CHILDREN_WITH_EACH_CALL; + remoteService.setProxyForTest(TEST_GET_CHILDREN_INCREASE_NUMBER_OF_CHILDREN_WITH_EACH_CALL); + CountDownLatch latch = new CountDownLatch(/* count= */ 1); + MediaBrowser browser = + createBrowser( + new MediaBrowser.Listener() { + @Override + public void onChildrenChanged( + MediaBrowser browser, + String parentId, + int itemCount, + @Nullable LibraryParams params) { + latch.countDown(); + } + }); + // Subscribing causes the first call to the legacy `onLoadChildren()` that we want to cache. + LibraryResult resultForSubscribe = + threadTestRule + .getHandler() + .postAndSync(() -> browser.subscribe(testParentId, null)) + .get(TIMEOUT_MS, MILLISECONDS); + assertThat(resultForSubscribe.resultCode).isEqualTo(RESULT_SUCCESS); + assertThat(latch.await(TIMEOUT_MS, MILLISECONDS)).isTrue(); + + LibraryResult> resultGetChildren = + threadTestRule + .getHandler() + .postAndSync( + () -> + browser.getChildren( + testParentId, /* page= */ 0, /* pageSize= */ 12, /* params= */ null)) + .get(TIMEOUT_MS, MILLISECONDS); + + assertThat(resultGetChildren.resultCode).isEqualTo(RESULT_SUCCESS); + // If caching in `MediaBrowserImplLegacy` doesn't work, the children would be delivered by a + // second call to the service which would have two items. + assertThat(resultGetChildren.value).hasSize(1); + + // Cache is cleared after delivery. We call the service again that now delivers two items. + LibraryResult> resultGetChildrenAgain = + threadTestRule + .getHandler() + .postAndSync( + () -> + browser.getChildren( + testParentId, /* page= */ 0, /* pageSize= */ 12, /* params= */ null)) + .get(TIMEOUT_MS, MILLISECONDS); + + assertThat(resultGetChildrenAgain.value).hasSize(2); + } + @Test public void getLibraryRoot_correctExtraKeyAndValue() throws Exception { remoteService.setProxyForTest(TEST_GET_LIBRARY_ROOT); diff --git a/libraries/test_session_current/src/main/java/androidx/media3/session/MockMediaBrowserServiceCompat.java b/libraries/test_session_current/src/main/java/androidx/media3/session/MockMediaBrowserServiceCompat.java index ce0d2b554f..a9ef2a8512 100644 --- a/libraries/test_session_current/src/main/java/androidx/media3/session/MockMediaBrowserServiceCompat.java +++ b/libraries/test_session_current/src/main/java/androidx/media3/session/MockMediaBrowserServiceCompat.java @@ -29,6 +29,7 @@ import static androidx.media3.test.session.common.MediaBrowserConstants.ROOT_ID_ import static androidx.media3.test.session.common.MediaBrowserServiceCompatConstants.TEST_CONNECT_REJECTED; import static androidx.media3.test.session.common.MediaBrowserServiceCompatConstants.TEST_GET_CHILDREN; import static androidx.media3.test.session.common.MediaBrowserServiceCompatConstants.TEST_GET_CHILDREN_FATAL_AUTHENTICATION_ERROR; +import static androidx.media3.test.session.common.MediaBrowserServiceCompatConstants.TEST_GET_CHILDREN_INCREASE_NUMBER_OF_CHILDREN_WITH_EACH_CALL; import static androidx.media3.test.session.common.MediaBrowserServiceCompatConstants.TEST_GET_CHILDREN_NON_FATAL_AUTHENTICATION_ERROR; 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; @@ -197,6 +198,7 @@ public class MockMediaBrowserServiceCompat extends MediaBrowserServiceCompat { return; } } + super.onLoadChildren(parentId, result, Bundle.EMPTY); } @Override @@ -302,6 +304,9 @@ public class MockMediaBrowserServiceCompat extends MediaBrowserServiceCompat { case TEST_SUBSCRIBE_THEN_REJECT_ON_LOAD_CHILDREN: setProxyForSubscribeAndRejectGetChildren(); break; + case TEST_GET_CHILDREN_INCREASE_NUMBER_OF_CHILDREN_WITH_EACH_CALL: + setProxyForGetChildrenIncreaseNumberOfChildrenWithEachCall(); + break; default: throw new IllegalArgumentException("Unknown testName: " + testName); } @@ -598,6 +603,31 @@ public class MockMediaBrowserServiceCompat extends MediaBrowserServiceCompat { } }); } + + private void setProxyForGetChildrenIncreaseNumberOfChildrenWithEachCall() { + setMediaBrowserServiceProxy( + new MockMediaBrowserServiceCompat.Proxy() { + private int callCount; + + @Override + public BrowserRoot onGetRoot( + String clientPackageName, int clientUid, @Nullable Bundle rootHints) { + return new BrowserRoot(ROOT_ID, ROOT_EXTRAS); + } + + @Override + public void onLoadChildren(String parentId, Result> result) { + super.onLoadChildren(parentId, result, Bundle.EMPTY); + } + + @Override + public void onLoadChildren( + String parentId, Result> result, Bundle options) { + result.sendResult(MediaTestUtils.createBrowserItems(callCount + 1)); + callCount++; + } + }); + } } private static ImmutableList createMediaItems() {