Always decode gif frames in sequence.

Fixes #207.

10 files changed, 78 insertions, 58 deletions

diff --git a/library/src/androidTest/java/com/bumptech/glide/load/resource/gif/GifDrawableTest.java b/library/src/androidTest/java/com/bumptech/glide/load/resource/gif/GifDrawableTest.java
index 20b1998e..3e66d83e 100644
--- a/library/src/androidTest/java/com/bumptech/glide/load/resource/gif/GifDrawableTest.java
+++ b/library/src/androidTest/java/com/bumptech/glide/load/resource/gif/GifDrawableTest.java
@@ -307,8 +307,8 @@ public class GifDrawableTest {
         Transformation<Bitmap> transformation = mock(Transformation.class);
         GifDecoder.BitmapProvider provider = mock(GifDecoder.BitmapProvider.class);
         Bitmap firstFrame = Bitmap.createBitmap(100, 100, Bitmap.Config.ARGB_8888);
-        drawable = new GifDrawable(Robolectric.application, provider, bitmapPool, transformation, 100, 100, "fakeId",
-                gifHeader, new byte[0], firstFrame);
+        drawable = new GifDrawable(Robolectric.application, provider, bitmapPool, transformation, 100, 100,  gifHeader,
+                new byte[0], firstFrame);
 
         assertNotNull(drawable.getConstantState().newDrawable());
         assertNotNull(drawable.getConstantState().newDrawable(Robolectric.application.getResources()));
diff --git a/library/src/androidTest/java/com/bumptech/glide/load/resource/gif/GifFrameManagerTest.java b/library/src/androidTest/java/com/bumptech/glide/load/resource/gif/GifFrameManagerTest.java
index ad7c8f5a..fecf2160 100644
--- a/library/src/androidTest/java/com/bumptech/glide/load/resource/gif/GifFrameManagerTest.java
+++ b/library/src/androidTest/java/com/bumptech/glide/load/resource/gif/GifFrameManagerTest.java
@@ -14,6 +14,6 @@ public class GifFrameManagerTest {
 
     @Test(expected = NullPointerException.class)
     public void testThrowsIfTransformationIsNull() {
-        new GifFrameManager(Robolectric.application, mock(GifDecoder.class), null, 100, 100, 75, 75);
+        new GifFrameManager(Robolectric.application, mock(GifDecoder.class), null, 100, 100);
     }
 }
\ No newline at end of file
diff --git a/library/src/androidTest/java/com/bumptech/glide/load/resource/gif/GifFrameModelLoaderTest.java b/library/src/androidTest/java/com/bumptech/glide/load/resource/gif/GifFrameModelLoaderTest.java
index 90bb8070..5f857c79 100644
--- a/library/src/androidTest/java/com/bumptech/glide/load/resource/gif/GifFrameModelLoaderTest.java
+++ b/library/src/androidTest/java/com/bumptech/glide/load/resource/gif/GifFrameModelLoaderTest.java
@@ -22,15 +22,13 @@ public class GifFrameModelLoaderTest {
     }
 
     @Test
-    public void testFetcherIdIncludesGifDecoderIdAndFrameIndex() {
+    public void testFetcherIdIncludesFrameIndex() {
         String id = "asdfasd";
         int frameIndex = 124;
-        when(decoder.getId()).thenReturn(id);
         when(decoder.getCurrentFrameIndex()).thenReturn(frameIndex);
 
         String fetcherId = loader.getResourceFetcher(decoder, 1, 2).getId();
 
-        assertThat(fetcherId, containsString(id));
         assertThat(fetcherId, containsString(String.valueOf(frameIndex)));
     }
 
diff --git a/library/src/androidTest/java/com/bumptech/glide/load/resource/gif/GifResourceDecoderTest.java b/library/src/androidTest/java/com/bumptech/glide/load/resource/gif/GifResourceDecoderTest.java
index 277ac0b3..8bb7c401 100644
--- a/library/src/androidTest/java/com/bumptech/glide/load/resource/gif/GifResourceDecoderTest.java
+++ b/library/src/androidTest/java/com/bumptech/glide/load/resource/gif/GifResourceDecoderTest.java
@@ -127,7 +127,7 @@ public class GifResourceDecoderTest {
 
         InOrder order = inOrder(decoderPool, gifDecoder);
         order.verify(decoderPool).obtain(any(GifDecoder.BitmapProvider.class));
-        order.verify(gifDecoder).setData(any(String.class), eq(gifHeader), eq(data));
+        order.verify(gifDecoder).setData(eq(gifHeader), eq(data));
         order.verify(gifDecoder).advance();
         order.verify(gifDecoder).getNextFrame();
         order.verify(decoderPool).release(eq(gifDecoder));
diff --git a/library/src/main/java/com/bumptech/glide/load/engine/Engine.java b/library/src/main/java/com/bumptech/glide/load/engine/Engine.java
index 822e6d32..c8c028e2 100644
--- a/library/src/main/java/com/bumptech/glide/load/engine/Engine.java
+++ b/library/src/main/java/com/bumptech/glide/load/engine/Engine.java
@@ -151,7 +151,7 @@ public class Engine implements EngineJobListener, MemoryCache.ResourceRemovedLis
             activeResources.put(key, new ResourceWeakReference(key, cached, resourceReferenceQueue));
             cb.onResourceReady(cached);
             if (Log.isLoggable(TAG, Log.VERBOSE)) {
-                Log.v(TAG, "loaded resource from cache in " + LogTime.getElapsedMillis(startTime));
+                logWithTimeAndKey("Loaded resource from cache", startTime, key);
             }
             return null;
         }
@@ -163,7 +163,7 @@ public class Engine implements EngineJobListener, MemoryCache.ResourceRemovedLis
                 active.acquire();
                 cb.onResourceReady(active);
                 if (Log.isLoggable(TAG, Log.VERBOSE)) {
-                    Log.v(TAG, "loaded resource from active resources in " + LogTime.getElapsedMillis(startTime));
+                    logWithTimeAndKey("Loaded resource from active resources", startTime, key);
                 }
                 return null;
             } else {
@@ -175,7 +175,7 @@ public class Engine implements EngineJobListener, MemoryCache.ResourceRemovedLis
         if (current != null) {
             current.addCallback(cb);
             if (Log.isLoggable(TAG, Log.VERBOSE)) {
-                Log.v(TAG, "added to existing load in " + LogTime.getElapsedMillis(startTime));
+                logWithTimeAndKey("Added to existing load", startTime, key);
             }
             return new LoadStatus(cb, current);
         }
@@ -189,11 +189,15 @@ public class Engine implements EngineJobListener, MemoryCache.ResourceRemovedLis
         engineJob.start(runnable);
 
         if (Log.isLoggable(TAG, Log.VERBOSE)) {
-            Log.v(TAG, "finished load in engine in " + LogTime.getElapsedMillis(startTime));
+            logWithTimeAndKey("Started new load", startTime, key);
         }
         return new LoadStatus(cb, engineJob);
     }
 
+    private static void logWithTimeAndKey(String log, long startTime, Key key) {
+        Log.v(TAG, log + " in " + LogTime.getElapsedMillis(startTime) + "ms, key: " + key);
+    }
+
     @SuppressWarnings("unchecked")
     private EngineResource<?> getFromCache(Key key) {
         Resource<?> cached = cache.remove(key);
diff --git a/library/src/main/java/com/bumptech/glide/load/resource/gif/GifBitmapProvider.java b/library/src/main/java/com/bumptech/glide/load/resource/gif/GifBitmapProvider.java
index be7381e5..5bfdd626 100644
--- a/library/src/main/java/com/bumptech/glide/load/resource/gif/GifBitmapProvider.java
+++ b/library/src/main/java/com/bumptech/glide/load/resource/gif/GifBitmapProvider.java
@@ -16,4 +16,11 @@ class GifBitmapProvider implements GifDecoder.BitmapProvider {
     public Bitmap obtain(int width, int height, Bitmap.Config config) {
         return bitmapPool.getDirty(width, height, config);
     }
+
+    @Override
+    public void release(Bitmap bitmap) {
+        if (!bitmapPool.put(bitmap)) {
+            bitmap.recycle();
+        }
+    }
 }
diff --git a/library/src/main/java/com/bumptech/glide/load/resource/gif/GifDrawable.java b/library/src/main/java/com/bumptech/glide/load/resource/gif/GifDrawable.java
index 1ed836c0..acf1058d 100644
--- a/library/src/main/java/com/bumptech/glide/load/resource/gif/GifDrawable.java
+++ b/library/src/main/java/com/bumptech/glide/load/resource/gif/GifDrawable.java
@@ -63,24 +63,23 @@ public class GifDrawable extends GlideDrawable implements GifFrameManager.FrameC
      *                         {@link com.bumptech.glide.request.target.Target} this drawable is being loaded into).
      * @param targetFrameHeight The desired height of the frames displayed by this drawable (the height of the view or
      *                          {@link com.bumptech.glide.request.target.Target} this drawable is being loaded into).
-     * @param id An id that uniquely identifies this particular gif.
      * @param gifHeader The header data for this gif.
      * @param data The full bytes of the gif.
      * @param firstFrame The decoded and transformed first frame of this gif.
      */
     public GifDrawable(Context context, GifDecoder.BitmapProvider bitmapProvider, BitmapPool bitmapPool,
-            Transformation<Bitmap> frameTransformation, int targetFrameWidth, int targetFrameHeight, String id,
+            Transformation<Bitmap> frameTransformation, int targetFrameWidth, int targetFrameHeight,
             GifHeader gifHeader, byte[] data, Bitmap firstFrame) {
-        this(new GifState(id, gifHeader, data, context, frameTransformation, targetFrameWidth, targetFrameHeight,
+        this(new GifState(gifHeader, data, context, frameTransformation, targetFrameWidth, targetFrameHeight,
                 bitmapProvider, bitmapPool, firstFrame));
     }
 
     GifDrawable(GifState state) {
         this.state = state;
         this.decoder = new GifDecoder(state.bitmapProvider);
-        decoder.setData(state.id, state.gifHeader, state.data);
+        decoder.setData(state.gifHeader, state.data);
         frameManager = new GifFrameManager(state.context, decoder, state.frameTransformation, state.targetWidth,
-                state.targetHeight, state.firstFrame.getWidth(), state.firstFrame.getHeight());
+                state.targetHeight);
     }
 
     // Visible for testing.
@@ -284,7 +283,6 @@ public class GifDrawable extends GlideDrawable implements GifFrameManager.FrameC
 
     static class GifState extends ConstantState {
         private static final int GRAVITY = Gravity.FILL;
-        String id;
         GifHeader gifHeader;
         byte[] data;
         Context context;
@@ -295,10 +293,9 @@ public class GifDrawable extends GlideDrawable implements GifFrameManager.FrameC
         BitmapPool bitmapPool;
         Bitmap firstFrame;
 
-        public GifState(String id, GifHeader header, byte[] data, Context context,
+        public GifState(GifHeader header, byte[] data, Context context,
                 Transformation<Bitmap> frameTransformation, int targetWidth, int targetHeight,
                 GifDecoder.BitmapProvider provider, BitmapPool bitmapPool, Bitmap firstFrame) {
-            this.id = id;
             gifHeader = header;
             this.data = data;
             this.bitmapPool = bitmapPool;
@@ -312,7 +309,6 @@ public class GifDrawable extends GlideDrawable implements GifFrameManager.FrameC
 
         public GifState(GifState original) {
             if (original != null) {
-                id = original.id;
                 gifHeader = original.gifHeader;
                 data = original.data;
                 context = original.context;
diff --git a/library/src/main/java/com/bumptech/glide/load/resource/gif/GifFrameManager.java b/library/src/main/java/com/bumptech/glide/load/resource/gif/GifFrameManager.java
index 3c56fdf4..42eea5d0 100644
--- a/library/src/main/java/com/bumptech/glide/load/resource/gif/GifFrameManager.java
+++ b/library/src/main/java/com/bumptech/glide/load/resource/gif/GifFrameManager.java
@@ -9,19 +9,21 @@ import android.os.SystemClock;
 import com.bumptech.glide.Glide;
 import com.bumptech.glide.gifdecoder.GifDecoder;
 import com.bumptech.glide.load.Encoder;
+import com.bumptech.glide.load.Key;
 import com.bumptech.glide.load.Transformation;
 import com.bumptech.glide.load.engine.DiskCacheStrategy;
 import com.bumptech.glide.load.engine.bitmap_recycle.BitmapPool;
-import com.bumptech.glide.load.engine.cache.MemorySizeCalculator;
 import com.bumptech.glide.load.resource.NullEncoder;
 import com.bumptech.glide.request.animation.GlideAnimation;
 import com.bumptech.glide.request.target.SimpleTarget;
-import com.bumptech.glide.util.Util;
+
+import java.io.UnsupportedEncodingException;
+import java.security.MessageDigest;
+import java.util.UUID;
 
 class GifFrameManager {
     /** 60fps is {@value #MIN_FRAME_DELAY}ms per frame. */
     private static final long MIN_FRAME_DELAY = 1000 / 60;
-    private final MemorySizeCalculator calculator;
     private final GifFrameModelLoader frameLoader;
     private final GifFrameResourceDecoder frameResourceDecoder;
     private final GifDecoder decoder;
@@ -31,7 +33,7 @@ class GifFrameManager {
     private final Transformation<Bitmap>[] transformation;
     private final int targetWidth;
     private final int targetHeight;
-    private final int totalFrameSize;
+    private final FrameSignature signature;
     private DelayTarget current;
     private DelayTarget next;
 
@@ -40,14 +42,14 @@ class GifFrameManager {
     }
 
     public GifFrameManager(Context context, GifDecoder decoder, Transformation<Bitmap> transformation, int targetWidth,
-            int targetHeight, int frameWidth, int frameHeight) {
+            int targetHeight) {
         this(context, Glide.get(context).getBitmapPool(), decoder, new Handler(Looper.getMainLooper()), transformation,
-                targetWidth, targetHeight, frameWidth, frameHeight);
+                targetWidth, targetHeight);
     }
 
     @SuppressWarnings("unchecked")
     public GifFrameManager(Context context, BitmapPool bitmapPool, GifDecoder decoder, Handler mainHandler,
-            Transformation<Bitmap> transformation, int targetWidth, int targetHeight, int frameWidth, int frameHeight) {
+            Transformation<Bitmap> transformation, int targetWidth, int targetHeight) {
         if (transformation == null) {
             throw new NullPointerException("Transformation must not be null");
         }
@@ -59,11 +61,9 @@ class GifFrameManager {
         this.transformation = new Transformation[] {transformation};
         this.targetWidth = targetWidth;
         this.targetHeight = targetHeight;
-        this.totalFrameSize = Util.getBitmapByteSize(frameWidth, frameHeight, Bitmap.Config.ARGB_8888);
-
-        this.calculator = new MemorySizeCalculator(context);
         this.frameLoader = new GifFrameModelLoader();
         this.sourceEncoder = NullEncoder.get();
+        this.signature = new FrameSignature();
     }
 
     Transformation<Bitmap> getTransformation() {
@@ -73,22 +73,21 @@ class GifFrameManager {
     public void getNextFrame(FrameCallback cb) {
         decoder.advance();
 
-        // We don't want to blow out the entire memory cache with frames of gifs, so try to set some
-        // maximum size beyond which we will always just decode one frame at a time.
-        boolean skipCache = totalFrameSize > calculator.getMemoryCacheSize() / 2;
-
         long targetTime = SystemClock.uptimeMillis() + Math.max(MIN_FRAME_DELAY, decoder.getNextDelay());
         next = new DelayTarget(cb, targetTime);
         next.setFrameIndex(decoder.getCurrentFrameIndex());
 
+        // Use an incrementing signature to make sure we never hit an active resource that matches one of our frames.
+        signature.increment();
         Glide.with(context)
                 .using(frameLoader, GifDecoder.class)
                 .load(decoder)
                 .as(Bitmap.class)
+                .signature(signature)
                 .sourceEncoder(sourceEncoder)
                 .decoder(frameResourceDecoder)
                 .transform(transformation)
-                .skipMemoryCache(skipCache)
+                .skipMemoryCache(true)
                 .diskCacheStrategy(DiskCacheStrategy.NONE)
                 .into(next);
     }
@@ -144,4 +143,38 @@ class GifFrameManager {
             resource = null;
         }
     }
+
+    private static class FrameSignature implements Key {
+        private final UUID uuid;
+        private int id;
+
+        public FrameSignature() {
+            this.uuid = UUID.randomUUID();
+        }
+
+        public void increment() {
+            id++;
+        }
+
+        @Override
+        public boolean equals(Object o) {
+            if (o instanceof FrameSignature) {
+                FrameSignature other = (FrameSignature) o;
+                return other.uuid.equals(uuid) && id == other.id;
+            }
+            return false;
+        }
+
+        @Override
+        public int hashCode() {
+            int result = uuid.hashCode();
+            result = 31 * result + id;
+            return result;
+        }
+
+        @Override
+        public void updateDiskCacheKey(MessageDigest messageDigest) throws UnsupportedEncodingException {
+            throw new UnsupportedOperationException("Not implemented");
+        }
+    }
 }
diff --git a/library/src/main/java/com/bumptech/glide/load/resource/gif/GifFrameModelLoader.java b/library/src/main/java/com/bumptech/glide/load/resource/gif/GifFrameModelLoader.java
index b9c17c20..4db390f9 100644
--- a/library/src/main/java/com/bumptech/glide/load/resource/gif/GifFrameModelLoader.java
+++ b/library/src/main/java/com/bumptech/glide/load/resource/gif/GifFrameModelLoader.java
@@ -31,7 +31,7 @@ class GifFrameModelLoader implements ModelLoader<GifDecoder, GifDecoder> {
 
         @Override
         public String getId() {
-            return decoder.getId() + decoder.getCurrentFrameIndex();
+            return String.valueOf(decoder.getCurrentFrameIndex());
         }
 
         @Override
diff --git a/library/src/main/java/com/bumptech/glide/load/resource/gif/GifResourceDecoder.java b/library/src/main/java/com/bumptech/glide/load/resource/gif/GifResourceDecoder.java
index cf56edfb..f4cbb1e2 100644
--- a/library/src/main/java/com/bumptech/glide/load/resource/gif/GifResourceDecoder.java
+++ b/library/src/main/java/com/bumptech/glide/load/resource/gif/GifResourceDecoder.java
@@ -17,10 +17,7 @@ import com.bumptech.glide.util.Util;
 import java.io.ByteArrayOutputStream;
 import java.io.IOException;
 import java.io.InputStream;
-import java.security.MessageDigest;
-import java.security.NoSuchAlgorithmException;
 import java.util.Queue;
-import java.util.UUID;
 
 /**
  * An {@link com.bumptech.glide.load.ResourceDecoder} that decodes
@@ -87,18 +84,17 @@ public class GifResourceDecoder implements ResourceDecoder<InputStream, GifDrawa
             return null;
         }
 
-        String id = getGifId(data);
-        Bitmap firstFrame = decodeFirstFrame(decoder, id, header, data);
+        Bitmap firstFrame = decodeFirstFrame(decoder, header, data);
         Transformation<Bitmap> unitTransformation = UnitTransformation.get();
 
-        GifDrawable gifDrawable = new GifDrawable(context, provider, bitmapPool, unitTransformation, width, height, id,
+        GifDrawable gifDrawable = new GifDrawable(context, provider, bitmapPool, unitTransformation, width, height,
                 header, data, firstFrame);
 
         return new GifDrawableResource(gifDrawable);
     }
 
-    private Bitmap decodeFirstFrame(GifDecoder decoder, String id, GifHeader header, byte[] data) {
-        decoder.setData(id, header, data);
+    private Bitmap decodeFirstFrame(GifDecoder decoder, GifHeader header, byte[] data) {
+        decoder.setData(header, data);
         decoder.advance();
         return decoder.getNextFrame();
     }
@@ -108,20 +104,6 @@ public class GifResourceDecoder implements ResourceDecoder<InputStream, GifDrawa
         return "";
     }
 
-    // A best effort attempt to get a unique id that can be used as a cache key for frames of the decoded GIF.
-    private static String getGifId(byte[] data) {
-        try {
-            MessageDigest digest = MessageDigest.getInstance("SHA-1");
-            digest.update(data);
-            return Util.sha1BytesToHex(digest.digest());
-        } catch (NoSuchAlgorithmException e) {
-            if (Log.isLoggable(TAG, Log.WARN)) {
-                Log.w(TAG, "Missing sha1 algorithm?", e);
-            }
-        }
-        return UUID.randomUUID().toString();
-    }
-
     private static byte[] inputStreamToBytes(InputStream is) {
         final int bufferSize = 16384;
         ByteArrayOutputStream buffer = new ByteArrayOutputStream(bufferSize);