Skip to content

Commit

Permalink
Avoid clearing Bitmaps in use by paused GifDrawables.
Browse files Browse the repository at this point in the history 
Fixes bumptech#2526.
  • Loading branch information
@sjudd
sjudd committed Oct 30, 2017
1 parent 2a0a594 commit e807803
Show file tree
Hide file tree
Showing 2 changed files with 125 additions and 12 deletions.
27 changes: 26 additions & 1 deletion library/src/main/java/com/bumptech/glide/load/resource/gif/GifFrameLoader.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ class GifFrameLoader {
private DelayTarget next;
private Bitmap firstFrame;
private Transformation<Bitmap> transformation;
private DelayTarget pendingTarget;

public interface FrameCallback {
void onFrameReady();
Expand Down Expand Up @@ -181,6 +182,10 @@ void clear() {
requestManager.clear(next);
next = null;
}
if (pendingTarget != null) {
requestManager.clear(pendingTarget);
pendingTarget = null;
}
gifDecoder.clear();
isCleared = true;
}
Expand All @@ -194,9 +199,17 @@ private void loadNextFrame() {
return;
}
if (startFromFirstFrame) {
Preconditions.checkArgument(
pendingTarget == null, "Pending target must be null when starting from the first frame");
gifDecoder.resetFrameIndex();
startFromFirstFrame = false;
}
if (pendingTarget != null) {
DelayTarget temp = pendingTarget;
pendingTarget = null;
onFrameReady(temp);
return;
}
isLoadPending = true;
// Get the delay before incrementing the pointer because the delay indicates the amount of time
// we want to spend on the current frame.
Expand All @@ -218,14 +231,27 @@ private void recycleFirstFrame() {
void setNextStartFromFirstFrame() {
Preconditions.checkArgument(!isRunning, "Can't restart a running animation");
startFromFirstFrame = true;
if (pendingTarget != null) {
requestManager.clear(pendingTarget);
pendingTarget = null;
}
}

// Visible for testing.
void onFrameReady(DelayTarget delayTarget) {
isLoadPending = false;
if (isCleared) {
handler.obtainMessage(FrameLoaderCallback.MSG_CLEAR, delayTarget).sendToTarget();
return;
}
// If we're not running, notifying here will recycle the frame that we might currently be
// showing, which breaks things (see #2526). We also can't discard this frame because we've
// already incremented the frame pointer and can't decode the same frame again. Instead we'll
// just hang on to this next frame until start() or clear() are called.
if (!isRunning) {
pendingTarget = delayTarget;
return;
}

if (delayTarget.getResource() != null) {
recycleFirstFrame();
Expand All @@ -242,7 +268,6 @@ void onFrameReady(DelayTarget delayTarget) {
}
}

isLoadPending = false;
loadNextFrame();
}

Expand Down
110 changes: 99 additions & 11 deletions library/src/test/java/com/bumptech/glide/load/resource/gif/GifFrameLoaderTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,14 +78,16 @@ public void tearDown() {
@NonNull
private GifFrameLoader createGifFrameLoader(Handler handler) {
Glide glide = getGlideSingleton();
return new GifFrameLoader(
GifFrameLoader result = new GifFrameLoader(
glide.getBitmapPool(),
requestManager,
gifDecoder,
handler,
requestBuilder,
transformation,
firstFrame);
result.subscribe(callback);
return result;
}

private static Glide getGlideSingleton() {
Expand All @@ -95,10 +97,11 @@ private static Glide getGlideSingleton() {
@SuppressWarnings("unchecked")
@Test
public void testSetFrameTransformationSetsTransformationOnRequestBuilder() {
verify(requestBuilder, times(2)).apply(isA(RequestOptions.class));
Transformation<Bitmap> transformation = mock(Transformation.class);
loader.setFrameTransformation(transformation, firstFrame);

verify(requestBuilder, times(2)).apply(isA(RequestOptions.class));
verify(requestBuilder, times(3)).apply(isA(RequestOptions.class));
}

@Test(expected = NullPointerException.class)
Expand All @@ -115,15 +118,11 @@ public void testReturnsSizeFromGifDecoderAndCurrentFrame() {

@Test
public void testStartGetsNextFrameIfNotStartedAndWithNoLoadPending() {
loader.subscribe(callback);

verify(requestBuilder).into(aTarget());
}

@Test
public void testGetNextFrameIncrementsSignatureAndAdvancesDecoderBeforeStartingLoad() {
loader.subscribe(callback);

InOrder order = inOrder(gifDecoder, requestBuilder);
order.verify(gifDecoder).advance();
order.verify(requestBuilder).apply(isA(RequestOptions.class));
Expand All @@ -147,22 +146,22 @@ public void testGetCurrentFrameReturnsCurrentBitmapAfterLoadHasCompleted() {

@Test
public void testStartDoesNotStartIfAlreadyRunning() {
loader.subscribe(callback);
loader.subscribe(mock(FrameCallback.class));

verify(requestBuilder, times(1)).into(aTarget());
}

@Test
public void testGetNextFrameDoesNotStartLoadIfLoaderIsNotRunning() {
verify(requestBuilder, times(1)).into(aTarget());
loader.unsubscribe(callback);
loader.onFrameReady(mock(DelayTarget.class));

verify(requestBuilder, never()).into(aTarget());
verify(requestBuilder, times(1)).into(aTarget());
}

@Test
public void testGetNextFrameDoesNotStartLoadIfLoadIsInProgress() {
loader.subscribe(callback);
loader.unsubscribe(callback);
loader.subscribe(callback);

Expand All @@ -171,7 +170,6 @@ public void testGetNextFrameDoesNotStartLoadIfLoadIsInProgress() {

@Test
public void testGetNextFrameDoesStartLoadIfRestartedAndNoLoadIsInProgress() {
loader.subscribe(callback);
loader.unsubscribe(callback);

loader.onFrameReady(mock(DelayTarget.class));
Expand All @@ -182,7 +180,6 @@ public void testGetNextFrameDoesStartLoadIfRestartedAndNoLoadIsInProgress() {

@Test
public void testGetNextFrameDoesStartLoadAfterLoadCompletesIfStarted() {
loader.subscribe(callback);
loader.onFrameReady(mock(DelayTarget.class));

verify(requestBuilder, times(2)).into(aTarget());
Expand Down Expand Up @@ -269,6 +266,97 @@ public void testClearsCompletedLoadOnFrameReadyIfCleared() {
assertNull(loader.getCurrentFrame());
}

@Test
public void onFrameReady_whenNotRunning_doesNotClearPreviouslyLoadedImage() {
loader = createGifFrameLoader(/*handler=*/ null);
DelayTarget loaded = mock(DelayTarget.class);
when(loaded.getResource()).thenReturn(Bitmap.createBitmap(100, 100, Bitmap.Config.ARGB_8888));
loader.onFrameReady(loaded);
loader.unsubscribe(callback);

DelayTarget nextFrame = mock(DelayTarget.class);
when(nextFrame.getResource())
.thenReturn(Bitmap.createBitmap(100, 100, Bitmap.Config.ARGB_8888));
loader.onFrameReady(nextFrame);
verify(requestManager, never()).clear(loaded);
}

@Test
public void onFrameReady_whenNotRunning_clearsPendingFrameOnClear() {
loader = createGifFrameLoader(/*handler=*/ null);
DelayTarget loaded = mock(DelayTarget.class);
when(loaded.getResource()).thenReturn(Bitmap.createBitmap(100, 100, Bitmap.Config.ARGB_8888));
loader.onFrameReady(loaded);
loader.unsubscribe(callback);

DelayTarget nextFrame = mock(DelayTarget.class);
when(nextFrame.getResource())
.thenReturn(Bitmap.createBitmap(100, 100, Bitmap.Config.ARGB_8888));
loader.onFrameReady(nextFrame);

loader.clear();
verify(requestManager).clear(loaded);
verify(requestManager).clear(nextFrame);
}

@Test
public void onFrameReady_whenNotRunning_clearsOldFrameOnStart() {
loader = createGifFrameLoader(/*handler=*/ null);
DelayTarget loaded = mock(DelayTarget.class);
when(loaded.getResource()).thenReturn(Bitmap.createBitmap(100, 100, Bitmap.Config.ARGB_8888));
loader.onFrameReady(loaded);
loader.unsubscribe(callback);

DelayTarget nextFrame = mock(DelayTarget.class);
when(nextFrame.getResource())
.thenReturn(Bitmap.createBitmap(100, 100, Bitmap.Config.ARGB_8888));
loader.onFrameReady(nextFrame);

loader.subscribe(callback);
verify(requestManager).clear(loaded);
}

@Test
public void onFrameReady_whenNotRunning_callsFrameReadyWithNewFrameOnStart() {
loader = createGifFrameLoader(/*handler=*/ null);
DelayTarget loaded = mock(DelayTarget.class);
when(loaded.getResource()).thenReturn(Bitmap.createBitmap(100, 100, Bitmap.Config.ARGB_8888));
loader.onFrameReady(loaded);
loader.unsubscribe(callback);

DelayTarget nextFrame = mock(DelayTarget.class);
Bitmap expected = Bitmap.createBitmap(200, 200, Bitmap.Config.ARGB_8888);
when(nextFrame.getResource())
.thenReturn(expected);
loader.onFrameReady(nextFrame);

verify(callback, times(1)).onFrameReady();
loader.subscribe(callback);
verify(callback, times(2)).onFrameReady();
assertThat(loader.getCurrentFrame()).isEqualTo(expected);
}

@Test
public void startFromFirstFrame_withPendingFrame_clearsPendingFrame() {
loader = createGifFrameLoader(/*handler=*/ null);
DelayTarget loaded = mock(DelayTarget.class);
when(loaded.getResource()).thenReturn(Bitmap.createBitmap(100, 100, Bitmap.Config.ARGB_8888));
loader.onFrameReady(loaded);
loader.unsubscribe(callback);

DelayTarget nextFrame = mock(DelayTarget.class);
Bitmap expected = Bitmap.createBitmap(200, 200, Bitmap.Config.ARGB_8888);
when(nextFrame.getResource())
.thenReturn(expected);
loader.onFrameReady(nextFrame);

loader.setNextStartFromFirstFrame();
verify(requestManager).clear(nextFrame);

loader.subscribe(callback);
verify(callback, times(1)).onFrameReady();
}

@SuppressWarnings("unchecked")
private static Target<Bitmap> aTarget() {
return isA(Target.class);
Expand Down

0 comments on commit e807803

Please sign in to comment.