From 93b3a18c5c9a0967a7bfc75a54abbd04c890ffb8 Mon Sep 17 00:00:00 2001 From: kean Date: Wed, 20 Sep 2023 07:28:25 -0400 Subject: [PATCH 01/10] Reduce the large thumbnail size on 3x phones --- WordPress/Classes/Models/Media.h | 6 +++--- WordPress/Classes/Services/MediaImportService.swift | 3 +-- .../Classes/Utility/Media/MediaThumbnailExporter.swift | 2 +- 3 files changed, 5 insertions(+), 6 deletions(-) diff --git a/WordPress/Classes/Models/Media.h b/WordPress/Classes/Models/Media.h index 43dd0cd17d97..7719b8514560 100644 --- a/WordPress/Classes/Models/Media.h +++ b/WordPress/Classes/Models/Media.h @@ -68,10 +68,10 @@ typedef NS_ENUM(NSUInteger, MediaType) { @property (nonatomic, strong, nullable) NSURL *absoluteLocalURL; /** - Local file URL for a preprocessed thumbnail of the Media's asset. This may be nil if the - thumbnail has been deleted from the cache directory. + Local file URL for a preprocessed **large** thumbnail that can be used for + a full-screen presentation. - Note: it is recommended to instead use MediaService to generate thumbnails with a preferred size. + - warning: It's not a small thumbnail designed for collection views. */ @property (nonatomic, strong, nullable) NSURL *absoluteThumbnailLocalURL; diff --git a/WordPress/Classes/Services/MediaImportService.swift b/WordPress/Classes/Services/MediaImportService.swift index a414b60cb81d..c1d95fa76420 100644 --- a/WordPress/Classes/Services/MediaImportService.swift +++ b/WordPress/Classes/Services/MediaImportService.swift @@ -318,8 +318,7 @@ class MediaImportService: NSObject { /// using `absoluteThumbnailLocalURL`. func exportPlaceholderThumbnail(for media: Media, completion: ((URL?) -> Void)?) { let thumbnailService = MediaThumbnailService(coreDataStack: coreDataStack) - let targetSize: CGSize = FeatureFlag.mediaModernization.enabled ? MediaImageService.preferredThumbnailPointSize : .zero - thumbnailService.thumbnailURL(forMedia: media, preferredSize: targetSize) { url in + thumbnailService.thumbnailURL(forMedia: media, preferredSize: .zero) { url in self.coreDataStack.performAndSave({ context in let mediaInContext = try context.existingObject(with: media.objectID) as! Media // Set the absoluteThumbnailLocalURL with the generated thumbnail's URL. diff --git a/WordPress/Classes/Utility/Media/MediaThumbnailExporter.swift b/WordPress/Classes/Utility/Media/MediaThumbnailExporter.swift index 94b428c833ea..0791044d3745 100644 --- a/WordPress/Classes/Utility/Media/MediaThumbnailExporter.swift +++ b/WordPress/Classes/Utility/Media/MediaThumbnailExporter.swift @@ -59,7 +59,7 @@ class MediaThumbnailExporter: MediaExporter { guard let size = preferredSize else { return nil } - return max(size.width, size.height) * scale + return max(size.width, size.height) * min(2, scale) } lazy var identifier: String = { From 9543444c06e0883e3c1647159afbccdfcc35feb2 Mon Sep 17 00:00:00 2001 From: kean Date: Wed, 20 Sep 2023 09:47:10 -0400 Subject: [PATCH 02/10] Rework thumbnail management for Media --- WordPress/Classes/Models/Media.h | 2 - .../Classes/Services/MediaImageService.swift | 150 ++++++++++++------ .../Media/MediaThumbnailExporter.swift | 2 +- 3 files changed, 104 insertions(+), 50 deletions(-) diff --git a/WordPress/Classes/Models/Media.h b/WordPress/Classes/Models/Media.h index 7719b8514560..0f7703fea6c1 100644 --- a/WordPress/Classes/Models/Media.h +++ b/WordPress/Classes/Models/Media.h @@ -70,8 +70,6 @@ typedef NS_ENUM(NSUInteger, MediaType) { /** Local file URL for a preprocessed **large** thumbnail that can be used for a full-screen presentation. - - - warning: It's not a small thumbnail designed for collection views. */ @property (nonatomic, strong, nullable) NSURL *absoluteThumbnailLocalURL; diff --git a/WordPress/Classes/Services/MediaImageService.swift b/WordPress/Classes/Services/MediaImageService.swift index c0eb3f5d6859..d37f09a0d397 100644 --- a/WordPress/Classes/Services/MediaImageService.swift +++ b/WordPress/Classes/Services/MediaImageService.swift @@ -7,10 +7,12 @@ final class MediaImageService: NSObject { private let session: URLSession private let coreDataStack: CoreDataStackSwift + private let mediaFileManager: MediaFileManager private let ioQueue = DispatchQueue(label: "org.automattic.MediaImageService") init(coreDataStack: CoreDataStackSwift = ContextManager.shared) { self.coreDataStack = coreDataStack + self.mediaFileManager = MediaFileManager(directory: .cache) let configuration = URLSessionConfiguration.default // `MediaImageService` has its own disk cache, so it's important to @@ -40,10 +42,11 @@ final class MediaImageService: NSObject { return CGSize(width: targetSize, height: targetSize) }() - /// Returns a decompressed thumbnail optimized for the device. + /// Returns a small thumbnail for the given media asset. /// - /// For local images added to the library, it expects the `absoluteThumbnailLocalURL` - /// to be set by `MediaImportService`. + /// The thumbnail size is different on different devices, but it's suitable + /// for presentation in collection views. The returned images are decompressed + /// (bitmapped) and are ready to be displayed. @MainActor func thumbnail(for media: Media) async throws -> UIImage { guard media.remoteStatus != .stub else { @@ -61,14 +64,87 @@ final class MediaImageService: NSObject { @MainActor private func _thumbnail(for media: Media) async throws -> UIImage { - if let fileURL = media.absoluteThumbnailLocalURL, - let image = try? await decompressedImage(forFileURL: fileURL) { + if let image = await cachedThumbnail(for: media) { return image } let targetSize = MediaImageService.preferredThumbnailSize + if let image = await localThumbnail(for: media, targetSize: targetSize) { + return image + } return try await remoteThumbnail(for: media, targetSize: targetSize) } + // MARK: - Cached Thumbnail + + /// Returns a local thumbnail for the given media object (if available). + @MainActor + private func cachedThumbnail(for media: Media) async -> UIImage? { + let objectID = media.objectID + return try? await Task.detached(priority: .userInitiated) { + let imageURL = try self.getCachedThumbnailURL(for: objectID) + let data = try Data(contentsOf: imageURL) + return try decompressedImage(from: data) + }.value + } + + private func getCachedThumbnailURL(for objectID: NSManagedObjectID) throws -> URL { + let objectID = objectID.uriRepresentation().lastPathComponent + return try mediaFileManager.makeLocalMediaURL( + withFilename: "\(objectID)-small-thumbnail", + fileExtension: nil // It can be different between local and remove thumbnails + ) + } + + // The save is performed asynchronously to eliminate any delays. It's + // exceedingly unlikely it'll result in any duplicated work thanks to the + // memore caches. + @MainActor + private func saveThumbnail(for media: Media, _ closure: @escaping (URL) throws -> Void) { + let objectID = media.objectID + ioQueue.async { + guard let targetURL = try? self.getCachedThumbnailURL(for: objectID) else { return } + try? closure(targetURL) + } + } + + // MARK: - Local Thumbnail + + /// Generates a thumbnail from a local asset. + /// + /// - Parameters: + /// - media: The Media object. + /// - targetSize: An ideal size of the thumbnail in pixels. + @MainActor + private func localThumbnail(for media: Media, targetSize: CGSize) async -> UIImage? { + let exporter = MediaThumbnailExporter() + exporter.mediaDirectoryType = .cache + exporter.options.preferredSize = targetSize + exporter.options.scale = 1 + + guard let sourceURL = media.absoluteLocalURL, + exporter.supportsThumbnailExport(forFile: sourceURL) else { + return nil + } + + guard let (_, export) = try? await exporter.exportThumbnail(forFileURL: sourceURL) else { + return nil + } + + let image = try? await Task.detached(priority: .userInitiated) { + let data = try Data(contentsOf: export.url) + return try decompressedImage(from: data) + }.value + + // The order is important to ensure `export.url` still exists when creating an image + saveThumbnail(for: media) { targetURL in + try FileManager.default.moveItem(at: export.url, to: targetURL) + } + + return image + } + + // MARK: - Remote Thumbnail + /// Downloads thumbnail for the given media object and saves it locally. Returns /// a file URL for the downloaded thumbnail. /// @@ -89,42 +165,15 @@ final class MediaImageService: NSObject { } let (data, _) = try await session.data(for: request) - // Saves the thumbnail and records `absoluteThumbnailLocalURL` asynchronously. - // The service doesn't wait for the completion to eliminate any delays - // for image display. This includes writing data to disk, which is relatively - // fast, and updating `absoluteThumbnailLocalURL` on the media object. - // The latter can be slow because there is only one background context - // and it's often busy with long operations that could delay the image - // display by seconds. - let mediaID = TaggedManagedObjectID(media) - ioQueue.async { - if let fileURL = try? self.saveThumbnail(data, for: imageURL) { - self.setLocalThumbnailURL(fileURL, for: mediaID) - } + saveThumbnail(for: media) { targetURL in + try data.write(to: targetURL) } return try await Task.detached(priority: .userInitiated) { - try decompressedImage(from: data, fileExtension: imageURL.pathExtension) + try decompressedImage(from: data) }.value } - private func saveThumbnail(_ data: Data, for imageURL: URL) throws -> URL { - let fileURL = try MediaFileManager.cache.directoryURL() - .appendingPathComponent(UUID().uuidString, isDirectory: false) - .appendingPathExtension(imageURL.pathExtension) - try data.write(to: fileURL) - return fileURL - } - - private func setLocalThumbnailURL(_ fileURL: URL, for mediaID: TaggedManagedObjectID) { - coreDataStack.performAndSave({ context in - let media = try context.existingObject(with: mediaID) - if media.absoluteThumbnailLocalURL != fileURL { - media.absoluteThumbnailLocalURL = fileURL - } - }, completion: nil, on: .main) - } - @MainActor private func remoteThumbnailURL(for media: Media, targetSize: CGSize) -> URL? { switch media.mediaType { @@ -161,30 +210,37 @@ final class MediaImageService: NSObject { // Forces decompression (or bitmapping) to happen in the background. // It's very expensive for some image formats, such as JPEG. -private func decompressedImage(forFileURL fileURL: URL) async throws -> UIImage { - assert(fileURL.isFileURL, "Unsupported URL: \(fileURL)") - return try await Task.detached(priority: .userInitiated) { - let data = try Data(contentsOf: fileURL) - return try decompressedImage(from: data, fileExtension: fileURL.pathExtension) - }.value -} - -private func decompressedImage(from data: Data, fileExtension: String) throws -> UIImage { +private func decompressedImage(from data: Data) throws -> UIImage { guard let image = UIImage(data: data) else { throw URLError(.cannotDecodeContentData) } - guard isDecompressionNeeded(for: fileExtension) else { + guard isDecompressionNeeded(for: data) else { return image } return image.preparingForDisplay() ?? image } -private func isDecompressionNeeded(for fileExtension: String) -> Bool { +private func isDecompressionNeeded(for data: Data) -> Bool { // This check is required to avoid the following error messages when // using `preparingForDisplay`: // // [Decompressor] Error -17102 decompressing image -- possibly corrupt // // More info: https://github.com/SDWebImage/SDWebImage/issues/3365 - return fileExtension == "jpeg" || fileExtension == "jpg" + data.isMatchingMagicNumbers(Data.jpegMagicNumbers) +} + +private extension Data { + // JPEG magic numbers https://en.wikipedia.org/wiki/JPEG + static let jpegMagicNumbers: [UInt8] = [0xFF, 0xD8, 0xFF] + + func isMatchingMagicNumbers(_ numbers: [UInt8?]) -> Bool { + guard self.count >= numbers.count else { + return false + } + return zip(numbers.indices, numbers).allSatisfy { index, number in + guard let number = number else { return true } + return self[index] == number + } + } } diff --git a/WordPress/Classes/Utility/Media/MediaThumbnailExporter.swift b/WordPress/Classes/Utility/Media/MediaThumbnailExporter.swift index 0791044d3745..dc87bc3f75fd 100644 --- a/WordPress/Classes/Utility/Media/MediaThumbnailExporter.swift +++ b/WordPress/Classes/Utility/Media/MediaThumbnailExporter.swift @@ -22,7 +22,7 @@ class MediaThumbnailExporter: MediaExporter { /// of the image within a layout's dimensions. If nil, the image will not be resized. /// /// - Note: The final size may or may not match the preferred dimensions, depending - /// on the original image. + /// on the original image /// var preferredSize: CGSize? From 8cb96d3260a36c270e53c0d8689c17156ef3ff25 Mon Sep 17 00:00:00 2001 From: kean Date: Wed, 20 Sep 2023 10:17:35 -0400 Subject: [PATCH 03/10] Set JPEG compression quality to 80% --- WordPress/Classes/Utility/Media/MediaThumbnailExporter.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/WordPress/Classes/Utility/Media/MediaThumbnailExporter.swift b/WordPress/Classes/Utility/Media/MediaThumbnailExporter.swift index dc87bc3f75fd..54843b33fb96 100644 --- a/WordPress/Classes/Utility/Media/MediaThumbnailExporter.swift +++ b/WordPress/Classes/Utility/Media/MediaThumbnailExporter.swift @@ -36,7 +36,7 @@ class MediaThumbnailExporter: MediaExporter { /// The compression quality of the thumbnail, if the image type supports compression. /// - var compressionQuality = 0.90 + var compressionQuality = 0.8 /// The target image type of the exported thumbnail images. /// From 9d57153e8d651954385c8e84891c3c35c8ecd5d8 Mon Sep 17 00:00:00 2001 From: kean Date: Wed, 20 Sep 2023 15:28:27 -0400 Subject: [PATCH 04/10] Update resizing of local thumbnails to use aspect-fill mode --- WordPress/Classes/Extensions/Math.swift | 8 ++ .../Classes/Services/MediaImageService.swift | 73 +++++++++++++------ WordPress/WordPress.xcodeproj/project.pbxproj | 4 + .../MediaImageServiceTests.swift | 47 ++++++++++++ 4 files changed, 111 insertions(+), 21 deletions(-) create mode 100644 WordPress/WordPressTest/MediaImageServiceTests.swift diff --git a/WordPress/Classes/Extensions/Math.swift b/WordPress/Classes/Extensions/Math.swift index 59ba550f740c..c19f51797659 100644 --- a/WordPress/Classes/Extensions/Math.swift +++ b/WordPress/Classes/Extensions/Math.swift @@ -49,6 +49,14 @@ extension CGSize { func clamp(min minValue: Int, max maxValue: Int) -> CGSize { return clamp(min: CGFloat(minValue), max: CGFloat(maxValue)) } + + func scaled(by scale: CGFloat) -> CGSize { + CGSize(width: width * scale, height: height * scale) + } + + func rounded() -> CGSize { + CGSize(width: width.rounded(), height: height.rounded()) + } } extension CGFloat { diff --git a/WordPress/Classes/Services/MediaImageService.swift b/WordPress/Classes/Services/MediaImageService.swift index d37f09a0d397..38725cff0e85 100644 --- a/WordPress/Classes/Services/MediaImageService.swift +++ b/WordPress/Classes/Services/MediaImageService.swift @@ -23,25 +23,6 @@ final class MediaImageService: NSObject { // MARK: - Thumbnails - /// Returns a preferred thumbnail size (in pixels) optimized for the device. - /// - /// - important: It makes sure the app uses the same thumbnails across - /// different screens and presentation modes to avoid fetching and caching - /// more than one version of the same image. - static let preferredThumbnailSize: CGSize = { - let scale = UIScreen.main.scale - let targetSize = preferredThumbnailPointSize - return CGSize(width: targetSize.width * scale, height: targetSize.height * scale) - }() - - static let preferredThumbnailPointSize: CGSize = { - let screenSide = min(UIScreen.main.bounds.width, UIScreen.main.bounds.height) - let itemPerRow = UIDevice.current.userInterfaceIdiom == .pad ? 5 : 4 - let availableWidth = screenSide - MediaViewController.spacing * CGFloat(itemPerRow - 1) - let targetSize = (availableWidth / CGFloat(itemPerRow)).rounded(.down) - return CGSize(width: targetSize, height: targetSize) - }() - /// Returns a small thumbnail for the given media asset. /// /// The thumbnail size is different on different devices, but it's suitable @@ -118,8 +99,8 @@ final class MediaImageService: NSObject { private func localThumbnail(for media: Media, targetSize: CGSize) async -> UIImage? { let exporter = MediaThumbnailExporter() exporter.mediaDirectoryType = .cache - exporter.options.preferredSize = targetSize - exporter.options.scale = 1 + exporter.options.preferredSize = MediaImageService.targetSize(for: media, targetSize: targetSize) + exporter.options.scale = 1 // In pixels guard let sourceURL = media.absoluteLocalURL, exporter.supportsThumbnailExport(forFile: sourceURL) else { @@ -204,6 +185,56 @@ final class MediaImageService: NSObject { let objectID = try await mediaRepository.getMedia(withID: mediaID, in: .init(media.blog)) return try coreDataStack.mainContext.existingObject(with: objectID) } + + // MARK: - Target Size + + /// Returns a preferred thumbnail size (in pixels) optimized for the device. + /// + /// - important: It makes sure the app uses the same thumbnails across + /// different screens and presentation modes to avoid fetching and caching + /// more than one version of the same image. + private static let preferredThumbnailSize: CGSize = { + let screenSide = min(UIScreen.main.bounds.width, UIScreen.main.bounds.height) + let itemPerRow = UIDevice.current.userInterfaceIdiom == .pad ? 5 : 4 + let availableWidth = screenSide - MediaViewController.spacing * CGFloat(itemPerRow - 1) + let targetSide = (availableWidth / CGFloat(itemPerRow)).rounded(.down) + let targetSize = CGSize(width: targetSide, height: targetSide) + return targetSize.scaled(by: UIScreen.main.scale) + }() + + // Both Photon (Site Optimizer) and MediaThumbnailExporter doesn't support + // "aspect-fill" resizing mode, which is want we want for the thumbnails. + // + // This method converts the target size to support aspect fill mode. + // + // Example: if media size is 2000x3000 px and targetSize is 200x200 px, the + // returned value will be 200x300 px. + private static func targetSize(for media: Media, targetSize: CGSize) -> CGSize { + let mediaSize = CGSize( + width: CGFloat(media.width?.floatValue ?? 0), + height: CGFloat(media.height?.floatValue ?? 0) + ) + return MediaImageService.targetSize(forMediaSize: mediaSize, targetSize: targetSize) + } + + static func targetSize(forMediaSize mediaSize: CGSize, targetSize originalTargetSize: CGSize) -> CGSize { + guard mediaSize.width > 0 && mediaSize.height > 0 else { + return originalTargetSize + } + let scaleHorizontal = originalTargetSize.width / mediaSize.width + let scaleVertical = originalTargetSize.height / mediaSize.height + // Scale image to fill the target size but avoid upscaling. + let aspectFillScale = min(1, max(scaleHorizontal, scaleVertical)) + let targetSize = mediaSize.scaled(by: aspectFillScale).rounded() + // Sanitize the size to make sure ultra-wide panoramas are still resized + // to fit the target size, but increase it a bit for an acceptable size. + if targetSize.width > originalTargetSize.width * 4 || + targetSize.height > originalTargetSize.height * 4 { + let aspectFitScale = min(scaleHorizontal, scaleVertical) + return mediaSize.scaled(by: aspectFitScale * 4).rounded() + } + return targetSize + } } // MARK: - Decompression diff --git a/WordPress/WordPress.xcodeproj/project.pbxproj b/WordPress/WordPress.xcodeproj/project.pbxproj index c4eb4c531f84..6e1ea5e59428 100644 --- a/WordPress/WordPress.xcodeproj/project.pbxproj +++ b/WordPress/WordPress.xcodeproj/project.pbxproj @@ -458,6 +458,7 @@ 0CDEC40D2A2FAF0500BB3A91 /* DashboardBlazeCampaignsCardView.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0CDEC40B2A2FAF0500BB3A91 /* DashboardBlazeCampaignsCardView.swift */; }; 0CED95602A460F4B0020F420 /* DebugFeatureFlagsView.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0CED955F2A460F4B0020F420 /* DebugFeatureFlagsView.swift */; }; 0CED95612A460F4B0020F420 /* DebugFeatureFlagsView.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0CED955F2A460F4B0020F420 /* DebugFeatureFlagsView.swift */; }; + 0CF7D6C32ABB753A006D1E89 /* MediaImageServiceTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0CF7D6C22ABB753A006D1E89 /* MediaImageServiceTests.swift */; }; 1702BBDC1CEDEA6B00766A33 /* BadgeLabel.swift in Sources */ = {isa = PBXBuildFile; fileRef = 1702BBDB1CEDEA6B00766A33 /* BadgeLabel.swift */; }; 1702BBE01CF3034E00766A33 /* DomainsService.swift in Sources */ = {isa = PBXBuildFile; fileRef = 1702BBDF1CF3034E00766A33 /* DomainsService.swift */; }; 17039225282E6D2800F602E9 /* ViewsVisitorsLineChartCell.swift in Sources */ = {isa = PBXBuildFile; fileRef = DC772B0728201F5300664C02 /* ViewsVisitorsLineChartCell.swift */; }; @@ -6098,6 +6099,7 @@ 0CD382852A4B6FCE00612173 /* DashboardBlazeCardCellViewModelTest.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = DashboardBlazeCardCellViewModelTest.swift; sourceTree = ""; }; 0CDEC40B2A2FAF0500BB3A91 /* DashboardBlazeCampaignsCardView.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = DashboardBlazeCampaignsCardView.swift; sourceTree = ""; }; 0CED955F2A460F4B0020F420 /* DebugFeatureFlagsView.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = DebugFeatureFlagsView.swift; sourceTree = ""; }; + 0CF7D6C22ABB753A006D1E89 /* MediaImageServiceTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = MediaImageServiceTests.swift; sourceTree = ""; }; 0CFD6C792A73E703003DD0A0 /* WordPress 152.xcdatamodel */ = {isa = PBXFileReference; lastKnownFileType = wrapper.xcdatamodel; path = "WordPress 152.xcdatamodel"; sourceTree = ""; }; 131D0EE49695795ECEDAA446 /* Pods-WordPressTest.release-alpha.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-WordPressTest.release-alpha.xcconfig"; path = "../Pods/Target Support Files/Pods-WordPressTest/Pods-WordPressTest.release-alpha.xcconfig"; sourceTree = ""; }; 150B6590614A28DF9AD25491 /* Pods-Apps-Jetpack.release-alpha.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-Apps-Jetpack.release-alpha.xcconfig"; path = "../Pods/Target Support Files/Pods-Apps-Jetpack/Pods-Apps-Jetpack.release-alpha.xcconfig"; sourceTree = ""; }; @@ -12301,6 +12303,7 @@ 24A2948225D602710000A51E /* BlogTimeZoneTests.m */, D848CC1620FF38EA00A9038F /* FormattableCommentRangeTests.swift */, 0879FC151E9301DD00E1EFC8 /* MediaTests.swift */, + 0CF7D6C22ABB753A006D1E89 /* MediaImageServiceTests.swift */, C38C5D8027F61D2C002F517E /* MenuItemTests.swift */, D848CC1420FF33FC00A9038F /* NotificationContentRangeTests.swift */, D826D67E211D21C700A5D8FE /* NullMockUserDefaults.swift */, @@ -23371,6 +23374,7 @@ DC13DB7E293FD09F00E33561 /* StatsInsightsStoreTests.swift in Sources */, ACACE3AE28D729FA000992F9 /* NoResultsViewControllerTests.swift in Sources */, 4A2C73E42A943DEA00ACE79E /* TaggedManagedObjectIDTests.swift in Sources */, + 0CF7D6C32ABB753A006D1E89 /* MediaImageServiceTests.swift in Sources */, 8BFE36FF230F1C850061EBA8 /* AbstractPost+fixLocalMediaURLsTests.swift in Sources */, C995C22629D30AB000ACEF43 /* WidgetUrlSourceTests.swift in Sources */, 08A2AD791CCED2A800E84454 /* PostTagServiceTests.m in Sources */, diff --git a/WordPress/WordPressTest/MediaImageServiceTests.swift b/WordPress/WordPressTest/MediaImageServiceTests.swift new file mode 100644 index 000000000000..370f61b77be0 --- /dev/null +++ b/WordPress/WordPressTest/MediaImageServiceTests.swift @@ -0,0 +1,47 @@ +import XCTest +@testable import WordPress + +class MediaImageServiceTests: CoreDataTestCase { + + // MARK: - Target Size + + func testThatLandscapeImageIsResizedToFillTargetSize() { + XCTAssertEqual( + MediaImageService.targetSize( + forMediaSize: CGSize(width: 3000, height: 2000), + targetSize: CGSize(width: 200, height: 200) + ), + CGSize(width: 300, height: 200) + ) + } + + func testThatPortraitImageIsResizedToFillTargetSize() { + XCTAssertEqual( + MediaImageService.targetSize( + forMediaSize: CGSize(width: 2000, height: 3000), + targetSize: CGSize(width: 200, height: 200) + ), + CGSize(width: 200, height: 300) + ) + } + + func testThatPanoramaIsResizedToSaneSize() { + XCTAssertEqual( + MediaImageService.targetSize( + forMediaSize: CGSize(width: 4000, height: 400), + targetSize: CGSize(width: 200, height: 200) + ), + CGSize(width: 800, height: 80) + ) + } + + func testThatImagesAreNotUpscaled() { + XCTAssertEqual( + MediaImageService.targetSize( + forMediaSize: CGSize(width: 30, height: 20), + targetSize: CGSize(width: 200, height: 200) + ), + CGSize(width: 30, height: 20) + ) + } +} From 58ddb02243eda6ed3a54d30c0df1277d5d91acce Mon Sep 17 00:00:00 2001 From: kean Date: Wed, 20 Sep 2023 16:16:43 -0400 Subject: [PATCH 05/10] Add MediaImageService unit tests --- .../Classes/Services/MediaImageService.swift | 138 ++++++++++-------- .../MediaImageServiceTests.swift | 114 +++++++++++++++ 2 files changed, 189 insertions(+), 63 deletions(-) diff --git a/WordPress/Classes/Services/MediaImageService.swift b/WordPress/Classes/Services/MediaImageService.swift index 38725cff0e85..9383cf50deb4 100644 --- a/WordPress/Classes/Services/MediaImageService.swift +++ b/WordPress/Classes/Services/MediaImageService.swift @@ -10,9 +10,10 @@ final class MediaImageService: NSObject { private let mediaFileManager: MediaFileManager private let ioQueue = DispatchQueue(label: "org.automattic.MediaImageService") - init(coreDataStack: CoreDataStackSwift = ContextManager.shared) { + init(coreDataStack: CoreDataStackSwift = ContextManager.shared, + mediaFileManager: MediaFileManager = MediaFileManager(directory: .cache)) { self.coreDataStack = coreDataStack - self.mediaFileManager = MediaFileManager(directory: .cache) + self.mediaFileManager = mediaFileManager let configuration = URLSessionConfiguration.default // `MediaImageService` has its own disk cache, so it's important to @@ -30,76 +31,80 @@ final class MediaImageService: NSObject { /// (bitmapped) and are ready to be displayed. @MainActor func thumbnail(for media: Media) async throws -> UIImage { + let size = ThumbnailSize.small guard media.remoteStatus != .stub else { let media = try await fetchStubMedia(for: media) - // This should never happen, but adding it just in case to avoid recusion guard media.remoteStatus != .stub else { assertionFailure("The fetched media still has a .stub status") throw MediaThumbnailExporter.ThumbnailExportError.failedToGenerateThumbnailFileURL } - return try await _thumbnail(for: media) + return try await _thumbnail(for: media, size: size) } - return try await _thumbnail(for: media) + return try await _thumbnail(for: media, size: size) } @MainActor - private func _thumbnail(for media: Media) async throws -> UIImage { - if let image = await cachedThumbnail(for: media) { + private func _thumbnail(for media: Media, size: ThumbnailSize) async throws -> UIImage { + if let image = await cachedThumbnail(for: media, size: size) { return image } - let targetSize = MediaImageService.preferredThumbnailSize - if let image = await localThumbnail(for: media, targetSize: targetSize) { + if let image = await localThumbnail(for: media, size: size) { return image } - return try await remoteThumbnail(for: media, targetSize: targetSize) + return try await remoteThumbnail(for: media, size: size) } // MARK: - Cached Thumbnail /// Returns a local thumbnail for the given media object (if available). @MainActor - private func cachedThumbnail(for media: Media) async -> UIImage? { + private func cachedThumbnail(for media: Media, size: ThumbnailSize) async -> UIImage? { let objectID = media.objectID - return try? await Task.detached(priority: .userInitiated) { - let imageURL = try self.getCachedThumbnailURL(for: objectID) + return try? await Task.detached { + let imageURL = try self.getCachedThumbnailURL(for: objectID, size: size) let data = try Data(contentsOf: imageURL) return try decompressedImage(from: data) }.value } - private func getCachedThumbnailURL(for objectID: NSManagedObjectID) throws -> URL { - let objectID = objectID.uriRepresentation().lastPathComponent - return try mediaFileManager.makeLocalMediaURL( - withFilename: "\(objectID)-small-thumbnail", - fileExtension: nil // It can be different between local and remove thumbnails - ) - } - // The save is performed asynchronously to eliminate any delays. It's // exceedingly unlikely it'll result in any duplicated work thanks to the // memore caches. @MainActor - private func saveThumbnail(for media: Media, _ closure: @escaping (URL) throws -> Void) { + private func saveThumbnail(for media: Media, size: ThumbnailSize, _ closure: @escaping (URL) throws -> Void) { let objectID = media.objectID ioQueue.async { - guard let targetURL = try? self.getCachedThumbnailURL(for: objectID) else { return } - try? closure(targetURL) + if let targetURL = try? self.getCachedThumbnailURL(for: objectID, size: size) { + try? closure(targetURL) + } } } - // MARK: - Local Thumbnail + private func getCachedThumbnailURL(for objectID: NSManagedObjectID, size: ThumbnailSize) throws -> URL { + let objectID = objectID.uriRepresentation().lastPathComponent + return try mediaFileManager.makeLocalMediaURL( + withFilename: "\(objectID)-\(size.rawValue)-thumbnail", + fileExtension: nil, // We don't know ahead of time + incremented: false + ) + } - /// Generates a thumbnail from a local asset. + /// Flushes all pending I/O changes to disk. /// - /// - Parameters: - /// - media: The Media object. - /// - targetSize: An ideal size of the thumbnail in pixels. + /// - warning: For testing purposes only. + func flush() { + ioQueue.sync {} + } + + // MARK: - Local Thumbnail + + /// Generates a thumbnail from a local asset and saves it in cache. @MainActor - private func localThumbnail(for media: Media, targetSize: CGSize) async -> UIImage? { + private func localThumbnail(for media: Media, size: ThumbnailSize) async -> UIImage? { let exporter = MediaThumbnailExporter() exporter.mediaDirectoryType = .cache - exporter.options.preferredSize = MediaImageService.targetSize(for: media, targetSize: targetSize) + exporter.options.preferredSize = MediaImageService.getThumbnailSize(for: media, size: size) exporter.options.scale = 1 // In pixels guard let sourceURL = media.absoluteLocalURL, @@ -111,13 +116,13 @@ final class MediaImageService: NSObject { return nil } - let image = try? await Task.detached(priority: .userInitiated) { + let image = try? await Task.detached { let data = try Data(contentsOf: export.url) return try decompressedImage(from: data) }.value // The order is important to ensure `export.url` still exists when creating an image - saveThumbnail(for: media) { targetURL in + saveThumbnail(for: media, size: size) { targetURL in try FileManager.default.moveItem(at: export.url, to: targetURL) } @@ -126,14 +131,10 @@ final class MediaImageService: NSObject { // MARK: - Remote Thumbnail - /// Downloads thumbnail for the given media object and saves it locally. Returns - /// a file URL for the downloaded thumbnail. - /// - /// - Parameters: - /// - media: The Media object. - /// - targetSize: An ideal size of the thumbnail in pixels. + /// Downloads a remote thumbnail and saves it in cache. @MainActor - private func remoteThumbnail(for media: Media, targetSize: CGSize) async throws -> UIImage { + private func remoteThumbnail(for media: Media, size: ThumbnailSize) async throws -> UIImage { + let targetSize = MediaImageService.getThumbnailSize(for: media, size: size) guard let imageURL = remoteThumbnailURL(for: media, targetSize: targetSize) else { throw URLError(.badURL) } @@ -146,11 +147,11 @@ final class MediaImageService: NSObject { } let (data, _) = try await session.data(for: request) - saveThumbnail(for: media) { targetURL in + saveThumbnail(for: media, size: size) { targetURL in try data.write(to: targetURL) } - return try await Task.detached(priority: .userInitiated) { + return try await Task.detached { try decompressedImage(from: data) }.value } @@ -188,35 +189,46 @@ final class MediaImageService: NSObject { // MARK: - Target Size - /// Returns a preferred thumbnail size (in pixels) optimized for the device. + enum ThumbnailSize: String { + case small + } + + /// Returns an optiomal target size in pixels for a thumbnail of the given + /// size for the given media asset. /// - /// - important: It makes sure the app uses the same thumbnails across - /// different screens and presentation modes to avoid fetching and caching - /// more than one version of the same image. - private static let preferredThumbnailSize: CGSize = { - let screenSide = min(UIScreen.main.bounds.width, UIScreen.main.bounds.height) - let itemPerRow = UIDevice.current.userInterfaceIdiom == .pad ? 5 : 4 - let availableWidth = screenSide - MediaViewController.spacing * CGFloat(itemPerRow - 1) - let targetSide = (availableWidth / CGFloat(itemPerRow)).rounded(.down) - let targetSize = CGSize(width: targetSide, height: targetSide) - return targetSize.scaled(by: UIScreen.main.scale) - }() - - // Both Photon (Site Optimizer) and MediaThumbnailExporter doesn't support - // "aspect-fill" resizing mode, which is want we want for the thumbnails. - // - // This method converts the target size to support aspect fill mode. - // - // Example: if media size is 2000x3000 px and targetSize is 200x200 px, the - // returned value will be 200x300 px. - private static func targetSize(for media: Media, targetSize: CGSize) -> CGSize { + /// The size is calculated to fill a collection view cell, assuming the app + /// displays a few cells in a row. The cell size can vary depending on whether the + /// device is in landscape or portrait mode, but the thumbnail size is + /// guaranteed to always be the same across app launches. + /// + /// Example: if media size is 2000x3000 px and targetSize is 200x200 px, the + /// returned value will be 200x300 px. + static func getThumbnailSize(for media: Media, size: ThumbnailSize) -> CGSize { let mediaSize = CGSize( width: CGFloat(media.width?.floatValue ?? 0), height: CGFloat(media.height?.floatValue ?? 0) ) + let targetSize = MediaImageService.getPreferredThumbnailSize(for: size) return MediaImageService.targetSize(forMediaSize: mediaSize, targetSize: targetSize) } + /// Returns a preferred thumbnail size (in pixels) optimized for the device. + /// + /// - important: It makes sure the app uses the same thumbnails across + /// different screens and presentation modes to avoid fetching and caching + /// more than one version of the same image. + private static func getPreferredThumbnailSize(for thumbnail: ThumbnailSize) -> CGSize { + switch thumbnail { + case .small: + let screenSide = min(UIScreen.main.bounds.width, UIScreen.main.bounds.height) + let itemPerRow = UIDevice.current.userInterfaceIdiom == .pad ? 5 : 4 + let availableWidth = screenSide - MediaViewController.spacing * CGFloat(itemPerRow - 1) + let targetSide = (availableWidth / CGFloat(itemPerRow)).rounded(.down) + let targetSize = CGSize(width: targetSide, height: targetSide) + return targetSize.scaled(by: UIScreen.main.scale) + } + } + static func targetSize(forMediaSize mediaSize: CGSize, targetSize originalTargetSize: CGSize) -> CGSize { guard mediaSize.width > 0 && mediaSize.height > 0 else { return originalTargetSize diff --git a/WordPress/WordPressTest/MediaImageServiceTests.swift b/WordPress/WordPressTest/MediaImageServiceTests.swift index 370f61b77be0..d4e5686ae0ff 100644 --- a/WordPress/WordPressTest/MediaImageServiceTests.swift +++ b/WordPress/WordPressTest/MediaImageServiceTests.swift @@ -1,7 +1,90 @@ import XCTest +import OHHTTPStubs @testable import WordPress class MediaImageServiceTests: CoreDataTestCase { + var mediaFileManager: MediaFileManager! + var sut: MediaImageService! + + override func setUp() { + super.setUp() + + mediaFileManager = MediaFileManager(directory: .temporary(id: UUID())) + sut = MediaImageService( + coreDataStack: contextManager, + mediaFileManager: mediaFileManager + ) + } + + override func tearDown() { + super.tearDown() + + HTTPStubs.removeAllStubs() + + if let directoryURL = try? mediaFileManager.directoryURL() { + try? FileManager.default.removeItem(at: directoryURL) + } + } + + // MARK: - Local Resources + + func testSmallThumbnailForLocalImage() async throws { + // GIVEN + let media = Media(context: mainContext) + media.mediaType = .image + media.width = 1024 + media.height = 680 + let localURL = try makeLocalURL(forResource: "test-image", fileExtension: "jpg") + media.absoluteLocalURL = localURL + try mainContext.obtainPermanentIDs(for: [media]) + + // WHEN + let thumbnail = try await sut.thumbnail(for: media) + + // THEN a small thumbnail is created + XCTAssertEqual(thumbnail.size, MediaImageService.getThumbnailSize(for: media, size: .small)) + + // GIVEN local asset is deleted + try FileManager.default.removeItem(at: localURL) + sut.flush() + + // WHEN + let cachedThumbnail = try await sut.thumbnail(for: media) + + // THEN cached thumbnail is still available + XCTAssertEqual(cachedThumbnail.size, MediaImageService.getThumbnailSize(for: media, size: .small)) + } + + // MARK: - Remote Resources + + func testSmallThumbnailForRemoteImage() async throws { + // GIVEN + let media = Media(context: mainContext) + media.mediaType = .image + media.width = 1024 + media.height = 680 + let remoteURL = try XCTUnwrap(URL(string: "https://example.files.wordpress.com/2023/09/image.jpg")) + media.remoteURL = remoteURL.absoluteString + try mainContext.obtainPermanentIDs(for: [media]) + + // GIVEN remote image is mocked and is resized based on the parameters + try mockRemoteImage(withResource: "test-image", fileExtension: "jpg") + + // WHEN + let thumbnail = try await sut.thumbnail(for: media) + + // THEN a small thumbnail is created + XCTAssertEqual(thumbnail.size, MediaImageService.getThumbnailSize(for: media, size: .small)) + + // GIVEN local asset is deleted + sut.flush() + + // WHEN + let cachedThumbnail = try await sut.thumbnail(for: media) + + // THEN cached thumbnail is still available + XCTAssertEqual(cachedThumbnail.size, MediaImageService.getThumbnailSize(for: media, size: .small)) + } // MARK: - Target Size @@ -44,4 +127,35 @@ class MediaImageServiceTests: CoreDataTestCase { CGSize(width: 30, height: 20) ) } + + // MARK: - Helpers + + /// `Media` is hardcoded to work with a specific direcoty URL managed by `MediaFileManager` + func makeLocalURL(forResource name: String, fileExtension: String) throws -> URL { + let sourceURL = try XCTUnwrap(Bundle.test.url(forResource: name, withExtension: fileExtension)) + let mediaURL = try MediaFileManager.default.makeLocalMediaURL(withFilename: name, fileExtension: fileExtension) + try FileManager.default.copyItem(at: sourceURL, to: mediaURL) + return mediaURL + } + + func mockRemoteImage(withResource name: String, fileExtension: String) throws { + let sourceURL = try XCTUnwrap(Bundle.test.url(forResource: name, withExtension: fileExtension)) + let image = try XCTUnwrap(UIImage(data: try Data(contentsOf: sourceURL))) + + stub(condition: { _ in + return true + }, response: { request in + guard let url = request.url, + let components = URLComponents(url: url, resolvingAgainstBaseURL: false), + let queryItems = components.queryItems, + let resize = queryItems.first(where: { $0.name == "resize" }), + let values = resize.value?.components(separatedBy: ","), values.count == 2, + let width = Int(values[0]), let height = Int(values[1]) else { + return HTTPStubsResponse(error: URLError(.unknown)) + } + let resizedImage = image.resizedImage(CGSize(width: width, height: height), interpolationQuality: .default) + let responseData = resizedImage?.jpegData(compressionQuality: 0.8) ?? Data() + return HTTPStubsResponse(data: responseData, statusCode: 200, headers: nil) + }) + } } From 702b0a0a62a4b82138e8083b5b126f666809b301 Mon Sep 17 00:00:00 2001 From: kean Date: Thu, 21 Sep 2023 10:38:18 -0400 Subject: [PATCH 06/10] Add MediaThumbnailID and cleanup --- .../Classes/Services/MediaImageService.swift | 126 +++++++++--------- 1 file changed, 65 insertions(+), 61 deletions(-) diff --git a/WordPress/Classes/Services/MediaImageService.swift b/WordPress/Classes/Services/MediaImageService.swift index 9383cf50deb4..56f4fb5ef21d 100644 --- a/WordPress/Classes/Services/MediaImageService.swift +++ b/WordPress/Classes/Services/MediaImageService.swift @@ -1,7 +1,7 @@ -import Foundation +import UIKit +import CoreData -/// A service for handling the process of retrieving and generating thumbnail images -/// for existing Media objects, whether remote or locally available. +/// A service for retrieval and caching of thumbnails for Media objects. final class MediaImageService: NSObject { static let shared = MediaImageService() @@ -24,29 +24,20 @@ final class MediaImageService: NSObject { // MARK: - Thumbnails - /// Returns a small thumbnail for the given media asset. - /// - /// The thumbnail size is different on different devices, but it's suitable - /// for presentation in collection views. The returned images are decompressed - /// (bitmapped) and are ready to be displayed. + /// Returns a thumbnail for the given media asset. The images are decompressed + /// (or bitmapped) and are ready to be displayed. @MainActor - func thumbnail(for media: Media) async throws -> UIImage { - let size = ThumbnailSize.small + func thumbnail(for media: Media, size: ThumbnailSize = .small) async throws -> UIImage { guard media.remoteStatus != .stub else { let media = try await fetchStubMedia(for: media) - guard media.remoteStatus != .stub else { - assertionFailure("The fetched media still has a .stub status") - throw MediaThumbnailExporter.ThumbnailExportError.failedToGenerateThumbnailFileURL - } return try await _thumbnail(for: media, size: size) } - return try await _thumbnail(for: media, size: size) } @MainActor private func _thumbnail(for media: Media, size: ThumbnailSize) async throws -> UIImage { - if let image = await cachedThumbnail(for: media, size: size) { + if let image = await cachedThumbnail(for: media.objectID, size: size) { return image } if let image = await localThumbnail(for: media, size: size) { @@ -58,33 +49,29 @@ final class MediaImageService: NSObject { // MARK: - Cached Thumbnail /// Returns a local thumbnail for the given media object (if available). - @MainActor - private func cachedThumbnail(for media: Media, size: ThumbnailSize) async -> UIImage? { - let objectID = media.objectID + private func cachedThumbnail(for mediaID: NSManagedObjectID, size: ThumbnailSize) async -> UIImage? { return try? await Task.detached { - let imageURL = try self.getCachedThumbnailURL(for: objectID, size: size) + let imageURL = try self.getCachedThumbnailURL(for: mediaID, size: size) let data = try Data(contentsOf: imageURL) return try decompressedImage(from: data) }.value } // The save is performed asynchronously to eliminate any delays. It's - // exceedingly unlikely it'll result in any duplicated work thanks to the + // exceedingly unlikely it will result in any duplicated work thanks to the // memore caches. - @MainActor - private func saveThumbnail(for media: Media, size: ThumbnailSize, _ closure: @escaping (URL) throws -> Void) { - let objectID = media.objectID + private func saveThumbnail(for mediaID: NSManagedObjectID, size: ThumbnailSize, _ closure: @escaping (URL) throws -> Void) { ioQueue.async { - if let targetURL = try? self.getCachedThumbnailURL(for: objectID, size: size) { + if let targetURL = try? self.getCachedThumbnailURL(for: mediaID, size: size) { try? closure(targetURL) } } } - private func getCachedThumbnailURL(for objectID: NSManagedObjectID, size: ThumbnailSize) throws -> URL { - let objectID = objectID.uriRepresentation().lastPathComponent + private func getCachedThumbnailURL(for mediaID: NSManagedObjectID, size: ThumbnailSize) throws -> URL { + let mediaID = mediaID.uriRepresentation().lastPathComponent return try mediaFileManager.makeLocalMediaURL( - withFilename: "\(objectID)-\(size.rawValue)-thumbnail", + withFilename: "\(mediaID)-\(size.rawValue)-thumbnail", fileExtension: nil, // We don't know ahead of time incremented: false ) @@ -122,7 +109,7 @@ final class MediaImageService: NSObject { }.value // The order is important to ensure `export.url` still exists when creating an image - saveThumbnail(for: media, size: size) { targetURL in + saveThumbnail(for: media.objectID, size: size) { targetURL in try FileManager.default.moveItem(at: export.url, to: targetURL) } @@ -135,7 +122,7 @@ final class MediaImageService: NSObject { @MainActor private func remoteThumbnail(for media: Media, size: ThumbnailSize) async throws -> UIImage { let targetSize = MediaImageService.getThumbnailSize(for: media, size: size) - guard let imageURL = remoteThumbnailURL(for: media, targetSize: targetSize) else { + guard let imageURL = media.getRemoteThumbnailURL(targetSize: targetSize) else { throw URLError(.badURL) } @@ -147,7 +134,7 @@ final class MediaImageService: NSObject { } let (data, _) = try await session.data(for: request) - saveThumbnail(for: media, size: size) { targetURL in + saveThumbnail(for: media.objectID, size: size) { targetURL in try data.write(to: targetURL) } @@ -156,25 +143,6 @@ final class MediaImageService: NSObject { }.value } - @MainActor - private func remoteThumbnailURL(for media: Media, targetSize: CGSize) -> URL? { - switch media.mediaType { - case .image: - guard let remoteURL = media.remoteURL.flatMap(URL.init) else { - return nil - } - if media.blog.isPrivateAtWPCom() || (!media.blog.isHostedAtWPcom && media.blog.isBasicAuthCredentialStored()) { - return WPImageURLHelper.imageURLWithSize(targetSize, forImageURL: remoteURL) - } else { - let scale = 1.0 / UIScreen.main.scale - let targetSize = targetSize.applying(CGAffineTransform(scaleX: scale, y: scale)) - return PhotonImageURLHelper.photonURL(with: targetSize, forImageURL: remoteURL) - } - default: - return media.remoteThumbnailURL.flatMap(URL.init) - } - } - // MARK: - Stubs @MainActor @@ -186,23 +154,20 @@ final class MediaImageService: NSObject { let objectID = try await mediaRepository.getMedia(withID: mediaID, in: .init(media.blog)) return try coreDataStack.mainContext.existingObject(with: objectID) } +} - // MARK: - Target Size +// MARK: - MediaImageService (ThumbnailSize) + +extension MediaImageService { enum ThumbnailSize: String { + /// The small thumbnail that can be used in collection view cells and + /// similar situations. case small } - /// Returns an optiomal target size in pixels for a thumbnail of the given + /// Returns an optimal target size in pixels for a thumbnail of the given /// size for the given media asset. - /// - /// The size is calculated to fill a collection view cell, assuming the app - /// displays a few cells in a row. The cell size can vary depending on whether the - /// device is in landscape or portrait mode, but the thumbnail size is - /// guaranteed to always be the same across app launches. - /// - /// Example: if media size is 2000x3000 px and targetSize is 200x200 px, the - /// returned value will be 200x300 px. static func getThumbnailSize(for media: Media, size: ThumbnailSize) -> CGSize { let mediaSize = CGSize( width: CGFloat(media.width?.floatValue ?? 0), @@ -220,6 +185,11 @@ final class MediaImageService: NSObject { private static func getPreferredThumbnailSize(for thumbnail: ThumbnailSize) -> CGSize { switch thumbnail { case .small: + /// The size is calculated to fill a collection view cell, assuming the app + /// displays a 4 or 5 cells in one row. The cell size can vary depending + /// on whether the device is in landscape or portrait mode, but the thumbnail size is + /// guaranteed to always be the same across app launches and optimized for + /// a portraint (dominant) mode. let screenSide = min(UIScreen.main.bounds.width, UIScreen.main.bounds.height) let itemPerRow = UIDevice.current.userInterfaceIdiom == .pad ? 5 : 4 let availableWidth = screenSide - MediaViewController.spacing * CGFloat(itemPerRow - 1) @@ -229,6 +199,11 @@ final class MediaImageService: NSObject { } } + /// Image CDN (Photon) and `MediaImageExporter` both don't support "aspect-fill" + /// resizing mode, so the service performs the necessary calculations by itself. + /// + /// Example: if media size is 2000x3000 px and targetSize is 200x200 px, the + /// returned value will be 200x300 px. For more examples, see `MediaImageServiceTests`. static func targetSize(forMediaSize mediaSize: CGSize, targetSize originalTargetSize: CGSize) -> CGSize { guard mediaSize.width > 0 && mediaSize.height > 0 else { return originalTargetSize @@ -249,7 +224,36 @@ final class MediaImageService: NSObject { } } -// MARK: - Decompression +// MARK: - Helpers (RemoteURL) + +private extension Media { + /// Returns the thumbnail remote URL with a given target size. It uses + /// Image CDN (formerly Photon) if available. + /// + /// - parameter targetSize: Target size in pixels. + func getRemoteThumbnailURL(targetSize: CGSize) -> URL? { + switch mediaType { + case .image: + guard let remoteURL = remoteURL.flatMap(URL.init) else { + return nil + } + if !isEligibleForPhoton { + return WPImageURLHelper.imageURLWithSize(targetSize, forImageURL: remoteURL) + } else { + let targetSize = targetSize.scaled(by: 1.0 / UIScreen.main.scale) + return PhotonImageURLHelper.photonURL(with: targetSize, forImageURL: remoteURL) + } + default: + return remoteThumbnailURL.flatMap(URL.init) + } + } + + var isEligibleForPhoton: Bool { + blog.isPrivateAtWPCom() || (!blog.isHostedAtWPcom && blog.isBasicAuthCredentialStored()) + } +} + +// MARK: - Helpers (Decompression) // Forces decompression (or bitmapping) to happen in the background. // It's very expensive for some image formats, such as JPEG. From 8ec168842dddcc8f8c38d33b43b459a00ab851c1 Mon Sep 17 00:00:00 2001 From: kean Date: Thu, 21 Sep 2023 13:14:49 -0400 Subject: [PATCH 07/10] Use non-retina size for GIFs --- WordPress/Classes/Services/MediaImageService.swift | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/WordPress/Classes/Services/MediaImageService.swift b/WordPress/Classes/Services/MediaImageService.swift index 56f4fb5ef21d..4b33782b40e3 100644 --- a/WordPress/Classes/Services/MediaImageService.swift +++ b/WordPress/Classes/Services/MediaImageService.swift @@ -237,6 +237,9 @@ private extension Media { guard let remoteURL = remoteURL.flatMap(URL.init) else { return nil } + // Download a non-retina version for GIFs: makes a massive difference + // in terms of size. Example: 2.4 MB -> 350 KB. + let targetSize = remoteURL.isGif ? targetSize.scaled(by: 1.0 / UIScreen.main.scale) : targetSize if !isEligibleForPhoton { return WPImageURLHelper.imageURLWithSize(targetSize, forImageURL: remoteURL) } else { @@ -249,7 +252,7 @@ private extension Media { } var isEligibleForPhoton: Bool { - blog.isPrivateAtWPCom() || (!blog.isHostedAtWPcom && blog.isBasicAuthCredentialStored()) + !(blog.isPrivateAtWPCom() || (!blog.isHostedAtWPcom && blog.isBasicAuthCredentialStored())) } } From 207480b00a6deb27ebd50d3a7c89590dc6f3ad0e Mon Sep 17 00:00:00 2001 From: kean Date: Thu, 21 Sep 2023 13:22:05 -0400 Subject: [PATCH 08/10] Clear media cache on migration --- WordPress/Classes/Services/MediaImageService.swift | 11 +++++++++++ WordPress/Classes/System/WordPressAppDelegate.swift | 1 + 2 files changed, 12 insertions(+) diff --git a/WordPress/Classes/Services/MediaImageService.swift b/WordPress/Classes/Services/MediaImageService.swift index 4b33782b40e3..505e4d457eea 100644 --- a/WordPress/Classes/Services/MediaImageService.swift +++ b/WordPress/Classes/Services/MediaImageService.swift @@ -22,6 +22,17 @@ final class MediaImageService: NSObject { self.session = URLSession(configuration: configuration) } + static func migrateCacheIfNeeded() { + let didMigrateKey = "MediaImageService-didMigrateCacheKey" + guard Feature.enabled(.mediaModernization) && !UserDefaults.standard.bool(forKey: didMigrateKey) else { + return + } + UserDefaults.standard.set(true, forKey: didMigrateKey) + DispatchQueue.global(qos: .utility).async { + MediaFileManager.clearAllMediaCacheFiles(onCompletion: nil, onError: nil) + } + } + // MARK: - Thumbnails /// Returns a thumbnail for the given media asset. The images are decompressed diff --git a/WordPress/Classes/System/WordPressAppDelegate.swift b/WordPress/Classes/System/WordPressAppDelegate.swift index 42d9868890cc..988cb72dee17 100644 --- a/WordPress/Classes/System/WordPressAppDelegate.swift +++ b/WordPress/Classes/System/WordPressAppDelegate.swift @@ -92,6 +92,7 @@ class WordPressAppDelegate: UIResponder, UIApplicationDelegate { window = UIWindow(frame: UIScreen.main.bounds) AppAppearance.overrideAppearance() MemoryCache.shared.register() + MediaImageService.migrateCacheIfNeeded() // Start CrashLogging as soon as possible (in case a crash happens during startup) try? loggingStack.start() From 70e1e851cc31c2fee6a2a043b20cc801c5dd4296 Mon Sep 17 00:00:00 2001 From: kean Date: Thu, 21 Sep 2023 13:24:42 -0400 Subject: [PATCH 09/10] Generate thumbnail earlier --- .../Media/Library/MediaCollectionCellViewModel.swift | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/WordPress/Classes/ViewRelated/Media/Library/MediaCollectionCellViewModel.swift b/WordPress/Classes/ViewRelated/Media/Library/MediaCollectionCellViewModel.swift index f06052043b63..91d8efaa0aa6 100644 --- a/WordPress/Classes/ViewRelated/Media/Library/MediaCollectionCellViewModel.swift +++ b/WordPress/Classes/ViewRelated/Media/Library/MediaCollectionCellViewModel.swift @@ -36,7 +36,7 @@ final class MediaCollectionCellViewModel { // No sure why but `.initial` didn't work. self.updateOverlayState() - thumbnailObservation = media.observe(\.localThumbnailURL, options: [.new]) { [weak self] media, _ in + thumbnailObservation = media.observe(\.localURL, options: [.new]) { [weak self] media, _ in self?.didUpdateLocalThumbnail() } } @@ -118,7 +118,7 @@ final class MediaCollectionCellViewModel { // Monitors thumbnails generated by `MediaImportService`. private func didUpdateLocalThumbnail() { - guard media.remoteStatus != .sync, media.localThumbnailURL != nil else { return } + guard media.remoteStatus != .sync, media.localURL != nil else { return } fetchThumbnailIfNeeded() } From 8e553ec397956d20d5e245b37b03c267c98f2e79 Mon Sep 17 00:00:00 2001 From: kean Date: Thu, 21 Sep 2023 15:02:20 -0400 Subject: [PATCH 10/10] Optimize target size for panoramas --- .../Classes/Services/MediaImageService.swift | 22 +++++++++++-------- .../MediaImageServiceTests.swift | 2 +- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/WordPress/Classes/Services/MediaImageService.swift b/WordPress/Classes/Services/MediaImageService.swift index 505e4d457eea..c51e193eae29 100644 --- a/WordPress/Classes/Services/MediaImageService.swift +++ b/WordPress/Classes/Services/MediaImageService.swift @@ -219,17 +219,21 @@ extension MediaImageService { guard mediaSize.width > 0 && mediaSize.height > 0 else { return originalTargetSize } - let scaleHorizontal = originalTargetSize.width / mediaSize.width - let scaleVertical = originalTargetSize.height / mediaSize.height - // Scale image to fill the target size but avoid upscaling. - let aspectFillScale = min(1, max(scaleHorizontal, scaleVertical)) - let targetSize = mediaSize.scaled(by: aspectFillScale).rounded() + // Scale image to fill the target size but avoid upscaling + let scale = min(1, max( + originalTargetSize.width / mediaSize.width, + originalTargetSize.height / mediaSize.height + )) + let targetSize = mediaSize.scaled(by: scale).rounded() + // Sanitize the size to make sure ultra-wide panoramas are still resized // to fit the target size, but increase it a bit for an acceptable size. - if targetSize.width > originalTargetSize.width * 4 || - targetSize.height > originalTargetSize.height * 4 { - let aspectFitScale = min(scaleHorizontal, scaleVertical) - return mediaSize.scaled(by: aspectFitScale * 4).rounded() + let threshold: CGFloat = 4 + if targetSize.width > originalTargetSize.width * threshold || targetSize.height > originalTargetSize.height * threshold { + return CGSize( + width: min(targetSize.width, originalTargetSize.width * threshold), + height: min(targetSize.height, originalTargetSize.height * threshold) + ) } return targetSize } diff --git a/WordPress/WordPressTest/MediaImageServiceTests.swift b/WordPress/WordPressTest/MediaImageServiceTests.swift index d4e5686ae0ff..990dcdebdbe3 100644 --- a/WordPress/WordPressTest/MediaImageServiceTests.swift +++ b/WordPress/WordPressTest/MediaImageServiceTests.swift @@ -114,7 +114,7 @@ class MediaImageServiceTests: CoreDataTestCase { forMediaSize: CGSize(width: 4000, height: 400), targetSize: CGSize(width: 200, height: 200) ), - CGSize(width: 800, height: 80) + CGSize(width: 800, height: 200) ) }