crossFade messes up aspect ratio for thumbnail/placeholder

[TWiStErRob]

Glide Version/Integration library (if any): 3.5.2 Device/Android Version: S4/4.4 Issue details/Repro steps: Load a different sized thumbnail and image, or placeholder and image and note the result:

Glide load line:

class Delay extends UnitTransformation {
    private final int sleepTime;
    public Delay(int sleepTime) { this.sleepTime = sleepTime; }
    @Override public Resource transform(Resource resource, int outWidth, int outHeight) {
        try { Thread.sleep(sleepTime); } catch (InterruptedException ex) {}
        return super.transform(resource, outWidth, outHeight);
    }
}

@Override protected void onCreate(Bundle savedInstanceState) {
    super.onCreate(savedInstanceState);
    final ImageView image = new ImageView(this);
    setContentView(image);

    // Saved as R.drawable.placeholder
    String placeholder = "http://placehold.it/300x500/eedddd.png&text=PLACEHOLDER";
    // Square image
    String thumb = "http://placehold.it/300x300/eeeedd.png&text=THUMB";
    // Wide image
    final String full = "http://placehold.it/300x100/eedddd.png&text=FULL";
    // Prepared request with unimportant clutter
    final DrawableRequestBuilder<String> req = Glide
            .with(this)
            .fromString()
            .diskCacheStrategy(DiskCacheStrategy.SOURCE) // disable network delay for demo
            .skipMemoryCache(true) // make sure transform runs for demo
            .crossFade(2000) // default, just stretch time for noticability
    ;

    req.clone()
        .thumbnail(req.clone()
                    .transform(new Delay(500)) // wait a little
                    .load(thumb)
        )
        .transform(new Delay(1000)) // wait before going from thumbnail to image
        //.dontAnimate() // solves the problem
        .load(full)
        .into(image);

    image.setOnClickListener(new OnClickListener() {
        @Override public void onClick(View v) {
            req.clone()
                .load(full)
                .placeholder(R.drawable.placeholder)
                .transform(new Delay(1000))
                //.animate(R.anim.abc_fade_in) // also solves the problem
                .into(image);
        }
    });

By removing all the delaying parts, the end result is better, but the now the thumbnail is messed up:

[sjudd]

Can you try overriding the Target and avoiding the SquaringDrawable?

[TWiStErRob]

Based on ImageViewFactory and the "dirty hack" in GlideDrawableImageViewTarget.onResourceReady I changed into(image) to .into((Target<GlideDrawable>)(Target)new DrawableImageViewTarget(image)), and confirmed in debug that the target receives a GlideBitmapDrawable, but after that I wanted to check if it was SquaringDrawable before: however the drawableRatio for thumb is 1, the view size is the whole activity, so that ratio is ~16:9 so the viewRatio doesn't fall into SQUARE_RATIO_MARGIN; so it seems there's no SquaringDrawable involved.

Is this what you meant?

[sjudd]

Yup that makes sense.

Unfortunately if SquaringDrawable isn't involved, then this is an issue with TransitionDrawable which is owned by the framework. I'd love to write our own cross fading Drawable because TransitionDrawable seems to have a number of issues, and this just reinforces that thought. However I can't say it's super likely to happen any time soon if left to just me...

[Tolriq]

Just faced this with a simple request with a place holder and crossfade.

Changing crossfade to animation solved this, so really sounds like a TransitionDrawable problem.

But maybe there should be some kind of notice somewhere because it took me some time to find this issue :(

[Tolriq]

@sjudd : Update :( It seems there's a crossfade by default applied so in fact this "problem" is quite important.

[sjudd]

Yeah it is applied by default... Again a pull request would be welcome here if anyone wants to try to take on writing a better TransitionDrawable specifically for cross fades.

[nathanielwolf]

I'm surprised there is no resolution for this issue yet. We've had to disable all crossfades because of this since we use a square placeholder and load images with various aspect ratios. We are using a fade in of the image, but this is not as good as a crossfade since the placeholder just disappears . Anyone else have a better work around?

[TWiStErRob]

Long shot: since you have square placeholder I assume the space for the images isn't much wider, how about creating Drawable with the space's aspect (pad the square) and/or add a transformation for the main image that adds tranparent padding to the loaded Bitmap. This would result in the layers of TransitionDrawable having the same intrinsic size aspect, and hence working as intended.

Alternatively, but in a similar fashion: placeholder with same aspect as available space and centerCrop so the loaded Bitmap has exactly the space's size (and aspect).

Sadly both of these are hacky workarounds, if you feel like it, you could fix Android's TransitionDrawable to handle different aspects.

[kevinvanmierlo]

@sjudd @TWiStErRob I know the code for a new TransitionDrawable which keeps the aspect ratio. I just don't know where to implement it. Can someone else help me where to implement this or file a pull request themselves using this code:

import android.content.Context;
import android.graphics.Bitmap;
import android.graphics.Canvas;
import android.graphics.ColorFilter;
import android.graphics.Rect;
import android.graphics.drawable.AnimationDrawable;
import android.graphics.drawable.BitmapDrawable;
import android.graphics.drawable.Drawable;
import android.os.Build;
import android.os.SystemClock;
import android.widget.ImageView;

public final class GlideTransitionDrawable extends BitmapDrawable
{
    // Only accessed from main thread.
    private static final float FADE_DURATION = 800f; //ms

    /**
     * Create or update the drawable on the target {@link ImageView} to display the supplied bitmap
     * image.
     */
    public static void setBitmap(ImageView target, Context context, Bitmap bitmap)
    {
        Drawable placeholder = target.getDrawable();
        if (placeholder instanceof AnimationDrawable)
        {
            ((AnimationDrawable) placeholder).stop();
        }
        GlideTransitionDrawable drawable = new GlideTransitionDrawable(context, bitmap, placeholder);
        target.setImageDrawable(drawable);
    }

    /**
     * Create or update the drawable on the target {@link ImageView} to display the supplied
     * placeholder image.
     */
    public static void setPlaceholder(ImageView target, Drawable placeholderDrawable)
    {
        target.setImageDrawable(placeholderDrawable);
        if (target.getDrawable() instanceof AnimationDrawable)
        {
            ((AnimationDrawable) target.getDrawable()).start();
        }
    }

    Drawable placeholder;

    long startTimeMillis;
    boolean animating;
    int alpha = 0xFF;

    GlideTransitionDrawable(Context context, Bitmap bitmap, Drawable placeholder)
    {
        super(context.getResources(), bitmap);

        this.placeholder = placeholder;
        animating = true;
        startTimeMillis = SystemClock.uptimeMillis();
    }

    @Override
    public void draw(Canvas canvas)
    {
        if (!animating)
        {
            super.draw(canvas);
        } else
        {
            float normalized = (SystemClock.uptimeMillis() - startTimeMillis) / FADE_DURATION;
            if (normalized >= 1f)
            {
                animating = false;
                placeholder = null;
                super.draw(canvas);
            } else
            {
                if (placeholder != null)
                {
                    placeholder.draw(canvas);
                }

                int partialAlpha = (int) (alpha * normalized);
                super.setAlpha(partialAlpha);
                super.draw(canvas);
                super.setAlpha(alpha);
                if (Build.VERSION.SDK_INT <= Build.VERSION_CODES.GINGERBREAD_MR1)
                {
                    invalidateSelf();
                }
            }
        }
    }

    @Override
    public void setAlpha(int alpha)
    {
        this.alpha = alpha;
        if (placeholder != null)
        {
            placeholder.setAlpha(alpha);
        }
        super.setAlpha(alpha);
    }

    @Override
    public void setColorFilter(ColorFilter cf)
    {
        if (placeholder != null)
        {
            placeholder.setColorFilter(cf);
        }
        super.setColorFilter(cf);
    }

    @Override
    protected void onBoundsChange(Rect bounds)
    {
        if (placeholder != null)
        {
            placeholder.setBounds(bounds);
        }
        super.onBoundsChange(bounds);
    }
}

I use this with a custom call at the moment, not ideal but it works. If someone could help me implement this in the Glide code I would be thankful.

[softvision-mariusduna]

the ideal fix would be not a workaround

[kevinvanmierlo]

@softvision-mariusduna My code is not a workaround, it should be implemented instead of a TransitionDrawable (which screws up the aspect ratio). I use it in my app and it works great!

[sjudd]

@kevinvanmierlo TransitionDrawable is currently used in Glide here: https://github.com/bumptech/glide/blob/master/library/src/main/java/com/bumptech/glide/request/transition/DrawableCrossFadeTransition.java#L44.

A pull request would be welcome!

[kevinvanmierlo]

@sjudd Ahh thanks! I'll make a pull request when I have the time! Which is probably the next couple days or the beginning of next week.

[kevinvanmierlo]

@sjudd It's going to take a little while longer. I don't have much time and I can't use BitmapDrawable, so I have to find another solution using Drawable.

[kevinvanmierlo]

@sjudd I found some time again, sorry that it takes so long. I wanted to test some options, but the library doesn't have .placeholder or GlideDrawable. Did I do something wrong?

[TWiStErRob]

master is 4.0, which has breaking changes and simplifications

.apply(RequestOptions.placeholderOf(drawable)) //=.placeholder(drawable)

You don't need GlideDrawable, crossFade has to work between any two Drawables. Sam's link is still valid though.

[kevinvanmierlo]

@TWiStErRob thanks for the quick response! Didn't know that. Do you perhaps know if the new Drawable is always a BitmapDrawable? Cause if it is a BitmapDrawable you can do more calculations on the image. If it is not I'll continue to look for a solution using Drawable

[TWiStErRob]

You shouldn't assume BitmapDrawable, I think it's possible to optimize for it, but a generic Drawable solution is needed anyway, so optimisation may not worth it. Assuming would break the genericness of the transcoder.

[kevinvanmierlo]

@TWiStErRob Okay thanks, I'll try to find a solution using Drawable then.

[kevinvanmierlo]

@TWiStErRob @sjudd The cross-fading now works with aspect ratio (not fully tested) but the image is always center cropped. You can find the project here: https://github.com/kevinvanmierlo/glide. I didn't clean up the code yet, so keep that in mind. I'm not sure how to keep the ScaleType of the ImageView, but I'll try to find out.

[TWiStErRob]

In the above linked commit I found another way to work around this. It's not a full solution to this bug, but it works for placeholders that are smaller than the full image and was worth mentioning here. For more info see http://stackoverflow.com/a/36944010/253468.

[Edijae]

@TWiStErRob This is the answer that i used to overcome this bug. For placeholders that are smaller than the full image. http://stackoverflow.com/a/41459535/3671509

[sheaam30]

I'm still seeing this issue on 3.7.0 and the only workaround for me are either .dontAnimate or .animate() but I would rather have a crossfade. Any suggestions?

[TWiStErRob]

@sheaam30 the issue is not closed; the simplest workaround is to have your placeholder and image aspect ratio match

[louis993546]

I guess this is no longer relevant in Glide v4 since there is no longer any transformation (and it seems fine to me as well)

[haniha]

@louistsaitszho no difference here , DrawableTransitionOptions.withCrossFade() cause same problem in glide4

[sjudd]

I'm still open to pull requests to improve the cross fade behavior, but I think the limitations are reasonably well understood and documented, so I'm going to remove the bug label.

[LOG-TAG]

Glide.with(mContext).load(item.getImage()).apply(new RequestOptions().placeholder(R.drawable.image_loading)).into((ImageView) helper.getView(R.id.media_image));

above code works fine, trouble starts when we use transition(new DrawableTransitionOptions().crossFade()

[LOG-TAG]

@TWiStErRob how to handle your imageview is inisde ConstraintLayout with

 <android.support.constraint.ConstraintLayout xmlns:app="http://schemas.android.com/apk/res-auto"
        android:layout_width="match_parent"
        android:layout_height="wrap_content">
        <android.support.v7.widget.AppCompatImageView
            android:id="@+id/media_image"
            android:layout_width="0dp"
            android:layout_height="0dp"
            android:foreground="@drawable/gradient"
            app:layout_constraintDimensionRatio="16:9"
            app:layout_constraintEnd_toEndOf="parent"
            app:layout_constraintHorizontal_bias="0.0"
            app:layout_constraintStart_toStartOf="parent"
            app:layout_constraintTop_toTopOf="parent"
            app:layout_constraintVertical_bias="0.0"
            app:srcCompat="@android:color/darker_gray"

            android:visibility="visible"

            app:layout_constraintBottom_toTopOf="@+id/share_button"
            app:layout_constraintDimensionRatio="16:9"
            app:layout_constraintEnd_toEndOf="parent"
            app:layout_constraintHorizontal_bias="0.0"
            app:layout_constraintStart_toStartOf="parent"
            app:layout_constraintTop_toTopOf="parent"
            app:layout_constraintVertical_bias="0.0"
            app:srcCompat="@android:color/darker_gray" />

how do I generate the placeholder and image aspect ratio match?

[TWiStErRob]

@LOG-TAG https://stackoverflow.com/a/36944010/253468 ?

[jemshit]

Using

.thumbnail(GlideApp.with(this).load(R.drawable.placeholder))
.transition(withCrossFade())

instead of .placeholder() works fine

[lawonga]

Having the same issue, however I find that even with the fix here: https://stackoverflow.com/a/36944010/253468 it still messes up. I do notice that targetHeight < drawable.getIntrinsicHeight in getCurrentDrawable. Any ideas?

[kevinvanmierlo]

Okay guys, sorry for the delay, but had some time again. I'm going to create a pull request, but before it gets accepted here is a gist with the code you can use in the meantime: https://gist.github.com/kevinvanmierlo/c46f66027e3ae37ebea85a8d2e12aaba.

[Sternbach-Software]

Where is this holding?

[kevinvanmierlo]

I don't know, haven't gotten any response anymore. I created the pull request. But Travis keeps failing and I don't really know why. I fixed all formatting issues I could find, but it still fails. I asked for help, but no response.

[J6ey]

I ended up using a vector over PNG as the placeholder and haven't seen any stretching