Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Launch the Interceptor chain on the main thread by default. #513

Merged
merged 9 commits into from
Aug 31, 2020
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions coil-base/api/coil-base.api
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ public abstract interface class coil/ImageLoader {

public final class coil/ImageLoader$Builder {
public fun <init> (Landroid/content/Context;)V
public final fun addLastModifiedToFileCacheKey (Z)Lcoil/ImageLoader$Builder;
public final fun allowHardware (Z)Lcoil/ImageLoader$Builder;
public final fun allowRgb565 (Z)Lcoil/ImageLoader$Builder;
public final fun availableMemoryPercentage (D)Lcoil/ImageLoader$Builder;
Expand All @@ -108,6 +109,7 @@ public final class coil/ImageLoader$Builder {
public final fun eventListener (Lcoil/EventListener;)Lcoil/ImageLoader$Builder;
public final fun fallback (I)Lcoil/ImageLoader$Builder;
public final fun fallback (Landroid/graphics/drawable/Drawable;)Lcoil/ImageLoader$Builder;
public final fun launchInterceptorChainOnMainThread (Z)Lcoil/ImageLoader$Builder;
public final fun logger (Lcoil/util/Logger;)Lcoil/ImageLoader$Builder;
public final fun memoryCachePolicy (Lcoil/request/CachePolicy;)Lcoil/ImageLoader$Builder;
public final fun networkCachePolicy (Lcoil/request/CachePolicy;)Lcoil/ImageLoader$Builder;
Expand Down
23 changes: 23 additions & 0 deletions coil-base/src/androidTest/java/coil/RealImageLoaderTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ import coil.util.Utils
import coil.util.createMockWebServer
import coil.util.decodeBitmapAsset
import coil.util.getDrawableCompat
import coil.util.runBlockingTest
import coil.util.size
import kotlinx.coroutines.CancellationException
import kotlinx.coroutines.Dispatchers
Expand Down Expand Up @@ -88,6 +89,8 @@ class RealImageLoaderTest {
callFactory = OkHttpClient(),
eventListenerFactory = EventListener.Factory.NONE,
componentRegistry = ComponentRegistry(),
addLastModifiedToFileCacheKey = true,
launchInterceptorChainOnMainThread = true,
logger = null
)
}
Expand Down Expand Up @@ -397,6 +400,26 @@ class RealImageLoaderTest {
}
}

@Test
fun cachedValueIsResolvedSynchronously() = runBlockingTest {
val key = MemoryCache.Key("fake_key")
val fileName = "normal.jpg"
decodeAssetAndAddToMemoryCache(key, fileName)

var isSuccessful = false
val request = ImageRequest.Builder(context)
.data("$SCHEME_FILE:///$ASSET_FILE_PATH_ROOT/$fileName")
.size(100, 100)
.precision(Precision.INEXACT)
.memoryCacheKey(key)
.target { isSuccessful = true }
.build()
imageLoader.enqueue(request).dispose()

// isSuccessful should be synchronously set to true
assertTrue(isSuccessful)
}

private fun testEnqueue(data: Any, expectedSize: PixelSize = PixelSize(80, 100)) {
val imageView = ImageView(context)
imageView.scaleType = ImageView.ScaleType.FIT_CENTER
Expand Down
18 changes: 16 additions & 2 deletions coil-base/src/androidTest/java/coil/fetch/FileFetcherTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ class FileFetcherTest {

@Test
fun basic() {
val fetcher = FileFetcher()
val fetcher = FileFetcher(true)
val file = context.copyAssetToFile("normal.jpg")

assertTrue(fetcher.handles(file))
Expand All @@ -44,7 +44,7 @@ class FileFetcherTest {

@Test
fun fileCacheKeyWithLastModified() {
val fetcher = FileFetcher()
val fetcher = FileFetcher(true)
val file = context.copyAssetToFile("normal.jpg")

file.setLastModified(1234L)
Expand All @@ -55,4 +55,18 @@ class FileFetcherTest {

assertNotEquals(secondKey, firstKey)
}

@Test
fun fileCacheKeyWithoutLastModified() {
val fetcher = FileFetcher(false)
val file = context.copyAssetToFile("normal.jpg")

file.setLastModified(1234L)
val firstKey = fetcher.key(file)

file.setLastModified(4321L)
val secondKey = fetcher.key(file)

assertEquals(secondKey, firstKey)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,8 @@ class SystemCallbacksTest {
callFactory = OkHttpClient(),
eventListenerFactory = EventListener.Factory.NONE,
componentRegistry = ComponentRegistry(),
addLastModifiedToFileCacheKey = true,
launchInterceptorChainOnMainThread = true,
logger = null
)
val systemCallbacks = SystemCallbacks(imageLoader, context)
Expand Down
5 changes: 3 additions & 2 deletions coil-base/src/main/java/coil/EventListener.kt
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package coil

import android.graphics.Bitmap
import androidx.annotation.AnyThread
import androidx.annotation.MainThread
import androidx.annotation.WorkerThread
import coil.annotation.ExperimentalCoilApi
Expand Down Expand Up @@ -53,7 +54,7 @@ interface EventListener : ImageRequest.Listener {
*
* @param input The data that will be converted.
*/
@WorkerThread
@AnyThread
fun mapStart(request: ImageRequest, input: Any) {}

/**
Expand All @@ -62,7 +63,7 @@ interface EventListener : ImageRequest.Listener {
* @param output The data after it has been converted. If there were no applicable mappers,
* [output] will be the same as [ImageRequest.data].
*/
@WorkerThread
@AnyThread
fun mapEnd(request: ImageRequest, output: Any) {}

/**
Expand Down
48 changes: 48 additions & 0 deletions coil-base/src/main/java/coil/ImageLoader.kt
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@ import coil.bitmap.EmptyBitmapReferenceCounter
import coil.bitmap.RealBitmapPool
import coil.bitmap.RealBitmapReferenceCounter
import coil.drawable.CrossfadeDrawable
import coil.fetch.Fetcher
import coil.intercept.Interceptor
import coil.map.Mapper
import coil.memory.EmptyWeakMemoryCache
import coil.memory.MemoryCache
import coil.memory.RealWeakMemoryCache
Expand All @@ -37,8 +40,11 @@ import coil.util.getDrawableCompat
import coil.util.lazyCallFactory
import kotlinx.coroutines.CoroutineDispatcher
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.MainCoroutineDispatcher
import kotlinx.coroutines.withContext
import okhttp3.Call
import okhttp3.OkHttpClient
import java.io.File

/**
* A service class that loads images by executing [ImageRequest]s. Image loaders handle caching, data fetching,
Expand Down Expand Up @@ -133,7 +139,9 @@ interface ImageLoader {

private var availableMemoryPercentage = Utils.getDefaultAvailableMemoryPercentage(applicationContext)
private var bitmapPoolPercentage = Utils.getDefaultBitmapPoolPercentage()
private var addLastModifiedToFileCacheKey = true
private var bitmapPoolingEnabled = true
private var launchInterceptorChainOnMainThread = true
private var trackWeakReferences = true

/**
Expand Down Expand Up @@ -262,6 +270,19 @@ interface ImageLoader {
this.defaults = this.defaults.copy(allowRgb565 = enable)
}

/**
* Enables adding [File.lastModified] to the memory cache key when loading an image from a [File].
*
* This allows subsequent requests that load the same file to miss the memory cache if the file has been updated.
* However, if the memory cache check occurs on the main thread (see [launchInterceptorChainOnMainThread])
* calling [File.lastModified] will cause a strict mode violation.
*
* Default: true
*/
fun addLastModifiedToFileCacheKey(enable: Boolean) = apply {
this.addLastModifiedToFileCacheKey = enable
}

/**
* Enables counting references to bitmaps so they can be automatically reused by a [BitmapPool]
* when their reference count reaches zero.
Expand All @@ -277,6 +298,31 @@ interface ImageLoader {
this.bitmapPoolingEnabled = enable
}

/**
* Enables launching the [Interceptor] chain on the main thread.
*
* If true, the [Interceptor] chain will be launched from [MainCoroutineDispatcher.immediate]. This allows
* the [ImageLoader] to check its memory cache and return a cached value synchronously if the request is
* started from the main thread. However, [Mapper.map] and [Fetcher.key] operations will be executed on the
* main thread as well, which has a performance cost.
*
* If false, the [Interceptor] chain will be launched from the request's [ImageRequest.dispatcher].
* This will result in better UI performance, but values from the memory cache will not be resolved
* synchronously.
*
* The actual fetch + decode process always occurs on [ImageRequest.dispatcher] and is unaffected by this flag.
*
* It's worth noting that [Interceptor]s can also control which [CoroutineDispatcher] the
* memory cache is checked on by calling [Interceptor.Chain.proceed] inside a [withContext] block.
* Therefore if you set [launchInterceptorChainOnMainThread] to true, you can control which [ImageRequest]s
* check the memory cache synchronously at runtime.
*
* Default: true
*/
fun launchInterceptorChainOnMainThread(enable: Boolean) = apply {
this.launchInterceptorChainOnMainThread = enable
}

/**
* Enables weak reference tracking of loaded images.
*
Expand Down Expand Up @@ -456,6 +502,8 @@ interface ImageLoader {
callFactory = callFactory ?: buildDefaultCallFactory(),
eventListenerFactory = eventListenerFactory ?: EventListener.Factory.NONE,
componentRegistry = registry ?: ComponentRegistry(),
addLastModifiedToFileCacheKey = addLastModifiedToFileCacheKey,
launchInterceptorChainOnMainThread = launchInterceptorChainOnMainThread,
logger = logger
)
}
Expand Down
19 changes: 14 additions & 5 deletions coil-base/src/main/java/coil/RealImageLoader.kt
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,12 @@ import coil.fetch.BitmapFetcher
import coil.fetch.ContentUriFetcher
import coil.fetch.DrawableFetcher
import coil.fetch.FileFetcher
import coil.fetch.HttpUriFetcher
import coil.fetch.HttpUrlFetcher
import coil.fetch.ResourceUriFetcher
import coil.intercept.EngineInterceptor
import coil.intercept.RealInterceptorChain
import coil.map.FileUriMapper
import coil.map.HttpUriMapper
import coil.map.ResourceIntMapper
import coil.map.ResourceUriMapper
import coil.map.StringMapper
Expand Down Expand Up @@ -78,6 +78,8 @@ internal class RealImageLoader(
callFactory: Call.Factory,
private val eventListenerFactory: EventListener.Factory,
componentRegistry: ComponentRegistry,
addLastModifiedToFileCacheKey: Boolean,
private val launchInterceptorChainOnMainThread: Boolean,
val logger: Logger?
) : ImageLoader {

Expand All @@ -92,13 +94,13 @@ internal class RealImageLoader(
private val registry = componentRegistry.newBuilder()
// Mappers
.add(StringMapper())
.add(HttpUriMapper())
.add(FileUriMapper())
.add(ResourceUriMapper(context))
.add(ResourceIntMapper(context))
// Fetchers
.add(HttpUriFetcher(callFactory))
.add(HttpUrlFetcher(callFactory))
.add(FileFetcher())
.add(FileFetcher(addLastModifiedToFileCacheKey))
.add(AssetUriFetcher(context))
.add(ContentUriFetcher(context))
.add(ResourceUriFetcher(context, drawableDecoder))
Expand Down Expand Up @@ -227,8 +229,15 @@ internal class RealImageLoader(
size: Size,
cached: Bitmap?,
eventListener: EventListener
): ImageResult = withContext(request.dispatcher) {
RealInterceptorChain(request, type, interceptors, 0, request, size, cached, eventListener).proceed(request)
): ImageResult {
val chain = RealInterceptorChain(request, type, interceptors, 0, request, size, cached, eventListener)
return if (launchInterceptorChainOnMainThread) {
chain.proceed(request)
} else {
withContext(request.dispatcher) {
chain.proceed(request)
}
}
}

private suspend inline fun onSuccess(
Expand Down
2 changes: 1 addition & 1 deletion coil-base/src/main/java/coil/fetch/Fetcher.kt
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import okio.BufferedSource
* To accomplish this, fetchers fit into one of two types:
*
* - Uses the data as a key to fetch bytes from a remote source (e.g. network or disk)
* and exposes it as a [BufferedSource]. e.g. [HttpUrlFetcher]
* and exposes it as a [BufferedSource]. e.g. [HttpFetcher]
* - Reads the data directly and translates it into a [Drawable]. e.g. [BitmapFetcher]
*/
interface Fetcher<T : Any> {
Expand Down
6 changes: 4 additions & 2 deletions coil-base/src/main/java/coil/fetch/FileFetcher.kt
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,11 @@ import okio.buffer
import okio.source
import java.io.File

internal class FileFetcher : Fetcher<File> {
internal class FileFetcher(private val addLastModifiedToFileCacheKey: Boolean) : Fetcher<File> {

override fun key(data: File) = "${data.path}:${data.lastModified()}"
override fun key(data: File): String {
return if (addLastModifiedToFileCacheKey) "${data.path}:${data.lastModified()}" else data.path
}

override suspend fun fetch(
pool: BitmapPool,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
package coil.fetch

import android.net.Uri
import android.webkit.MimeTypeMap
import androidx.annotation.VisibleForTesting
import coil.bitmap.BitmapPool
import coil.decode.DataSource
import coil.decode.Options
import coil.map.Mapper
import coil.network.HttpException
import coil.size.Size
import coil.util.await
Expand All @@ -15,17 +17,38 @@ import okhttp3.HttpUrl
import okhttp3.Request
import okhttp3.ResponseBody

internal class HttpUrlFetcher(private val callFactory: Call.Factory) : Fetcher<HttpUrl> {
internal class HttpUriFetcher(callFactory: Call.Factory) : HttpFetcher<Uri>(callFactory) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Restoring the optimization to call HttpUrl.get as part of Fetcher.fetch instead of part of Mapper.map as Fetcher.fetch is always called on a background thread.


override fun handles(data: Uri) = data.scheme == "http" || data.scheme == "https"

override fun key(data: Uri) = data.toString()

override fun Uri.toHttpUrl(): HttpUrl = HttpUrl.get(toString())
}

internal class HttpUrlFetcher(callFactory: Call.Factory) : HttpFetcher<HttpUrl>(callFactory) {

override fun key(data: HttpUrl) = data.toString()

override fun HttpUrl.toHttpUrl(): HttpUrl = this
}

internal abstract class HttpFetcher<T : Any>(private val callFactory: Call.Factory) : Fetcher<T> {

/**
* Perform this conversion in a [Fetcher] instead of a [Mapper] so
* [HttpUriFetcher] can execute [HttpUrl.get] on a background thread.
*/
abstract fun T.toHttpUrl(): HttpUrl

override suspend fun fetch(
pool: BitmapPool,
data: HttpUrl,
data: T,
size: Size,
options: Options
): FetchResult {
val request = Request.Builder().url(data).headers(options.headers)
val url = data.toHttpUrl()
val request = Request.Builder().url(url).headers(options.headers)

val networkRead = options.networkCachePolicy.readEnabled
val diskRead = options.diskCachePolicy.readEnabled
Expand All @@ -52,7 +75,7 @@ internal class HttpUrlFetcher(private val callFactory: Call.Factory) : Fetcher<H

return SourceResult(
source = body.source(),
mimeType = getMimeType(data, body),
mimeType = getMimeType(url, body),
dataSource = if (response.cacheResponse() != null) DataSource.DISK else DataSource.NETWORK
)
}
Expand Down
Loading