Skip to content

Commit

Permalink
Handle null outConfigs from BitmapFactory on API 26.
Browse files Browse the repository at this point in the history
We may be able to be smarter about how we perform this fallback. Not
all images are affected and those that are may be limited to some
subtypes of images. This fix may have negative side affects if it causes
us to use the wrong config for the inBitmap. Although the image should
still decode if we use the wrong config because we retry on failures
with a null inBitmap, it can prevent us from re-using Bitmaps and waste
memory and CPU cycles.
  • Loading branch information
sjudd committed Jan 3, 2018
1 parent 01e5f74 commit 0deee30
Show file tree
Hide file tree
Showing 6 changed files with 365 additions and 2 deletions.
1 change: 0 additions & 1 deletion build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ subprojects { project ->
if ("gifencoder" != project.getName()) {
options.compilerArgs \
<< "-Xlint:all" \
<< "-Werror" \
/*
* Java expects every annotation to have a processor, but we use
* javax.annotation.Nullable, which doesn't have one.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,349 @@
package com.bumptech.glide;

import static com.google.common.truth.Truth.assertThat;
import static org.junit.Assume.assumeTrue;

import android.content.ContentResolver;
import android.content.Context;
import android.content.res.Resources;
import android.graphics.Bitmap;
import android.graphics.BitmapFactory;
import android.graphics.ColorSpace;
import android.net.Uri;
import android.os.Build;
import android.support.annotation.DrawableRes;
import android.support.annotation.NonNull;
import android.support.test.InstrumentationRegistry;
import android.support.test.runner.AndroidJUnit4;
import com.bumptech.glide.load.DataSource;
import com.bumptech.glide.load.DecodeFormat;
import com.bumptech.glide.load.Options;
import com.bumptech.glide.load.data.DataFetcher;
import com.bumptech.glide.load.model.ModelLoader;
import com.bumptech.glide.load.model.ModelLoaderFactory;
import com.bumptech.glide.load.model.MultiModelLoaderFactory;
import com.bumptech.glide.signature.ObjectKey;
import com.bumptech.glide.test.ConcurrencyHelper;
import com.bumptech.glide.test.GlideApp;
import com.bumptech.glide.test.ResourceIds;
import com.bumptech.glide.test.TearDownGlide;
import java.io.ByteArrayOutputStream;
import java.io.FileNotFoundException;
import java.io.IOException;
import java.io.InputStream;
import java.util.Locale;
import org.junit.Rule;
import org.junit.Test;
import org.junit.runner.RunWith;

/**
* On API 26, decoding a variety of different images can cause {@link BitmapFactory} with
* {@link BitmapFactory.Options#inJustDecodeBounds} set to {@code true} to set
* {@link BitmapFactory.Options#outConfig} to null instead of a valid value, even though the image
* can be decoded successfully. Glide can mask these failures by decoding some image sources
* (notably including resource ids) using other data types and decoders.
*
* <p>This test ensures that we've worked around the framework issue by loading a variety of images
* and image types without the normal fallback behavior.
*/
@RunWith(AndroidJUnit4.class)
public class LoadResourcesWithDownsamplerTest {
@Rule public final TearDownGlide tearDownGlide = new TearDownGlide();
private final ConcurrencyHelper concurrency = new ConcurrencyHelper();
private final Context context = InstrumentationRegistry.getTargetContext();

@Test
public void loadJpegResource_withNoOtherLoaders_decodesResource() {
Glide.get(context).getRegistry()
.prepend(
Object.class,
InputStream.class,
new FakeModelLoader<>(ResourceIds.raw.canonical));

Bitmap bitmap =
concurrency.get(
Glide.with(context)
.asBitmap()
.load(new Object())
.submit());
assertThat(bitmap).isNotNull();
}

@Test
public void loadWideGamutJpegResource_withNoOtherLoaders_decodesWideGamutBitmap() {
assumeTrue(
"Wide gamut is only available on O+", Build.VERSION.SDK_INT >= Build.VERSION_CODES.O);
Glide.get(context).getRegistry()
.prepend(
Object.class,
InputStream.class,
new FakeModelLoader<>(ResourceIds.raw.webkit_logo_p3));

Bitmap bitmap =
concurrency.get(
Glide.with(context)
.asBitmap()
.load(new Object())
.submit());
assertThat(bitmap).isNotNull();
assertThat(bitmap.getConfig()).isEqualTo(Bitmap.Config.RGBA_F16);
assertThat(bitmap.getColorSpace())
.isEqualTo(ColorSpace.get(ColorSpace.Named.LINEAR_EXTENDED_SRGB));
}

@Test
public void loadOpaquePngResource_withNoOtherLoaders_decodesResource() {
Glide.get(context).getRegistry()
.prepend(
Object.class,
InputStream.class,
new FakeModelLoader<>(ResourceIds.raw.canonical_png));

Bitmap bitmap =
concurrency.get(
Glide.with(context)
.asBitmap()
.load(new Object())
.submit());
assertThat(bitmap).isNotNull();
}

@Test
public void loadTransparentPngResource_withNoOtherLoaders_decodesResource() {
Glide.get(context).getRegistry()
.prepend(
Object.class,
InputStream.class,
new FakeModelLoader<>(ResourceIds.raw.canonical_transparent_png));

Bitmap bitmap =
concurrency.get(
Glide.with(context)
.asBitmap()
.load(new Object())
.submit());
assertThat(bitmap).isNotNull();
}

@Test
public void loadTransparentGifResource_withNoOtherLoaders_decodesResource() {
Glide.get(context).getRegistry()
.prepend(
Object.class,
InputStream.class,
new FakeModelLoader<>(ResourceIds.raw.transparent_gif));

Bitmap bitmap =
concurrency.get(
Glide.with(context)
.asBitmap()
.load(new Object())
.submit());
assertThat(bitmap).isNotNull();
}

@Test
public void loadTransparentGifResource_asHardware_withNoOtherLoaders_decodesResource() {
assumeTrue(
"Hardware Bitmaps are only supported on O+",
Build.VERSION.SDK_INT >= Build.VERSION_CODES.O);

Glide.get(context).getRegistry()
.prepend(
Object.class,
InputStream.class,
new FakeModelLoader<>(ResourceIds.raw.transparent_gif));

Bitmap bitmap =
concurrency.get(
GlideApp.with(context)
.asBitmap()
// Allow HARDWARE Bitmaps.
.format(DecodeFormat.PREFER_ARGB_8888)
.load(new Object())
.submit());
assertThat(bitmap).isNotNull();
assertThat(bitmap.getConfig()).isEqualTo(Bitmap.Config.HARDWARE);
}

@Test
public void loadTransparentGifResource_withNoOtherLoaders_fromBytes_decodesResource() {
byte[] data = getBytes(ResourceIds.raw.transparent_gif);
Bitmap bitmap =
concurrency.get(
Glide.with(context)
.asBitmap()
.load(data)
.submit());
assertThat(bitmap).isNotNull();
}

@Test
public void loadOpaqueGifResource_withNoOtherLoaders_decodesResource() {
Glide.get(context).getRegistry()
.prepend(
Object.class,
InputStream.class,
new FakeModelLoader<>(ResourceIds.raw.opaque_gif));

Bitmap bitmap =
concurrency.get(
Glide.with(context)
.asBitmap()
.load(new Object())
.submit());
assertThat(bitmap).isNotNull();
}

@Test
public void loadOpaqueGifResource_asBytes_decodesResource() {
byte[] data = getBytes(ResourceIds.raw.opaque_gif);
Bitmap bitmap =
concurrency.get(
Glide.with(context)
.asBitmap()
.load(data)
.submit());
assertThat(bitmap).isNotNull();
}

@Test
public void loadOpaqueGifResource_asHardware_withNoOtherLoaders_decodesResource() {
assumeTrue(
"Hardware Bitmaps are only supported on O+",
Build.VERSION.SDK_INT >= Build.VERSION_CODES.O);

Glide.get(context).getRegistry()
.prepend(
Object.class,
InputStream.class,
new FakeModelLoader<>(ResourceIds.raw.opaque_gif));

Bitmap bitmap =
concurrency.get(
GlideApp.with(context)
.asBitmap()
// Allow HARDWARE Bitmaps.
.format(DecodeFormat.PREFER_ARGB_8888)
.load(new Object())
.submit());
assertThat(bitmap).isNotNull();
}


private byte[] getBytes(int resourceId) {
ByteArrayOutputStream os = new ByteArrayOutputStream();
InputStream is = null;
try {
is = context.getResources().openRawResource(resourceId);
byte[] buffer = new byte[1024 * 1024];
int read;
while ((read = is.read(buffer)) != -1) {
os.write(buffer, 0, read);
}
} catch (IOException e) {
throw new RuntimeException(e);
} finally {
if (is != null) {
try {
is.close();
} catch (IOException e) {
// Ignored;
}
}
}

return os.toByteArray();
}

private class FakeModelLoader<T> implements
ModelLoader<T, InputStream>,
ModelLoaderFactory<T, InputStream> {

private final int resourceId;

FakeModelLoader(int resourceId) {
this.resourceId = resourceId;
}

@android.support.annotation.Nullable
@Override
public LoadData<InputStream> buildLoadData(@NonNull Object o, int width, int height,
@NonNull Options options) {
return new LoadData<>(new ObjectKey(o), new Fetcher());
}

@Override
public boolean handles(@NonNull Object o) {
return true;
}

@NonNull
@Override
public ModelLoader<T, InputStream> build(@NonNull MultiModelLoaderFactory multiFactory) {
return this;
}

@Override
public void teardown() { }

private final class Fetcher implements DataFetcher<InputStream> {
private InputStream inputStream;

@Override
public void loadData(@NonNull Priority priority,
@NonNull DataCallback<? super InputStream> callback) {
inputStream = getInputStreamForResource(context, resourceId);
callback.onDataReady(inputStream);
}

private InputStream getInputStreamForResource(
Context context, @DrawableRes int resourceId) {
Resources resources = context.getResources();
try {
Uri parse =
Uri.parse(
String.format(
Locale.US,
"%s://%s/%s/%s",
ContentResolver.SCHEME_ANDROID_RESOURCE,
resources.getResourcePackageName(resourceId),
resources.getResourceTypeName(resourceId),
resources.getResourceEntryName(resourceId)));
return context.getContentResolver().openInputStream(parse);
} catch (Resources.NotFoundException | FileNotFoundException e) {
throw new IllegalArgumentException("Resource ID " + resourceId + " not found", e);
}
}

@Override
public void cleanup() {
InputStream local = inputStream;
if (local != null) {
try {
local.close();
} catch (IOException e) {
// Ignored.
}
}
}

@Override
public void cancel() {
// Do nothing.
}

@NonNull
@Override
public Class<InputStream> getDataClass() {
return InputStream.class;
}

@NonNull
@Override
public DataSource getDataSource() {
return DataSource.LOCAL;
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ private ResourceIds() {
public interface raw {
int dl_world_anim = getResourceId("raw", "dl_world_anim");
int canonical = getResourceId("raw", "canonical");
int canonical_png = getResourceId("raw", "canonical_png");
int canonical_transparent_png = getResourceId("raw", "canonical_transparent_png");
int interlaced_transparent_gif = getResourceId("raw", "interlaced_transparent_gif");
int transparent_gif = getResourceId("raw", "transparent_gif");
int opaque_gif = getResourceId("raw", "opaque_gif");
Expand Down
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Original file line number Diff line number Diff line change
Expand Up @@ -666,14 +666,27 @@ private static IOException newIoExceptionForInBitmapAssertion(IllegalArgumentExc
@TargetApi(Build.VERSION_CODES.O)
private static void setInBitmap(
BitmapFactory.Options options, BitmapPool bitmapPool, int width, int height) {
@Nullable Bitmap.Config expectedConfig = null;
// Avoid short circuiting, it appears to break on some devices.
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) {
if (options.inPreferredConfig == Config.HARDWARE) {
return;
}
// On API 26 outConfig may be null for some images even if the image is valid, can be decoded
// and outWidth/outHeight/outColorSpace are populated (see b/71513049).
expectedConfig = options.outConfig;
}

if (expectedConfig == null) {
// We're going to guess that BitmapFactory will return us the config we're requesting. This
// isn't always the case, even though our guesses tend to be conservative and prefer configs
// of larger sizes so that the Bitmap will fit our image anyway. If we're wrong here and the
// config we choose is too small, our initial decode will fail, but we will retry with no
// inBitmap which will succeed so if we're wrong here, we're less efficient but still correct.
expectedConfig = options.inPreferredConfig;
}
// BitmapFactory will clear out the Bitmap before writing to it, so getDirty is safe.
options.inBitmap = bitmapPool.getDirty(width, height, options.inPreferredConfig);
options.inBitmap = bitmapPool.getDirty(width, height, expectedConfig);
}

private static synchronized BitmapFactory.Options getDefaultOptions() {
Expand Down

0 comments on commit 0deee30

Please sign in to comment.