Wait for the app to render at least one frame before enabling Hardwar…

…e bitmaps. There appears to be a bug on versions of Android < Q where trying to create a hardware bitmap before the application has rendered a single frame causes a native crash. This works around the issue by making sure Glide only uses regular Bitmaps until the application has drawn at least one frame. PiperOrigin-RevId: 330840252
bumptech · Sep 10, 2020 · edd96d8 · edd96d8
1 parent e7d16d7
commit edd96d8
Show file tree

Hide file tree

Showing 8 changed files with 93 additions and 26 deletions.
diff --git a/annotation/compiler/test/src/test/resources/EmptyAppGlideModuleTest/GlideApp.java b/annotation/compiler/test/src/test/resources/EmptyAppGlideModuleTest/GlideApp.java
@@ -69,6 +69,15 @@ public static void init(@NonNull Context context, @NonNull GlideBuilder builder)
     Glide.init(context, builder);
   }
 
+  /**
+   * @see Glide#enableHardwareBitmaps()
+   */
+  @VisibleForTesting
+  @SuppressLint("VisibleForTests")
+  public static void enableHardwareBitmaps() {
+    Glide.enableHardwareBitmaps();
+  }
+
   /**
    * @see Glide#tearDown()
    */

diff --git a/...rumentation/src/androidTest/java/com/bumptech/glide/LoadResourcesWithDownsamplerTest.java b/...rumentation/src/androidTest/java/com/bumptech/glide/LoadResourcesWithDownsamplerTest.java
@@ -121,6 +121,7 @@ public void loadTransparentGifResource_asHardware_withNoOtherLoaders_decodesReso
     assumeTrue(
         "Hardware Bitmaps are only supported on O+",
         Build.VERSION.SDK_INT >= Build.VERSION_CODES.O);
+    Glide.enableHardwareBitmaps();
 
     Glide.get(context)
         .getRegistry()

diff --git a/library/src/main/java/com/bumptech/glide/Glide.java b/library/src/main/java/com/bumptech/glide/Glide.java
@@ -63,6 +63,7 @@
 import com.bumptech.glide.load.resource.bitmap.DefaultImageHeaderParser;
 import com.bumptech.glide.load.resource.bitmap.Downsampler;
 import com.bumptech.glide.load.resource.bitmap.ExifInterfaceImageHeaderParser;
+import com.bumptech.glide.load.resource.bitmap.HardwareConfigState;
 import com.bumptech.glide.load.resource.bitmap.InputStreamBitmapImageDecoderResourceDecoder;
 import com.bumptech.glide.load.resource.bitmap.ParcelFileDescriptorBitmapDecoder;
 import com.bumptech.glide.load.resource.bitmap.ResourceBitmapDecoder;
@@ -235,6 +236,19 @@ public static void init(@NonNull Context context, @NonNull GlideBuilder builder)
     }
   }
 
+  /**
+   * Allows hardware Bitmaps to be used prior to the first frame in the app being drawn as soon as
+   * this method is called.
+   *
+   * <p>If you use this method in non-test code, your app will experience native crashes on some
+   * versions of Android if you try to decode a hardware Bitmap. This method is only useful for
+   * testing.
+   */
+  @VisibleForTesting
+  public static void enableHardwareBitmaps() {
+    HardwareConfigState.getInstance().unblockHardwareBitmaps();
+  }
+
   @VisibleForTesting
   public static void tearDown() {
     synchronized (Glide.class) {
@@ -373,7 +387,6 @@ private static void throwIncorrectGlideModule(Exception e) {
       @NonNull List<RequestListener<Object>> defaultRequestListeners,
       boolean isLoggingRequestOriginsEnabled,
       boolean isImageDecoderEnabledForBitmaps,
-      boolean blockHardwareBitmaps,
       int manualOverrideHardwareBitmapMaxFdCount) {
     this.engine = engine;
     this.bitmapPool = bitmapPool;

diff --git a/library/src/main/java/com/bumptech/glide/GlideBuilder.java b/library/src/main/java/com/bumptech/glide/GlideBuilder.java
@@ -66,9 +66,7 @@ public RequestOptions build() {
   private boolean isLoggingRequestOriginsEnabled;
 
   private boolean isImageDecoderEnabledForBitmaps;
-  private boolean waitForFirstFrameBeforeEnablingHardwareBitmaps;
   private int manualOverrideHardwareBitmapMaxFdCount = HardwareConfigState.NO_MAX_FD_COUNT;
-  private boolean waitForCallBeforeEnablingHardwareBitmaps;
 
   /**
    * Sets the {@link com.bumptech.glide.load.engine.bitmap_recycle.BitmapPool} implementation to use
@@ -563,11 +561,7 @@ Glide build(@NonNull Context context) {
     }
 
     RequestManagerRetriever requestManagerRetriever =
-        new RequestManagerRetriever(
-            requestManagerFactory, waitForFirstFrameBeforeEnablingHardwareBitmaps);
-
-    boolean blockHardwareBitmaps =
-        waitForFirstFrameBeforeEnablingHardwareBitmaps || waitForCallBeforeEnablingHardwareBitmaps;
+        new RequestManagerRetriever(requestManagerFactory);
 
     return new Glide(
         context,
@@ -583,7 +577,6 @@ Glide build(@NonNull Context context) {
         defaultRequestListeners,
         isLoggingRequestOriginsEnabled,
         isImageDecoderEnabledForBitmaps,
-        blockHardwareBitmaps,
         manualOverrideHardwareBitmapMaxFdCount);
   }
 }
diff --git a/library/src/main/java/com/bumptech/glide/load/resource/bitmap/HardwareConfigState.java b/library/src/main/java/com/bumptech/glide/load/resource/bitmap/HardwareConfigState.java
@@ -15,6 +15,15 @@
  * Android O+.
  */
 public final class HardwareConfigState {
+  /**
+   * Force the state to wait until a call to allow hardware Bitmaps to be used when they'd otherwise
+   * be eligible to work around a framework issue pre Q that can cause a native crash when
+   * allocating a hardware Bitmap in this specific circumstance. See b/126573603#comment12 for
+   * details.
+   */
+  private static final boolean BLOCK_HARDWARE_BITMAPS_BY_DEFAULT =
+      Build.VERSION.SDK_INT < Build.VERSION_CODES.Q;
+
   /**
    * The minimum size in pixels a {@link Bitmap} must be in both dimensions to be created with the
    * {@link Bitmap.Config#HARDWARE} configuration.
@@ -66,7 +75,6 @@ public final class HardwareConfigState {
   public static final int NO_MAX_FD_COUNT = -1;
 
   private static volatile HardwareConfigState instance;
-  private static volatile boolean blockHardwareBitmapsByDefault;
   private static volatile int manualOverrideMaxFdCount = NO_MAX_FD_COUNT;
 
   private final boolean isHardwareConfigAllowedByDeviceModel;
@@ -104,6 +112,10 @@ public static HardwareConfigState getInstance() {
     }
   }
 
+  public void unblockHardwareBitmaps() {
+    areHardwareBitmapsUnblocked = true;
+  }
+
   public boolean isHardwareConfigAllowed(
       int targetWidth,
       int targetHeight,
@@ -112,7 +124,7 @@ public boolean isHardwareConfigAllowed(
     if (!isHardwareConfigAllowed
         || !isHardwareConfigAllowedByDeviceModel
         || Build.VERSION.SDK_INT < Build.VERSION_CODES.O
-        || (blockHardwareBitmapsByDefault && !areHardwareBitmapsUnblocked)
+        || areHardwareBitmapsBlockedByAppState()
         || isExifOrientationRequired) {
       return false;
     }
@@ -123,6 +135,10 @@ public boolean isHardwareConfigAllowed(
         && isFdSizeBelowHardwareLimit();
   }
 
+  private boolean areHardwareBitmapsBlockedByAppState() {
+    return BLOCK_HARDWARE_BITMAPS_BY_DEFAULT && !areHardwareBitmapsUnblocked;
+  }
+
   @TargetApi(Build.VERSION_CODES.O)
   boolean setHardwareConfigIfAllowed(
       int targetWidth,

diff --git a/library/src/main/java/com/bumptech/glide/manager/RequestManagerRetriever.java b/library/src/main/java/com/bumptech/glide/manager/RequestManagerRetriever.java
@@ -70,12 +70,10 @@ public class RequestManagerRetriever implements Handler.Callback {
   // This is really misplaced here, but to put it anywhere else means duplicating all of the
   // Fragment/Activity extraction logic that already exists here. It's gross, but less likely to
   // break.
-  @Nullable private final FirstFrameWaiter firstFrameWaiter;
+  private final FirstFrameWaiter firstFrameWaiter = new FirstFrameWaiter();
 
-  public RequestManagerRetriever(
-      @Nullable RequestManagerFactory factory, boolean addFirstFrameWaiter) {
+  public RequestManagerRetriever(@Nullable RequestManagerFactory factory) {
     this.factory = factory != null ? factory : DEFAULT_FACTORY;
-    firstFrameWaiter = addFirstFrameWaiter ? new FirstFrameWaiter() : null;
     handler = new Handler(Looper.getMainLooper(), this /* Callback */);
   }
 
@@ -126,19 +124,13 @@ public RequestManager get(@NonNull Context context) {
     return getApplicationManager(context);
   }
 
-  private void maybeRegisterFirstFrameWaiter(@NonNull Activity activity) {
-    if (firstFrameWaiter != null) {
-      firstFrameWaiter.registerSelf(activity);
-    }
-  }
-
   @NonNull
   public RequestManager get(@NonNull FragmentActivity activity) {
     if (Util.isOnBackgroundThread()) {
       return get(activity.getApplicationContext());
     } else {
       assertNotDestroyed(activity);
-      maybeRegisterFirstFrameWaiter(activity);
+      firstFrameWaiter.registerSelf(activity);
       FragmentManager fm = activity.getSupportFragmentManager();
       return supportFragmentGet(activity, fm, /*parentHint=*/ null, isActivityVisible(activity));
     }
@@ -152,7 +144,13 @@ public RequestManager get(@NonNull Fragment fragment) {
     if (Util.isOnBackgroundThread()) {
       return get(fragment.getContext().getApplicationContext());
     } else {
-      maybeRegisterFirstFrameWaiter(fragment.getActivity());
+      // In some unusual cases, it's possible to have a Fragment not hosted by an activity. There's
+      // not all that much we can do here. Most apps will be started with a standard activity. If
+      // we manage not to register the first frame waiter for a while, the consequences are not
+      // catastrophic, we'll just use some extra memory.
+      if (fragment.getActivity() != null) {
+        firstFrameWaiter.registerSelf(fragment.getActivity());
+      }
       FragmentManager fm = fragment.getChildFragmentManager();
       return supportFragmentGet(fragment.getContext(), fm, fragment, fragment.isVisible());
     }
@@ -167,7 +165,7 @@ public RequestManager get(@NonNull Activity activity) {
       return get((FragmentActivity) activity);
     } else {
       assertNotDestroyed(activity);
-      maybeRegisterFirstFrameWaiter(activity);
+      firstFrameWaiter.registerSelf(activity);
       android.app.FragmentManager fm = activity.getFragmentManager();
       return fragmentGet(activity, fm, /*parentHint=*/ null, isActivityVisible(activity));
     }
@@ -347,7 +345,13 @@ public RequestManager get(@NonNull android.app.Fragment fragment) {
     if (Util.isOnBackgroundThread() || Build.VERSION.SDK_INT < Build.VERSION_CODES.JELLY_BEAN_MR1) {
       return get(fragment.getActivity().getApplicationContext());
     } else {
-      maybeRegisterFirstFrameWaiter(fragment.getActivity());
+      // In some unusual cases, it's possible to have a Fragment not hosted by an activity. There's
+      // not all that much we can do here. Most apps will be started with a standard activity. If
+      // we manage not to register the first frame waiter for a while, the consequences are not
+      // catastrophic, we'll just use some extra memory.
+      if (fragment.getActivity() != null) {
+        firstFrameWaiter.registerSelf(fragment.getActivity());
+      }
       android.app.FragmentManager fm = fragment.getChildFragmentManager();
       return fragmentGet(fragment.getActivity(), fm, fragment, fragment.isVisible());
     }

diff --git a/...y/test/src/test/java/com/bumptech/glide/load/resource/bitmap/HardwareConfigStateTest.java b/...y/test/src/test/java/com/bumptech/glide/load/resource/bitmap/HardwareConfigStateTest.java
@@ -21,6 +21,7 @@ public class HardwareConfigStateTest {
   public void
       setHardwareConfigIfAllowed_withAllowedState_setsInPreferredConfigAndMutable_returnsFalse() {
     HardwareConfigState state = new HardwareConfigState();
+    state.unblockHardwareBitmaps();
     BitmapFactory.Options options = new BitmapFactory.Options();
     boolean result =
         state.setHardwareConfigIfAllowed(
@@ -39,6 +40,7 @@ public class HardwareConfigStateTest {
   @Test
   public void setHardwareConfigIfAllowed_withSmallerThanMinWidth_returnsFalse_doesNotSetValues() {
     HardwareConfigState state = new HardwareConfigState();
+    state.unblockHardwareBitmaps();
     BitmapFactory.Options options = new BitmapFactory.Options();
     options.inPreferredConfig = null;
     options.inMutable = true;
@@ -60,6 +62,7 @@ public void setHardwareConfigIfAllowed_withSmallerThanMinWidth_returnsFalse_does
   @Test
   public void setHardwareConfigIfAllowed_withSmallerThanMinHeight_returnsFalse_doesNotSetValues() {
     HardwareConfigState state = new HardwareConfigState();
+    state.unblockHardwareBitmaps();
     BitmapFactory.Options options = new BitmapFactory.Options();
     options.inPreferredConfig = null;
     options.inMutable = true;
@@ -82,6 +85,7 @@ public void setHardwareConfigIfAllowed_withSmallerThanMinHeight_returnsFalse_doe
   public void
       setHardwareConfigIfAllowed_withHardwareConfigDisallowed_returnsFalse_doesNotSetValues() {
     HardwareConfigState state = new HardwareConfigState();
+    state.unblockHardwareBitmaps();
     BitmapFactory.Options options = new BitmapFactory.Options();
     options.inPreferredConfig = null;
     options.inMutable = true;
@@ -104,6 +108,7 @@ public void setHardwareConfigIfAllowed_withSmallerThanMinHeight_returnsFalse_doe
   public void
       setHardwareConfigIfAllowed_withExifOrientationRequired_returnsFalse_doesNotSetValues() {
     HardwareConfigState state = new HardwareConfigState();
+    state.unblockHardwareBitmaps();
     BitmapFactory.Options options = new BitmapFactory.Options();
     options.inPreferredConfig = null;
     options.inMutable = true;
@@ -125,6 +130,29 @@ public void setHardwareConfigIfAllowed_withSmallerThanMinHeight_returnsFalse_doe
   @Test
   public void setHardwareConfigIfAllowed_withOsLessThanO_returnsFalse_doesNotSetValues() {
     HardwareConfigState state = new HardwareConfigState();
+    state.unblockHardwareBitmaps();
+    BitmapFactory.Options options = new BitmapFactory.Options();
+    options.inPreferredConfig = null;
+    options.inMutable = true;
+
+    boolean result =
+        state.setHardwareConfigIfAllowed(
+            /*targetWidth=*/ HardwareConfigState.MIN_HARDWARE_DIMENSION_O,
+            /*targetHeight=*/ HardwareConfigState.MIN_HARDWARE_DIMENSION_O,
+            options,
+            /*isHardwareConfigAllowed=*/ true,
+            /*isExifOrientationRequired=*/ false);
+
+    assertThat(result).isFalse();
+    assertThat(options.inMutable).isTrue();
+    assertThat(options.inPreferredConfig).isNull();
+  }
+
+  @Config(sdk = Build.VERSION_CODES.P)
+  @Test
+  public void
+      setHardwareConfigIfAllowed_withOsLessThanQ_beforeUnblockingHardwareBitmaps_returnsFalseAndDoesNotSetValues() {
+    HardwareConfigState state = new HardwareConfigState();
     BitmapFactory.Options options = new BitmapFactory.Options();
     options.inPreferredConfig = null;
     options.inMutable = true;
@@ -152,6 +180,7 @@ public void setHardwareConfigIfAllowed_withOsLessThanO_returnsFalse_doesNotSetVa
         }) {
       ShadowBuild.setModel(model);
       HardwareConfigState state = new HardwareConfigState();
+      state.unblockHardwareBitmaps();
       BitmapFactory.Options options = new BitmapFactory.Options();
       options.inPreferredConfig = null;
       options.inMutable = true;
@@ -179,6 +208,7 @@ public void setHardwareConfigIfAllowed_withDisallowedSamsungDevices_OMR1_returns
         }) {
       ShadowBuild.setModel(model);
       HardwareConfigState state = new HardwareConfigState();
+      state.unblockHardwareBitmaps();
       BitmapFactory.Options options = new BitmapFactory.Options();
       options.inPreferredConfig = null;
       options.inMutable = true;
@@ -206,6 +236,7 @@ public void setHardwareConfigIfAllowed_withShortEmptyOrNullModelNames_returnsTru
         new String[] {null, ".", "-", "", "S", "SM", "SM-", "SM-G", "SM-G9.", "SM-G93"}) {
       ShadowBuild.setModel(model);
       HardwareConfigState state = new HardwareConfigState();
+      state.unblockHardwareBitmaps();
       BitmapFactory.Options options = new BitmapFactory.Options();
       options.inPreferredConfig = null;
       options.inMutable = true;

diff --git a/library/test/src/test/java/com/bumptech/glide/manager/RequestManagerRetrieverTest.java b/library/test/src/test/java/com/bumptech/glide/manager/RequestManagerRetrieverTest.java
@@ -58,7 +58,7 @@ public class RequestManagerRetrieverTest {
   public void setUp() {
     appContext = ApplicationProvider.getApplicationContext();
 
-    retriever = new RequestManagerRetriever(/*factory=*/ null, /* addFirstFrameWaiter= */ false);
+    retriever = new RequestManagerRetriever(/*factory=*/ null);
 
     harnesses =
         new RetrieverHarness[] {new DefaultRetrieverHarness(), new SupportRetrieverHarness()};