Skip to content

Commit

Permalink
Rollforward after disabling first frame waiter < O (hardware Bitmaps …
Browse files Browse the repository at this point in the history
…require O+ anyway).

PiperOrigin-RevId: 331577544
  • Loading branch information
sjudd authored and glide-copybara-robot committed Sep 14, 2020
1 parent 7b40fd2 commit a3a3a65
Show file tree
Hide file tree
Showing 11 changed files with 121 additions and 29 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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()
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
15 changes: 14 additions & 1 deletion library/src/main/java/com/bumptech/glide/Glide.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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;
Expand Down
9 changes: 1 addition & 8 deletions library/src/main/java/com/bumptech/glide/GlideBuilder.java
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand All @@ -583,7 +577,6 @@ Glide build(@NonNull Context context) {
defaultRequestListeners,
isLoggingRequestOriginsEnabled,
isImageDecoderEnabledForBitmaps,
blockHardwareBitmaps,
manualOverrideHardwareBitmapMaxFdCount);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -104,6 +112,10 @@ public static HardwareConfigState getInstance() {
}
}

public void unblockHardwareBitmaps() {
areHardwareBitmapsUnblocked = true;
}

public boolean isHardwareConfigAllowed(
int targetWidth,
int targetHeight,
Expand All @@ -112,7 +124,7 @@ public boolean isHardwareConfigAllowed(
if (!isHardwareConfigAllowed
|| !isHardwareConfigAllowedByDeviceModel
|| Build.VERSION.SDK_INT < Build.VERSION_CODES.O
|| (blockHardwareBitmapsByDefault && !areHardwareBitmapsUnblocked)
|| areHardwareBitmapsBlockedByAppState()
|| isExifOrientationRequired) {
return false;
}
Expand All @@ -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,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
package com.bumptech.glide.manager;

import android.app.Activity;

final class DoNothingFirstFrameWaiter implements FirstFrameWaiter {

@Override
public void registerSelf(Activity activity) {
// Do nothing.
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

import android.app.Activity;

final class FirstFrameWaiter {

void registerSelf(Activity activity) {}
interface FirstFrameWaiter {
void registerSelf(Activity activity);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
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) {}
}
Original file line number Diff line number Diff line change
Expand Up @@ -70,12 +70,13 @@ 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 =
Build.VERSION.SDK_INT >= Build.VERSION_CODES.O
? new FirstFrameWaiterO()
: new DoNothingFirstFrameWaiter();

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 */);
}

Expand Down Expand Up @@ -126,19 +127,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));
}
Expand All @@ -152,7 +147,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());
}
Expand All @@ -167,7 +168,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));
}
Expand Down Expand Up @@ -347,7 +348,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());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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;
Expand All @@ -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;
Expand All @@ -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;
Expand All @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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()};
Expand Down

0 comments on commit a3a3a65

Please sign in to comment.