Placeholder issue with reloading same images on recyclerView when the images are not in memory

[seankim-android]

Hello - I’m running into some reload/flicker issues with gif/video frame support in RecyclerView. I think it might be related to #280

We have RecyclerViews where notifyDataSetChanged() is called under certain conditions (when clicking on the item and etc.). I’m noticing some flicker when the same images should be re-bind but they aren’t loaded from memory.

For example, when the same image (I’m mostly seeing gif or video frame) is loaded after notifyDataSetChanged() from DISK, I’m noticing the issue from step 4 & 5 below.

1. Coil image request starts and does fresh load for Image A
2. notifyDataSetChanged() is called
3. Coil image request starts again for Image A
4. onStart() called on the view target to load placeholder
5. Takes some time until onSuccess() to load from DISK

When I load with a custom ViewTarget and disable setting the placeholder, the step 4 doesn’t happen so the same image stays between the reload. This isn't a solution because I do need to clear out the image when a new load starts, but I cannot differentiate if the view is binding from notifyDataSetChanged() or if it is a fresh load. I’m hoping there is a way not to call onStart() if we’re requesting the same image.

While I’m looking into why I’m not seeing the issue on Glide, I was wondering if you could advise if this can be handled from Coil. Thank you 🙇‍♂️

The resulting experience looks like this

Code

From RecyclerView.Adapter

override fun onBindViewHolder(holder: SomeViewHolder, position: Int) {
  val request = ImageRequest.Builder(imageView.context)
    .data(uri)
    .target(imageView)
    .placeholder(R.color.some_placeholder)
    .build()
  myImageLoader.enqueue(request)
} 

override fun onViewRecycled(holder: SomeViewHolder) {
  CoilUtils.clear(holder.thumbnail)
}

[seankim-android]

The device in the GIF is an Android 8 device using GifDecoder. I'm seeing a better result (loads faster but still same issue) with Android 11 device using ImageDecoderDecoder.

It looks to me that the GifDecoder never returns a sampled gif result in https://github.com/coil-kt/coil/blob/main/coil-gif/src/main/java/coil/decode/GifDecoder.kt#L80-L83 vs. the ImageDecoderDecoder does return a sampled version https://github.com/coil-kt/coil/blob/main/coil-gif/src/main/java/coil/decode/ImageDecoderDecoder.kt#L108-L112. Is there a reason for this?

[seankim-android]

Adding some more gifs to help visualize the issue I'm seeing. Here two mountains images are .jpeg and others are .gif.

Android 8 GifDecoder	Android 11 ImageDecoderDecoder

Edit: Adding some logs with OnSuccess() metadata. They are from different sessions from the gif ☝️, but it is captured with same files and under same scenario. The ones with dataSource=DISK are .gif files.
Android 8 logs
Android 11 logs

[seankim-android]

Similar issue after adding video frame support with VideoFrameFileFetcher, VideoFrameUriFetcher, and VideoFrameDecoder.

Items are getting thumbnails from the video.

[seankim-android]

Added some code to Code section in the description.

[seankim-android]

Some more updates... It does look like the images have to be in-memory to avoid this issue on Glide as well.

When I set memoryCachePolicy(CachePolicy.DISABLED) to Coil requests, I'm seeing the same issue in any images - not just gifs or thumbnails from the embedded videos.
Also seeing same issue on Glide when skipMemoryCache(true) is set on GlideRequest. The images loaded from disk or network are resulting in the same UX.

However, even with the same set of images, the gifs and video frames result in memory most of the time on Glide. This was probably the reason why we did not see the issue before Coil. Another way to to increase the memcache hit ratio is to set downsample(DownsampleStrategy.AT_MOST) on the request and the downsampling is done at the decoder level on ImageDecoderResourceDecoder.

Do you have any plan or suggestions for supporting downsample image requests at the time of request creation? Please let me know if you have any other suggestions to increase memory hit with images from gif/video frame decoders.

[seankim-android]

Glide's gif downsizing done in ByteBufferGifDecoder : https://github.com/bumptech/glide/blob/master/library/src/main/java/com/bumptech/glide/load/resource/gif/ByteBufferGifDecoder.java#L134-L159

[seankim-android]

Also found this logic which might have been the reason why I saw the same issue when I set skipMemoryCache(true) on Glide. It will always ignore and re-set the placeholder if skipMemoryCache is true.

From: https://github.com/bumptech/glide/blob/master/library/src/main/java/com/bumptech/glide/RequestBuilder.java#L750-L753

private boolean isSkipMemoryCacheWithCompletePreviousRequest(
      BaseRequestOptions<?> options, Request previous) {
    return !options.isMemoryCacheable() && previous.isComplete();
 }

Looks like Glide is explicitly checking if the previous request was the same on the target. I think having something similar to this logic will help skipping the placeholder so the image can stay between a reload.
https://github.com/bumptech/glide/blob/master/library/src/main/java/com/bumptech/glide/RequestBuilder.java#L722-L743

From the comment:

Use the previous request rather than the new one to allow for optimizations like skipping
setting placeholders, tracking and un-tracking Targets, and obtaining View dimensions
that are done in the individual Request.

[seankim-android]

Updating my questions here based on my investigation above.

I think I need support for the items below to match the Glide behavior on our app. Is there a good way to support 1 & 2 below as of today?

1. Support for skipping the placeholder if the same requests consecutively happen on a view target - this happens with notifyDatasetChanged() in my case. PoolableViewTarget should not do this, but maybe with a custom ViewTarget.
2. Image requests with downsampling image request options and/or other solutions to increase memcache hit.

Other questions:

It looks to me that the GifDecoder never returns a sampled gif result in https://github.com/coil-kt/coil/blob/main/coil-gif/src/main/java/coil/decode/GifDecoder.kt#L80-L83 vs. the ImageDecoderDecoder does return a sampled version https://github.com/coil-kt/coil/blob/main/coil-gif/src/main/java/coil/decode/ImageDecoderDecoder.kt#L108-L112. Is there a reason for this?

[colinrtwhite]

@seankim-android From the videos it looks like you're running into the flickering behaviour as Coil doesn't keep animated images in its memory cache (due to a couple reasons including animated images can only be used by one target at a time whereas bitmaps can be shared). If you're reloading the recyclerview, you'll need keep a reference to the drawable returned by Coil to set it on the new ImageView.

If you need global memory caching of animated images, I'd recommend creating an Interceptor that caches the animated drawables based on their URL - similar to this code. You'll need to be careful that you don't use the same animated drawable in two places at the same time, though.

To answer your questions:

1. You can use a custom ImageViewTarget subclass that skips setting the drawable in onStart if the ImageView already has a drawable.
2. You can either increase the size of your memory cache or decrease the size of the loaded image. To decrease the size of the loaded image, you could subclass ViewSizeResolver and do something like override suspend fun size() = 0.5 * super.size() (not exact code).
3. Android's Movie class doesn't support downsampling GIFs.

[seankim-android]

@colinrtwhite Thank you for your response and explanations. Based on your response, I think skip setting the drawable in onStart is a more straightforward short-term fix for my issue.

I have some follow-up questions.

1.I wrote a similar explanation in the issue description, but I've tried using a custom ViewTarget subclass to skip setting the drawable in onStart. This doesn't work because it shouldn't skip setting the drawable all the time in onStart. It needs some logic to check if the current view.drawable is the resulting drawable of the new request. ImageViewTarget doesn't have any information about the associated ImageRequest so I was hoping to find an alternative approach.

What I'm looking for is similar to Glide's logic where it compares the current request with a new request from RequestBuilder.into. Then I can use this check to skip setting placeholders if the requests are the same. One idea I had was getting the request by the view through something like Coil's view.requestManager, but the request is currently not accessible.

2.ImageLoader already has a default memory caching implementation for Bitmap in ImageLoader.buildDefaultMemoryCache(). I'm a bit hesitant to add a custom memory cache layer for animated images as it can introduce overhead from unknowns and potential memory issues.

Ideally, if we allocate some memory for Coil's memory cache, I think it makes sense that it covers both bitmap and non-animated drawables. But I'm also not aware of all the reasons as to why it is currently not implemented as part of Coil's memory cache. What are your thoughts on adding animated image cache to Coil and have both animated images & bitmap cache managed by the library? I'm thinking something like StrongMemoryCache which is part of RealMemoryCache equivalent for animated images.

3.For downsampling, I was thinking something similar to Glide's DownsampleStrategy logic. The downsampling is done after source size is read in the decoder. It uses both source size and target size (view size) to figure out final sizing. In ViewSizeResolver, I only have access to target size so I wondering if this something which should belong to the Coil's Decoder.

[colinrtwhite]

1. I'm not sure I understand, onStart won't be called with the result drawable of the image request - it's called with the placeholder or null. onSuccess gets the result drawable of the image request. There are a number of ways to do this, but you can cache the request manually using a view tag before enqueuing it to get a reference to the request:

val request = ImageRequest.Builder(context)
    .data(url)
    .target(CustomImageViewTarget(imageView))
    .build()
imageView.setTag(R.id.coil_request, request)
context.imageLoader.enqueue(request)

// Call imageView.getTag(R.id.coil_request, request) inside onStart to get the latest request.

2. There are a number of reasons why it's tough/not possible to cache animated drawables in the memory cache and why it isn't likely to be added:

• Bitmaps can be shared between image views if they're immutable. Drawables (especially animated drawables) cannot be shared; this means they need to be cleared from the view to be returned to the cache, which has other issues and can be unexpected.
• Animated drawables hold mutable state internally. We would need a way to set an animated drawable back to it's initial state when it was decoded or else it could be returned from the cache and start playing part way through the animation as an example. For Android's AnimatedImageDrawable I don't think it's possible to reset the drawable.
• By far the easiest way to implement this is to have consumers manually add drawables back to a custom cache and reset them when they're done with them.
• Overall, it's a lot of work and extra complexity. I have to be careful about how I spend my time and I've spent it on other parts of Coil and the Instacart app. If you're open to implementing this, please let me know.

3. As mentioned, Movie doesn't support downsampling while decoding (though it will scale the image at draw-time). I don't think it's possible to add the logic you linked, though I'm open to a PR if you'd like to implement it.

[seankim-android]

@colinrtwhite I was trying to skip onStart() based on the result drawable from onSuccess(). I read it again and it does sound confusing :) Anyways, your suggestion makes sense for both onStart and onSuccess.

2 & 3 make a lot of sense. Thank you for all the clarifications. It does look like a lot of work and extra complexity. I'll see if I can solve the issue with a custom ViewTarget first before trying to implement the other parts.

I'm hoping the tag or some other approach to skip the new request based on the previous request works out. My actual code from RecyclerView.Adapters are more tricky than what I posted in the description so I'll try out few more things based on your response.

Thanks again for your response! I'll either close this issue or come back with new questions in the next couple of days.

[seankim-android]

Closing with updates. Thanks again for your suggestions and clarifications.

1. Used a custom ViewTarget and some tagging logic to skip duplicate requests. This resolved the flicker issue on re-bind which is good enough for now.
2. Global memory caching of animated images is indeed a lot of work with unknowns. Will look into the suggested option.

By far the easiest way to implement this is to have consumers manually add drawables back to a custom cache and reset them when they're done with them.

3. You're right. I couldn't find a way for Movie to support downsampling before draw time.