1and2papa / CTAssetsPickerController

iOS control that allows picking multiple photos and videos from user's photo library.
MIT License
2.15k stars 550 forks source link

Memory Leak #189

Closed chadwilken closed 8 years ago

chadwilken commented 8 years ago

I am using a pretty vanilla setup of the library and simply presenting the view controller and scrolling to the top adds ~40mb to my in use memory. Have you experienced this before and if so do you have a work-around?

[PHPhotoLibrary requestAuthorization:^(PHAuthorizationStatus status){
    dispatch_async(dispatch_get_main_queue(), ^{

        CTAssetsPickerController *picker = [[CTAssetsPickerController alloc] init];
        PHFetchOptions *options = [[PHFetchOptions alloc] init];

        options.predicate = [NSPredicate predicateWithFormat:@"mediaType == %d", PHAssetMediaTypeImage];
        picker.assetsFetchOptions = options;
        picker.delegate = self;
        picker.showsCancelButton = YES;

        [self presentViewController:picker animated:YES completion:nil];
    });
}];

// The delegate method
- (void)assetsPickerController:(CTAssetsPickerController *)picker didFinishPickingAssets:(NSArray *)assets
{
    self.uploadAssets = [assets mutableCopy];

    [self dismissViewControllerAnimated:YES completion:nil];

    dispatch_async(dispatch_get_main_queue(), ^{
        self.uploadingImageCount = self.uploadAssets.count;
        self.completedUploadImageCount = 0;

        self.alertView = [[SCLAlertView alloc] init];
        self.alertView.customViewColor = [UIColor ccBlueColor];

        [self.alertView showWaiting:self.tabBarController title:@"Uploading Photos" subTitle:@"Please wait while we upload your photos" closeButtonTitle:nil duration:0];

        // This recurses and uploads to my server, even upon completion it still maintains a large in memory footprint.
        [self fetchImageForAsset:[self.uploadAssets firstObject]];
    });
}
chrisze commented 8 years ago

@chiunam : First of, thank you for creating this pod. It looks great. We've just integrated it with our app and are testing it to learn more about it. I also did notice that it uses a lot of memory when scrolling through a large collection of images. On iPhone 6 Plus with 2000 images, the working set starts at 30 MB when the asset picker is first presented, and goes up to almost 200 MB when I scroll through the collection a few times.

After a cursory look, it doesn't seem like a memory leak though - eventually, the memory gets cleaned up if I dismiss the picker and present it again. Still, that's a lot of memory. Possibly related to that - scroll performance seems to be an issue on iPhone 6 Plus. I agree with your comment earlier that perf is adequate on iPhone 6, but with more images visible on screen + higher resolution on iPhone 6 Plus, the experience is not smooth.

I've spend less than an hour looking at the code so far, but it feels like PHCachingImageManager is contributing to both problems. In CTAssetsGridViewController, if I replace:

- (void)scrollViewDidScroll:(UIScrollView *)scrollView
{
    [self updateCachedAssetImages];
}

with

- (void)scrollViewDidEndDecelerating:(UIScrollView *)scrollView
{
    [self updateCachedAssetImages];
}

then scrolling is a lot smoother, and the app ends up using a lot less memory (I couldn't go past 60 MB in the exact same experiment as above).

/cc: @chadwilken

chadwilken commented 8 years ago

Good find! I would love to get a patch of this soon. If you need help with anything let me know.

chrisze commented 8 years ago

I've updated my PR to include the fix for this: https://github.com/chiunam/CTAssetsPickerController/pull/188. I changed thumbnail request resize mode from PHImageRequestOptionsResizeModeExact to PHImageRequestOptionsResizeModeFast, which seems to address the memory usage problem and improve scroll performance. Having said that, I think it would be good to test and iterate on it some more. When playing with the picker a bit more extensively, the grid view froze; when I attached the debugger, there were more than 100 photo library worker threads. I doubt that's related to resize mode, since I've seen similar behavior before.

1and2papa commented 8 years ago

@chrisze Thanks a a lot for your investigation and contribution. I need some time to study and test your PR. Might not able to merge it in a short time as I am pretty busy recently.

@chadwilken Please help to test @chrisze's PR if you are free.

chadwilken commented 8 years ago

I did some testing of the PR and it seems to be much better. I didn't get any deadlocks and the memory issues got a tad better as well. cc/ @chrisze @chiunam

1and2papa commented 8 years ago

Guys, sorry for being late. I'm going to merge his PR. Just have to clarify some changes with @chrisze. Stay tuned.