ImageView briefly losing the image only on 4.14.2 (produces a flicker)

[sky-ricardocarrapico]

Glide Version: 4.14.2

Integration libraries: OkHttp 4.9.0

Device/Android Version: Reproducible in any device

Issue details / Repro steps / Use case background:

Since we've upgraded to 4.14.2, we've been having an issue where, briefly, the ImageView loses the image. This will make its size be 0x0 (we're using wrap_content for this specific view) and it'll produce a flicker.

I recorded a video highlighting the problem:
https://user-images.githubusercontent.com/67684386/202150949-fd0cc824-b35f-4ae9-9a91-ac359e171958.mp4

The video shows that we had no image (haven't been loaded yet), then it loads and show, then it disappears, then it shows again.

We're loading the image in a RecyclerView (no compose at all here) and there's multiple rebinds happening at that time, so it could be related.

From debugging we could track this down into loading the image with properties on the RequestBuilder that are actually a new instance. Such as:

.transition(DrawableTransitionOptions.withCrossFade(crossFadeFactory))
or
.listener(listener)

Not calling those methods on RequestBuilder fixes the problem.

A very important detail is that this does not happen on 4.14.1, which makes me very suspicious of this commit f3d6ff7. In the meantime we had to rollback to this version.

Glide load line / GlideModule (if any) / list Adapter code (if any):

requestBuilder
    .listener(listener)
    .transition(DrawableTransitionOptions.withCrossFade(crossFadeFactory))
    .skipMemoryCache(config.skipMemoryCache)
    .diskCacheStrategy(config.cacheStrategy)
    .into(this)

This code will be called multiple times in the rebind of a ViewHolder.

Layout XML:

<ImageView
        android:id="@+id/pdp_title_logo_img"
        android:layout_width="wrap_content"
        android:layout_height="wrap_content"
        android:layout_marginTop="@dimen/pdp_title_margin_vertical"
        android:adjustViewBounds="true"
        android:maxWidth="@dimen/pdp_title_frame_width"
        android:maxHeight="@dimen/pdp_title_frame_height"
        android:scaleType="fitStart"
        android:visibility="gone"
        app:layout_constraintDimensionRatio="1"
        app:layout_constraintEnd_toEndOf="parent"
        app:layout_constraintHorizontal_bias="0"
        app:layout_constraintStart_toStartOf="@id/guide_start"
        app:layout_constraintTop_toBottomOf="@id/pdp_channel_logo_img"
        tools:srcCompat="@drawable/ic_app_logo_small"
        tools:visibility="visible" />

[sjudd]

If you call .listener(), but not .transition(), does it work? Or does it only work if you call neither .listener() nor .transition()?

It would be interesting to see the Engine logs when you make these requests: https://bumptech.github.io/glide/doc/debugging.html#unexpected-cache-misses

My guess is that in 4.14.1, the request is never made due to this logic:

glide/library/src/main/java/com/bumptech/glide/RequestBuilder.java

Lines 800 to 812 in 18bba92

	if (request.isEquivalentTo(previous)
	&& !isSkipMemoryCacheWithCompletePreviousRequest(options, previous)) {
	// If the request is completed, beginning again will ensure the result is re-delivered,
	// triggering RequestListeners and Targets. If the request is failed, beginning again will
	// restart the request, giving it another chance to complete. If the request is already
	// running, we can let it continue running without interruption.
	if (!Preconditions.checkNotNull(previous).isRunning()) {
	// 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.
	previous.begin();
	}
	return target;

. But I don't see anything in that logic that cares about the changes to equals/hashcode. The relevant implementation seems to be:

glide/library/src/main/java/com/bumptech/glide/request/SingleRequest.java

Lines 769 to 778 in 18bba92

	return localOverrideWidth == otherLocalOverrideWidth
	&& localOverrideHeight == otherLocalOverrideHeight
	&& Util.bothModelsNullEquivalentOrEquals(localModel, otherLocalModel)
	&& localTranscodeClass.equals(otherLocalTranscodeClass)
	&& localRequestOptions.equals(otherLocalRequestOptions)
	&& localPriority == otherLocalPriority
	// We do not want to require that RequestListeners implement equals/hashcode, so we
	// don't compare them using equals(). We can however, at least assert that the same
	// amount of request listeners are present in both requests.
	&& localListenerCount == otherLocalListenerCount;

.

The next possibility if the isEquivalentTo logic isn't relevant in either 4.14.1 or 4.14.2 is that there's been a change to the memory cache key. If that's the case, you will see a difference in the Engine logs between 4.14.1 and 4.14.2, somewhere around here:

glide/library/src/main/java/com/bumptech/glide/load/engine/Engine.java

Line 190 in 18bba92

memoryResource = loadFromMemory(key, isMemoryCacheable, startTime);

But again I don't see either the listener or the transition in the key implementation:

glide/library/src/main/java/com/bumptech/glide/load/engine/EngineKey.java

Lines 44 to 58 in 18bba92

	@Override
	public boolean equals(Object o) {
	if (o instanceof EngineKey) {
	EngineKey other = (EngineKey) o;
	return model.equals(other.model)
	&& signature.equals(other.signature)
	&& height == other.height
	&& width == other.width
	&& transformations.equals(other.transformations)
	&& resourceClass.equals(other.resourceClass)
	&& transcodeClass.equals(other.transcodeClass)
	&& options.equals(other.options);
	}
	return false;
	}

[sjudd]

Oh there is one bug for compose at least which is that we do not implement equals/hashcode for TransitionFactorys. Again though I don't immediately see how that would impact this use case.

[sky-ricardocarrapico]

Thanks for answering!

If you call .listener(), but not .transition(), does it work? Or does it only work if you call neither .listener() nor .transition()?

It only works if I call neither of those. So I was assuming that it happens for new object instances, where equals/hashCode may not match. Because using other methods with a boolean or an enum, does not seem to affect it.

I was not able to turn on those logs using adb shell. Nothing would appear on my logcat. I did however use builder.setLogLevel(Log.VERBOSE) and was able to gather some information.

I did these tests with different pages and different images. I omitted the url, but it's always the same in each test and we're not using custom cache keys.

On 4.14.2:

2022-12-15 12:27:34.853 10939-10939/com.xxx D/Glide: Finished loading BitmapDrawable from REMOTE for https://xxx with size [2200x2200] in 1533.1391529999999 ms
2022-12-15 12:27:34.998 10939-10939/com.xxx D/Glide: Finished loading BitmapDrawable from DATA_DISK_CACHE for https://xxx with size [735x215] in 19.825231 ms
2022-12-15 12:27:35.617 10939-10939/com.xxx D/Glide: Finished loading BitmapDrawable from DATA_DISK_CACHE for https://xxx with size [732x215] in 41.172308 ms

2022-12-15 12:30:04.184 10939-10939/com.xxx D/Glide: Finished loading BitmapDrawable from RESOURCE_DISK_CACHE for https://xxx with size [2200x2200] in 152.524577 ms
2022-12-15 12:30:05.521 10939-10939/com.xxx D/Glide: Finished loading BitmapDrawable from DATA_DISK_CACHE for https://xxx with size [735x123] in 10.774576999999999 ms
2022-12-15 12:30:06.679 10939-10939/com.xxx D/Glide: Finished loading BitmapDrawable from DATA_DISK_CACHE for https://xxx with size [733x123] in 34.568808 ms

2022-12-15 12:32:31.540 10939-10939/com.xxx D/Glide: Finished loading BitmapDrawable from RESOURCE_DISK_CACHE for https://xxx with size [2200x2200] in 86.814692 ms
2022-12-15 12:32:32.765 10939-10939/com.xxx D/Glide: Finished loading BitmapDrawable from RESOURCE_DISK_CACHE for https://xxx with size [735x102] in 9.274269 ms
2022-12-15 12:32:33.557 10939-10939/com.xxx D/Glide: Finished loading BitmapDrawable from RESOURCE_DISK_CACHE for https://xxx with size [731x102] in 43.186499 ms

On 4.14.1:

2022-12-15 12:38:54.270 13240-13240/com.xxx D/Glide: Finished loading BitmapDrawable from RESOURCE_DISK_CACHE for https://xxx with size [2200x2200] in 71.82553899999999 ms
2022-12-15 12:38:56.515 13240-13240/com.xxx D/Glide: Finished loading BitmapDrawable from MEMORY_CACHE for https://xxx with size [2200x2200] in 0.150692 ms
2022-12-15 12:38:58.247 13240-13240/com.xxx D/Glide: Finished loading BitmapDrawable from MEMORY_CACHE for https://xxx with size [2200x2200] in 0.076154 ms

2022-12-15 12:40:46.417 13240-13240/com.xxx D/Glide: Finished loading BitmapDrawable from RESOURCE_DISK_CACHE for https://xxx with size [2200x2200] in 132.727769 ms
2022-12-15 12:40:47.994 13240-13240/com.xxx D/Glide: Finished loading BitmapDrawable from MEMORY_CACHE for https://xxx with size [2200x2200] in 0.098885 ms
2022-12-15 12:40:48.710 13240-13240/com.xxx D/Glide: Finished loading BitmapDrawable from MEMORY_CACHE for https://xxx with size [2200x2200] in 0.07672999999999999 ms

2022-12-15 12:41:55.479 13240-13240/com.xxx D/Glide: Finished loading BitmapDrawable from REMOTE for https://xxx with size [2200x2200] in 913.768884 ms
2022-12-15 12:41:56.233 13240-13240/com.xxx D/Glide: Finished loading BitmapDrawable from MEMORY_CACHE for https://xxx with size [2200x2200] in 0.0805 ms
2022-12-15 12:41:56.872 13240-13240/com.xxx D/Glide: Finished loading BitmapDrawable from MEMORY_CACHE for https://xxx with size [2200x2200] in 0.072654 ms

I can also see these logs in both versions:

2022-12-15 12:32:31.462 10939-10939/com.xxx I/ViewTarget: Glide treats LayoutParams.WRAP_CONTENT as a request for an image the size of this device's screen dimensions. If you want to load the original image and are ok with the corresponding memory cost and OOMs (depending on the input size), use override(Target.SIZE_ORIGINAL). Otherwise, use LayoutParams.MATCH_PARENT, set layout_width and layout_height to fixed dimension, or use .override() with fixed dimensions.
2022-12-15 12:32:31.462 10939-10939/com.xxx I/ViewTarget: Glide treats LayoutParams.WRAP_CONTENT as a request for an image the size of this device's screen dimensions. If you want to load the original image and are ok with the corresponding memory cost and OOMs (depending on the input size), use override(Target.SIZE_ORIGINAL). Otherwise, use LayoutParams.MATCH_PARENT, set layout_width and layout_height to fixed dimension, or use .override() with fixed dimensions.

We must be doing something wrong regarding wrap_content, because for this case, it doesn't make sense to load images that big.

But looking at the logs something has happened between 4.14.1 and 4.14.2. On the latter it tries to load the image with different sizes, where on the first it would be consistent.

Now, how does this relate with having a .listener()/.transition()? When I remove these calls on 4.14.2, the logs start being similar to 4.14.1, it'll always load the image with the same size (eg. 2200x2200).

Does this make sense, somehow? Let me know what else can I do.
And thank you for your time on this.

[sky-ricardocarrapico]

I can make some XML changes to avoid wrap_content for width. That'll make Glide load the image as 735x2200 on the first load and generate one less wrap_content warning. Does not fix the issue though.

I've been playing around with some changes to try to avoid using wrap_content at all, as I already read that it's not very recommended to use on Glide, but I'm not able to because it's really the requirement that we have.

We do have a maxWidth/maxHeight, but I guess Glide does not take that into account? It's wrap_content but we know that it'll not surpass that defined max, so it's a bit wasteful loading the full device size image.

[sjudd]

The differences in Glide between 4.14.1 and 4.14.2 are here: 4.14.1...4.14.2. I don't see anything that seems like it would impact the size determination.

Glide does not look at maxWidth or maxHeight. It might be interesting to try to debug the SizeDeterminer to see how it's managing to acquire a different sizes on 4.14.2:

glide/library/src/main/java/com/bumptech/glide/request/target/ViewTarget.java

Line 319 in c9f76cc

static final class SizeDeterminer {

One thing you can do though is just use [.override()](https://bumptech.github.io/glide/javadocs/430/com/bumptech/glide/request/RequestOptions.html#override-int-int-) with the values you use for maxWidth and maxHeight. You can continue to use wrap_content for the view itself. override() tells Glide exactly what size to use, so it doesn't have to try to guess.

[mori-atsushi]

I guess the issue is caused by this change.

glide/library/src/main/java/com/bumptech/glide/RequestBuilder.java

Lines 1257 to 1273 in ca47d9e

	@Override
	public boolean equals(Object o) {
	if (o instanceof RequestBuilder<?>) {
	RequestBuilder<?> that = (RequestBuilder<?>) o;
	return super.equals(that)
	&& Objects.equals(transcodeClass, that.transcodeClass)
	&& transitionOptions.equals(that.transitionOptions)
	&& Objects.equals(model, that.model)
	&& Objects.equals(requestListeners, that.requestListeners)
	&& Objects.equals(thumbnailBuilder, that.thumbnailBuilder)
	&& Objects.equals(errorBuilder, that.errorBuilder)
	&& Objects.equals(thumbSizeMultiplier, that.thumbSizeMultiplier)
	&& isDefaultTransitionOptionsSet == that.isDefaultTransitionOptionsSet
	&& isModelSet == that.isModelSet;
	}
	return false;
	}

This equals implementation was added in the RequestBuilder class on 4.14.2.
The isEquivalentTo method in the SingleRequest class references the RequestBuilder’s equals method, because RequestBuilder inherits from BaseRequestOptions.

Here is the call stack:

into:841, RequestBuilder (com.bumptech.glide)
isEquivalentTo:773, SingleRequest (com.bumptech.glide.request)
equals:1302, RequestBuilder (com.bumptech.glide)

Therefore, the request.isEquivalentTo(previous) always returns false and it is unable to skip the rendering in the following cases:

// Using the `com.bumptech.glide.request.RequestListener`
Glide.with(fragment)
    .load(url)
    .listener(object : RequestListener {
        /* without the equals implimentation */ 
    })
    .into(imageView)

// Using transactions
Glide.with(fragment)
    .load(url)
    .transition(DrawableTransitionOptions.withCrossFade(crossFadeFactory))
    .into(imageView)

// Using the `com.bumptech.glide.load.model.Model` interface
class SampleMode(val url: Strong) : Model {
    override fun isEquivalentTo(other: Any?): Boolean =
        other is SampleMode && url == other.url

    /* without the equals implimentation */ 
}
Glide.with(fragment)
    .load(SampleMode(url))
    .into(imageView)

I think we need to implement isEquivalentTo instead of equals in the BaseRequestOptions class and the RequestBuilder class.
If there is no problem with this policy, I can create a PR.