Fix scaling for Bitmap resources with original size #1072
Conversation
| @@ -78,7 +79,15 @@ class BitmapFactoryDecoder @JvmOverloads constructor( | |||
| // Always create immutable bitmaps as they have performance benefits. | |||
| inMutable = false | |||
|
|
|||
| if (outWidth > 0 && outHeight > 0) { | |||
| if (options.size.isOriginal && source.metadata is ResourceMetadata) { | |||
There was a problem hiding this comment.
This won't account for the case when only one dimension is Dimension.Original; also I'm not keen on adding a special case in BitmapFactoryDecoder only for resources. Maybe we can set the density after the bitmap is decoded and avoid creating a special case in BitmapFactoryDecoder?
There was a problem hiding this comment.
This won't account for the case when only one dimension is Dimension.Original
Good callout when one dimension is Dimension.Original. Do you think we need to support mixed Dimension.Original with Dimension.Pixel? Is this something we can regard as invalid?
Also do we need to make similar changes in the other decoders
I would need to check the behavior of ImageDecoderDecoder. Other decoders are probably not density-aware.
Is it possible to avoid adding special cases to each decoder? Maybe we can set the density after the bitmap is decoded and avoid creating a special case in BitmapFactoryDecoder?
- The problem with resetting the density after
BitmapFactory.decode()is that, we probably need another bitmap transformation to scale the previous image to meet the dimension need. - For additional special case, it might be necessary because Android
BitmapFactoryandImageDecoderboth expose specific API to decode nativeResource.Resourceis unique from other sources in that it is density-aware.
There was a problem hiding this comment.
I played around with ImageDecoderDecoder and GifDecoder. They seem to be very different from the default BitmapFactory decoder - Therefore this PR might be better to scoped to only handle Bitmap. I have updated the PR title accordingly.
There was a problem hiding this comment.
Do you think we need to support mixed Dimension.Original with Dimension.Pixel?
My colleague @CordulaMW created the issue #1053 for this PR, so I can comment on at least what we need: We don't need mixed Dimension values. We would either use Dimension.Original or Dimension.Pixel, never both at the same time.
I generally don't think it would make sense to support this, as it's afaik only for scaling. Why would someone want the height dpi-scaled but the width not?
As a counter argument, the android layout system does support it. Not sure.
chao2zhang
changed the title
chao2zhang
force-pushed
the
chao/fixresourcescale
branch
from
8a7a1b9
to
5a30dd9
Compare
|
Hello there, I have tried out the PR and it works great for me at least :) |
There was a problem hiding this comment.
Sorry for the delay on this. I agree let's tackle this only for the case where both dimensions are Dimension.Original. We can "fix" the behaviour for single dimension later if someone has a strong argument that it should be handled differently than it currently is. Thanks again!
* Fix scaling for resources with original size * API dump * Refactor exif related from BitmapFactoryDecoder * Extract scaling into a separate function * Fix ktlint
Resolves #1053
When the requested size is
Original, previously it would fall into(outWidth > 0 && outHeight > 0), whereval dstWidth = width.pxOrElse { srcWidth }would always be evaluated tosrcWidth, andsrcWidthignores the screen density.This PR addresses the case of android resources specifically by applying the same logic handled inside
BitmapFactory.decodeResourceStream().Testing
https://github.com/chao2zhang/coil/tree/chao/fixresourcescale-test
Tested on 440dpi and 560dpi devices using the code snippet in the original issue. Coil loaded image and android native BitmapFactory displays the image of the same size.