Skip to content

Commit

Permalink
Add a method of frame waiting that resets when we get certain trim me…
Browse files Browse the repository at this point in the history
…mory 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
  • Loading branch information
sjudd authored and glide-copybara-robot committed Oct 19, 2020
1 parent 6d7c484 commit b0a7efe
Show file tree
Hide file tree
Showing 11 changed files with 179 additions and 48 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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()
Expand Down
16 changes: 11 additions & 5 deletions library/src/main/java/com/bumptech/glide/GlideBuilder.java
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@
@SuppressWarnings("PMD.ImmutableField")
public final class GlideBuilder {
private final Map<Class<?>, 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;
Expand Down Expand Up @@ -448,7 +448,7 @@ public GlideBuilder addGlobalRequestListener(@NonNull RequestListener<Object> li
* <p>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;
}

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand All @@ -571,7 +572,7 @@ Glide build(@NonNull Context context) {
defaultRequestOptionsFactory,
defaultTransitionOptions,
defaultRequestListeners,
glideExperiments.build());
experiments);
}

static final class ManualOverrideHardwareBitmapMaxFdCount implements Experiment {
Expand All @@ -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)}. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/**
Expand Down Expand Up @@ -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.
*
* <p>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) {
Expand All @@ -112,31 +122,77 @@ 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(
int targetWidth,
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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

import android.app.Activity;

final class DoNothingFirstFrameWaiter implements FirstFrameWaiter {
final class DoNothingFirstFrameWaiter implements FrameWaiter {

@Override
public void registerSelf(Activity activity) {
Expand Down
Original file line number Diff line number Diff line change
@@ -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() {}
}
Original file line number Diff line number Diff line change
@@ -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) {}
}

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
package com.bumptech.glide.manager;

import android.app.Activity;

interface FrameWaiter {
void registerSelf(Activity activity);
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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));
}
Expand All @@ -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());
Expand All @@ -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));
}
Expand Down Expand Up @@ -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());
Expand Down
Loading

0 comments on commit b0a7efe

Please sign in to comment.