From 01c16809044e34ea663fbd52373e44720fe2eca3 Mon Sep 17 00:00:00 2001 From: Garrett Moon Date: Wed, 27 Jan 2016 20:05:03 -0800 Subject: [PATCH 1/5] Switch to instance methods of draw and display This patch switches to instance methods of draw and display for ASTextNode and ASImageNode to attempt to increase their performance. It also fixes some thread safety issues in ASImageNode which appear to have been regressions (though probably not hit very often). And it sets up work for allowing modification of CGContexts before and after a node's contents are drawn. --- AsyncDisplayKit/ASDisplayNode+Beta.h | 23 +++ AsyncDisplayKit/ASDisplayNode.h | 5 + AsyncDisplayKit/ASDisplayNode.mm | 6 +- AsyncDisplayKit/ASImageNode.mm | 140 +++++++++++------- AsyncDisplayKit/ASTextNode.mm | 76 ++++------ .../Private/ASDisplayNode+AsyncDisplay.mm | 59 +++++++- .../Private/ASDisplayNodeInternal.h | 5 + 7 files changed, 207 insertions(+), 107 deletions(-) diff --git a/AsyncDisplayKit/ASDisplayNode+Beta.h b/AsyncDisplayKit/ASDisplayNode+Beta.h index 473b565f27..3691abb08d 100644 --- a/AsyncDisplayKit/ASDisplayNode+Beta.h +++ b/AsyncDisplayKit/ASDisplayNode+Beta.h @@ -6,6 +6,8 @@ * of patent rights can be found in the PATENTS file in the same directory. */ +#import "_ASDisplayLayer.h" + @interface ASDisplayNode (Beta) + (BOOL)shouldUseNewRenderingRange; @@ -20,4 +22,25 @@ */ - (void)recursivelyEnsureDisplaySynchronously:(BOOL)synchronously; +- (void)drawRect:(CGRect)bounds withParameters:(id )parameters isCancelled:(asdisplaynode_iscancelled_block_t)isCancelledBlock isRasterizing:(BOOL)isRasterizing; + +- (UIImage *)displayWithParameters:(id )parameters isCancelled:(asdisplaynode_iscancelled_block_t)isCancelled; + +/** + * @abstract allow modification of a context before the node's content is drawn + * + * @discussion Set the block to be called after the context has been created and before the node's content is drawn. + * You can override this to modify the context before the content is drawn. You are responsible for saving and + * restoring context if necessary. Restoring can be done in contextDidDisplayNodeContent + * This block can be called from *any* thread and it is unsafe to access any UIKit main thread properties from it. + */ +@property (nonatomic, strong) ASDisplayNodeContextModifier willDisplayNodeContentBlock; + +/** + * @abstract allow modification of a context after the node's content is drawn + * + * @discussion + */ +@property (nonatomic, strong) ASDisplayNodeContextModifier didDisplayNodeContentBlock; + @end diff --git a/AsyncDisplayKit/ASDisplayNode.h b/AsyncDisplayKit/ASDisplayNode.h index 524b977fb7..701df672ad 100644 --- a/AsyncDisplayKit/ASDisplayNode.h +++ b/AsyncDisplayKit/ASDisplayNode.h @@ -37,6 +37,11 @@ typedef CALayer * _Nonnull(^ASDisplayNodeLayerBlock)(); */ typedef void (^ASDisplayNodeDidLoadBlock)(ASDisplayNode * _Nonnull node); +/** + * ASDisplayNode will / did render node content in context. + */ +typedef void (^ASDisplayNodeContextModifier)(CGContextRef context); + /** Interface state is available on ASDisplayNode and ASViewController, and allows checking whether a node is in an interface situation where it is prudent to trigger certain diff --git a/AsyncDisplayKit/ASDisplayNode.mm b/AsyncDisplayKit/ASDisplayNode.mm index bdde67d6ab..aa54b85671 100644 --- a/AsyncDisplayKit/ASDisplayNode.mm +++ b/AsyncDisplayKit/ASDisplayNode.mm @@ -110,8 +110,12 @@ static struct ASDisplayNodeFlags GetASDisplayNodeFlags(Class c, ASDisplayNode *i flags.implementsImageDisplay = ([c respondsToSelector:@selector(displayWithParameters:isCancelled:)] ? 1 : 0); if (instance) { flags.implementsDrawParameters = ([instance respondsToSelector:@selector(drawParametersForAsyncLayer:)] ? 1 : 0); + flags.implementsInstanceDrawRect = ([instance respondsToSelector:@selector(drawRect:withParameters:isCancelled:isRasterizing:)] ? 1 : 0); + flags.implementsInstanceImageDisplay = ([instance respondsToSelector:@selector(displayWithParameters:isCancelled:)] ? 1 : 0); } else { flags.implementsDrawParameters = ([c instancesRespondToSelector:@selector(drawParametersForAsyncLayer:)] ? 1 : 0); + flags.implementsInstanceDrawRect = ([c instancesRespondToSelector:@selector(drawRect:withParameters:isCancelled:isRasterizing:)] ? 1 : 0); + flags.implementsInstanceImageDisplay = ([c instancesRespondToSelector:@selector(displayWithParameters:isCancelled:)] ? 1 : 0); } return flags; } @@ -1491,7 +1495,7 @@ static NSInteger incrementIfFound(NSInteger i) { // Helper method to summarize whether or not the node run through the display process - (BOOL)__implementsDisplay { - return _flags.implementsDrawRect == YES || _flags.implementsImageDisplay == YES || self.shouldRasterizeDescendants; + return _flags.implementsDrawRect == YES || _flags.implementsImageDisplay == YES || self.shouldRasterizeDescendants || _flags.implementsInstanceDrawRect || _flags.implementsInstanceImageDisplay; } - (void)_setupPlaceholderLayer diff --git a/AsyncDisplayKit/ASImageNode.mm b/AsyncDisplayKit/ASImageNode.mm index c9024c7510..fda905e0ca 100644 --- a/AsyncDisplayKit/ASImageNode.mm +++ b/AsyncDisplayKit/ASImageNode.mm @@ -14,6 +14,7 @@ #import #import #import +#import #import "ASImageNode+CGExtras.h" @@ -22,42 +23,34 @@ @interface _ASImageNodeDrawParameters : NSObject -@property (nonatomic, assign, readonly) BOOL cropEnabled; @property (nonatomic, assign) BOOL opaque; -@property (nonatomic, retain) UIImage *image; @property (nonatomic, assign) CGRect bounds; @property (nonatomic, assign) CGFloat contentsScale; @property (nonatomic, retain) UIColor *backgroundColor; @property (nonatomic, assign) UIViewContentMode contentMode; -@property (nonatomic, assign) CGRect cropRect; -@property (nonatomic, copy) asimagenode_modification_block_t imageModificationBlock; @end // TODO: eliminate explicit parameters with a set of keys copied from the node @implementation _ASImageNodeDrawParameters -- (id)initWithCrop:(BOOL)cropEnabled opaque:(BOOL)opaque image:(UIImage *)image bounds:(CGRect)bounds contentsScale:(CGFloat)contentsScale backgroundColor:(UIColor *)backgroundColor contentMode:(UIViewContentMode)contentMode cropRect:(CGRect)cropRect imageModificationBlock:(asimagenode_modification_block_t)imageModificationBlock +- (id)initWithBounds:(CGRect)bounds opaque:(BOOL)opaque contentsScale:(CGFloat)contentsScale backgroundColor:(UIColor *)backgroundColor contentMode:(UIViewContentMode)contentMode { self = [self init]; if (!self) return nil; - _cropEnabled = cropEnabled; _opaque = opaque; - _image = image; _bounds = bounds; _contentsScale = contentsScale; _backgroundColor = backgroundColor; _contentMode = contentMode; - _cropRect = cropRect; - _imageModificationBlock = [imageModificationBlock copy]; return self; } - (NSString *)description { - return [NSString stringWithFormat:@"<%@ : %p image:%@ cropEnabled:%@ opaque:%@ bounds:%@ contentsScale:%.2f backgroundColor:%@ contentMode:%@ cropRect:%@>", [self class], self, self.image, @(self.cropEnabled), @(self.opaque), NSStringFromCGRect(self.bounds), self.contentsScale, self.backgroundColor, ASDisplayNodeNSStringFromUIContentMode(self.contentMode), NSStringFromCGRect(self.cropRect)]; + return [NSString stringWithFormat:@"<%@ : %p opaque:%@ bounds:%@ contentsScale:%.2f backgroundColor:%@ contentMode:%@>", [self class], self, @(self.opaque), NSStringFromCGRect(self.bounds), self.contentsScale, self.backgroundColor, ASDisplayNodeNSStringFromUIContentMode(self.contentMode)]; } @end @@ -78,6 +71,7 @@ } @synthesize image = _image; +@synthesize imageModificationBlock = _imageModificationBlock; - (id)init { @@ -153,88 +147,104 @@ { BOOL hasValidCropBounds = _cropEnabled && !CGRectIsNull(_cropDisplayBounds) && !CGRectIsEmpty(_cropDisplayBounds); - return [[_ASImageNodeDrawParameters alloc] initWithCrop:_cropEnabled - opaque:self.opaque - image:self.image - bounds:(hasValidCropBounds ? _cropDisplayBounds : self.bounds) - contentsScale:self.contentsScaleForDisplay - backgroundColor:self.backgroundColor - contentMode:self.contentMode - cropRect:self.cropRect - imageModificationBlock:self.imageModificationBlock]; + return [[_ASImageNodeDrawParameters alloc] initWithBounds:self.bounds + opaque:self.opaque + contentsScale:self.contentsScaleForDisplay + backgroundColor:self.backgroundColor + contentMode:self.contentMode]; } -+ (UIImage *)displayWithParameters:(_ASImageNodeDrawParameters *)parameters isCancelled:(asdisplaynode_iscancelled_block_t)isCancelled +- (UIImage *)displayWithParameters:(_ASImageNodeDrawParameters *)parameters isCancelled:(asdisplaynode_iscancelled_block_t)isCancelled { - UIImage *image = parameters.image; - + ASDN::MutexLocker l(_imageLock); + UIImage *image = _image; if (!image) { return nil; } - - ASDisplayNodeAssert(parameters.contentsScale > 0, @"invalid contentsScale at display time"); - + + BOOL cropEnabled = _cropEnabled; + CGFloat contentsScale = _contentsScaleForDisplay; + CGRect cropDisplayBounds = _cropDisplayBounds; + CGRect cropRect = _cropRect; + asimagenode_modification_block_t imageModificationBlock = _imageModificationBlock; + + ASDN::MutexUnlocker u(_imageLock); + + ASDisplayNodeContextModifier preContextBlock = self.willDisplayNodeContentBlock; + ASDisplayNodeContextModifier postContextBlock = self.didDisplayNodeContentBlock; + + BOOL hasValidCropBounds = cropEnabled && !CGRectIsNull(cropDisplayBounds) && !CGRectIsEmpty(cropDisplayBounds); + + CGRect bounds = (hasValidCropBounds ? cropDisplayBounds : parameters.bounds); + BOOL isOpaque = parameters.opaque; + CGFloat contentsScaleForDisplay = parameters.contentsScale; + UIColor *backgroundColor = parameters.backgroundColor; + UIViewContentMode contentMode = parameters.contentMode; + + ASDisplayNodeAssert(contentsScale > 0, @"invalid contentsScale at display time"); + // if the image is resizable, bail early since the image has likely already been configured BOOL stretchable = !UIEdgeInsetsEqualToEdgeInsets(image.capInsets, UIEdgeInsetsZero); if (stretchable) { - if (parameters.imageModificationBlock != NULL) { - image = parameters.imageModificationBlock(image); + if (imageModificationBlock != NULL) { + image = imageModificationBlock(image); } return image; } - - CGRect bounds = parameters.bounds; - - CGFloat contentsScale = parameters.contentsScale; - UIViewContentMode contentMode = parameters.contentMode; + CGSize imageSize = image.size; CGSize imageSizeInPixels = CGSizeMake(imageSize.width * image.scale, imageSize.height * image.scale); CGSize boundsSizeInPixels = CGSizeMake(floorf(bounds.size.width * contentsScale), floorf(bounds.size.height * contentsScale)); - + BOOL contentModeSupported = contentMode == UIViewContentModeScaleAspectFill - || contentMode == UIViewContentModeScaleAspectFit - || contentMode == UIViewContentModeCenter; - + || contentMode == UIViewContentModeScaleAspectFit + || contentMode == UIViewContentModeCenter; + CGSize backingSize; CGRect imageDrawRect; - + if (boundsSizeInPixels.width * contentsScale < 1.0f || boundsSizeInPixels.height * contentsScale < 1.0f || imageSizeInPixels.width < 1.0f || imageSizeInPixels.height < 1.0f) { return nil; } - + // If we're not supposed to do any cropping, just decode image at original size - if (!parameters.cropEnabled || !contentModeSupported || stretchable) { + if (!cropEnabled || !contentModeSupported || stretchable) { backingSize = imageSizeInPixels; imageDrawRect = (CGRect){.size = backingSize}; } else { ASCroppedImageBackingSizeAndDrawRectInBounds(imageSizeInPixels, boundsSizeInPixels, contentMode, - parameters.cropRect, + cropRect, &backingSize, &imageDrawRect); } - + if (backingSize.width <= 0.0f || backingSize.height <= 0.0f || imageDrawRect.size.width <= 0.0f || imageDrawRect.size.height <= 0.0f) { return nil; } - + // Use contentsScale of 1.0 and do the contentsScale handling in boundsSizeInPixels so ASCroppedImageBackingSizeAndDrawRectInBounds // will do its rounding on pixel instead of point boundaries - UIGraphicsBeginImageContextWithOptions(backingSize, parameters.opaque, 1.0); + UIGraphicsBeginImageContextWithOptions(backingSize, isOpaque, 1.0); // if view is opaque, fill the context with background color - if (parameters.opaque && parameters.backgroundColor) { - [parameters.backgroundColor setFill]; + if (isOpaque && backgroundColor) { + [backgroundColor setFill]; UIRectFill({ .size = backingSize }); } - + + CGContextRef context = UIGraphicsGetCurrentContext(); + if (preContextBlock) { + preContextBlock(context); + } + // iOS 9 appears to contain a thread safety regression when drawing the same CGImageRef on // multiple threads concurrently. In fact, instead of crashing, it appears to deadlock. // The issue is present in Mac OS X El Capitan and has been seen hanging Pro apps like Adobe Premier, @@ -250,20 +260,24 @@ @synchronized(image) { [image drawInRect:imageDrawRect]; } - + + if (postContextBlock) { + postContextBlock(context); + } + if (isCancelled()) { UIGraphicsEndImageContext(); return nil; } - + UIImage *result = UIGraphicsGetImageFromCurrentImageContext(); - + UIGraphicsEndImageContext(); - - if (parameters.imageModificationBlock != NULL) { - result = parameters.imageModificationBlock(result); + + if (imageModificationBlock != NULL) { + result = imageModificationBlock(result); } - + return result; } @@ -271,8 +285,9 @@ { [super displayDidFinish]; + ASDN::MutexLocker l(_imageLock); // If we've got a block to perform after displaying, do it. - if (self.image && _displayCompletionBlock) { + if (_image && _displayCompletionBlock) { // FIXME: _displayCompletionBlock is not protected by lock _displayCompletionBlock(NO); @@ -291,6 +306,7 @@ // Stash the block and call-site queue. We'll invoke it in -displayDidFinish. // FIXME: _displayCompletionBlock not protected by lock + ASDN::MutexLocker l(_imageLock); if (_displayCompletionBlock != displayCompletionBlock) { _displayCompletionBlock = [displayCompletionBlock copy]; } @@ -301,6 +317,7 @@ #pragma mark - Cropping - (BOOL)isCropEnabled { + ASDN::MutexLocker l(_imageLock); return _cropEnabled; } @@ -311,6 +328,7 @@ - (void)setCropEnabled:(BOOL)cropEnabled recropImmediately:(BOOL)recropImmediately inBounds:(CGRect)cropBounds { + ASDN::MutexLocker l(_imageLock); if (_cropEnabled == cropEnabled) return; @@ -331,11 +349,13 @@ - (CGRect)cropRect { + ASDN::MutexLocker l(_imageLock); return _cropRect; } - (void)setCropRect:(CGRect)cropRect { + ASDN::MutexLocker l(_imageLock); if (CGRectEqualToRect(_cropRect, cropRect)) return; @@ -354,6 +374,18 @@ }); } +- (asimagenode_modification_block_t)imageModificationBlock +{ + ASDN::MutexLocker l(_imageLock); + return _imageModificationBlock; +} + +- (void)setImageModificationBlock:(asimagenode_modification_block_t)imageModificationBlock +{ + ASDN::MutexLocker l(_imageLock); + _imageModificationBlock = imageModificationBlock; +} + @end diff --git a/AsyncDisplayKit/ASTextNode.mm b/AsyncDisplayKit/ASTextNode.mm index 8dbea6eb57..2741dda576 100644 --- a/AsyncDisplayKit/ASTextNode.mm +++ b/AsyncDisplayKit/ASTextNode.mm @@ -34,13 +34,7 @@ static NSString *ASTextNodeTruncationTokenAttributeName = @"ASTextNodeTruncation @interface ASTextNodeDrawParameters : NSObject -- (instancetype)initWithRenderer:(ASTextKitRenderer *)renderer - textOrigin:(CGPoint)textOrigin - backgroundColor:(UIColor *)backgroundColor; - -@property (nonatomic, strong, readonly) ASTextKitRenderer *renderer; - -@property (nonatomic, assign, readonly) CGPoint textOrigin; +@property (nonatomic, assign, readonly) CGRect bounds; @property (nonatomic, strong, readonly) UIColor *backgroundColor; @@ -48,30 +42,16 @@ static NSString *ASTextNodeTruncationTokenAttributeName = @"ASTextNodeTruncation @implementation ASTextNodeDrawParameters -- (instancetype)initWithRenderer:(ASTextKitRenderer *)renderer - textOrigin:(CGPoint)textOrigin - backgroundColor:(UIColor *)backgroundColor +- (instancetype)initWithBounds:(CGRect)bounds + backgroundColor:(UIColor *)backgroundColor { if (self = [super init]) { - _renderer = renderer; - _textOrigin = textOrigin; + _bounds = bounds; _backgroundColor = backgroundColor; } return self; } -- (void)dealloc -{ - // Destruction of the layout managers/containers/text storage is quite - // expensive, and can take some time, so we dispatch onto a bg queue to - // actually dealloc. - __block ASTextKitRenderer *renderer = _renderer; - ASPerformBlockOnBackgroundThread(^{ - renderer = nil; - }); - _renderer = nil; -} - @end @interface ASTextNode () @@ -237,11 +217,17 @@ static NSArray *DefaultLinkAttributeNames = @[ NSLinkAttributeName ]; #pragma mark - Renderer Management +//only safe to call on the main thread - (ASTextKitRenderer *)_renderer +{ + return [self _rendererWithBounds:self.bounds]; +} + +- (ASTextKitRenderer *)_rendererWithBounds:(CGRect)bounds { ASDN::MutexLocker l(_rendererLock); if (_renderer == nil) { - CGSize constrainedSize = _constrainedSize.width != -INFINITY ? _constrainedSize : self.bounds.size; + CGSize constrainedSize = _constrainedSize.width != -INFINITY ? _constrainedSize : bounds.size; _renderer = [[ASTextKitRenderer alloc] initWithTextKitAttributes:[self _rendererAttributes] constrainedSize:constrainedSize]; } @@ -420,13 +406,17 @@ static NSArray *DefaultLinkAttributeNames = @[ NSLinkAttributeName ]; #pragma mark - Drawing -+ (void)drawRect:(CGRect)bounds withParameters:(ASTextNodeDrawParameters *)parameters isCancelled:(asdisplaynode_iscancelled_block_t)isCancelledBlock isRasterizing:(BOOL)isRasterizing +- (void)drawRect:(CGRect)bounds withParameters:(ASTextNodeDrawParameters *)parameters isCancelled:(asdisplaynode_iscancelled_block_t)isCancelledBlock isRasterizing:(BOOL)isRasterizing { CGContextRef context = UIGraphicsGetCurrentContext(); ASDisplayNodeAssert(context, @"This is no good without a context."); - + CGContextSaveGState(context); - + + ASTextKitRenderer *renderer = [self _rendererWithBounds:parameters.bounds]; + UIEdgeInsets shadowPadding = [self shadowPaddingWithRenderer:renderer]; + CGPoint textOrigin = CGPointMake(parameters.bounds.origin.x - shadowPadding.left, parameters.bounds.origin.y - shadowPadding.top); + // Fill background if (!isRasterizing) { UIColor *backgroundColor = parameters.backgroundColor; @@ -435,28 +425,20 @@ static NSArray *DefaultLinkAttributeNames = @[ NSLinkAttributeName ]; UIRectFillUsingBlendMode(CGContextGetClipBoundingBox(context), kCGBlendModeCopy); } } - + // Draw shadow - [[parameters.renderer shadower] setShadowInContext:context]; - + [[renderer shadower] setShadowInContext:context]; + // Draw text - bounds.origin = parameters.textOrigin; - [parameters.renderer drawInContext:context bounds:bounds]; - + bounds.origin = textOrigin; + [renderer drawInContext:context bounds:bounds]; + CGContextRestoreGState(context); } - (NSObject *)drawParametersForAsyncLayer:(_ASDisplayLayer *)layer { - CGRect bounds = self.bounds; - [self _invalidateRendererIfNeededForBoundsSize:bounds.size]; - - // Offset the text origin by any shadow padding - UIEdgeInsets shadowPadding = [self shadowPadding]; - CGPoint textOrigin = CGPointMake(bounds.origin.x - shadowPadding.left, bounds.origin.y - shadowPadding.top); - return [[ASTextNodeDrawParameters alloc] initWithRenderer:[self _renderer] - textOrigin:textOrigin - backgroundColor:self.backgroundColor]; + return [[ASTextNodeDrawParameters alloc] initWithBounds:self.bounds backgroundColor:self.backgroundColor]; } #pragma mark - Attributes @@ -1016,9 +998,15 @@ static NSArray *DefaultLinkAttributeNames = @[ NSLinkAttributeName ]; } } +//only safe to call on main thread - (UIEdgeInsets)shadowPadding { - return [self _renderer].shadower.shadowPadding; + return [self shadowPaddingWithRenderer:[self _renderer]]; +} + +- (UIEdgeInsets)shadowPaddingWithRenderer:(ASTextKitRenderer *)renderer +{ + return renderer.shadower.shadowPadding; } #pragma mark - Truncation Message diff --git a/AsyncDisplayKit/Private/ASDisplayNode+AsyncDisplay.mm b/AsyncDisplayKit/Private/ASDisplayNode+AsyncDisplay.mm index a642275305..e60257114e 100644 --- a/AsyncDisplayKit/Private/ASDisplayNode+AsyncDisplay.mm +++ b/AsyncDisplayKit/Private/ASDisplayNode+AsyncDisplay.mm @@ -12,6 +12,7 @@ #import "ASAssert.h" #import "ASDisplayNodeInternal.h" #import "ASDisplayNode+FrameworkPrivate.h" +#import "ASDisplayNode+Beta.h" @interface ASDisplayNode () <_ASDisplayLayerDelegate> @end @@ -224,25 +225,30 @@ static void __ASDisplayLayerDecrementConcurrentDisplayCount(BOOL displayIsAsync, return image; }; - } else if (_flags.implementsImageDisplay) { + } else if (_flags.implementsInstanceImageDisplay || _flags.implementsImageDisplay) { // Capture drawParameters from delegate on main thread id drawParameters = [self drawParameters]; - + displayBlock = ^id{ __ASDisplayLayerIncrementConcurrentDisplayCount(asynchronous, rasterizing); if (isCancelledBlock()) { __ASDisplayLayerDecrementConcurrentDisplayCount(asynchronous, rasterizing); return nil; } - + ASDN_DELAY_FOR_DISPLAY(); - - UIImage *result = [[self class] displayWithParameters:drawParameters isCancelled:isCancelledBlock]; + + UIImage *result = nil; + if (_flags.implementsInstanceImageDisplay) { + result = [self displayWithParameters:drawParameters isCancelled:isCancelledBlock]; + } else { + result = [[self class] displayWithParameters:drawParameters isCancelled:isCancelledBlock]; + } __ASDisplayLayerDecrementConcurrentDisplayCount(asynchronous, rasterizing); return result; }; - - } else if (_flags.implementsDrawRect) { + + } else if (_flags.implementsInstanceDrawRect || _flags.implementsDrawRect) { CGRect bounds = self.bounds; if (CGRectIsEmpty(bounds)) { @@ -267,7 +273,20 @@ static void __ASDisplayLayerDecrementConcurrentDisplayCount(BOOL displayIsAsync, UIGraphicsBeginImageContextWithOptions(bounds.size, opaque, contentsScaleForDisplay); } - [[self class] drawRect:bounds withParameters:drawParameters isCancelled:isCancelledBlock isRasterizing:rasterizing]; + CGContextRef currentContext = UIGraphicsGetCurrentContext(); + if (_preContextModifier) { + _preContextModifier(currentContext); + } + + if (_flags.implementsInstanceDrawRect) { + [self drawRect:bounds withParameters:drawParameters isCancelled:isCancelledBlock isRasterizing:rasterizing]; + } else { + [[self class] drawRect:bounds withParameters:drawParameters isCancelled:isCancelledBlock isRasterizing:rasterizing]; + } + + if (_postContextModifier) { + _postContextModifier(currentContext); + } if (isCancelledBlock()) { if (!rasterizing) { @@ -370,4 +389,28 @@ static void __ASDisplayLayerDecrementConcurrentDisplayCount(BOOL displayIsAsync, [_displaySentinel increment]; } +- (ASDisplayNodeContextModifier)willDisplayNodeContentBlock +{ + ASDN::MutexLocker l(_propertyLock); + return _preContextModifier; +} + +- (ASDisplayNodeContextModifier)didDisplayNodeContentBlock +{ + ASDN::MutexLocker l(_propertyLock); + return _postContextModifier; +} + +- (void)setWillDisplayNodeContentBlock:(ASDisplayNodeContextModifier)contextModifier +{ + ASDN::MutexLocker l(_propertyLock); + _preContextModifier = contextModifier; +} + +- (void)setDidDisplayNodeContentBlock:(ASDisplayNodeContextModifier)contextModifier; +{ + ASDN::MutexLocker l(_propertyLock); + _postContextModifier = contextModifier; +} + @end diff --git a/AsyncDisplayKit/Private/ASDisplayNodeInternal.h b/AsyncDisplayKit/Private/ASDisplayNodeInternal.h index 4bac260403..a1864518ce 100644 --- a/AsyncDisplayKit/Private/ASDisplayNodeInternal.h +++ b/AsyncDisplayKit/Private/ASDisplayNodeInternal.h @@ -87,7 +87,9 @@ typedef NS_OPTIONS(NSUInteger, ASDisplayNodeMethodOverrides) unsigned hasCustomDrawingPriority:1; // whether custom drawing is enabled + unsigned implementsInstanceDrawRect:1; unsigned implementsDrawRect:1; + unsigned implementsInstanceImageDisplay:1; unsigned implementsImageDisplay:1; unsigned implementsDrawParameters:1; @@ -101,6 +103,9 @@ typedef NS_OPTIONS(NSUInteger, ASDisplayNodeMethodOverrides) ASDisplayNodeExtraIvars _extra; + ASDisplayNodeContextModifier _preContextModifier; + ASDisplayNodeContextModifier _postContextModifier; + #if TIME_DISPLAYNODE_OPS @public NSTimeInterval _debugTimeToCreateView; From d7d36c0a6b91b3ecc991ecc77622094d32c9e2cd Mon Sep 17 00:00:00 2001 From: Garrett Moon Date: Wed, 27 Jan 2016 21:08:04 -0800 Subject: [PATCH 2/5] Addressing Scott's comments --- AsyncDisplayKit/ASDisplayNode+Beta.h | 10 ++------ AsyncDisplayKit/ASImageNode.mm | 17 +++++++++---- .../Private/ASDisplayNode+AsyncDisplay.mm | 24 +++++++++---------- .../Private/ASDisplayNode+FrameworkPrivate.h | 13 ++++++++++ .../Private/ASDisplayNodeInternal.h | 4 ++-- 5 files changed, 42 insertions(+), 26 deletions(-) diff --git a/AsyncDisplayKit/ASDisplayNode+Beta.h b/AsyncDisplayKit/ASDisplayNode+Beta.h index 3691abb08d..378c060455 100644 --- a/AsyncDisplayKit/ASDisplayNode+Beta.h +++ b/AsyncDisplayKit/ASDisplayNode+Beta.h @@ -6,8 +6,6 @@ * of patent rights can be found in the PATENTS file in the same directory. */ -#import "_ASDisplayLayer.h" - @interface ASDisplayNode (Beta) + (BOOL)shouldUseNewRenderingRange; @@ -22,10 +20,6 @@ */ - (void)recursivelyEnsureDisplaySynchronously:(BOOL)synchronously; -- (void)drawRect:(CGRect)bounds withParameters:(id )parameters isCancelled:(asdisplaynode_iscancelled_block_t)isCancelledBlock isRasterizing:(BOOL)isRasterizing; - -- (UIImage *)displayWithParameters:(id )parameters isCancelled:(asdisplaynode_iscancelled_block_t)isCancelled; - /** * @abstract allow modification of a context before the node's content is drawn * @@ -34,13 +28,13 @@ * restoring context if necessary. Restoring can be done in contextDidDisplayNodeContent * This block can be called from *any* thread and it is unsafe to access any UIKit main thread properties from it. */ -@property (nonatomic, strong) ASDisplayNodeContextModifier willDisplayNodeContentBlock; +@property (nonatomic, strong) ASDisplayNodeContextModifier willDisplayNodeContentWithRenderingContext; /** * @abstract allow modification of a context after the node's content is drawn * * @discussion */ -@property (nonatomic, strong) ASDisplayNodeContextModifier didDisplayNodeContentBlock; +@property (nonatomic, strong) ASDisplayNodeContextModifier didDisplayNodeContentWithRenderingContext; @end diff --git a/AsyncDisplayKit/ASImageNode.mm b/AsyncDisplayKit/ASImageNode.mm index fda905e0ca..3239328aa0 100644 --- a/AsyncDisplayKit/ASImageNode.mm +++ b/AsyncDisplayKit/ASImageNode.mm @@ -170,8 +170,8 @@ ASDN::MutexUnlocker u(_imageLock); - ASDisplayNodeContextModifier preContextBlock = self.willDisplayNodeContentBlock; - ASDisplayNodeContextModifier postContextBlock = self.didDisplayNodeContentBlock; + ASDisplayNodeContextModifier preContextBlock = self.willDisplayNodeContentWithRenderingContext; + ASDisplayNodeContextModifier postContextBlock = self.didDisplayNodeContentWithRenderingContext; BOOL hasValidCropBounds = cropEnabled && !CGRectIsNull(cropDisplayBounds) && !CGRectIsEmpty(cropDisplayBounds); @@ -286,12 +286,21 @@ [super displayDidFinish]; ASDN::MutexLocker l(_imageLock); + + void (^displayCompletionBlock)(BOOL canceled) = _displayCompletionBlock; + UIImage *image = _image; + + ASDN::MutexLocker u(_imageLock); + // If we've got a block to perform after displaying, do it. - if (_image && _displayCompletionBlock) { + if (image && displayCompletionBlock) { // FIXME: _displayCompletionBlock is not protected by lock - _displayCompletionBlock(NO); + displayCompletionBlock(NO); + + ASDN::MutexLocker l(_imageLock); _displayCompletionBlock = nil; + ASDN::MutexLocker u(_imageLock); } } diff --git a/AsyncDisplayKit/Private/ASDisplayNode+AsyncDisplay.mm b/AsyncDisplayKit/Private/ASDisplayNode+AsyncDisplay.mm index e60257114e..f3e09b61bc 100644 --- a/AsyncDisplayKit/Private/ASDisplayNode+AsyncDisplay.mm +++ b/AsyncDisplayKit/Private/ASDisplayNode+AsyncDisplay.mm @@ -274,8 +274,8 @@ static void __ASDisplayLayerDecrementConcurrentDisplayCount(BOOL displayIsAsync, } CGContextRef currentContext = UIGraphicsGetCurrentContext(); - if (_preContextModifier) { - _preContextModifier(currentContext); + if (_willDisplayNodeContentWithRenderingContext) { + _willDisplayNodeContentWithRenderingContext(currentContext); } if (_flags.implementsInstanceDrawRect) { @@ -284,8 +284,8 @@ static void __ASDisplayLayerDecrementConcurrentDisplayCount(BOOL displayIsAsync, [[self class] drawRect:bounds withParameters:drawParameters isCancelled:isCancelledBlock isRasterizing:rasterizing]; } - if (_postContextModifier) { - _postContextModifier(currentContext); + if (_didDisplayNodeContentWithRenderingContext) { + _didDisplayNodeContentWithRenderingContext(currentContext); } if (isCancelledBlock()) { @@ -389,28 +389,28 @@ static void __ASDisplayLayerDecrementConcurrentDisplayCount(BOOL displayIsAsync, [_displaySentinel increment]; } -- (ASDisplayNodeContextModifier)willDisplayNodeContentBlock +- (ASDisplayNodeContextModifier)willDisplayNodeContentWithRenderingContext { ASDN::MutexLocker l(_propertyLock); - return _preContextModifier; + return _willDisplayNodeContentWithRenderingContext; } -- (ASDisplayNodeContextModifier)didDisplayNodeContentBlock +- (ASDisplayNodeContextModifier)didDisplayNodeContentWithRenderingContext { ASDN::MutexLocker l(_propertyLock); - return _postContextModifier; + return _didDisplayNodeContentWithRenderingContext; } -- (void)setWillDisplayNodeContentBlock:(ASDisplayNodeContextModifier)contextModifier +- (void)setWillDisplayNodeContentWithRenderingContext:(ASDisplayNodeContextModifier)contextModifier { ASDN::MutexLocker l(_propertyLock); - _preContextModifier = contextModifier; + _willDisplayNodeContentWithRenderingContext = contextModifier; } -- (void)setDidDisplayNodeContentBlock:(ASDisplayNodeContextModifier)contextModifier; +- (void)setDidDisplayNodeContentWithRenderingContext:(ASDisplayNodeContextModifier)contextModifier; { ASDN::MutexLocker l(_propertyLock); - _postContextModifier = contextModifier; + _didDisplayNodeContentWithRenderingContext = contextModifier; } @end diff --git a/AsyncDisplayKit/Private/ASDisplayNode+FrameworkPrivate.h b/AsyncDisplayKit/Private/ASDisplayNode+FrameworkPrivate.h index d3a5e711af..7384a5e839 100644 --- a/AsyncDisplayKit/Private/ASDisplayNode+FrameworkPrivate.h +++ b/AsyncDisplayKit/Private/ASDisplayNode+FrameworkPrivate.h @@ -17,6 +17,7 @@ #import "ASSentinel.h" #import "ASThread.h" #import "ASLayoutOptions.h" +#import "_ASDisplayLayer.h" NS_ASSUME_NONNULL_BEGIN @@ -100,6 +101,18 @@ typedef NS_OPTIONS(NSUInteger, ASHierarchyState) */ - (void)recursivelyEnsureDisplaySynchronously:(BOOL)synchronously; +/** + * @abstract instance version of drawRect class method + * @see drawRect:withParameters:isCancelled:isRasterizing class method + */ +- (void)drawRect:(CGRect)bounds withParameters:(id )parameters isCancelled:(asdisplaynode_iscancelled_block_t)isCancelledBlock isRasterizing:(BOOL)isRasterizing; + +/** + * @abstract instance version of display class method + * @see displayWithParameters:isCancelled class method + */ +- (UIImage *)displayWithParameters:(id )parameters isCancelled:(asdisplaynode_iscancelled_block_t)isCancelled; + /** * @abstract Allows a node to bypass all ensureDisplay passes. Defaults to NO. * diff --git a/AsyncDisplayKit/Private/ASDisplayNodeInternal.h b/AsyncDisplayKit/Private/ASDisplayNodeInternal.h index a1864518ce..d688289e15 100644 --- a/AsyncDisplayKit/Private/ASDisplayNodeInternal.h +++ b/AsyncDisplayKit/Private/ASDisplayNodeInternal.h @@ -103,8 +103,8 @@ typedef NS_OPTIONS(NSUInteger, ASDisplayNodeMethodOverrides) ASDisplayNodeExtraIvars _extra; - ASDisplayNodeContextModifier _preContextModifier; - ASDisplayNodeContextModifier _postContextModifier; + ASDisplayNodeContextModifier _willDisplayNodeContentWithRenderingContext; + ASDisplayNodeContextModifier _didDisplayNodeContentWithRenderingContext; #if TIME_DISPLAYNODE_OPS @public From 48150668c65f2b168e658a4251f42d4b808fa14e Mon Sep 17 00:00:00 2001 From: Garrett Moon Date: Thu, 28 Jan 2016 08:33:18 -0800 Subject: [PATCH 3/5] Fix up build --- AsyncDisplayKit/ASDisplayNode.h | 2 +- AsyncDisplayKit/ASImageNode.mm | 3 --- AsyncDisplayKit/Details/_ASDisplayLayer.h | 12 ++++++++++++ .../Private/ASDisplayNode+FrameworkPrivate.h | 12 ------------ 4 files changed, 13 insertions(+), 16 deletions(-) diff --git a/AsyncDisplayKit/ASDisplayNode.h b/AsyncDisplayKit/ASDisplayNode.h index 701df672ad..b10ebd5f0e 100644 --- a/AsyncDisplayKit/ASDisplayNode.h +++ b/AsyncDisplayKit/ASDisplayNode.h @@ -40,7 +40,7 @@ typedef void (^ASDisplayNodeDidLoadBlock)(ASDisplayNode * _Nonnull node); /** * ASDisplayNode will / did render node content in context. */ -typedef void (^ASDisplayNodeContextModifier)(CGContextRef context); +typedef void (^ASDisplayNodeContextModifier)(_Nonnull CGContextRef context); /** Interface state is available on ASDisplayNode and ASViewController, and diff --git a/AsyncDisplayKit/ASImageNode.mm b/AsyncDisplayKit/ASImageNode.mm index 3239328aa0..d87dea3591 100644 --- a/AsyncDisplayKit/ASImageNode.mm +++ b/AsyncDisplayKit/ASImageNode.mm @@ -145,8 +145,6 @@ - (NSObject *)drawParametersForAsyncLayer:(_ASDisplayLayer *)layer; { - BOOL hasValidCropBounds = _cropEnabled && !CGRectIsNull(_cropDisplayBounds) && !CGRectIsEmpty(_cropDisplayBounds); - return [[_ASImageNodeDrawParameters alloc] initWithBounds:self.bounds opaque:self.opaque contentsScale:self.contentsScaleForDisplay @@ -177,7 +175,6 @@ CGRect bounds = (hasValidCropBounds ? cropDisplayBounds : parameters.bounds); BOOL isOpaque = parameters.opaque; - CGFloat contentsScaleForDisplay = parameters.contentsScale; UIColor *backgroundColor = parameters.backgroundColor; UIViewContentMode contentMode = parameters.contentMode; diff --git a/AsyncDisplayKit/Details/_ASDisplayLayer.h b/AsyncDisplayKit/Details/_ASDisplayLayer.h index 6927348842..95e59660a9 100644 --- a/AsyncDisplayKit/Details/_ASDisplayLayer.h +++ b/AsyncDisplayKit/Details/_ASDisplayLayer.h @@ -94,6 +94,18 @@ typedef BOOL(^asdisplaynode_iscancelled_block_t)(void); */ + (UIImage *)displayWithParameters:(id)parameters isCancelled:(asdisplaynode_iscancelled_block_t)isCancelledBlock; +/** + * @abstract instance version of drawRect class method + * @see drawRect:withParameters:isCancelled:isRasterizing class method + */ +- (void)drawRect:(CGRect)bounds withParameters:(id )parameters isCancelled:(asdisplaynode_iscancelled_block_t)isCancelledBlock isRasterizing:(BOOL)isRasterizing; + +/** + * @abstract instance version of display class method + * @see displayWithParameters:isCancelled class method + */ +- (UIImage *)displayWithParameters:(id )parameters isCancelled:(asdisplaynode_iscancelled_block_t)isCancelled; + // Called on the main thread only /** diff --git a/AsyncDisplayKit/Private/ASDisplayNode+FrameworkPrivate.h b/AsyncDisplayKit/Private/ASDisplayNode+FrameworkPrivate.h index 7384a5e839..4fdaa8632a 100644 --- a/AsyncDisplayKit/Private/ASDisplayNode+FrameworkPrivate.h +++ b/AsyncDisplayKit/Private/ASDisplayNode+FrameworkPrivate.h @@ -101,18 +101,6 @@ typedef NS_OPTIONS(NSUInteger, ASHierarchyState) */ - (void)recursivelyEnsureDisplaySynchronously:(BOOL)synchronously; -/** - * @abstract instance version of drawRect class method - * @see drawRect:withParameters:isCancelled:isRasterizing class method - */ -- (void)drawRect:(CGRect)bounds withParameters:(id )parameters isCancelled:(asdisplaynode_iscancelled_block_t)isCancelledBlock isRasterizing:(BOOL)isRasterizing; - -/** - * @abstract instance version of display class method - * @see displayWithParameters:isCancelled class method - */ -- (UIImage *)displayWithParameters:(id )parameters isCancelled:(asdisplaynode_iscancelled_block_t)isCancelled; - /** * @abstract Allows a node to bypass all ensureDisplay passes. Defaults to NO. * From 7b002c408df15f1e2c5ccab79fe5003c01e44402 Mon Sep 17 00:00:00 2001 From: Garrett Moon Date: Thu, 28 Jan 2016 08:38:40 -0800 Subject: [PATCH 4/5] Ensure context is non-null --- AsyncDisplayKit/ASImageNode.mm | 4 ++-- AsyncDisplayKit/Private/ASDisplayNode+AsyncDisplay.mm | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/AsyncDisplayKit/ASImageNode.mm b/AsyncDisplayKit/ASImageNode.mm index d87dea3591..3864ca5ad6 100644 --- a/AsyncDisplayKit/ASImageNode.mm +++ b/AsyncDisplayKit/ASImageNode.mm @@ -238,7 +238,7 @@ } CGContextRef context = UIGraphicsGetCurrentContext(); - if (preContextBlock) { + if (context && preContextBlock) { preContextBlock(context); } @@ -258,7 +258,7 @@ [image drawInRect:imageDrawRect]; } - if (postContextBlock) { + if (context && postContextBlock) { postContextBlock(context); } diff --git a/AsyncDisplayKit/Private/ASDisplayNode+AsyncDisplay.mm b/AsyncDisplayKit/Private/ASDisplayNode+AsyncDisplay.mm index f3e09b61bc..24100027a9 100644 --- a/AsyncDisplayKit/Private/ASDisplayNode+AsyncDisplay.mm +++ b/AsyncDisplayKit/Private/ASDisplayNode+AsyncDisplay.mm @@ -274,7 +274,7 @@ static void __ASDisplayLayerDecrementConcurrentDisplayCount(BOOL displayIsAsync, } CGContextRef currentContext = UIGraphicsGetCurrentContext(); - if (_willDisplayNodeContentWithRenderingContext) { + if (currentContext && _willDisplayNodeContentWithRenderingContext) { _willDisplayNodeContentWithRenderingContext(currentContext); } @@ -284,7 +284,7 @@ static void __ASDisplayLayerDecrementConcurrentDisplayCount(BOOL displayIsAsync, [[self class] drawRect:bounds withParameters:drawParameters isCancelled:isCancelledBlock isRasterizing:rasterizing]; } - if (_didDisplayNodeContentWithRenderingContext) { + if (currentContext && _didDisplayNodeContentWithRenderingContext) { _didDisplayNodeContentWithRenderingContext(currentContext); } From 1ecfd81e3ec2497d318fbc37bdd59d3629ac3a33 Mon Sep 17 00:00:00 2001 From: Garrett Moon Date: Thu, 28 Jan 2016 16:05:18 -0800 Subject: [PATCH 5/5] Address comments --- AsyncDisplayKit/ASImageNode.mm | 33 +++++++++++-------- AsyncDisplayKit/ASTextNode.mm | 7 ++-- .../Private/ASDisplayNode+AsyncDisplay.mm | 2 ++ 3 files changed, 25 insertions(+), 17 deletions(-) diff --git a/AsyncDisplayKit/ASImageNode.mm b/AsyncDisplayKit/ASImageNode.mm index 3864ca5ad6..9f67f03d2e 100644 --- a/AsyncDisplayKit/ASImageNode.mm +++ b/AsyncDisplayKit/ASImageNode.mm @@ -154,20 +154,27 @@ - (UIImage *)displayWithParameters:(_ASImageNodeDrawParameters *)parameters isCancelled:(asdisplaynode_iscancelled_block_t)isCancelled { - ASDN::MutexLocker l(_imageLock); - UIImage *image = _image; - if (!image) { - return nil; + UIImage *image; + BOOL cropEnabled; + CGFloat contentsScale; + CGRect cropDisplayBounds; + CGRect cropRect; + asimagenode_modification_block_t imageModificationBlock; + + { + ASDN::MutexLocker l(_imageLock); + image = _image; + if (!image) { + return nil; + } + + cropEnabled = _cropEnabled; + contentsScale = _contentsScaleForDisplay; + cropDisplayBounds = _cropDisplayBounds; + cropRect = _cropRect; + imageModificationBlock = _imageModificationBlock; } - BOOL cropEnabled = _cropEnabled; - CGFloat contentsScale = _contentsScaleForDisplay; - CGRect cropDisplayBounds = _cropDisplayBounds; - CGRect cropRect = _cropRect; - asimagenode_modification_block_t imageModificationBlock = _imageModificationBlock; - - ASDN::MutexUnlocker u(_imageLock); - ASDisplayNodeContextModifier preContextBlock = self.willDisplayNodeContentWithRenderingContext; ASDisplayNodeContextModifier postContextBlock = self.didDisplayNodeContentWithRenderingContext; @@ -292,7 +299,6 @@ // If we've got a block to perform after displaying, do it. if (image && displayCompletionBlock) { - // FIXME: _displayCompletionBlock is not protected by lock displayCompletionBlock(NO); ASDN::MutexLocker l(_imageLock); @@ -311,7 +317,6 @@ } // Stash the block and call-site queue. We'll invoke it in -displayDidFinish. - // FIXME: _displayCompletionBlock not protected by lock ASDN::MutexLocker l(_imageLock); if (_displayCompletionBlock != displayCompletionBlock) { _displayCompletionBlock = [displayCompletionBlock copy]; diff --git a/AsyncDisplayKit/ASTextNode.mm b/AsyncDisplayKit/ASTextNode.mm index 2741dda576..7e37477e7c 100644 --- a/AsyncDisplayKit/ASTextNode.mm +++ b/AsyncDisplayKit/ASTextNode.mm @@ -217,7 +217,7 @@ static NSArray *DefaultLinkAttributeNames = @[ NSLinkAttributeName ]; #pragma mark - Renderer Management -//only safe to call on the main thread +//only safe to call on the main thread because self.bounds is only safe to call on the main thread one our node is loaded - (ASTextKitRenderer *)_renderer { return [self _rendererWithBounds:self.bounds]; @@ -415,7 +415,8 @@ static NSArray *DefaultLinkAttributeNames = @[ NSLinkAttributeName ]; ASTextKitRenderer *renderer = [self _rendererWithBounds:parameters.bounds]; UIEdgeInsets shadowPadding = [self shadowPaddingWithRenderer:renderer]; - CGPoint textOrigin = CGPointMake(parameters.bounds.origin.x - shadowPadding.left, parameters.bounds.origin.y - shadowPadding.top); + CGPoint boundsOrigin = parameters.bounds.origin; + CGPoint textOrigin = CGPointMake(boundsOrigin.x - shadowPadding.left, boundsOrigin.y - shadowPadding.top); // Fill background if (!isRasterizing) { @@ -998,7 +999,7 @@ static NSArray *DefaultLinkAttributeNames = @[ NSLinkAttributeName ]; } } -//only safe to call on main thread +//only safe to call on main thread, because [self _renderer] is only safe to call on the main thread - (UIEdgeInsets)shadowPadding { return [self shadowPaddingWithRenderer:[self _renderer]]; diff --git a/AsyncDisplayKit/Private/ASDisplayNode+AsyncDisplay.mm b/AsyncDisplayKit/Private/ASDisplayNode+AsyncDisplay.mm index 24100027a9..aa0982b77a 100644 --- a/AsyncDisplayKit/Private/ASDisplayNode+AsyncDisplay.mm +++ b/AsyncDisplayKit/Private/ASDisplayNode+AsyncDisplay.mm @@ -239,6 +239,8 @@ static void __ASDisplayLayerDecrementConcurrentDisplayCount(BOOL displayIsAsync, ASDN_DELAY_FOR_DISPLAY(); UIImage *result = nil; + //We can't call _willDisplayNodeContentWithRenderingContext or _didDisplayNodeContentWithRenderingContext because we don't + //have a context. We rely on implementors of displayWithParameters:isCancelled: to call if (_flags.implementsInstanceImageDisplay) { result = [self displayWithParameters:drawParameters isCancelled:isCancelledBlock]; } else {