From 125aabb0a3298fbf3a9d26b3dda0b9aab90951ce Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Thu, 7 Dec 2017 11:49:31 -0500 Subject: [PATCH 1/8] Change image resizing/quality behavior, preferring smaller images in the common case. --- .../ConversationViewController.m | 22 ++++-- .../ViewControllers/DebugUI/DebugUIMessages.m | 13 ++- .../attachments/SignalAttachment.swift | 79 +++++++++++++------ .../ShareViewController.swift | 2 +- 4 files changed, 81 insertions(+), 35 deletions(-) diff --git a/Signal/src/ViewControllers/ConversationView/ConversationViewController.m b/Signal/src/ViewControllers/ConversationView/ConversationViewController.m index c2c98e70a..77c254892 100644 --- a/Signal/src/ViewControllers/ConversationView/ConversationViewController.m +++ b/Signal/src/ViewControllers/ConversationView/ConversationViewController.m @@ -2481,7 +2481,10 @@ typedef NS_ENUM(NSInteger, MessagesRangeSizeMode) { } [dataSource setSourceFilename:filename]; - SignalAttachment *attachment = [SignalAttachment attachmentWithDataSource:dataSource dataUTI:type]; + // "Document picker" attachments _SHOULD NOT_ be resized, if possible. + SignalAttachment *attachment = [SignalAttachment attachmentWithDataSource:dataSource + dataUTI:type + attachmentQuality:TSAttachmentQualityOriginal]; [self tryToSendAttachmentIfApproved:attachment]; } @@ -2605,10 +2608,12 @@ typedef NS_ENUM(NSInteger, MessagesRangeSizeMode) { OWSAssert([NSThread isMainThread]); if (imageFromCamera) { + // "Camera" attachments _SHOULD_ be resized, if possible. SignalAttachment *attachment = [SignalAttachment imageAttachmentWithImage:imageFromCamera dataUTI:(NSString *)kUTTypeJPEG - filename:filename]; + filename:filename + attachmentQuality:TSAttachmentQualityCompact]; if (!attachment || [attachment hasError]) { DDLogWarn(@"%@ %s Invalid attachment: %@.", self.logTag, @@ -2655,8 +2660,11 @@ typedef NS_ENUM(NSInteger, MessagesRangeSizeMode) { DataSource *_Nullable dataSource = [DataSourceValue dataSourceWithData:imageData utiType:dataUTI]; [dataSource setSourceFilename:filename]; + // "Camera Roll" attachments _SHOULD_ be resized, if possible. SignalAttachment *attachment = - [SignalAttachment attachmentWithDataSource:dataSource dataUTI:dataUTI]; + [SignalAttachment attachmentWithDataSource:dataSource + dataUTI:dataUTI + attachmentQuality:TSAttachmentQualityCompact]; [self dismissViewControllerAnimated:YES completion:^{ OWSAssert([NSThread isMainThread]); @@ -2745,7 +2753,8 @@ typedef NS_ENUM(NSInteger, MessagesRangeSizeMode) { [dataSource setShouldDeleteOnDeallocation]; SignalAttachment *attachment = [SignalAttachment attachmentWithDataSource:dataSource - dataUTI:(NSString *)kUTTypeMPEG4]; + dataUTI:(NSString *)kUTTypeMPEG4 + attachmentQuality:TSAttachmentQualityOriginal]; if (!attachment || [attachment hasError]) { DDLogError(@"%@ %s Invalid attachment: %@.", self.logTag, @@ -3847,8 +3856,9 @@ typedef NS_ENUM(NSInteger, MessagesRangeSizeMode) { TSOutgoingMessage *message; if ([text lengthOfBytesUsingEncoding:NSUTF8StringEncoding] >= kOversizeTextMessageSizeThreshold) { DataSource *_Nullable dataSource = [DataSourceValue dataSourceWithOversizeText:text]; - SignalAttachment *attachment = - [SignalAttachment attachmentWithDataSource:dataSource dataUTI:kOversizeTextAttachmentUTI]; + SignalAttachment *attachment = [SignalAttachment attachmentWithDataSource:dataSource + dataUTI:kOversizeTextAttachmentUTI + attachmentQuality:TSAttachmentQualityOriginal]; message = [ThreadUtil sendMessageWithAttachment:attachment inThread:self.thread messageSender:self.messageSender]; } else { diff --git a/Signal/src/ViewControllers/DebugUI/DebugUIMessages.m b/Signal/src/ViewControllers/DebugUI/DebugUIMessages.m index 633a8cceb..7d30101c6 100644 --- a/Signal/src/ViewControllers/DebugUI/DebugUIMessages.m +++ b/Signal/src/ViewControllers/DebugUI/DebugUIMessages.m @@ -378,7 +378,9 @@ NS_ASSUME_NONNULL_BEGIN NSString *utiType = [MIMETypeUtil utiTypeForFileExtension:filename.pathExtension]; DataSource *_Nullable dataSource = [DataSourcePath dataSourceWithFilePath:filePath]; [dataSource setSourceFilename:filename]; - SignalAttachment *attachment = [SignalAttachment attachmentWithDataSource:dataSource dataUTI:utiType]; + SignalAttachment *attachment = [SignalAttachment attachmentWithDataSource:dataSource + dataUTI:utiType + attachmentQuality:TSAttachmentQualityOriginal]; OWSAssert(attachment); if ([attachment hasError]) { DDLogError(@"attachment[%@]: %@", [attachment sourceFilename], [attachment errorName]); @@ -641,8 +643,9 @@ NS_ASSUME_NONNULL_BEGIN } DataSource *_Nullable dataSource = [DataSourceValue dataSourceWithOversizeText:message]; - SignalAttachment *attachment = - [SignalAttachment attachmentWithDataSource:dataSource dataUTI:kOversizeTextAttachmentUTI]; + SignalAttachment *attachment = [SignalAttachment attachmentWithDataSource:dataSource + dataUTI:kOversizeTextAttachmentUTI + attachmentQuality:TSAttachmentQualityOriginal]; [ThreadUtil sendMessageWithAttachment:attachment inThread:thread messageSender:messageSender]; } @@ -668,7 +671,9 @@ NS_ASSUME_NONNULL_BEGIN OWSMessageSender *messageSender = [Environment current].messageSender; DataSource *_Nullable dataSource = [DataSourceValue dataSourceWithData:[self createRandomNSDataOfSize:length] utiType:uti]; - SignalAttachment *attachment = [SignalAttachment attachmentWithDataSource:dataSource dataUTI:uti]; + SignalAttachment *attachment = [SignalAttachment attachmentWithDataSource:dataSource + dataUTI:uti + attachmentQuality:TSAttachmentQualityOriginal]; [ThreadUtil sendMessageWithAttachment:attachment inThread:thread messageSender:messageSender ignoreErrors:YES]; } + (OWSSignalServiceProtosEnvelope *)createEnvelopeForThread:(TSThread *)thread diff --git a/SignalMessaging/attachments/SignalAttachment.swift b/SignalMessaging/attachments/SignalAttachment.swift index 06deca330..82dd920b0 100644 --- a/SignalMessaging/attachments/SignalAttachment.swift +++ b/SignalMessaging/attachments/SignalAttachment.swift @@ -53,13 +53,31 @@ extension SignalAttachmentError: LocalizedError { } } -enum TSImageQuality { - case uncropped +@objc +public enum TSImageQuality: UInt { + case original case high + case mediumHigh case medium + case mediumLow case low } +@objc +public enum TSAttachmentQuality: UInt { + case original + case compact + + func imageQuality() -> TSImageQuality { + switch self { + case .original: + return .original + case .compact: + return .mediumHigh + } + } +} + // Represents a possible attachment to upload. // The attachment may be invalid. // @@ -448,7 +466,8 @@ public class SignalAttachment: NSObject { return nil } let dataSource = DataSourceValue.dataSource(with:data, utiType: dataUTI) - return imageAttachment(dataSource : dataSource, dataUTI : dataUTI) + // Pasted images should not be resized, if possible. + return imageAttachment(dataSource : dataSource, dataUTI : dataUTI, attachmentQuality:.original) } } for dataUTI in videoUTISet { @@ -507,7 +526,7 @@ public class SignalAttachment: NSObject { // NOTE: The attachment returned by this method may not be valid. // Check the attachment's error property. @objc - public class func imageAttachment(dataSource: DataSource?, dataUTI: String) -> SignalAttachment { + public class func imageAttachment(dataSource: DataSource?, dataUTI: String, attachmentQuality: TSAttachmentQuality) -> SignalAttachment { assert(dataUTI.count > 0) assert(dataSource != nil) @@ -546,7 +565,7 @@ public class SignalAttachment: NSObject { } attachment.cachedImage = image - if isInputImageValidOutputImage(image: image, dataSource: dataSource, dataUTI: dataUTI) { + if isInputImageValidOutputImage(image: image, dataSource: dataSource, dataUTI: dataUTI, attachmentQuality:attachmentQuality) { if let sourceFilename = dataSource.sourceFilename, let sourceFileExtension = sourceFilename.fileExtension, ["heic", "heif"].contains(sourceFileExtension.lowercased()) { @@ -570,19 +589,14 @@ public class SignalAttachment: NSObject { return attachment } - Logger.verbose("\(TAG) Compressing attachment as image/jpeg") - return compressImageAsJPEG(image : image, attachment : attachment, filename:dataSource.sourceFilename) + Logger.verbose("\(TAG) Compressing attachment as image/jpeg, \(dataSource.dataLength()) bytes") + return compressImageAsJPEG(image : image, attachment : attachment, filename:dataSource.sourceFilename, attachmentQuality:attachmentQuality) } } - private class func defaultImageUploadQuality() -> TSImageQuality { - // Currently default to a original image quality and size. - return .uncropped - } - // If the proposed attachment already conforms to the // file size and content size limits, don't recompress it. - private class func isInputImageValidOutputImage(image: UIImage?, dataSource: DataSource?, dataUTI: String) -> Bool { + private class func isInputImageValidOutputImage(image: UIImage?, dataSource: DataSource?, dataUTI: String, attachmentQuality: TSAttachmentQuality) -> Bool { guard let image = image else { return false } @@ -593,8 +607,9 @@ public class SignalAttachment: NSObject { return false } + let imageQuality = attachmentQuality.imageQuality() let maxSize = maxSizeForImage(image: image, - imageUploadQuality:defaultImageUploadQuality()) + imageUploadQuality:imageQuality) if image.size.width <= maxSize && image.size.height <= maxSize && dataSource.dataLength() <= kMaxFileSizeImage { @@ -608,7 +623,7 @@ public class SignalAttachment: NSObject { // NOTE: The attachment returned by this method may nil or not be valid. // Check the attachment's error property. @objc - public class func imageAttachment(image: UIImage?, dataUTI: String, filename: String?) -> SignalAttachment { + public class func imageAttachment(image: UIImage?, dataUTI: String, filename: String?, attachmentQuality: TSAttachmentQuality) -> SignalAttachment { assert(dataUTI.count > 0) guard let image = image else { @@ -626,13 +641,13 @@ public class SignalAttachment: NSObject { attachment.cachedImage = image Logger.verbose("\(TAG) Writing \(attachment.mimeType) as image/jpeg") - return compressImageAsJPEG(image : image, attachment : attachment, filename:filename) + return compressImageAsJPEG(image : image, attachment : attachment, filename:filename, attachmentQuality:attachmentQuality) } - private class func compressImageAsJPEG(image: UIImage, attachment: SignalAttachment, filename: String?) -> SignalAttachment { + private class func compressImageAsJPEG(image: UIImage, attachment: SignalAttachment, filename: String?, attachmentQuality: TSAttachmentQuality) -> SignalAttachment { assert(attachment.error == nil) - var imageUploadQuality = defaultImageUploadQuality() + var imageUploadQuality = attachmentQuality.imageQuality() while true { let maxSize = maxSizeForImage(image: image, imageUploadQuality:imageUploadQuality) @@ -649,6 +664,7 @@ public class SignalAttachment: NSObject { guard let dataSource = DataSourceValue.dataSource(with:jpgImageData, fileExtension:"jpg") else { attachment.error = .couldNotConvertToJpeg + Logger.verbose("\(TAG) Could not convert \(attachment.mimeType) to image/jpeg") return attachment } @@ -659,6 +675,7 @@ public class SignalAttachment: NSObject { if UInt(jpgImageData.count) <= kMaxFileSizeImage { let recompressedAttachment = SignalAttachment(dataSource : dataSource, dataUTI: kUTTypeJPEG as String) recompressedAttachment.cachedImage = dstImage + Logger.verbose("\(TAG) Converted \(attachment.mimeType) to image/jpeg, \(jpgImageData.count) bytes") return recompressedAttachment } @@ -666,14 +683,19 @@ public class SignalAttachment: NSObject { // continue to try again by progressively reducing the // image upload quality. switch imageUploadQuality { - case .uncropped: + case .original: imageUploadQuality = .high case .high: + imageUploadQuality = .mediumHigh + case .mediumHigh: imageUploadQuality = .medium case .medium: + imageUploadQuality = .mediumLow + case .mediumLow: imageUploadQuality = .low case .low: attachment.error = .fileSizeTooLarge + Logger.verbose("\(TAG) Image too large to convert \(attachment.mimeType) to image/jpeg") return attachment } } @@ -697,12 +719,16 @@ public class SignalAttachment: NSObject { private class func maxSizeForImage(image: UIImage, imageUploadQuality: TSImageQuality) -> CGFloat { switch imageUploadQuality { - case .uncropped: + case .original: return max(image.size.width, image.size.height) case .high: return 2048 + case .mediumHigh: + return 1536 case .medium: return 1024 + case .mediumLow: + return 768 case .low: return 512 } @@ -710,12 +736,16 @@ public class SignalAttachment: NSObject { private class func jpegCompressionQuality(imageUploadQuality: TSImageQuality) -> CGFloat { switch imageUploadQuality { - case .uncropped: + case .original: return 1 case .high: return 0.9 + case .mediumHigh: + return 0.7 case .medium: return 0.5 + case .mediumLow: + return 0.4 case .low: return 0.3 } @@ -790,9 +820,9 @@ public class SignalAttachment: NSObject { // NOTE: The attachment returned by this method may not be valid. // Check the attachment's error property. @objc - public class func attachment(dataSource: DataSource?, dataUTI: String) -> SignalAttachment { + public class func attachment(dataSource: DataSource?, dataUTI: String, attachmentQuality: TSAttachmentQuality) -> SignalAttachment { if inputImageUTISet.contains(dataUTI) { - return imageAttachment(dataSource : dataSource, dataUTI : dataUTI) + return imageAttachment(dataSource : dataSource, dataUTI : dataUTI, attachmentQuality:attachmentQuality) } else if videoUTISet.contains(dataUTI) { return videoAttachment(dataSource : dataSource, dataUTI : dataUTI) } else if audioUTISet.contains(dataUTI) { @@ -805,7 +835,8 @@ public class SignalAttachment: NSObject { @objc public class func empty() -> SignalAttachment { return SignalAttachment.attachment(dataSource : DataSourceValue.emptyDataSource(), - dataUTI: kUTTypeContent as String) + dataUTI: kUTTypeContent as String, + attachmentQuality:.original) } // MARK: Helper Methods diff --git a/SignalShareExtension/ShareViewController.swift b/SignalShareExtension/ShareViewController.swift index 10d2c0bbe..f25940109 100644 --- a/SignalShareExtension/ShareViewController.swift +++ b/SignalShareExtension/ShareViewController.swift @@ -435,7 +435,7 @@ public class ShareViewController: UINavigationController, ShareViewDelegate, SAE } } - let attachment = SignalAttachment.attachment(dataSource: dataSource, dataUTI: specificUTIType) + let attachment = SignalAttachment.attachment(dataSource: dataSource, dataUTI: specificUTIType, attachmentQuality:.compact) return attachment } From 55aa5eef6cb304408409b0ccf5920b33dda9e0ff Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Thu, 7 Dec 2017 11:54:16 -0500 Subject: [PATCH 2/8] Clean up ahead of PR. --- SignalMessaging/attachments/SignalAttachment.swift | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/SignalMessaging/attachments/SignalAttachment.swift b/SignalMessaging/attachments/SignalAttachment.swift index 82dd920b0..f57a5383c 100644 --- a/SignalMessaging/attachments/SignalAttachment.swift +++ b/SignalMessaging/attachments/SignalAttachment.swift @@ -466,7 +466,7 @@ public class SignalAttachment: NSObject { return nil } let dataSource = DataSourceValue.dataSource(with:data, utiType: dataUTI) - // Pasted images should not be resized, if possible. + // Pasted images _SHOULD _NOT_ be resized, if possible. return imageAttachment(dataSource : dataSource, dataUTI : dataUTI, attachmentQuality:.original) } } @@ -664,7 +664,6 @@ public class SignalAttachment: NSObject { guard let dataSource = DataSourceValue.dataSource(with:jpgImageData, fileExtension:"jpg") else { attachment.error = .couldNotConvertToJpeg - Logger.verbose("\(TAG) Could not convert \(attachment.mimeType) to image/jpeg") return attachment } @@ -695,7 +694,6 @@ public class SignalAttachment: NSObject { imageUploadQuality = .low case .low: attachment.error = .fileSizeTooLarge - Logger.verbose("\(TAG) Image too large to convert \(attachment.mimeType) to image/jpeg") return attachment } } From 84061cca9f34184bc73fc4ffcd06ccf1c43a915a Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Thu, 7 Dec 2017 12:29:29 -0500 Subject: [PATCH 3/8] Change image resizing/quality behavior, preferring smaller images in the common case. --- .../ConversationView/ConversationViewController.m | 2 +- SignalMessaging/attachments/SignalAttachment.swift | 5 ++++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/Signal/src/ViewControllers/ConversationView/ConversationViewController.m b/Signal/src/ViewControllers/ConversationView/ConversationViewController.m index 77c254892..21ea0bf47 100644 --- a/Signal/src/ViewControllers/ConversationView/ConversationViewController.m +++ b/Signal/src/ViewControllers/ConversationView/ConversationViewController.m @@ -2664,7 +2664,7 @@ typedef NS_ENUM(NSInteger, MessagesRangeSizeMode) { SignalAttachment *attachment = [SignalAttachment attachmentWithDataSource:dataSource dataUTI:dataUTI - attachmentQuality:TSAttachmentQualityCompact]; + attachmentQuality:TSAttachmentQualityMedium]; [self dismissViewControllerAnimated:YES completion:^{ OWSAssert([NSThread isMainThread]); diff --git a/SignalMessaging/attachments/SignalAttachment.swift b/SignalMessaging/attachments/SignalAttachment.swift index f57a5383c..979392024 100644 --- a/SignalMessaging/attachments/SignalAttachment.swift +++ b/SignalMessaging/attachments/SignalAttachment.swift @@ -66,14 +66,17 @@ public enum TSImageQuality: UInt { @objc public enum TSAttachmentQuality: UInt { case original + case medium case compact func imageQuality() -> TSImageQuality { switch self { case .original: return .original - case .compact: + case .medium: return .mediumHigh + case .compact: + return .medium } } } From 89db8b3a44c37de24ea32bfbcef39a725c0a7005 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Thu, 7 Dec 2017 14:50:30 -0500 Subject: [PATCH 4/8] Respond to CR. --- SignalShareExtension/ShareViewController.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/SignalShareExtension/ShareViewController.swift b/SignalShareExtension/ShareViewController.swift index f25940109..58b518880 100644 --- a/SignalShareExtension/ShareViewController.swift +++ b/SignalShareExtension/ShareViewController.swift @@ -435,7 +435,7 @@ public class ShareViewController: UINavigationController, ShareViewDelegate, SAE } } - let attachment = SignalAttachment.attachment(dataSource: dataSource, dataUTI: specificUTIType, attachmentQuality:.compact) + let attachment = SignalAttachment.attachment(dataSource: dataSource, dataUTI: specificUTIType, attachmentQuality:.medium) return attachment } From 11b4848530849960a500d57be2612feeecc03efe Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Thu, 7 Dec 2017 17:29:47 -0500 Subject: [PATCH 5/8] Respond to CR. --- .../ConversationViewController.m | 13 +++---- .../ViewControllers/DebugUI/DebugUIMessages.m | 12 +++--- .../GifPicker/GifPickerViewController.swift | 2 +- .../MessageDetailViewController.swift | 2 +- .../attachments/SignalAttachment.swift | 38 +++++++++---------- .../ShareViewController.swift | 2 +- 6 files changed, 33 insertions(+), 36 deletions(-) diff --git a/Signal/src/ViewControllers/ConversationView/ConversationViewController.m b/Signal/src/ViewControllers/ConversationView/ConversationViewController.m index 21ea0bf47..adf9f2e44 100644 --- a/Signal/src/ViewControllers/ConversationView/ConversationViewController.m +++ b/Signal/src/ViewControllers/ConversationView/ConversationViewController.m @@ -2482,9 +2482,8 @@ typedef NS_ENUM(NSInteger, MessagesRangeSizeMode) { [dataSource setSourceFilename:filename]; // "Document picker" attachments _SHOULD NOT_ be resized, if possible. - SignalAttachment *attachment = [SignalAttachment attachmentWithDataSource:dataSource - dataUTI:type - attachmentQuality:TSAttachmentQualityOriginal]; + SignalAttachment *attachment = + [SignalAttachment attachmentWithDataSource:dataSource dataUTI:type imageQuality:TSImageQualityOriginal]; [self tryToSendAttachmentIfApproved:attachment]; } @@ -2613,7 +2612,7 @@ typedef NS_ENUM(NSInteger, MessagesRangeSizeMode) { [SignalAttachment imageAttachmentWithImage:imageFromCamera dataUTI:(NSString *)kUTTypeJPEG filename:filename - attachmentQuality:TSAttachmentQualityCompact]; + imageQuality:TSImageQualityCompact]; if (!attachment || [attachment hasError]) { DDLogWarn(@"%@ %s Invalid attachment: %@.", self.logTag, @@ -2664,7 +2663,7 @@ typedef NS_ENUM(NSInteger, MessagesRangeSizeMode) { SignalAttachment *attachment = [SignalAttachment attachmentWithDataSource:dataSource dataUTI:dataUTI - attachmentQuality:TSAttachmentQualityMedium]; + imageQuality:TSImageQualityMedium]; [self dismissViewControllerAnimated:YES completion:^{ OWSAssert([NSThread isMainThread]); @@ -2754,7 +2753,7 @@ typedef NS_ENUM(NSInteger, MessagesRangeSizeMode) { SignalAttachment *attachment = [SignalAttachment attachmentWithDataSource:dataSource dataUTI:(NSString *)kUTTypeMPEG4 - attachmentQuality:TSAttachmentQualityOriginal]; + imageQuality:TSImageQualityOriginal]; if (!attachment || [attachment hasError]) { DDLogError(@"%@ %s Invalid attachment: %@.", self.logTag, @@ -3858,7 +3857,7 @@ typedef NS_ENUM(NSInteger, MessagesRangeSizeMode) { DataSource *_Nullable dataSource = [DataSourceValue dataSourceWithOversizeText:text]; SignalAttachment *attachment = [SignalAttachment attachmentWithDataSource:dataSource dataUTI:kOversizeTextAttachmentUTI - attachmentQuality:TSAttachmentQualityOriginal]; + imageQuality:TSImageQualityOriginal]; message = [ThreadUtil sendMessageWithAttachment:attachment inThread:self.thread messageSender:self.messageSender]; } else { diff --git a/Signal/src/ViewControllers/DebugUI/DebugUIMessages.m b/Signal/src/ViewControllers/DebugUI/DebugUIMessages.m index 7d30101c6..b50e96dfa 100644 --- a/Signal/src/ViewControllers/DebugUI/DebugUIMessages.m +++ b/Signal/src/ViewControllers/DebugUI/DebugUIMessages.m @@ -378,9 +378,8 @@ NS_ASSUME_NONNULL_BEGIN NSString *utiType = [MIMETypeUtil utiTypeForFileExtension:filename.pathExtension]; DataSource *_Nullable dataSource = [DataSourcePath dataSourceWithFilePath:filePath]; [dataSource setSourceFilename:filename]; - SignalAttachment *attachment = [SignalAttachment attachmentWithDataSource:dataSource - dataUTI:utiType - attachmentQuality:TSAttachmentQualityOriginal]; + SignalAttachment *attachment = + [SignalAttachment attachmentWithDataSource:dataSource dataUTI:utiType imageQuality:TSImageQualityOriginal]; OWSAssert(attachment); if ([attachment hasError]) { DDLogError(@"attachment[%@]: %@", [attachment sourceFilename], [attachment errorName]); @@ -645,7 +644,7 @@ NS_ASSUME_NONNULL_BEGIN DataSource *_Nullable dataSource = [DataSourceValue dataSourceWithOversizeText:message]; SignalAttachment *attachment = [SignalAttachment attachmentWithDataSource:dataSource dataUTI:kOversizeTextAttachmentUTI - attachmentQuality:TSAttachmentQualityOriginal]; + imageQuality:TSImageQualityOriginal]; [ThreadUtil sendMessageWithAttachment:attachment inThread:thread messageSender:messageSender]; } @@ -671,9 +670,8 @@ NS_ASSUME_NONNULL_BEGIN OWSMessageSender *messageSender = [Environment current].messageSender; DataSource *_Nullable dataSource = [DataSourceValue dataSourceWithData:[self createRandomNSDataOfSize:length] utiType:uti]; - SignalAttachment *attachment = [SignalAttachment attachmentWithDataSource:dataSource - dataUTI:uti - attachmentQuality:TSAttachmentQualityOriginal]; + SignalAttachment *attachment = + [SignalAttachment attachmentWithDataSource:dataSource dataUTI:uti imageQuality:TSImageQualityOriginal]; [ThreadUtil sendMessageWithAttachment:attachment inThread:thread messageSender:messageSender ignoreErrors:YES]; } + (OWSSignalServiceProtosEnvelope *)createEnvelopeForThread:(TSThread *)thread diff --git a/Signal/src/ViewControllers/GifPicker/GifPickerViewController.swift b/Signal/src/ViewControllers/GifPicker/GifPickerViewController.swift index 19d1f3221..8bcc4a206 100644 --- a/Signal/src/ViewControllers/GifPicker/GifPickerViewController.swift +++ b/Signal/src/ViewControllers/GifPicker/GifPickerViewController.swift @@ -372,7 +372,7 @@ class GifPickerViewController: OWSViewController, UISearchBarDelegate, UICollect owsFail("\(strongSelf.TAG) couldn't load asset.") return } - let attachment = SignalAttachment.imageAttachment(dataSource: dataSource, dataUTI: asset.rendition.utiType) + let attachment = SignalAttachment.attachment(dataSource: dataSource, dataUTI: asset.rendition.utiType, imageQuality: .original) strongSelf.delegate?.gifPickerDidSelect(attachment: attachment) diff --git a/Signal/src/ViewControllers/MessageDetailViewController.swift b/Signal/src/ViewControllers/MessageDetailViewController.swift index 71d65f07b..4f115fe1c 100644 --- a/Signal/src/ViewControllers/MessageDetailViewController.swift +++ b/Signal/src/ViewControllers/MessageDetailViewController.swift @@ -434,7 +434,7 @@ class MessageDetailViewController: OWSViewController, UIScrollViewDelegate { let contentType = attachment.contentType if let dataUTI = MIMETypeUtil.utiType(forMIMEType: contentType) { - let attachment = SignalAttachment.attachment(dataSource: dataSource, dataUTI: dataUTI) + let attachment = SignalAttachment.attachment(dataSource: dataSource, dataUTI: dataUTI, imageQuality: .original) let mediaMessageView = MediaMessageView(attachment: attachment, mode: .small) mediaMessageView.backgroundColor = UIColor.white self.mediaMessageView = mediaMessageView diff --git a/SignalMessaging/attachments/SignalAttachment.swift b/SignalMessaging/attachments/SignalAttachment.swift index 979392024..5a74fab9d 100644 --- a/SignalMessaging/attachments/SignalAttachment.swift +++ b/SignalMessaging/attachments/SignalAttachment.swift @@ -54,7 +54,7 @@ extension SignalAttachmentError: LocalizedError { } @objc -public enum TSImageQuality: UInt { +public enum TSImageQualityTier: UInt { case original case high case mediumHigh @@ -64,12 +64,12 @@ public enum TSImageQuality: UInt { } @objc -public enum TSAttachmentQuality: UInt { +public enum TSImageQuality: UInt { case original case medium case compact - func imageQuality() -> TSImageQuality { + func imageQualityTier() -> TSImageQualityTier { switch self { case .original: return .original @@ -470,7 +470,7 @@ public class SignalAttachment: NSObject { } let dataSource = DataSourceValue.dataSource(with:data, utiType: dataUTI) // Pasted images _SHOULD _NOT_ be resized, if possible. - return imageAttachment(dataSource : dataSource, dataUTI : dataUTI, attachmentQuality:.original) + return attachment(dataSource : dataSource, dataUTI : dataUTI, imageQuality:.original) } } for dataUTI in videoUTISet { @@ -529,7 +529,7 @@ public class SignalAttachment: NSObject { // NOTE: The attachment returned by this method may not be valid. // Check the attachment's error property. @objc - public class func imageAttachment(dataSource: DataSource?, dataUTI: String, attachmentQuality: TSAttachmentQuality) -> SignalAttachment { + private class func imageAttachment(dataSource: DataSource?, dataUTI: String, imageQuality: TSImageQuality) -> SignalAttachment { assert(dataUTI.count > 0) assert(dataSource != nil) @@ -568,7 +568,7 @@ public class SignalAttachment: NSObject { } attachment.cachedImage = image - if isInputImageValidOutputImage(image: image, dataSource: dataSource, dataUTI: dataUTI, attachmentQuality:attachmentQuality) { + if isInputImageValidOutputImage(image: image, dataSource: dataSource, dataUTI: dataUTI, imageQuality:imageQuality) { if let sourceFilename = dataSource.sourceFilename, let sourceFileExtension = sourceFilename.fileExtension, ["heic", "heif"].contains(sourceFileExtension.lowercased()) { @@ -593,13 +593,13 @@ public class SignalAttachment: NSObject { } Logger.verbose("\(TAG) Compressing attachment as image/jpeg, \(dataSource.dataLength()) bytes") - return compressImageAsJPEG(image : image, attachment : attachment, filename:dataSource.sourceFilename, attachmentQuality:attachmentQuality) + return compressImageAsJPEG(image : image, attachment : attachment, filename:dataSource.sourceFilename, imageQuality:imageQuality) } } // If the proposed attachment already conforms to the // file size and content size limits, don't recompress it. - private class func isInputImageValidOutputImage(image: UIImage?, dataSource: DataSource?, dataUTI: String, attachmentQuality: TSAttachmentQuality) -> Bool { + private class func isInputImageValidOutputImage(image: UIImage?, dataSource: DataSource?, dataUTI: String, imageQuality: TSImageQuality) -> Bool { guard let image = image else { return false } @@ -610,9 +610,9 @@ public class SignalAttachment: NSObject { return false } - let imageQuality = attachmentQuality.imageQuality() + let imageQualityTier = imageQuality.imageQualityTier() let maxSize = maxSizeForImage(image: image, - imageUploadQuality:imageQuality) + imageUploadQuality:imageQualityTier) if image.size.width <= maxSize && image.size.height <= maxSize && dataSource.dataLength() <= kMaxFileSizeImage { @@ -626,7 +626,7 @@ public class SignalAttachment: NSObject { // NOTE: The attachment returned by this method may nil or not be valid. // Check the attachment's error property. @objc - public class func imageAttachment(image: UIImage?, dataUTI: String, filename: String?, attachmentQuality: TSAttachmentQuality) -> SignalAttachment { + public class func imageAttachment(image: UIImage?, dataUTI: String, filename: String?, imageQuality: TSImageQuality) -> SignalAttachment { assert(dataUTI.count > 0) guard let image = image else { @@ -644,13 +644,13 @@ public class SignalAttachment: NSObject { attachment.cachedImage = image Logger.verbose("\(TAG) Writing \(attachment.mimeType) as image/jpeg") - return compressImageAsJPEG(image : image, attachment : attachment, filename:filename, attachmentQuality:attachmentQuality) + return compressImageAsJPEG(image : image, attachment : attachment, filename:filename, imageQuality:imageQuality) } - private class func compressImageAsJPEG(image: UIImage, attachment: SignalAttachment, filename: String?, attachmentQuality: TSAttachmentQuality) -> SignalAttachment { + private class func compressImageAsJPEG(image: UIImage, attachment: SignalAttachment, filename: String?, imageQuality: TSImageQuality) -> SignalAttachment { assert(attachment.error == nil) - var imageUploadQuality = attachmentQuality.imageQuality() + var imageUploadQuality = imageQuality.imageQualityTier() while true { let maxSize = maxSizeForImage(image: image, imageUploadQuality:imageUploadQuality) @@ -718,7 +718,7 @@ public class SignalAttachment: NSObject { return updatedImage! } - private class func maxSizeForImage(image: UIImage, imageUploadQuality: TSImageQuality) -> CGFloat { + private class func maxSizeForImage(image: UIImage, imageUploadQuality: TSImageQualityTier) -> CGFloat { switch imageUploadQuality { case .original: return max(image.size.width, image.size.height) @@ -735,7 +735,7 @@ public class SignalAttachment: NSObject { } } - private class func jpegCompressionQuality(imageUploadQuality: TSImageQuality) -> CGFloat { + private class func jpegCompressionQuality(imageUploadQuality: TSImageQualityTier) -> CGFloat { switch imageUploadQuality { case .original: return 1 @@ -821,9 +821,9 @@ public class SignalAttachment: NSObject { // NOTE: The attachment returned by this method may not be valid. // Check the attachment's error property. @objc - public class func attachment(dataSource: DataSource?, dataUTI: String, attachmentQuality: TSAttachmentQuality) -> SignalAttachment { + public class func attachment(dataSource: DataSource?, dataUTI: String, imageQuality: TSImageQuality = .original) -> SignalAttachment { if inputImageUTISet.contains(dataUTI) { - return imageAttachment(dataSource : dataSource, dataUTI : dataUTI, attachmentQuality:attachmentQuality) + return imageAttachment(dataSource : dataSource, dataUTI : dataUTI, imageQuality:imageQuality) } else if videoUTISet.contains(dataUTI) { return videoAttachment(dataSource : dataSource, dataUTI : dataUTI) } else if audioUTISet.contains(dataUTI) { @@ -837,7 +837,7 @@ public class SignalAttachment: NSObject { public class func empty() -> SignalAttachment { return SignalAttachment.attachment(dataSource : DataSourceValue.emptyDataSource(), dataUTI: kUTTypeContent as String, - attachmentQuality:.original) + imageQuality:.original) } // MARK: Helper Methods diff --git a/SignalShareExtension/ShareViewController.swift b/SignalShareExtension/ShareViewController.swift index 58b518880..020579fe4 100644 --- a/SignalShareExtension/ShareViewController.swift +++ b/SignalShareExtension/ShareViewController.swift @@ -435,7 +435,7 @@ public class ShareViewController: UINavigationController, ShareViewDelegate, SAE } } - let attachment = SignalAttachment.attachment(dataSource: dataSource, dataUTI: specificUTIType, attachmentQuality:.medium) + let attachment = SignalAttachment.attachment(dataSource: dataSource, dataUTI: specificUTIType, imageQuality:.medium) return attachment } From 80ae5e0fcf16ab123945175074227e80c1a246b1 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Thu, 7 Dec 2017 17:45:28 -0500 Subject: [PATCH 6/8] Respond to CR. --- .../ConversationView/ConversationViewController.m | 9 +++------ Signal/src/ViewControllers/DebugUI/DebugUIMessages.m | 11 ++++------- .../GifPicker/GifPickerViewController.swift | 2 +- .../ViewControllers/MessageDetailViewController.swift | 2 +- SignalMessaging/attachments/SignalAttachment.swift | 9 +++++++++ 5 files changed, 18 insertions(+), 15 deletions(-) diff --git a/Signal/src/ViewControllers/ConversationView/ConversationViewController.m b/Signal/src/ViewControllers/ConversationView/ConversationViewController.m index adf9f2e44..089addd83 100644 --- a/Signal/src/ViewControllers/ConversationView/ConversationViewController.m +++ b/Signal/src/ViewControllers/ConversationView/ConversationViewController.m @@ -2492,7 +2492,6 @@ typedef NS_ENUM(NSInteger, MessagesRangeSizeMode) { /* * Presenting UIImagePickerController */ - - (void)takePictureOrVideo { [self ows_askForCameraPermissions:^(BOOL granted) { @@ -2752,8 +2751,7 @@ typedef NS_ENUM(NSInteger, MessagesRangeSizeMode) { [dataSource setShouldDeleteOnDeallocation]; SignalAttachment *attachment = [SignalAttachment attachmentWithDataSource:dataSource - dataUTI:(NSString *)kUTTypeMPEG4 - imageQuality:TSImageQualityOriginal]; + dataUTI:(NSString *)kUTTypeMPEG4]; if (!attachment || [attachment hasError]) { DDLogError(@"%@ %s Invalid attachment: %@.", self.logTag, @@ -3855,9 +3853,8 @@ typedef NS_ENUM(NSInteger, MessagesRangeSizeMode) { TSOutgoingMessage *message; if ([text lengthOfBytesUsingEncoding:NSUTF8StringEncoding] >= kOversizeTextMessageSizeThreshold) { DataSource *_Nullable dataSource = [DataSourceValue dataSourceWithOversizeText:text]; - SignalAttachment *attachment = [SignalAttachment attachmentWithDataSource:dataSource - dataUTI:kOversizeTextAttachmentUTI - imageQuality:TSImageQualityOriginal]; + SignalAttachment *attachment = + [SignalAttachment attachmentWithDataSource:dataSource dataUTI:kOversizeTextAttachmentUTI]; message = [ThreadUtil sendMessageWithAttachment:attachment inThread:self.thread messageSender:self.messageSender]; } else { diff --git a/Signal/src/ViewControllers/DebugUI/DebugUIMessages.m b/Signal/src/ViewControllers/DebugUI/DebugUIMessages.m index b50e96dfa..633a8cceb 100644 --- a/Signal/src/ViewControllers/DebugUI/DebugUIMessages.m +++ b/Signal/src/ViewControllers/DebugUI/DebugUIMessages.m @@ -378,8 +378,7 @@ NS_ASSUME_NONNULL_BEGIN NSString *utiType = [MIMETypeUtil utiTypeForFileExtension:filename.pathExtension]; DataSource *_Nullable dataSource = [DataSourcePath dataSourceWithFilePath:filePath]; [dataSource setSourceFilename:filename]; - SignalAttachment *attachment = - [SignalAttachment attachmentWithDataSource:dataSource dataUTI:utiType imageQuality:TSImageQualityOriginal]; + SignalAttachment *attachment = [SignalAttachment attachmentWithDataSource:dataSource dataUTI:utiType]; OWSAssert(attachment); if ([attachment hasError]) { DDLogError(@"attachment[%@]: %@", [attachment sourceFilename], [attachment errorName]); @@ -642,9 +641,8 @@ NS_ASSUME_NONNULL_BEGIN } DataSource *_Nullable dataSource = [DataSourceValue dataSourceWithOversizeText:message]; - SignalAttachment *attachment = [SignalAttachment attachmentWithDataSource:dataSource - dataUTI:kOversizeTextAttachmentUTI - imageQuality:TSImageQualityOriginal]; + SignalAttachment *attachment = + [SignalAttachment attachmentWithDataSource:dataSource dataUTI:kOversizeTextAttachmentUTI]; [ThreadUtil sendMessageWithAttachment:attachment inThread:thread messageSender:messageSender]; } @@ -670,8 +668,7 @@ NS_ASSUME_NONNULL_BEGIN OWSMessageSender *messageSender = [Environment current].messageSender; DataSource *_Nullable dataSource = [DataSourceValue dataSourceWithData:[self createRandomNSDataOfSize:length] utiType:uti]; - SignalAttachment *attachment = - [SignalAttachment attachmentWithDataSource:dataSource dataUTI:uti imageQuality:TSImageQualityOriginal]; + SignalAttachment *attachment = [SignalAttachment attachmentWithDataSource:dataSource dataUTI:uti]; [ThreadUtil sendMessageWithAttachment:attachment inThread:thread messageSender:messageSender ignoreErrors:YES]; } + (OWSSignalServiceProtosEnvelope *)createEnvelopeForThread:(TSThread *)thread diff --git a/Signal/src/ViewControllers/GifPicker/GifPickerViewController.swift b/Signal/src/ViewControllers/GifPicker/GifPickerViewController.swift index 8bcc4a206..89d7f5036 100644 --- a/Signal/src/ViewControllers/GifPicker/GifPickerViewController.swift +++ b/Signal/src/ViewControllers/GifPicker/GifPickerViewController.swift @@ -372,7 +372,7 @@ class GifPickerViewController: OWSViewController, UISearchBarDelegate, UICollect owsFail("\(strongSelf.TAG) couldn't load asset.") return } - let attachment = SignalAttachment.attachment(dataSource: dataSource, dataUTI: asset.rendition.utiType, imageQuality: .original) + let attachment = SignalAttachment.attachment(dataSource: dataSource, dataUTI: asset.rendition.utiType) strongSelf.delegate?.gifPickerDidSelect(attachment: attachment) diff --git a/Signal/src/ViewControllers/MessageDetailViewController.swift b/Signal/src/ViewControllers/MessageDetailViewController.swift index 4f115fe1c..71d65f07b 100644 --- a/Signal/src/ViewControllers/MessageDetailViewController.swift +++ b/Signal/src/ViewControllers/MessageDetailViewController.swift @@ -434,7 +434,7 @@ class MessageDetailViewController: OWSViewController, UIScrollViewDelegate { let contentType = attachment.contentType if let dataUTI = MIMETypeUtil.utiType(forMIMEType: contentType) { - let attachment = SignalAttachment.attachment(dataSource: dataSource, dataUTI: dataUTI, imageQuality: .original) + let attachment = SignalAttachment.attachment(dataSource: dataSource, dataUTI: dataUTI) let mediaMessageView = MediaMessageView(attachment: attachment, mode: .small) mediaMessageView.backgroundColor = UIColor.white self.mediaMessageView = mediaMessageView diff --git a/SignalMessaging/attachments/SignalAttachment.swift b/SignalMessaging/attachments/SignalAttachment.swift index 5a74fab9d..3d5ccec86 100644 --- a/SignalMessaging/attachments/SignalAttachment.swift +++ b/SignalMessaging/attachments/SignalAttachment.swift @@ -816,6 +816,15 @@ public class SignalAttachment: NSObject { // MARK: Attachments + // Factory method for attachments of any kind. + // + // NOTE: The attachment returned by this method may not be valid. + // Check the attachment's error property. + @objc + public class func attachment(dataSource: DataSource?, dataUTI: String) -> SignalAttachment { + return attachment(dataSource: dataSource, dataUTI: dataUTI, imageQuality: .original) + } + // Factory method for attachments of any kind. // // NOTE: The attachment returned by this method may not be valid. From c9182795972073f773cece8e7106c60e960aa1c0 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Thu, 7 Dec 2017 18:05:30 -0500 Subject: [PATCH 7/8] Convert image attachment thresholds to be based on file size. --- .../attachments/SignalAttachment.swift | 28 ++++++++++++------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/SignalMessaging/attachments/SignalAttachment.swift b/SignalMessaging/attachments/SignalAttachment.swift index 3d5ccec86..a9ca0e667 100644 --- a/SignalMessaging/attachments/SignalAttachment.swift +++ b/SignalMessaging/attachments/SignalAttachment.swift @@ -610,11 +610,7 @@ public class SignalAttachment: NSObject { return false } - let imageQualityTier = imageQuality.imageQualityTier() - let maxSize = maxSizeForImage(image: image, - imageUploadQuality:imageQualityTier) - if image.size.width <= maxSize && - image.size.height <= maxSize && + if doesImageHaveAcceptableFileSize(dataSource: dataSource, imageQuality: imageQuality) && dataSource.dataLength() <= kMaxFileSizeImage { return true } @@ -674,7 +670,8 @@ public class SignalAttachment: NSObject { let jpgFilename = baseFilename?.appendingFileExtension("jpg") dataSource.sourceFilename = jpgFilename - if UInt(jpgImageData.count) <= kMaxFileSizeImage { + if doesImageHaveAcceptableFileSize(dataSource: dataSource, imageQuality: imageQuality) && + dataSource.dataLength() <= kMaxFileSizeImage { let recompressedAttachment = SignalAttachment(dataSource : dataSource, dataUTI: kUTTypeJPEG as String) recompressedAttachment.cachedImage = dstImage Logger.verbose("\(TAG) Converted \(attachment.mimeType) to image/jpeg, \(jpgImageData.count) bytes") @@ -718,6 +715,17 @@ public class SignalAttachment: NSObject { return updatedImage! } + private class func doesImageHaveAcceptableFileSize(dataSource: DataSource, imageQuality: TSImageQuality) -> Bool { + switch imageQuality { + case .original: + return true + case .medium: + return dataSource.dataLength() < UInt(1024 * 1024) + case .compact: + return dataSource.dataLength() < UInt(400 * 1024) + } + } + private class func maxSizeForImage(image: UIImage, imageUploadQuality: TSImageQualityTier) -> CGFloat { switch imageUploadQuality { case .original: @@ -742,13 +750,13 @@ public class SignalAttachment: NSObject { case .high: return 0.9 case .mediumHigh: - return 0.7 + return 0.8 case .medium: - return 0.5 + return 0.7 case .mediumLow: - return 0.4 + return 0.6 case .low: - return 0.3 + return 0.5 } } From bf09c805b99cdc71926f0a3323ecffadcecb2bdc Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Mon, 11 Dec 2017 11:27:53 -0500 Subject: [PATCH 8/8] Respond to CR. --- SignalMessaging/attachments/SignalAttachment.swift | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/SignalMessaging/attachments/SignalAttachment.swift b/SignalMessaging/attachments/SignalAttachment.swift index a9ca0e667..72b0434e5 100644 --- a/SignalMessaging/attachments/SignalAttachment.swift +++ b/SignalMessaging/attachments/SignalAttachment.swift @@ -190,7 +190,7 @@ public class SignalAttachment: NSObject { public var errorName: String? { guard let error = error else { // This method should only be called if there is an error. - owsFail("Missing error") + owsFail("\(TAG) Missing error") return nil } @@ -201,7 +201,7 @@ public class SignalAttachment: NSObject { public var localizedErrorDescription: String? { guard let error = self.error else { // This method should only be called if there is an error. - owsFail("Missing error") + owsFail("\(TAG) Missing error") return nil } @@ -470,7 +470,7 @@ public class SignalAttachment: NSObject { } let dataSource = DataSourceValue.dataSource(with:data, utiType: dataUTI) // Pasted images _SHOULD _NOT_ be resized, if possible. - return attachment(dataSource : dataSource, dataUTI : dataUTI, imageQuality:.original) + return attachment(dataSource : dataSource, dataUTI : dataUTI, imageQuality:.medium) } } for dataUTI in videoUTISet { @@ -830,6 +830,9 @@ public class SignalAttachment: NSObject { // Check the attachment's error property. @objc public class func attachment(dataSource: DataSource?, dataUTI: String) -> SignalAttachment { + if inputImageUTISet.contains(dataUTI) { + owsFail("\(TAG) must specify image quality type") + } return attachment(dataSource: dataSource, dataUTI: dataUTI, imageQuality: .original) } @@ -838,7 +841,7 @@ public class SignalAttachment: NSObject { // NOTE: The attachment returned by this method may not be valid. // Check the attachment's error property. @objc - public class func attachment(dataSource: DataSource?, dataUTI: String, imageQuality: TSImageQuality = .original) -> SignalAttachment { + public class func attachment(dataSource: DataSource?, dataUTI: String, imageQuality: TSImageQuality) -> SignalAttachment { if inputImageUTISet.contains(dataUTI) { return imageAttachment(dataSource : dataSource, dataUTI : dataUTI, imageQuality:imageQuality) } else if videoUTISet.contains(dataUTI) { @@ -882,7 +885,7 @@ public class SignalAttachment: NSObject { } guard dataSource.dataLength() > 0 else { - owsFail("Empty attachment") + owsFail("\(TAG) Empty attachment") assert(dataSource.dataLength() > 0) attachment.error = .invalidData return attachment