Fix memory cache miss when Transformation reduces input bitmap's size.

coil-base/api/coil-base.api

@@ -577,8 +577,9 @@ public final class coil/request/Parameters : java/lang/Iterable, kotlin/jvm/inte
 	public static final field Companion Lcoil/request/Parameters$Companion;
 	public static final field EMPTY Lcoil/request/Parameters;
 	public fun <init> ()V
-	public synthetic fun <init> (Ljava/util/SortedMap;Lkotlin/jvm/internal/DefaultConstructorMarker;)V
+	public synthetic fun <init> (Ljava/util/Map;Lkotlin/jvm/internal/DefaultConstructorMarker;)V
 	public final fun cacheKey (Ljava/lang/String;)Ljava/lang/String;
+	public final fun cacheKeys ()Ljava/util/Map;
 	public final fun count ()I
 	public final fun entry (Ljava/lang/String;)Lcoil/request/Parameters$Entry;
 	public fun equals (Ljava/lang/Object;)Z
@@ -588,6 +589,7 @@ public final class coil/request/Parameters : java/lang/Iterable, kotlin/jvm/inte
 	public final fun newBuilder ()Lcoil/request/Parameters$Builder;
 	public fun toString ()Ljava/lang/String;
 	public final fun value (Ljava/lang/String;)Ljava/lang/Object;
+	public final fun values ()Ljava/util/Map;
 }
 
 public final class coil/request/Parameters$Builder {

coil-base/src/main/java/coil/RealImageLoader.kt

@@ -15,7 +15,6 @@ import coil.decode.DataSource
 import coil.decode.DrawableDecoderService
 import coil.decode.EmptyDecoder
 import coil.decode.Options
-import coil.extension.isNotEmpty
 import coil.fetch.AssetUriFetcher
 import coil.fetch.BitmapFetcher
 import coil.fetch.ContentUriFetcher
@@ -196,12 +195,13 @@ internal class RealImageLoader(
 
             // Compute the cache key.
             val fetcher = request.validateFetcher(mappedData) ?: registry.requireFetcher(mappedData)
-            val cacheKey = request.key ?: computeCacheKey(fetcher, mappedData, request.parameters, request.transformations, lazySizeResolver)
+            val cacheKey = request.key?.let { MemoryCache.Key(it) }
+                ?: computeCacheKey(fetcher, mappedData, request.parameters, request.transformations, lazySizeResolver)
 
             // Check the memory cache.
             val memoryCachePolicy = request.memoryCachePolicy ?: defaults.memoryCachePolicy
             val cachedValue = takeIf(memoryCachePolicy.readEnabled) {
-                memoryCache.getValue(cacheKey) ?: request.aliasKeys.firstNotNullIndices { memoryCache.getValue(it) }
+                memoryCache.getValue(cacheKey) ?: request.aliasKeys.firstNotNullIndices { memoryCache.getValue(MemoryCache.Key(it)) }
             }
 
             // Ignore the cached bitmap if it is hardware-backed and the request disallows hardware bitmaps.
@@ -216,7 +216,7 @@ internal class RealImageLoader(
             val scale = requestService.scale(request, sizeResolver)
 
             // Short circuit if the cached drawable is valid for the target.
-            if (cachedDrawable != null && memoryCacheService.isCachedDrawableValid(cachedDrawable, cachedValue.isSampled, request, sizeResolver, size, scale)) {
+            if (cachedDrawable != null && memoryCacheService.isCachedDrawableValid(cacheKey, cachedValue, request, sizeResolver, size, scale)) {
                 logger?.log(TAG, Log.INFO) { "${Emoji.BRAIN} Cached - $data" }
                 targetDelegate.success(cachedDrawable, true, request.transition ?: defaults.transition)
                 eventListener.onSuccess(request, DataSource.MEMORY)
@@ -298,26 +298,14 @@ internal class RealImageLoader(
         parameters: Parameters,
         transformations: List<Transformation>,
         lazySizeResolver: LazySizeResolver
-    ): String? {
-        val baseCacheKey = fetcher.key(data) ?: return null
+    ): MemoryCache.Key? {
+        val baseKey = fetcher.key(data) ?: return null
 
-        return buildString(baseCacheKey.count()) {
-            append(baseCacheKey)
-
-            // Check isNotEmpty first to avoid allocating an Iterator.
-            if (parameters.isNotEmpty()) {
-                for ((key, entry) in parameters) {
-                    val cacheKey = entry.cacheKey ?: continue
-                    append('#').append(key).append('=').append(cacheKey)
-                }
-            }
-
-            if (transformations.isNotEmpty()) {
-                transformations.forEachIndices { append('#').append(it.key()) }
-
-                // Append the size if there are any transformations.
-                append('#').append(lazySizeResolver.size())
-            }
+        return if (transformations.isEmpty()) {
+            MemoryCache.Key(baseKey, parameters)
+        } else {
+            // Resolve the size if there are any transformations.
+            MemoryCache.Key(baseKey, transformations, lazySizeResolver.size(), parameters)
         }
     }
 
@@ -422,8 +410,9 @@ internal class RealImageLoader(
     }
 
     override fun invalidate(key: String) {
-        memoryCache.invalidate(key)
-        weakMemoryCache.invalidate(key)
+        val cacheKey = MemoryCache.Key(key)
+        memoryCache.invalidate(cacheKey)
+        weakMemoryCache.invalidate(cacheKey)
     }
 
     override fun onTrimMemory(level: Int) {

coil-base/src/main/java/coil/memory/MemoryCache.kt

@@ -7,10 +7,16 @@ import android.content.ComponentCallbacks2.TRIM_MEMORY_UI_HIDDEN
 import android.graphics.Bitmap
 import android.util.Log
 import androidx.collection.LruCache
+import coil.fetch.Fetcher
+import coil.memory.MemoryCache.Key
 import coil.memory.MemoryCache.Value
+import coil.request.Parameters
+import coil.size.Size
+import coil.transform.Transformation
 import coil.util.Logger
 import coil.util.getAllocationByteCountCompat
 import coil.util.log
+import coil.util.mapIndices
 
 /** An in-memory cache for [Bitmap]s. */
 internal interface MemoryCache {
@@ -31,10 +37,10 @@ internal interface MemoryCache {
     }
 
     /** Get the value associated with [key]. */
-    fun get(key: String): Value?
+    fun get(key: Key): Value?
 
     /** Set the value associated with [key]. */
-    fun set(key: String, bitmap: Bitmap, isSampled: Boolean)
+    fun set(key: Key, bitmap: Bitmap, isSampled: Boolean)
 
     /** Return the **current size** of the memory cache in bytes. */
     fun size(): Int
@@ -43,14 +49,80 @@ internal interface MemoryCache {
     fun maxSize(): Int
 
     /** Remove the value referenced by [key] from this cache if it is present. */
-    fun invalidate(key: String)
+    fun invalidate(key: Key)
 
     /** Remove all values from this cache. */
     fun clearMemory()
 
     /** @see ComponentCallbacks2.onTrimMemory */
     fun trimMemory(level: Int)
 
+    /** Cache key for [MemoryCache] and [WeakMemoryCache]. */
+    class Key {
+
+        /** The base component of the cache key. This is typically [Fetcher.key]. */
+        val baseKey: String
+
+        /** An ordered list of [Transformation.key]s. */
+        val transformationKeys: List<String>
+
+        /** The resolved size for the request. This is null if [transformationKeys] is empty. */
+        val size: Size?
+
+        /** @see Parameters.cacheKeys */
+        val parameterKeys: Map<String, String>
+
+        constructor(
+            baseKey: String,
+            parameters: Parameters = Parameters.EMPTY
+        ) {
+            this.baseKey = baseKey
+            this.transformationKeys = emptyList()
+            this.size = null
+            this.parameterKeys = parameters.cacheKeys()
+        }
+
+        constructor(
+            baseKey: String,
+            transformations: List<Transformation>,
+            size: Size,
+            parameters: Parameters = Parameters.EMPTY
+        ) {
+            this.baseKey = baseKey
+            if (transformations.isEmpty()) {
+                this.transformationKeys = emptyList()
+                this.size = null
+            } else {
+                this.transformationKeys = transformations.mapIndices { it.key() }
+                this.size = size
+            }
+            this.parameterKeys = parameters.cacheKeys()
+        }
+
+        override fun equals(other: Any?): Boolean {
+            if (this === other) return true
+            if (other !is Key) return false
+            if (baseKey != other.baseKey) return false
+            if (parameterKeys != other.parameterKeys) return false
+            if (transformationKeys != other.transformationKeys) return false
+            if (size != other.size) return false
+            return true
+        }
+
+        override fun hashCode(): Int {
+            var result = baseKey.hashCode()
+            result = 31 * result + parameterKeys.hashCode()
+            result = 31 * result + transformationKeys.hashCode()
+            result = 31 * result + (size?.hashCode() ?: 0)
+            return result
+        }
+
+        override fun toString(): String {
+            return "MemoryCache.Key(baseKey='$baseKey', transformationKeys=$transformationKeys, size=$size, parameterKeys=$parameterKeys)"
+        }
+    }
+
+    /** Cache value for [MemoryCache] and [WeakMemoryCache]. */
     interface Value {
         val bitmap: Bitmap
         val isSampled: Boolean
@@ -60,15 +132,15 @@ internal interface MemoryCache {
 /** A [MemoryCache] implementation that caches nothing. */
 private object EmptyMemoryCache : MemoryCache {
 
-    override fun get(key: String): Value? = null
+    override fun get(key: Key): Value? = null
 
-    override fun set(key: String, bitmap: Bitmap, isSampled: Boolean) {}
+    override fun set(key: Key, bitmap: Bitmap, isSampled: Boolean) {}
 
     override fun size(): Int = 0
 
     override fun maxSize(): Int = 0
 
-    override fun invalidate(key: String) {}
+    override fun invalidate(key: Key) {}
 
     override fun clearMemory() {}
 
@@ -80,17 +152,17 @@ private class ForwardingMemoryCache(
     private val weakMemoryCache: WeakMemoryCache
 ) : MemoryCache {
 
-    override fun get(key: String) = weakMemoryCache.get(key)
+    override fun get(key: Key) = weakMemoryCache.get(key)
 
-    override fun set(key: String, bitmap: Bitmap, isSampled: Boolean) {
+    override fun set(key: Key, bitmap: Bitmap, isSampled: Boolean) {
         weakMemoryCache.set(key, bitmap, isSampled, bitmap.getAllocationByteCountCompat())
     }
 
     override fun size() = 0
 
     override fun maxSize() = 0
 
-    override fun invalidate(key: String) {}
+    override fun invalidate(key: Key) {}
 
     override fun clearMemory() {}
 
@@ -109,10 +181,10 @@ private class RealMemoryCache(
         private const val TAG = "RealMemoryCache"
     }
 
-    private val cache = object : LruCache<String, InternalValue>(maxSize) {
+    private val cache = object : LruCache<Key, InternalValue>(maxSize) {
         override fun entryRemoved(
             evicted: Boolean,
-            key: String,
+            key: Key,
             oldValue: InternalValue,
             newValue: InternalValue?
         ) {
@@ -123,12 +195,12 @@ private class RealMemoryCache(
             }
         }
 
-        override fun sizeOf(key: String, value: InternalValue) = value.size
+        override fun sizeOf(key: Key, value: InternalValue) = value.size
     }
 
-    override fun get(key: String) = cache.get(key) ?: weakMemoryCache.get(key)
+    override fun get(key: Key) = cache.get(key) ?: weakMemoryCache.get(key)
 
-    override fun set(key: String, bitmap: Bitmap, isSampled: Boolean) {
+    override fun set(key: Key, bitmap: Bitmap, isSampled: Boolean) {
         // If the bitmap is too big for the cache, don't even attempt to store it. Doing so will cause
         // the cache to be cleared. Instead just evict an existing element with the same key if it exists.
         val size = bitmap.getAllocationByteCountCompat()
@@ -149,7 +221,7 @@ private class RealMemoryCache(
 
     override fun maxSize() = cache.maxSize()
 
-    override fun invalidate(key: String) {
+    override fun invalidate(key: Key) {
         logger?.log(TAG, Log.VERBOSE) { "invalidate, key=$key" }
         cache.remove(key)
     }

coil-base/src/main/java/coil/memory/MemoryCacheService.kt

@@ -1,7 +1,6 @@
 package coil.memory
 
 import android.graphics.Bitmap
-import android.graphics.drawable.BitmapDrawable
 import android.util.Log
 import coil.DefaultRequestOptions
 import coil.decode.DecodeUtils
@@ -30,65 +29,77 @@ internal class MemoryCacheService(
 
     /** Return true if the [Bitmap] returned from [MemoryCache] satisfies the [Request]. */
     fun isCachedDrawableValid(
-        cached: BitmapDrawable,
-        isSampled: Boolean,
+        cacheKey: MemoryCache.Key?,
+        cacheValue: MemoryCache.Value,
         request: Request,
         sizeResolver: SizeResolver,
         size: Size,
         scale: Scale
     ): Boolean {
         // Ensure the size is valid for the target.
-        val bitmap = cached.bitmap
         when (size) {
             is OriginalSize -> {
-                if (isSampled) {
+                if (cacheValue.isSampled) {
                     logger?.log(TAG, Log.DEBUG) {
                         "${request.data}: Requested original size, but cached image is sampled."
                     }
                     return false
                 }
             }
             is PixelSize -> {
+                val cachedWidth: Int
+                val cachedHeight: Int
+                when (val cachedSize = cacheKey?.size) {
+                    is PixelSize -> {
+                        cachedWidth = cachedSize.width
+                        cachedHeight = cachedSize.height
+                    }
+                    OriginalSize, null -> {
+                        val bitmap = cacheValue.bitmap
+                        cachedWidth = bitmap.width
+                        cachedHeight = bitmap.height
+                    }
+                }
                 val multiple = DecodeUtils.computeSizeMultiplier(
-                    srcWidth = bitmap.width,
-                    srcHeight = bitmap.height,
+                    srcWidth = cachedWidth,
+                    srcHeight = cachedHeight,
                     dstWidth = size.width,
                     dstHeight = size.height,
                     scale = scale
                 )
                 if (multiple != 1.0 && !requestService.allowInexactSize(request, sizeResolver)) {
                     logger?.log(TAG, Log.DEBUG) {
-                        "${request.data}: Cached image's size (${bitmap.width}, ${bitmap.height}) " +
-                            "does not exactly match the requested size (${size.width}, ${size.height})."
+                        "${request.data}: Cached image's request size ($cachedWidth, $cachedHeight) " +
+                            "does not exactly match the requested size (${size.width}, ${size.height}, $scale)."
                     }
                     return false
                 }
-                if (multiple > 1.0 && isSampled) {
+                if (multiple > 1.0 && cacheValue.isSampled) {
                     logger?.log(TAG, Log.DEBUG) {
-                        "${request.data}: Cached image's size (${bitmap.width}, ${bitmap.height}) " +
-                            "is smaller than the requested size (${size.width}, ${size.height})."
+                        "${request.data}: Cached image's request size ($cachedWidth, $cachedHeight) " +
+                            "is smaller than the requested size (${size.width}, ${size.height}, $scale)."
                     }
                     return false
                 }
             }
         }
 
         // Ensure we don't return a hardware bitmap if the request doesn't allow it.
-        if (!requestService.isConfigValidForHardware(request, bitmap.safeConfig)) {
+        if (!requestService.isConfigValidForHardware(request, cacheValue.bitmap.safeConfig)) {
             logger?.log(TAG, Log.DEBUG) {
                 "${request.data}: Cached bitmap is hardware-backed, which is incompatible with the request."
             }
             return false
         }
 
         // Allow returning a cached RGB_565 bitmap if allowRgb565 is enabled.
-        if ((request.allowRgb565 ?: defaults.allowRgb565) && bitmap.config == Bitmap.Config.RGB_565) {
+        if ((request.allowRgb565 ?: defaults.allowRgb565) && cacheValue.bitmap.config == Bitmap.Config.RGB_565) {
             return true
         }
 
         // Ensure the requested config matches the cached config.
         // Hardware and ARGB_8888 bitmaps are treated as equal for this comparison.
-        val cachedConfig = bitmap.config.toSoftware()
+        val cachedConfig = cacheValue.bitmap.config.toSoftware()
         val requestedConfig = request.bitmapConfigOrDefault(defaults).toSoftware()
         if (cachedConfig != requestedConfig) {
             logger?.log(TAG, Log.DEBUG) {