Call onChildrenChanged to close the legacy subscription error path

When receiving an error from a legacy `MediaBrowserService` after
having successfully subscribed to a given `parentId`, the callback
needs to be asked to load the children again to receive an error
from the service.

Before this change such an error was dropped as a no-op by Media3
without the `MediaBrowser` giving a chance to react on such an error
being sent by the legacy service.

#cherrypick

PiperOrigin-RevId: 686052969
This commit is contained in:
bachinger 2024-10-15 04:36:00 -07:00 committed by Copybara-Service
parent 407bd49ed5
commit 5a827829b0
8 changed files with 199 additions and 19 deletions

View file

@ -134,6 +134,9 @@
`MediaBrowserCompat` when connecting to a legacy `MediaBrowserCompat`.
The service can receive the connection hints passed in as root hints
with the first call to `onGetRoot()`.
* 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`.
* UI:
* Make the stretched/cropped video in
`PlayerView`-in-Compose-`AndroidView` workaround opt-in, due to issues

View file

@ -151,15 +151,16 @@ import org.checkerframework.checker.initialization.qual.UnderInitialization;
if (browserCompat == null) {
return Futures.immediateFuture(LibraryResult.ofError(ERROR_SESSION_DISCONNECTED));
}
Bundle options = createOptionsForSubscription(params);
SettableFuture<LibraryResult<Void>> future = SettableFuture.create();
SubscribeCallback callback = new SubscribeCallback(future);
SubscribeCallback callback = new SubscribeCallback(parentId, options, future);
List<SubscribeCallback> list = subscribeCallbacks.get(parentId);
if (list == null) {
list = new ArrayList<>();
subscribeCallbacks.put(parentId, list);
}
list.add(callback);
browserCompat.subscribe(parentId, createOptions(params), callback);
browserCompat.subscribe(parentId, options, callback);
return future;
}
@ -199,7 +200,7 @@ import org.checkerframework.checker.initialization.qual.UnderInitialization;
}
SettableFuture<LibraryResult<ImmutableList<MediaItem>>> future = SettableFuture.create();
Bundle options = createOptions(params, page, pageSize);
Bundle options = createOptionsWithPagingInfo(params, page, pageSize);
browserCompat.subscribe(parentId, options, new GetChildrenCallback(future, parentId));
return future;
}
@ -295,7 +296,7 @@ import org.checkerframework.checker.initialization.qual.UnderInitialization;
}
SettableFuture<LibraryResult<ImmutableList<MediaItem>>> future = SettableFuture.create();
Bundle options = createOptions(params, page, pageSize);
Bundle options = createOptionsWithPagingInfo(params, page, pageSize);
options.putInt(MediaBrowserCompat.EXTRA_PAGE, page);
options.putInt(MediaBrowserCompat.EXTRA_PAGE_SIZE, pageSize);
browserCompat.search(
@ -367,12 +368,13 @@ import org.checkerframework.checker.initialization.qual.UnderInitialization;
return browserCompats.get(extras);
}
private static Bundle createOptions(@Nullable LibraryParams params) {
private static Bundle createOptionsForSubscription(@Nullable LibraryParams params) {
return params == null ? new Bundle() : new Bundle(params.extras);
}
private static Bundle createOptions(@Nullable LibraryParams params, int page, int pageSize) {
Bundle options = createOptions(params);
private static Bundle createOptionsWithPagingInfo(
@Nullable LibraryParams params, int page, int pageSize) {
Bundle options = createOptionsForSubscription(params);
options.putInt(MediaBrowserCompat.EXTRA_PAGE, page);
options.putInt(MediaBrowserCompat.EXTRA_PAGE_SIZE, pageSize);
return options;
@ -465,26 +467,33 @@ import org.checkerframework.checker.initialization.qual.UnderInitialization;
private class SubscribeCallback extends SubscriptionCallback {
private final String subscriptionParentId;
private final Bundle subscriptionOptions;
private final SettableFuture<LibraryResult<Void>> future;
public SubscribeCallback(SettableFuture<LibraryResult<Void>> future) {
public SubscribeCallback(
String subscriptionParentId,
Bundle subscriptionOptions,
SettableFuture<LibraryResult<Void>> future) {
this.subscriptionParentId = subscriptionParentId;
this.subscriptionOptions = subscriptionOptions;
this.future = future;
}
@Override
public void onError(@Nullable String parentId) {
onErrorInternal();
onError(subscriptionParentId, subscriptionOptions);
}
@Override
public void onError(@Nullable String parentId, @Nullable Bundle options) {
onErrorInternal();
onErrorInternal(subscriptionParentId, subscriptionOptions);
}
@Override
public void onChildrenLoaded(
@Nullable String parentId, @Nullable List<MediaBrowserCompat.MediaItem> children) {
onChildrenLoadedInternal(parentId, children);
onChildrenLoadedInternal(subscriptionParentId, children);
}
@Override
@ -492,17 +501,31 @@ import org.checkerframework.checker.initialization.qual.UnderInitialization;
@Nullable String parentId,
@Nullable List<MediaBrowserCompat.MediaItem> children,
@Nullable Bundle options) {
onChildrenLoadedInternal(parentId, children);
onChildrenLoadedInternal(subscriptionParentId, children);
}
private void onErrorInternal() {
private void onErrorInternal(String parentId, Bundle options) {
if (future.isDone()) {
// Delegate to the browser by calling `onChildrenChanged` that makes the app call
// `getChildren()` for which the service can return the appropriate error code. This makes a
// redundant call to `subscribe` that can be expected to be not expensive as it just returns
// an exception.
getInstance()
.notifyBrowserListener(
listener ->
listener.onChildrenChanged(
getInstance(),
parentId,
Integer.MAX_VALUE,
new LibraryParams.Builder().setExtras(options).build()));
}
// Don't need to unsubscribe here, because MediaBrowserServiceCompat can notify children
// changed after the initial failure and MediaBrowserCompat could receive the changes.
future.set(LibraryResult.ofError(ERROR_UNKNOWN));
}
private void onChildrenLoadedInternal(
@Nullable String parentId, @Nullable List<MediaBrowserCompat.MediaItem> children) {
String parentId, @Nullable List<MediaBrowserCompat.MediaItem> children) {
if (TextUtils.isEmpty(parentId)) {
Log.w(TAG, "SubscribeCallback.onChildrenLoaded(): Ignoring empty parentId");
return;
@ -526,7 +549,6 @@ import org.checkerframework.checker.initialization.qual.UnderInitialization;
getInstance()
.notifyBrowserListener(
listener -> {
// TODO(b/193193565): Cache children result for later getChildren() calls.
listener.onChildrenChanged(getInstance(), parentId, itemCount, params);
});
future.set(LibraryResult.ofVoid());

View file

@ -50,6 +50,8 @@ public class MediaBrowserConstants {
"parent_auth_expired_error_non_fatal";
public static final String PARENT_ID_AUTH_EXPIRED_ERROR_KEY_ERROR_RESOLUTION_ACTION_LABEL =
"parent_auth_expired_error_label";
public static final String PARENT_ID_ALLOW_FIRST_ON_GET_CHILDREN =
"parent_allow_first_on_get_children";
public static final List<String> GET_CHILDREN_RESULT = new ArrayList<>();
public static final int CHILDREN_COUNT = 100;

View file

@ -32,6 +32,8 @@ public class MediaBrowserServiceCompatConstants {
public static final String TEST_SEND_CUSTOM_COMMAND = "sendCustomCommand";
public static final String TEST_MEDIA_ITEMS_WITH_BROWSE_ACTIONS =
"getLibraryRoot_withBrowseActions";
public static final String TEST_SUBSCRIBE_THEN_REJECT_ON_LOAD_CHILDREN =
"subscribe_thenRejectOnLoadChildren";
private MediaBrowserServiceCompatConstants() {}
}

View file

@ -36,6 +36,7 @@ import static androidx.media3.test.session.common.MediaBrowserConstants.CHILDREN
import static androidx.media3.test.session.common.MediaBrowserConstants.CONNECTION_HINTS_KEY_LIBRARY_ERROR_REPLICATION_MODE;
import static androidx.media3.test.session.common.MediaBrowserConstants.CUSTOM_ACTION;
import static androidx.media3.test.session.common.MediaBrowserConstants.CUSTOM_ACTION_EXTRAS;
import static androidx.media3.test.session.common.MediaBrowserConstants.EXTRAS_KEY_NOTIFY_CHILDREN_CHANGED_MEDIA_ID;
import static androidx.media3.test.session.common.MediaBrowserConstants.GET_CHILDREN_RESULT;
import static androidx.media3.test.session.common.MediaBrowserConstants.LONG_LIST_COUNT;
import static androidx.media3.test.session.common.MediaBrowserConstants.MEDIA_ID_GET_BROWSABLE_ITEM;
@ -43,6 +44,7 @@ import static androidx.media3.test.session.common.MediaBrowserConstants.MEDIA_ID
import static androidx.media3.test.session.common.MediaBrowserConstants.MEDIA_ID_GET_ITEM_WITH_METADATA;
import static androidx.media3.test.session.common.MediaBrowserConstants.MEDIA_ID_GET_PLAYABLE_ITEM;
import static androidx.media3.test.session.common.MediaBrowserConstants.PARENT_ID;
import static androidx.media3.test.session.common.MediaBrowserConstants.PARENT_ID_ALLOW_FIRST_ON_GET_CHILDREN;
import static androidx.media3.test.session.common.MediaBrowserConstants.PARENT_ID_AUTH_EXPIRED_ERROR;
import static androidx.media3.test.session.common.MediaBrowserConstants.PARENT_ID_AUTH_EXPIRED_ERROR_DEPRECATED;
import static androidx.media3.test.session.common.MediaBrowserConstants.PARENT_ID_AUTH_EXPIRED_ERROR_KEY_ERROR_RESOLUTION_ACTION_LABEL;
@ -670,7 +672,7 @@ public class MediaBrowserCompatWithMediaLibraryServiceTest
@Test
public void getChildren_emptyResult() throws Exception {
String testParentId = PARENT_ID_NO_CHILDREN;
connectAndWait(/* rootHints= */ Bundle.EMPTY);
connectAndWait(/* rootHints= */ null);
CountDownLatch latch = new CountDownLatch(1);
AtomicReference<List<MediaItem>> childrenRef = new AtomicReference<>();
@ -712,8 +714,7 @@ public class MediaBrowserCompatWithMediaLibraryServiceTest
}
@Test
public void getChildren_browserNotifyChildrenChanged_callsOnChildrenLoadedTwice()
throws Exception {
public void subscribe_browserNotifyChildrenChanged_callsOnChildrenLoadedTwice() throws Exception {
String testParentId = SUBSCRIBE_PARENT_ID_2;
connectAndWait(/* rootHints= */ Bundle.EMPTY);
CountDownLatch latch = new CountDownLatch(2);
@ -743,6 +744,53 @@ public class MediaBrowserCompatWithMediaLibraryServiceTest
assertThat(childrenList.get(1)).hasSize(12);
}
@Test
public void subscribe_onChildrenChangedWithMaxValue_convertedToOnError() throws Exception {
String testParentId = PARENT_ID_ALLOW_FIRST_ON_GET_CHILDREN;
connectAndWait(/* rootHints= */ Bundle.EMPTY);
CountDownLatch latch = new CountDownLatch(2);
List<String> parentIds = new ArrayList<>();
List<Bundle> optionsList = new ArrayList<>();
List<List<MediaItem>> childrenList = new ArrayList<>();
Bundle requestNotifyChildrenWithDelayBundle =
createNotifyChildrenChangedBundle(
testParentId,
/* itemCount= */ Integer.MAX_VALUE,
/* delayMs= */ 100L,
/* broadcast= */ false);
requestNotifyChildrenWithDelayBundle.putInt(MediaBrowserCompat.EXTRA_PAGE_SIZE, 12);
browserCompat.subscribe(
testParentId,
requestNotifyChildrenWithDelayBundle,
new SubscriptionCallback() {
@Override
public void onChildrenLoaded(String parentId, List<MediaItem> children, Bundle options) {
parentIds.add(parentId);
childrenList.add(children);
optionsList.add(options);
latch.countDown();
}
@Override
public void onError(String parentId, Bundle options) {
parentIds.add(parentId);
optionsList.add(options);
latch.countDown();
}
});
assertThat(latch.await(TIMEOUT_MS, MILLISECONDS)).isTrue();
assertThat(parentIds).containsExactly(testParentId, testParentId);
assertThat(childrenList).hasSize(1);
assertThat(childrenList.get(0)).hasSize(12);
assertThat(optionsList).hasSize(2);
assertThat(optionsList.get(0).getString(EXTRAS_KEY_NOTIFY_CHILDREN_CHANGED_MEDIA_ID))
.isEqualTo(PARENT_ID_ALLOW_FIRST_ON_GET_CHILDREN);
assertThat(optionsList.get(1).getString(EXTRAS_KEY_NOTIFY_CHILDREN_CHANGED_MEDIA_ID))
.isEqualTo(PARENT_ID_ALLOW_FIRST_ON_GET_CHILDREN);
}
@Test
public void getChildren_broadcastNotifyChildrenChanged_callsOnChildrenLoadedTwice()
throws Exception {

View file

@ -35,6 +35,7 @@ import static androidx.media3.test.session.common.MediaBrowserServiceCompatConst
import static androidx.media3.test.session.common.MediaBrowserServiceCompatConstants.TEST_MEDIA_ITEMS_WITH_BROWSE_ACTIONS;
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.MediaBrowserServiceCompatConstants.TEST_SUBSCRIBE_THEN_REJECT_ON_LOAD_CHILDREN;
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;
@ -65,6 +66,7 @@ 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 java.util.concurrent.atomic.AtomicReference;
import org.junit.After;
import org.junit.Before;
@ -140,6 +142,7 @@ public class MediaBrowserListenerWithMediaBrowserServiceCompatTest {
@Test
public void connect_useConnectionHints_connectionHintsPassedToLegacyServerOnGetRootAsRootHints()
throws Exception {
remoteService.setProxyForTest(TEST_GET_CHILDREN);
Bundle connectionHints = new Bundle();
connectionHints.putBoolean(EXTRAS_KEY_SEND_ROOT_HINTS_AS_SESSION_EXTRAS, true);
CountDownLatch latch = new CountDownLatch(/* count= */ 1);
@ -610,4 +613,47 @@ public class MediaBrowserListenerWithMediaBrowserServiceCompatTest {
assertThat(sessionResultRef.get().resultCode).isEqualTo(SessionResult.RESULT_ERROR_UNKNOWN);
assertThat(sessionResultRef.get().extras.getString("key-1")).isEqualTo("error-from-service");
}
@Test
public void
subscribe_thenNullListOnLoadChildren_exceptionConvertedToOnChildrenChangedIntegerMaxValue()
throws Exception {
remoteService.setProxyForTest(TEST_SUBSCRIBE_THEN_REJECT_ON_LOAD_CHILDREN);
CountDownLatch onChildrenChangedLatch = new CountDownLatch(2);
AtomicBoolean onErrorCalled = new AtomicBoolean();
List<String> changedParentIds = new ArrayList<>();
List<Integer> changedItemCounts = new ArrayList<>();
MediaBrowser browser =
createBrowser(
new MediaBrowser.Listener() {
@Override
public void onChildrenChanged(
MediaBrowser browser,
String parentId,
int itemCount,
@Nullable LibraryParams params) {
changedParentIds.add(parentId);
changedItemCounts.add(itemCount);
onChildrenChangedLatch.countDown();
}
@Override
public void onError(MediaController controller, SessionError sessionError) {
onErrorCalled.set(true);
}
});
ListenableFuture<LibraryResult<Void>> future =
threadTestRule
.getHandler()
.postAndSync(() -> browser.subscribe("parentId", new LibraryParams.Builder().build()));
// Trigger calling onLoadChildren that is rejected.
remoteService.notifyChildrenChanged("parentId");
assertThat(future.get().resultCode).isEqualTo(RESULT_SUCCESS);
assertThat(onChildrenChangedLatch.await(TIMEOUT_MS, MILLISECONDS)).isTrue();
assertThat(changedParentIds).containsExactly("parentId", "parentId");
assertThat(changedItemCounts).containsExactly(2, Integer.MAX_VALUE).inOrder();
assertThat(onErrorCalled.get()).isFalse();
}
}

View file

@ -35,6 +35,7 @@ import static androidx.media3.test.session.common.MediaBrowserServiceCompatConst
import static androidx.media3.test.session.common.MediaBrowserServiceCompatConstants.TEST_MEDIA_ITEMS_WITH_BROWSE_ACTIONS;
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.MediaBrowserServiceCompatConstants.TEST_SUBSCRIBE_THEN_REJECT_ON_LOAD_CHILDREN;
import static java.lang.Math.min;
import android.content.Intent;
@ -100,7 +101,6 @@ public class MockMediaBrowserServiceCompat extends MediaBrowserServiceCompat {
sessionCompat.setCallback(new MediaSessionCompat.Callback() {});
sessionCompat.setActive(true);
setSessionToken(sessionCompat.getSessionToken());
testBinder = new RemoteMediaBrowserServiceCompatStub(sessionCompat);
}
@ -299,6 +299,9 @@ public class MockMediaBrowserServiceCompat extends MediaBrowserServiceCompat {
case TEST_MEDIA_ITEMS_WITH_BROWSE_ACTIONS:
setProxyForMediaItemsWithBrowseActions(session);
break;
case TEST_SUBSCRIBE_THEN_REJECT_ON_LOAD_CHILDREN:
setProxyForSubscribeAndRejectGetChildren();
break;
default:
throw new IllegalArgumentException("Unknown testName: " + testName);
}
@ -456,6 +459,45 @@ public class MockMediaBrowserServiceCompat extends MediaBrowserServiceCompat {
});
}
private void setProxyForSubscribeAndRejectGetChildren() {
setMediaBrowserServiceProxy(
new MockMediaBrowserServiceCompat.Proxy() {
private boolean isSubscribed;
@Override
public void onLoadChildren(String parentId, Result<List<MediaItem>> result) {
onLoadChildren(parentId, result, new Bundle());
}
@Override
public void onLoadChildren(
String parentId, Result<List<MediaItem>> result, Bundle options) {
if (isSubscribed) {
// Accept the first call that a Media3 browser interprets as a successful
// subscription. Then reject any further access to onLoadChildren().
result.sendResult(null);
return;
}
isSubscribed = true;
result.sendResult(
ImmutableList.of(
new MediaItem(
new MediaDescriptionCompat.Builder()
.setMediaUri(Uri.parse("http://www.example.com/1"))
.setMediaId("mediaId1")
.build(),
MediaItem.FLAG_PLAYABLE),
new MediaItem(
new MediaDescriptionCompat.Builder()
.setMediaUri(Uri.parse("http://www.example.com/2"))
.setMediaId("mediaId2")
.build(),
MediaItem.FLAG_PLAYABLE)));
}
});
}
private void getChildren_authenticationError_receivesPlaybackException(
MediaSessionCompat session, boolean isFatal) {
setMediaBrowserServiceProxy(

View file

@ -42,6 +42,7 @@ import static androidx.media3.test.session.common.MediaBrowserConstants.MEDIA_ID
import static androidx.media3.test.session.common.MediaBrowserConstants.MEDIA_ID_GET_ITEM_WITH_METADATA;
import static androidx.media3.test.session.common.MediaBrowserConstants.MEDIA_ID_GET_PLAYABLE_ITEM;
import static androidx.media3.test.session.common.MediaBrowserConstants.PARENT_ID;
import static androidx.media3.test.session.common.MediaBrowserConstants.PARENT_ID_ALLOW_FIRST_ON_GET_CHILDREN;
import static androidx.media3.test.session.common.MediaBrowserConstants.PARENT_ID_AUTH_EXPIRED_ERROR;
import static androidx.media3.test.session.common.MediaBrowserConstants.PARENT_ID_AUTH_EXPIRED_ERROR_DEPRECATED;
import static androidx.media3.test.session.common.MediaBrowserConstants.PARENT_ID_AUTH_EXPIRED_ERROR_KEY_ERROR_RESOLUTION_ACTION_LABEL;
@ -227,6 +228,7 @@ public class MockMediaLibraryService extends MediaLibraryService {
playlistAddExtras.putString("key-1", "playlist_add");
Bundle radioExtras = new Bundle();
radioExtras.putString("key-1", "radio");
Log.d("notifyChildrenChanged", "new TestLibrarySessionCallback()");
session =
new MediaLibrarySession.Builder(
MockMediaLibraryService.this,
@ -284,6 +286,8 @@ public class MockMediaLibraryService extends MediaLibraryService {
private class TestLibrarySessionCallback implements MediaLibrarySession.Callback {
private int getChildrenCallCount = 0;
@Override
public MediaSession.ConnectionResult onConnect(
MediaSession session, ControllerInfo controller) {
@ -351,6 +355,7 @@ public class MockMediaLibraryService extends MediaLibraryService {
}
switch (mediaId) {
case MEDIA_ID_GET_BROWSABLE_ITEM:
case PARENT_ID_ALLOW_FIRST_ON_GET_CHILDREN:
case SUBSCRIBE_PARENT_ID_2:
return Futures.immediateFuture(
LibraryResult.ofItem(createBrowsableMediaItem(mediaId), /* params= */ null));
@ -380,6 +385,7 @@ public class MockMediaLibraryService extends MediaLibraryService {
int page,
int pageSize,
@Nullable LibraryParams params) {
getChildrenCallCount++;
assertLibraryParams(params);
if (Objects.equals(parentId, PARENT_ID_NO_CHILDREN)) {
return Futures.immediateFuture(LibraryResult.ofItemList(ImmutableList.of(), params));
@ -399,6 +405,15 @@ public class MockMediaLibraryService extends MediaLibraryService {
errorBundle.putString("key", "value");
return Futures.immediateFuture(
LibraryResult.ofError(new SessionError(ERROR_BAD_VALUE, "error message", errorBundle)));
} else if (Objects.equals(parentId, PARENT_ID_ALLOW_FIRST_ON_GET_CHILDREN)) {
return getChildrenCallCount == 1
? Futures.immediateFuture(
LibraryResult.ofItemList(
getPaginatedResult(GET_CHILDREN_RESULT, page, pageSize), params))
: Futures.immediateFuture(
LibraryResult.ofError(
LibraryResult.RESULT_ERROR_SESSION_AUTHENTICATION_EXPIRED,
new LibraryParams.Builder().build()));
} else if (Objects.equals(parentId, PARENT_ID_AUTH_EXPIRED_ERROR)
|| Objects.equals(parentId, PARENT_ID_AUTH_EXPIRED_ERROR_DEPRECATED)
|| Objects.equals(parentId, PARENT_ID_SKIP_LIMIT_REACHED_ERROR)) {