Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

PINCachedAnimatedImage: Fix retain cycles #546

Merged
merged 4 commits into from
Mar 10, 2020
Merged

PINCachedAnimatedImage: Fix retain cycles #546

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
83ce982
PINCachedaAnimatedImage: Fix retain cycles
Mar 10, 2020
2af8fed
wrap notification handling in PIN_TARGET_IOS
Mar 10, 2020
eebd2a4
use PINWeakify/PINStrongify
Mar 10, 2020
92cbaa9
forget to test compilation. sigh.
Mar 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions PINRemoteImage.xcodeproj/project.pbxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -443,6 +443,8 @@
A7343C5B228993D100972894 /* NSHTTPURLResponse+MaxAge.m in Sources */ = {isa = PBXBuildFile; fileRef = ACD28EAE81695DDF84BB76B8 /* NSHTTPURLResponse+MaxAge.m */; };
A7343C60228993F400972894 /* NSHTTPURLResponse+MaxAge.m in Sources */ = {isa = PBXBuildFile; fileRef = ACD28EAE81695DDF84BB76B8 /* NSHTTPURLResponse+MaxAge.m */; };
ACD28AB87FABF6BA3B9BF4E4 /* NSDate+PINCacheTests.m in Sources */ = {isa = PBXBuildFile; fileRef = ACD28D963D79EEC14EE071CE /* NSDate+PINCacheTests.m */; };
D93A340224182D46005EB15E /* TestAnimatedImage.m in Sources */ = {isa = PBXBuildFile; fileRef = D93A340124182D46005EB15E /* TestAnimatedImage.m */; };
D93A340324182D46005EB15E /* TestAnimatedImage.m in Sources */ = {isa = PBXBuildFile; fileRef = D93A340124182D46005EB15E /* TestAnimatedImage.m */; };
F165DFD91BD0504A0008C6E8 /* PINRemoteImageMacros.h in Headers */ = {isa = PBXBuildFile; fileRef = F165DFD81BD0504A0008C6E8 /* PINRemoteImageMacros.h */; settings = {ATTRIBUTES = (Public, ); }; };
F1B918FF1BCF23C900710963 /* PINRemoteImageCategoryManager.m in Sources */ = {isa = PBXBuildFile; fileRef = F1B918DC1BCF23C800710963 /* PINRemoteImageCategoryManager.m */; };
F1B919001BCF23C900710963 /* NSData+ImageDetectors.h in Headers */ = {isa = PBXBuildFile; fileRef = F1B918DE1BCF23C800710963 /* NSData+ImageDetectors.h */; settings = {ATTRIBUTES = (Public, ); }; };
Expand Down Expand Up @@ -784,6 +786,8 @@
ACD28A0374E664CFF0BB3297 /* NSHTTPURLResponse+MaxAge.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = "NSHTTPURLResponse+MaxAge.h"; sourceTree = "<group>"; };
ACD28D963D79EEC14EE071CE /* NSDate+PINCacheTests.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = "NSDate+PINCacheTests.m"; sourceTree = "<group>"; };
ACD28EAE81695DDF84BB76B8 /* NSHTTPURLResponse+MaxAge.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = "NSHTTPURLResponse+MaxAge.m"; sourceTree = "<group>"; };
D93A340024182D46005EB15E /* TestAnimatedImage.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = TestAnimatedImage.h; sourceTree = "<group>"; };
D93A340124182D46005EB15E /* TestAnimatedImage.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = TestAnimatedImage.m; sourceTree = "<group>"; };
F165DFD81BD0504A0008C6E8 /* PINRemoteImageMacros.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = PINRemoteImageMacros.h; sourceTree = "<group>"; };
F1B918D11BCF239200710963 /* PINRemoteImage.framework */ = {isa = PBXFileReference; explicitFileType = wrapper.framework; includeInIndex = 0; path = PINRemoteImage.framework; sourceTree = BUILT_PRODUCTS_DIR; };
F1B918D61BCF239200710963 /* Info.plist */ = {isa = PBXFileReference; lastKnownFileType = text.plist.xml; path = Info.plist; sourceTree = "<group>"; };
Expand Down Expand Up @@ -1099,6 +1103,8 @@
children = (
68A0FC1B1E523434000B552D /* PINRemoteImageTests.m */,
6864A9211F6D94AF007BB848 /* PINAnimatedImageTests.swift */,
D93A340024182D46005EB15E /* TestAnimatedImage.h */,
D93A340124182D46005EB15E /* TestAnimatedImage.m */,
68A0FC1D1E523434000B552D /* Info.plist */,
6864A9201F6D94AF007BB848 /* PINRemoteImageTests-Bridging-Header.h */,
683128F31F95045200D5B4A8 /* PINAnimatedImage+PINAnimatedImageTesting.h */,
Expand Down Expand Up @@ -1828,6 +1834,7 @@
isa = PBXSourcesBuildPhase;
buildActionMask = 2147483647;
files = (
D93A340324182D46005EB15E /* TestAnimatedImage.m in Sources */,
);
runOnlyForDeploymentPostprocessing = 0;
};
Expand All @@ -1838,6 +1845,7 @@
6864A9221F6D94AF007BB848 /* PINAnimatedImageTests.swift in Sources */,
68A0FC1C1E523434000B552D /* PINRemoteImageTests.m in Sources */,
683128F51F95045200D5B4A8 /* PINAnimatedImage+PINAnimatedImageTesting.m in Sources */,
D93A340224182D46005EB15E /* TestAnimatedImage.m in Sources */,
ACD28AB87FABF6BA3B9BF4E4 /* NSDate+PINCacheTests.m in Sources */,
);
runOnlyForDeploymentPostprocessing = 0;
Expand Down
191 changes: 113 additions & 78 deletions Source/Classes/AnimatedImages/PINCachedAnimatedImage.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,29 +83,40 @@ - (instancetype)initWithAnimatedImage:(id <PINAnimatedImage>)animatedImage

#if PIN_TARGET_IOS
_lastMemoryWarning = [NSDate distantPast];
PINWeakify(self);
[[NSNotificationCenter defaultCenter] addObserverForName:UIApplicationDidReceiveMemoryWarningNotification object:nil queue:nil usingBlock:^(NSNotification * _Nonnull note) {
PINStrongify(self);
NSDate *now = [NSDate date];
if (-[self.lastMemoryWarning timeIntervalSinceDate:now] < kSecondsBetweenMemoryWarnings) {
self.cachingFramesCausingMemoryWarnings = YES;
}
self.lastMemoryWarning = now;
[self cleanupFrames];
}];
[[NSNotificationCenter defaultCenter] addObserver:self
selector:@selector(didReceiveMemoryWarningNotification:)
name:UIApplicationDidReceiveMemoryWarningNotification
object:nil];
#endif

_operationQueue = [[PINOperationQueue alloc] initWithMaxConcurrentOperations:kFramesToRenderForLargeFrames];
_cachingQueue = dispatch_queue_create("Caching Queue", DISPATCH_QUEUE_SERIAL);

// dispatch later so that blocks can be set after init this runloop
__weak typeof(self) weakSelf = self;
bolsinga marked this conversation as resolved.
Show resolved Hide resolved
dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{
[self imageAtIndex:0];
typeof(self) strongSelf = weakSelf;
bolsinga marked this conversation as resolved.
Show resolved Hide resolved
[strongSelf imageAtIndex:0];
});
}
return self;
}

- (void)dealloc
{
[[NSNotificationCenter defaultCenter] removeObserver:self];
}

- (void)didReceiveMemoryWarningNotification:(NSNotification *)notification
{
NSDate *now = [NSDate date];
if (-[self.lastMemoryWarning timeIntervalSinceDate:now] < kSecondsBetweenMemoryWarnings) {
self.cachingFramesCausingMemoryWarnings = YES;
}
self.lastMemoryWarning = now;
[self cleanupFrames];
}

- (PINImage *)coverImage
{
__block PINImage *coverImage = nil;
Expand Down Expand Up @@ -229,45 +240,55 @@ - (CGImageRef)imageAtIndex:(NSUInteger)index
if (cachingDisabled && imageRef == NULL) {
imageRef = [_animatedImage imageAtIndex:index cacheProvider:self];
} else {
__weak typeof(self) weakSelf = self;
bolsinga marked this conversation as resolved.
Show resolved Hide resolved
dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{
[self updateCache];
typeof(self) strongSelf = weakSelf;
bolsinga marked this conversation as resolved.
Show resolved Hide resolved
[strongSelf updateCache];
});
}

return imageRef;
}

- (void)_updateCacheOnQueue
{
// Kick off, in order, caching frames which need to be cached
NSRange endKeepRange;
NSRange beginningKeepRange;

[self getKeepRanges:&endKeepRange beginningKeepRange:&beginningKeepRange];

[self->_lock lockWithBlock:^{
for (NSUInteger idx = endKeepRange.location; idx < NSMaxRange(endKeepRange); idx++) {
if ([self->_cachedOrCachingFrames containsIndex:idx] == NO) {
[self _locked_cacheFrame:idx];
}
}

if (beginningKeepRange.location != NSNotFound) {
for (NSUInteger idx = beginningKeepRange.location; idx < NSMaxRange(beginningKeepRange); idx++) {
if ([self->_cachedOrCachingFrames containsIndex:idx] == NO) {
[self _locked_cacheFrame:idx];
}
}
}
}];
}

- (void)updateCache
{
__weak typeof(self) weakSelf = self;
bolsinga marked this conversation as resolved.
Show resolved Hide resolved
// skip if we don't have any frames to cache
if ([self framesToCache] > 0) {
[_operationQueue scheduleOperation:^{
// Kick off, in order, caching frames which need to be cached
NSRange endKeepRange;
NSRange beginningKeepRange;

[self getKeepRanges:&endKeepRange beginningKeepRange:&beginningKeepRange];

[self->_lock lockWithBlock:^{
for (NSUInteger idx = endKeepRange.location; idx < NSMaxRange(endKeepRange); idx++) {
if ([self->_cachedOrCachingFrames containsIndex:idx] == NO) {
[self _locked_cacheFrame:idx];
}
}

if (beginningKeepRange.location != NSNotFound) {
for (NSUInteger idx = beginningKeepRange.location; idx < NSMaxRange(beginningKeepRange); idx++) {
if ([self->_cachedOrCachingFrames containsIndex:idx] == NO) {
[self _locked_cacheFrame:idx];
}
}
}
}];
typeof(self) strongSelf = weakSelf;
bolsinga marked this conversation as resolved.
Show resolved Hide resolved
[strongSelf _updateCacheOnQueue];
}];
}

[_operationQueue scheduleOperation:^{
[self cleanupFrames];
typeof(self) strongSelf = weakSelf;
bolsinga marked this conversation as resolved.
Show resolved Hide resolved
[strongSelf cleanupFrames];
}];
}

Expand Down Expand Up @@ -320,50 +341,57 @@ - (void)cleanupFrames
}];
}

- (void)_cacheWithFrameIndex:(NSUInteger)frameIndex
{
CGImageRef imageRef = [self->_animatedImage imageAtIndex:frameIndex cacheProvider:self];
PINLog(@"Generating: %lu", (unsigned long)frameIndex);

if (imageRef) {
__block PINImage *coverImage = nil;
__block PINAnimatedImageInfoReady coverImageReadyCallback = nil;
[self->_lock lockWithBlock:^{
[self->_frameCache setObject:(__bridge id _Nonnull)(imageRef) forKey:@(frameIndex)];

// Update the cover image
if (frameIndex == 0) {
BOOL notifyCallback = [self _locked_updateCoverImage:imageRef];
coverImageReadyCallback = notifyCallback ? self->_coverImageReadyCallback : nil;
coverImage = self->_coverImage;
}

self->_frameRenderCount--;
NSAssert(self->_frameRenderCount >= 0, @"playback ready is less than zero, something is wrong :(");

PINLog(@"Frames left: %ld", (long)_frameRenderCount);

dispatch_block_t notify = nil;
if (self->_frameRenderCount == 0 && self->_notifyOnReady) {
self->_notifyOnReady = NO;
if (self->_playbackReadyCallback) {
notify = self->_playbackReadyCallback;
[self->_operationQueue scheduleOperation:^{
notify();
}];
}
}
}];
if (coverImageReadyCallback) {
coverImageReadyCallback(coverImage);
}
}
}

- (void)_locked_cacheFrame:(NSUInteger)frameIndex
{
if ([_cachedOrCachingFrames containsIndex:frameIndex] == NO && _cacheCleared == NO) {
PINLog(@"Requesting: %lu", (unsigned long)frameIndex);
[_cachedOrCachingFrames addIndex:frameIndex];
_frameRenderCount++;

__weak typeof(self) weakSelf = self;
dispatch_async(_cachingQueue, ^{
CGImageRef imageRef = [self->_animatedImage imageAtIndex:frameIndex cacheProvider:self];
PINLog(@"Generating: %lu", (unsigned long)frameIndex);

if (imageRef) {
__block PINImage *coverImage = nil;
__block PINAnimatedImageInfoReady coverImageReadyCallback = nil;
[self->_lock lockWithBlock:^{
[self->_frameCache setObject:(__bridge id _Nonnull)(imageRef) forKey:@(frameIndex)];

// Update the cover image
if (frameIndex == 0) {
BOOL notifyCallback = [self _locked_updateCoverImage:imageRef];
coverImageReadyCallback = notifyCallback ? self->_coverImageReadyCallback : nil;
coverImage = self->_coverImage;
}

self->_frameRenderCount--;
NSAssert(self->_frameRenderCount >= 0, @"playback ready is less than zero, something is wrong :(");

PINLog(@"Frames left: %ld", (long)_frameRenderCount);

dispatch_block_t notify = nil;
if (self->_frameRenderCount == 0 && self->_notifyOnReady) {
self->_notifyOnReady = NO;
if (self->_playbackReadyCallback) {
notify = self->_playbackReadyCallback;
[self->_operationQueue scheduleOperation:^{
notify();
}];
}
}
}];
if (coverImageReadyCallback) {
coverImageReadyCallback(coverImage);
}
}
typeof(self) strongSelf = weakSelf;
[strongSelf _cacheWithFrameIndex:frameIndex];
});
}
}
Expand Down Expand Up @@ -451,20 +479,27 @@ - (void)setCoverImageReadyCallback:(PINAnimatedImageInfoReady)coverImageReadyCal
}];
}

- (void)_clearAnimatedImageCache
{
[self->_lock lockWithBlock:^{
self->_cacheCleared = YES;
self->_coverImage = nil;
[self->_cachedOrCachingFrames enumerateIndexesUsingBlock:^(NSUInteger idx, BOOL * _Nonnull stop) {
[self->_frameCache removeObjectForKey:@(idx)];
}];
[self->_cachedOrCachingFrames removeAllIndexes];
}];
}

/**
@abstract Clear any cached data. Called when playback is paused.
*/
- (void)clearAnimatedImageCache
{
__weak typeof(self) weakSelf = self;
dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{
[self->_lock lockWithBlock:^{
self->_cacheCleared = YES;
self->_coverImage = nil;
[self->_cachedOrCachingFrames enumerateIndexesUsingBlock:^(NSUInteger idx, BOOL * _Nonnull stop) {
[self->_frameCache removeObjectForKey:@(idx)];
}];
[self->_cachedOrCachingFrames removeAllIndexes];
}];
typeof(self) strongSelf = weakSelf;
[strongSelf _clearAnimatedImageCache];
});
}

Expand Down
11 changes: 11 additions & 0 deletions Tests/PINAnimatedImageTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,4 +189,15 @@ class PINAnimatedImageTests: XCTestCase, PINRemoteImageManagerAlternateRepresent
let webpAnimatedImage = PINWebPAnimatedImage(animatedImageData: data)
XCTAssert(webpAnimatedImage == nil)
}

func test_retainCycle() {
weak var weakCachedAnimatedImage : PINCachedAnimatedImage?
autoreleasepool {
let animatedImage = TestAnimatedImage.init()
let cachedAnimatedImage = PINCachedAnimatedImage.init(animatedImage: animatedImage)
weakCachedAnimatedImage = cachedAnimatedImage
}
let cachedAnimatedImage : PINCachedAnimatedImage? = weakCachedAnimatedImage
bolsinga marked this conversation as resolved.
Show resolved Hide resolved
XCTAssertNil(cachedAnimatedImage)
}
}
1 change: 1 addition & 0 deletions Tests/PINRemoteImageTests-Bridging-Header.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,4 @@
//

#import "PINAnimatedImage+PINAnimatedImageTesting.h"
#import "TestAnimatedImage.h"
17 changes: 17 additions & 0 deletions Tests/TestAnimatedImage.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
//
// TestAnimatedImage.h
// PINRemoteImage
//
// Created by Greg Bolsinga on 3/10/20.
// Copyright © 2020 Pinterest. All rights reserved.
//

#import "PINAnimatedImage.h"

NS_ASSUME_NONNULL_BEGIN

@interface TestAnimatedImage : NSObject <PINAnimatedImage>

@end

NS_ASSUME_NONNULL_END
39 changes: 39 additions & 0 deletions Tests/TestAnimatedImage.m
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
//
// TestAnimatedImage.m
// PINRemoteImage
//
// Created by Greg Bolsinga on 3/10/20.
// Copyright © 2020 Pinterest. All rights reserved.
//

#import "TestAnimatedImage.h"

@implementation TestAnimatedImage

@synthesize bytesPerFrame;

@synthesize data;

@synthesize error;

@synthesize frameCount;

@synthesize frameInterval;

@synthesize height;

@synthesize loopCount;

@synthesize totalDuration;

@synthesize width;

- (CFTimeInterval)durationAtIndex:(NSUInteger)index {
return 0;
}

- (id)imageAtIndex:(NSUInteger)index cacheProvider:(nullable id<PINCachedAnimatedFrameProvider>)cacheProvider {
return nil;
}

@end