Remove bitmap caching from ScrollableImagePreview

Remove bitmap caching from ScrollableImagePreview; instead, extend the
image loader interface to proivde a cache-control option.
ScrollableImagePreview$BatchPreviewLoader requests caching only for the
first visible items as we'd like them to be displayed as fast as
possible and not be evicted by any of the remaining items.
Minor fixes inside ImagePreviewImageLoader.
ImageLoader is moved into contentpreview package.

Bug: 271613784
Test: unit tests
Test: Manual testing with injected extensive logging.
Change-Id: Ic07572e1fb73d589d98207af8fe58ae52698802a

9 files changed, 134 insertions, 64 deletions

diff --git a/java/src/com/android/intentresolver/ChooserActivity.java b/java/src/com/android/intentresolver/ChooserActivity.java
index 404d6da3..917a4e5d 100644
--- a/java/src/com/android/intentresolver/ChooserActivity.java
+++ b/java/src/com/android/intentresolver/ChooserActivity.java
@@ -84,6 +84,7 @@ import com.android.intentresolver.chooser.MultiDisplayResolveInfo;
 import com.android.intentresolver.chooser.TargetInfo;
 import com.android.intentresolver.contentpreview.ChooserContentPreviewUi;
 import com.android.intentresolver.contentpreview.HeadlineGeneratorImpl;
+import com.android.intentresolver.contentpreview.ImageLoader;
 import com.android.intentresolver.flags.FeatureFlagRepository;
 import com.android.intentresolver.flags.FeatureFlagRepositoryFactory;
 import com.android.intentresolver.grid.ChooserGridAdapter;
@@ -1287,9 +1288,9 @@ public class ChooserActivity extends ResolverActivity implements
     protected ImageLoader createPreviewImageLoader() {
         final int cacheSize;
         float chooserWidth = getResources().getDimension(R.dimen.chooser_width);
-        // imageWidth = imagePreviewHeight / minAspectRatio (see ScrollableImagePreviewView)
+        // imageWidth = imagePreviewHeight * minAspectRatio (see ScrollableImagePreviewView)
         float imageWidth =
-                getResources().getDimension(R.dimen.chooser_preview_image_height_tall) * 5 / 2;
+                getResources().getDimension(R.dimen.chooser_preview_image_height_tall) * 2 / 5;
         cacheSize = (int) (Math.ceil(chooserWidth / imageWidth) + 2);
         return new ImagePreviewImageLoader(this, getLifecycle(), cacheSize);
     }
diff --git a/java/src/com/android/intentresolver/ImageLoader.kt b/java/src/com/android/intentresolver/ImageLoader.kt
deleted file mode 100644
index 0ed8b122..00000000
--- a/java/src/com/android/intentresolver/ImageLoader.kt
+++ /dev/null
@@ -1,26 +0,0 @@
-/*
- * Copyright (C) 2022 The Android Open Source Project
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- *      http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-
-package com.android.intentresolver
-
-import android.graphics.Bitmap
-import android.net.Uri
-import java.util.function.Consumer
-
-interface ImageLoader : suspend (Uri) -> Bitmap? {
-    fun loadImage(uri: Uri, callback: Consumer<Bitmap?>)
-    fun prePopulate(uris: List<Uri>)
-}
diff --git a/java/src/com/android/intentresolver/ImagePreviewImageLoader.kt b/java/src/com/android/intentresolver/ImagePreviewImageLoader.kt
index 9650403e..c97efdd1 100644
--- a/java/src/com/android/intentresolver/ImagePreviewImageLoader.kt
+++ b/java/src/com/android/intentresolver/ImagePreviewImageLoader.kt
@@ -26,8 +26,11 @@ import androidx.annotation.VisibleForTesting
 import androidx.collection.LruCache
 import androidx.lifecycle.Lifecycle
 import androidx.lifecycle.coroutineScope
+import com.android.intentresolver.contentpreview.ImageLoader
+import kotlinx.coroutines.CancellationException
 import kotlinx.coroutines.CompletableDeferred
 import kotlinx.coroutines.CoroutineDispatcher
+import kotlinx.coroutines.Deferred
 import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.isActive
 import kotlinx.coroutines.launch
@@ -35,6 +38,10 @@ import java.util.function.Consumer
 
 private const val TAG = "ImagePreviewImageLoader"
 
+/**
+ * Implements preview image loading for the content preview UI. Provides requests deduplication and
+ * image caching.
+ */
 @VisibleForTesting
 class ImagePreviewImageLoader @JvmOverloads constructor(
     private val context: Context,
@@ -48,14 +55,17 @@ class ImagePreviewImageLoader @JvmOverloads constructor(
             Size(it, it)
         }
 
-    @GuardedBy("self")
-    private val cache = LruCache<Uri, CompletableDeferred<Bitmap?>>(cacheSize)
+    private val lock = Any()
+    @GuardedBy("lock")
+    private val cache = LruCache<Uri, RequestRecord>(cacheSize)
+    @GuardedBy("lock")
+    private val runningRequests = HashMap<Uri, RequestRecord>()
 
-    override suspend fun invoke(uri: Uri): Bitmap? = loadImageAsync(uri)
+    override suspend fun invoke(uri: Uri, caching: Boolean): Bitmap? = loadImageAsync(uri, caching)
 
     override fun loadImage(uri: Uri, callback: Consumer<Bitmap?>) {
         lifecycle.coroutineScope.launch {
-            val image = loadImageAsync(uri)
+            val image = loadImageAsync(uri, caching = true)
             if (isActive) {
                 callback.accept(image)
             }
@@ -65,23 +75,44 @@ class ImagePreviewImageLoader @JvmOverloads constructor(
     override fun prePopulate(uris: List<Uri>) {
         uris.asSequence().take(cache.maxSize()).forEach { uri ->
             lifecycle.coroutineScope.launch {
-                loadImageAsync(uri)
+                loadImageAsync(uri, caching = true)
             }
         }
     }
 
-    private suspend fun loadImageAsync(uri: Uri): Bitmap? {
-        return synchronized(cache) {
-            cache.get(uri) ?: CompletableDeferred<Bitmap?>().also { result ->
-                cache.put(uri, result)
-                lifecycle.coroutineScope.launch(dispatcher) {
-                    result.loadBitmap(uri)
+    private suspend fun loadImageAsync(uri: Uri, caching: Boolean): Bitmap? {
+        return getRequestDeferred(uri, caching)
+            .await()
+    }
+
+    private fun getRequestDeferred(uri: Uri, caching: Boolean): Deferred<Bitmap?> {
+        var shouldLaunchImageLoading = false
+        val request = synchronized(lock) {
+            cache[uri]
+                ?: runningRequests.getOrPut(uri) {
+                    shouldLaunchImageLoading = true
+                    RequestRecord(uri, CompletableDeferred(), caching)
+                }.apply {
+                    this.caching = this.caching || caching
                 }
+        }
+        if (shouldLaunchImageLoading) {
+            request.loadBitmapAsync()
+        }
+        return request.deferred
+    }
+
+    private fun RequestRecord.loadBitmapAsync() {
+        lifecycle.coroutineScope.launch(dispatcher) {
+            loadBitmap()
+        }.invokeOnCompletion { cause ->
+            if (cause is CancellationException) {
+                cancel()
             }
-        }.await()
+        }
     }
 
-    private fun CompletableDeferred<Bitmap?>.loadBitmap(uri: Uri) {
+    private fun RequestRecord.loadBitmap() {
         val bitmap = try {
             context.contentResolver.loadThumbnail(uri,  thumbnailSize, null)
         } catch (t: Throwable) {
@@ -90,4 +121,27 @@ class ImagePreviewImageLoader @JvmOverloads constructor(
         }
         complete(bitmap)
     }
+
+    private fun RequestRecord.cancel() {
+        synchronized(lock) {
+            runningRequests.remove(uri)
+            deferred.cancel()
+        }
+    }
+
+    private fun RequestRecord.complete(bitmap: Bitmap?) {
+        deferred.complete(bitmap)
+        synchronized(lock) {
+            runningRequests.remove(uri)
+            if (bitmap != null && caching) {
+                cache.put(uri, this)
+            }
+        }
+    }
+
+    private class RequestRecord(
+        val uri: Uri,
+        val deferred: CompletableDeferred<Bitmap?>,
+        @GuardedBy("lock") var caching: Boolean
+    )
 }
diff --git a/java/src/com/android/intentresolver/contentpreview/ChooserContentPreviewUi.java b/java/src/com/android/intentresolver/contentpreview/ChooserContentPreviewUi.java
index 56027a16..181fe117 100644
--- a/java/src/com/android/intentresolver/contentpreview/ChooserContentPreviewUi.java
+++ b/java/src/com/android/intentresolver/contentpreview/ChooserContentPreviewUi.java
@@ -38,7 +38,6 @@ import android.view.ViewGroup;
 
 import androidx.annotation.Nullable;
 
-import com.android.intentresolver.ImageLoader;
 import com.android.intentresolver.widget.ActionRow;
 import com.android.intentresolver.widget.ImagePreviewView.TransitionElementStatusCallback;
 
diff --git a/java/src/com/android/intentresolver/contentpreview/ImageLoader.kt b/java/src/com/android/intentresolver/contentpreview/ImageLoader.kt
new file mode 100644
index 00000000..225807ee
--- /dev/null
+++ b/java/src/com/android/intentresolver/contentpreview/ImageLoader.kt
@@ -0,0 +1,51 @@
+/*
+ * Copyright (C) 2023 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.android.intentresolver.contentpreview
+
+import android.graphics.Bitmap
+import android.net.Uri
+import java.util.function.Consumer
+
+/**
+ * A content preview image loader.
+ */
+interface ImageLoader : suspend (Uri) -> Bitmap?, suspend (Uri, Boolean) -> Bitmap? {
+    /**
+     * Load preview image asynchronously; caching is allowed.
+     * @param uri content URI
+     * @param callback a callback that will be invoked with the loaded image or null if loading has
+     * failed.
+     */
+    fun loadImage(uri: Uri, callback: Consumer<Bitmap?>)
+
+    /**
+     * Prepopulate the image loader cache.
+     */
+    fun prePopulate(uris: List<Uri>)
+
+    /**
+     * Load preview image; caching is allowed.
+     */
+    override suspend fun invoke(uri: Uri) = invoke(uri, true)
+
+    /**
+     * Load preview image.
+     * @param uri content URI
+     * @param caching indicates if the loaded image could be cached.
+     */
+    override suspend fun invoke(uri: Uri, caching: Boolean): Bitmap?
+}
diff --git a/java/src/com/android/intentresolver/contentpreview/TextContentPreviewUi.java b/java/src/com/android/intentresolver/contentpreview/TextContentPreviewUi.java
index ece0c312..6bf9a1cc 100644
--- a/java/src/com/android/intentresolver/contentpreview/TextContentPreviewUi.java
+++ b/java/src/com/android/intentresolver/contentpreview/TextContentPreviewUi.java
@@ -27,7 +27,6 @@ import android.widget.TextView;
 
 import androidx.annotation.Nullable;
 
-import com.android.intentresolver.ImageLoader;
 import com.android.intentresolver.R;
 import com.android.intentresolver.widget.ActionRow;
 
diff --git a/java/src/com/android/intentresolver/contentpreview/UnifiedContentPreviewUi.java b/java/src/com/android/intentresolver/contentpreview/UnifiedContentPreviewUi.java
index 9ce875c8..709ec566 100644
--- a/java/src/com/android/intentresolver/contentpreview/UnifiedContentPreviewUi.java
+++ b/java/src/com/android/intentresolver/contentpreview/UnifiedContentPreviewUi.java
@@ -31,7 +31,6 @@ import android.widget.TextView;
 
 import androidx.annotation.Nullable;
 
-import com.android.intentresolver.ImageLoader;
 import com.android.intentresolver.R;
 import com.android.intentresolver.widget.ActionRow;
 import com.android.intentresolver.widget.ImagePreviewView.TransitionElementStatusCallback;
diff --git a/java/src/com/android/intentresolver/widget/ImagePreviewView.kt b/java/src/com/android/intentresolver/widget/ImagePreviewView.kt
index 5f92b149..3f0458ee 100644
--- a/java/src/com/android/intentresolver/widget/ImagePreviewView.kt
+++ b/java/src/com/android/intentresolver/widget/ImagePreviewView.kt
@@ -16,12 +16,8 @@
 
 package com.android.intentresolver.widget
 
-import android.graphics.Bitmap
-import android.net.Uri
 import android.view.View
 
-internal typealias ImageLoader = suspend (Uri) -> Bitmap?
-
 interface ImagePreviewView {
     fun setTransitionElementStatusCallback(callback: TransitionElementStatusCallback?)
     fun getTransitionView(): View?
diff --git a/java/src/com/android/intentresolver/widget/ScrollableImagePreviewView.kt b/java/src/com/android/intentresolver/widget/ScrollableImagePreviewView.kt
index 7755610d..524b4f81 100644
--- a/java/src/com/android/intentresolver/widget/ScrollableImagePreviewView.kt
+++ b/java/src/com/android/intentresolver/widget/ScrollableImagePreviewView.kt
@@ -56,6 +56,8 @@ private const val MIN_ASPECT_RATIO_STRING = "2:5"
 private const val MAX_ASPECT_RATIO = 2.5f
 private const val MAX_ASPECT_RATIO_STRING = "5:2"
 
+private typealias CachingImageLoader = suspend (Uri, Boolean) -> Bitmap?
+
 class ScrollableImagePreviewView : RecyclerView, ImagePreviewView {
     constructor(context: Context) : this(context, null)
     constructor(context: Context, attrs: AttributeSet?) : this(context, attrs, 0)
@@ -131,7 +133,7 @@ class ScrollableImagePreviewView : RecyclerView, ImagePreviewView {
         return null
     }
 
-    fun setPreviews(previews: List<Preview>, otherItemCount: Int, imageLoader: ImageLoader) {
+    fun setPreviews(previews: List<Preview>, otherItemCount: Int, imageLoader: CachingImageLoader) {
         previewAdapter.reset(0, imageLoader)
         batchLoader?.cancel()
         batchLoader = BatchPreviewLoader(
@@ -176,8 +178,6 @@ class ScrollableImagePreviewView : RecyclerView, ImagePreviewView {
     ) {
         constructor(type: PreviewType, uri: Uri) : this(type, uri, "1:1")
 
-        internal var bitmap: Bitmap? = null
-
         internal fun updateAspectRatio(width: Int, height: Int) {
             if (width <= 0 || height <= 0) return
             val aspectRatio = width.toFloat() / height.toFloat()
@@ -197,7 +197,7 @@ class ScrollableImagePreviewView : RecyclerView, ImagePreviewView {
         private val context: Context
     ) : RecyclerView.Adapter<ViewHolder>() {
         private val previews = ArrayList<Preview>()
-        private var imageLoader: ImageLoader? = null
+        private var imageLoader: CachingImageLoader? = null
         private var firstImagePos = -1
         private var totalItemCount: Int = 0
 
@@ -206,7 +206,7 @@ class ScrollableImagePreviewView : RecyclerView, ImagePreviewView {
 
         var transitionStatusElementCallback: TransitionElementStatusCallback? = null
 
-        fun reset(totalItemCount: Int, imageLoader: ImageLoader) {
+        fun reset(totalItemCount: Int, imageLoader: CachingImageLoader) {
             this.imageLoader = imageLoader
             firstImagePos = -1
             previews.clear()
@@ -299,7 +299,7 @@ class ScrollableImagePreviewView : RecyclerView, ImagePreviewView {
 
         fun bind(
             preview: Preview,
-            imageLoader: ImageLoader,
+            imageLoader: CachingImageLoader,
             isSharedTransitionElement: Boolean,
             previewReadyCallback: ((String) -> Unit)?
         ) {
@@ -334,11 +334,11 @@ class ScrollableImagePreviewView : RecyclerView, ImagePreviewView {
             }
         }
 
-        private suspend fun loadImage(preview: Preview, imageLoader: ImageLoader) {
-            val bitmap = preview.bitmap ?: runCatching {
+        private suspend fun loadImage(preview: Preview, imageLoader: CachingImageLoader) {
+            val bitmap = runCatching {
                 // it's expected for all loading/caching optimizations to be implemented by the
                 // loader
-                imageLoader(preview.uri)
+                imageLoader(preview.uri, true)
             }.getOrNull()
             image.setImageBitmap(bitmap)
         }
@@ -384,7 +384,7 @@ class ScrollableImagePreviewView : RecyclerView, ImagePreviewView {
 
     private class BatchPreviewLoader(
         private val adapter: Adapter,
-        private val imageLoader: ImageLoader,
+        private val imageLoader: CachingImageLoader,
         previews: List<Preview>,
         otherItemCount: Int,
         private val onNoPreviewCallback: (() -> Unit)
@@ -435,18 +435,15 @@ class ScrollableImagePreviewView : RecyclerView, ImagePreviewView {
                     launch {
                         while (pendingPreviews.isNotEmpty()) {
                             val preview = pendingPreviews.poll() ?: continue
+                            val isVisible = loadedPreviewWidth < maxWidth
                             val bitmap = runCatching {
                                 // TODO: decide on adding a timeout
-                                imageLoader(preview.uri)
+                                imageLoader(preview.uri, isVisible)
                             }.getOrNull() ?: continue
                             preview.updateAspectRatio(bitmap.width, bitmap.height)
                             updates.add(preview)
-                            if (loadedPreviewWidth < maxWidth) {
+                            if (isVisible) {
                                 loadedPreviewWidth += previewWidthCalculator(bitmap)
-                                // cache bitmaps for the first preview items to aovid potential
-                                // double-loading (in case those values are evicted from the image
-                                // loader's cache)
-                                preview.bitmap = bitmap
                                 if (loadedPreviewWidth >= maxWidth) {
                                     // notify that the preview now can be displayed
                                     reportFlow.emit(updateEvent)