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

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

Open
kaciula opened this issue Mar 1, 2014 · 32 comments
Open

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

kaciula opened this issue Mar 1, 2014 · 32 comments

Comments

@kaciula
Copy link

kaciula commented Mar 1, 2014

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".

@dnkoutso dnkoutso added this to the Picasso Next milestone Mar 2, 2014
@nazr
Copy link

nazr commented May 11, 2014

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
Copy link

zorius commented May 13, 2014

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
Copy link
Contributor

dlew commented Jan 15, 2015

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
Copy link
Member

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

@dlew
Copy link
Contributor

dlew commented Jan 15, 2015

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
Copy link
Member

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
Copy link
Contributor

dlew commented Jan 15, 2015

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
Copy link
Contributor

dlew commented Jan 16, 2015

@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
Copy link
Member

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
Copy link
Contributor

dlew commented Jan 19, 2015

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
Copy link
Contributor

dlew commented May 7, 2015

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
Copy link
Collaborator

dnkoutso commented May 9, 2015

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

@dlew
Copy link
Contributor

dlew commented May 9, 2015

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
Copy link
Collaborator

dnkoutso commented May 9, 2015

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

@dlew
Copy link
Contributor

dlew commented May 10, 2015

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
Copy link

recodyx commented Jun 2, 2015

I hope this issue will be fixed some day.

@luccascorrea
Copy link

So do I.

@midnightviolin
Copy link

noFade() can fix overlap problem when use placeholder

@AndreiHarasemiuc
Copy link

soooooome day

@Dayanand515
Copy link

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

@Danihelsan
Copy link

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
Copy link

BoD commented Jan 23, 2016

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
Copy link
Member

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
#427 (comment).

@BoD
Copy link

BoD commented Jan 23, 2016

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.

BoD added a commit to BoD/CineToday that referenced this issue Jan 23, 2016
@JakeWharton
Copy link
Member

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
#427 (comment).

@BoD
Copy link

BoD commented Jan 23, 2016

@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
Copy link

martymiller commented Sep 14, 2016

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
Copy link
Member

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
#427 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAEEEaJv6HKoJ79GzDD_QpeLXPBIriXhks5qqHDXgaJpZM4BmAyW
.

@donUA
Copy link

donUA commented Dec 11, 2016

I found a solution that works for this issue
1.Set ScaleType of ImageView to CenterInside
2. in Picasso declaration do the following;
.fit()
.CenterCrop()
.noFade()

@svmszcck
Copy link

@donUA It works thanks dude ;)

@JakeWharton JakeWharton removed this from the Picasso Next milestone Mar 3, 2018
@dehghani-mehdi
Copy link

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

@icrespopp
Copy link

icrespopp commented Nov 4, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet