Bug 1403653 - Part 1 - Refactor getDominantColor. r=nechen
☠☠ backed out by 9f108013a77d ☠ ☠
authorJan Henning <jh+bugzilla@buttercookie.de>
Sat, 14 Oct 2017 19:23:03 +0200
changeset 438297 e40240828163f60206ab8cee82abb9eb1bd535c4
parent 438296 66232f26609ec498e075b71a94c2b719307867fb
child 438298 1e66240eb8b6287b0830066e2ba2e5d9c86f2a03
push id8114
push userjlorenzo@mozilla.com
push dateThu, 02 Nov 2017 16:33:21 +0000
treeherdermozilla-beta@73e0d89a540f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersnechen
bugs1403653, 1318667
milestone58.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1403653 - Part 1 - Refactor getDominantColor. r=nechen We want to use the Palette library for getting a fallback accent colour for lightweight themes, however because of bug 1318667, we might have to continue using our own implementation of getDominantColor on x86 devices. Therefore we move this into BitmapUtils, so we can have a central location from which to switch between our own and the Palette library implementation. MozReview-Commit-ID: 52WsfZbW12x
mobile/android/app/build.gradle
mobile/android/base/java/org/mozilla/gecko/icons/processing/ColorProcessor.java
mobile/android/base/java/org/mozilla/gecko/util/ShortcutUtils.java
mobile/android/geckoview/build.gradle
mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/BitmapUtils.java
mobile/android/search/java/org/mozilla/search/autocomplete/SearchBar.java
--- a/mobile/android/app/build.gradle
+++ b/mobile/android/app/build.gradle
@@ -224,17 +224,16 @@ android {
 
 dependencies {
     compile "com.android.support:support-v4:${mozconfig.substs.ANDROID_SUPPORT_LIBRARY_VERSION}"
     compile "com.android.support:appcompat-v7:${mozconfig.substs.ANDROID_SUPPORT_LIBRARY_VERSION}"
     compile "com.android.support:cardview-v7:${mozconfig.substs.ANDROID_SUPPORT_LIBRARY_VERSION}"
     compile "com.android.support:recyclerview-v7:${mozconfig.substs.ANDROID_SUPPORT_LIBRARY_VERSION}"
     compile "com.android.support:design:${mozconfig.substs.ANDROID_SUPPORT_LIBRARY_VERSION}"
     compile "com.android.support:customtabs:${mozconfig.substs.ANDROID_SUPPORT_LIBRARY_VERSION}"
-    compile "com.android.support:palette-v7:${mozconfig.substs.ANDROID_SUPPORT_LIBRARY_VERSION}"
 
     if (mozconfig.substs.MOZ_NATIVE_DEVICES) {
         compile "com.android.support:mediarouter-v7:${mozconfig.substs.ANDROID_SUPPORT_LIBRARY_VERSION}"
         compile "com.google.android.gms:play-services-basement:${mozconfig.substs.ANDROID_GOOGLE_PLAY_SERVICES_VERSION}"
         compile "com.google.android.gms:play-services-base:${mozconfig.substs.ANDROID_GOOGLE_PLAY_SERVICES_VERSION}"
         compile "com.google.android.gms:play-services-cast:${mozconfig.substs.ANDROID_GOOGLE_PLAY_SERVICES_VERSION}"
     }
 
--- a/mobile/android/base/java/org/mozilla/gecko/icons/processing/ColorProcessor.java
+++ b/mobile/android/base/java/org/mozilla/gecko/icons/processing/ColorProcessor.java
@@ -32,46 +32,18 @@ public class ColorProcessor implements P
         final Bitmap bitmap = response.getBitmap();
 
         final @ColorInt Integer edgeColor = getEdgeColor(bitmap);
         if (edgeColor != null) {
             response.updateColor(edgeColor);
             return;
         }
 
-        if (HardwareUtils.isX86System()) {
-            // (Bug 1318667) We are running into crashes when using the palette library with
-            // specific icons on x86 devices. They take down the whole VM and are not recoverable.
-            // Unfortunately our release icon is triggering this crash. Until we can switch to a
-            // newer version of the support library where this does not happen, we are using our
-            // own slower implementation.
-            extractColorUsingCustomImplementation(response);
-        } else {
-            extractColorUsingPaletteSupportLibrary(response);
-        }
-    }
-
-    private void extractColorUsingPaletteSupportLibrary(final IconResponse response) {
-        try {
-            final Palette palette = Palette.from(response.getBitmap()).generate();
-            response.updateColor(palette.getVibrantColor(DEFAULT_COLOR) & 0x7FFFFFFF);
-        } catch (ArrayIndexOutOfBoundsException e) {
-            // We saw the palette library fail with an ArrayIndexOutOfBoundsException intermittently
-            // in automation. In this case lets just swallow the exception and move on without a
-            // color. This is a valid condition and callers should handle this gracefully (Bug 1318560).
-            Log.e(LOGTAG, "Palette generation failed with ArrayIndexOutOfBoundsException", e);
-
-            response.updateColor(DEFAULT_COLOR);
-        }
-    }
-
-    private void extractColorUsingCustomImplementation(final IconResponse response) {
-        final int dominantColor = BitmapUtils.getDominantColor(response.getBitmap());
-
-        response.updateColor(dominantColor);
+        final @ColorInt int dominantColor = BitmapUtils.getDominantColor(response.getBitmap(), DEFAULT_COLOR);
+        response.updateColor(dominantColor & 0x7FFFFFFF);
     }
 
     /**
      * If a bitmap has a consistent edge colour (i.e. if all the border pixels have the same colour),
      * return that colour.
      * @param bitmap
      * @return The edge colour. null if there is no consistent edge color.
      */
--- a/mobile/android/base/java/org/mozilla/gecko/util/ShortcutUtils.java
+++ b/mobile/android/base/java/org/mozilla/gecko/util/ShortcutUtils.java
@@ -134,17 +134,17 @@ public class ShortcutUtils {
         } else if (aSource.getWidth() >= insetSize || aSource.getHeight() >= insetSize) {
             // Otherwise, if the icon is large enough, just draw it.
             final Rect iconBounds = new Rect(0, 0, size, size);
             canvas.drawBitmap(aSource, null, iconBounds, null);
             return bitmap;
         } else {
             // Otherwise, use the dominant color from the icon +
             // a layer of transparent white to lighten it somewhat.
-            final int color = BitmapUtils.getDominantColor(aSource);
+            final int color = BitmapUtils.getDominantColorCustomImplementation(aSource);
             paint.setColor(color);
             canvas.drawRoundRect(new RectF(kOffset, kOffset, size - kOffset, size - kOffset),
                                            kRadius, kRadius, paint);
             paint.setColor(Color.argb(100, 255, 255, 255));
             canvas.drawRoundRect(new RectF(kOffset, kOffset, size - kOffset, size - kOffset),
                                  kRadius, kRadius, paint);
         }
 
--- a/mobile/android/geckoview/build.gradle
+++ b/mobile/android/geckoview/build.gradle
@@ -99,16 +99,17 @@ android {
             assets {
             }
         }
     }
 }
 
 dependencies {
     compile "com.android.support:support-v4:${mozconfig.substs.ANDROID_SUPPORT_LIBRARY_VERSION}"
+    compile "com.android.support:palette-v7:${mozconfig.substs.ANDROID_SUPPORT_LIBRARY_VERSION}"
 }
 
 task syncPreprocessedCode(type: Sync, dependsOn: rootProject.generateCodeAndResources) {
     into("${project.buildDir}/generated/source/preprocessed_code")
     from("${topobjdir}/mobile/android/base/generated/preprocessed") {
         // These constants files are included in the main app project.
         exclude '**/AdjustConstants.java'
         exclude '**/MmaConstants.java'
--- a/mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/BitmapUtils.java
+++ b/mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/BitmapUtils.java
@@ -14,19 +14,23 @@ import android.content.Context;
 import android.content.res.Resources;
 import android.graphics.Bitmap;
 import android.graphics.BitmapFactory;
 import android.graphics.Canvas;
 import android.graphics.Color;
 import android.graphics.drawable.BitmapDrawable;
 import android.graphics.drawable.Drawable;
 import android.net.Uri;
+import android.support.annotation.ColorInt;
+import android.support.v7.graphics.Palette;
 import android.util.Base64;
 import android.util.Log;
 
+import org.mozilla.gecko.util.HardwareUtils;
+
 public final class BitmapUtils {
     private static final String LOGTAG = "GeckoBitmapUtils";
 
     private BitmapUtils() {}
 
     public static Bitmap decodeByteArray(byte[] bytes) {
         return decodeByteArray(bytes, null);
     }
@@ -130,23 +134,49 @@ public final class BitmapUtils {
         try {
             return BitmapFactory.decodeResource(resources, id, options);
         } catch (OutOfMemoryError e) {
             Log.e(LOGTAG, "decodeResource() OOM! Resource id=" + id, e);
             return null;
         }
     }
 
-    public static int getDominantColor(Bitmap source) {
-        return getDominantColor(source, true);
+    public static @ColorInt int getDominantColor(Bitmap source, @ColorInt int defaultColor) {
+        if (HardwareUtils.isX86System()) {
+            // (Bug 1318667) We are running into crashes when using the palette library with
+            // specific icons on x86 devices. They take down the whole VM and are not recoverable.
+            // Unfortunately our release icon is triggering this crash. Until we can switch to a
+            // newer version of the support library where this does not happen, we are using our
+            // own slower implementation.
+            return getDominantColorCustomImplementation(source, true, defaultColor);
+        } else {
+            try {
+                final Palette palette = Palette.from(source).generate();
+                return palette.getVibrantColor(defaultColor);
+            } catch (ArrayIndexOutOfBoundsException e) {
+                // We saw the palette library fail with an ArrayIndexOutOfBoundsException intermittently
+                // in automation. In this case lets just swallow the exception and move on without a
+                // color. This is a valid condition and callers should handle this gracefully (Bug 1318560).
+                Log.e(LOGTAG, "Palette generation failed with ArrayIndexOutOfBoundsException", e);
+
+                return defaultColor;
+            }
+        }
     }
 
-    public static int getDominantColor(Bitmap source, boolean applyThreshold) {
-      if (source == null)
-        return Color.argb(255, 255, 255, 255);
+    public static @ColorInt int getDominantColorCustomImplementation(Bitmap source) {
+        return getDominantColorCustomImplementation(source, true, Color.WHITE);
+    }
+
+    public static @ColorInt int getDominantColorCustomImplementation(Bitmap source,
+                                                                     boolean applyThreshold,
+                                                                     @ColorInt int defaultColor) {
+      if (source == null) {
+          return defaultColor;
+      }
 
       // Keep track of how many times a hue in a given bin appears in the image.
       // Hue values range [0 .. 360), so dividing by 10, we get 36 bins.
       int[] colorBins = new int[36];
 
       // The bin with the most colors. Initialize to -1 to prevent accidentally
       // thinking the first bin holds the dominant color.
       int maxBin = -1;
@@ -188,18 +218,19 @@ public final class BitmapUtils {
 
           // Keep track of the bin that holds the most colors.
           if (maxBin < 0 || colorBins[bin] > colorBins[maxBin])
             maxBin = bin;
         }
       }
 
       // maxBin may never get updated if the image holds only transparent and/or black/white pixels.
-      if (maxBin < 0)
-        return Color.argb(255, 255, 255, 255);
+      if (maxBin < 0) {
+          return defaultColor;
+      }
 
       // Return a color with the average hue/saturation/value of the bin with the most colors.
       hsv[0] = sumHue[maxBin] / colorBins[maxBin];
       hsv[1] = sumSat[maxBin] / colorBins[maxBin];
       hsv[2] = sumVal[maxBin] / colorBins[maxBin];
       return Color.HSVToColor(hsv);
     }
 
--- a/mobile/android/search/java/org/mozilla/search/autocomplete/SearchBar.java
+++ b/mobile/android/search/java/org/mozilla/search/autocomplete/SearchBar.java
@@ -134,21 +134,21 @@ public class SearchBar extends FrameLayo
     public void setEngine(SearchEngine engine) {
         final String iconURL = engine.getIconURL();
         final Bitmap bitmap = BitmapUtils.getBitmapFromDataURI(iconURL);
         final BitmapDrawable d = new BitmapDrawable(getResources(), bitmap);
         engineIcon.setImageDrawable(d);
         engineIcon.setContentDescription(engine.getName());
 
         // Update the focused background color.
-        int color = BitmapUtils.getDominantColor(bitmap);
+        int color = BitmapUtils.getDominantColorCustomImplementation(bitmap);
 
-        // BitmapUtils#getDominantColor ignores black and white pixels, but it will
-        // return white if no dominant color was found. We don't want to create a
-        // white underline for the search bar, so we default to black instead.
+        // BitmapUtils#getDominantColorCustomImplementation ignores black and white pixels,
+        // but it will return white if no dominant color was found. We don't want to create
+        // a white underline for the search bar, so we default to black instead.
         if (color == Color.WHITE) {
             color = Color.BLACK;
         }
         focusedBackground.setColorFilter(new PorterDuffColorFilter(color, PorterDuff.Mode.MULTIPLY));
 
         editText.setHint(getResources().getString(R.string.search_bar_hint, engine.getName()));
     }