Improved threading for image loader

Summary: public

The image loader was previously returning on the main thread, which could lead to poor performance due to various call sites doing further image processing (resizing, cropping, etc.) directly in the completion block.

This diff modifies the loader to return on a background thread (the same one used to load the image), and updates the call sites to dispatch to the explicit thread they need.

Reviewed By: javache

Differential Revision: D2549774

fb-gh-sync-id: fed73b7c163fdf67ff65bae72ab1986327e75815

Libraries/CameraRoll/RCTAssetsLibraryImageLoader.h

@@ -15,11 +15,6 @@
 
 @interface RCTBridge (RCTAssetsLibraryImageLoader)
 
-/**
- * The shared Assets Library image loader
- */
-@property (nonatomic, readonly) RCTAssetsLibraryImageLoader *assetsLibraryImageLoader;
-
 /**
  * The shared asset library instance.
  */

Libraries/CameraRoll/RCTAssetsLibraryImageLoader.m

@@ -41,10 +41,15 @@ RCT_EXPORT_MODULE()
 
 - (BOOL)canLoadImageURL:(NSURL *)requestURL
 {
-  return [requestURL.scheme.lowercaseString isEqualToString:@"assets-library"];
+  return [requestURL.scheme caseInsensitiveCompare:@"assets-library"] == NSOrderedSame;
 }
 
-- (RCTImageLoaderCancellationBlock)loadImageForURL:(NSURL *)imageURL size:(CGSize)size scale:(CGFloat)scale resizeMode:(UIViewContentMode)resizeMode progressHandler:(RCTImageLoaderProgressBlock)progressHandler completionHandler:(RCTImageLoaderCompletionBlock)completionHandler
+- (RCTImageLoaderCancellationBlock)loadImageForURL:(NSURL *)imageURL
+                                              size:(CGSize)size
+                                             scale:(CGFloat)scale
+                                        resizeMode:(UIViewContentMode)resizeMode
+                                   progressHandler:(RCTImageLoaderProgressBlock)progressHandler
+                                 completionHandler:(RCTImageLoaderCompletionBlock)completionHandler
 {
   __block volatile uint32_t cancelled = 0;
 
@@ -69,7 +74,8 @@ RCT_EXPORT_MODULE()
           BOOL useMaximumSize = CGSizeEqualToSize(size, CGSizeZero);
           ALAssetRepresentation *representation = [asset defaultRepresentation];
 
-          #if RCT_DEV
+#if RCT_DEV
+
           CGSize sizeBeingLoaded = size;
           if (useMaximumSize) {
             CGSize pointSize = representation.dimensions;
@@ -78,7 +84,7 @@ RCT_EXPORT_MODULE()
 
           CGSize screenSize;
           if ([[[UIDevice currentDevice] systemVersion] compare:@"8.0" options:NSNumericSearch] == NSOrderedDescending) {
-            screenSize = UIScreen.mainScreen.nativeBounds.size;
+            screenSize = [UIScreen mainScreen].nativeBounds.size;
           } else {
             CGSize mainScreenSize = [UIScreen mainScreen].bounds.size;
             CGFloat mainScreenScale = [[UIScreen mainScreen] scale];
@@ -87,9 +93,11 @@ RCT_EXPORT_MODULE()
           CGFloat maximumPixelDimension = fmax(screenSize.width, screenSize.height);
 
           if (sizeBeingLoaded.width > maximumPixelDimension || sizeBeingLoaded.height > maximumPixelDimension) {
-            RCTLogInfo(@"[PERF ASSETS] Loading %@ at size %@, which is larger than screen size %@", representation.filename, NSStringFromCGSize(sizeBeingLoaded), NSStringFromCGSize(screenSize));
+            RCTLogInfo(@"[PERF ASSETS] Loading %@ at size %@, which is larger than screen size %@",
+                       representation.filename, NSStringFromCGSize(sizeBeingLoaded), NSStringFromCGSize(screenSize));
           }
-          #endif
+
+#endif
 
           UIImage *image;
           NSError *error = nil;
@@ -106,8 +114,7 @@ RCT_EXPORT_MODULE()
       });
     } else {
       NSString *errorText = [NSString stringWithFormat:@"Failed to load asset at URL %@ with no error message.", imageURL];
-      NSError *error = RCTErrorWithMessage(errorText);
+      completionHandler(RCTErrorWithMessage(errorText), nil);
-      completionHandler(error, nil);
     }
   } failureBlock:^(NSError *loadError) {
     if (cancelled) {
@@ -115,8 +122,7 @@ RCT_EXPORT_MODULE()
     }
 
     NSString *errorText = [NSString stringWithFormat:@"Failed to load asset at URL %@.\niOS Error: %@", imageURL, loadError];
-    NSError *error = RCTErrorWithMessage(errorText);
+    completionHandler(RCTErrorWithMessage(errorText), nil);
-    completionHandler(error, nil);
   }];
 
   return ^{
@@ -128,14 +134,9 @@ RCT_EXPORT_MODULE()
 
 @implementation RCTBridge (RCTAssetsLibraryImageLoader)
 
-- (RCTAssetsLibraryImageLoader *)assetsLibraryImageLoader
-{
-  return self.modules[RCTBridgeModuleNameForClass([RCTAssetsLibraryImageLoader class])];
-}
-
 - (ALAssetsLibrary *)assetsLibrary
 {
-  return [self.assetsLibraryImageLoader assetsLibrary];
+  return [self.modules[RCTBridgeModuleNameForClass([RCTAssetsLibraryImageLoader class])] assetsLibrary];
 }
 
 @end
@@ -154,7 +155,11 @@ static dispatch_queue_t RCTAssetsLibraryImageLoaderQueue(void)
 // Why use a custom scaling method? Greater efficiency, reduced memory overhead:
 // http://www.mindsea.com/2012/12/downscaling-huge-alassets-without-fear-of-sigkill
 
-static UIImage *RCTScaledImageForAsset(ALAssetRepresentation *representation, CGSize size, CGFloat scale, UIViewContentMode resizeMode, NSError **error)
+static UIImage *RCTScaledImageForAsset(ALAssetRepresentation *representation,
+                                       CGSize size,
+                                       CGFloat scale,
+                                       UIViewContentMode resizeMode,
+                                       NSError **error)
 {
   NSUInteger length = (NSUInteger)representation.size;
   NSMutableData *data = [NSMutableData dataWithLength:length];

Libraries/CameraRoll/RCTCameraRollManager.m

@@ -35,14 +35,17 @@ RCT_EXPORT_METHOD(saveImageWithTag:(NSString *)imageTag
       errorCallback(loadError);
       return;
     }
-    [_bridge.assetsLibrary writeImageToSavedPhotosAlbum:loadedImage.CGImage metadata:nil completionBlock:^(NSURL *assetURL, NSError *saveError) {
+    // It's unclear if writeImageToSavedPhotosAlbum is thread-safe
-      if (saveError) {
+    dispatch_async(dispatch_get_main_queue(), ^{
-        RCTLogWarn(@"Error saving cropped image: %@", saveError);
+      [_bridge.assetsLibrary writeImageToSavedPhotosAlbum:loadedImage.CGImage metadata:nil completionBlock:^(NSURL *assetURL, NSError *saveError) {
-        errorCallback(saveError);
+        if (saveError) {
-      } else {
+          RCTLogWarn(@"Error saving cropped image: %@", saveError);
-        successCallback(@[assetURL.absoluteString]);
+          errorCallback(saveError);
-      }
+        } else {
-    }];
+          successCallback(@[assetURL.absoluteString]);
+        }
+      }];
+    });
   }];
 }
 

Libraries/CameraRoll/RCTPhotoLibraryImageLoader.m

@@ -24,41 +24,51 @@ RCT_EXPORT_MODULE()
 
 - (BOOL)canLoadImageURL:(NSURL *)requestURL
 {
-  return [requestURL.scheme.lowercaseString isEqualToString:@"ph"];
+  return [requestURL.scheme caseInsensitiveCompare:@"ph"] == NSOrderedSame;
 }
 
-- (RCTImageLoaderCancellationBlock)loadImageForURL:(NSURL *)imageURL size:(CGSize)size scale:(CGFloat)scale resizeMode:(UIViewContentMode)resizeMode progressHandler:(RCTImageLoaderProgressBlock)progressHandler completionHandler:(RCTImageLoaderCompletionBlock)completionHandler
+- (RCTImageLoaderCancellationBlock)loadImageForURL:(NSURL *)imageURL
+                                              size:(CGSize)size
+                                             scale:(CGFloat)scale
+                                        resizeMode:(UIViewContentMode)resizeMode
+                                   progressHandler:(RCTImageLoaderProgressBlock)progressHandler
+                                 completionHandler:(RCTImageLoaderCompletionBlock)completionHandler
 {
   // Using PhotoKit for iOS 8+
   // The 'ph://' prefix is used by FBMediaKit to differentiate between
   // assets-library. It is prepended to the local ID so that it is in the
   // form of an, NSURL which is what assets-library uses.
-  NSString *phAssetID = [imageURL.absoluteString substringFromIndex:[@"ph://" length]];
+  NSString *phAssetID = [imageURL.absoluteString substringFromIndex:@"ph://".length];
   PHFetchResult *results = [PHAsset fetchAssetsWithLocalIdentifiers:@[phAssetID] options:nil];
   if (results.count == 0) {
     NSString *errorText = [NSString stringWithFormat:@"Failed to fetch PHAsset with local identifier %@ with no error message.", phAssetID];
-    NSError *error = RCTErrorWithMessage(errorText);
+    completionHandler(RCTErrorWithMessage(errorText), nil);
-    completionHandler(error, nil);
     return ^{};
   }
 
   PHAsset *asset = [results firstObject];
-
   PHImageRequestOptions *imageOptions = [PHImageRequestOptions new];
-  imageOptions.progressHandler = ^(double progress, NSError *error, BOOL *stop, NSDictionary *info) {
+
-    static const double multiplier = 1e6;
+  if (progressHandler) {
-    progressHandler(progress * multiplier, multiplier);
+    imageOptions.progressHandler = ^(double progress, NSError *error, BOOL *stop, NSDictionary *info) {
-  };
+      static const double multiplier = 1e6;
+      progressHandler(progress * multiplier, multiplier);
+    };
+  }
+
+  // Note: PhotoKit defaults to a deliveryMode of PHImageRequestOptionsDeliveryModeOpportunistic
+  // which means it may call back multiple times - we probably don't want that
 
   BOOL useMaximumSize = CGSizeEqualToSize(size, CGSizeZero);
   CGSize targetSize;
-
   if (useMaximumSize) {
     targetSize = PHImageManagerMaximumSize;
     imageOptions.resizeMode = PHImageRequestOptionsResizeModeNone;
+    imageOptions.deliveryMode = PHImageRequestOptionsDeliveryModeHighQualityFormat;
   } else {
     targetSize = size;
     imageOptions.resizeMode = PHImageRequestOptionsResizeModeFast;
+    imageOptions.deliveryMode = PHImageRequestOptionsDeliveryModeFastFormat;
   }
 
   PHImageContentMode contentMode = PHImageContentModeAspectFill;
@@ -66,7 +76,12 @@ RCT_EXPORT_MODULE()
     contentMode = PHImageContentModeAspectFit;
   }
 
-  PHImageRequestID requestID = [[PHImageManager defaultManager] requestImageForAsset:asset targetSize:targetSize contentMode:contentMode options:imageOptions resultHandler:^(UIImage *result, NSDictionary *info) {
+  PHImageRequestID requestID =
+  [[PHImageManager defaultManager] requestImageForAsset:asset
+                                             targetSize:targetSize
+                                            contentMode:contentMode
+                                                options:imageOptions
+                                          resultHandler:^(UIImage *result, NSDictionary *info) {
     if (result) {
       completionHandler(nil, result);
     } else {

Libraries/Image/RCTImageLoader.h

@@ -28,7 +28,7 @@ typedef void (^RCTImageLoaderCancellationBlock)(void);
 
 /**
  * Loads the specified image at the highest available resolution.
- * Can be called from any thread, will always call callback on main thread.
+ * Can be called from any thread, will call back on an unspecified thread.
  */
 - (RCTImageLoaderCancellationBlock)loadImageWithTag:(NSString *)imageTag
                                            callback:(RCTImageLoaderCompletionBlock)callback;

Libraries/Image/RCTImageLoader.m

@@ -9,6 +9,7 @@
 
 #import "RCTImageLoader.h"
 
+#import <libkern/OSAtomic.h>
 #import <UIKit/UIKit.h>
 
 #import "RCTConvert.h"
@@ -18,17 +19,6 @@
 #import "RCTNetworking.h"
 #import "RCTUtils.h"
 
-static void RCTDispatchCallbackOnMainQueue(void (^callback)(NSError *, id), NSError *error, UIImage *image)
-{
-  if ([NSThread isMainThread]) {
-    callback(error, image);
-  } else {
-    dispatch_async(dispatch_get_main_queue(), ^{
-      callback(error, image);
-    });
-  }
-}
-
 @implementation UIImage (React)
 
 - (CAKeyframeAnimation *)reactKeyframeAnimation
@@ -184,7 +174,7 @@ RCT_EXPORT_MODULE()
                                                size:(CGSize)size
                                               scale:(CGFloat)scale
                                          resizeMode:(UIViewContentMode)resizeMode
-                                      progressBlock:(RCTImageLoaderProgressBlock)progressBlock
+                                      progressBlock:(RCTImageLoaderProgressBlock)progressHandler
                                     completionBlock:(RCTImageLoaderCompletionBlock)completionBlock
 {
   if (imageTag.length == 0) {
@@ -192,23 +182,20 @@ RCT_EXPORT_MODULE()
     return ^{};
   }
 
-  // Ensure progress is dispatched on main thread
+  __block volatile uint32_t cancelled = 0;
-  RCTImageLoaderProgressBlock progressHandler = nil;
-  if (progressBlock) {
-    progressHandler = ^(int64_t progress, int64_t total) {
-      if ([NSThread isMainThread]) {
-        progressBlock(progress, total);
-      } else {
-        dispatch_async(dispatch_get_main_queue(), ^{
-          progressBlock(progress, total);
-        });
-      }
-    };
-  }
-
-  // Ensure completion is dispatched on main thread
   RCTImageLoaderCompletionBlock completionHandler = ^(NSError *error, UIImage *image) {
-    RCTDispatchCallbackOnMainQueue(completionBlock, error, image);
+    if ([NSThread isMainThread]) {
+
+      // Most loaders do not return on the main thread, so caller is probably not
+      // expecting it, and may do expensive post-processing in the callback
+      dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{
+        if (!cancelled) {
+          completionBlock(error, image);
+        }
+      });
+    } else if (!cancelled) {
+      completionBlock(error, image);
+    }
   };
 
   // Find suitable image URL loader
@@ -296,9 +283,7 @@ RCT_EXPORT_MODULE()
       processResponse(response, data, nil);
 
     }];
-    if (progressBlock) {
+    task.downloadProgressBlock = progressHandler;
-      task.downloadProgressBlock = progressBlock;
-    }
     [task start];
 
     return ^{
@@ -306,6 +291,7 @@ RCT_EXPORT_MODULE()
       if (decodeCancel) {
         decodeCancel();
       }
+      OSAtomicOr32Barrier(1, &cancelled);
     };
   }
 
@@ -317,27 +303,36 @@ RCT_EXPORT_MODULE()
                                               size:(CGSize)size
                                              scale:(CGFloat)scale
                                         resizeMode:(UIViewContentMode)resizeMode
-                                   completionBlock:(RCTImageLoaderCompletionBlock)completionBlock
+                                   completionBlock:(RCTImageLoaderCompletionBlock)completionHandler
 {
   id<RCTImageDataDecoder> imageDecoder = [self imageDataDecoderForData:data];
   if (imageDecoder) {
+
     return [imageDecoder decodeImageData:data
                                     size:size
                                    scale:scale
                               resizeMode:resizeMode
-                       completionHandler:completionBlock];
+                       completionHandler:completionHandler];
   } else {
+
+    __block volatile uint32_t cancelled = 0;
     dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{
+      if (cancelled) {
+        return;
+      }
       UIImage *image = [UIImage imageWithData:data scale:scale];
       if (image) {
-        completionBlock(nil, image);
+        completionHandler(nil, image);
       } else {
         NSString *errorMessage = [NSString stringWithFormat:@"Error decoding image data <NSData %p; %tu bytes>", data, data.length];
         NSError *finalError = RCTErrorWithMessage(errorMessage);
-        completionBlock(finalError, nil);
+        completionHandler(finalError, nil);
       }
     });
-    return ^{};
+
+    return ^{
+      OSAtomicOr32Barrier(1, &cancelled);
+    };
   }
 }
 

Libraries/Image/RCTImageView.m

@@ -183,24 +183,26 @@ RCT_NOT_IMPLEMENTED(- (instancetype)init)
                                                                resizeMode:self.contentMode
                                                             progressBlock:progressHandler
                                                           completionBlock:^(NSError *error, UIImage *image) {
-      if (image.reactKeyframeAnimation) {
+      dispatch_async(dispatch_get_main_queue(), ^{
-        [self.layer addAnimation:image.reactKeyframeAnimation forKey:@"contents"];
+        if (image.reactKeyframeAnimation) {
-      } else {
+          [self.layer addAnimation:image.reactKeyframeAnimation forKey:@"contents"];
-        [self.layer removeAnimationForKey:@"contents"];
+        } else {
-        self.image = image;
+          [self.layer removeAnimationForKey:@"contents"];
-      }
+          self.image = image;
-      if (error) {
-        if (_onError) {
-          _onError(@{ @"error": error.localizedDescription });
         }
-      } else {
+        if (error) {
-        if (_onLoad) {
+          if (_onError) {
-          _onLoad(nil);
+            _onError(@{ @"error": error.localizedDescription });
+          }
+        } else {
+          if (_onLoad) {
+            _onLoad(nil);
+          }
         }
-      }
+        if (_onLoadEnd) {
-      if (_onLoadEnd) {
+           _onLoadEnd(nil);
-         _onLoadEnd(nil);
+        }
-      }
+      });
     }];
   } else {
     [self clearImage];

Libraries/Image/RCTShadowVirtualImage.m

@@ -38,7 +38,13 @@ RCT_NOT_IMPLEMENTED(-(instancetype)init)
     CGFloat scale = [RCTConvert CGFloat:_source[@"scale"]] ?: 1;
 
     __weak RCTShadowVirtualImage *weakSelf = self;
-    [_bridge.imageLoader loadImageWithTag:imageTag size:CGSizeZero scale:scale resizeMode:UIViewContentModeScaleToFill progressBlock:nil completionBlock:^(NSError *error, UIImage *image) {
+    [_bridge.imageLoader loadImageWithTag:imageTag
+                                     size:CGSizeZero
+                                    scale:scale
+                               resizeMode:UIViewContentModeScaleToFill
+                            progressBlock:nil
+                          completionBlock:^(NSError *error, UIImage *image) {
+
       dispatch_async(_bridge.uiManager.methodQueue, ^{
         RCTShadowVirtualImage *strongSelf = weakSelf;
         strongSelf->_image = image;

Libraries/Image/RCTXCAssetImageLoader.m

@@ -9,6 +9,8 @@
 
 #import "RCTXCAssetImageLoader.h"
 
+#import <libkern/OSAtomic.h>
+
 #import "RCTUtils.h"
 
 @implementation RCTXCAssetImageLoader
@@ -20,34 +22,34 @@ RCT_EXPORT_MODULE()
   return RCTIsXCAssetURL(requestURL);
 }
 
- - (RCTImageLoaderCancellationBlock)loadImageForURL:(NSURL *)imageURL size:(CGSize)size scale:(CGFloat)scale resizeMode:(UIViewContentMode)resizeMode progressHandler:(RCTImageLoaderProgressBlock)progressHandler completionHandler:(RCTImageLoaderCompletionBlock)completionHandler
+ - (RCTImageLoaderCancellationBlock)loadImageForURL:(NSURL *)imageURL
+                                               size:(CGSize)size
+                                              scale:(CGFloat)scale
+                                         resizeMode:(UIViewContentMode)resizeMode
+                                    progressHandler:(RCTImageLoaderProgressBlock)progressHandler
+                                  completionHandler:(RCTImageLoaderCompletionBlock)completionHandler
 {
-  __block BOOL cancelled = NO;
+  __block volatile uint32_t cancelled = 0;
   dispatch_async(dispatch_get_main_queue(), ^{
+
     if (cancelled) {
       return;
     }
-
     NSString *imageName = RCTBundlePathForURL(imageURL);
     UIImage *image = [UIImage imageNamed:imageName];
     if (image) {
       if (progressHandler) {
         progressHandler(1, 1);
       }
-
+      completionHandler(nil, image);
-      if (completionHandler) {
-        completionHandler(nil, image);
-      }
     } else {
-      if (completionHandler) {
+      NSString *message = [NSString stringWithFormat:@"Could not find image named %@", imageName];
-        NSString *message = [NSString stringWithFormat:@"Could not find image named %@", imageName];
+      completionHandler(RCTErrorWithMessage(message), nil);
-        completionHandler(RCTErrorWithMessage(message), nil);
-      }
     }
   });
 
   return ^{
-    cancelled = YES;
+    OSAtomicOr32Barrier(1, &cancelled);
   };
 }