coil-kt / coil

Image loading for Android and Compose Multiplatform.
https://coil-kt.github.io/coil/
Apache License 2.0
10.87k stars 669 forks source link

Image displayed at placeholder aspect ratio #1505

Open CurtJRees opened 2 years ago

CurtJRees commented 2 years ago

Describe the bug It appears that using a combination of Placeholder, Crossfade and ContentScale.FillWidth will cause an image to be displayed at the placeholder aspect ratio and not the final image.

To Reproduce The test_image drawable in this case is a 16:9 image

@Composable
fun Sample() {
    Column(Modifier.fillMaxSize().verticalScroll(rememberScrollState())) {
        //Correct aspect ratio
        AsyncImage(
            modifier = Modifier.fillMaxWidth(),
            model = ImageRequest.Builder(LocalContext.current)
                .data("https://s3.amazonaws.com/www-inside-design/uploads/2020/10/aspect-ratios-blogpost-1x1-1.png")
                .placeholder(R.drawable.test_image)
                .crossfade(false)
                .build(),
            contentDescription = null,
            contentScale = ContentScale.FillWidth
        )

        Spacer(Modifier.height(16.dp))

        //Incorrect aspect ratio
        AsyncImage(
            modifier = Modifier.fillMaxWidth(),
            model = ImageRequest.Builder(LocalContext.current)
                .data("https://s3.amazonaws.com/www-inside-design/uploads/2020/10/aspect-ratios-blogpost-1x1-1.png")
                .placeholder(R.drawable.test_image)
                .crossfade(true)
                .build(),
            contentDescription = null,
            contentScale = ContentScale.FillWidth
        )
    }
}

Logs/Screenshots Screenshot_20221011-165830

Version Reproduced on both 2.0.0 and 2.2.2, using a Pixel 6 Pro on Android 13

colinrtwhite commented 2 years ago

Which version of Compose are you using?

CurtJRees commented 2 years ago

Which version of Compose are you using?

I've managed to reproduce using Compose 1.1.1, 1.2.0 and 1.3.0-rc01

digitalheir commented 1 year ago

Ran into the same issue trying to workaround #1568. Coil version 2.2.2. Perhaps the solution could be fixed together?

digitalheir commented 1 year ago

The bug seems specific for ContentScale.FillWidth.

Constraints for my container are: Constraints(minWidth = 0, maxWidth = 828, minHeight = 0, maxHeight = 1260)

In modifyConstraints, this causes hasBoundedSize && (hasFixedWidth || hasFixedHeight) to be false, causing the else block to run:

dstWidth = ... constraints.constrainWidth(intrinsicWidth)
dstHeight = ... constraints.constrainHeight(intrinsicHeight)

But this is unexpected to me. Why should it clamp to the intrinsic width? It should clamp to the max width of the constraints!

Something like this is better. Perhaps a bit messy. I'm not acquainted enough with the source code to determine whether this if-else spaghetti is what you really want.

val dstWidth: Float
val dstHeight: Float
if(contentScale == ContentScale.FillWidth && constraints.hasBoundedWidth && intrinsicWidth.isFinite() && intrinsicHeight.isFinite()) {
    val intrinsicRatio = intrinsicWidth / intrinsicHeight 
    dstWidth = constraints.maxWidth
    dstHeight = constraints.maxWidth / intrinsicRatio 
} else if (hasBoundedSize && (hasFixedWidth || hasFixedHeight)) {
   ... etc ...
} else {
   ... etc ...
}

Insn't it better?

Equivalently, my issue is worked-around by passing a fixed width, because then hasFixedWidth || hasFixedHeight will be true.

dessalines commented 2 months ago

I can verify that this is still a bug. When using placeholder + ContentScale.FillWidth, the image sizing gets messed up.