Closed Bug 1065490 Opened 5 years ago Closed 5 years ago

BitmapUtils.getDominantColor takes 990ms on startup

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 35

People

(Reporter: rnewman, Assigned: mfinkle, Mentored)

References

(Blocks 2 open bugs)

Details

(Keywords: perf, Whiteboard: [good second bug][lang=java])

Attachments

(1 file, 1 obsolete file)

mfinkle: "BitmapUtils.getDominantColor is taking 990ms, mainly from trying to get the dominant color of the lightweight theme. I think this was used to exend a color into the old about:home page. It doesn't work anymore, although we might want it back. In any case, I bet we don't need to examine the entire lightweight theme image to get the color."

http://people.mozilla.org/~mfinkle/fennec/profiles/nexuss-home-startup-notfirstrun-20140910.trace
We compute the dominant color of the bottom chunk of the LWT for a few reasons:

* To decide if this is a light or dark theme. This should really be done with the *top* of the image.
* To compute a fill color in case the image is too narrow for the display. This should also be done with the top of the image.
* To produce a LightweightThemeDrawable on request, with the right background color.

We already only check the bottom 25% of the image. That should probably be further limited to a pixel count.

My proposal:

* Compute the dominant color using a strip the size of the toolbar, taken from the *top* of the image.
* Do so without copying.

Doing this lazily doesn't help; every UI element is a Themed*, which will immediately check if the theme is light or dark.

We should store whether the theme is light or dark, so we don't have to do this every time!
We might also consider optimizing getDominantColor.

It's a Java-land loop over every pixel in the bitmap. colorToHSV alone is 6.8% of the entire startup profile, of which 75% is Color.RGBtoHSV.

We could memoize the color lookup, we could move this into native code, or we could see if there's a better way to do this within Android already.
Three sub-bugs here. File 'em as you take 'em.
Mentor: rnewman
Whiteboard: [good second bug][lang=java]
Keywords: perf
(In reply to Richard Newman [:rnewman] from comment #1)

> My proposal:
> 
> * Compute the dominant color using a strip the size of the toolbar, taken
> from the *top* of the image.

How about: using a top/left square of the image where the square is the toolbar height. It's the top/left side of the image which potentially uses the color anyway.

> * Do so without copying.

I added some timing code to the crop/copy and it takes ~1ms. I think the bitmap code must use COW and we never write.

I'll add an additional point:
* Use the accentcolor which is an optional part of the lightweight theme itself. Desktop uses the "accentcolor" as the background color and uses it for luminance calculations too.
 
> We should store whether the theme is light or dark, so we don't have to do
> this every time!

I plan to do this as part of bug 846184
Attached patch lwt-pref v0.1 (obsolete) — Splinter Review
This patch passes the accentcolor into the code that determines the dominant color. We skip the dominant color path if accentcolor is given.

If the dominant color is required, we use the top/left square with <toolbar height> dimensions.

Using some System.currentTimeMillis timing, the current code takes ~2200ms to get the dominant color. Using the top/left toolbar height square takes ~370ms. A significant drop, but could be better. Take solace in knowing we will skip it altogether for the majority of themes.

Also, caching the color is a different bug, but will make it a true "one time" hit.
Assignee: nobody → mark.finkle
Attachment #8487680 - Flags: review?(rnewman)
Comment on attachment 8487680 [details] [diff] [review]
lwt-pref v0.1

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

::: mobile/android/base/LightweightTheme.java
@@ +134,4 @@
>  
>          // The lightweight theme image's width and height.
>          int bitmapWidth = bitmap.getWidth();
>          int bitmapHeight = bitmap.getHeight();

Make these two final for reassurance; they're now used much later in the method.

@@ +136,5 @@
>          int bitmapWidth = bitmap.getWidth();
>          int bitmapHeight = bitmap.getHeight();
>  
> +        // Calculate the dominant color the hard way, if not given to us.
> +        if (TextUtils.isEmpty(color)) {

You should probably attempt to parse the color here, because a malformed string can cause parseColor to throw:

  Color parsedColor = null;
  if (!TextUtils.isEmpty(color)) {
      try {
          parsedColor = Color.parseColor(color);
      } catch (IllegalArgumentException e) {
          // Malformed color.
      }
  }

  if (parsedColor == null) {
      // Find the dominant color.
  }

@@ +143,2 @@
>  
> +            // A cropped bitmap of the top/left pixels.

Reading the source, it looks like most of the code paths create a new bitmap, so I guess this is fast because bitmaps need to be fast!

@@ +149,5 @@
> +
> +            // Dominant color based on the cropped bitmap.
> +            mColor = BitmapUtils.getDominantColor(cropped, false);
> +        } else {
> +            mColor = Color.parseColor(color);

... and we don't want it to throw here!

@@ +155,4 @@
>  
>          // Calculate the luminance to determine if it's a light or a dark theme.
>          double luminance = (0.2125 * ((mColor & 0x00FF0000) >> 16)) + 
>                             (0.7154 * ((mColor & 0x0000FF00) >> 8)) + 

Bonus points for fixing trailing whitespace on these two lines.

@@ +165,1 @@
>              mBitmap = bitmap;

It's a tiny bit of scope creep, but should only be one line: what do you think about cropping the image down to screen width here?

Best case, it copies the bitmap and GCs the old one: our resident set becomes smaller. That's important if this is a huge theme on a narrow-screened device.

Worst case, it just gives us a new reference to the same large byte array, but reading the source it looks like that doesn't happen.

We might even be able to crop in height, assuming we know the max height of toolbar, tabs switcher, etc.
Attachment #8487680 - Flags: review?(rnewman) → feedback+
(In reply to Richard Newman [:rnewman] from comment #6)

> >          int bitmapWidth = bitmap.getWidth();
> >          int bitmapHeight = bitmap.getHeight();
> 
> Make these two final for reassurance; they're now used much later in the
> method.

Done

> > +        // Calculate the dominant color the hard way, if not given to us.
> > +        if (TextUtils.isEmpty(color)) {
> 
> You should probably attempt to parse the color here, because a malformed
> string can cause parseColor to throw:

I did basically as you described. Color.parseColor returns an | int | though so I couldn't use | null | as the unknown state. Switched to a boolean.

> >          // Calculate the luminance to determine if it's a light or a dark theme.
> >          double luminance = (0.2125 * ((mColor & 0x00FF0000) >> 16)) + 
> >                             (0.7154 * ((mColor & 0x0000FF00) >> 8)) + 
> 
> Bonus points for fixing trailing whitespace on these two lines.

Done

> >              mBitmap = bitmap;
> 
> It's a tiny bit of scope creep, but should only be one line: what do you
> think about cropping the image down to screen width here?

This failed to work correctly for me:
            mBitmap = Bitmap.createBitmap(bitmap, 0, 0, maxWidth, bitmapHeight);

So I am punting this to a followup bug. The background was just the dominant/accent color. No image was visible.
Attached patch lwt-pref v0.2Splinter Review
Most comments addressed.
Attachment #8487680 - Attachment is obsolete: true
Attachment #8488121 - Flags: review?(rnewman)
Attachment #8488121 - Flags: review?(rnewman) → review+
Blocks: 1066320
https://hg.mozilla.org/mozilla-central/rev/c08968fcc9bd
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
You need to log in before you can comment on or make changes to this bug.