Improve inflation of display images #702
Conversation
…ncImageLoader to use inflateDisplayImage() method
…ed on native screen density, vs b) scaling based on bounding restrictions imposed by image container or screen size; commented out ResizingImageView
…ridEntityView (although seems not to be affecting case tiles?)
…inction between image util methods that are for images being saved (in FileUtils) and images being displayed (MediaUtil)
There was a problem hiding this comment.
This feels like the wrong place for this method, but didn't have ideas for a better util class to move it to. If you have any thoughts, let me know.
There was a problem hiding this comment.
There's a GeoUtils class in ./app/src/org/odk/collect/android/utilities, maybe there?
(I haven't read this PR, am saying this solely based on the method name)
There was a problem hiding this comment.
This used to work with jr refs that didn't point to files, we should probably have a fallback for what to do here when this doesn't point to a file
|
@amstone326 This is looking good after hte changes outlined |
…st scale down factor we can without making the image too small
…ainer to not scale down too far and use getBitmapScaledExact as a suroutine
There was a problem hiding this comment.
Is this always > 1? If not it'll make it scale to 0, right?
There was a problem hiding this comment.
So when we aren't scaling for density reasons (where the exact final dimension isn't required to be specific), I'm worried about the runtime implications of this scaling. If we load an image and don't density-adjust it and it's 20% bigger than it needs to be it seems like this is gonna really slow things down, right? I can't imagine it's a fast operation.
(Just trying to consider implications for people who don't have the new feature enabled)
Improve inflation of display images
Summary: Technique for predictable inflation of user-provided display images, applied across CommCare wherever it is applicable (Qualification: Not actually everywhere yet, there are probably places I missed. @ctsims, Would like to do a pass with you to determine if there is anywhere else in the app that should be using this).
Details: The key method is getBitmapScaledForNativeDensity() in MediaUtil, which implements the following essential logic (I'm simplifying slightly in words, the code makes the nuances clear):
-Users can set a per-app target density, which indicates the screen density they are designing their images for
-We then apply a scaling technique to up- or down-scale images if they are being shown on a screen whose density is different from the target density
-We also have the option of providing bounding dimensions for the inflation -- if provided, the image will never be scaled up to larger than those dimens. This is useful for not wasting memory on inflating/keeping around large bitmaps in instances in which we know that an image is going into a container of a certain size, which is frequent (images in the case list, icons in GridMediaView, etc.). If explicit bounding dimens are not provided, the screen's width and height are used.
Other Important Notes:
-This is currently disabled by default, and can be enabled in Developer Settings. You can then also set the desired target density in the Developer Settings. Did this to make it easy for ICDS and other projects to play around with different target densities combined with different original image sizes at will.
-I have set it up such that this strategy and ResizingImageView cannot be applied in tandem -- If this strategy is turned off, ResizingImageView will still be applied, assuming the app has it turned on. If this strategy is enabled, ResizingImageView will not get used no matter what.
One less important note: There are currently 4 calls to inflateDisplayImage() where we are NOT providing explicit bounding dimens (and thus defaulting to the screen dimens). In 2 of them I think this is fine/correct because there doesn't seem to be any clear bounding box. But in the other 2, I think there probably is a clear boundary, but I couldn't figure out how exactly to get at them from the code available. Those 2 places are:
Both of these seem like they could benefit from restricting the size of the image inflation based on container. If anyone is doing a really thorough review of this and has thoughts on where to get those values in these 2 instances, let me know!