From 552b982a33c087a83534bfc7d4e3829cd0d5af86 Mon Sep 17 00:00:00 2001 From: Chris Loer Date: Fri, 1 Jun 2018 15:12:52 -0700 Subject: [ios, macos] Fix MGLMapSnapshotter concurrency bugs (issue #11827). - Biggest change: when we apply the watermark on a background thread, don't capture self (turn most of the related instance methods into class methods) - Don't call mbglMapSnapshotter->snapshot from a user-provided queue, since it's an asynchronous call anyway and starting it on the user's queue requires capturing self. --- platform/darwin/src/MGLMapSnapshotter.mm | 315 ++++++++++++++++--------------- 1 file changed, 162 insertions(+), 153 deletions(-) diff --git a/platform/darwin/src/MGLMapSnapshotter.mm b/platform/darwin/src/MGLMapSnapshotter.mm index 76d99a0411..73db8ea151 100644 --- a/platform/darwin/src/MGLMapSnapshotter.mm +++ b/platform/darwin/src/MGLMapSnapshotter.mm @@ -85,12 +85,9 @@ const CGFloat MGLSnapshotterMinimumPixelSize = 64; @end @implementation MGLMapSnapshotter { - std::shared_ptr _mbglThreadPool; std::unique_ptr _mbglMapSnapshotter; std::unique_ptr> _snapshotCallback; - NSArray *_attributionInfo; - } - (instancetype)initWithOptions:(MGLMapSnapshotOptions *)options @@ -119,8 +116,13 @@ const CGFloat MGLSnapshotterMinimumPixelSize = 64; self.loading = true; __weak __typeof__(self) weakSelf = self; + // mbgl::Scheduler::GetCurrent() scheduler means "run callback on current (ie UI/main) thread" + // capture weakSelf to avoid retain cycle if callback is never called (ie snapshot cancelled) _snapshotCallback = std::make_unique>(*mbgl::Scheduler::GetCurrent(), [=](std::exception_ptr mbglError, mbgl::PremultipliedImage image, mbgl::MapSnapshotter::Attributions attributions, mbgl::MapSnapshotter::PointForFn pointForFn) { __typeof__(self) strongSelf = weakSelf; + // If self had died, _snapshotCallback would have been destroyed and this block would not be executed + assert(strongSelf); + strongSelf.loading = false; if (mbglError) { @@ -142,20 +144,159 @@ const CGFloat MGLSnapshotterMinimumPixelSize = 64; #endif [strongSelf drawAttributedSnapshot:attributions snapshotImage:mglImage pointForFn:pointForFn queue:queue completionHandler:completion]; } - _snapshotCallback = NULL; + strongSelf->_snapshotCallback = NULL; }); - dispatch_async(queue, ^{ - __typeof__(self) strongSelf = weakSelf; - if (!strongSelf) { - return; + // Launches snapshot on background Thread owned by mbglMapSnapshotter + // _snapshotCallback->self() is an ActorRef: if the callback is destroyed, further messages + // to the callback are just no-ops + _mbglMapSnapshotter->snapshot(_snapshotCallback->self()); +} + ++ (void)drawAttributedSnapshotWorker:(mbgl::MapSnapshotter::Attributions)attributions snapshotImage:(MGLImage *)mglImage pointForFn:(mbgl::MapSnapshotter::PointForFn)pointForFn queue:(dispatch_queue_t)queue scale:(CGFloat)scale size:(CGSize)size completionHandler:(MGLMapSnapshotCompletionHandler)completion { + + NSArray* attributionInfo = [MGLMapSnapshotter generateAttributionInfos:attributions]; + +#if TARGET_OS_IPHONE + MGLAttributionInfoStyle attributionInfoStyle = MGLAttributionInfoStyleLong; + for (NSUInteger styleValue = MGLAttributionInfoStyleLong; styleValue >= MGLAttributionInfoStyleShort; styleValue--) { + attributionInfoStyle = (MGLAttributionInfoStyle)styleValue; + CGSize attributionSize = [MGLMapSnapshotter attributionSizeWithLogoStyle:attributionInfoStyle sourceAttributionStyle:attributionInfoStyle attributionInfo:attributionInfo]; + if (attributionSize.width <= mglImage.size.width) { + break; + } + } + + UIImage *logoImage = [MGLMapSnapshotter logoImageWithStyle:attributionInfoStyle]; + CGSize attributionBackgroundSize = [MGLMapSnapshotter attributionTextSizeWithStyle:attributionInfoStyle attributionInfo:attributionInfo]; + + CGRect logoImageRect = CGRectMake(MGLLogoImagePosition.x, mglImage.size.height - (MGLLogoImagePosition.y + logoImage.size.height), logoImage.size.width, logoImage.size.height); + CGPoint attributionOrigin = CGPointMake(mglImage.size.width - 10 - attributionBackgroundSize.width, + logoImageRect.origin.y + (logoImageRect.size.height / 2) - (attributionBackgroundSize.height / 2) + 1); + if (!logoImage) { + CGSize defaultLogoSize = [MGLMapSnapshotter mapboxLongStyleLogo].size; + logoImageRect = CGRectMake(0, mglImage.size.height - (MGLLogoImagePosition.y + defaultLogoSize.height), 0, defaultLogoSize.height); + attributionOrigin = CGPointMake(10, logoImageRect.origin.y + (logoImageRect.size.height / 2) - (attributionBackgroundSize.height / 2) + 1); + } + + CGRect attributionBackgroundFrame = CGRectMake(attributionOrigin.x, + attributionOrigin.y, + attributionBackgroundSize.width, + attributionBackgroundSize.height); + CGPoint attributionTextPosition = CGPointMake(attributionBackgroundFrame.origin.x + 10, + attributionBackgroundFrame.origin.y - 1); + + CGRect cropRect = CGRectMake(attributionBackgroundFrame.origin.x * mglImage.scale, + attributionBackgroundFrame.origin.y * mglImage.scale, + attributionBackgroundSize.width * mglImage.scale, + attributionBackgroundSize.height * mglImage.scale); + + + UIGraphicsBeginImageContextWithOptions(mglImage.size, NO, scale); + + [mglImage drawInRect:CGRectMake(0, 0, mglImage.size.width, mglImage.size.height)]; + + [logoImage drawInRect:logoImageRect]; + + UIImage *currentImage = UIGraphicsGetImageFromCurrentImageContext(); + CGImageRef attributionImageRef = CGImageCreateWithImageInRect([currentImage CGImage], cropRect); + UIImage *attributionImage = [UIImage imageWithCGImage:attributionImageRef]; + CGImageRelease(attributionImageRef); + + CIImage *ciAttributionImage = [[CIImage alloc] initWithCGImage:attributionImage.CGImage]; + + UIImage *blurredAttributionBackground = [MGLMapSnapshotter blurredAttributionBackground:ciAttributionImage]; + + [blurredAttributionBackground drawInRect:attributionBackgroundFrame]; + + [MGLMapSnapshotter drawAttributionTextWithStyle:attributionInfoStyle origin:attributionTextPosition attributionInfo:attributionInfo]; + + UIImage *compositedImage = UIGraphicsGetImageFromCurrentImageContext(); + + UIGraphicsEndImageContext(); +#else + NSSize targetSize = NSMakeSize(size.width, size.height); + NSRect targetFrame = NSMakeRect(0, 0, targetSize.width, targetSize.height); + + MGLAttributionInfoStyle attributionInfoStyle = MGLAttributionInfoStyleLong; + for (NSUInteger styleValue = MGLAttributionInfoStyleLong; styleValue >= MGLAttributionInfoStyleShort; styleValue--) { + attributionInfoStyle = (MGLAttributionInfoStyle)styleValue; + CGSize attributionSize = [MGLMapSnapshotter attributionSizeWithLogoStyle:attributionInfoStyle sourceAttributionStyle:attributionInfoStyle attributionInfo:attributionInfo]; + if (attributionSize.width <= mglImage.size.width) { + break; } - strongSelf->_mbglMapSnapshotter->snapshot(strongSelf->_snapshotCallback->self()); + } + + NSImage *logoImage = [MGLMapSnapshotter logoImageWithStyle:attributionInfoStyle]; + CGSize attributionBackgroundSize = [MGLMapSnapshotter attributionTextSizeWithStyle:attributionInfoStyle attributionInfo:attributionInfo]; + NSImage *sourceImage = mglImage; + + CGRect logoImageRect = CGRectMake(MGLLogoImagePosition.x, MGLLogoImagePosition.y, logoImage.size.width, logoImage.size.height); + CGPoint attributionOrigin = CGPointMake(targetFrame.size.width - 10 - attributionBackgroundSize.width, + MGLLogoImagePosition.y + 1); + if (!logoImage) { + CGSize defaultLogoSize = [MGLMapSnapshotter mapboxLongStyleLogo].size; + logoImageRect = CGRectMake(0, MGLLogoImagePosition.y, 0, defaultLogoSize.height); + attributionOrigin = CGPointMake(10, attributionOrigin.y); + } + + CGRect attributionBackgroundFrame = CGRectMake(attributionOrigin.x, + attributionOrigin.y, + attributionBackgroundSize.width, + attributionBackgroundSize.height); + CGPoint attributionTextPosition = CGPointMake(attributionBackgroundFrame.origin.x + 10, + logoImageRect.origin.y + (logoImageRect.size.height / 2) - (attributionBackgroundSize.height / 2)); + + + NSImage *compositedImage = nil; + NSImageRep *sourceImageRep = [sourceImage bestRepresentationForRect:targetFrame + context:nil + hints:nil]; + compositedImage = [[NSImage alloc] initWithSize:targetSize]; + + [compositedImage lockFocus]; + + [sourceImageRep drawInRect: targetFrame]; + + if (logoImage) { + [logoImage drawInRect:logoImageRect]; + } + + NSBitmapImageRep *attributionBackground = [[NSBitmapImageRep alloc] initWithFocusedViewRect:attributionBackgroundFrame]; + + CIImage *attributionBackgroundImage = [[CIImage alloc] initWithCGImage:[attributionBackground CGImage]]; + + NSImage *blurredAttributionBackground = [MGLMapSnapshotter blurredAttributionBackground:attributionBackgroundImage]; + + [blurredAttributionBackground drawInRect:attributionBackgroundFrame]; + + [MGLMapSnapshotter drawAttributionTextWithStyle:attributionInfoStyle origin:attributionTextPosition attributionInfo:attributionInfo]; + + [compositedImage unlockFocus]; + +#endif + // Dispatch result to origin queue + dispatch_async(queue, ^{ + MGLMapSnapshot* snapshot = [[MGLMapSnapshot alloc] initWithImage:compositedImage scale:scale pointForFn:pointForFn]; + completion(snapshot, nil); }); } -- (MGLImage *)drawAttributedSnapshot:(mbgl::MapSnapshotter::Attributions)attributions snapshotImage:(MGLImage *)mglImage pointForFn:(mbgl::MapSnapshotter::PointForFn)pointForFn queue:(dispatch_queue_t)queue completionHandler:(MGLMapSnapshotCompletionHandler)completion { +- (void)drawAttributedSnapshot:(mbgl::MapSnapshotter::Attributions)attributions snapshotImage:(MGLImage *)mglImage pointForFn:(mbgl::MapSnapshotter::PointForFn)pointForFn queue:(dispatch_queue_t)queue completionHandler:(MGLMapSnapshotCompletionHandler)completion { + // Process image watermark in a work queue + dispatch_queue_t workQueue = dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0); + // Capture scale and size by value to avoid accessing self from another thread + CGFloat scale = self.options.scale; + CGSize size = self.options.size; + // pointForFn is a copyable std::function that captures state by value: see MapSnapshotter::Impl::snapshot + dispatch_async(workQueue, ^{ + // Call a class method to ensure we're not accidentally capturing self + [MGLMapSnapshotter drawAttributedSnapshotWorker:attributions snapshotImage:mglImage pointForFn:pointForFn queue:queue scale:scale size:size completionHandler:completion]; + }); +} + ++ (NSArray*) generateAttributionInfos:(mbgl::MapSnapshotter::Attributions)attributions { NSMutableArray *infos = [NSMutableArray array]; #if TARGET_OS_IPHONE @@ -172,144 +313,12 @@ const CGFloat MGLSnapshotterMinimumPixelSize = 64; linkColor:attributeFontColor]; [infos growArrayByAddingAttributionInfosFromArray:tileSetInfos]; } - - _attributionInfo = infos; - - // Process image watermark in a work queue - dispatch_queue_t workQueue = dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0); - dispatch_async(workQueue, ^{ -#if TARGET_OS_IPHONE - MGLAttributionInfoStyle attributionInfoStyle = MGLAttributionInfoStyleLong; - for (NSUInteger styleValue = MGLAttributionInfoStyleLong; styleValue >= MGLAttributionInfoStyleShort; styleValue--) { - attributionInfoStyle = (MGLAttributionInfoStyle)styleValue; - CGSize attributionSize = [self attributionSizeWithLogoStyle:attributionInfoStyle sourceAttributionStyle:attributionInfoStyle]; - if (attributionSize.width <= mglImage.size.width) { - break; - } - } - - UIImage *logoImage = [self logoImageWithStyle:attributionInfoStyle]; - CGSize attributionBackgroundSize = [self attributionTextSizeWithStyle:attributionInfoStyle]; - - CGRect logoImageRect = CGRectMake(MGLLogoImagePosition.x, mglImage.size.height - (MGLLogoImagePosition.y + logoImage.size.height), logoImage.size.width, logoImage.size.height); - CGPoint attributionOrigin = CGPointMake(mglImage.size.width - 10 - attributionBackgroundSize.width, - logoImageRect.origin.y + (logoImageRect.size.height / 2) - (attributionBackgroundSize.height / 2) + 1); - if (!logoImage) { - CGSize defaultLogoSize = [self mapboxLongStyleLogo].size; - logoImageRect = CGRectMake(0, mglImage.size.height - (MGLLogoImagePosition.y + defaultLogoSize.height), 0, defaultLogoSize.height); - attributionOrigin = CGPointMake(10, logoImageRect.origin.y + (logoImageRect.size.height / 2) - (attributionBackgroundSize.height / 2) + 1); - } - - CGRect attributionBackgroundFrame = CGRectMake(attributionOrigin.x, - attributionOrigin.y, - attributionBackgroundSize.width, - attributionBackgroundSize.height); - CGPoint attributionTextPosition = CGPointMake(attributionBackgroundFrame.origin.x + 10, - attributionBackgroundFrame.origin.y - 1); - - CGRect cropRect = CGRectMake(attributionBackgroundFrame.origin.x * mglImage.scale, - attributionBackgroundFrame.origin.y * mglImage.scale, - attributionBackgroundSize.width * mglImage.scale, - attributionBackgroundSize.height * mglImage.scale); - - - UIGraphicsBeginImageContextWithOptions(mglImage.size, NO, self.options.scale); - - [mglImage drawInRect:CGRectMake(0, 0, mglImage.size.width, mglImage.size.height)]; - - [logoImage drawInRect:logoImageRect]; - - UIImage *currentImage = UIGraphicsGetImageFromCurrentImageContext(); - CGImageRef attributionImageRef = CGImageCreateWithImageInRect([currentImage CGImage], cropRect); - UIImage *attributionImage = [UIImage imageWithCGImage:attributionImageRef]; - CGImageRelease(attributionImageRef); - - CIImage *ciAttributionImage = [[CIImage alloc] initWithCGImage:attributionImage.CGImage]; - - UIImage *blurredAttributionBackground = [self blurredAttributionBackground:ciAttributionImage]; - - [blurredAttributionBackground drawInRect:attributionBackgroundFrame]; - - [self drawAttributionTextWithStyle:attributionInfoStyle origin:attributionTextPosition]; - - UIImage *compositedImage = UIGraphicsGetImageFromCurrentImageContext(); - - UIGraphicsEndImageContext(); -#else - NSSize targetSize = NSMakeSize(self.options.size.width, self.options.size.height); - NSRect targetFrame = NSMakeRect(0, 0, targetSize.width, targetSize.height); - - MGLAttributionInfoStyle attributionInfoStyle = MGLAttributionInfoStyleLong; - for (NSUInteger styleValue = MGLAttributionInfoStyleLong; styleValue >= MGLAttributionInfoStyleShort; styleValue--) { - attributionInfoStyle = (MGLAttributionInfoStyle)styleValue; - CGSize attributionSize = [self attributionSizeWithLogoStyle:attributionInfoStyle sourceAttributionStyle:attributionInfoStyle]; - if (attributionSize.width <= mglImage.size.width) { - break; - } - } - - NSImage *logoImage = [self logoImageWithStyle:attributionInfoStyle]; - CGSize attributionBackgroundSize = [self attributionTextSizeWithStyle:attributionInfoStyle]; - NSImage *sourceImage = mglImage; - - CGRect logoImageRect = CGRectMake(MGLLogoImagePosition.x, MGLLogoImagePosition.y, logoImage.size.width, logoImage.size.height); - CGPoint attributionOrigin = CGPointMake(targetFrame.size.width - 10 - attributionBackgroundSize.width, - MGLLogoImagePosition.y + 1); - if (!logoImage) { - CGSize defaultLogoSize = [self mapboxLongStyleLogo].size; - logoImageRect = CGRectMake(0, MGLLogoImagePosition.y, 0, defaultLogoSize.height); - attributionOrigin = CGPointMake(10, attributionOrigin.y); - } - - CGRect attributionBackgroundFrame = CGRectMake(attributionOrigin.x, - attributionOrigin.y, - attributionBackgroundSize.width, - attributionBackgroundSize.height); - CGPoint attributionTextPosition = CGPointMake(attributionBackgroundFrame.origin.x + 10, - logoImageRect.origin.y + (logoImageRect.size.height / 2) - (attributionBackgroundSize.height / 2)); - - - NSImage *compositedImage = nil; - NSImageRep *sourceImageRep = [sourceImage bestRepresentationForRect:targetFrame - context:nil - hints:nil]; - compositedImage = [[NSImage alloc] initWithSize:targetSize]; - - [compositedImage lockFocus]; - - [sourceImageRep drawInRect: targetFrame]; - - if (logoImage) { - [logoImage drawInRect:logoImageRect]; - } - - NSBitmapImageRep *attributionBackground = [[NSBitmapImageRep alloc] initWithFocusedViewRect:attributionBackgroundFrame]; - - CIImage *attributionBackgroundImage = [[CIImage alloc] initWithCGImage:[attributionBackground CGImage]]; - - NSImage *blurredAttributionBackground = [self blurredAttributionBackground:attributionBackgroundImage]; - - [blurredAttributionBackground drawInRect:attributionBackgroundFrame]; - - [self drawAttributionTextWithStyle:attributionInfoStyle origin:attributionTextPosition]; - - [compositedImage unlockFocus]; - - -#endif - - // Dispatch result to origin queue - dispatch_async(queue, ^{ - MGLMapSnapshot* snapshot = [[MGLMapSnapshot alloc] initWithImage:compositedImage scale:self.options.scale pointForFn:pointForFn]; - completion(snapshot, nil); - }); - }); - return nil; + return infos; } -- (void)drawAttributionTextWithStyle:(MGLAttributionInfoStyle)attributionInfoStyle origin:(CGPoint)origin ++ (void)drawAttributionTextWithStyle:(MGLAttributionInfoStyle)attributionInfoStyle origin:(CGPoint)origin attributionInfo:(NSArray*)attributionInfo { - for (MGLAttributionInfo *info in _attributionInfo) { + for (MGLAttributionInfo *info in attributionInfo) { if (info.isFeedbackLink) { continue; } @@ -320,7 +329,7 @@ const CGFloat MGLSnapshotterMinimumPixelSize = 64; } } -- (MGLImage *)blurredAttributionBackground:(CIImage *)backgroundImage ++ (MGLImage *)blurredAttributionBackground:(CIImage *)backgroundImage { CGAffineTransform transform = CGAffineTransformIdentity; CIFilter *clamp = [CIFilter filterWithName:@"CIAffineClamp"]; @@ -351,12 +360,12 @@ const CGFloat MGLSnapshotterMinimumPixelSize = 64; return image; } -- (MGLImage *)logoImageWithStyle:(MGLAttributionInfoStyle)style ++ (MGLImage *)logoImageWithStyle:(MGLAttributionInfoStyle)style { MGLImage *logoImage; switch (style) { case MGLAttributionInfoStyleLong: - logoImage = [self mapboxLongStyleLogo]; + logoImage = [MGLMapSnapshotter mapboxLongStyleLogo]; break; case MGLAttributionInfoStyleMedium: #if TARGET_OS_IPHONE @@ -372,7 +381,7 @@ const CGFloat MGLSnapshotterMinimumPixelSize = 64; return logoImage; } -- (MGLImage *)mapboxLongStyleLogo ++ (MGLImage *)mapboxLongStyleLogo { MGLImage *logoImage; #if TARGET_OS_IPHONE @@ -383,11 +392,11 @@ const CGFloat MGLSnapshotterMinimumPixelSize = 64; return logoImage; } -- (CGSize)attributionSizeWithLogoStyle:(MGLAttributionInfoStyle)logoStyle sourceAttributionStyle:(MGLAttributionInfoStyle)attributionStyle ++ (CGSize)attributionSizeWithLogoStyle:(MGLAttributionInfoStyle)logoStyle sourceAttributionStyle:(MGLAttributionInfoStyle)attributionStyle attributionInfo:(NSArray*)attributionInfo { MGLImage *logoImage = [self logoImageWithStyle:logoStyle]; - CGSize attributionBackgroundSize = [self attributionTextSizeWithStyle:attributionStyle]; + CGSize attributionBackgroundSize = [MGLMapSnapshotter attributionTextSizeWithStyle:attributionStyle attributionInfo:attributionInfo]; CGSize attributionSize = CGSizeZero; @@ -400,10 +409,10 @@ const CGFloat MGLSnapshotterMinimumPixelSize = 64; return attributionSize; } -- (CGSize)attributionTextSizeWithStyle:(MGLAttributionInfoStyle)attributionStyle ++ (CGSize)attributionTextSizeWithStyle:(MGLAttributionInfoStyle)attributionStyle attributionInfo:(NSArray*)attributionInfo { CGSize attributionBackgroundSize = CGSizeMake(10, 0); - for (MGLAttributionInfo *info in _attributionInfo) { + for (MGLAttributionInfo *info in attributionInfo) { if (info.isFeedbackLink) { continue; } -- cgit v1.2.1