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

[TIMOB-24528] Android: Fixed issue where images (such as camera photos) will fail to load if too big to be shown by the GPU. Now auto-downscales to fit. #8951

Closed
wants to merge 2 commits into from

Conversation

jquick-axway
Copy link
Contributor

JIRA: https://jira.appcelerator.org/browse/TIMOB-24528

Testing:
See JIRA case above for steps to reproduce this issue.

Notes:

  • Most image loading code has been re-routed to the TiDrawableReference class which now automatically down-samples images if larger than GPU max texture size or 100 MB drawable bitmap limit.
  • Fixed TiDrawableReference.getDensityScaledBitmap() which was wrongly upscaling non-resource bitmaps in RAM and then shrinking the drawable back to the image's original pixel size, causing no size changes onscreen, wasting memory, and increasing chance of out-of-memory exceptions. It now loads images as-is and scales the drawable.
  • Changes made to TiUIImage and TiUIButton are not breaking changes. They will still size/scale images the same as before. They were changed because TiDrawableReference was returning incorrectly scaled drawables (were defaulting to MDPI on all devices), which is now fixed.
  • Fixed caching bugs in TiDrawableReference.peekBounds() method. It was incorrectly using hash-codes as a key (not guaranteed to be unique). Plus, it shouldn't have been caching the bounds of mutable images, such as files under external storage.
  • Fixed 9-patch handling code which incorrectly scaled based on MDPI on all devices. (Also, bitmap loading fixes made TiDrawableReference impacted TiNinePatchHelper class.)

…o load if too big to be shown by the GPU. Now auto-downscales to fit.
@jquick-axway jquick-axway changed the title Android: Fixed issue where images (such as camera photos) will fail to load if too big to be shown by the GPU. Now auto-downscales to fit. [TIMOB-24528] Android: Fixed issue where images (such as camera photos) will fail to load if too big to be shown by the GPU. Now auto-downscales to fit. Apr 8, 2017
@garymathews garymathews added this to the 6.2.0 milestone Apr 11, 2017
Copy link
Contributor

@garymathews garymathews left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tested this with png jpg and bmp file-types using test case:

var window = Titanium.UI.createWindow(),
    imageView = Ti.UI.createImageView({
        image: 'large.jpg', // 10,000x10,000 png, jpg, bmp
        height: Ti.UI.FILL,
        width: Ti.UI.FILL
    });
window.add(imageView);
window.open();

It works fine, but loading an extremely large Bitmap file (~200MB) causes the app to crash. I think we should throw an exception and not attempt to load such a large image.

Loading a large HTTP image (~16MB) also causes the app to crash:

var window = Titanium.UI.createWindow(),
    imageView = Ti.UI.createImageView({
        image: 'http://eoimages.gsfc.nasa.gov/images/imagerecords/73000/73751/world.topo.bathy.200407.3x21600x21600.B2.jpg',
        height: Ti.UI.FILL,
        width: Ti.UI.FILL
    });
window.add(imageView);
window.open();

NOTE: Put the JIRA ticket in each commit you make [TIMOB-24528] blah... and try to keep commit descriptions concise.

Great work! 👍

Log.d(TAG, "path not found: " + apath);
Log.d(TAG, "path not found: " + apath);
}
finally {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor, try to keep consistent with the formatting used in the source file:

...
} finally {
    if (is != null) {
        try {
            is.close();
        } catch (Exception ex) {
        }
    }
...

There are other instances, but I'll just comment here.

* <p>
* You cannot create instances of this class. Instead, you are expected to call its static functions.
*/
private static class GpuInfo
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class should probably be moved into it's own file:
android/titanium/src/java/org/appcelerator/titanium/util/TiGpuInfo.java

@jquick-axway
Copy link
Contributor Author

Good catch regarding loading images via the Internet. I forgot to try that out and left that part of the code as-is, meaning it won't auto-downscale/sample. The only way I can think of fixing it for image files over the network is to download them to a temp file first via our "TiTempFileHelper", because we need to access the image file at least twice (1st time to read its bounds; 2nd time to decode the image to a bitmap). I don't know of any other way to do this reliably... but that said, this is exactly what web browsers do too. Also, we may need to do this to handle the JPEG "EXIF" orientation setting (which we're currently ignoring) so that we can rotate the image to an upright position (involves a 3rd read of the file).

Regarding loading a huge 200 MB image file, I haven't been able to reproduce the crash you're seeing. The size of the image file doesn't matter to the new image loading code since it technically down-samples (not downscales) the image during the decoding process, which means it's skipping pixels during the decoding process, making it the best solution for low-memory situations. For example, a sample size of 2 means it only reads/decodes every other image pixel, which means the resulting bitmap will be smaller by a power of 2.

I've tried the following image files:

  • 10,000x10,000 pixel PNG (215 MB)
  • 10,000x10,000 pixel BMP (400 MB)
  • 30,000x30,000 pixel JPEG (60 MB)

The above all loaded fine on a Pixel XL. A Galaxy Nexus (Android v4.2) was able to load all of the above image except for the BMP, but the BMP didn't cause a crash, however our Activity code was triggering a finish/restart. Perhaps because it was taking too long to load on startup? I'll investigate further.

@jquick-axway
Copy link
Contributor Author

Gary, you've inadvertently found an issue in our TiFileHelper.handleNetworkURL() method with that URL you gave me. The URL you gave me causes an HTTP redirect response to happen (HTTP to HTTPS). That method doesn't handle redirects and ends up providing an InputStream to the HTTP redirect response instead of the image. This means that for the old code (before my change), redirected URLs would not work for "backgroundImage" assignments. The TiDrawableReference class' fallback HTTP request handling code does handle redirects, but it's used after a call to TiFileHandler.handleNetworkURL(). I'm going to fix TiFileHelper to handle redirects and remove the fallback HTTP request code from TiDrawableReference().

Good find!

@hansemannn
Copy link
Collaborator

This may be one of the biggest possible improvements, but it looks outdated and possibly not finished. If there is some feedback how to continue with this, please let me know :)

@jquick-axway
Copy link
Contributor Author

@hansemannn , this was my 1st PR for Titanium and it's super out of date.

I would close it. However, how I'm fetching the GPU's "max texture size" via OpenGL and the UI MAX_BITMAP_BYTES constant are still relevant. These values let you know if the image you are trying to load is too big to be displayed by the GPU or Google's rendering system respectively. It would be nice to integrate them in the future. At least for ImageView's "image" property. (I was trying to make the change for all places images are displayed, but there's is image loading code everywhere and would require a huge refactoring.)

@hansemannn hansemannn closed this Mar 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants