959627 - BitmapUtils.getDominantColor can be expensive

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

Startup profiles show BitmapUtils.getDominatColor taking ~6% (~301ms) of the startup time. It was only called twice.

We should look for ways to improve performance, maybe even looking at less-than 100% pixel coverage. Make sure we don't regress the utility badly in the process. See bug 867249.

profile: http://people.mozilla.org/~mfinkle/fennec/profiles/nexuss-home-startup.trace

Comment 1

[:Margaret Leibovic]

As a start, we can try moving these getWidth()/getHeight() calls into local variables:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/gfx/BitmapUtils.java#238

After that, I wonder if we can try a random sample of the pixels. We definitely shouldn't do something like looking at every other pixel, since icons may have patterns that would make that approach problematic. Maybe we can randomly increment row and col by 1 or 2, instead of always incrementing by 1? Or maybe just do that for col, so we don't skip entire rows (although that would certainly save time! ;)

Comment 2

[:Margaret Leibovic]

Or, should we be doing something smarter to convert the bitmap into a byte array, rather than using these nested for loops and calling getPixel on every iteration?

Cc'ing ckitching, since this sounds like something he might have fun with :)

Comment 3

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

(In reply to :Margaret Leibovic from comment #2)
> Or, should we be doing something smarter to convert the bitmap into a byte
> array, rather than using these nested for loops and calling getPixel on
> every iteration?

Example of that here:
http://stackoverflow.com/questions/4715840/improving-speed-of-getpixel-and-setpixel-on-android-bitmap

But be mindful that it does have memory impact.

Comment 4

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

Attachment: Faster dominant code v0.1

This patch makes a few changes:
1. Local variables for bitmap width and height.
2. Pull the hsv float array out of the loop so we don't new an array each pass.
3. Extract the raw pixels instead of using getPixel in the loop.

The result via profiling on the Nexus S is a drop in BitmapUtils.getDominantColor to 2.7% (122ms) of startup, around a 60% improvement.

Color.colorToHSV is still taking a decent chunk of the time (59ms) but I think this patch is enough of a win to land.

Note: Bitmap.getPixels(...) is barely showing up (0.354ms)

Comment 5

[:Margaret Leibovic]

Comment on attachment 8360213 [details] [diff] [review]
Faster dominant code v0.1

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

Nice, I like that you have numbers to back up this change.

::: mobile/android/base/gfx/BitmapUtils.java
@@ +238,5 @@
>  
> +      int height = source.getHeight();
> +      int width = source.getWidth();
> +      int[] pixels = new int[width * height];
> +      source.getPixels(pixels, 0, width, 0, 0, width, height);

What a terribly unreadable API! But not your fault :)

Comment 6

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

https://hg.mozilla.org/integration/fx-team/rev/bcdaa7136007

Comment 7

[Richard Newman [:rnewman]]

Landed a follow-up for a for-loop typo.

https://hg.mozilla.org/integration/fx-team/rev/37c28e253d43

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bcdaa7136007
https://hg.mozilla.org/mozilla-central/rev/37c28e253d43