Do not cache images if they are of unknown format

Summary: Fixes #1413 and #675

Reviewed By: oprisnik

Differential Revision: D8780521

fbshipit-source-id: efec5b689ce696c2132771d70fe3533ea4597680

imagepipeline/src/main/java/com/facebook/imagepipeline/producers/DiskCacheWriteProducer.java

@@ -9,6 +9,7 @@
 
 import com.facebook.cache.common.CacheKey;
 import com.facebook.common.internal.VisibleForTesting;
+import com.facebook.imageformat.ImageFormat;
 import com.facebook.imagepipeline.cache.BufferedDiskCache;
 import com.facebook.imagepipeline.cache.CacheKeyFactory;
 import com.facebook.imagepipeline.image.EncodedImage;
@@ -106,8 +107,11 @@ private DiskCacheWriteConsumer(
     @Override
     public void onNewResultImpl(EncodedImage newResult, @Status int status) {
       // intermediate, null or uncacheable results are not cached, so we just forward them
-      if (isNotLast(status) || newResult == null ||
-          statusHasAnyFlag(status, DO_NOT_CACHE_ENCODED | IS_PARTIAL_RESULT)) {
+      // as well as the images with unknown format which could be html response from the server
+      if (isNotLast(status)
+          || newResult == null
+          || statusHasAnyFlag(status, DO_NOT_CACHE_ENCODED | IS_PARTIAL_RESULT)
+          || newResult.getImageFormat() == ImageFormat.UNKNOWN) {
         getConsumer().onNewResult(newResult, status);
         return;
       }

imagepipeline/src/main/java/com/facebook/imagepipeline/producers/PartialDiskCacheProducer.java

@@ -20,6 +20,7 @@
 import com.facebook.common.memory.PooledByteBufferOutputStream;
 import com.facebook.common.references.CloseableReference;
 import com.facebook.common.util.ByteConstants;
+import com.facebook.imageformat.ImageFormat;
 import com.facebook.imagepipeline.cache.BufferedDiskCache;
 import com.facebook.imagepipeline.cache.CacheKeyFactory;
 import com.facebook.imagepipeline.common.BytesRange;
@@ -277,7 +278,9 @@ public void onNewResultImpl(EncodedImage newResult, @Status int status) {
         }
 
         mDefaultBufferedDiskCache.remove(mPartialImageCacheKey);
-      } else if (statusHasFlag(status, IS_PARTIAL_RESULT) && isLast(status)) {
+      } else if (statusHasFlag(status, IS_PARTIAL_RESULT)
+          && isLast(status)
+          && newResult.getImageFormat() != ImageFormat.UNKNOWN) {
         mDefaultBufferedDiskCache.put(mPartialImageCacheKey, newResult);
         getConsumer().onNewResult(newResult, status);
       } else {

imagepipeline/src/test/java/com/facebook/imagepipeline/producers/DiskCacheWriteProducerTest.java

@@ -23,6 +23,7 @@
 import com.facebook.cache.common.SimpleCacheKey;
 import com.facebook.common.memory.PooledByteBuffer;
 import com.facebook.common.references.CloseableReference;
+import com.facebook.imageformat.ImageFormat;
 import com.facebook.imagepipeline.cache.BufferedDiskCache;
 import com.facebook.imagepipeline.cache.CacheKeyFactory;
 import com.facebook.imagepipeline.common.Priority;
@@ -74,6 +75,7 @@ public class DiskCacheWriteProducerTest {
   private CloseableReference<PooledByteBuffer> mIntermediateImageReference;
   private CloseableReference<PooledByteBuffer> mFinalImageReference;
   private EncodedImage mIntermediateEncodedImage;
+  private EncodedImage mFinalEncodedImageFormatUnknown;
   private EncodedImage mFinalEncodedImage;
   private DiskCacheWriteProducer mDiskCacheWriteProducer;
 
@@ -94,7 +96,10 @@ public void setUp() {
     mIntermediateImageReference = CloseableReference.of(mIntermediatePooledByteBuffer);
     mFinalImageReference = CloseableReference.of(mFinalPooledByteBuffer);
     mIntermediateEncodedImage = new EncodedImage(mIntermediateImageReference);
+    mFinalEncodedImageFormatUnknown = new EncodedImage(mFinalImageReference);
+
     mFinalEncodedImage = new EncodedImage(mFinalImageReference);
+    mFinalEncodedImage.setImageFormat(new ImageFormat("jpeg", null));
 
     mProducerContext = new SettableProducerContext(
         mImageRequest,
@@ -148,15 +153,29 @@ public void testSmallImageDiskCacheInputProducerSuccess() {
     verifyZeroInteractions(mProducerListener);
   }
 
+  @Test
+  public void testSmallImageDiskCacheInputProducerUnknownFormat() {
+    when(mImageRequest.getCacheChoice()).thenReturn(ImageRequest.CacheChoice.SMALL);
+    setupInputProducerSuccessFormatUnknown();
+    mDiskCacheWriteProducer.produceResults(mConsumer, mProducerContext);
+    verify(mSmallImageBufferedDiskCache, never()).put(mCacheKey, mIntermediateEncodedImage);
+    verify(mSmallImageBufferedDiskCache, never()).put(mCacheKey, mFinalEncodedImageFormatUnknown);
+    verify(mConsumer).onNewResult(mIntermediateEncodedImage, Consumer.NO_FLAGS);
+    verify(mConsumer).onNewResult(mFinalEncodedImageFormatUnknown, Consumer.IS_LAST);
+    verifyZeroInteractions(mProducerListener);
+  }
+
   @Test
   public void testDoesNotWriteToCacheIfPartialResult() {
-    setupInputProducerSuccessWithStatusFlags(Consumer.IS_PARTIAL_RESULT);
+    setupInputProducerSuccessWithStatusFlags(
+        Consumer.IS_PARTIAL_RESULT, mFinalEncodedImageFormatUnknown);
 
     mDiskCacheWriteProducer.produceResults(mConsumer, mProducerContext);
 
     verify(mConsumer).onNewResult(mIntermediateEncodedImage, Consumer.IS_PARTIAL_RESULT);
     verify(mConsumer)
-        .onNewResult(mFinalEncodedImage, Consumer.IS_LAST | Consumer.IS_PARTIAL_RESULT);
+        .onNewResult(
+            mFinalEncodedImageFormatUnknown, Consumer.IS_LAST | Consumer.IS_PARTIAL_RESULT);
 
     verifyZeroInteractions(mDefaultBufferedDiskCache, mSmallImageBufferedDiskCache);
   }
@@ -186,10 +205,10 @@ public void testInputProducerFailure() {
   @Test
   public void testDoesNotWriteResultToCacheIfNotEnabled() {
     when(mImageRequest.isDiskCacheEnabled()).thenReturn(false);
-    setupInputProducerSuccess();
+    setupInputProducerSuccessFormatUnknown();
     mDiskCacheWriteProducer.produceResults(mConsumer, mProducerContext);
     verify(mConsumer).onNewResult(mIntermediateEncodedImage, Consumer.NO_FLAGS);
-    verify(mConsumer).onNewResult(mFinalEncodedImage, Consumer.IS_LAST);
+    verify(mConsumer).onNewResult(mFinalEncodedImageFormatUnknown, Consumer.IS_LAST);
     verifyNoMoreInteractions(
         mProducerListener,
         mCacheKeyFactory,
@@ -199,11 +218,13 @@ public void testDoesNotWriteResultToCacheIfNotEnabled() {
 
   @Test
   public void testDoesNotWriteResultToCacheIfResultStatusSaysNotTo() {
-    setupInputProducerSuccessWithStatusFlags(Consumer.DO_NOT_CACHE_ENCODED);
+    setupInputProducerSuccessWithStatusFlags(
+        Consumer.DO_NOT_CACHE_ENCODED, mFinalEncodedImageFormatUnknown);
     mDiskCacheWriteProducer.produceResults(mConsumer, mProducerContext);
     verify(mConsumer).onNewResult(mIntermediateEncodedImage, Consumer.DO_NOT_CACHE_ENCODED);
     verify(mConsumer)
-        .onNewResult(mFinalEncodedImage, Consumer.IS_LAST | Consumer.DO_NOT_CACHE_ENCODED);
+        .onNewResult(
+            mFinalEncodedImageFormatUnknown, Consumer.IS_LAST | Consumer.DO_NOT_CACHE_ENCODED);
     verifyNoMoreInteractions(
         mProducerListener,
         mCacheKeyFactory,
@@ -223,21 +244,28 @@ public void testLowestLevelReached() {
         mProducerListener);
   }
 
+  private void setupInputProducerSuccessFormatUnknown() {
+    setupInputProducerSuccessWithStatusFlags(0, mFinalEncodedImageFormatUnknown);
+  }
+
   private void setupInputProducerSuccess() {
-    setupInputProducerSuccessWithStatusFlags(0);
+    setupInputProducerSuccessWithStatusFlags(0, mFinalEncodedImage);
   }
 
-  private void setupInputProducerSuccessWithStatusFlags(final @Consumer.Status int statusFlags) {
+  private void setupInputProducerSuccessWithStatusFlags(
+      final @Consumer.Status int statusFlags, final EncodedImage finalEncodedImage) {
     doAnswer(
-        new Answer<Object>() {
-          @Override
-          public Object answer(InvocationOnMock invocation) throws Throwable {
-            Consumer consumer = (Consumer) invocation.getArguments()[0];
-            consumer.onNewResult(mIntermediateEncodedImage, Consumer.NO_FLAGS | statusFlags);
-            consumer.onNewResult(mFinalEncodedImage, Consumer.IS_LAST | statusFlags);
-            return null;
-          }
-        }).when(mInputProducer).produceResults(any(Consumer.class), eq(mProducerContext));
+            new Answer<Object>() {
+              @Override
+              public Object answer(InvocationOnMock invocation) throws Throwable {
+                Consumer consumer = (Consumer) invocation.getArguments()[0];
+                consumer.onNewResult(mIntermediateEncodedImage, Consumer.NO_FLAGS | statusFlags);
+                consumer.onNewResult(finalEncodedImage, Consumer.IS_LAST | statusFlags);
+                return null;
+              }
+            })
+        .when(mInputProducer)
+        .produceResults(any(Consumer.class), eq(mProducerContext));
   }
 
   private void setupInputProducerFailure() {