From dd2c16bc459af50c126b9666d3134595ba58e4cb Mon Sep 17 00:00:00 2001 From: christosts Date: Fri, 22 Jul 2022 09:05:10 +0000 Subject: [PATCH] DefaultMediaNotificationProvider: limit requests to load same bitmap The DefaultMediaNotificationProvider caches the last loaded artwork bitmap so that the bitmap isn't loaded again when the notification is updated, e.g., the player is transiting from playing to paused. However, loading bitmap requests for bitmaps that are already being loaded are not suppressed. For example, if the notification is updated while the artwork is still downloading, the same artwork might be downloaded multiple times. This change suppresses a bitmap load request if the same artwork is still being loaded, to avoid additional artwork downloads. #minor-release PiperOrigin-RevId: 462572221 --- .../DefaultMediaNotificationProvider.java | 110 ++++++++++-------- .../DefaultMediaNotificationProviderTest.java | 68 ++++++++++- 2 files changed, 124 insertions(+), 54 deletions(-) diff --git a/libraries/session/src/main/java/androidx/media3/session/DefaultMediaNotificationProvider.java b/libraries/session/src/main/java/androidx/media3/session/DefaultMediaNotificationProvider.java index 4b588cfda7..372a576683 100644 --- a/libraries/session/src/main/java/androidx/media3/session/DefaultMediaNotificationProvider.java +++ b/libraries/session/src/main/java/androidx/media3/session/DefaultMediaNotificationProvider.java @@ -25,7 +25,6 @@ import static androidx.media3.common.Player.COMMAND_SEEK_TO_PREVIOUS_MEDIA_ITEM; import static androidx.media3.common.Player.COMMAND_STOP; import static androidx.media3.common.util.Assertions.checkState; import static androidx.media3.common.util.Assertions.checkStateNotNull; -import static androidx.media3.common.util.Util.castNonNull; import android.app.Notification; import android.app.NotificationChannel; @@ -45,7 +44,6 @@ import androidx.media.app.NotificationCompat.MediaStyle; import androidx.media3.common.C; import androidx.media3.common.MediaMetadata; import androidx.media3.common.Player; -import androidx.media3.common.util.Consumer; import androidx.media3.common.util.Log; import androidx.media3.common.util.UnstableApi; import androidx.media3.common.util.Util; @@ -58,6 +56,7 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.List; import java.util.concurrent.ExecutionException; +import org.checkerframework.checker.nullness.qual.MonotonicNonNull; /** * The default {@link MediaNotification.Provider}. @@ -231,12 +230,12 @@ public class DefaultMediaNotificationProvider implements MediaNotification.Provi @StringRes private final int channelNameResourceId; private final NotificationManager notificationManager; private final BitmapLoader bitmapLoader; - // Cache the last loaded bitmap to avoid reloading the bitmap again, particularly useful when - // showing a notification for the same item (e.g. when switching from playing to paused). - private final LoadedBitmapInfo lastLoadedBitmapInfo; + // Cache the last bitmap load request to avoid reloading the bitmap again, particularly useful + // when showing a notification for the same item (e.g. when switching from playing to paused). + private final BitmapLoadRequest lastBitmapLoadRequest; private final Handler mainHandler; - private OnBitmapLoadedFutureCallback pendingOnBitmapLoadedFutureCallback; + private @MonotonicNonNull OnBitmapLoadedFutureCallback pendingOnBitmapLoadedFutureCallback; @DrawableRes private int smallIconResourceId; private DefaultMediaNotificationProvider(Builder builder) { @@ -248,9 +247,8 @@ public class DefaultMediaNotificationProvider implements MediaNotification.Provi notificationManager = checkStateNotNull( (NotificationManager) context.getSystemService(Context.NOTIFICATION_SERVICE)); - lastLoadedBitmapInfo = new LoadedBitmapInfo(); + lastBitmapLoadRequest = new BitmapLoadRequest(); mainHandler = new Handler(Looper.getMainLooper()); - pendingOnBitmapLoadedFutureCallback = new OnBitmapLoadedFutureCallback(bitmap -> {}); smallIconResourceId = R.drawable.media3_notification_small_icon; } @@ -281,6 +279,9 @@ public class DefaultMediaNotificationProvider implements MediaNotification.Provi builder.setContentTitle(metadata.title).setContentText(metadata.artist); @Nullable ListenableFuture bitmapFuture = loadArtworkBitmap(metadata); if (bitmapFuture != null) { + if (pendingOnBitmapLoadedFutureCallback != null) { + pendingOnBitmapLoadedFutureCallback.discardIfPending(); + } if (bitmapFuture.isDone()) { try { builder.setLargeIcon(Futures.getDone(bitmapFuture)); @@ -288,14 +289,12 @@ public class DefaultMediaNotificationProvider implements MediaNotification.Provi Log.w(TAG, "Failed to load bitmap", e); } } else { + pendingOnBitmapLoadedFutureCallback = + new OnBitmapLoadedFutureCallback( + notificationId, builder, onNotificationChangedCallback); Futures.addCallback( bitmapFuture, - new OnBitmapLoadedFutureCallback( - bitmap -> { - builder.setLargeIcon(bitmap); - onNotificationChangedCallback.onNotificationChanged( - new MediaNotification(notificationId, builder.build())); - }), + pendingOnBitmapLoadedFutureCallback, // This callback must be executed on the next looper iteration, after this method has // returned a media notification. mainHandler::post); @@ -517,32 +516,19 @@ public class DefaultMediaNotificationProvider implements MediaNotification.Provi */ @Nullable private ListenableFuture loadArtworkBitmap(MediaMetadata metadata) { - if (lastLoadedBitmapInfo.matches(metadata.artworkData) - || lastLoadedBitmapInfo.matches(metadata.artworkUri)) { - return Futures.immediateFuture(lastLoadedBitmapInfo.getBitmap()); - } - - ListenableFuture future; - Consumer onBitmapLoaded; - if (metadata.artworkData != null) { + @Nullable ListenableFuture future; + if (lastBitmapLoadRequest.matches(metadata.artworkData) + || lastBitmapLoadRequest.matches(metadata.artworkUri)) { + future = lastBitmapLoadRequest.getFuture(); + } else if (metadata.artworkData != null) { future = bitmapLoader.decodeBitmap(metadata.artworkData); - onBitmapLoaded = - bitmap -> lastLoadedBitmapInfo.setBitmapInfo(castNonNull(metadata.artworkData), bitmap); + lastBitmapLoadRequest.setBitmapFuture(metadata.artworkData, future); } else if (metadata.artworkUri != null) { future = bitmapLoader.loadBitmap(metadata.artworkUri); - onBitmapLoaded = - bitmap -> lastLoadedBitmapInfo.setBitmapInfo(castNonNull(metadata.artworkUri), bitmap); + lastBitmapLoadRequest.setBitmapFuture(metadata.artworkUri, future); } else { - return null; + future = null; } - - pendingOnBitmapLoadedFutureCallback.discardIfPending(); - pendingOnBitmapLoadedFutureCallback = new OnBitmapLoadedFutureCallback(onBitmapLoaded); - Futures.addCallback( - future, - pendingOnBitmapLoadedFutureCallback, - // It's ok to run this immediately to update the last loaded bitmap. - runnable -> Util.postOrRun(mainHandler, runnable)); return future; } @@ -560,13 +546,19 @@ public class DefaultMediaNotificationProvider implements MediaNotification.Provi } private static class OnBitmapLoadedFutureCallback implements FutureCallback { - - private final Consumer consumer; + private final int notificationId; + private final NotificationCompat.Builder builder; + private final Callback onNotificationChangedCallback; private boolean discarded; - private OnBitmapLoadedFutureCallback(Consumer consumer) { - this.consumer = consumer; + public OnBitmapLoadedFutureCallback( + int notificationId, + NotificationCompat.Builder builder, + Callback onNotificationChangedCallback) { + this.notificationId = notificationId; + this.builder = builder; + this.onNotificationChangedCallback = onNotificationChangedCallback; } public void discardIfPending() { @@ -576,7 +568,9 @@ public class DefaultMediaNotificationProvider implements MediaNotification.Provi @Override public void onSuccess(Bitmap result) { if (!discarded) { - consumer.accept(result); + builder.setLargeIcon(result); + onNotificationChangedCallback.onNotificationChanged( + new MediaNotification(notificationId, builder.build())); } } @@ -589,35 +583,51 @@ public class DefaultMediaNotificationProvider implements MediaNotification.Provi } /** - * Caches the last loaded bitmap. The key to identify a bitmap is either a byte array, if the - * bitmap is loaded from compressed data, or a URI, if the bitmap was loaded from a URI. + * Stores the result of a bitmap load request. Requests are identified either by a byte array, if + * the bitmap is loaded from compressed data, or a URI, if the bitmap was loaded from a URI. */ - private static class LoadedBitmapInfo { + private static class BitmapLoadRequest { @Nullable private byte[] data; @Nullable private Uri uri; - @Nullable private Bitmap bitmap; + @Nullable private ListenableFuture bitmapFuture; + /** Whether the bitmap load request was performed for {@code data}. */ public boolean matches(@Nullable byte[] data) { return this.data != null && data != null && Arrays.equals(this.data, data); } + /** Whether the bitmap load request was performed for {@code uri}. */ public boolean matches(@Nullable Uri uri) { return this.uri != null && this.uri.equals(uri); } - public Bitmap getBitmap() { - return checkStateNotNull(bitmap); + /** + * Returns the future that set for the bitmap load request. + * + * @see #setBitmapFuture(Uri, ListenableFuture) + * @see #setBitmapFuture(byte[], ListenableFuture) + */ + public ListenableFuture getFuture() { + return checkStateNotNull(bitmapFuture); } - public void setBitmapInfo(byte[] data, Bitmap bitmap) { + /** + * Sets the future result of requesting to {@linkplain BitmapLoader#decodeBitmap(byte[]) decode} + * a bitmap from {@code data}. + */ + public void setBitmapFuture(byte[] data, ListenableFuture bitmapFuture) { this.data = data; - this.bitmap = bitmap; + this.bitmapFuture = bitmapFuture; this.uri = null; } - public void setBitmapInfo(Uri uri, Bitmap bitmap) { + /** + * Sets the future result of requesting {@linkplain BitmapLoader#loadBitmap(Uri) load} a bitmap + * from {@code uri}. + */ + public void setBitmapFuture(Uri uri, ListenableFuture bitmapFuture) { this.uri = uri; - this.bitmap = bitmap; + this.bitmapFuture = bitmapFuture; this.data = null; } } diff --git a/libraries/session/src/test/java/androidx/media3/session/DefaultMediaNotificationProviderTest.java b/libraries/session/src/test/java/androidx/media3/session/DefaultMediaNotificationProviderTest.java index b6555098a7..b3b33ff610 100644 --- a/libraries/session/src/test/java/androidx/media3/session/DefaultMediaNotificationProviderTest.java +++ b/libraries/session/src/test/java/androidx/media3/session/DefaultMediaNotificationProviderTest.java @@ -21,14 +21,18 @@ import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoInteractions; import static org.mockito.Mockito.verifyNoMoreInteractions; import static org.mockito.Mockito.when; +import static org.robolectric.Shadows.shadowOf; import android.app.NotificationChannel; import android.app.NotificationManager; import android.content.Context; +import android.graphics.Bitmap; import android.net.Uri; import android.os.Bundle; +import android.os.Looper; import androidx.annotation.Nullable; import androidx.core.app.NotificationCompat; import androidx.media3.common.MediaMetadata; @@ -37,6 +41,7 @@ import androidx.media3.common.Player.Commands; import androidx.test.core.app.ApplicationProvider; import androidx.test.ext.junit.runners.AndroidJUnit4; import com.google.common.collect.ImmutableList; +import com.google.common.util.concurrent.SettableFuture; import java.util.List; import org.junit.Test; import org.junit.runner.RunWith; @@ -45,6 +50,7 @@ import org.mockito.InOrder; import org.mockito.Mockito; import org.robolectric.Robolectric; import org.robolectric.Shadows; +import org.robolectric.shadows.ShadowLooper; import org.robolectric.shadows.ShadowNotificationManager; /** Tests for {@link DefaultMediaNotificationProvider}. */ @@ -389,6 +395,60 @@ public class DefaultMediaNotificationProviderTest { assertThat(actions.get(0).getExtras().size()).isEqualTo(0); } + /** + * Tests that the {@link DefaultMediaNotificationProvider} will not request to load the same + * artwork bitmap again, if the same bitmap has been requested already. + */ + @Test + public void requestsSameBitmap_withPendingRequest_oneRequestOnly() { + // We will advance the main looper manually in the test. + shadowOf(Looper.getMainLooper()).pause(); + // Create a MediaSession whose player returns non-null media metadata so that the + // notification provider will request to load artwork bitmaps. + MediaSession mockMediaSession = + createMockMediaSessionForNotification( + new MediaMetadata.Builder() + .setArtworkUri(Uri.parse("http://example.test/image.jpg")) + .build()); + DefaultActionFactory defaultActionFactory = + new DefaultActionFactory(Robolectric.setupService(TestService.class)); + BitmapLoader mockBitmapLoader = mock(BitmapLoader.class); + SettableFuture bitmapFuture = SettableFuture.create(); + when(mockBitmapLoader.loadBitmap(any())).thenReturn(bitmapFuture); + DefaultMediaNotificationProvider defaultMediaNotificationProvider = + new DefaultMediaNotificationProvider.Builder(ApplicationProvider.getApplicationContext()) + .setBitmapLoader(mockBitmapLoader) + .build(); + + // Ask the notification provider to create a notification twice. Use separate callback instances + // for each notification so that we can distinguish for which notification we received a + // callback. + MediaNotification.Provider.Callback mockOnNotificationChangedCallback1 = + mock(MediaNotification.Provider.Callback.class); + defaultMediaNotificationProvider.createNotification( + mockMediaSession, + /* customLayout= */ ImmutableList.of(), + defaultActionFactory, + mockOnNotificationChangedCallback1); + ShadowLooper.idleMainLooper(); + verify(mockBitmapLoader).loadBitmap(Uri.parse("http://example.test/image.jpg")); + verifyNoInteractions(mockOnNotificationChangedCallback1); + MediaNotification.Provider.Callback mockOnNotificationChangedCallback2 = + mock(MediaNotification.Provider.Callback.class); + defaultMediaNotificationProvider.createNotification( + mockMediaSession, + /* customLayout= */ ImmutableList.of(), + defaultActionFactory, + mockOnNotificationChangedCallback2); + // The bitmap has arrived. + bitmapFuture.set(Bitmap.createBitmap(/* width= */ 8, /* height= */ 8, Bitmap.Config.RGB_565)); + ShadowLooper.idleMainLooper(); + + verifyNoMoreInteractions(mockBitmapLoader); + verify(mockOnNotificationChangedCallback2).onNotificationChanged(any()); + verifyNoInteractions(mockOnNotificationChangedCallback1); + } + @Test public void provider_withCustomIds_notificationsUseCustomIds() { Context context = ApplicationProvider.getApplicationContext(); @@ -398,7 +458,7 @@ public class DefaultMediaNotificationProviderTest { .setChannelId(/* channelId= */ "customChannelId") .setChannelName(/* channelNameResourceId= */ R.string.media3_controls_play_description) .build(); - MediaSession mockMediaSession = createMockMediaSessionForNotification(); + MediaSession mockMediaSession = createMockMediaSessionForNotification(MediaMetadata.EMPTY); DefaultActionFactory defaultActionFactory = new DefaultActionFactory(Robolectric.setupService(TestService.class)); @@ -427,7 +487,7 @@ public class DefaultMediaNotificationProviderTest { new DefaultMediaNotificationProvider.Builder(context).build(); DefaultActionFactory defaultActionFactory = new DefaultActionFactory(Robolectric.setupService(TestService.class)); - MediaSession mockMediaSession = createMockMediaSessionForNotification(); + MediaSession mockMediaSession = createMockMediaSessionForNotification(MediaMetadata.EMPTY); MediaNotification notification = defaultMediaNotificationProvider.createNotification( @@ -467,10 +527,10 @@ public class DefaultMediaNotificationProviderTest { assertThat(found).isTrue(); } - private static MediaSession createMockMediaSessionForNotification() { + private static MediaSession createMockMediaSessionForNotification(MediaMetadata mediaMetadata) { Player mockPlayer = mock(Player.class); when(mockPlayer.getAvailableCommands()).thenReturn(Commands.EMPTY); - when(mockPlayer.getMediaMetadata()).thenReturn(MediaMetadata.EMPTY); + when(mockPlayer.getMediaMetadata()).thenReturn(mediaMetadata); MediaSession mockMediaSession = mock(MediaSession.class); when(mockMediaSession.getPlayer()).thenReturn(mockPlayer); MediaSessionImpl mockMediaSessionImpl = mock(MediaSessionImpl.class);