-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
refactor(mobile): cleanup image request pipeline on iOS #27481
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
4272fc2
27b4f5f
eeca96d
ca2b444
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,91 @@ | ||
| import Accelerate | ||
| import Foundation | ||
|
|
||
| class ImageRequest { | ||
| private let lock = UnfairLock() | ||
| private var _isCancelled = false | ||
| private var callback: ((Result<[String: Int64]?, any Error>) -> Void)? | ||
|
|
||
| var isCancelled: Bool { | ||
| lock.lock() | ||
| defer { lock.unlock() } | ||
| return _isCancelled | ||
| } | ||
|
|
||
| init(callback: @escaping (Result<[String: Int64]?, any Error>) -> Void) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This probably shouldn't be nullable. |
||
| self.callback = callback | ||
| } | ||
|
|
||
| func cancel() { | ||
| lock.lock() | ||
| _isCancelled = true | ||
| let cb = callback | ||
| callback = nil | ||
| lock.unlock() | ||
|
|
||
| onCancel() | ||
| cb?(ImageProcessing.cancelledResult) | ||
| } | ||
|
|
||
| func finish(with result: Result<[String: Int64]?, any Error>) { | ||
| lock.lock() | ||
| let cb = callback | ||
| callback = nil | ||
| lock.unlock() | ||
|
|
||
| cb?(result) | ||
| } | ||
|
|
||
| func onCancel() {} | ||
|
|
||
| func encodeToPointer(_ data: Data) { | ||
| let length = data.count | ||
| let pointer = malloc(length)! | ||
| data.copyBytes(to: pointer.assumingMemoryBound(to: UInt8.self), count: length) | ||
| if isCancelled { | ||
| free(pointer) | ||
| return | ||
| } | ||
| finish(with: .success([ | ||
| "pointer": Int64(Int(bitPattern: pointer)), | ||
| "length": Int64(length), | ||
| ])) | ||
| } | ||
|
|
||
| func cgImageToPointer(_ cgImage: CGImage, format: vImage_CGImageFormat) throws { | ||
| var fmt = format | ||
|
LeLunZ marked this conversation as resolved.
|
||
| let buffer = try vImage_Buffer(cgImage: cgImage, format: fmt) | ||
| if isCancelled { | ||
| buffer.free() | ||
| return | ||
| } | ||
| finish(with: .success([ | ||
| "pointer": Int64(Int(bitPattern: buffer.data)), | ||
| "width": Int64(buffer.width), | ||
| "height": Int64(buffer.height), | ||
| "rowBytes": Int64(buffer.rowBytes), | ||
| ])) | ||
| } | ||
| } | ||
|
|
||
| class RequestRegistry<T: ImageRequest> { | ||
| private let lock = UnfairLock() | ||
| private var requests = [Int64: T]() | ||
|
|
||
| func add(requestId: Int64, request: T) { | ||
| lock.lock() | ||
| requests[requestId] = request | ||
| lock.unlock() | ||
| } | ||
|
|
||
| @discardableResult | ||
| func remove(requestId: Int64) -> T? { | ||
| lock.lock() | ||
| defer { lock.unlock() } | ||
| return requests.removeValue(forKey: requestId) | ||
| } | ||
|
|
||
| func cancel(requestId: Int64) { | ||
| remove(requestId: requestId)?.cancel() | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,13 +3,11 @@ import Flutter | |
| import MobileCoreServices | ||
| import Photos | ||
|
|
||
| class LocalImageRequest { | ||
| class LocalImageRequest: ImageRequest { | ||
| weak var operation: Operation? | ||
| var isCancelled = false | ||
| let callback: (Result<[String: Int64]?, any Error>) -> Void | ||
|
|
||
| init(callback: @escaping (Result<[String: Int64]?, any Error>) -> Void) { | ||
| self.callback = callback | ||
| override func onCancel() { | ||
| operation?.cancel() | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -31,18 +29,15 @@ class LocalImageApiImpl: LocalImageApi { | |
| return requestOptions | ||
| }() | ||
|
|
||
| private static let assetQueue = DispatchQueue(label: "thumbnail.assets", qos: .userInitiated) | ||
| private static let requestQueue = DispatchQueue(label: "thumbnail.requests", qos: .userInitiated) | ||
| private static let cancelQueue = DispatchQueue(label: "thumbnail.cancellation", qos: .default) | ||
| private static let registry = RequestRegistry<LocalImageRequest>() | ||
|
|
||
| private static var rgbaFormat = vImage_CGImageFormat( | ||
| private static let rgbaFormat = vImage_CGImageFormat( | ||
| bitsPerComponent: 8, | ||
| bitsPerPixel: 32, | ||
| colorSpace: CGColorSpaceCreateDeviceRGB(), | ||
| bitmapInfo: CGBitmapInfo(rawValue: CGImageAlphaInfo.premultipliedLast.rawValue), | ||
| renderingIntent: .defaultIntent | ||
| )! | ||
| private static var requests = [Int64: LocalImageRequest]() | ||
| private static let assetCache = { | ||
| let assetCache = NSCache<NSString, PHAsset>() | ||
| assetCache.countLimit = 10000 | ||
|
|
@@ -67,19 +62,17 @@ class LocalImageApiImpl: LocalImageApi { | |
| func requestImage(assetId: String, requestId: Int64, width: Int64, height: Int64, isVideo: Bool, preferEncoded: Bool, completion: @escaping (Result<[String: Int64]?, any Error>) -> Void) { | ||
| let request = LocalImageRequest(callback: completion) | ||
| let operation = BlockOperation { | ||
| if request.isCancelled { | ||
| return completion(ImageProcessing.cancelledResult) | ||
| } | ||
| if request.isCancelled { return } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Where does it respond to Pigeon in this case?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Always in the cancel method of the request. |
||
|
|
||
| guard let asset = Self.requestAsset(assetId: assetId) | ||
| else { | ||
| Self.remove(requestId: requestId) | ||
| completion(.failure(PigeonError(code: "", message: "Could not get asset data for \(assetId)", details: nil))) | ||
| return | ||
| Self.registry.remove(requestId: requestId) | ||
| return request.finish(with: .failure(PigeonError(code: "", message: "Could not get asset data for \(assetId)", details: nil))) | ||
| } | ||
|
|
||
| if request.isCancelled { | ||
| return completion(ImageProcessing.cancelledResult) | ||
| Self.registry.remove(requestId: requestId) | ||
| return | ||
| } | ||
|
|
||
| if preferEncoded { | ||
|
|
@@ -98,30 +91,17 @@ class LocalImageApiImpl: LocalImageApi { | |
| ) | ||
|
|
||
| if request.isCancelled { | ||
| Self.remove(requestId: requestId) | ||
| return completion(ImageProcessing.cancelledResult) | ||
| Self.registry.remove(requestId: requestId) | ||
| return | ||
| } | ||
|
|
||
| guard let data = imageData else { | ||
| Self.remove(requestId: requestId) | ||
| return completion(.failure(PigeonError(code: "", message: "Could not get image data for \(assetId)", details: nil))) | ||
| } | ||
|
|
||
| let length = data.count | ||
| let pointer = malloc(length)! | ||
| data.copyBytes(to: pointer.assumingMemoryBound(to: UInt8.self), count: length) | ||
|
|
||
| if request.isCancelled { | ||
| free(pointer) | ||
| Self.remove(requestId: requestId) | ||
| return completion(ImageProcessing.cancelledResult) | ||
| Self.registry.remove(requestId: requestId) | ||
| return request.finish(with: .failure(PigeonError(code: "", message: "Could not get image data for \(assetId)", details: nil))) | ||
| } | ||
|
|
||
| request.callback(.success([ | ||
| "pointer": Int64(Int(bitPattern: pointer)), | ||
| "length": Int64(length), | ||
| ])) | ||
| Self.remove(requestId: requestId) | ||
| request.encodeToPointer(data) | ||
| Self.registry.remove(requestId: requestId) | ||
| return | ||
| } | ||
|
|
||
|
|
@@ -137,76 +117,47 @@ class LocalImageApiImpl: LocalImageApi { | |
| ) | ||
|
|
||
| if request.isCancelled { | ||
| return completion(ImageProcessing.cancelledResult) | ||
| Self.registry.remove(requestId: requestId) | ||
| return | ||
| } | ||
|
|
||
| guard let image = image, | ||
| let cgImage = image.cgImage else { | ||
| Self.remove(requestId: requestId) | ||
| return completion(.failure(PigeonError(code: "", message: "Could not get pixel data for \(assetId)", details: nil))) | ||
| Self.registry.remove(requestId: requestId) | ||
| return request.finish(with: .failure(PigeonError(code: "", message: "Could not get pixel data for \(assetId)", details: nil))) | ||
| } | ||
|
|
||
| if request.isCancelled { | ||
| return completion(ImageProcessing.cancelledResult) | ||
| Self.registry.remove(requestId: requestId) | ||
| return | ||
| } | ||
|
|
||
| do { | ||
| let buffer = try vImage_Buffer(cgImage: cgImage, format: Self.rgbaFormat) | ||
|
|
||
| if request.isCancelled { | ||
| buffer.free() | ||
| return completion(ImageProcessing.cancelledResult) | ||
| } | ||
|
|
||
| request.callback(.success([ | ||
| "pointer": Int64(Int(bitPattern: buffer.data)), | ||
| "width": Int64(buffer.width), | ||
| "height": Int64(buffer.height), | ||
| "rowBytes": Int64(buffer.rowBytes) | ||
| ])) | ||
| Self.remove(requestId: requestId) | ||
| try request.cgImageToPointer(cgImage, format: Self.rgbaFormat) | ||
| Self.registry.remove(requestId: requestId) | ||
| } catch { | ||
| Self.remove(requestId: requestId) | ||
| return completion(.failure(PigeonError(code: "", message: "Failed to convert image for \(assetId): \(error)", details: nil))) | ||
| Self.registry.remove(requestId: requestId) | ||
| return request.finish(with: .failure(PigeonError(code: "", message: "Failed to convert image for \(assetId): \(error)", details: nil))) | ||
| } | ||
| } | ||
|
|
||
| request.operation = operation | ||
| Self.add(requestId: requestId, request: request) | ||
| Self.registry.add(requestId: requestId, request: request) | ||
| ImageProcessing.queue.addOperation(operation) | ||
| } | ||
|
|
||
| func cancelRequest(requestId: Int64) { | ||
| Self.cancel(requestId: requestId) | ||
| } | ||
|
|
||
| private static func add(requestId: Int64, request: LocalImageRequest) -> Void { | ||
| requestQueue.sync { requests[requestId] = request } | ||
| } | ||
|
|
||
| private static func remove(requestId: Int64) -> Void { | ||
| requestQueue.sync { requests[requestId] = nil } | ||
| } | ||
|
|
||
| private static func cancel(requestId: Int64) -> Void { | ||
| requestQueue.async { | ||
| guard let request = requests.removeValue(forKey: requestId) else { return } | ||
| request.isCancelled = true | ||
| guard let operation = request.operation else { return } | ||
| if operation.isCancelled { | ||
| cancelQueue.async { request.callback(ImageProcessing.cancelledResult) } | ||
| } | ||
| } | ||
| Self.registry.cancel(requestId: requestId) | ||
| } | ||
|
|
||
| private static func requestAsset(assetId: String) -> PHAsset? { | ||
| var asset: PHAsset? | ||
| assetQueue.sync { asset = assetCache.object(forKey: assetId as NSString) } | ||
| if asset != nil { return asset } | ||
| if let cached = assetCache.object(forKey: assetId as NSString) { | ||
| return cached | ||
| } | ||
|
|
||
| guard let asset = PHAsset.fetchAssets(withLocalIdentifiers: [assetId], options: Self.fetchOptions).firstObject | ||
| else { return nil } | ||
| assetQueue.async { assetCache.setObject(asset, forKey: assetId as NSString) } | ||
| assetCache.setObject(asset, forKey: assetId as NSString) | ||
| return asset | ||
|
Comment on lines
+154
to
161
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The dictionary isn't thread-safe.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn’t the NSCache thread save, the docs are linked above? |
||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need a lock?