square / picasso

A powerful image downloading and caching library for Android
https://square.github.io/picasso/
Apache License 2.0
18.72k stars 3.97k forks source link

Placeholder is stretched before updating the ImageView (2.2.0) #427

Open kaciula opened 10 years ago

kaciula commented 10 years ago

In version 2.2.0 the placeholder image is stretched right before being replaced with the actual image. It looks ugly. In previous version 2.1.1, this worked as expected. Here is the relevant code:

mPicasso.load(url).placeholder(R.drawable.bird_placeholder).error(R.drawable .bird_placeholder).into(holder.photo);

holder.photo is a SquaredImageView, meaning it has the same width and height. And the scaleType is "centerCrop".

nazr commented 10 years ago

Having the same problem. scaleType doesn't seem to have any effect. The only way to work around the problem I found is to have placeholder images exactly the same size as the ImageView. That means several versions (or sets of images in case of animations) for different sizes - very ugly. And of course you don't always know the size.

zorius commented 10 years ago

You should call resize() or resizeDimen() after load(url). Picasso needs specific bounds before "centerCrop" or "centerResize" to prevent distortion of the aspect ratio.

dlew commented 9 years ago

I've noticed that this still seems to be happening in the latest Picasso.

If the ImageView is centerInside, but the request uses fit().centerCrop(), then at the last second (before it fully loads) the placeholder expands as if the scaletype were actually centerCrop.

JakeWharton commented 9 years ago

Yes. PicassoDrawable doesn't honor the scaleType for the placeholder.

dlew commented 9 years ago

I guess I'm a bit confused, then, why it changes size. If it doesn't honor scaleType the entire time, why does it initially start at the correct size, then resize for a split second before the image loads fully?

JakeWharton commented 9 years ago

Ah, sorry for not elaborating. We set the placeholder directly on the ImageView initially. When the URL (or whatever) has finished loading, we copy the existing drawable into a PicassoDrawable instance and then do a ~200ms cross-fade between the two. Since PicassoDrawable doesn't honor scaleType, if the image is being resized by the ImageView you'll see it jump to a different size during the cross fade.

The fix would be to copy ImageView's math for scaleType and apply it to the placeholder drawable that we steal from it.

dlew commented 9 years ago

Ah, that falls in line with something else I just discovered, which was that using noFade() fixes the problem (at the cost of removing the fade).

I might look into doing a PR to do what you propose; seems like it should be possible in any case where it's being loaded into an ImageView.

dlew commented 9 years ago

@JakeWharton I've been investigating the issue and duplicating ImageView's logic is going to be tough, since it would depend heavily on the ImageView itself being accessible in PicassoDrawable (which I'm guessing is not what we want).

I'm looking at it from another angle now: why is PicassoDrawable an instance of BitmapDrawable, instead of something more suited to handle multiple Drawables at once, like LayerDrawable? In particular, I'm wondering if using TransitionDrawable would be more appropriate. Using either would mean that both layers (the placeholder and the bitmap) would render correctly.

The downside would be wrecking everyone's code that depends on PicassoDrawable being an instance of BitmapDrawable... not sure if that's an acceptable change.

JakeWharton commented 9 years ago

I would have to dig up old threads to see all the reasons. In the interest of replying sooner rather than later, one of them was that it reduced our allocations by 1 or 2 per successful image download. Since many images usually comprise a screen we try to be conscious about allocation. That said, fixing something like this would be worth undoing that and chasing allocations elsewhere.

dlew commented 9 years ago

For the sake of moving things along, I tried implementing PicassoDrawable with TransitionDrawable. You can see the results in PR #855.

There are a fair number more allocations by using TransitionDrawable... so I can see why it might've been avoided. It does solve the aspect ratio issue, though (and simplifies much of PicassoDrawable, since a lot of its previous logic is implemented by TransitionDrawable).

dlew commented 9 years ago

I've been digging into a solution to this (that doesn't involve TransitionDrawable). I've gotten to the root of the problem.

The Canvas matrix in the ImageView is calculated based on three factors:

  1. Scale type
  2. ImageView width/height
  3. Drawable intrinsic width/height.

The core problem is that the intrinsic width/height often changes when switching from the placeholder to the actual image. This causes the matrix to change, which in turn distorts the placeholder.

This also means that the LayerDrawable-based solution didn't actually work - its intrinsic width/height was an amalgamation of the placeholder and the image, causing the image to skew differently (based on the placeholder below it).

The only solution, as I see it, would be to pre-configure the placeholder with the future image's intrinsic width/height. This means the placeholder wouldn't shift when it's rendered along with the image (later on). It would only work properly if you are using something like resize() or fit(), since otherwise there's no way to know the future image's size.

dnkoutso commented 9 years ago

That sounds correct. Did you attempt a fix/update in PicassoDrawable?

dlew commented 9 years ago

I threw something ugly together quickly that proved it solved the problem, but nothing I'd send as a PR. :)

I'll have time next week to work on a good solution. The issue I ran into was that currently the placeholder is set immediately, but in order for it to work with fit() it has to be deferrable. But I should be able to reason it out once I have more time.

dnkoutso commented 9 years ago

Thank you man. We're on the same boat here with respect to "having more time".

dlew commented 9 years ago

I took another look at it and now I'm conflicted. If we fix the aspect ratio of the placeholder when cross-fading (by having the placeholder match the aspect ratio of the loaded image), that means the placeholder's ratio will be wrong.

For example, if the placeholder is a rectangle, but the requested size is a square, then the placeholder will be squished into a square aspect ratio. Now the placeholder won't suddenly warp during the crossfade, but it'll look wrong the entire time the image is loading, which is just as bad.

If you want to see it in action, I've got a branch with it working here: https://github.com/dlew/picasso/tree/dlew/placeholder-fix

I suspect the only way to properly solve this dilemma would be to have a custom ImageView that renders two Drawables at once, each with their own matrices, so they could each preserve their aspect ratio while honoring the ImageView's scaleType. But custom ImageViews seems outside of the scope of Picasso.

I would love some guidance on what you think is best here.

recodyx commented 9 years ago

I hope this issue will be fixed some day.

luccascorrea commented 9 years ago

So do I.

midnightviolin commented 9 years ago

noFade() can fix overlap problem when use placeholder

AndreiHarasemiuc commented 9 years ago

soooooome day

Dayanand515 commented 9 years ago

I am using framelayout to show placeholder image in center, with second layer for picasso image to load. Works like charm.

Danihelsan commented 9 years ago

Hello guys,

my current solution to this is to use a scaleType= "center" or the one required for the placeHolder to be displayed correctly. Implement the Target in the activity or fragment and then do something like this (previously instantiating the ImageView in the activity or fragment):

@Override public void onBitmapLoaded(Bitmap bitmap, Picasso.LoadedFrom from) { image.setScaleType(ImageView.ScaleType.FIT_XY); image.setImageBitmap(bitmap); }

@Override
public void onBitmapFailed(Drawable errorDrawable) {
    image.setImageDrawable(errorDrawable);
}

@Override
public void onPrepareLoad(Drawable placeHolderDrawable) {
    image.setImageDrawable(placeHolderDrawable);
}
BoD commented 8 years ago

Am I right to say that because of this issue, the "fading" feature of Picasso is not really currently usable? Or is there something I'm missing?

JakeWharton commented 8 years ago

Use placeholders that are sized with the same aspect ratio as the target image and it works perfectly fine.

On Sat, Jan 23, 2016, 1:20 PM Benoit Lubek notifications@github.com wrote:

Am I right to say that because of this issue, the "fading" feature of Picasso is not really currently usable? Or is there something I'm missing?

— Reply to this email directly or view it on GitHub https://github.com/square/picasso/issues/427#issuecomment-174210006.

BoD commented 8 years ago

Ok thank you for your answer, @JakeWharton. In my current case it's not really possible since my ImageView's width is the screen's width (depends on the device) whereas its height is fixed.

JakeWharton commented 8 years ago

What's the placeholder? That sounds like a great case for a 9-patch. Even if the stretchable regions are just solid colors on the sides to ensure the drawable fills the space completely.

On Sat, Jan 23, 2016, 1:25 PM Benoit Lubek notifications@github.com wrote:

Ok thank you for your answer, @JakeWharton https://github.com/JakeWharton. In my current case it's not really possible since my ImageView's width is the screen's width (depends on the device) whereas its height is fixed.

— Reply to this email directly or view it on GitHub https://github.com/square/picasso/issues/427#issuecomment-174210332.

BoD commented 8 years ago

@JakeWharton Currently my placeholder is not a 9-patch, I didn't think about it. I will try that and see if it works as intended, thank you.

martymiller commented 8 years ago

How can the placeholder be the same size as the target image if the target image hasn't downloaded it yet? The size is unknown.

JakeWharton commented 8 years ago

Rarely are you loading an image and letting its size dictate the size of the UI. Usually the UI component size is known and thus the placeholder can be correctly sized for it based on dp.

On Wed, Sep 14, 2016 at 6:10 PM martymiller notifications@github.com wrote:

How can the placeholder be the same size as the target image if the target images hasn't downloaded it yet? The size is unknown.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/square/picasso/issues/427#issuecomment-247172193, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEEEaJv6HKoJ79GzDD_QpeLXPBIriXhks5qqHDXgaJpZM4BmAyW .

donMuthiani commented 7 years ago

I found a solution that works for this issue 1.Set ScaleType of ImageView to CenterInside

  1. in Picasso declaration do the following; .fit() .CenterCrop() .noFade()
svmszcck commented 7 years ago

@donUA It works thanks dude ;)

dehghani-mehdi commented 4 years ago

Opened in 2014, now we are almost in 2020, still open!

icrespopp commented 1 year ago

almost 2023 and still open, it happens in my app, can not use a placeholder with same aspect ratio of the target image. Using noFade() workaround.

UPDATE: found an interesting alternative, wrapping the placeholder drawable in this CenterCropDrawable fixes the issue https://gist.github.com/rudchenkos/e33dc0d6669a61dde9d6548f6c3e0e7e