Return valid bitmap references for FrescoFrameCache

Summary:
We cannot use `CloseableStaticBitmap.convertToBitmapReference()` to create the `CloseableReference<Bitmap>` from a `CloseableReference<CloseableImage>`.
It works fine if prefetching is disabled (if we do not hold on to the references), but if prefetching is enabled and we hold on to the bitmaps until they are first drawn (see stacked diff), this does NOT work correctly.
The reason for this is the implementations of `CountingMemoryCache` and `CountingLruMap` make use of `CloseableImage.getSizeInBytes()` to update their respective sizes in bytes.
`CloseableStaticBitmap.convertToBitmapReference()` will detach the underlying bitmap from the `CloseableStaticBitmap`, which is used to calculate the size in bytes. Hence, the size will be reported as 0 when the elements are changed and the cache cannot correctly decrease its size. This means that the cache thinks its size is constantly increasing until it will eventually stop caching new items - which will break all other images as well.

The solution to this problem is to not detach the bitmap reference from the bitmap. Instead, I am just returning a new `CloseableReference` with the underlying bitmap with a custom `ResourceReleaser` that will close the `CloseableImage` when appropriate, which will result in correct computations.

Reviewed By: kirwan

Differential Revision: D4914167

fbshipit-source-id: b34ce0b3bff0a6b3e93e0c7d15379cb2f2872f1a

animated-drawable/src/main/java/com/facebook/fresco/animation/bitmap/cache/FrescoFrameCache.java

@@ -16,6 +16,7 @@
 import com.facebook.common.internal.Preconditions;
 import com.facebook.common.internal.VisibleForTesting;
 import com.facebook.common.references.CloseableReference;
+import com.facebook.common.references.ResourceReleaser;
 import com.facebook.fresco.animation.bitmap.BitmapAnimationBackend;
 import com.facebook.fresco.animation.bitmap.BitmapFrameCache;
 import com.facebook.imagepipeline.animated.impl.AnimatedFrameCache;
@@ -45,13 +46,13 @@ public FrescoFrameCache(AnimatedFrameCache animatedFrameCache, boolean enableBit
  @Nullable
  @Override
  public synchronized CloseableReference<Bitmap> getCachedFrame(int frameNumber) {
- return extractAndClose(mAnimatedFrameCache.get(frameNumber));
+ return convertToBitmapReference(mAnimatedFrameCache.get(frameNumber));
  }
 
  @Nullable
  @Override
  public synchronized CloseableReference<Bitmap> getFallbackFrame(int frameNumber) {
- return extractAndClose(CloseableReference.cloneOrNull(mLastCachedItem));
+ return convertToBitmapReference(CloseableReference.cloneOrNull(mLastCachedItem));
  }
 
  @Nullable
@@ -63,7 +64,7 @@ public synchronized CloseableReference<Bitmap> getBitmapToReuseForFrame(
  if (!mEnableBitmapReusing) {
  return null;
  }
- return extractAndClose(mAnimatedFrameCache.getForReuse());
+ return convertToBitmapReference(mAnimatedFrameCache.getForReuse());
  }
 
  @Override
@@ -121,20 +122,31 @@ public void setFrameCacheListener(FrameCacheListener frameCacheListener) {
 
  @VisibleForTesting
  @Nullable
- static CloseableReference<Bitmap> extractAndClose(
- @Nullable CloseableReference<CloseableImage> closeableImage) {
- try {
- if (CloseableReference.isValid(closeableImage) &&
- closeableImage.get() instanceof CloseableStaticBitmap) {
- CloseableStaticBitmap closeableStaticBitmap = (CloseableStaticBitmap) closeableImage.get();
- if (closeableStaticBitmap != null && !closeableStaticBitmap.isClosed()) {
- return closeableStaticBitmap.convertToBitmapReference();
- }
+ static CloseableReference<Bitmap> convertToBitmapReference(
+ final @Nullable CloseableReference<CloseableImage> closeableImage) {
+ if (CloseableReference.isValid(closeableImage) &&
+ closeableImage.get() instanceof CloseableStaticBitmap) {
+ CloseableStaticBitmap closeableStaticBitmap = (CloseableStaticBitmap) closeableImage.get();
+ if (closeableStaticBitmap != null && !closeableStaticBitmap.isClosed()) {
+ // For better performance, we directly use the given closeable image and dont clone it.
+ // NOTE: closeableStaticBitmap.convertToBitmapReference() cannot be used here
+ // since this would detach the underlying reference from the CloseableImage
+ // which is required for correct size computations in CountingMemoryCache.
+ // If convertToBitmapReference() is used, the cache cannot correctly compute the
+ // size in bytes, which will slowly increase until no more bitmaps can be cached.
+ return CloseableReference.of(
+ closeableStaticBitmap.getUnderlyingBitmap(),
+ new ResourceReleaser<Bitmap>() {
+ @Override
+ public void release(Bitmap value) {
+ CloseableReference.closeSafely(closeableImage);
+ }
+ });
  }
- return null;
- } finally {
- CloseableReference.closeSafely(closeableImage);
  }
+ // Not a bitmap reference, so we have to close right away.
+ CloseableReference.closeSafely(closeableImage);
+ return null;
  }
 
  private static int getBitmapSizeBytes(

animated-drawable/src/test/java/com/facebook/fresco/animation/bitmap/cache/FrescoFrameCacheTest.java

@@ -22,6 +22,7 @@
 import org.robolectric.RobolectricTestRunner;
 
 import static org.fest.assertions.api.Assertions.assertThat;
+import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.verifyZeroInteractions;
 import static org.mockito.Mockito.when;
@@ -55,9 +56,14 @@ public void setup() {
  @Test
  public void testExtractAndClose() throws Exception {
  CloseableReference<Bitmap> extractedReference =
- FrescoFrameCache.extractAndClose(mImageReference);
+ FrescoFrameCache.convertToBitmapReference(mImageReference);
+
+ assertThat(extractedReference).isNotNull();
+ assertThat(extractedReference.get()).isEqualTo(mUnderlyingBitmap);
+ verify(mImageReference, never()).close();
+
+ extractedReference.close();
 
- assertThat(extractedReference).isEqualTo(mBitmapReference);
  verify(mImageReference).close();
  }
 
@@ -66,10 +72,14 @@ public void testExtractAndClose_whenBitmapRecycled_thenReturnReference() throws
  when(mUnderlyingBitmap.isRecycled()).thenReturn(true);
 
  CloseableReference<Bitmap> extractedReference =
- FrescoFrameCache.extractAndClose(mImageReference);
+ FrescoFrameCache.convertToBitmapReference(mImageReference);
 
  // We only detach the reference and do not care if the bitmap is valid
- assertThat(extractedReference).isEqualTo(mBitmapReference);
+ assertThat(extractedReference).isNotNull();
+ assertThat(extractedReference.get()).isEqualTo(mUnderlyingBitmap);
+
+ extractedReference.close();
+
  verify(mImageReference).close();
  }
 
@@ -79,10 +89,14 @@ public void testExtractAndClose_whenBitmapReferenceInvalid_thenReturnReference()
  when(mBitmapReference.isValid()).thenReturn(false);
 
  CloseableReference<Bitmap> extractedReference =
- FrescoFrameCache.extractAndClose(mImageReference);
+ FrescoFrameCache.convertToBitmapReference(mImageReference);
 
  // We only detach the reference and do not care if the bitmap reference is valid
- assertThat(extractedReference).isEqualTo(mBitmapReference);
+ assertThat(extractedReference).isNotNull();
+ assertThat(extractedReference.get()).isEqualTo(mUnderlyingBitmap);
+
+ extractedReference.close();
+
  verify(mImageReference).close();
  }
 
@@ -92,7 +106,7 @@ public void testExtractAndClose_whenCloseableStaticBitmapClosed_thenReturnNull()
  when(mCloseableStaticBitmap.isClosed()).thenReturn(true);
 
  CloseableReference<Bitmap> extractedReference =
- FrescoFrameCache.extractAndClose(mImageReference);
+ FrescoFrameCache.convertToBitmapReference(mImageReference);
 
  // We only detach the reference and do not care if the bitmap is valid
  assertThat(extractedReference).isNull();
@@ -104,7 +118,7 @@ public void testExtractAndClose_whenImageReferenceInvalid_thenReturnNull() throw
  when(mImageReference.isValid()).thenReturn(false);
 
  CloseableReference<Bitmap> extractedReference =
- FrescoFrameCache.extractAndClose(mImageReference);
+ FrescoFrameCache.convertToBitmapReference(mImageReference);
 
  // We only detach the reference and do not care if the bitmap is valid
  assertThat(extractedReference).isNull();
@@ -114,29 +128,18 @@ public void testExtractAndClose_whenImageReferenceInvalid_thenReturnNull() throw
  @Test
  public void testExtractAndClose_whenInputNull_thenReturnNull() throws Exception {
  CloseableReference<Bitmap> extractedReference =
- FrescoFrameCache.extractAndClose(null);
+ FrescoFrameCache.convertToBitmapReference(null);
 
  assertThat(extractedReference).isNull();
  verifyZeroInteractions(mImageReference);
  }
 
- @Test
- public void testExtractAndClose_whenBitmapReferenceNull_thenReturnNull() throws Exception {
- when(mCloseableStaticBitmap.convertToBitmapReference()).thenReturn(null);
-
- CloseableReference<Bitmap> extractedReference =
- FrescoFrameCache.extractAndClose(mImageReference);
-
- assertThat(extractedReference).isNull();
- verify(mImageReference).close();
- }
-
  @Test
  public void testExtractAndClose_whenCloseableStaticBitmapNull_thenReturnNull() throws Exception {
  when(mImageReference.get()).thenReturn(null);
 
  CloseableReference<Bitmap> extractedReference =
- FrescoFrameCache.extractAndClose(mImageReference);
+ FrescoFrameCache.convertToBitmapReference(mImageReference);
 
  assertThat(extractedReference).isNull();
  verify(mImageReference).close();