[TIMOB-11282] Android TableView with remote images is very slow

[krowley]

Move image handling into an AsyncTask so it doesn't tie up the UI thread.

[billdawson]

I think there is something wrong. The images only start appearing if I scroll down and back up again.

For example: run the code on an Android 2.2 emulator with this branch. When the application loads in the emulator, don't do anything -- just stare at the first section. If I do that, images never come in. Later, if I then scroll down a whole screen and then back up again, the images start coming in.

[krowley]

OK the changes are ready for review.

[krowley]

I just left more fixes to block "old" images on a fast
scroll, i.e. where you "kick-off" and then the tableview
scrolls for many screens.

[billdawson]

this class has a lot of whitespace formatting problems. Please convert the spaces to tabs.

[billdawson]

I agree that peek() looks pretty darn expensive, since it computes that hash and all that. That being said, there might be more ways to cut down on calling it. Here's a little pseudo-code (actually it's not even pseudo-code, just words) that describes a typical scenario (assume we start with nothing in the cache):

- setImage:
  - if network url:
    - create and execute BackgroundImageTask

- BackgroundImageTask.doInBackground:
  - TiDrawableReference.getBitmapAsync(listener)

- ImageDownloadListener.downloadFinished (assume now successfully in cache):
  - create and execute BackgroundImageTask

- BackgroundImageTask.doInBackground:
  - return the bitmap created by TiDrawableReference.getBitmap (now that the bytes are in the cache) 

In that sequence, peek() is called five times for the same url:

• Twice in BackgroundImageTask.doInBackground.
• Once in ImageDownloadListener.downloadFinished.
• Once in TiDownloadManager.download (called via TiDrawableReference.getBitmapAsync).
• Once in TiFileHelper.openInputStream (called via TiDrawableReference.getBitmap).

I think there are definitely some opportunities for getting those calls down.

Also, I'm worried that maybe the code is a little complicated. Wouldn't it have been enough to kinda just keep everything the way it was except adding the async task to gather up the bitmap after downloadFinished is called? (I.e., did we really need BackgroundImageTask?) I haven't completely thought this through, it's just a suspicion.

[krowley]

OK I submitted changes to address the formatting issues mentioned above,
also I got rid of the array reference that was mentioned.

There is a lot more room for performance improvements here, including the
issues mentioned, but it's going to take a lot more work to get there.

The following could be of benefit in the future:
(1) Cache the last N resized images, where N is some small number like 10-20, this would
probably reduce the need to resize by over 50 percent. Or maybe just cache the resized
images and not the raw ones -- the code isn't set up for that right now, but it could be
changed.
(2) Consider pre-fetching images in a tableview.

I agree that the code is complicated. Just resizing an image after download from the internet
isn't enough here. There is also the case where a bitmap is already in the cache, but
needs to be resized for display. In that case the peek into the cache and the image resize both
need to be done in an AsyncTask. I left the TiDownloadManager class alone here -- it seems to
be reasonable for its task, which is to retrieve images from the internet and put them into the cache.

[billdawson]

Those are some good points (specifically about the raw bytes in the cache already but needing to be scaled.) I have a better understanding of why it's somewhat complicated.

So I'm doing what might well be the final review. More later.

[billdawson]

I was just about to say unit test and KS testing succeeded... but one (and only one) of the KS Image View tests fails, the one called "Image File". So in KitchenSink it's at Base UI -> Views -> Image Views -> Image File. Please have a look at that.

All UI-related Anvil tests passed.

[billdawson]

BTW, I confirmed that the failure mentioned above does not happen on the master branch. It's an NPE in BackgroundImageTask.postExecute.

[krowley]

OK, I fixed the bug that was introduced for images from files.

[billdawson]

CR/FR accepted. KS image view tests fine. UI-related Anvil tests fine.

[realsurfer]

I just downloaded the latest 3.0.2 release from http://builds.appcelerator.com.s3.amazonaws.com/index.html that I assume includes these fixes. Scrolling speed is a lot better but when I reach towards the bottom of my 25 row tableview it's not refreshing the images correctly. I am getting images from previous rows.