Skip to content

Commit

Permalink
fixes bumptech#738: Add optional threading lock for bitmap manipulati…
Browse files Browse the repository at this point in the history
…on (affects only Moto X 2nd gen on api 22)

this addresses a threading bug on this specific device, the bug manifests itself by displaying
black images instead of resized images.
  • Loading branch information
Matthijs Mullender committed Nov 20, 2015
1 parent def92c7 commit 8315299
Show file tree
Hide file tree
Showing 3 changed files with 139 additions and 43 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
package com.bumptech.glide.load.resource.bitmap;

import android.os.Build;
import android.support.annotation.NonNull;

import java.util.concurrent.TimeUnit;
import java.util.concurrent.locks.Condition;
import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantLock;

/**
* https://github.com/bumptech/glide/issues/738 On some devices (Moto X with android 5.1) bitmap
* drawing is not thread safe.
*
* This class provides a lock implementation that only really locks for these devices. For other
* types of devices the lock is always available and therefore does not impact performance The
* actual lock logic is delegated to {@see ReentrantLock}
*/
final class BitmapDrawLock implements Lock {
private static final BitmapDrawLock INSTANCE = new BitmapDrawLock();
private final Lock lock = new ReentrantLock();
// only enable the lock for Moto X 2nd gen on android 5.1
private final boolean shouldUseLock = "XT1097".equals(Build.MODEL)
&& Build.VERSION.SDK_INT == Build.VERSION_CODES.LOLLIPOP_MR1;

private BitmapDrawLock() {
}

public static BitmapDrawLock getInstance() {
return INSTANCE;
}

@Override
public void lock() {
if (!shouldUseLock) {
return;
}
lock.lock();
}

@Override
public void lockInterruptibly() throws InterruptedException {
if (!shouldUseLock) {
return;
}
lock.lockInterruptibly();
}

@NonNull
@Override
public Condition newCondition() {
if (!shouldUseLock) {
return null;
}
return lock.newCondition();
}

@Override
public boolean tryLock() {
if (!shouldUseLock) {
return true;
}
return lock.tryLock();
}

@Override
public boolean tryLock(long l, TimeUnit timeUnit) throws InterruptedException {
if (!shouldUseLock) {
return true;
}
return lock.tryLock(l, timeUnit);
}

@Override
public void unlock() {
if (!shouldUseLock) {
return;
}
lock.unlock();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,8 @@ public final class Downsampler {
"com.bumptech.glide.load.resource.bitmap.Downsampler.DecodeFormat", DecodeFormat.DEFAULT);
/**
* Indicates the {@link com.bumptech.glide.load.resource.bitmap.DownsampleStrategy} option that
* will be used to calculate the sample size to use to downsample an image given the original
* and target dimensions of the image.
* will be used to calculate the sample size to use to downsample an image given the original and
* target dimensions of the image.
*/
public static final Option<DownsampleStrategy> DOWNSAMPLE_STRATEGY =
Option.memory("com.bumptech.glide.load.resource.bitmap.Downsampler.DownsampleStrategy",
Expand Down Expand Up @@ -109,22 +109,21 @@ public Resource<Bitmap> decode(InputStream is, int outWidth, int outHeight,
* data present in the stream and that is downsampled according to the given dimensions and any
* provided {@link com.bumptech.glide.load.resource.bitmap.DownsampleStrategy} option.
*
* <p> If a Bitmap is present in the
* {@link com.bumptech.glide.load.engine.bitmap_recycle.BitmapPool} whose dimensions exactly match
* those of the image for the given InputStream is available, the operation is much less expensive
* in terms of memory. </p>
* If a Bitmap is present in the {@link com.bumptech.glide.load.engine.bitmap_recycle.BitmapPool}
* whose dimensions exactly match those of the image for the given InputStream is available, the
* operation is much less expensive in terms of memory.
*
* <p> The provided {@link java.io.InputStream} must return <code>true</code> from
* {@link java.io.InputStream#markSupported()} and is expected to support a reasonably large
* mark limit to accommodate reading large image headers (~5MB). </p>
* The provided {@link java.io.InputStream} must return <code>true</code> from {@link
* java.io.InputStream#markSupported()} and is expected to support a reasonably large mark limit
* to accommodate reading large image headers (~5MB).
*
* @param is An {@link InputStream} to the data for the image.
* @param is An {@link InputStream} to the data for the image.
* @param requestedWidth The width the final image should be close to.
* @param requestedHeight The height the final image should be close to.
* @param options A set of options that may contain one or more supported options that influence
* how a Bitmap will be decoded from the given stream.
* @param callbacks A set of callbacks allowing callers to optionally respond to various
* significant events during the decode process.
* @param options A set of options that may contain one or more supported options that
* influence how a Bitmap will be decoded from the given stream.
* @param callbacks A set of callbacks allowing callers to optionally respond to various
* significant events during the decode process.
* @return A new bitmap containing the image from the given InputStream, or recycle if recycle is
* not null.
*/
Expand Down Expand Up @@ -248,7 +247,17 @@ static float calculateScaling(DownsampleStrategy downsampleStrategy, int degrees
float adjustedScaleFactor = powerOfTwoSampleSize * exactScaleFactor;

options.inSampleSize = powerOfTwoSampleSize;

// Density scaling is only supported if inBitmap is null prior to KitKat. Avoid setting
// densities here so we calculate the final Bitmap size correctly.
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.KITKAT) {
options.inTargetDensity = (int) (1000 * adjustedScaleFactor + 0.5f);
options.inDensity = 1000;
}
if (isScaling(options)) {
options.inScaled = true;
} else {
options.inDensity = options.inTargetDensity = 0;
}
if (Log.isLoggable(TAG, Log.VERBOSE)) {
Log.v(TAG, "Calculate scaling"
+ ", source: [" + sourceWidth + "x" + sourceHeight + "]"
Expand Down Expand Up @@ -369,7 +378,7 @@ private static int[] getDimensions(InputStream is, BitmapFactory.Options options
options.inJustDecodeBounds = true;
decodeStream(is, options, decodeCallbacks);
options.inJustDecodeBounds = false;
return new int[] { options.outWidth, options.outHeight };
return new int[]{options.outWidth, options.outHeight};
}

private static Bitmap decodeStream(InputStream is, BitmapFactory.Options options,
Expand All @@ -390,17 +399,19 @@ private static Bitmap decodeStream(InputStream is, BitmapFactory.Options options
int sourceHeight = options.outHeight;
String outMimeType = options.outMimeType;
final Bitmap result;
BitmapDrawLock.getInstance().lock();
try {
result = BitmapFactory.decodeStream(is, null, options);
} catch (IllegalArgumentException e) {
throw newIoExceptionForInBitmapAssertion(e, sourceWidth, sourceHeight, outMimeType, options);
} finally {
BitmapDrawLock.getInstance().unlock();
}

if (options.inJustDecodeBounds) {
is.reset();

}

return result;
}

Expand Down Expand Up @@ -449,10 +460,10 @@ private static String getBitmapString(Bitmap bitmap) {
private static IOException newIoExceptionForInBitmapAssertion(IllegalArgumentException e,
int outWidth, int outHeight, String outMimeType, BitmapFactory.Options options) {
return new IOException("Exception decoding bitmap"
+ ", outWidth: " + outWidth
+ ", outHeight: " + outHeight
+ ", outMimeType: " + outMimeType
+ ", inBitmap: " + getInBitmapString(options), e);
+ ", outWidth: " + outWidth
+ ", outHeight: " + outHeight
+ ", outMimeType: " + outMimeType
+ ", inBitmap: " + getInBitmapString(options), e);
}

@TargetApi(Build.VERSION_CODES.HONEYCOMB)
Expand Down Expand Up @@ -510,6 +521,7 @@ private static void resetOptions(BitmapFactory.Options decodeBitmapOptions) {
*/
public interface DecodeCallbacks {
void onObtainBounds();

void onDecodeComplete(BitmapPool bitmapPool, Bitmap downsampled) throws IOException;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ public final class TransformationUtils {
private static final int CIRCLE_CROP_PAINT_FLAGS = PAINT_FLAGS | Paint.ANTI_ALIAS_FLAG;
private static final Paint CIRCLE_CROP_SHAPE_PAINT = new Paint(CIRCLE_CROP_PAINT_FLAGS);
private static final Paint CIRCLE_CROP_BITMAP_PAINT;
private static final Object LOCK = new Object();

static {
CIRCLE_CROP_BITMAP_PAINT = new Paint(CIRCLE_CROP_PAINT_FLAGS);
Expand Down Expand Up @@ -76,11 +75,7 @@ public static Bitmap centerCrop(@NonNull BitmapPool pool, @NonNull Bitmap inBitm
// We don't add or remove alpha, so keep the alpha setting of the Bitmap we were given.
TransformationUtils.setAlpha(inBitmap, result);

synchronized (LOCK) {
Canvas canvas = new Canvas(result);
canvas.drawBitmap(inBitmap, m, DEFAULT_PAINT);
clear(canvas);
}
applyMatrix(inBitmap, result, m);
return result;
}

Expand Down Expand Up @@ -133,15 +128,23 @@ public static Bitmap fitCenter(@NonNull BitmapPool pool, @NonNull Bitmap inBitma
Log.v(TAG, "minPct: " + minPercentage);
}

synchronized (LOCK) {
Canvas canvas = new Canvas(toReuse);
Matrix matrix = new Matrix();
matrix.setScale(minPercentage, minPercentage);
Matrix matrix = new Matrix();
matrix.setScale(minPercentage, minPercentage);
applyMatrix(inBitmap, toReuse, matrix);

return toReuse;
}

private static void applyMatrix(@NonNull Bitmap inBitmap, @NonNull Bitmap targetBitmap,
Matrix matrix) {
BitmapDrawLock.getInstance().lock();
try {
Canvas canvas = new Canvas(targetBitmap);
canvas.drawBitmap(inBitmap, matrix, DEFAULT_PAINT);
clear(canvas);
} finally {
BitmapDrawLock.getInstance().unlock();
}

return toReuse;
}

/**
Expand Down Expand Up @@ -246,11 +249,7 @@ public static Bitmap rotateImageExif(@NonNull BitmapPool pool, @NonNull Bitmap i

matrix.postTranslate(-newRect.left, -newRect.top);

synchronized (LOCK) {
final Canvas canvas = new Canvas(result);
canvas.drawBitmap(inBitmap, matrix, DEFAULT_PAINT);
clear(canvas);
}
applyMatrix(inBitmap, result, matrix);
return result;
}

Expand Down Expand Up @@ -283,16 +282,18 @@ public static Bitmap circleCrop(@NonNull BitmapPool pool, @NonNull Bitmap inBitm
Bitmap result = pool.get(destWidth, destHeight, getSafeConfig(toTransform));
setAlphaIfAvailable(result, true /*hasAlpha*/);

synchronized (LOCK) {
Canvas canvas = new Canvas(result);

BitmapDrawLock.getInstance().lock();
try {
Canvas canvas = new Canvas(result);
// Draw a circle
canvas.drawCircle(destRect.left + radius, destRect.top + radius, radius,
CIRCLE_CROP_SHAPE_PAINT);

// Draw the bitmap in the circle
canvas.drawBitmap(toTransform, srcRect, destRect, CIRCLE_CROP_BITMAP_PAINT);
clear(canvas);
} finally {
BitmapDrawLock.getInstance().unlock();
}

if (!toTransform.equals(inBitmap)) {
Expand Down Expand Up @@ -345,12 +346,14 @@ public static Bitmap roundedCorners(@NonNull BitmapPool pool, @NonNull Bitmap in
paint.setAntiAlias(true);
paint.setShader(shader);
RectF rect = new RectF(0, 0, result.getWidth(), result.getHeight());

synchronized (LOCK) {
BitmapDrawLock.getInstance().lock();
try {
Canvas canvas = new Canvas(result);
canvas.drawColor(Color.TRANSPARENT, PorterDuff.Mode.CLEAR);
canvas.drawRoundRect(rect, roundingRadius, roundingRadius, paint);
clear(canvas);
} finally {
BitmapDrawLock.getInstance().unlock();
}

if (!toTransform.equals(inBitmap)) {
Expand Down

0 comments on commit 8315299

Please sign in to comment.