BitmapUtils.getDominantColor can be expensive

RESOLVED FIXED in Firefox 29

Status

()

Firefox for Android
General
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mfinkle, Assigned: mfinkle)

Tracking

(Blocks: 1 bug, {perf})

Trunk
Firefox 29
All
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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

4 years ago
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

4 years ago
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 :)
(Assignee)

Updated

4 years ago
Blocks: 959776
(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.
Created attachment 8360213 [details] [diff] [review]
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)
Assignee: nobody → mark.finkle
Attachment #8360213 - Flags: review?(margaret.leibovic)

Comment 5

4 years ago
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 :)
Attachment #8360213 - Flags: review?(margaret.leibovic) → review+
https://hg.mozilla.org/integration/fx-team/rev/bcdaa7136007
Landed a follow-up for a for-loop typo.

https://hg.mozilla.org/integration/fx-team/rev/37c28e253d43
Status: NEW → ASSIGNED
Hardware: ARM → All
Target Milestone: --- → Firefox 29
https://hg.mozilla.org/mozilla-central/rev/bcdaa7136007
https://hg.mozilla.org/mozilla-central/rev/37c28e253d43
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.