From 8584b450f3af6c18e0393a3bff015a62a08d5eb5 Mon Sep 17 00:00:00 2001 From: Sam Judd Date: Fri, 11 Sep 2020 12:41:27 -0700 Subject: [PATCH] Wait for the app to render at least one frame before enabling Hardware 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: 331202490 --- .../EmptyAppGlideModuleTest/GlideApp.java | 9 ------ .../LoadResourcesWithDownsamplerTest.java | 1 - .../main/java/com/bumptech/glide/Glide.java | 15 +-------- .../java/com/bumptech/glide/GlideBuilder.java | 9 +++++- .../resource/bitmap/HardwareConfigState.java | 20 ++---------- .../manager/RequestManagerRetriever.java | 32 ++++++++----------- .../bitmap/HardwareConfigStateTest.java | 31 ------------------ .../manager/RequestManagerRetrieverTest.java | 2 +- 8 files changed, 26 insertions(+), 93 deletions(-) diff --git a/annotation/compiler/test/src/test/resources/EmptyAppGlideModuleTest/GlideApp.java b/annotation/compiler/test/src/test/resources/EmptyAppGlideModuleTest/GlideApp.java index 2b18540210..a4daa3be3b 100644 --- a/annotation/compiler/test/src/test/resources/EmptyAppGlideModuleTest/GlideApp.java +++ b/annotation/compiler/test/src/test/resources/EmptyAppGlideModuleTest/GlideApp.java @@ -69,15 +69,6 @@ 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/instrumentation/src/androidTest/java/com/bumptech/glide/LoadResourcesWithDownsamplerTest.java b/instrumentation/src/androidTest/java/com/bumptech/glide/LoadResourcesWithDownsamplerTest.java index 2c023b3755..1da10d6cb6 100644 --- a/instrumentation/src/androidTest/java/com/bumptech/glide/LoadResourcesWithDownsamplerTest.java +++ b/instrumentation/src/androidTest/java/com/bumptech/glide/LoadResourcesWithDownsamplerTest.java @@ -121,7 +121,6 @@ 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 index 8e9160fc6d..554e7b37eb 100644 --- a/library/src/main/java/com/bumptech/glide/Glide.java +++ b/library/src/main/java/com/bumptech/glide/Glide.java @@ -63,7 +63,6 @@ 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; @@ -236,19 +235,6 @@ 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. - * - *

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) { @@ -387,6 +373,7 @@ private static void throwIncorrectGlideModule(Exception e) { @NonNull List> 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 index 46aee07786..50c774e5d0 100644 --- a/library/src/main/java/com/bumptech/glide/GlideBuilder.java +++ b/library/src/main/java/com/bumptech/glide/GlideBuilder.java @@ -66,7 +66,9 @@ 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 @@ -561,7 +563,11 @@ Glide build(@NonNull Context context) { } RequestManagerRetriever requestManagerRetriever = - new RequestManagerRetriever(requestManagerFactory); + new RequestManagerRetriever( + requestManagerFactory, waitForFirstFrameBeforeEnablingHardwareBitmaps); + + boolean blockHardwareBitmaps = + waitForFirstFrameBeforeEnablingHardwareBitmaps || waitForCallBeforeEnablingHardwareBitmaps; return new Glide( context, @@ -577,6 +583,7 @@ 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 index eef183a4b4..d7e62c5ba0 100644 --- 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,15 +15,6 @@ * 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. @@ -75,6 +66,7 @@ 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; @@ -112,10 +104,6 @@ public static HardwareConfigState getInstance() { } } - public void unblockHardwareBitmaps() { - areHardwareBitmapsUnblocked = true; - } - public boolean isHardwareConfigAllowed( int targetWidth, int targetHeight, @@ -124,7 +112,7 @@ public boolean isHardwareConfigAllowed( if (!isHardwareConfigAllowed || !isHardwareConfigAllowedByDeviceModel || Build.VERSION.SDK_INT < Build.VERSION_CODES.O - || areHardwareBitmapsBlockedByAppState() + || (blockHardwareBitmapsByDefault && !areHardwareBitmapsUnblocked) || isExifOrientationRequired) { return false; } @@ -135,10 +123,6 @@ 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 index fe747a7c0b..0a4c02f108 100644 --- a/library/src/main/java/com/bumptech/glide/manager/RequestManagerRetriever.java +++ b/library/src/main/java/com/bumptech/glide/manager/RequestManagerRetriever.java @@ -70,10 +70,12 @@ 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. - private final FirstFrameWaiter firstFrameWaiter = new FirstFrameWaiter(); + @Nullable private final FirstFrameWaiter firstFrameWaiter; - public RequestManagerRetriever(@Nullable RequestManagerFactory factory) { + public RequestManagerRetriever( + @Nullable RequestManagerFactory factory, boolean addFirstFrameWaiter) { this.factory = factory != null ? factory : DEFAULT_FACTORY; + firstFrameWaiter = addFirstFrameWaiter ? new FirstFrameWaiter() : null; handler = new Handler(Looper.getMainLooper(), this /* Callback */); } @@ -124,13 +126,19 @@ 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); - firstFrameWaiter.registerSelf(activity); + maybeRegisterFirstFrameWaiter(activity); FragmentManager fm = activity.getSupportFragmentManager(); return supportFragmentGet(activity, fm, /*parentHint=*/ null, isActivityVisible(activity)); } @@ -144,13 +152,7 @@ public RequestManager get(@NonNull Fragment fragment) { if (Util.isOnBackgroundThread()) { return get(fragment.getContext().getApplicationContext()); } else { - // 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()); - } + maybeRegisterFirstFrameWaiter(fragment.getActivity()); FragmentManager fm = fragment.getChildFragmentManager(); return supportFragmentGet(fragment.getContext(), fm, fragment, fragment.isVisible()); } @@ -165,7 +167,7 @@ public RequestManager get(@NonNull Activity activity) { return get((FragmentActivity) activity); } else { assertNotDestroyed(activity); - firstFrameWaiter.registerSelf(activity); + maybeRegisterFirstFrameWaiter(activity); android.app.FragmentManager fm = activity.getFragmentManager(); return fragmentGet(activity, fm, /*parentHint=*/ null, isActivityVisible(activity)); } @@ -345,13 +347,7 @@ 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 { - // 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()); - } + maybeRegisterFirstFrameWaiter(fragment.getActivity()); android.app.FragmentManager fm = fragment.getChildFragmentManager(); return fragmentGet(fragment.getActivity(), fm, fragment, fragment.isVisible()); } diff --git a/library/test/src/test/java/com/bumptech/glide/load/resource/bitmap/HardwareConfigStateTest.java b/library/test/src/test/java/com/bumptech/glide/load/resource/bitmap/HardwareConfigStateTest.java index aca894e069..39fd47ff8e 100644 --- a/library/test/src/test/java/com/bumptech/glide/load/resource/bitmap/HardwareConfigStateTest.java +++ b/library/test/src/test/java/com/bumptech/glide/load/resource/bitmap/HardwareConfigStateTest.java @@ -21,7 +21,6 @@ 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( @@ -40,7 +39,6 @@ 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; @@ -62,7 +60,6 @@ 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; @@ -85,7 +82,6 @@ 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; @@ -108,7 +104,6 @@ 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; @@ -130,29 +125,6 @@ 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; @@ -180,7 +152,6 @@ 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; @@ -208,7 +179,6 @@ 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; @@ -236,7 +206,6 @@ 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 index e2ffbcfab3..1fb865b5f9 100644 --- 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); + retriever = new RequestManagerRetriever(/*factory=*/ null, /* addFirstFrameWaiter= */ false); harnesses = new RetrieverHarness[] {new DefaultRetrieverHarness(), new SupportRetrieverHarness()};