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

Crash in glide compose alpha.5, when computing scale factor #5256

Closed
joerogers opened this issue Aug 23, 2023 · 2 comments · Fixed by #5264
Closed

Crash in glide compose alpha.5, when computing scale factor #5256

joerogers opened this issue Aug 23, 2023 · 2 comments · Fixed by #5264

Comments

@joerogers
Copy link

joerogers commented Aug 23, 2023

Glide Version: 1.0.0-alpha.5/4.16.0

Integration libraries: okhttp4-integration (although issue is not here)

Device/Android Version: Emulator/Pixel 6a

Issue details / Repro steps / Use case background:

Looks like glide triggers a ScaleFactor is unspecified exception, if either the width or height is implicit and the other is computed from a ratio if the width or height resolves to 0.dp.

I found this, because I had gotten 1.0.0-alpha.3 working with a compose MotionLayout where the image is "Gone" in the initial constraints and appears in end constraints. It took time to determine the root cause to see if it was the MotionLayout code or Glide, but I was able to reproduce using a standard GlideImage without MotionLayout involved.

Here is the abbreviated stack:

FATAL EXCEPTION: main
                              Process: com.example.app, PID: 5135
                              java.lang.IllegalStateException: ScaleFactor is unspecified
                              at androidx.compose.ui.layout.ScaleFactor.getScaleX-impl(ScaleFactor.kt:48)
                              at androidx.compose.ui.layout.ScaleFactorKt.times-UQTWf7w(ScaleFactor.kt:154)
                              at com.bumptech.glide.integration.compose.GlideNode.modifyConstraints-ZezNO4M(GlideModifier.kt:481)
                              at com.bumptech.glide.integration.compose.GlideNode.measure-3p2s80s(GlideModifier.kt:440)
                              at androidx.compose.ui.node.LayoutModifierNodeCoordinator.measure-BRTryo0(LayoutModifierNodeCoordinator.kt:116)
                              at androidx.compose.foundation.layout.AspectRatioNode.measure-3p2s80s(AspectRatio.kt:119)

It seems like the bug is the intrinsicWidth/intrinsicHeight is 0.dp and/or the constrainedWidth/Height is 0.dp. Essentially this should be checked for 0.dp as division by 0 results in NaN and ends up triggering the exception. I suspect if 0.dp is determined as the width or height there is no need to issue the network download or draw the image as it can not be seen. The easy fix is to not crash, but I couldn't quite follow to determine if there is an optimization to avoid the network/cache fetch as well.

The code in question is:

   val constrainedWidth = constraints.constrainWidth(intrinsicWidth)
   val constrainedHeight = constraints.constrainHeight(intrinsicHeight)

   --->  Check for 0.dp here and if so skip the scale computation.
   
    val srcSize = Size(intrinsicWidth.toFloat(), intrinsicHeight.toFloat())
    val scaledSize =
      srcSize * contentScale.computeScaleFactor(
        srcSize, Size(constrainedWidth.toFloat(), constrainedHeight.toFloat())
      )

Here are some examples outside of the MotionLayout that trigger the same. I suspect MotionLayout under the covers is doing one of these variations when the image layout is marked as Gone.

GlideImage(
                model = "https:// image from internet....",
                contentDescription = null,
                modifier = Modifier.width(IntrinsicSize.Min).aspectRatio(1f),
                contentScale = ContentScale.Crop
            )

I had some crashes with Modifier.fillMaxWidth(0f), but not as consistent at reproduction.

Technically it doesn't make sense to have a width/height resolve to zero. However a MotionLayout where the image is initially Gone will trigger the issue. It also seems related to the aspect ratio. I'm downloading into a square so I used 1, but essentially I don't know the exact size as it is a percentage of the width.

I do have a few work arounds but neither are ideal for the final solution:

  1. Set the image to be invisible to start vs gone. However, if the user never interacts triggering the view expansion, I'm in theory fetching and drawing an image with alpha 0.
  2. Use a Box + GlideSubcomposition() which doesn't seem to have this same issue, but this seems heavy handed.
  3. Fallback to alpha.3. (at this point, I'd probably temporarily do 1 or 2.
@sjudd
Copy link
Collaborator

sjudd commented Aug 24, 2023

Thanks for reporting this!

sjudd added a commit that referenced this issue Aug 24, 2023
This will happen if either width or height (or both) are constrained to
have a 0 size.

We already avoid loading the image via AsyncImageSize /
inferredGlideSize. This only impacts layout.

Fixes #5256
sjudd added a commit that referenced this issue Aug 24, 2023
This will happen if either width or height (or both) are constrained to
have a 0 size.

We already avoid loading the image via AsyncImageSize /
inferredGlideSize. This only impacts layout.

Fixes #5256
@joerogers
Copy link
Author

joerogers commented Sep 6, 2023

@sjudd Just wanted to let you know this fix doesn't look like it got correctly pulled into the alpha 6 build. I know you were trying a new build process so maybe there is a glitch in this process.

Here is what I noticed related to the new build:

  1. The new process doesn't provide source code. So all investigations are based on decompiling to java.
  2. In the fix you added a check to see if the scale factor was unspecified, however in the decompilation that check is missing and it proceeded into the times logic as before which implies that alpha 6 is using the old code somehow.

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

Successfully merging a pull request may close this issue.

2 participants