Avoid clearing Bitmaps in use by paused GifDrawables.

Fixes #2526.

Author: sjudd

library/src/main/java/com/bumptech/glide/load/resource/gif/GifFrameLoader.java

@@ -43,6 +43,7 @@ class GifFrameLoader {
   private DelayTarget next;
   private Bitmap firstFrame;
   private Transformation<Bitmap> transformation;
+  private DelayTarget pendingTarget;
 
   public interface FrameCallback {
     void onFrameReady();
@@ -181,6 +182,10 @@ void clear() {
       requestManager.clear(next);
       next = null;
     }
+    if (pendingTarget != null) {
+      requestManager.clear(pendingTarget);
+      pendingTarget = null;
+    }
     gifDecoder.clear();
     isCleared = true;
   }
@@ -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.
@@ -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();
@@ -242,7 +268,6 @@ void onFrameReady(DelayTarget delayTarget) {
       }
     }
 
-    isLoadPending = false;
     loadNextFrame();
   }
 

library/src/test/java/com/bumptech/glide/load/resource/gif/GifFrameLoaderTest.java

@@ -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() {
@@ -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)
@@ -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));
@@ -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);
 
@@ -171,7 +170,6 @@ public void testGetNextFrameDoesNotStartLoadIfLoadIsInProgress() {
 
   @Test
   public void testGetNextFrameDoesStartLoadIfRestartedAndNoLoadIsInProgress() {
-    loader.subscribe(callback);
     loader.unsubscribe(callback);
 
     loader.onFrameReady(mock(DelayTarget.class));
@@ -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());
@@ -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);