Closed Bug 959627 Opened 11 years ago Closed 11 years ago

BitmapUtils.getDominantColor can be expensive

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 29

People

(Reporter: mfinkle, Assigned: mfinkle)

References

Details

(Keywords: perf)

Attachments

(1 file)

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
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! ;)
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 :)
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.
Attached patch Faster dominant code v0.1 (deleted) — Splinter Review
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 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+
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
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: