From b0a7efecf54723506bcaac2d801e29845f04f229 Mon Sep 17 00:00:00 2001 From: Sam Judd Date: Mon, 19 Oct 2020 14:01:31 -0700 Subject: [PATCH] Add a method of frame waiting that resets when we get certain trim memory callbacks. The GL context may be destroyed on TRIM_MEMORY_MODERATE or higher. We could strictly listen to that callback, but we'd end up racing because we only get trimMemory on the UI thread and decodes happen on multiple background threads. Hopefully by resetting when the app is backgrounded we can minimize those races. PiperOrigin-RevId: 337927814 --- .../LoadResourcesWithDownsamplerTest.java | 18 +++- .../java/com/bumptech/glide/GlideBuilder.java | 16 ++-- .../resource/bitmap/HardwareConfigState.java | 84 +++++++++++++++---- .../manager/DoNothingFirstFrameWaiter.java | 2 +- .../FirstFrameAndAfterTrimMemoryWaiter.java | 24 ++++++ .../glide/manager/FirstFrameWaiter.java | 9 +- .../glide/manager/FirstFrameWaiterO.java | 12 --- .../bumptech/glide/manager/FrameWaiter.java | 7 ++ .../manager/RequestManagerRetriever.java | 29 +++++-- .../bitmap/HardwareConfigStateTest.java | 23 ++++- .../manager/RequestManagerRetrieverTest.java | 3 +- 11 files changed, 179 insertions(+), 48 deletions(-) create mode 100644 library/src/main/java/com/bumptech/glide/manager/FirstFrameAndAfterTrimMemoryWaiter.java delete mode 100644 library/src/main/java/com/bumptech/glide/manager/FirstFrameWaiterO.java create mode 100644 library/src/main/java/com/bumptech/glide/manager/FrameWaiter.java diff --git a/instrumentation/src/androidTest/java/com/bumptech/glide/LoadResourcesWithDownsamplerTest.java b/instrumentation/src/androidTest/java/com/bumptech/glide/LoadResourcesWithDownsamplerTest.java index 2c023b3755..8c68b91a63 100644 --- a/instrumentation/src/androidTest/java/com/bumptech/glide/LoadResourcesWithDownsamplerTest.java +++ b/instrumentation/src/androidTest/java/com/bumptech/glide/LoadResourcesWithDownsamplerTest.java @@ -28,11 +28,14 @@ import com.bumptech.glide.test.GlideApp; import com.bumptech.glide.test.ResourceIds; import com.bumptech.glide.test.TearDownGlide; +import com.bumptech.glide.util.Util; import java.io.ByteArrayOutputStream; import java.io.FileNotFoundException; import java.io.IOException; import java.io.InputStream; import java.util.Locale; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; import org.junit.Rule; import org.junit.Test; import org.junit.runner.RunWith; @@ -117,11 +120,22 @@ public void loadTransparentGifResource_withNoOtherLoaders_decodesResource() { } @Test - public void loadTransparentGifResource_asHardware_withNoOtherLoaders_decodesResource() { + public void loadTransparentGifResource_asHardware_withNoOtherLoaders_decodesResource() + throws InterruptedException { assumeTrue( "Hardware Bitmaps are only supported on O+", Build.VERSION.SDK_INT >= Build.VERSION_CODES.O); - Glide.enableHardwareBitmaps(); + // enableHardwareBitmaps must be called on the main thread. + final CountDownLatch latch = new CountDownLatch(1); + Util.postOnUiThread( + new Runnable() { + @Override + public void run() { + Glide.enableHardwareBitmaps(); + latch.countDown(); + } + }); + latch.await(5, TimeUnit.SECONDS); Glide.get(context) .getRegistry() diff --git a/library/src/main/java/com/bumptech/glide/GlideBuilder.java b/library/src/main/java/com/bumptech/glide/GlideBuilder.java index aa523c647b..41373766ba 100644 --- a/library/src/main/java/com/bumptech/glide/GlideBuilder.java +++ b/library/src/main/java/com/bumptech/glide/GlideBuilder.java @@ -41,7 +41,7 @@ @SuppressWarnings("PMD.ImmutableField") public final class GlideBuilder { private final Map, TransitionOptions> defaultTransitionOptions = new ArrayMap<>(); - private final GlideExperiments.Builder glideExperiments = new GlideExperiments.Builder(); + private final GlideExperiments.Builder glideExperimentsBuilder = new GlideExperiments.Builder(); private Engine engine; private BitmapPool bitmapPool; private ArrayPool arrayPool; @@ -448,7 +448,7 @@ public GlideBuilder addGlobalRequestListener(@NonNull RequestListener li *

This is an experimental API that may be removed in the future. */ public GlideBuilder setLogRequestOrigins(boolean isEnabled) { - glideExperiments.update(new LogRequestOrigins(), isEnabled); + glideExperimentsBuilder.update(new LogRequestOrigins(), isEnabled); return this; } @@ -479,7 +479,7 @@ public GlideBuilder setLogRequestOrigins(boolean isEnabled) { * which may not agree. */ public GlideBuilder setImageDecoderEnabledForBitmaps(boolean isEnabled) { - glideExperiments.update( + glideExperimentsBuilder.update( new EnableImageDecoderForBitmaps(), /*isEnabled=*/ isEnabled && Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q); return this; @@ -556,8 +556,9 @@ Glide build(@NonNull Context context) { defaultRequestListeners = Collections.unmodifiableList(defaultRequestListeners); } + GlideExperiments experiments = glideExperimentsBuilder.build(); RequestManagerRetriever requestManagerRetriever = - new RequestManagerRetriever(requestManagerFactory); + new RequestManagerRetriever(requestManagerFactory, experiments); return new Glide( context, @@ -571,7 +572,7 @@ Glide build(@NonNull Context context) { defaultRequestOptionsFactory, defaultTransitionOptions, defaultRequestListeners, - glideExperiments.build()); + experiments); } static final class ManualOverrideHardwareBitmapMaxFdCount implements Experiment { @@ -583,6 +584,11 @@ static final class ManualOverrideHardwareBitmapMaxFdCount implements Experiment } } + /** See {@link #setWaitForFramesAfterTrimMemory(boolean)}. */ + public static final class WaitForFramesAfterTrimMemory implements Experiment { + private WaitForFramesAfterTrimMemory() {} + } + static final class EnableImageDecoderForBitmaps implements Experiment {} /** See {@link #setLogRequestOrigins(boolean)}. */ 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..6eaa2ddaa4 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 @@ -7,21 +7,25 @@ import android.util.Log; import androidx.annotation.GuardedBy; import androidx.annotation.VisibleForTesting; +import com.bumptech.glide.util.Util; import java.io.File; import java.util.Arrays; +import java.util.concurrent.atomic.AtomicBoolean; /** * State and constants for interacting with {@link android.graphics.Bitmap.Config#HARDWARE} on * Android O+. */ public final class HardwareConfigState { + private static final String TAG = "HardwareConfig"; + /** * 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 = + private static final boolean BLOCK_HARDWARE_BITMAPS_WHEN_GL_CONTEXT_MIGHT_NOT_BE_INITIALIZED = Build.VERSION.SDK_INT < Build.VERSION_CODES.Q; /** @@ -87,7 +91,13 @@ public final class HardwareConfigState { @GuardedBy("this") private boolean isFdSizeBelowHardwareLimit = true; - private volatile boolean areHardwareBitmapsUnblocked; + /** + * Only mutated on the main thread. Read by any number of background threads concurrently. + * + *

Defaults to {@code false} because we need to wait for the GL context to be initialized and + * it defaults to not initialized (https://b.corp.google.com/issues/126573603#comment12). + */ + private final AtomicBoolean isHardwareConfigAllowedByAppState = new AtomicBoolean(false); public static HardwareConfigState getInstance() { if (instance == null) { @@ -112,8 +122,14 @@ public static HardwareConfigState getInstance() { } } - public void unblockHardwareBitmaps() { - areHardwareBitmapsUnblocked = true; + public synchronized void blockHardwareBitmaps() { + Util.assertMainThread(); + isHardwareConfigAllowedByAppState.set(false); + } + + public synchronized void unblockHardwareBitmaps() { + Util.assertMainThread(); + isHardwareConfigAllowedByAppState.set(true); } public boolean isHardwareConfigAllowed( @@ -121,22 +137,62 @@ public boolean isHardwareConfigAllowed( int targetHeight, boolean isHardwareConfigAllowed, boolean isExifOrientationRequired) { - if (!isHardwareConfigAllowed - || !isHardwareConfigAllowedByDeviceModel - || Build.VERSION.SDK_INT < Build.VERSION_CODES.O - || areHardwareBitmapsBlockedByAppState() - || isExifOrientationRequired) { + if (!isHardwareConfigAllowed) { + if (Log.isLoggable(TAG, Log.VERBOSE)) { + Log.v(TAG, "Hardware config disallowed by caller"); + } + return false; + } + if (!isHardwareConfigAllowedByDeviceModel) { + if (Log.isLoggable(TAG, Log.VERBOSE)) { + Log.v(TAG, "Hardware config disallowed by device model"); + } + return false; + } + if (Build.VERSION.SDK_INT < Build.VERSION_CODES.O) { + if (Log.isLoggable(TAG, Log.VERBOSE)) { + Log.v(TAG, "Hardware config disallowed by sdk"); + } + return false; + } + if (areHardwareBitmapsBlockedByAppState()) { + if (Log.isLoggable(TAG, Log.VERBOSE)) { + Log.v(TAG, "Hardware config disallowed by app state"); + } + return false; + } + if (isExifOrientationRequired) { + if (Log.isLoggable(TAG, Log.VERBOSE)) { + Log.v(TAG, "Hardware config disallowed because exif orientation is required"); + } + return false; + } + if (targetWidth < minHardwareDimension) { + if (Log.isLoggable(TAG, Log.VERBOSE)) { + Log.v(TAG, "Hardware config disallowed because width is too small"); + } + return false; + } + if (targetHeight < minHardwareDimension) { + if (Log.isLoggable(TAG, Log.VERBOSE)) { + Log.v(TAG, "Hardware config disallowed because height is too small"); + } + return false; + } + // Make sure to call isFdSizeBelowHardwareLimit last because it has side affects. + if (!isFdSizeBelowHardwareLimit()) { + if (Log.isLoggable(TAG, Log.VERBOSE)) { + Log.v(TAG, "Hardware config disallowed because there are insufficient FDs"); + } return false; } - return targetWidth >= minHardwareDimension - && targetHeight >= minHardwareDimension - // Make sure to call isFdSizeBelowHardwareLimit last because it has side affects. - && isFdSizeBelowHardwareLimit(); + return true; } private boolean areHardwareBitmapsBlockedByAppState() { - return BLOCK_HARDWARE_BITMAPS_BY_DEFAULT && !areHardwareBitmapsUnblocked; + return BLOCK_HARDWARE_BITMAPS_WHEN_GL_CONTEXT_MIGHT_NOT_BE_INITIALIZED + && !isHardwareConfigAllowedByAppState.get(); } @TargetApi(Build.VERSION_CODES.O) diff --git a/library/src/main/java/com/bumptech/glide/manager/DoNothingFirstFrameWaiter.java b/library/src/main/java/com/bumptech/glide/manager/DoNothingFirstFrameWaiter.java index 4edc6968de..1778e26ac0 100644 --- a/library/src/main/java/com/bumptech/glide/manager/DoNothingFirstFrameWaiter.java +++ b/library/src/main/java/com/bumptech/glide/manager/DoNothingFirstFrameWaiter.java @@ -2,7 +2,7 @@ import android.app.Activity; -final class DoNothingFirstFrameWaiter implements FirstFrameWaiter { +final class DoNothingFirstFrameWaiter implements FrameWaiter { @Override public void registerSelf(Activity activity) { diff --git a/library/src/main/java/com/bumptech/glide/manager/FirstFrameAndAfterTrimMemoryWaiter.java b/library/src/main/java/com/bumptech/glide/manager/FirstFrameAndAfterTrimMemoryWaiter.java new file mode 100644 index 0000000000..b03b87aced --- /dev/null +++ b/library/src/main/java/com/bumptech/glide/manager/FirstFrameAndAfterTrimMemoryWaiter.java @@ -0,0 +1,24 @@ +package com.bumptech.glide.manager; + +import android.app.Activity; +import android.content.ComponentCallbacks2; +import android.content.res.Configuration; +import android.os.Build; +import androidx.annotation.NonNull; +import androidx.annotation.RequiresApi; + +@RequiresApi(Build.VERSION_CODES.O) +final class FirstFrameAndAfterTrimMemoryWaiter implements FrameWaiter, ComponentCallbacks2 { + + @Override + public void registerSelf(Activity activity) {} + + @Override + public void onTrimMemory(int level) {} + + @Override + public void onConfigurationChanged(@NonNull Configuration newConfig) {} + + @Override + public void onLowMemory() {} +} diff --git a/library/src/main/java/com/bumptech/glide/manager/FirstFrameWaiter.java b/library/src/main/java/com/bumptech/glide/manager/FirstFrameWaiter.java index 2eb404b1a7..a6f44c1900 100644 --- a/library/src/main/java/com/bumptech/glide/manager/FirstFrameWaiter.java +++ b/library/src/main/java/com/bumptech/glide/manager/FirstFrameWaiter.java @@ -1,7 +1,12 @@ package com.bumptech.glide.manager; import android.app.Activity; +import android.os.Build; +import androidx.annotation.RequiresApi; -interface FirstFrameWaiter { - void registerSelf(Activity activity); +@RequiresApi(Build.VERSION_CODES.O) +final class FirstFrameWaiter implements FrameWaiter { + + @Override + public void registerSelf(Activity activity) {} } diff --git a/library/src/main/java/com/bumptech/glide/manager/FirstFrameWaiterO.java b/library/src/main/java/com/bumptech/glide/manager/FirstFrameWaiterO.java deleted file mode 100644 index 8031edb747..0000000000 --- a/library/src/main/java/com/bumptech/glide/manager/FirstFrameWaiterO.java +++ /dev/null @@ -1,12 +0,0 @@ -package com.bumptech.glide.manager; - -import android.app.Activity; -import android.os.Build; -import androidx.annotation.RequiresApi; - -@RequiresApi(Build.VERSION_CODES.O) -final class FirstFrameWaiterO implements FirstFrameWaiter { - - @Override - public void registerSelf(Activity activity) {} -} diff --git a/library/src/main/java/com/bumptech/glide/manager/FrameWaiter.java b/library/src/main/java/com/bumptech/glide/manager/FrameWaiter.java new file mode 100644 index 0000000000..8db3b1a8d3 --- /dev/null +++ b/library/src/main/java/com/bumptech/glide/manager/FrameWaiter.java @@ -0,0 +1,7 @@ +package com.bumptech.glide.manager; + +import android.app.Activity; + +interface FrameWaiter { + void registerSelf(Activity activity); +} 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 de8e2a3c16..912efa3297 100644 --- a/library/src/main/java/com/bumptech/glide/manager/RequestManagerRetriever.java +++ b/library/src/main/java/com/bumptech/glide/manager/RequestManagerRetriever.java @@ -22,6 +22,8 @@ import androidx.fragment.app.FragmentActivity; import androidx.fragment.app.FragmentManager; import com.bumptech.glide.Glide; +import com.bumptech.glide.GlideBuilder.WaitForFramesAfterTrimMemory; +import com.bumptech.glide.GlideExperiments; import com.bumptech.glide.RequestManager; import com.bumptech.glide.util.Preconditions; import com.bumptech.glide.util.Util; @@ -70,14 +72,23 @@ 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 = - Build.VERSION.SDK_INT >= Build.VERSION_CODES.O - ? new FirstFrameWaiterO() - : new DoNothingFirstFrameWaiter(); + private final FrameWaiter frameWaiter; - public RequestManagerRetriever(@Nullable RequestManagerFactory factory) { + public RequestManagerRetriever( + @Nullable RequestManagerFactory factory, GlideExperiments experiments) { this.factory = factory != null ? factory : DEFAULT_FACTORY; handler = new Handler(Looper.getMainLooper(), this /* Callback */); + + frameWaiter = buildFrameWaiter(experiments); + } + + private static FrameWaiter buildFrameWaiter(GlideExperiments experiments) { + if (Build.VERSION.SDK_INT < Build.VERSION_CODES.O) { + return new DoNothingFirstFrameWaiter(); + } + return experiments.isEnabled(WaitForFramesAfterTrimMemory.class) + ? new FirstFrameAndAfterTrimMemoryWaiter() + : new FirstFrameWaiter(); } @NonNull @@ -133,7 +144,7 @@ public RequestManager get(@NonNull FragmentActivity activity) { return get(activity.getApplicationContext()); } else { assertNotDestroyed(activity); - firstFrameWaiter.registerSelf(activity); + frameWaiter.registerSelf(activity); FragmentManager fm = activity.getSupportFragmentManager(); return supportFragmentGet(activity, fm, /*parentHint=*/ null, isActivityVisible(activity)); } @@ -152,7 +163,7 @@ public RequestManager get(@NonNull Fragment fragment) { // 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()); + frameWaiter.registerSelf(fragment.getActivity()); } FragmentManager fm = fragment.getChildFragmentManager(); return supportFragmentGet(fragment.getContext(), fm, fragment, fragment.isVisible()); @@ -168,7 +179,7 @@ public RequestManager get(@NonNull Activity activity) { return get((FragmentActivity) activity); } else { assertNotDestroyed(activity); - firstFrameWaiter.registerSelf(activity); + frameWaiter.registerSelf(activity); android.app.FragmentManager fm = activity.getFragmentManager(); return fragmentGet(activity, fm, /*parentHint=*/ null, isActivityVisible(activity)); } @@ -353,7 +364,7 @@ public RequestManager get(@NonNull android.app.Fragment fragment) { // 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()); + frameWaiter.registerSelf(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..3f755b4547 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 @@ -19,7 +19,7 @@ public class HardwareConfigStateTest { @Config(sdk = Build.VERSION_CODES.O) @Test public void - setHardwareConfigIfAllowed_withAllowedState_setsInPreferredConfigAndMutable_returnsFalse() { + setHardwareConfigIfAllowed_withAllowedState_setsInPreferredConfigAndMutable_returnsTrue() { HardwareConfigState state = new HardwareConfigState(); state.unblockHardwareBitmaps(); BitmapFactory.Options options = new BitmapFactory.Options(); @@ -32,10 +32,29 @@ public class HardwareConfigStateTest { /*isExifOrientationRequired=*/ false); assertThat(result).isTrue(); - assertThat(options.inMutable).isFalse(); assertThat(options.inPreferredConfig).isEqualTo(Bitmap.Config.HARDWARE); } + @Config(sdk = Build.VERSION_CODES.O) + @Test + public void + setHardwareConfigIfAllowed_withAllowedState_afterReblock_returnsFalseAndDoesNotSetValues() { + HardwareConfigState state = new HardwareConfigState(); + state.unblockHardwareBitmaps(); + state.blockHardwareBitmaps(); + BitmapFactory.Options options = new BitmapFactory.Options(); + 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.inPreferredConfig).isNotEqualTo(Bitmap.Config.HARDWARE); + } + @Config(sdk = Build.VERSION_CODES.O) @Test public void setHardwareConfigIfAllowed_withSmallerThanMinWidth_returnsFalse_doesNotSetValues() { 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..ac1a700345 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 @@ -24,6 +24,7 @@ import androidx.fragment.app.FragmentController; import androidx.fragment.app.FragmentHostCallback; import androidx.test.core.app.ApplicationProvider; +import com.bumptech.glide.GlideExperiments; import com.bumptech.glide.RequestManager; import com.bumptech.glide.tests.BackgroundUtil.BackgroundTester; import com.bumptech.glide.tests.GlideShadowLooper; @@ -58,7 +59,7 @@ public class RequestManagerRetrieverTest { public void setUp() { appContext = ApplicationProvider.getApplicationContext(); - retriever = new RequestManagerRetriever(/*factory=*/ null); + retriever = new RequestManagerRetriever(/*factory=*/ null, mock(GlideExperiments.class)); harnesses = new RetrieverHarness[] {new DefaultRetrieverHarness(), new SupportRetrieverHarness()};