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()};