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-25540] Android: Fix sizeOf() calculation to prevent IllegalStateException #9620

Merged
merged 7 commits into from Jan 4, 2018

Conversation

garymathews
Copy link
Contributor

  • onTrimMemory() can cause an exception when attempting to release memory
  • Fix sizeOf() calculation to prevent IllegalStateException
TEST CASE
  • Download an image first
  • Run app and select image from Downloads folder
  • Press back to exit app, app should not crash
Ti.Media.openPhotoGallery({
  success:function(event) {
    var image = event.media.imageAsResized(300, 300);
    console.log('resized image');
  }
});

JIRA Ticket

@build
Copy link
Contributor

build commented Nov 20, 2017

Messages
📖

👍 Hey!, You deleted more code than you added. That's awesome!

📖

💾 Here's the generated SDK zipfile.

Generated by 🚫 dangerJS

Copy link
Contributor

@jquick-axway jquick-axway left a comment

Choose a reason for hiding this comment

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

There is a problem here.

The value passed to the LruCache() constructor is in kilobytes and the value returned by the sizeOf() method is in bytes. The two need to use the same units of measure or else the LruCache won't function correctly. In this case, it'll remove bitmaps from the cache too soon.

From looking at Google's Bitmap class docs, this method should be written like the below. I also noticed that we were not rounding-up to the nearest kilobyte, which might have been the source of the error?

int byteCount;
if (android.os.Build.VERSION.SDK_INT >= 19) {
    byteCount = bitmap.getAllocationByteCount();
} else {
    byteCount = bitmap.getByteCount();
}
return (int)Math.ceil((double)byteCount / 1024.0);

Alternatively, switching from kilobytes to bytes for the max size and sizeOf() might be simpler.

@jquick-axway
Copy link
Contributor

Side Note:
I actually want to get rid of our LruCache class usage. It's not Titanium's job to assume which images need to be cached. It's the developer's job to do so, which they can do now on all platform by caching loaded images to blobs.

@garymathews
Copy link
Contributor Author

garymathews commented Nov 21, 2017

@jquick-axway The crash still occurs with your requested changes. I'm not sure why..

@garymathews
Copy link
Contributor Author

I see now, both getByteCount() and getAllocationByteCount() will return 0 on recycled bitmaps. We need to manually calculate the minimum size:

int byteCount = bitmap.getRowBytes() * bitmap.getHeight();
return byteCount / 1024;

@build build added the android label Nov 21, 2017
Copy link
Contributor

@jquick-axway jquick-axway left a comment

Choose a reason for hiding this comment

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

CR: Pass

@ssjsamir ssjsamir self-requested a review December 20, 2017 00:18
@build build added the android label Dec 20, 2017
Copy link
Contributor

@ssjsamir ssjsamir left a comment

Choose a reason for hiding this comment

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

FR Passed application no longer crashed when going back and the log showed the following:
resized image

Test steps

  • Created a new Titanium project
  • Added the test case from the description above
  • Built the app using the artefacts from this PR
  • Ran the program
  • Clicked on a downloaded image
  • Console log message was shown as application went to appc logo
  • Pressed back
  • Application did not crash and was able to view downloaded images

Test Environment
APPC Studio: 5.0.0.201711280737
APPC CLI: 7.0.0
Device: Nexus 6p (8.1.0)
Operating System Name: Mac OS High Sierra
Operating System Version: 10.13
Node.js Version: 8.9.1

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