Status Update
Comments
ch...@google.com <ch...@google.com>
nj...@google.com <nj...@google.com> #2
If you have unbounded constraints coming in, what size will you use to load the images? Even if you do attempt to download the image but we don't end up drawing it, it is wasted cycles to download an image that we do not end up rendering. Even if we have the constraints, we would end up potentially downloading a larger image than necessary and drawing it in smaller bounds causing other potential performance issues as we consume more of the texture cache on the GPU.
ch...@gmail.com <ch...@gmail.com> #3
If you have unbounded constraints coming in, what size will you use to load the images?
Well, they're usually only bounded on 1 dimension. For constraints of width = 300.dp, height = Infinity
, Painters without an intrinsic size resolves these to width = 300, height = 0
and doesn't call onDraw()
. That's fine, but I'd love a call to know that size. With (300,0) I can assume that 0 = unbounded, and load the image with a width of 300.
If the constraints are unbounded on both directions, then sure there's no signal to use.
Even if we have the constraints, we would end up potentially downloading a larger image than necessary
How so? (maybe I'm missing something). If the constraints allows a maximum of 300dp, that is the size we would fetch. If the image is in fact smaller than that, we just load the smaller image with no scaling.
nj...@google.com <nj...@google.com> #4
That's fine, but I'd love a call to know that size. With (300,0) I can assume that 0 = unbounded, and load the image with a width of 300.
What would you do with this information? Even loading an image with dimensions of 300 x 0 doesn't draw anything. The current Painter implementation helps developers do the right thing here and skip drawing and arguably should skip downloading the image as well saving on bandwidth.
How so? (maybe I'm missing something). If the constraints allows a maximum of 300dp, that is the size we would fetch. If the image is in fact smaller than that, we just load the smaller image with no scaling.
If you did not have any bounds information, some implementations might just download the full image size which is something we should avoid. In addition to wasted bandwidth, we would download an image much larger than the area it eventually gets drawn into causing more performance issues with GPU texture caching etc.
Overall this seems like a problem better suited to add minimum width/height modifiers to the same composable the painter is being used with. Taking a step back, what is the issue we are trying to solve here? It is not immediately clear how adding a sizeChange callback would help in cases where the content dimension is 0. Feel free to setup some time on calendar and we can walk through some examples. My general concern is that adding this API surface might not address the root problem that is trying to be solved and can potentially cause others.
ch...@gmail.com <ch...@gmail.com> #5
What would you do with this information? Even loading an image with dimensions of 300 x 0 doesn't draw anything.
With a size of (300, 0) I'd load an image with width = 300, height = auto based on aspect ratio (since I'm assuming that 0 == infinity).
If you did not have any bounds information, some implementations might just download the full image size which is something we should avoid. In addition to wasted bandwidth, we would download an image much larger than the area it eventually gets drawn into causing more performance issues with GPU texture caching etc.
Yep, that's exactly the problem I'm trying to solve.
Note: there's actually two problems here:
-
What 'size' image to download. This is up to apps themselves to implement, but we do provide the necessary hooks for apps to do this. Again, using the resolved painter size.
-
Regardless of what image is downloaded, both Coil and Glide will decode and scale the resulting image to meet the size (using
inSampleSize
I assume). This is the main one we have control over.
Taking a step back, what is the issue we are trying to solve here? It is not immediately clear how adding a sizeChange callback would help in cases where the content dimension is 0.
Yes agreed, since I'm assuming that 0 == infinity in most cases. This is where the incoming constraints are better, as they are the source of truth. PaintModifier
is also the last modifier in the chain for Image
so should be the 'final' constraints.
In an ideal world I'd do something like this:
override fun onConstraintsIncoming(constraints) {
requestSize = IntSize(
width = if (constraints.hasBoundedWidth) constraints.maxWidth else AUTO,
height = if (constraints.hasBoundedHeight) constraints.maxHeight else AUTO
)
}
nj...@google.com <nj...@google.com> #6
But at draw time 0 isn't a signal for infinity, 0 is 0. So even if we see the actual constraints and download the image based on the width, we still won't draw any pixels here. This looks like more of a layout issue than a Painter one.
Have we tried configuring a minWidth/minHeight modifier on the composable the Painter is drawn on or even a fixed size modifier? Also is there a sample composable I could look at? Feel free to throw some time up on GVC with me and we can discuss in more detail.
ph...@monzo.com <ph...@monzo.com> #7
Did you get anywhere with this? I've been migrating us from Glide to Coil, and I ran up against this a bunch of times. It makes me wonder whether Painter
is the right API to use for image loading, or whether I should write something custom that's either a @Composable
function or a Modifier
(so it can hook in to layout constraints).
It's tricky to even even figure out what the expected behaviour is in cases where one or both dimensions are unbounded, but the current behaviour seems surprising to me. This is about the most basic example you could imagine, and it doesn't work (it'll never even request the image). I think I would probably expect it to load the image at its intrinsic size.
Column(Modifier.fillMaxSize().verticalScroll(rememberScrollState())) {
Image(
rememberImagePainter("https://picsum.photos/seed/abc123/512/512"),
contentDescription = null
)
}
Here's another example that renders nothing, where the intention seems pretty clearly to be "use at most 256dp of width, and determine the height based on the intrinsic size of the image".
Column(Modifier.fillMaxSize().verticalScroll(rememberScrollState())) {
Image(
rememberImagePainter("https://picsum.photos/seed/abc123/512/1024"),
contentDescription = null,
modifier = Modifier.widthIn(max = 256.dp),
contentScale = ContentScale.Fit
)
}
One assumption I'm making here is that the vast majority of images we use in mobile apps actually have an intrinsic size - they don't come from a service where you can request images at an arbitrary size. So it seems to me that sensible default behaviour is more important than concerns about downloading an image that's too large - most of the time you don't have any control over that (what you do have control over is the size of the in-memory bitmap you create). Anyone using a service that can provide images at an arbitrary size will be writing custom integration code anyway, so it seems reasonable to make them decide what they'll request when one or both dimensions are unbounded.
This all starts to get even harder when you think about how it interacts with things like ContentScale
or coil.size.Scale
Description
Currently Painters only see the resolved layout size in
onDraw()
. This has an issue though, thatonDraw()
is only called if the height & width is > 0. If either is 0, which can happen frequently when unbounded constraints come in (say from LazyColumn),onDraw()
isn't called and the painter doesn't see the size.Specifically this for the Accompanist image loading composables. We've recently had a large refactor, and use the
size
ononDraw()
to influence what size image to load. The ideal situation is that the painter sees the incoming constraints fromPaintModifier
. This would allow Accompanist to properly load the image based on the constraints, rather than the resolved size.Could we add a new callback to Painter?
onSizeChange()
,onIncomingConstraints()
, etc