From b91751a1143ad1dd49086d5104ec99feac89d898 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Thu, 6 Sep 2018 00:01:35 -0400 Subject: [PATCH] Respond to CR. --- .../src/Contacts/Threads/TSGroupThread.m | 2 +- .../Messages/Attachments/OWSMediaUtils.swift | 16 +------ .../Attachments/OWSThumbnailService.swift | 44 +++++++++---------- .../Messages/Attachments/TSAttachmentStream.h | 6 ++- .../Messages/Attachments/TSAttachmentStream.m | 13 ++++-- SignalServiceKit/src/Util/NSData+Image.m | 8 ++-- SignalServiceKit/src/Util/UIImage+OWS.m | 7 +++ 7 files changed, 50 insertions(+), 46 deletions(-) diff --git a/SignalServiceKit/src/Contacts/Threads/TSGroupThread.m b/SignalServiceKit/src/Contacts/Threads/TSGroupThread.m index c8bff39ad..a06bdb092 100644 --- a/SignalServiceKit/src/Contacts/Threads/TSGroupThread.m +++ b/SignalServiceKit/src/Contacts/Threads/TSGroupThread.m @@ -191,7 +191,7 @@ NSString *const TSGroupThread_NotificationKey_UniqueId = @"TSGroupThread_Notific OWSAssert(attachmentStream); OWSAssert(transaction); - self.groupModel.groupImage = [attachmentStream originalImage]; + self.groupModel.groupImage = [attachmentStream thumbnailImageSmallSync]; [self saveWithTransaction:transaction]; [transaction addCompletionQueue:nil diff --git a/SignalServiceKit/src/Messages/Attachments/OWSMediaUtils.swift b/SignalServiceKit/src/Messages/Attachments/OWSMediaUtils.swift index 64d59148a..4f78788c0 100644 --- a/SignalServiceKit/src/Messages/Attachments/OWSMediaUtils.swift +++ b/SignalServiceKit/src/Messages/Attachments/OWSMediaUtils.swift @@ -26,26 +26,14 @@ public enum OWSMediaError: Error { guard let originalImage = UIImage(contentsOfFile: path) else { throw OWSMediaError.failure(description: "Could not load original image.") } - // We use UIGraphicsBeginImageContextWithOptions() to scale. - // Core Image would provide better quality (e.g. Lanczos) but - // at a perf cost we don't want to pay. We could also use - // CoreGraphics directly, but I'm not sure there's any benefit. guard let thumbnailImage = originalImage.resized(withMaxDimensionPoints: maxDimension) else { throw OWSMediaError.failure(description: "Could not thumbnail image.") } return thumbnailImage } - private static let kMaxVideoStillSize: CGFloat = 1024 - - @objc public class func thumbnail(forVideoAtPath path: String) throws -> UIImage { - return try thumbnail(forVideoAtPath: path, maxSize: CGSize(width: kMaxVideoStillSize, height: kMaxVideoStillSize)) - } - - @objc public class func thumbnail(forVideoAtPath path: String, maxSize: CGSize) throws -> UIImage { - var maxSize = maxSize - maxSize.width = min(maxSize.width, kMaxVideoStillSize) - maxSize.height = min(maxSize.height, kMaxVideoStillSize) + @objc public class func thumbnail(forVideoAtPath path: String, maxDimension: CGFloat) throws -> UIImage { + let maxSize = CGSize(width: maxDimension, height: maxDimension) guard FileManager.default.fileExists(atPath: path) else { throw OWSMediaError.failure(description: "Media file missing.") diff --git a/SignalServiceKit/src/Messages/Attachments/OWSThumbnailService.swift b/SignalServiceKit/src/Messages/Attachments/OWSThumbnailService.swift index b67633025..9a2ad6910 100644 --- a/SignalServiceKit/src/Messages/Attachments/OWSThumbnailService.swift +++ b/SignalServiceKit/src/Messages/Attachments/OWSThumbnailService.swift @@ -7,36 +7,42 @@ import AVFoundation public enum OWSThumbnailError: Error { case failure(description: String) + case assertionFailure(description: String) + case externalError(description: String, underlyingError:Error) } @objc public class OWSLoadedThumbnail: NSObject { public typealias DataSourceBlock = () throws -> Data - @objc public let image: UIImage + @objc + public let image: UIImage let dataSourceBlock: DataSourceBlock - @objc public init(image: UIImage, filePath: String) { + @objc + public init(image: UIImage, filePath: String) { self.image = image self.dataSourceBlock = { return try Data(contentsOf: URL(fileURLWithPath: filePath)) } } - @objc public init(image: UIImage, data: Data) { + @objc + public init(image: UIImage, data: Data) { self.image = image self.dataSourceBlock = { return data } } - @objc public func data() throws -> Data { + @objc + public func data() throws -> Data { return try dataSourceBlock() } } private struct OWSThumbnailRequest { public typealias SuccessBlock = (OWSLoadedThumbnail) -> Void - public typealias FailureBlock = () -> Void + public typealias FailureBlock = (Error) -> Void let attachment: TSAttachmentStream let thumbnailDimensionPoints: UInt @@ -59,12 +65,10 @@ private struct OWSThumbnailRequest { public static let shared = OWSThumbnailService() public typealias SuccessBlock = (OWSLoadedThumbnail) -> Void - public typealias FailureBlock = () -> Void + public typealias FailureBlock = (Error) -> Void private let serialQueue = DispatchQueue(label: "OWSThumbnailService") - private let dbConnection: YapDatabaseConnection - // This property should only be accessed on the serialQueue. // // We want to process requests in _reverse_ order in which they @@ -72,9 +76,6 @@ private struct OWSThumbnailRequest { private var thumbnailRequestStack = [OWSThumbnailRequest]() private override init() { - - dbConnection = OWSPrimaryStorage.shared().newDatabaseConnection() - super.init() SwiftSingletons.register(self) @@ -86,7 +87,8 @@ private struct OWSThumbnailRequest { // completion will only be called on success. // completion will be called async on the main thread. - @objc public func ensureThumbnail(forAttachment attachment: TSAttachmentStream, + @objc + public func ensureThumbnail(forAttachment attachment: TSAttachmentStream, thumbnailDimensionPoints: UInt, success: @escaping SuccessBlock, failure: @escaping FailureBlock) { @@ -106,10 +108,9 @@ private struct OWSThumbnailRequest { // This should only be called on the serialQueue. private func processNextRequestSync() { - guard !thumbnailRequestStack.isEmpty else { + guard let thumbnailRequest = thumbnailRequestStack.popLast() else { return } - let thumbnailRequest = thumbnailRequestStack.removeLast() do { let loadedThumbnail = try process(thumbnailRequest: thumbnailRequest) @@ -120,7 +121,7 @@ private struct OWSThumbnailRequest { Logger.error("Could not create thumbnail: \(error)") DispatchQueue.main.async { - thumbnailRequest.failure() + thumbnailRequest.failure(error) } } } @@ -146,8 +147,8 @@ private struct OWSThumbnailRequest { return OWSLoadedThumbnail(image: image, filePath: thumbnailPath) } let thumbnailDirPath = (thumbnailPath as NSString).deletingLastPathComponent - if !FileManager.default.fileExists(atPath: thumbnailDirPath) { - try FileManager.default.createDirectory(atPath: thumbnailDirPath, withIntermediateDirectories: true, attributes: nil) + guard OWSFileSystem.ensureDirectoryExists(thumbnailDirPath) else { + throw OWSThumbnailError.failure(description: "Could not create attachment's thumbnail directory.") } guard let originalFilePath = attachment.originalFilePath else { throw OWSThumbnailError.failure(description: "Missing original file path.") @@ -157,18 +158,17 @@ private struct OWSThumbnailRequest { if attachment.isImage || attachment.isAnimated { thumbnailImage = try OWSMediaUtils.thumbnail(forImageAtPath: originalFilePath, maxDimension: maxDimension) } else if attachment.isVideo { - let maxSize = CGSize(width: maxDimension, height: maxDimension) - thumbnailImage = try OWSMediaUtils.thumbnail(forVideoAtPath: originalFilePath, maxSize: maxSize) + thumbnailImage = try OWSMediaUtils.thumbnail(forVideoAtPath: originalFilePath, maxDimension: maxDimension) } else { - throw OWSThumbnailError.failure(description: "Invalid attachment type.") + throw OWSThumbnailError.assertionFailure(description: "Invalid attachment type.") } guard let thumbnailData = UIImageJPEGRepresentation(thumbnailImage, 0.85) else { throw OWSThumbnailError.failure(description: "Could not convert thumbnail to JPEG.") } do { - try thumbnailData.write(to: NSURL.fileURL(withPath: thumbnailPath), options: .atomicWrite) + try thumbnailData.write(to: URL(fileURLWithPath: thumbnailPath), options: .atomicWrite) } catch let error as NSError { - throw OWSThumbnailError.failure(description: "File write failed: \(thumbnailPath), \(error)") + throw OWSThumbnailError.externalError(description: "File write failed: \(thumbnailPath), \(error)", underlyingError: error) } return OWSLoadedThumbnail(image: thumbnailImage, data: thumbnailData) } diff --git a/SignalServiceKit/src/Messages/Attachments/TSAttachmentStream.h b/SignalServiceKit/src/Messages/Attachments/TSAttachmentStream.h index 158c42e04..06ad4f80f 100644 --- a/SignalServiceKit/src/Messages/Attachments/TSAttachmentStream.h +++ b/SignalServiceKit/src/Messages/Attachments/TSAttachmentStream.h @@ -82,8 +82,10 @@ typedef void (^OWSThumbnailFailure)(void); #pragma mark - Thumbnails // On cache hit, the thumbnail will be returned synchronously and completion will never be invoked. -// On cache miss, nil will be returned and the completion will be invoked async on main if -// thumbnail can be generated. +// On cache miss, nil will be returned and success will be invoked if thumbnail can be generated; +// otherwise failure will be invoked. +// +// success and failure are invoked async on main. - (nullable UIImage *)thumbnailImageWithSizeHint:(CGSize)sizeHint success:(OWSThumbnailSuccess)success failure:(OWSThumbnailFailure)failure; diff --git a/SignalServiceKit/src/Messages/Attachments/TSAttachmentStream.m b/SignalServiceKit/src/Messages/Attachments/TSAttachmentStream.m index e9976ae85..a8b7193e5 100644 --- a/SignalServiceKit/src/Messages/Attachments/TSAttachmentStream.m +++ b/SignalServiceKit/src/Messages/Attachments/TSAttachmentStream.m @@ -265,8 +265,10 @@ typedef void (^OWSLoadedThumbnailSuccess)(OWSLoadedThumbnail *loadedThumbnail); return nil; } + // Thumbnails are written to the caches directory, so that iOS can + // remove them if necessary. NSString *dirName = [NSString stringWithFormat:@"%@-thumbnails", self.uniqueId]; - return [[[self class] attachmentsFolder] stringByAppendingPathComponent:dirName]; + return [OWSFileSystem.cachesDirectoryPath stringByAppendingPathComponent:dirName]; } - (NSString *)pathForThumbnailDimensionPoints:(NSUInteger)thumbnailDimensionPoints @@ -403,7 +405,9 @@ typedef void (^OWSLoadedThumbnailSuccess)(OWSLoadedThumbnail *loadedThumbnail); - (nullable UIImage *)videoStillImage { NSError *error; - UIImage *_Nullable image = [OWSMediaUtils thumbnailForVideoAtPath:self.originalFilePath error:&error]; + UIImage *_Nullable image = [OWSMediaUtils thumbnailForVideoAtPath:self.originalFilePath + maxDimension:ThumbnailDimensionPointsLarge() + error:&error]; if (error || !image) { DDLogError(@"Could not create video still: %@.", error); return nil; @@ -702,7 +706,10 @@ typedef void (^OWSLoadedThumbnailSuccess)(OWSLoadedThumbnail *loadedThumbnail); [OWSThumbnailService.shared ensureThumbnailForAttachment:self thumbnailDimensionPoints:thumbnailDimensionPoints success:success - failure:failure]; + failure:^(NSError *error) { + DDLogError(@"Failed to create thumbnail: %@", error); + failure(); + }]; return nil; } diff --git a/SignalServiceKit/src/Util/NSData+Image.m b/SignalServiceKit/src/Util/NSData+Image.m index 75c4b44b5..ddcfe3f58 100644 --- a/SignalServiceKit/src/Util/NSData+Image.m +++ b/SignalServiceKit/src/Util/NSData+Image.m @@ -132,12 +132,12 @@ typedef NS_ENUM(NSInteger, ImageFormat) { } // We only support (A)RGB and (A)Grayscale, so worst case is 4. - CGFloat kWorseCastComponentsPerPixel = 4; + const CGFloat kWorseCastComponentsPerPixel = 4; CGFloat bytesPerPixel = kWorseCastComponentsPerPixel * depthBytes; - CGFloat kMaxDimension = 4 * 1024; - CGFloat kExpectedBytePerPixel = 4; - CGFloat kMaxBytes = kMaxDimension * kMaxDimension * kExpectedBytePerPixel; + const CGFloat kExpectedBytePerPixel = 4; + const CGFloat kMaxValidImageDimension = 4 * 1024; + CGFloat kMaxBytes = kMaxValidImageDimension * kMaxValidImageDimension * kExpectedBytePerPixel; CGFloat actualBytes = width * height * bytesPerPixel; if (actualBytes > kMaxBytes) { DDLogWarn(@"invalid dimensions width: %f, height %f, bytesPerPixel: %f", width, height, bytesPerPixel); diff --git a/SignalServiceKit/src/Util/UIImage+OWS.m b/SignalServiceKit/src/Util/UIImage+OWS.m index c8282e46a..e0ff330ca 100644 --- a/SignalServiceKit/src/Util/UIImage+OWS.m +++ b/SignalServiceKit/src/Util/UIImage+OWS.m @@ -42,6 +42,13 @@ DDLogError(@"Invalid original size: %@", NSStringFromCGSize(originalSize)); return nil; } + + CGFloat maxOriginalDimensionPoints = MAX(originalSize.width, originalSize.height); + if (maxOriginalDimension < maxDimensionPoints) { + // Don't bother scaling an image that is already smaller than the max dimension. + return self; + } + CGSize thumbnailSize = CGSizeZero; if (originalSize.width > originalSize.height) { thumbnailSize.width = maxDimensionPoints;