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 affects image scaling when we use CrossfadeTransition and the placeholder image is larger #1688

Open
tfcporciuncula opened this issue Mar 26, 2023 · 3 comments
Labels
help wanted Issues that are up for grabs + are good candidates for community PRs

Comments

@tfcporciuncula
Copy link

Describe the bug
When I have CrossfadeTransition.Factory() passed to the ImageLoader, I get a different final scale in an AsyncImage if it uses a placeholder that is larger than the final image. Everything is fine if I remove the crossfade, remove the placeholder, or use a placeholder that matches the size of the final image.

To Reproduce
Minimum reprodicible example here: https://github.com/tfcporciuncula/coil-crossfade

You can clone the repo, run the code, and observe that:

  • If we keep CrossfadeTransition.Factory() around, we'll get different scaling results depending on whether we keep or remove the placeholder.
  • If we remove CrossfadeTransition.Factory(), the scaling is the same regardless if we have the placeholder or not.
  • If we keep CrossfadeTransition.Factory() but change the placeholder from R.drawable.placeholder_larger to R.drawable.placeholder, which has the exact same size as our final imagem, then everything is fine.

Logs/Screenshots

Image on the left is what we get with crossfade and the large placeholder. Image on the right is the correct scaling which we get by either removing the crossfade, removing the placeholder, or using a placeholder that matches the final image size.

1 2

We're using ContentScale.Crop above, but we can move to ContentScale.Fit to make the difference more visible:

w c

Version
Coil 2.3.0, reproduced on a Pixel 4 running Android 13 and on a Pixel 6 emulator (1080x2400 420dpi) running Android 10 (screenshots from the latter).

@colinrtwhite
Copy link
Member

Thanks for the repro project! At first glance, seems like this could be related to: #1505

@colinrtwhite colinrtwhite added the help wanted Issues that are up for grabs + are good candidates for community PRs label Mar 26, 2023
@tfcporciuncula
Copy link
Author

tfcporciuncula commented Mar 27, 2023

True! After reading through it I thought the issue would also happen with a smaller placeholder that has a different aspect ratio, but it doesn’t. The placeholder needs to have a different aspect ratio, but must be larger. I tested it with a larger placeholder that has the same aspect ratio and the issue didn’t happen.

I pushed one more commit in my project adding more placeholder variations and updating my comment just so it’s also easy to verify that:

  • Issue doesn’t happen if placeholder is smaller, even if it has a different aspect ratio
  • Issue doesn’t happen if the placeholder is larger but has the same aspect ration

@alexej520
Copy link

Hi!
I forked @tfcporciuncula example, and added working workaround commit
The solution is simple: use a copy of CrossfadeTransition instead of original one.
This is a strange solution, but it works for me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Issues that are up for grabs + are good candidates for community PRs
Projects
None yet
Development

No branches or pull requests

3 participants