From b31950789facd9f5a2123df2cd17b952dd875216 Mon Sep 17 00:00:00 2001 From: Mauro <34335419+Velin92@users.noreply.github.com> Date: Tue, 13 Oct 2026 17:33:22 +0200 Subject: [PATCH] Move synchronous image upload preparation off the main actor (#6323) * Move synchronous image upload preparation off the main actor Signed-off-by: Ivan Magda * Make media preparation cooperative with cancellation Replace the dispatch queue with concurrent copy, image and video-thumbnail functions. Return sendable upload metadata directly, bound each batch to two preparations and discard unfinished results when cancellation occurs. Keep file metadata reads synchronous and prevent the preview from sending a result after its preparation task has been cancelled. Replace unrelated tests with cancellation and batch-order coverage requested during review. * Update MediaUploadingPreprocessor.swift * Run media preparation in a nonisolated preprocessor Make MediaUploadingPreprocessor a nonisolated struct whose copy, image processing and video thumbnail run in @concurrent instance methods, instead of a main-actor type with static helpers and an extra setting parameter. Check cancellation with plain early returns between the slow steps, name the batch limit, stop logging cancelled video exports as errors, and restore develop's structure and doc comments where they were changed without need. Replace the tests that didn't reach the new code with tests for cleanup on failure, on batch failure and on cancellation, and make the preview test set up its blocking mock before the view model starts processing. * Apply review suggestions to media preparation Docs: - Drop the doc comment on maximumConcurrentProcessingCount, the name says it. - Move the cleanup note into the main description of both processMedia functions, so the Returns lines match develop again. Tests: - Rename the test helper to copyResourceToTemporaryDirectory(_:). --------- Signed-off-by: Ivan Magda Co-authored-by: Ivan Magda --- ElementX/Sources/Other/BlurHashEncode.swift | 20 +-- .../MediaUploadPreviewScreenViewModel.swift | 6 +- .../Media/MediaUploadingPreprocessor.swift | 151 +++++++++++++----- ...diaUploadPreviewScreenViewModelTests.swift | 33 +++- .../MediaUploadingPreprocessorTests.swift | 82 ++++++++++ 5 files changed, 237 insertions(+), 55 deletions(-) diff --git a/ElementX/Sources/Other/BlurHashEncode.swift b/ElementX/Sources/Other/BlurHashEncode.swift index 48baedfc9..a5bc9563a 100644 --- a/ElementX/Sources/Other/BlurHashEncode.swift +++ b/ElementX/Sources/Other/BlurHashEncode.swift @@ -22,9 +22,9 @@ // swiftlint:disable all -import UIKit +import SwiftUI -extension UIImage { +nonisolated extension UIImage { public func blurHash(numberOfComponents components: (Int, Int)) -> String? { let pixelWidth = Int(round(size.width * scale)) let pixelHeight = Int(round(size.height * scale)) @@ -116,14 +116,14 @@ extension UIImage { } } -private func encodeDC(_ value: (Float, Float, Float)) -> Int { +private nonisolated func encodeDC(_ value: (Float, Float, Float)) -> Int { let roundedR = linearTosRGB(value.0) let roundedG = linearTosRGB(value.1) let roundedB = linearTosRGB(value.2) return (roundedR << 16) + (roundedG << 8) + roundedB } -private func encodeAC(_ value: (Float, Float, Float), maximumValue: Float) -> Int { +private nonisolated func encodeAC(_ value: (Float, Float, Float), maximumValue: Float) -> Int { let quantR = Int(max(0, min(18, floor(signPow(value.0 / maximumValue, 0.5) * 9 + 9.5)))) let quantG = Int(max(0, min(18, floor(signPow(value.1 / maximumValue, 0.5) * 9 + 9.5)))) let quantB = Int(max(0, min(18, floor(signPow(value.2 / maximumValue, 0.5) * 9 + 9.5)))) @@ -131,11 +131,11 @@ private func encodeAC(_ value: (Float, Float, Float), maximumValue: Float) -> In return quantR * 19 * 19 + quantG * 19 + quantB } -private func signPow(_ value: Float, _ exp: Float) -> Float { +private nonisolated func signPow(_ value: Float, _ exp: Float) -> Float { copysign(pow(abs(value), exp), value) } -private func linearTosRGB(_ value: Float) -> Int { +private nonisolated func linearTosRGB(_ value: Float) -> Int { let v = max(0, min(1, value)) if v <= 0.0031308 { return Int(v * 12.92 * 255 + 0.5) @@ -144,7 +144,7 @@ private func linearTosRGB(_ value: Float) -> Int { } } -private func sRGBToLinear(_ value: Type) -> Float { +private nonisolated func sRGBToLinear(_ value: Type) -> Float { let v = Float(Int64(value)) / 255 if v <= 0.04045 { return v / 12.92 @@ -153,9 +153,9 @@ private func sRGBToLinear(_ value: Type) -> Float { } } -private let encodeCharacters: [String] = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz#$%*+,-.:;=?@[]^_{|}~".map { String($0) } +private nonisolated let encodeCharacters: [String] = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz#$%*+,-.:;=?@[]^_{|}~".map { String($0) } -extension BinaryInteger { +nonisolated extension BinaryInteger { func encode83(length: Int) -> String { var result = "" for i in 1...length { @@ -166,7 +166,7 @@ extension BinaryInteger { } } -private func pow(_ base: Int, _ exponent: Int) -> Int { +private nonisolated func pow(_ base: Int, _ exponent: Int) -> Int { (0.. 1 { switch await sendGallery(mediaInfos: mediaInfos, caption: caption) { diff --git a/ElementX/Sources/Services/Media/MediaUploadingPreprocessor.swift b/ElementX/Sources/Services/Media/MediaUploadingPreprocessor.swift index 682690855..074557f64 100644 --- a/ElementX/Sources/Services/Media/MediaUploadingPreprocessor.swift +++ b/ElementX/Sources/Services/Media/MediaUploadingPreprocessor.swift @@ -8,10 +8,11 @@ import AVFoundation import MatrixRustSDK -import UIKit +import SwiftUI import UniformTypeIdentifiers -indirect enum MediaUploadingPreprocessorError: Error { +indirect nonisolated enum MediaUploadingPreprocessorError: Error { + case cancelled case maxUploadSizeUnknown case maxUploadSizeExceeded(limit: UInt) @@ -30,7 +31,7 @@ indirect enum MediaUploadingPreprocessorError: Error { case failedConvertingVideo } -enum MediaInfo { +nonisolated enum MediaInfo { case image(imageURL: URL, thumbnailURL: URL, imageInfo: ImageInfo) case video(videoURL: URL, thumbnailURL: URL, videoInfo: VideoInfo) case audio(audioURL: URL, audioInfo: AudioInfo) @@ -69,7 +70,7 @@ enum MediaInfo { } } -private struct ImageProcessingInfo { +private nonisolated struct ImageProcessingInfo { let url: URL let height: Double let width: Double @@ -77,7 +78,7 @@ private struct ImageProcessingInfo { let blurhash: String? } -private struct VideoProcessingInfo { +private nonisolated struct VideoProcessingInfo { let url: URL let height: Double let width: Double @@ -85,90 +86,151 @@ private struct VideoProcessingInfo { let mimeType: String } -struct MediaUploadingPreprocessor { +nonisolated struct MediaUploadingPreprocessor { let userSettings: UserSettings - enum Constants { + nonisolated enum Constants { static let maximumThumbnailSize = CGSize(width: 800, height: 600) static let optimizedMaxPixelSize = 2048.0 static let jpegCompressionQuality = 0.78 static let videoThumbnailTime = 5.0 // seconds + static let maximumConcurrentProcessingCount = 2 } /// Processes media at the given URLs. It will generate thumbnails for images and videos, convert videos to 1080p mp4, strip GPS locations - /// from images and retrieve associated media information - /// - Parameter urls: the file URL + /// from images and retrieve associated media information. On failure or cancellation all the prepared files are removed. + /// - Parameter urls: the file URLs /// - Returns: a collection of results containing specific type of `MediaInfo` depending on the file type /// and its associated details or any resulting error func processMedia(at urls: [URL], maxUploadSize: UInt) async -> Result<[MediaInfo], MediaUploadingPreprocessorError> { - await withTaskGroup { taskGroup in - for (index, url) in urls.enumerated() { - taskGroup.addTask { - let result = await processMedia(at: url, maxUploadSize: maxUploadSize) - return (index, result) + await withTaskGroup(of: (Int, Result).self) { taskGroup in + var pendingIndices = urls.indices.makeIterator() + + func processNextURL() { + guard let index = pendingIndices.next() else { return } + _ = taskGroup.addTaskUnlessCancelled { + await (index, processMedia(at: urls[index], maxUploadSize: maxUploadSize)) } } + for _ in 0.. Result { - // Start by copying the file to a unique temporary location in order to avoid conflicts if processing it multiple times - // All the other operations will be made relative to it - let uniqueFolder = FileManager.default.temporaryDirectory.appendingPathComponent(UUID().uuidString) - var newURL = uniqueFolder.appendingPathComponent(url.lastPathComponent) + guard !Task.isCancelled else { return .failure(.cancelled) } + + let preparedURL: URL do { - try FileManager.default.createDirectory(at: uniqueFolder, withIntermediateDirectories: true) - try FileManager.default.copyItem(at: url, to: newURL) + preparedURL = try await copyToUniqueFolder(url) } catch { - return .failure(.failedProcessingMedia(error)) + return .failure(error) } - // Process unknown types as plain files - guard let type = UTType(filenameExtension: newURL.pathExtension), - let mimeType = type.preferredMIMEType else { - return processFile(at: newURL, mimeType: "application/octet-stream", maxUploadSize: maxUploadSize) + let result = await processPreparedMedia(at: preparedURL, maxUploadSize: maxUploadSize) + + if Task.isCancelled { + await removeUniqueFolders(containing: [preparedURL]) + return .failure(.cancelled) + } + if case .failure = result { + await removeUniqueFolders(containing: [preparedURL]) } - if type.conforms(to: .image) { - return processImage(at: &newURL, type: type, mimeType: mimeType, maxUploadSize: maxUploadSize) - } else if type.conforms(to: .movie) || type.conforms(to: .video) { - return await processVideo(at: newURL, maxUploadSize: maxUploadSize) - } else if type.conforms(to: .audio) { - return await processAudio(at: newURL, mimeType: mimeType, maxUploadSize: maxUploadSize) - } else { - return processFile(at: newURL, mimeType: mimeType, maxUploadSize: maxUploadSize) - } + return result } // MARK: - Private + /// Copies the file into a new unique folder to avoid conflicts when processing the same file more than once. + /// Every file derived from the copy (converted media, thumbnails) is written into that same folder. + @concurrent + private func copyToUniqueFolder(_ url: URL) async throws(MediaUploadingPreprocessorError) -> URL { + let uniqueFolder = FileManager.default.temporaryDirectory.appendingPathComponent(UUID().uuidString) + let newURL = uniqueFolder.appendingPathComponent(url.lastPathComponent) + do { + try FileManager.default.createDirectory(at: uniqueFolder, withIntermediateDirectories: true) + try FileManager.default.copyItem(at: url, to: newURL) + return newURL + } catch { + try? FileManager.default.removeItem(at: uniqueFolder) + throw .failedProcessingMedia(error) + } + } + + /// Removes the whole unique folder (see `copyToUniqueFolder`) that holds each of the given files. + @concurrent + private func removeUniqueFolders(containing urls: [URL]) async { + for url in urls { + try? FileManager.default.removeItem(at: url.deletingLastPathComponent()) + } + } + + private func processPreparedMedia(at url: URL, maxUploadSize: UInt) async -> Result { + guard !Task.isCancelled else { return .failure(.cancelled) } + + // Process unknown types as plain files + guard let type = UTType(filenameExtension: url.pathExtension), + let mimeType = type.preferredMIMEType else { + return processFile(at: url, mimeType: "application/octet-stream", maxUploadSize: maxUploadSize) + } + + if type.conforms(to: .image) { + return await processImage(at: url, type: type, mimeType: mimeType, maxUploadSize: maxUploadSize) + } else if type.conforms(to: .movie) || type.conforms(to: .video) { + return await processVideo(at: url, maxUploadSize: maxUploadSize) + } else if type.conforms(to: .audio) { + return await processAudio(at: url, mimeType: mimeType, maxUploadSize: maxUploadSize) + } else { + return processFile(at: url, mimeType: mimeType, maxUploadSize: maxUploadSize) + } + } + /// Prepares an image for upload. Strips location data from it and generates a thumbnail /// - Parameters: /// - url: The image URL /// - type: its UTType /// - mimeType: the mimeType extracted from the UTType /// - Returns: Returns a `MediaInfo.image` containing the URLs for the modified image and its thumbnail plus the corresponding `ImageInfo` - private func processImage(at url: inout URL, type: UTType, mimeType: String, maxUploadSize: UInt) -> Result { + @concurrent + private func processImage(at url: URL, type: UTType, mimeType: String, maxUploadSize: UInt) async -> Result { + var url = url do { try stripLocationFromImage(at: url, type: type) + guard !Task.isCancelled else { return .failure(.cancelled) } var mimeType = mimeType if userSettings.app.optimizeMediaUploads, !type.conforms(to: .gif) { @@ -187,6 +249,7 @@ struct MediaUploadingPreprocessor { url = convertedURL } } + guard !Task.isCancelled else { return .failure(.cancelled) } let thumbnailResult = try generateThumbnailForImage(at: url) @@ -231,7 +294,9 @@ struct MediaUploadingPreprocessor { private func processVideo(at url: URL, maxUploadSize: UInt) async -> Result { do { let result = try await convertVideoToMP4(url, targetFileSize: UInt(maxUploadSize)) - let thumbnailResult = try await generateThumbnailForVideoAt(result.url) + guard !Task.isCancelled else { return .failure(.cancelled) } + + let thumbnailResult = try await generateThumbnailForVideo(at: result.url) let videoSize = (try? FileManager.default.sizeForItem(at: result.url)) ?? 0 let thumbnailSize = (try? FileManager.default.sizeForItem(at: thumbnailResult.url)) ?? 0 @@ -396,7 +461,8 @@ struct MediaUploadingPreprocessor { /// Generates a thumbnail for the video at the given URL /// - Parameter url: the video URL /// - Returns: the URL for the resulting thumbnail and its sizing info as an `ImageProcessingResult` - private func generateThumbnailForVideoAt(_ url: URL) async throws(MediaUploadingPreprocessorError) -> ImageProcessingInfo { + @concurrent + private func generateThumbnailForVideo(at url: URL) async throws(MediaUploadingPreprocessorError) -> ImageProcessingInfo { let assetImageGenerator = AVAssetImageGenerator(asset: AVURLAsset(url: url)) assetImageGenerator.appliesPreferredTrackTransform = true assetImageGenerator.maximumSize = Constants.maximumThumbnailSize @@ -464,6 +530,7 @@ struct MediaUploadingPreprocessor { do { try await exportSession.export(to: outputURL, as: .mp4) } catch { + guard !Task.isCancelled else { throw .cancelled } MXLog.error("Video conversion failed: \(error)") throw .failedConvertingVideo } @@ -494,7 +561,7 @@ struct MediaUploadingPreprocessor { // MARK: - Extensions -private extension CGImageSource { +private nonisolated extension CGImageSource { var size: CGSize? { guard let properties = CGImageSourceCopyPropertiesAtIndex(self, 0, nil) as? [NSString: Any], var width = properties[kCGImagePropertyPixelWidth] as? Int, @@ -517,7 +584,7 @@ private extension CGImageSource { } } -private extension AVAssetTrack { +private nonisolated extension AVAssetTrack { var size: CGSize { get async throws { let naturalSize = try await load(.naturalSize) diff --git a/UnitTests/Sources/MediaUploadPreviewScreenViewModelTests.swift b/UnitTests/Sources/MediaUploadPreviewScreenViewModelTests.swift index bf273cb3a..00b085b00 100644 --- a/UnitTests/Sources/MediaUploadPreviewScreenViewModelTests.swift +++ b/UnitTests/Sources/MediaUploadPreviewScreenViewModelTests.swift @@ -34,6 +34,35 @@ final class MediaUploadPreviewScreenViewModelTests { userIndicatorController = UserIndicatorControllerMock() } + @Test + func cancelWhileSendWaitsForPreparation() async throws { + // Given a screen whose media preparation is still running. + let (startedStream, startedContinuation) = AsyncStream.makeStream() + let (resumeStream, resumeContinuation) = AsyncStream.makeStream() + defer { resumeContinuation.finish() } + + let preparationStarted = deferFulfillment(startedStream) { _ in true } + setUpViewModel(urls: [imageURL], expectedCaption: nil) { + startedContinuation.yield() + var iterator = resumeStream.makeAsyncIterator() + _ = await iterator.next() // Only returns once the preparation is cancelled. + return .success(100 * 1024 * 1024) + } + try await preparationStarted.fulfill() + + // When sending and then dismissing the screen before the preparation finishes. + context.send(viewAction: .send) + let loadingFinished = deferFulfillment(context.observe(\.viewState.shouldDisableInteraction)) { !$0 } + context.send(viewAction: .cancel) + viewModel.stopProcessing() // Called by the coordinator when the screen is dismissed. + try await loadingFinished.fulfill() + + // Then nothing is sent and no error is shown. + #expect(timelineProxy.sendImageUrlThumbnailURLImageInfoCaptionRequestHandleCallsCount == 0) + #expect(userIndicatorController.submitIndicatorDelayCallsCount == 1, "Only the loading indicator should be shown") + #expect(context.viewState.bindings.alertInfo == nil) + } + @Test func imageUploadWithoutCaption() async throws { setUpViewModel(urls: [imageURL], expectedCaption: nil) @@ -293,7 +322,8 @@ final class MediaUploadPreviewScreenViewModelTests { expectedCaption: String?, maxUploadSizeResult: Result? = nil, simulateGallerySendFailure: Bool = false, - galleryEnabled: Bool = true) { + galleryEnabled: Bool = true, + maxUploadSizeClosure: (() async -> Result)? = nil) { timelineProxy = TimelineProxyMock(.init()) timelineProxy.sendAudioUrlAudioInfoCaptionRequestHandleClosure = { [weak self] _, _, caption, _ in self?.verifyCaption(caption, expectedCaption: expectedCaption) ?? .failure(.sdkError(TestError.unknown)) @@ -316,6 +346,7 @@ final class MediaUploadPreviewScreenViewModelTests { if let maxUploadSizeResult { clientProxy.underlyingMaxMediaUploadSize = maxUploadSizeResult } + clientProxy.maxMediaUploadSizeClosure = maxUploadSizeClosure viewModel = MediaUploadPreviewScreenViewModel(mediaURLs: urls, caption: nil, diff --git a/UnitTests/Sources/MediaUploadingPreprocessorTests.swift b/UnitTests/Sources/MediaUploadingPreprocessorTests.swift index c4ee5dde2..72efa16a6 100644 --- a/UnitTests/Sources/MediaUploadingPreprocessorTests.swift +++ b/UnitTests/Sources/MediaUploadingPreprocessorTests.swift @@ -435,8 +435,90 @@ final class MediaUploadingPreprocessorTests { #expect(optimizedImageInfo.height == 2048) } + @Test + func batchPreservesInputOrderBeyondConcurrencyLimit() async throws { + let names = ["test_image.png", "test_animated_image.gif", "landscape_test_image.jpg"] + try #require(names.count > MediaUploadingPreprocessor.Constants.maximumConcurrentProcessingCount) + let inputs = try names.map { try #require(Bundle(for: Self.self).url(forResource: $0, withExtension: nil)) } + let results = try await mediaUploadingPreprocessor.processMedia(at: inputs, maxUploadSize: maxUploadSize).get() + defer { + for result in results { + try? FileManager.default.removeItem(at: result.url.deletingLastPathComponent()) + } + } + #expect(results.map(\.url.lastPathComponent) == names) + for result in results { + let thumbnailURL = try #require(result.thumbnailURL) + #expect(try Data(contentsOf: result.url).isEmpty == false) + #expect(try Data(contentsOf: thumbnailURL).isEmpty == false) + } + } + + @Test + func failedPreparationRemovesItsFiles() async throws { + let url = try copyResourceToTemporaryDirectory("test_image.png") + defer { try? FileManager.default.removeItem(at: url.deletingLastPathComponent()) } + + guard case .failure(.maxUploadSizeExceeded) = await mediaUploadingPreprocessor.processMedia(at: url, maxUploadSize: 1) else { + Issue.record("Expected the upload size to be exceeded") + return + } + #expect(try preparedFolders(for: url).isEmpty) + } + + @Test + func batchFailureReturnsTheErrorAndRemovesPreparedFiles() async throws { + let url = try copyResourceToTemporaryDirectory("test_image.png") + defer { try? FileManager.default.removeItem(at: url.deletingLastPathComponent()) } + let missingURL = url.deletingLastPathComponent().appendingPathComponent("missing.png") + + // The missing file comes last so that at least one image has already been prepared when the batch fails. + guard case .failure(.failedProcessingMedia) = await mediaUploadingPreprocessor.processMedia(at: [url, url, missingURL], maxUploadSize: maxUploadSize) else { + Issue.record("Expected the missing file to fail the batch") + return + } + #expect(try preparedFolders(for: url).isEmpty) + } + + @Test + func cancellationRemovesPreparedFiles() async throws { + let url = try copyResourceToTemporaryDirectory("test_image.png") + defer { try? FileManager.default.removeItem(at: url.deletingLastPathComponent()) } + + let task = Task { await mediaUploadingPreprocessor.processMedia(at: url, maxUploadSize: maxUploadSize) } + await Task.yield() // Let the preparation start. + task.cancel() + + guard case .failure(.cancelled) = await task.value else { + Issue.record("Expected the preparation to be cancelled") + return + } + #expect(try preparedFolders(for: url).isEmpty) + } + // MARK: - Private + /// Copies a test asset to a uniquely named file, so the folders the preprocessor prepares from it can be found afterwards. + private func copyResourceToTemporaryDirectory(_ resource: String) throws -> URL { + let assetURL = try #require(Bundle(for: Self.self).url(forResource: resource, withExtension: nil)) + let directory = FileManager.default.temporaryDirectory.appendingPathComponent("\(Self.self)-\(UUID().uuidString)") + try FileManager.default.createDirectory(at: directory, withIntermediateDirectories: true) + let url = directory.appendingPathComponent("\(UUID().uuidString).\(assetURL.pathExtension)") + try FileManager.default.copyItem(at: assetURL, to: url) + return url + } + + /// The unique folders created by the preprocessor for the given file that still exist. + private func preparedFolders(for url: URL) throws -> [URL] { + let name = url.deletingPathExtension().lastPathComponent + return try FileManager.default.contentsOfDirectory(at: FileManager.default.temporaryDirectory, includingPropertiesForKeys: nil) + .filter { UUID(uuidString: $0.lastPathComponent) != nil } + .filter { folder in + let contents = (try? FileManager.default.contentsOfDirectory(atPath: folder.path(percentEncoded: false))) ?? [] + return contents.contains { $0.contains(name) } + } + } + private func isEqual(_ lhs: N, _ rhs: N, within tolerance: N) -> Bool { isEqual(Double(lhs), Double(rhs), within: Double(tolerance)) }