From 938ecd9b6f352877db4386a33bc67fbd369fcb68 Mon Sep 17 00:00:00 2001 From: Garrett Moon Date: Tue, 3 May 2016 15:56:38 -0700 Subject: [PATCH 01/16] Fix deadlock when laying out on multiple threads Summary: We observed a deadlock which occurred when two threads were laying out the same set of nodes. On one thread, layout would occur on a leaf node. It would lock and as part of this layout process, ASDK walks up the node tree and calls __setNeedsLayout on its supernode until it reaches the supernode with no supernode. When the supernode gets its call to __setNeedsLayout it also locks. So leaf node locks and then awaits supernode lock. On another thread, we're doing a layout pass on the supernode in the above thread. This locks the supernode and attempts to lock the leaf node. This deadlocks (remember the above thread is holding onto the leaf lock and awaiting the supernode lock. This thread is holding onto the supernode lock and awaiting the leaf lock). This is all exacerbated by the use of recursive locks. --- AsyncDisplayKit/ASCellNode.mm | 2 + AsyncDisplayKit/ASDisplayNode.mm | 38 ++++++++++--------- .../Private/ASDisplayNode+UIViewBridge.mm | 7 ++++ 3 files changed, 30 insertions(+), 17 deletions(-) diff --git a/AsyncDisplayKit/ASCellNode.mm b/AsyncDisplayKit/ASCellNode.mm index b1971fafe6..3effb78afa 100644 --- a/AsyncDisplayKit/ASCellNode.mm +++ b/AsyncDisplayKit/ASCellNode.mm @@ -125,6 +125,8 @@ { CGSize oldSize = self.calculatedSize; [super __setNeedsLayout]; + + ASDN::MutexLocker l(_propertyLock); [self didRelayoutFromOldSize:oldSize toNewSize:self.calculatedSize]; } diff --git a/AsyncDisplayKit/ASDisplayNode.mm b/AsyncDisplayKit/ASDisplayNode.mm index 81375d39ff..74314a5f67 100644 --- a/AsyncDisplayKit/ASDisplayNode.mm +++ b/AsyncDisplayKit/ASDisplayNode.mm @@ -1001,27 +1001,31 @@ static ASDisplayNodeMethodOverrides GetASDisplayNodeMethodOverrides(Class c) [self invalidateCalculatedLayout]; if (_supernode) { + ASDisplayNode *supernode = _supernode; + ASDN::MutexUnlocker u(_propertyLock); // Cause supernode's layout to be invalidated + // We need to release the lock to prevent a deadlock [_supernode setNeedsLayout]; - } else { - // This is the root node. Trigger a full measurement pass on *current* thread. Old constrained size is re-used. - [self measureWithSizeRange:oldConstrainedSize]; + return; + } + + // This is the root node. Trigger a full measurement pass on *current* thread. Old constrained size is re-used. + [self measureWithSizeRange:oldConstrainedSize]; - CGRect oldBounds = self.bounds; - CGSize oldSize = oldBounds.size; - CGSize newSize = _layout.size; + CGRect oldBounds = self.bounds; + CGSize oldSize = oldBounds.size; + CGSize newSize = _layout.size; + + if (! CGSizeEqualToSize(oldSize, newSize)) { + self.bounds = (CGRect){ oldBounds.origin, newSize }; - if (! CGSizeEqualToSize(oldSize, newSize)) { - self.bounds = (CGRect){ oldBounds.origin, newSize }; - - // Frame's origin must be preserved. Since it is computed from bounds size, anchorPoint - // and position (see frame setter in ASDisplayNode+UIViewBridge), position needs to be adjusted. - CGPoint anchorPoint = self.anchorPoint; - CGPoint oldPosition = self.position; - CGFloat xDelta = (newSize.width - oldSize.width) * anchorPoint.x; - CGFloat yDelta = (newSize.height - oldSize.height) * anchorPoint.y; - self.position = CGPointMake(oldPosition.x + xDelta, oldPosition.y + yDelta); - } + // Frame's origin must be preserved. Since it is computed from bounds size, anchorPoint + // and position (see frame setter in ASDisplayNode+UIViewBridge), position needs to be adjusted. + CGPoint anchorPoint = self.anchorPoint; + CGPoint oldPosition = self.position; + CGFloat xDelta = (newSize.width - oldSize.width) * anchorPoint.x; + CGFloat yDelta = (newSize.height - oldSize.height) * anchorPoint.y; + self.position = CGPointMake(oldPosition.x + xDelta, oldPosition.y + yDelta); } } diff --git a/AsyncDisplayKit/Private/ASDisplayNode+UIViewBridge.mm b/AsyncDisplayKit/Private/ASDisplayNode+UIViewBridge.mm index 57f588db25..60a20da1de 100644 --- a/AsyncDisplayKit/Private/ASDisplayNode+UIViewBridge.mm +++ b/AsyncDisplayKit/Private/ASDisplayNode+UIViewBridge.mm @@ -37,6 +37,7 @@ #if DISPLAYNODE_USE_LOCKS #define _bridge_prologue_read ASDN::MutexLocker l(_propertyLock); ASDisplayNodeAssertThreadAffinity(self) #define _bridge_prologue_write ASDN::MutexLocker l(_propertyLock) +#define _bridge_prologue_write_unlock ASDN::MutexUnlocker u(_propertyLock) #else #define _bridge_prologue_read ASDisplayNodeAssertThreadAffinity(self) #define _bridge_prologue_write @@ -331,7 +332,11 @@ if (shouldApply) { _layer.layerProperty = (layerValueExpr); } else { ASDisplayNo // The node is loaded and we're on main. // Quite the opposite of setNeedsDisplay, we must call __setNeedsLayout before messaging // the view or layer to ensure that measurement and implicitly added subnodes have been handled. + + // Calling __setNeedsLayout while holding the property lock can cause deadlocks + _bridge_prologue_write_unlock; [self __setNeedsLayout]; + _bridge_prologue_write; _messageToViewOrLayer(setNeedsLayout); } else if (__loaded(self)) { // The node is loaded but we're not on main. @@ -341,7 +346,9 @@ if (shouldApply) { _layer.layerProperty = (layerValueExpr); } else { ASDisplayNo [ASDisplayNodeGetPendingState(self) setNeedsLayout]; } else { // The node is not loaded and we're not on main. + _bridge_prologue_write_unlock; [self __setNeedsLayout]; + _bridge_prologue_write; } } From 025dd553128674e8c83979b411ee5689542fd722 Mon Sep 17 00:00:00 2001 From: Garrett Moon Date: Tue, 3 May 2016 15:59:21 -0700 Subject: [PATCH 02/16] Dang, message local var, not ivar --- AsyncDisplayKit/ASDisplayNode.mm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/AsyncDisplayKit/ASDisplayNode.mm b/AsyncDisplayKit/ASDisplayNode.mm index 74314a5f67..9e159498b2 100644 --- a/AsyncDisplayKit/ASDisplayNode.mm +++ b/AsyncDisplayKit/ASDisplayNode.mm @@ -1005,7 +1005,7 @@ static ASDisplayNodeMethodOverrides GetASDisplayNodeMethodOverrides(Class c) ASDN::MutexUnlocker u(_propertyLock); // Cause supernode's layout to be invalidated // We need to release the lock to prevent a deadlock - [_supernode setNeedsLayout]; + [supernode setNeedsLayout]; return; } From dc56c060d0c653b766f04af13f1bd5cffb472af2 Mon Sep 17 00:00:00 2001 From: Garrett Moon Date: Thu, 5 May 2016 10:02:29 -0700 Subject: [PATCH 03/16] Add comment for why lock is added. --- AsyncDisplayKit/ASCellNode.mm | 2 ++ 1 file changed, 2 insertions(+) diff --git a/AsyncDisplayKit/ASCellNode.mm b/AsyncDisplayKit/ASCellNode.mm index 3effb78afa..0803c81f79 100644 --- a/AsyncDisplayKit/ASCellNode.mm +++ b/AsyncDisplayKit/ASCellNode.mm @@ -126,6 +126,8 @@ CGSize oldSize = self.calculatedSize; [super __setNeedsLayout]; + //Adding this lock because lock used to be held when this method was called. Not sure if it's necessary for + //didRelayoutFromOldSize:toNewSize: ASDN::MutexLocker l(_propertyLock); [self didRelayoutFromOldSize:oldSize toNewSize:self.calculatedSize]; } From dc6d2e76604083ca8a73160e91c562a53410af89 Mon Sep 17 00:00:00 2001 From: Michael Schneider Date: Wed, 4 May 2016 11:52:45 -0700 Subject: [PATCH 04/16] Improve Transition ID handling --- AsyncDisplayKit/ASDisplayNode.mm | 89 +++++++++++-------- .../Private/ASDisplayNodeInternal.h | 2 + 2 files changed, 54 insertions(+), 37 deletions(-) diff --git a/AsyncDisplayKit/ASDisplayNode.mm b/AsyncDisplayKit/ASDisplayNode.mm index 9e159498b2..6d8515d74f 100644 --- a/AsyncDisplayKit/ASDisplayNode.mm +++ b/AsyncDisplayKit/ASDisplayNode.mm @@ -654,6 +654,8 @@ static ASDisplayNodeMethodOverrides GetASDisplayNodeMethodOverrides(Class c) return (!_flags.isMeasured || !ASSizeRangeEqualToSizeRange(constrainedSize, _constrainedSize)); } +#pragma mark - Layout Transition + - (void)transitionLayoutWithAnimation:(BOOL)animated shouldMeasureAsync:(BOOL)shouldMeasureAsync measurementCompletion:(void(^)())completion @@ -681,10 +683,10 @@ static ASDisplayNodeMethodOverrides GetASDisplayNodeMethodOverrides(Class c) ASDisplayNodeAssert(ASHierarchyStateIncludesLayoutPending(_hierarchyState) == NO, @"Can't start a transition when one of the supernodes is performing one."); } - int32_t transitionID = [self _newTransitionID]; + int32_t transitionID = [self _startNewTransition]; ASDisplayNodePerformBlockOnEverySubnode(self, ^(ASDisplayNode * _Nonnull node) { - ASDisplayNodeAssert([node _hasTransitionsInProgress] == NO, @"Can't start a transition when one of the subnodes is performing one."); + ASDisplayNodeAssert([node _hasTransitionInProgress] == NO, @"Can't start a transition when one of the subnodes is performing one."); node.hierarchyState |= ASHierarchyStateLayoutPending; node.pendingTransitionID = transitionID; }); @@ -726,13 +728,13 @@ static ASDisplayNodeMethodOverrides GetASDisplayNodeMethodOverrides(Class c) ASSizeRange previousConstrainedSize = _constrainedSize; [self applyLayout:newLayout constrainedSize:constrainedSize layoutContext:nil]; - [self _invalidateTransitionSentinel]; - ASDisplayNodePerformBlockOnEverySubnode(self, ^(ASDisplayNode * _Nonnull node) { [node applyPendingLayoutContext]; [node _completeLayoutCalculation]; node.hierarchyState &= (~ASHierarchyStateLayoutPending); }); + + [self _finishOrCancelTransition]; if (completion) { completion(); @@ -781,7 +783,6 @@ static ASDisplayNodeMethodOverrides GetASDisplayNodeMethodOverrides(Class c) } } - - (void)calculatedLayoutDidChange { // subclass override @@ -790,9 +791,10 @@ static ASDisplayNodeMethodOverrides GetASDisplayNodeMethodOverrides(Class c) - (void)cancelLayoutTransitionsInProgress { ASDN::MutexLocker l(_propertyLock); - if ([self _hasTransitionsInProgress]) { - // Invalidate transition sentinel to cancel transitions in progress - [self _invalidateTransitionSentinel]; + if ([self _hasTransitionInProgress]) { + // Cancel transition in progress + [self _finishOrCancelTransition]; + // Tell subnodes to exit layout pending state and clear related properties ASDisplayNodePerformBlockOnEverySubnode(self, ^(ASDisplayNode * _Nonnull node) { node.hierarchyState &= (~ASHierarchyStateLayoutPending); @@ -800,8 +802,6 @@ static ASDisplayNodeMethodOverrides GetASDisplayNodeMethodOverrides(Class c) } } -#pragma mark - Layout Transition - - (BOOL)usesImplicitHierarchyManagement { ASDN::MutexLocker l(_propertyLock); @@ -814,6 +814,40 @@ static ASDisplayNodeMethodOverrides GetASDisplayNodeMethodOverrides(Class c) _usesImplicitHierarchyManagement = value; } +- (BOOL)_hasTransitionInProgress +{ + ASDN::MutexLocker l(_propertyLock); + return _transitionInProgress; +} + +/// Starts a new transition and returns the transition id +- (int32_t)_startNewTransition +{ + ASDN::MutexLocker l(_propertyLock); + _transitionInProgress = YES; + + if (!_transitionSentinel) { + _transitionSentinel = [[ASSentinel alloc] init]; + } + return [_transitionSentinel increment]; +} + +- (void)_finishOrCancelTransition +{ + ASDN::MutexLocker l(_propertyLock); + _transitionInProgress = NO; +} + +- (BOOL)_shouldAbortTransitionWithID:(int32_t)transitionID +{ + ASDN::MutexLocker l(_propertyLock); + if (_transitionInProgress) { + return _transitionSentinel == nil || transitionID != _transitionSentinel.value; + } + + return YES; +} + - (void)animateLayoutTransition:(id)context { [self __layoutSublayouts]; @@ -827,6 +861,7 @@ static ASDisplayNodeMethodOverrides GetASDisplayNodeMethodOverrides(Class c) _pendingLayoutContext = nil; } + #pragma mark - _ASTransitionContextCompletionDelegate - (void)transitionContext:(_ASTransitionContext *)context didComplete:(BOOL)didComplete @@ -1689,9 +1724,11 @@ static NSInteger incrementIfFound(NSInteger i) { } if (supernodeDidChange) { + // Hierarchy state ASHierarchyState stateToEnterOrExit = (newSupernode ? newSupernode.hierarchyState : oldSupernode.hierarchyState); + // Rasterized state BOOL parentWasOrIsRasterized = (newSupernode ? newSupernode.shouldRasterizeDescendants : oldSupernode.shouldRasterizeDescendants); if (parentWasOrIsRasterized) { @@ -1700,6 +1737,10 @@ static NSInteger incrementIfFound(NSInteger i) { if (newSupernode) { [self enterHierarchyState:stateToEnterOrExit]; } else { + // If a node will be removed from the supernode it should go out from the layout pending state to remove all + // layout pending state related properties on the node + stateToEnterOrExit |= ASHierarchyStateLayoutPending; + [self exitHierarchyState:stateToEnterOrExit]; } } @@ -2644,38 +2685,12 @@ static const char *ASDisplayNodeDrawingPriorityKey = "ASDrawingPriority"; _flags.isInHierarchy = inHierarchy; } -- (BOOL)_hasTransitionsInProgress -{ - ASDN::MutexLocker l(_propertyLock); - return _transitionSentinel != nil; -} - -- (void)_invalidateTransitionSentinel -{ - ASDN::MutexLocker l(_propertyLock); - _transitionSentinel = nil; -} - -- (BOOL)_shouldAbortTransitionWithID:(int32_t)transitionID -{ - ASDN::MutexLocker l(_propertyLock); - return _transitionSentinel == nil || transitionID != _transitionSentinel.value; -} - -- (int32_t)_newTransitionID -{ - ASDN::MutexLocker l(_propertyLock); - if (!_transitionSentinel) { - _transitionSentinel = [[ASSentinel alloc] init]; - } - return [_transitionSentinel increment]; -} - - (id)finalLayoutable { return self; } + #pragma mark - ASEnvironment - (ASEnvironmentState)environmentState diff --git a/AsyncDisplayKit/Private/ASDisplayNodeInternal.h b/AsyncDisplayKit/Private/ASDisplayNodeInternal.h index 4828b8bec6..b4668ac613 100644 --- a/AsyncDisplayKit/Private/ASDisplayNodeInternal.h +++ b/AsyncDisplayKit/Private/ASDisplayNodeInternal.h @@ -93,7 +93,9 @@ FOUNDATION_EXPORT NSString * const ASRenderingEngineDidDisplayNodesScheduledBefo ASDisplayNode * __weak _supernode; ASSentinel *_displaySentinel; + ASSentinel *_transitionSentinel; + BOOL _transitionInProgress; // This is the desired contentsScale, not the scale at which the layer's contents should be displayed CGFloat _contentsScaleForDisplay; From d1054d6ed98a3fc68ab078b3d6af360ae09f21d2 Mon Sep 17 00:00:00 2001 From: Michael Schneider Date: Thu, 5 May 2016 20:24:26 -0700 Subject: [PATCH 05/16] Move from ASSentinel to a atomic int --- AsyncDisplayKit/ASDisplayNode.mm | 14 +++----------- AsyncDisplayKit/Private/ASDisplayNodeInternal.h | 2 +- 2 files changed, 4 insertions(+), 12 deletions(-) diff --git a/AsyncDisplayKit/ASDisplayNode.mm b/AsyncDisplayKit/ASDisplayNode.mm index 6d8515d74f..6e54a450c1 100644 --- a/AsyncDisplayKit/ASDisplayNode.mm +++ b/AsyncDisplayKit/ASDisplayNode.mm @@ -373,7 +373,6 @@ static ASDisplayNodeMethodOverrides GetASDisplayNodeMethodOverrides(Class c) _pendingViewState = nil; _displaySentinel = nil; - _transitionSentinel = nil; _pendingDisplayNodes = nil; } @@ -825,11 +824,8 @@ static ASDisplayNodeMethodOverrides GetASDisplayNodeMethodOverrides(Class c) { ASDN::MutexLocker l(_propertyLock); _transitionInProgress = YES; - - if (!_transitionSentinel) { - _transitionSentinel = [[ASSentinel alloc] init]; - } - return [_transitionSentinel increment]; + _transitionID = OSAtomicAdd32(1, &_transitionID); + return _transitionID; } - (void)_finishOrCancelTransition @@ -841,11 +837,7 @@ static ASDisplayNodeMethodOverrides GetASDisplayNodeMethodOverrides(Class c) - (BOOL)_shouldAbortTransitionWithID:(int32_t)transitionID { ASDN::MutexLocker l(_propertyLock); - if (_transitionInProgress) { - return _transitionSentinel == nil || transitionID != _transitionSentinel.value; - } - - return YES; + return (!_transitionInProgress || _transitionID != transitionID); } - (void)animateLayoutTransition:(id)context diff --git a/AsyncDisplayKit/Private/ASDisplayNodeInternal.h b/AsyncDisplayKit/Private/ASDisplayNodeInternal.h index b4668ac613..ca11a8746a 100644 --- a/AsyncDisplayKit/Private/ASDisplayNodeInternal.h +++ b/AsyncDisplayKit/Private/ASDisplayNodeInternal.h @@ -94,7 +94,7 @@ FOUNDATION_EXPORT NSString * const ASRenderingEngineDidDisplayNodesScheduledBefo ASSentinel *_displaySentinel; - ASSentinel *_transitionSentinel; + int32_t _transitionID; BOOL _transitionInProgress; // This is the desired contentsScale, not the scale at which the layer's contents should be displayed From 253692df65caf0d767ba4e98c6ee6a11d6790396 Mon Sep 17 00:00:00 2001 From: Adlai Holler Date: Fri, 6 May 2016 21:45:31 -0700 Subject: [PATCH 06/16] [ASMultiplexImageNode] Do not request already-loaded images from data source to avoid loop --- AsyncDisplayKit/ASMultiplexImageNode.mm | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/AsyncDisplayKit/ASMultiplexImageNode.mm b/AsyncDisplayKit/ASMultiplexImageNode.mm index cb83bb1156..2af4f1836e 100644 --- a/AsyncDisplayKit/ASMultiplexImageNode.mm +++ b/AsyncDisplayKit/ASMultiplexImageNode.mm @@ -436,8 +436,12 @@ typedef void(^ASMultiplexImageLoadCompletionBlock)(UIImage *image, id imageIdent } // Grab the best available image from the data source. + UIImage *existingImage = self.image; for (id imageIdentifier in _imageIdentifiers) { - UIImage *image = [_dataSource multiplexImageNode:self imageForImageIdentifier:imageIdentifier]; + // If this image is already loaded, don't request it from the data source again because + // the data source may generate a new instance of UIImage that returns NO for isEqual: + // and we'll end up in an infinite loading loop. + UIImage *image = ASObjectIsEqual(imageIdentifier, _loadedImageIdentifier) ? existingImage : [_dataSource multiplexImageNode:self imageForImageIdentifier:imageIdentifier]; if (image) { if (imageIdentifierOut) { *imageIdentifierOut = imageIdentifier; From 73850f24bb79457056feab4599997ec7cf2e7f01 Mon Sep 17 00:00:00 2001 From: Hannah Troisi Date: Fri, 6 May 2016 23:43:29 -0700 Subject: [PATCH 07/16] [Update README.md with Slack badge + invite bot] - update cocoapods numbers - add slack badge - clicking it should take you to a slack invite landing page using a slack invite bot hosted on Heroku (Slackin) --- README.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index e4d1dcd36f..d916239814 100644 --- a/README.md +++ b/README.md @@ -1,7 +1,8 @@ ![AsyncDisplayKit](https://github.com/facebook/AsyncDisplayKit/blob/master/docs/assets/logo.png) -[![Apps Using](https://img.shields.io/badge/Apps%20Using%20ASDK-%3E3,178-28B9FE.svg)](http://cocoapods.org/pods/AsyncDisplayKit) -[![Downloads](https://img.shields.io/badge/Total%20Downloads-%3E336,372-28B9FE.svg)](http://cocoapods.org/pods/AsyncDisplayKit) +[![Apps Using](https://img.shields.io/badge/Apps%20Using%20ASDK-%3E3,658-28B9FE.svg)](http://cocoapods.org/pods/AsyncDisplayKit) +[![Downloads](https://img.shields.io/badge/Total%20Downloads-%3E377,749-28B9FE.svg)](http://cocoapods.org/pods/AsyncDisplayKit) +[![Slack Status](http://asdk-slack-auto-invite.herokuapp.com/badge.svg)](http://asdk-slack-auto-invite.herokuapp.com) [![Platform](https://img.shields.io/badge/platforms-iOS%20%7C%20tvOS-orange.svg)](http://AsyncDisplayKit.org) [![Languages](https://img.shields.io/badge/languages-ObjC%20%7C%20Swift-orange.svg)](http://AsyncDisplayKit.org) From 9f75e9a8e00a28cff02d28641e8fecd6b3703052 Mon Sep 17 00:00:00 2001 From: Hannah Troisi Date: Sat, 7 May 2016 01:20:13 -0700 Subject: [PATCH 08/16] [README.md] Removing Slack badge to save server time Will re-add once we have we have unlimited server time (paid Heroku account or deployed on our own server). --- README.md | 1 - 1 file changed, 1 deletion(-) diff --git a/README.md b/README.md index d916239814..b1e0f3f201 100644 --- a/README.md +++ b/README.md @@ -2,7 +2,6 @@ [![Apps Using](https://img.shields.io/badge/Apps%20Using%20ASDK-%3E3,658-28B9FE.svg)](http://cocoapods.org/pods/AsyncDisplayKit) [![Downloads](https://img.shields.io/badge/Total%20Downloads-%3E377,749-28B9FE.svg)](http://cocoapods.org/pods/AsyncDisplayKit) -[![Slack Status](http://asdk-slack-auto-invite.herokuapp.com/badge.svg)](http://asdk-slack-auto-invite.herokuapp.com) [![Platform](https://img.shields.io/badge/platforms-iOS%20%7C%20tvOS-orange.svg)](http://AsyncDisplayKit.org) [![Languages](https://img.shields.io/badge/languages-ObjC%20%7C%20Swift-orange.svg)](http://AsyncDisplayKit.org) From fab117b8249991e1ac7dfe17b6a670dadcd706f5 Mon Sep 17 00:00:00 2001 From: Adlai Holler Date: Sat, 7 May 2016 15:14:30 -0700 Subject: [PATCH 09/16] [ASPINRemoteImageDownloader] Call download progress handler --- .../Details/ASPINRemoteImageDownloader.m | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/AsyncDisplayKit/Details/ASPINRemoteImageDownloader.m b/AsyncDisplayKit/Details/ASPINRemoteImageDownloader.m index 401211709c..2a11e70381 100644 --- a/AsyncDisplayKit/Details/ASPINRemoteImageDownloader.m +++ b/AsyncDisplayKit/Details/ASPINRemoteImageDownloader.m @@ -96,8 +96,9 @@ - (id )synchronouslyFetchedCachedImageWithURL:(NSURL *)URL; { - NSString *key = [[self sharedPINRemoteImageManager] cacheKeyForURL:URL processorKey:nil]; - PINRemoteImageManagerResult *result = [[self sharedPINRemoteImageManager] synchronousImageFromCacheWithCacheKey:key options:PINRemoteImageManagerDownloadOptionsSkipDecode]; + PINRemoteImageManager *manager = [self sharedPINRemoteImageManager]; + NSString *key = [manager cacheKeyForURL:URL processorKey:nil]; + PINRemoteImageManagerResult *result = [manager synchronousImageFromCacheWithCacheKey:key options:PINRemoteImageManagerDownloadOptionsSkipDecode]; #if PIN_ANIMATED_AVAILABLE if (result.alternativeRepresentation) { return result.alternativeRepresentation; @@ -133,7 +134,16 @@ downloadProgress:(ASImageDownloaderProgress)downloadProgress completion:(ASImageDownloaderCompletion)completion; { - return [[self sharedPINRemoteImageManager] downloadImageWithURL:URL options:PINRemoteImageManagerDownloadOptionsSkipDecode completion:^(PINRemoteImageManagerResult *result) { + return [[self sharedPINRemoteImageManager] downloadImageWithURL:URL options:PINRemoteImageManagerDownloadOptionsSkipDecode progressDownload:^(int64_t completedBytes, int64_t totalBytes) { + /// If we're targeting the main queue and we're on the main thread, complete immediately. + if (ASDisplayNodeThreadIsMain() && callbackQueue == dispatch_get_main_queue()) { + downloadProgress(totalBytes / (CGFloat)completedBytes); + } else { + dispatch_async(callbackQueue, ^{ + downloadProgress(totalBytes / (CGFloat)completedBytes); + }); + } + } completion:^(PINRemoteImageManagerResult * _Nonnull result) { /// If we're targeting the main queue and we're on the main thread, complete immediately. if (ASDisplayNodeThreadIsMain() && callbackQueue == dispatch_get_main_queue()) { #if PIN_ANIMATED_AVAILABLE From 654518b2b9d98182ce7fa228340c85b49fe40fc6 Mon Sep 17 00:00:00 2001 From: Adlai Holler Date: Sat, 7 May 2016 15:16:52 -0700 Subject: [PATCH 10/16] Fix nullability of ASMultiplexImageNode.imageManager --- AsyncDisplayKit/ASMultiplexImageNode.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/AsyncDisplayKit/ASMultiplexImageNode.h b/AsyncDisplayKit/ASMultiplexImageNode.h index 73a74cbdeb..c81a402c5b 100644 --- a/AsyncDisplayKit/ASMultiplexImageNode.h +++ b/AsyncDisplayKit/ASMultiplexImageNode.h @@ -123,7 +123,7 @@ typedef NS_ENUM(NSUInteger, ASMultiplexImageNodeErrorCode) { * @see `+[NSURL URLWithAssetLocalIdentifier:targetSize:contentMode:options:]` below. */ -@property (nonatomic, strong) PHImageManager *imageManager; +@property (nullable, nonatomic, strong) PHImageManager *imageManager; #endif @end From 68e324d2d00edf3543d20551f2818a9dd7c2c505 Mon Sep 17 00:00:00 2001 From: Adlai Holler Date: Sat, 7 May 2016 15:18:11 -0700 Subject: [PATCH 11/16] Improve commentary --- AsyncDisplayKit/Details/ASPINRemoteImageDownloader.m | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/AsyncDisplayKit/Details/ASPINRemoteImageDownloader.m b/AsyncDisplayKit/Details/ASPINRemoteImageDownloader.m index 2a11e70381..59a83afb80 100644 --- a/AsyncDisplayKit/Details/ASPINRemoteImageDownloader.m +++ b/AsyncDisplayKit/Details/ASPINRemoteImageDownloader.m @@ -135,7 +135,7 @@ completion:(ASImageDownloaderCompletion)completion; { return [[self sharedPINRemoteImageManager] downloadImageWithURL:URL options:PINRemoteImageManagerDownloadOptionsSkipDecode progressDownload:^(int64_t completedBytes, int64_t totalBytes) { - /// If we're targeting the main queue and we're on the main thread, complete immediately. + /// If we're targeting the main queue and we're on the main thread, call immediately. if (ASDisplayNodeThreadIsMain() && callbackQueue == dispatch_get_main_queue()) { downloadProgress(totalBytes / (CGFloat)completedBytes); } else { From 33d0919b1f5579c2af9556b079b95967d19652f0 Mon Sep 17 00:00:00 2001 From: Adlai Holler Date: Sat, 7 May 2016 15:50:03 -0700 Subject: [PATCH 12/16] [ASImageProtocols] Be smarter about nullability with image downloader protocol --- AsyncDisplayKit/Details/ASImageProtocols.h | 14 ++++++++++---- .../Details/ASPINRemoteImageDownloader.m | 2 ++ 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/AsyncDisplayKit/Details/ASImageProtocols.h b/AsyncDisplayKit/Details/ASImageProtocols.h index 6b86fb97db..ff6803bd8d 100644 --- a/AsyncDisplayKit/Details/ASImageProtocols.h +++ b/AsyncDisplayKit/Details/ASImageProtocols.h @@ -61,7 +61,16 @@ typedef void(^ASImageCacherCompletion)(id _Nullable i @end +/** + @param image The image that was downloaded, if the image could be successfully downloaded; nil otherwise. + @param error An error describing why the download of `URL` failed, if the download failed; nil otherwise. + @param downloadIdentifier The identifier for the download task that completed. + */ typedef void(^ASImageDownloaderCompletion)(id _Nullable image, NSError * _Nullable error, id _Nullable downloadIdentifier); + +/** + @param progress The progress of the download, in the range of (0.0, 1.0), inclusive. + */ typedef void(^ASImageDownloaderProgress)(CGFloat progress); typedef void(^ASImageDownloaderProgressImage)(UIImage *progressImage, CGFloat progress, id _Nullable downloadIdentifier); @@ -98,10 +107,7 @@ typedef NS_ENUM(NSUInteger, ASImageDownloaderPriority) { @param URL The URL of the image to download. @param callbackQueue The queue to call `downloadProgressBlock` and `completion` on. @param downloadProgress The block to be invoked when the download of `URL` progresses. - @param progress The progress of the download, in the range of (0.0, 1.0), inclusive. @param completion The block to be invoked when the download has completed, or has failed. - @param image The image that was downloaded, if the image could be successfully downloaded; nil otherwise. - @param error An error describing why the download of `URL` failed, if the download failed; nil otherwise. @discussion This method is likely to be called on the main thread, so any custom implementations should make sure to background any expensive download operations. @result An opaque identifier to be used in canceling the download, via `cancelImageDownloadForIdentifier:`. You must retain the identifier if you wish to use it later. @@ -109,7 +115,7 @@ typedef NS_ENUM(NSUInteger, ASImageDownloaderPriority) { - (nullable id)downloadImageWithURL:(NSURL *)URL callbackQueue:(dispatch_queue_t)callbackQueue downloadProgress:(nullable ASImageDownloaderProgress)downloadProgress - completion:(nullable ASImageDownloaderCompletion)completion; + completion:(ASImageDownloaderCompletion)completion; /** diff --git a/AsyncDisplayKit/Details/ASPINRemoteImageDownloader.m b/AsyncDisplayKit/Details/ASPINRemoteImageDownloader.m index 59a83afb80..ee78b0e7b6 100644 --- a/AsyncDisplayKit/Details/ASPINRemoteImageDownloader.m +++ b/AsyncDisplayKit/Details/ASPINRemoteImageDownloader.m @@ -135,6 +135,8 @@ completion:(ASImageDownloaderCompletion)completion; { return [[self sharedPINRemoteImageManager] downloadImageWithURL:URL options:PINRemoteImageManagerDownloadOptionsSkipDecode progressDownload:^(int64_t completedBytes, int64_t totalBytes) { + if (downloadProgress == nil) { return; } + /// If we're targeting the main queue and we're on the main thread, call immediately. if (ASDisplayNodeThreadIsMain() && callbackQueue == dispatch_get_main_queue()) { downloadProgress(totalBytes / (CGFloat)completedBytes); From 2e2ebe8c080a3b7ae5958b586576e6e02f2ea855 Mon Sep 17 00:00:00 2001 From: Adlai Holler Date: Sat, 7 May 2016 15:53:28 -0700 Subject: [PATCH 13/16] [ASPINRemoteImageDownloader] Replace instancetype with actual class name for shared instance --- AsyncDisplayKit/Details/ASPINRemoteImageDownloader.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/AsyncDisplayKit/Details/ASPINRemoteImageDownloader.h b/AsyncDisplayKit/Details/ASPINRemoteImageDownloader.h index 12c6b27376..b76b298173 100644 --- a/AsyncDisplayKit/Details/ASPINRemoteImageDownloader.h +++ b/AsyncDisplayKit/Details/ASPINRemoteImageDownloader.h @@ -11,6 +11,6 @@ @interface ASPINRemoteImageDownloader : NSObject -+ (instancetype)sharedDownloader; ++ (ASPINRemoteImageDownloader *)sharedDownloader; @end From f9cd9730f77361e0078b17ca9c83b7fe009aaf14 Mon Sep 17 00:00:00 2001 From: Adlai Holler Date: Sat, 7 May 2016 15:55:00 -0700 Subject: [PATCH 14/16] Add nullability to ASPINRemoteImageDownloader.h --- AsyncDisplayKit/Details/ASPINRemoteImageDownloader.h | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/AsyncDisplayKit/Details/ASPINRemoteImageDownloader.h b/AsyncDisplayKit/Details/ASPINRemoteImageDownloader.h index b76b298173..a1238272ed 100644 --- a/AsyncDisplayKit/Details/ASPINRemoteImageDownloader.h +++ b/AsyncDisplayKit/Details/ASPINRemoteImageDownloader.h @@ -9,8 +9,12 @@ #import #import "ASImageProtocols.h" +NS_ASSUME_NONNULL_BEGIN + @interface ASPINRemoteImageDownloader : NSObject + (ASPINRemoteImageDownloader *)sharedDownloader; @end + +NS_ASSUME_NONNULL_END From c0eb6cac09cf819f864ab753585977f70bd70f5b Mon Sep 17 00:00:00 2001 From: Garrett Moon Date: Mon, 9 May 2016 15:00:44 -0700 Subject: [PATCH 15/16] Add support for disabling progressive image rendering Differential Revision: https://phabricator.pinadmin.com/D89742 --- AsyncDisplayKit/ASMultiplexImageNode.h | 6 ++ AsyncDisplayKit/ASMultiplexImageNode.mm | 79 +++++++++++++++++-------- AsyncDisplayKit/ASNetworkImageNode.h | 6 ++ AsyncDisplayKit/ASNetworkImageNode.mm | 27 ++++++++- 4 files changed, 92 insertions(+), 26 deletions(-) diff --git a/AsyncDisplayKit/ASMultiplexImageNode.h b/AsyncDisplayKit/ASMultiplexImageNode.h index 73a74cbdeb..0826cd951b 100644 --- a/AsyncDisplayKit/ASMultiplexImageNode.h +++ b/AsyncDisplayKit/ASMultiplexImageNode.h @@ -117,6 +117,12 @@ typedef NS_ENUM(NSUInteger, ASMultiplexImageNodeErrorCode) { */ @property (nullable, nonatomic, readonly) ASImageIdentifier displayedImageIdentifier; +/** + * @abstract If the downloader implements progressive image rendering and this value is YES progressive renders of the + * image will be displayed as the image downloads. Defaults to YES. + */ +@property (nonatomic, assign, readwrite) BOOL shouldRenderProgressImages; + #if TARGET_OS_IOS /** * @abstract The image manager that this image node should use when requesting images from the Photos framework. If this is `nil` (the default), then `PHImageManager.defaultManager` is used. diff --git a/AsyncDisplayKit/ASMultiplexImageNode.mm b/AsyncDisplayKit/ASMultiplexImageNode.mm index cb83bb1156..e5d6e47fbe 100644 --- a/AsyncDisplayKit/ASMultiplexImageNode.mm +++ b/AsyncDisplayKit/ASMultiplexImageNode.mm @@ -85,6 +85,10 @@ typedef void(^ASMultiplexImageLoadCompletionBlock)(UIImage *image, id imageIdent ASDN::RecursiveMutex _downloadIdentifierLock; id _downloadIdentifier; + // Properties + ASDN::RecursiveMutex _propertyLock; + BOOL _shouldRenderProgressImages; + //set on init only BOOL _downloaderSupportsNewProtocol; BOOL _downloaderImplementsSetProgress; @@ -186,6 +190,8 @@ typedef void(^ASMultiplexImageLoadCompletionBlock)(UIImage *image, id imageIdent _cacheSupportsNewProtocol = [cache respondsToSelector:@selector(cachedImageWithURL:callbackQueue:completion:)]; _cacheSupportsClearing = [cache respondsToSelector:@selector(clearFetchedImageFromCacheWithURL:)]; + _shouldRenderProgressImages = YES; + self.shouldBypassEnsureDisplay = YES; return self; @@ -339,6 +345,27 @@ typedef void(^ASMultiplexImageLoadCompletionBlock)(UIImage *image, id imageIdent #endif } + +- (void)setShouldRenderProgressImages:(BOOL)shouldRenderProgressImages +{ + ASDN::MutexLocker l(_propertyLock); + if (shouldRenderProgressImages == _shouldRenderProgressImages) { + return; + } + + _shouldRenderProgressImages = shouldRenderProgressImages; + + + ASDN::MutexUnlocker u(_propertyLock); + [self _updateProgressImageBlockOnDownloaderIfNeeded]; +} + +- (BOOL)shouldRenderProgressImages +{ + ASDN::MutexLocker l(_propertyLock); + return _shouldRenderProgressImages; +} + #pragma mark - #pragma mark - @@ -458,32 +485,34 @@ typedef void(^ASMultiplexImageLoadCompletionBlock)(UIImage *image, id imageIdent */ - (void)_updateProgressImageBlockOnDownloaderIfNeeded { - // Read our interface state before locking so that we don't lock super while holding our lock. - ASInterfaceState interfaceState = self.interfaceState; - ASDN::MutexLocker l(_downloadIdentifierLock); - - if (!_downloaderImplementsSetProgress || _downloadIdentifier == nil) { + BOOL shouldRenderProgressImages = self.shouldRenderProgressImages; + + // Read our interface state before locking so that we don't lock super while holding our lock. + ASInterfaceState interfaceState = self.interfaceState; + ASDN::MutexLocker l(_downloadIdentifierLock); + + if (!_downloaderImplementsSetProgress || _downloadIdentifier == nil) { + return; + } + + ASImageDownloaderProgressImage progress = nil; + if (shouldRenderProgressImages && ASInterfaceStateIncludesVisible(interfaceState)) { + __weak __typeof__(self) weakSelf = self; + progress = ^(UIImage * _Nonnull progressImage, CGFloat progress, id _Nullable downloadIdentifier) { + __typeof__(self) strongSelf = weakSelf; + if (strongSelf == nil) { return; - } - - ASImageDownloaderProgressImage progress = nil; - if (ASInterfaceStateIncludesVisible(interfaceState)) { - __weak __typeof__(self) weakSelf = self; - progress = ^(UIImage * _Nonnull progressImage, CGFloat progress, id _Nullable downloadIdentifier) { - __typeof__(self) strongSelf = weakSelf; - if (strongSelf == nil) { - return; - } - - ASDN::MutexLocker l(strongSelf->_downloadIdentifierLock); - //Getting a result back for a different download identifier, download must not have been successfully canceled - if (ASObjectIsEqual(strongSelf->_downloadIdentifier, downloadIdentifier) == NO && downloadIdentifier != nil) { - return; - } - strongSelf.image = progressImage; - }; - } - [_downloader setProgressImageBlock:progress callbackQueue:dispatch_get_main_queue() withDownloadIdentifier:_downloadIdentifier]; + } + + ASDN::MutexLocker l(strongSelf->_downloadIdentifierLock); + //Getting a result back for a different download identifier, download must not have been successfully canceled + if (ASObjectIsEqual(strongSelf->_downloadIdentifier, downloadIdentifier) == NO && downloadIdentifier != nil) { + return; + } + strongSelf.image = progressImage; + }; + } + [_downloader setProgressImageBlock:progress callbackQueue:dispatch_get_main_queue() withDownloadIdentifier:_downloadIdentifier]; } - (void)_clearImage diff --git a/AsyncDisplayKit/ASNetworkImageNode.h b/AsyncDisplayKit/ASNetworkImageNode.h index ef3feba1e1..abbc800840 100644 --- a/AsyncDisplayKit/ASNetworkImageNode.h +++ b/AsyncDisplayKit/ASNetworkImageNode.h @@ -73,6 +73,12 @@ NS_ASSUME_NONNULL_BEGIN */ @property (nonatomic, assign, readwrite) BOOL shouldCacheImage; +/** + * If the downloader implements progressive image rendering and this value is YES progressive renders of the + * image will be displayed as the image downloads. Defaults to YES. + */ +@property (nonatomic, assign, readwrite) BOOL shouldRenderProgressImages; + /** * The image quality of the current image. This is a number between 0 and 1 and can be used to track * progressive progress. Calculated by dividing number of bytes / expected number of total bytes. diff --git a/AsyncDisplayKit/ASNetworkImageNode.mm b/AsyncDisplayKit/ASNetworkImageNode.mm index 27a677fe61..c52e01304d 100755 --- a/AsyncDisplayKit/ASNetworkImageNode.mm +++ b/AsyncDisplayKit/ASNetworkImageNode.mm @@ -45,6 +45,8 @@ static const CGSize kMinReleaseImageOnBackgroundSize = {20.0, 20.0}; BOOL _delegateSupportsDidStartFetchingData; BOOL _delegateSupportsDidFailWithError; BOOL _delegateSupportsImageNodeDidFinishDecoding; + + BOOL _shouldRenderProgressImages; //set on init only BOOL _downloaderSupportsNewProtocol; @@ -83,6 +85,7 @@ static const CGSize kMinReleaseImageOnBackgroundSize = {20.0, 20.0}; _cacheSupportsSynchronousFetch = [cache respondsToSelector:@selector(synchronouslyFetchedCachedImageWithURL:)]; _shouldCacheImage = YES; + _shouldRenderProgressImages = YES; self.shouldBypassEnsureDisplay = YES; return self; @@ -218,6 +221,26 @@ static const CGSize kMinReleaseImageOnBackgroundSize = {20.0, 20.0}; return _delegate; } +- (void)setShouldRenderProgressImages:(BOOL)shouldRenderProgressImages +{ + ASDN::MutexLocker l(_lock); + if (shouldRenderProgressImages == _shouldRenderProgressImages) { + return; + } + + _shouldRenderProgressImages = shouldRenderProgressImages; + + + ASDN::MutexUnlocker u(_lock); + [self _updateProgressImageBlockOnDownloaderIfNeeded]; +} + +- (BOOL)shouldRenderProgressImages +{ + ASDN::MutexLocker l(_lock); + return _shouldRenderProgressImages; +} + - (BOOL)placeholderShouldPersist { ASDN::MutexLocker l(_lock); @@ -309,6 +332,8 @@ static const CGSize kMinReleaseImageOnBackgroundSize = {20.0, 20.0}; */ - (void)_updateProgressImageBlockOnDownloaderIfNeeded { + BOOL shouldRenderProgressImages = self.shouldRenderProgressImages; + // Read our interface state before locking so that we don't lock super while holding our lock. ASInterfaceState interfaceState = self.interfaceState; ASDN::MutexLocker l(_lock); @@ -318,7 +343,7 @@ static const CGSize kMinReleaseImageOnBackgroundSize = {20.0, 20.0}; } ASImageDownloaderProgressImage progress = nil; - if (ASInterfaceStateIncludesVisible(interfaceState)) { + if (shouldRenderProgressImages && ASInterfaceStateIncludesVisible(interfaceState)) { __weak __typeof__(self) weakSelf = self; progress = ^(UIImage * _Nonnull progressImage, CGFloat progress, id _Nullable downloadIdentifier) { __typeof__(self) strongSelf = weakSelf; From 36ce9527de44324396c4884c6f181dc381eb1f6f Mon Sep 17 00:00:00 2001 From: Garrett Moon Date: Mon, 9 May 2016 17:20:06 -0700 Subject: [PATCH 16/16] Update comments --- AsyncDisplayKit/ASMultiplexImageNode.h | 3 ++- AsyncDisplayKit/ASNetworkImageNode.h | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/AsyncDisplayKit/ASMultiplexImageNode.h b/AsyncDisplayKit/ASMultiplexImageNode.h index 0826cd951b..eb01209dfd 100644 --- a/AsyncDisplayKit/ASMultiplexImageNode.h +++ b/AsyncDisplayKit/ASMultiplexImageNode.h @@ -119,7 +119,8 @@ typedef NS_ENUM(NSUInteger, ASMultiplexImageNodeErrorCode) { /** * @abstract If the downloader implements progressive image rendering and this value is YES progressive renders of the - * image will be displayed as the image downloads. Defaults to YES. + * image will be displayed as the image downloads. Regardless of this properties value, progress renders will + * only occur when the node is visible. Defaults to YES. */ @property (nonatomic, assign, readwrite) BOOL shouldRenderProgressImages; diff --git a/AsyncDisplayKit/ASNetworkImageNode.h b/AsyncDisplayKit/ASNetworkImageNode.h index abbc800840..d2aadfe854 100644 --- a/AsyncDisplayKit/ASNetworkImageNode.h +++ b/AsyncDisplayKit/ASNetworkImageNode.h @@ -75,7 +75,8 @@ NS_ASSUME_NONNULL_BEGIN /** * If the downloader implements progressive image rendering and this value is YES progressive renders of the - * image will be displayed as the image downloads. Defaults to YES. + * image will be displayed as the image downloads. Regardless of this properties value, progress renders will + * only occur when the node is visible. Defaults to YES. */ @property (nonatomic, assign, readwrite) BOOL shouldRenderProgressImages;