709103 - Optimize creating and saving thumbnail bitmaps

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

Currently, we create a copy of the LayerView mBuffer as a bitmap, then we create a second copy cropped to the minimum length (width or height of screen), then we save it to the DB. On HDPI phones, we are saving at least 480x480 px images as bitmaps. These images are ~1.1MB on the Tegras.

Then we use those images in places like the AboutHome page, sync loading the image and displaying it in a 122px image.

We also hold a copy of the thumbnail with each tab. We can stop doing this if we have no plans to use thumbnails in the tab list. These images are ~1.1MB on the Tegras.

I'd like to streamline this a bit, if possible:
* Avoid making a copy of the mBuffer. Can we use Cairo or GL here?
* Scale to lower than minimum screen dimension, so the we are holding and saving much smaller images - true thumbnails. It will also make loading the images from the DB faster and use less memory.

I need to see what size the stock browser saves thumbnails as in the history DB as a comparison.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Call used to get the base bitmap:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/gfx/GeckoSoftwareLayerClient.java#205

I'd like to make a specialized version for just getting the thumbnail bitmap.

Call to crop and save the thumbnail:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Tab.java#135

Note the mThumbnail holds on to the bitmap, increasing our memory usage.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch 1

This patch starts to try to clean things up:
* Do not make a thumbnail on tab select. We don't need to do this.
* Move the thumbnail creation off the main UI thread in handleContentLoaded
* Scale the thumbnail to 96x96 when holding in a Tab and saving to DB

The patch does not try to reduce the memory use of creating bitmaps, but does reduce our overall memory use. Also speeds up loading ABoutHome due to smaller thumbnails. Speeds up page load by moving getBitmap off main UI and killing one of the getBitmap calls that happens during page load.

I wish we could reuse the getBitmap call used for rememeberLastScreen runnable in handleContentLoaded. Followup bug?

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch 1 v2

Moves the mRememberLastScreenRunnable into the new runnable, instead of having two separate ones.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 580488 [details] [diff] [review]
patch 1

The v2 patch doesn't build. COntinue with this one for now please.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 580492 [details] [diff] [review]
patch 1 v2

Doesn't build :(

Comment 6

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 580488 [details] [diff] [review]
patch 1

Review of attachment 580488 [details] [diff] [review]:
-----------------------------------------------------------------

the scaling is right, but lets do all of our screenshot operations in one runnable (and using one bitmap)

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch 2

This patch is basically the same as the last, but I made mRememberLastScreenRunnable into SessionSnapshotRunnable, which takes an optional param to create a thumbnail for a tab. This means we only use one bitmap (and one call to getBitmap) during pageload.

I also found that DOMContentLoaded was generally too early in the pageload cycle to reliably create screen snapshots. I found the handleDocumentEnd happens a little later and makes thumbnails much more reliably. I kept the 500ms delay in the hopes it allows the page scripts to wrap up before making the screenshot bitmap. We might try removing that too.

Comment 8

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 580718 [details] [diff] [review]
patch 2

Review of attachment 580718 [details] [diff] [review]:
-----------------------------------------------------------------

r-, mostly for the incorrect synchronization

::: mobile/android/base/GeckoApp.java
@@ +568,5 @@
>          if (outState == null)
>              outState = new Bundle();
> +
> +        Runnable r = new SessionSnapshotRunnable(null);
> +        r.run();

just new SessionSnapshotRunnable(null).run();

@@ -572,5 @@
>          outState.putString(SAVED_STATE_VIEWPORT, mLastViewport);
>          outState.putByteArray(SAVED_STATE_SCREEN, mLastScreen);
>      }
>  
> -    Runnable mRememberLastScreenRunnable = new Runnable() {; 

please don't move the runnable class definition, it would make the review easier

@@ +920,5 @@
> +            mThumbnailTab = thumbnailTab;
> +        }
> +
> +        public void run() {
> +            synchronized (this) {

this isn't doing anything because you're creating new runnables every time. You'll need to keep some object around to synchronize on.

@@ +922,5 @@
> +
> +        public void run() {
> +            synchronized (this) {
> +                if (mUserDefinedProfile)
> +                    return;

this isn't right anymore, please drop it while you're here.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch 3

* Moves code get to old location so patch is easier to read
* Uses | new SessionSnapshotRunnable(null).run(); |
* Removes mUserDefinedProfile
* Synchronizes on the selected tab object

Tested and I still see thumbnails! I'll remove the debug output before landing.

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 580918 [details] [diff] [review]
patch 3

Review of attachment 580918 [details] [diff] [review]:
-----------------------------------------------------------------

r=blassey with the synchronization changed

::: mobile/android/base/GeckoApp.java
@@ +588,2 @@
>  
> +            synchronized (tab) {

I thought about this some more, and I think synchronizing on mSoftwareLayerClient is the better choice. I can see a race condition happening where the load happens, the user switches tab then clicks the home button. We'd get two Runnables operating on the same bitmap because they'd have different tab objects.

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

switched to mSoftwareLayerClient and landed
https://hg.mozilla.org/mozilla-central/rev/70096993b6d6

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: bustage fix for OOM

Uses a try/catch to squelch the fatal exception

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

OOM fix
https://hg.mozilla.org/mozilla-central/rev/2662a7249e7d