867627 - Dominant color favicon backgrounds are really slow to update

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[:Margaret Leibovic]

We call updateFavicon() a lot, we should do something smarter to cache the dominant color instead of computing it each time we want to use it.

Comment 1

[:Margaret Leibovic]

Attachment: WIP

This is a lazy way of caching the colors. Ideally, it would be nice to better centralize all this repeated favicon logic, but that's hard to do with the way our awesomescreen tabs are currently implemented, so maybe that's just something to consider when we implement the new about:home.

I want to do some benchmarking to see how long BitmapUtils.getDominantColor() actually takes for a 16x16 icon, and if it's not very long, it's probably fine to be doing that on the main thread, especially if we're caching it like this.

Comment 2

[Brian Nicholson (:bnicholson)]

Comment on attachment 744373 [details] [diff] [review]
WIP

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

::: mobile/android/base/Favicons.java
@@ +76,5 @@
>          // Create a failed favicon memory cache that has up to 64 entries
>          mFailedCache = new LruCache<String, Long>(64);
> +
> +        // Create a cache to store favicon dominant colors
> +        mColorCache = new LruCache<String, Integer>(64);

64 seems small to me; I see more than 64 entries in AllPagesTab alone. Assuming an average favicon size of 32x32 (which is an overestimate), that means the typical favicon will be 32x32x4 = 4kB, so we should probably have at least 256 entries.

It'd be nice if we could use the same cache to store a pairing of favicon/color data rather than having two separate caches, but I guess that's not really an option until we use the favicon cache for all tabs.

Comment 3

[:Margaret Leibovic]

Attachment: patch

I decided that I didn't like calling getFaviconColor() in a bunch of different places then passing around the color, mainly because we don't even need the color in the case of empty or large favicons. As a compromise, instead of letting FaviconView know about urls, I just made the parameter a string key that's used for caching (this is more accurate for search engines anyway, for which we just use the engine name).

I tested this with my (huge/old) default profile on my Galaxy Nexus, and this performs way better than Nightly. I also timed getDominantColor(), and times ranged from 4-20ms, but most times were around 8-10ms.

Comment 4

[Brian Nicholson (:bnicholson)]

Comment on attachment 745932 [details] [diff] [review]
patch

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

::: mobile/android/base/awesomebar/AwesomeBarTab.java
@@ -92,5 @@
> -        Bitmap favicon = null;
> -        if (b != null) {
> -            Bitmap bitmap = BitmapUtils.decodeByteArray(b);
> -            if (bitmap != null) {
> -                favicon = Favicons.getInstance().scaleImage(bitmap);

It looks like you moved the scaleImage() call from AwesomeBarTab to BookmarksTab. Won't that break favicon scaling for AllPagesTab and HistoryTab?

Comment 5

[Brian Nicholson (:bnicholson)]

Comment on attachment 745932 [details] [diff] [review]
patch

Oh nevermind...that's the only place that uses the cursor.

Comment 6

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ffda3e37d827

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ffda3e37d827

Comment 8

[Aaron Train [:aaronmt]]

Fast!