Skip to content

Commit

Permalink
Fix memory cache misses in some cases where precision is not set. (#2680
Browse files Browse the repository at this point in the history 
)

* Fix scale not being properly passed around.

* Update the request instead of the defaults.
  • Loading branch information
@colinrtwhite
colinrtwhite authored Nov 13, 2024
1 parent 69e1395 commit beebb0a
Show file tree
Hide file tree
Showing 17 changed files with 131 additions and 131 deletions.
4 changes: 2 additions & 2 deletions coil-compose-core/src/androidInstrumentedTest/kotlin/coil3/compose/AsyncImageTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,13 +58,13 @@ import coil3.size.Scale
import coil3.test.utils.ComposeTestActivity
import coil3.test.utils.context
import java.util.concurrent.atomic.AtomicInteger
import kotlin.coroutines.EmptyCoroutineContext
import kotlin.test.assertContains
import kotlin.test.assertEquals
import kotlin.test.assertIs
import kotlin.test.assertTrue
import kotlin.test.fail
import kotlin.time.Duration.Companion.milliseconds
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.delay
import kotlinx.coroutines.flow.flow
import org.junit.After
Expand Down Expand Up @@ -577,7 +577,7 @@ class AsyncImageTest {
model = ImageRequest.Builder(LocalContext.current)
.data("https://example.com/image")
.memoryCachePolicy(CachePolicy.ENABLED)
.coroutineContext(Dispatchers.Main.immediate)
.coroutineContext(EmptyCoroutineContext)
.build(),
contentDescription = null,
imageLoader = imageLoader,
Expand Down
4 changes: 3 additions & 1 deletion coil-core/api/android/coil-core.api
View file
Original file line number Diff line number Diff line change
Expand Up @@ -665,8 +665,10 @@ public final class coil3/request/ImageRequest$Defaults {
public fun <init> ()V
public fun <init> (Lokio/FileSystem;Lkotlin/coroutines/CoroutineContext;Lkotlin/coroutines/CoroutineContext;Lkotlin/coroutines/CoroutineContext;Lcoil3/request/CachePolicy;Lcoil3/request/CachePolicy;Lcoil3/request/CachePolicy;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function1;Lcoil3/size/SizeResolver;Lcoil3/size/Scale;Lcoil3/size/Precision;Lcoil3/Extras;)V
public synthetic fun <init> (Lokio/FileSystem;Lkotlin/coroutines/CoroutineContext;Lkotlin/coroutines/CoroutineContext;Lkotlin/coroutines/CoroutineContext;Lcoil3/request/CachePolicy;Lcoil3/request/CachePolicy;Lcoil3/request/CachePolicy;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function1;Lcoil3/size/SizeResolver;Lcoil3/size/Scale;Lcoil3/size/Precision;Lcoil3/Extras;ILkotlin/jvm/internal/DefaultConstructorMarker;)V
public final fun copy (Lokio/FileSystem;Lkotlin/coroutines/CoroutineContext;Lkotlin/coroutines/CoroutineContext;Lkotlin/coroutines/CoroutineContext;Lcoil3/request/CachePolicy;Lcoil3/request/CachePolicy;Lcoil3/request/CachePolicy;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function1;Lcoil3/size/Precision;Lcoil3/Extras;)Lcoil3/request/ImageRequest$Defaults;
public final synthetic fun copy (Lokio/FileSystem;Lkotlin/coroutines/CoroutineContext;Lkotlin/coroutines/CoroutineContext;Lkotlin/coroutines/CoroutineContext;Lcoil3/request/CachePolicy;Lcoil3/request/CachePolicy;Lcoil3/request/CachePolicy;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function1;Lcoil3/size/Precision;Lcoil3/Extras;)Lcoil3/request/ImageRequest$Defaults;
public final fun copy (Lokio/FileSystem;Lkotlin/coroutines/CoroutineContext;Lkotlin/coroutines/CoroutineContext;Lkotlin/coroutines/CoroutineContext;Lcoil3/request/CachePolicy;Lcoil3/request/CachePolicy;Lcoil3/request/CachePolicy;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function1;Lcoil3/size/SizeResolver;Lcoil3/size/Scale;Lcoil3/size/Precision;Lcoil3/Extras;)Lcoil3/request/ImageRequest$Defaults;
public static synthetic fun copy$default (Lcoil3/request/ImageRequest$Defaults;Lokio/FileSystem;Lkotlin/coroutines/CoroutineContext;Lkotlin/coroutines/CoroutineContext;Lkotlin/coroutines/CoroutineContext;Lcoil3/request/CachePolicy;Lcoil3/request/CachePolicy;Lcoil3/request/CachePolicy;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function1;Lcoil3/size/Precision;Lcoil3/Extras;ILjava/lang/Object;)Lcoil3/request/ImageRequest$Defaults;
public static synthetic fun copy$default (Lcoil3/request/ImageRequest$Defaults;Lokio/FileSystem;Lkotlin/coroutines/CoroutineContext;Lkotlin/coroutines/CoroutineContext;Lkotlin/coroutines/CoroutineContext;Lcoil3/request/CachePolicy;Lcoil3/request/CachePolicy;Lcoil3/request/CachePolicy;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function1;Lcoil3/size/SizeResolver;Lcoil3/size/Scale;Lcoil3/size/Precision;Lcoil3/Extras;ILjava/lang/Object;)Lcoil3/request/ImageRequest$Defaults;
public fun equals (Ljava/lang/Object;)Z
public final fun getDecoderCoroutineContext ()Lkotlin/coroutines/CoroutineContext;
public final fun getDiskCachePolicy ()Lcoil3/request/CachePolicy;
Expand Down
1 change: 1 addition & 0 deletions coil-core/api/coil-core.klib.api
View file
Original file line number Diff line number Diff line change
Expand Up @@ -659,6 +659,7 @@ final class coil3.request/ImageRequest { // coil3.request/ImageRequest|null[0]
final fun <get-sizeResolver>(): coil3.size/SizeResolver // coil3.request/ImageRequest.Defaults.sizeResolver.<get-sizeResolver>|<get-sizeResolver>(){}[0]

final fun copy(okio/FileSystem = ..., kotlin.coroutines/CoroutineContext = ..., kotlin.coroutines/CoroutineContext = ..., kotlin.coroutines/CoroutineContext = ..., coil3.request/CachePolicy = ..., coil3.request/CachePolicy = ..., coil3.request/CachePolicy = ..., kotlin/Function1<coil3.request/ImageRequest, coil3/Image?> = ..., kotlin/Function1<coil3.request/ImageRequest, coil3/Image?> = ..., kotlin/Function1<coil3.request/ImageRequest, coil3/Image?> = ..., coil3.size/Precision = ..., coil3/Extras = ...): coil3.request/ImageRequest.Defaults // coil3.request/ImageRequest.Defaults.copy|copy(okio.FileSystem;kotlin.coroutines.CoroutineContext;kotlin.coroutines.CoroutineContext;kotlin.coroutines.CoroutineContext;coil3.request.CachePolicy;coil3.request.CachePolicy;coil3.request.CachePolicy;kotlin.Function1<coil3.request.ImageRequest,coil3.Image?>;kotlin.Function1<coil3.request.ImageRequest,coil3.Image?>;kotlin.Function1<coil3.request.ImageRequest,coil3.Image?>;coil3.size.Precision;coil3.Extras){}[0]
final fun copy(okio/FileSystem = ..., kotlin.coroutines/CoroutineContext = ..., kotlin.coroutines/CoroutineContext = ..., kotlin.coroutines/CoroutineContext = ..., coil3.request/CachePolicy = ..., coil3.request/CachePolicy = ..., coil3.request/CachePolicy = ..., kotlin/Function1<coil3.request/ImageRequest, coil3/Image?> = ..., kotlin/Function1<coil3.request/ImageRequest, coil3/Image?> = ..., kotlin/Function1<coil3.request/ImageRequest, coil3/Image?> = ..., coil3.size/SizeResolver = ..., coil3.size/Scale = ..., coil3.size/Precision = ..., coil3/Extras = ...): coil3.request/ImageRequest.Defaults // coil3.request/ImageRequest.Defaults.copy|copy(okio.FileSystem;kotlin.coroutines.CoroutineContext;kotlin.coroutines.CoroutineContext;kotlin.coroutines.CoroutineContext;coil3.request.CachePolicy;coil3.request.CachePolicy;coil3.request.CachePolicy;kotlin.Function1<coil3.request.ImageRequest,coil3.Image?>;kotlin.Function1<coil3.request.ImageRequest,coil3.Image?>;kotlin.Function1<coil3.request.ImageRequest,coil3.Image?>;coil3.size.SizeResolver;coil3.size.Scale;coil3.size.Precision;coil3.Extras){}[0]
final fun equals(kotlin/Any?): kotlin/Boolean // coil3.request/ImageRequest.Defaults.equals|equals(kotlin.Any?){}[0]
final fun hashCode(): kotlin/Int // coil3.request/ImageRequest.Defaults.hashCode|hashCode(){}[0]
final fun toString(): kotlin/String // coil3.request/ImageRequest.Defaults.toString|toString(){}[0]
Expand Down
4 changes: 3 additions & 1 deletion coil-core/api/jvm/coil-core.api
View file
Original file line number Diff line number Diff line change
Expand Up @@ -582,8 +582,10 @@ public final class coil3/request/ImageRequest$Defaults {
public fun <init> ()V
public fun <init> (Lokio/FileSystem;Lkotlin/coroutines/CoroutineContext;Lkotlin/coroutines/CoroutineContext;Lkotlin/coroutines/CoroutineContext;Lcoil3/request/CachePolicy;Lcoil3/request/CachePolicy;Lcoil3/request/CachePolicy;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function1;Lcoil3/size/SizeResolver;Lcoil3/size/Scale;Lcoil3/size/Precision;Lcoil3/Extras;)V
public synthetic fun <init> (Lokio/FileSystem;Lkotlin/coroutines/CoroutineContext;Lkotlin/coroutines/CoroutineContext;Lkotlin/coroutines/CoroutineContext;Lcoil3/request/CachePolicy;Lcoil3/request/CachePolicy;Lcoil3/request/CachePolicy;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function1;Lcoil3/size/SizeResolver;Lcoil3/size/Scale;Lcoil3/size/Precision;Lcoil3/Extras;ILkotlin/jvm/internal/DefaultConstructorMarker;)V
public final fun copy (Lokio/FileSystem;Lkotlin/coroutines/CoroutineContext;Lkotlin/coroutines/CoroutineContext;Lkotlin/coroutines/CoroutineContext;Lcoil3/request/CachePolicy;Lcoil3/request/CachePolicy;Lcoil3/request/CachePolicy;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function1;Lcoil3/size/Precision;Lcoil3/Extras;)Lcoil3/request/ImageRequest$Defaults;
public final synthetic fun copy (Lokio/FileSystem;Lkotlin/coroutines/CoroutineContext;Lkotlin/coroutines/CoroutineContext;Lkotlin/coroutines/CoroutineContext;Lcoil3/request/CachePolicy;Lcoil3/request/CachePolicy;Lcoil3/request/CachePolicy;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function1;Lcoil3/size/Precision;Lcoil3/Extras;)Lcoil3/request/ImageRequest$Defaults;
public final fun copy (Lokio/FileSystem;Lkotlin/coroutines/CoroutineContext;Lkotlin/coroutines/CoroutineContext;Lkotlin/coroutines/CoroutineContext;Lcoil3/request/CachePolicy;Lcoil3/request/CachePolicy;Lcoil3/request/CachePolicy;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function1;Lcoil3/size/SizeResolver;Lcoil3/size/Scale;Lcoil3/size/Precision;Lcoil3/Extras;)Lcoil3/request/ImageRequest$Defaults;
public static synthetic fun copy$default (Lcoil3/request/ImageRequest$Defaults;Lokio/FileSystem;Lkotlin/coroutines/CoroutineContext;Lkotlin/coroutines/CoroutineContext;Lkotlin/coroutines/CoroutineContext;Lcoil3/request/CachePolicy;Lcoil3/request/CachePolicy;Lcoil3/request/CachePolicy;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function1;Lcoil3/size/Precision;Lcoil3/Extras;ILjava/lang/Object;)Lcoil3/request/ImageRequest$Defaults;
public static synthetic fun copy$default (Lcoil3/request/ImageRequest$Defaults;Lokio/FileSystem;Lkotlin/coroutines/CoroutineContext;Lkotlin/coroutines/CoroutineContext;Lkotlin/coroutines/CoroutineContext;Lcoil3/request/CachePolicy;Lcoil3/request/CachePolicy;Lcoil3/request/CachePolicy;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function1;Lcoil3/size/SizeResolver;Lcoil3/size/Scale;Lcoil3/size/Precision;Lcoil3/Extras;ILjava/lang/Object;)Lcoil3/request/ImageRequest$Defaults;
public fun equals (Ljava/lang/Object;)Z
public final fun getDecoderCoroutineContext ()Lkotlin/coroutines/CoroutineContext;
public final fun getDiskCachePolicy ()Lcoil3/request/CachePolicy;
Expand Down
3 changes: 2 additions & 1 deletion coil-core/src/androidInstrumentedTest/kotlin/coil3/RealImageLoaderAndroidTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ import coil3.util.getDrawableCompat
import coil3.util.toDrawable
import java.io.File
import java.nio.ByteBuffer
import kotlin.coroutines.EmptyCoroutineContext
import kotlin.coroutines.resume
import kotlin.coroutines.resumeWithException
import kotlin.math.roundToInt
Expand Down Expand Up @@ -268,7 +269,7 @@ class RealImageLoaderAndroidTest {
.size(100, 100)
.precision(Precision.INEXACT)
.allowHardware(true)
.coroutineContext(Dispatchers.Main.immediate)
.coroutineContext(EmptyCoroutineContext)
.target(
onStart = {
// The drawable in the memory cache should be returned here.
Expand Down
71 changes: 37 additions & 34 deletions coil-core/src/androidMain/kotlin/coil3/request/RequestService.android.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import coil3.BitmapImage
import coil3.Extras
import coil3.ImageLoader
import coil3.memory.MemoryCache
import coil3.size.Dimension
import coil3.size.Precision
import coil3.size.Scale
import coil3.size.Size
Expand Down Expand Up @@ -70,32 +69,33 @@ internal class AndroidRequestService(
return context.getLifecycle()
}

override fun sizeResolver(request: ImageRequest): SizeResolver {
if (request.defined.sizeResolver != null) {
return request.defined.sizeResolver
override fun updateRequest(request: ImageRequest): ImageRequest {
val builder = request.newBuilder()
.defaults(imageLoader.defaults)

var sizeResolver = request.defined.sizeResolver
if (sizeResolver == null) {
sizeResolver = request.resolveSizeResolver()
builder.size(sizeResolver)
}

val target = request.target
if (target is ViewTarget<*>) {
// CENTER and MATRIX scale types should be decoded at the image's original size.
val view = target.view
if (view is ImageView && view.scaleType.let { it == CENTER || it == MATRIX }) {
return SizeResolver.ORIGINAL
} else {
return ViewSizeResolver(view)
}
} else {
// Fall back to the image's source dimensions.
return SizeResolver.ORIGINAL
if (request.defined.scale == null) {
builder.scale(request.resolveScale())
}

if (request.defined.precision == null) {
builder.precision(request.resolvePrecision(sizeResolver))
}

return builder.build()
}

override fun options(request: ImageRequest, sizeResolver: SizeResolver, size: Size): Options {
override fun options(request: ImageRequest, size: Size): Options {
return Options(
request.context,
size,
request.resolveScale(size),
request.resolvePrecision(sizeResolver),
request.scale,
request.precision,
request.diskCacheKey,
request.fileSystem,
request.memoryCachePolicy,
Expand All @@ -105,16 +105,22 @@ internal class AndroidRequestService(
)
}

private fun ImageRequest.resolveScale(size: Size): Scale {
if (defined.scale != null) {
return defined.scale
}

// Use `Scale.FIT` if either dimension is undefined.
if (size.width == Dimension.Undefined || size.height == Dimension.Undefined) {
return Scale.FIT
private fun ImageRequest.resolveSizeResolver(): SizeResolver {
if (target is ViewTarget<*>) {
// CENTER and MATRIX scale types should be decoded at the image's original size.
val view = target.view
if (view is ImageView && view.scaleType.let { it == CENTER || it == MATRIX }) {
return SizeResolver.ORIGINAL
} else {
return ViewSizeResolver(view)
}
} else {
// Fall back to the image's source dimensions.
return SizeResolver.ORIGINAL
}
}

private fun ImageRequest.resolveScale(): Scale {
// Autodetect the scale from the ImageView.
val imageView = (target as? ViewTarget<*>)?.view as? ImageView
if (imageView != null) {
Expand All @@ -125,10 +131,7 @@ internal class AndroidRequestService(
}

private fun ImageRequest.resolvePrecision(sizeResolver: SizeResolver): Precision {
if (defined.precision != null) {
return defined.precision
}

// Use inexact precision if we're falling back to the source dimensions.
if (defined.sizeResolver == null && sizeResolver == SizeResolver.ORIGINAL) {
return Precision.INEXACT
}
Expand Down Expand Up @@ -173,7 +176,7 @@ internal class AndroidRequestService(
return builder.build()
}

override fun updateOptionsOnWorkerThread(options: Options): Options {
override fun updateOptions(options: Options): Options {
var extras = options.extras
var changed = false

Expand Down Expand Up @@ -233,7 +236,7 @@ internal class AndroidRequestService(
}

/** Return 'true' if the current bitmap config is valid, else use [Bitmap.Config.ARGB_8888]. */
fun isBitmapConfigValidMainThread(
private fun isBitmapConfigValidMainThread(
request: ImageRequest,
size: Size,
): Boolean {
Expand All @@ -246,7 +249,7 @@ internal class AndroidRequestService(
}

/** Return 'true' if the current bitmap config is valid, else use [Bitmap.Config.ARGB_8888]. */
fun isBitmapConfigValidWorkerThread(
private fun isBitmapConfigValidWorkerThread(
options: Options,
): Boolean {
return !options.bitmapConfig.isHardware || hardwareBitmapService.allowHardwareWorkerThread()
Expand Down
19 changes: 1 addition & 18 deletions coil-core/src/androidMain/kotlin/coil3/transition/CrossfadeTransition.kt
View file
Original file line number Diff line number Diff line change
@@ -1,15 +1,12 @@
package coil3.transition

import android.widget.ImageView
import coil3.asDrawable
import coil3.asImage
import coil3.decode.DataSource
import coil3.request.DEFAULT_CROSSFADE_MILLIS
import coil3.request.ErrorResult
import coil3.request.ImageResult
import coil3.request.SuccessResult
import coil3.size.Scale
import coil3.util.scale

/**
* A [Transition] that crossfades from the current drawable to a new one.
Expand All @@ -32,7 +29,7 @@ class CrossfadeTransition @JvmOverloads constructor(
val drawable = CrossfadeDrawable(
start = target.drawable,
end = result.image?.asDrawable(target.view.resources),
scale = scale(),
scale = result.request.scale,
durationMillis = durationMillis,
fadeStart = result !is SuccessResult || !result.isPlaceholderCached,
preferExactIntrinsicSize = preferExactIntrinsicSize,
Expand All @@ -43,20 +40,6 @@ class CrossfadeTransition @JvmOverloads constructor(
}
}

private fun scale(): Scale {
val scale = result.request.defined.scale
if (scale != null) {
return scale
}

val view = target.view
if (view is ImageView) {
return view.scale
}

return result.request.scale
}

class Factory @JvmOverloads constructor(
val durationMillis: Int = DEFAULT_CROSSFADE_MILLIS,
val preferExactIntrinsicSize: Boolean = false,
Expand Down
Loading

0 comments on commit beebb0a

Please sign in to comment.