From c1578b4b0605df086a5c575e60500b75ef6f7ea2 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Fri, 7 Dec 2018 15:46:43 -0500 Subject: [PATCH 01/10] Always load conversation media async. --- .../Cells/ConversationMediaView.swift | 235 ++++++++++++++---- 1 file changed, 185 insertions(+), 50 deletions(-) diff --git a/Signal/src/ViewControllers/ConversationView/Cells/ConversationMediaView.swift b/Signal/src/ViewControllers/ConversationView/Cells/ConversationMediaView.swift index 36a334713..ef461a0ea 100644 --- a/Signal/src/ViewControllers/ConversationView/Cells/ConversationMediaView.swift +++ b/Signal/src/ViewControllers/ConversationView/Cells/ConversationMediaView.swift @@ -28,7 +28,23 @@ public class ConversationMediaView: UIView { private let maxMessageWidth: CGFloat private var loadBlock : (() -> Void)? private var unloadBlock : (() -> Void)? - private var didFailToLoad = false + + enum LoadState { + case unloaded + case loading + case loaded + case failed + } + + // The loadState property allows us to: + // + // * Make sure we only have one load attempt + // enqueued at a time for a given piece of media. + // * We never retry media that can't be loaded. + // * We skip media loads which are no longer + // necessary by the time they reach the front + // of the queue. + private var loadState : LoadState = .unloaded @objc public required init(mediaCache: NSCache, @@ -148,13 +164,11 @@ public class ConversationMediaView: UIView { _ = addUploadProgressIfNecessary(animatedImageView) loadBlock = { [weak self] in - guard let strongSelf = self else { - return - } if animatedImageView.image != nil { + owsFailDebug("Unexpectedly already loaded.") return } - let cachedValue = strongSelf.tryToLoadMedia(loadMediaBlock: { () -> AnyObject? in + self?.tryToLoadMedia(loadMediaBlock: { () -> AnyObject? in guard attachmentStream.isValidImage else { owsFailDebug("Ignoring invalid attachment.") return nil @@ -166,12 +180,16 @@ public class ConversationMediaView: UIView { let animatedImage = YYImage(contentsOfFile: filePath) return animatedImage }, - cacheKey: cacheKey, - canLoadAsync: true) - guard let image = cachedValue as? YYImage else { - return - } - animatedImageView.image = image + applyMediaBlock: { (media) in + AssertIsOnMainThread() + + guard let image = media as? YYImage else { + owsFailDebug("Media has unexpected type: \(type(of:media))") + return + } + animatedImageView.image = image + }, + cacheKey: cacheKey) } unloadBlock = { animatedImageView.image = nil @@ -196,29 +214,33 @@ public class ConversationMediaView: UIView { stillImageView.autoPinEdgesToSuperviewEdges() _ = addUploadProgressIfNecessary(stillImageView) loadBlock = { [weak self] in - guard let strongSelf = self else { - return - } if stillImageView.image != nil { + owsFailDebug("Unexpectedly already loaded.") return } - let cachedValue = strongSelf.tryToLoadMedia(loadMediaBlock: { () -> AnyObject? in + self?.tryToLoadMedia(loadMediaBlock: { () -> AnyObject? in guard attachmentStream.isValidImage else { owsFailDebug("Ignoring invalid attachment.") return nil } return attachmentStream.thumbnailImageMedium(success: { (image) in + AssertIsOnMainThread() + stillImageView.image = image }, failure: { Logger.error("Could not load thumbnail") }) }, - cacheKey: cacheKey, - canLoadAsync: true) - guard let image = cachedValue as? UIImage else { - return - } - stillImageView.image = image + applyMediaBlock: { (media) in + AssertIsOnMainThread() + + guard let image = media as? UIImage else { + owsFailDebug("Media has unexpected type: \(type(of:media))") + return + } + stillImageView.image = image + }, + cacheKey: cacheKey) } unloadBlock = { stillImageView.image = nil @@ -251,29 +273,33 @@ public class ConversationMediaView: UIView { } loadBlock = { [weak self] in - guard let strongSelf = self else { - return - } if stillImageView.image != nil { + owsFailDebug("Unexpectedly already loaded.") return } - let cachedValue = strongSelf.tryToLoadMedia(loadMediaBlock: { () -> AnyObject? in + self?.tryToLoadMedia(loadMediaBlock: { () -> AnyObject? in guard attachmentStream.isValidVideo else { owsFailDebug("Ignoring invalid attachment.") return nil } return attachmentStream.thumbnailImageMedium(success: { (image) in + AssertIsOnMainThread() + stillImageView.image = image }, failure: { Logger.error("Could not load thumbnail") }) }, - cacheKey: cacheKey, - canLoadAsync: true) - guard let image = cachedValue as? UIImage else { - return - } - stillImageView.image = image + applyMediaBlock: { (media) in + AssertIsOnMainThread() + + guard let image = media as? UIImage else { + owsFailDebug("Media has unexpected type: \(type(of:media))") + return + } + stillImageView.image = image + }, + cacheKey: cacheKey) } unloadBlock = { stillImageView.image = nil @@ -313,48 +339,157 @@ public class ConversationMediaView: UIView { } private func tryToLoadMedia(loadMediaBlock: @escaping () -> AnyObject?, - cacheKey: String, - canLoadAsync: Bool) -> AnyObject? { + applyMediaBlock: @escaping (AnyObject) -> Void, + cacheKey: String) { AssertIsOnMainThread() - guard !didFailToLoad else { - return nil + // It's critical that we update loadState once + // our load attempt is complete. + let loadCompletion : (AnyObject?) -> Void = { [weak self] (possibleMedia) in + AssertIsOnMainThread() + + guard let strongSelf = self else { + return + } + guard strongSelf.loadState == .loading else { + Logger.verbose("Skipping obsolete load.") + return + } + guard let media = possibleMedia else { + strongSelf.loadState = .failed + // TODO: + // [self showAttachmentErrorViewWithMediaView:mediaView]; + return + } + + applyMediaBlock(media) + + strongSelf.loadState = .loaded + } + + guard loadState == .loading else { + owsFailDebug("Unexpected load state: \(loadState)") + return } + let mediaCache = self.mediaCache if let media = mediaCache.object(forKey: cacheKey as NSString) { Logger.verbose("media cache hit") - return media + DispatchQueue.main.async { + loadCompletion(media) + } + return } - if let media = loadMediaBlock() { - Logger.verbose("media cache miss") - mediaCache.setObject(media, forKey: cacheKey as NSString) - return media + Logger.verbose("media cache miss") + + // * loadCompletion is used to update the view's state to reflect + // the outcome of the load attempt. + // * loadQueueCompletion is used to kick off the next load. + let asyncLoadBlock : AsyncLoadBlock = { [weak self] (loadQueueCompletion: @escaping AsyncLoadCompletionBlock) in + AssertIsOnMainThread() + + guard let strongSelf = self else { + loadQueueCompletion() + return + } + guard strongSelf.loadState == .loading else { + loadQueueCompletion() + return + } + + ConversationMediaView.loadQueue.async { + guard let media = loadMediaBlock() else { + Logger.error("Failed to load media.") + + DispatchQueue.main.async { + loadCompletion(nil) + loadQueueCompletion() + } + return + } + + DispatchQueue.main.async { + mediaCache.setObject(media, forKey: cacheKey as NSString) + loadCompletion(media) + loadQueueCompletion() + } + } } - guard canLoadAsync else { - Logger.error("Failed to load media.") - didFailToLoad = true - // TODO: - // [self showAttachmentErrorViewWithMediaView:mediaView]; - return nil + + ConversationMediaView.enqueue(asyncLoadBlock:asyncLoadBlock) + } + + // We maintain a serial "queue" (as in the data structure, + // not a dispatch queue) of media loads. We don't just use + // a serial dispatch queue because we want to perform the + // loads _in the opposite of the order_ in which they are + // enqueued (see below). + private typealias AsyncLoadCompletionBlock = () -> Void + private typealias AsyncLoadBlock = (@escaping AsyncLoadCompletionBlock) -> Void + private static var asyncLoadBlocks = [AsyncLoadBlock]() + + // We use this queue to perform the media loads. + // These loads are expensive, so we want to: + // + // * Do them off the main thread. + // * Only do one at a time. + // * Avoid this work if possible (obsolete loads for + // views that are no longer visible, redundant loads + // of media already being loaded, don't retry media + // that can't be loaded, etc.). + private static let loadQueue = DispatchQueue(label: "org.signal.asyncMediaLoadQueue") + + private class func enqueue(asyncLoadBlock : @escaping AsyncLoadBlock) { + AssertIsOnMainThread() + + asyncLoadBlocks.append(asyncLoadBlock) + + processNextAsyncLoadBlock() + } + + // We want to load views _in the opposite order_ in which + // their loads were enqueued, to reflect current view state. + // I.e. the currently visible views were enqueued _after_ + // any views which are no longer visible. + private class func processNextAsyncLoadBlock() { + AssertIsOnMainThread() + + guard let block = asyncLoadBlocks.popLast() else { + // No more load blocks to process. + return } - return nil + + block({ + DispatchQueue.main.async { + self.processNextAsyncLoadBlock() + } + }) } @objc public func loadMedia() { AssertIsOnMainThread() - guard let loadBlock = loadBlock else { - return + switch loadState { + case .unloaded: + loadState = .loading + + guard let loadBlock = loadBlock else { + return + } + loadBlock() + case .loading, .loaded, .failed: + break } - loadBlock() } @objc public func unloadMedia() { AssertIsOnMainThread() + loadState = .unloaded + guard let unloadBlock = unloadBlock else { return } From 956859244dc8fd1c8cb7de9dc1981a1b3296f106 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Fri, 7 Dec 2018 15:56:02 -0500 Subject: [PATCH 02/10] Always load conversation media async. --- .../Cells/ConversationMediaView.swift | 110 +++++------------- 1 file changed, 27 insertions(+), 83 deletions(-) diff --git a/Signal/src/ViewControllers/ConversationView/Cells/ConversationMediaView.swift b/Signal/src/ViewControllers/ConversationView/Cells/ConversationMediaView.swift index ef461a0ea..09751834f 100644 --- a/Signal/src/ViewControllers/ConversationView/Cells/ConversationMediaView.swift +++ b/Signal/src/ViewControllers/ConversationView/Cells/ConversationMediaView.swift @@ -28,7 +28,7 @@ public class ConversationMediaView: UIView { private let maxMessageWidth: CGFloat private var loadBlock : (() -> Void)? private var unloadBlock : (() -> Void)? - + enum LoadState { case unloaded case loading @@ -44,7 +44,7 @@ public class ConversationMediaView: UIView { // * We skip media loads which are no longer // necessary by the time they reach the front // of the queue. - private var loadState : LoadState = .unloaded + private var loadState: LoadState = .unloaded @objc public required init(mediaCache: NSCache, @@ -184,7 +184,7 @@ public class ConversationMediaView: UIView { AssertIsOnMainThread() guard let image = media as? YYImage else { - owsFailDebug("Media has unexpected type: \(type(of:media))") + owsFailDebug("Media has unexpected type: \(type(of: media))") return } animatedImageView.image = image @@ -225,7 +225,7 @@ public class ConversationMediaView: UIView { } return attachmentStream.thumbnailImageMedium(success: { (image) in AssertIsOnMainThread() - + stillImageView.image = image }, failure: { Logger.error("Could not load thumbnail") @@ -233,9 +233,9 @@ public class ConversationMediaView: UIView { }, applyMediaBlock: { (media) in AssertIsOnMainThread() - + guard let image = media as? UIImage else { - owsFailDebug("Media has unexpected type: \(type(of:media))") + owsFailDebug("Media has unexpected type: \(type(of: media))") return } stillImageView.image = image @@ -284,7 +284,7 @@ public class ConversationMediaView: UIView { } return attachmentStream.thumbnailImageMedium(success: { (image) in AssertIsOnMainThread() - + stillImageView.image = image }, failure: { Logger.error("Could not load thumbnail") @@ -292,9 +292,9 @@ public class ConversationMediaView: UIView { }, applyMediaBlock: { (media) in AssertIsOnMainThread() - + guard let image = media as? UIImage else { - owsFailDebug("Media has unexpected type: \(type(of:media))") + owsFailDebug("Media has unexpected type: \(type(of: media))") return } stillImageView.image = image @@ -345,9 +345,9 @@ public class ConversationMediaView: UIView { // It's critical that we update loadState once // our load attempt is complete. - let loadCompletion : (AnyObject?) -> Void = { [weak self] (possibleMedia) in + let loadCompletion: (AnyObject?) -> Void = { [weak self] (possibleMedia) in AssertIsOnMainThread() - + guard let strongSelf = self else { return } @@ -361,12 +361,12 @@ public class ConversationMediaView: UIView { // [self showAttachmentErrorViewWithMediaView:mediaView]; return } - + applyMediaBlock(media) strongSelf.loadState = .loaded } - + guard loadState == .loading else { owsFailDebug("Unexpected load state: \(loadState)") return @@ -382,53 +382,24 @@ public class ConversationMediaView: UIView { } Logger.verbose("media cache miss") - - // * loadCompletion is used to update the view's state to reflect - // the outcome of the load attempt. - // * loadQueueCompletion is used to kick off the next load. - let asyncLoadBlock : AsyncLoadBlock = { [weak self] (loadQueueCompletion: @escaping AsyncLoadCompletionBlock) in - AssertIsOnMainThread() - - guard let strongSelf = self else { - loadQueueCompletion() - return - } - guard strongSelf.loadState == .loading else { - loadQueueCompletion() - return - } - - ConversationMediaView.loadQueue.async { - guard let media = loadMediaBlock() else { - Logger.error("Failed to load media.") - - DispatchQueue.main.async { - loadCompletion(nil) - loadQueueCompletion() - } - return - } - + + ConversationMediaView.loadQueue.async { + guard let media = loadMediaBlock() else { + Logger.error("Failed to load media.") + DispatchQueue.main.async { - mediaCache.setObject(media, forKey: cacheKey as NSString) - loadCompletion(media) - loadQueueCompletion() + loadCompletion(nil) } + return + } + + DispatchQueue.main.async { + mediaCache.setObject(media, forKey: cacheKey as NSString) + loadCompletion(media) } } - - ConversationMediaView.enqueue(asyncLoadBlock:asyncLoadBlock) } - // We maintain a serial "queue" (as in the data structure, - // not a dispatch queue) of media loads. We don't just use - // a serial dispatch queue because we want to perform the - // loads _in the opposite of the order_ in which they are - // enqueued (see below). - private typealias AsyncLoadCompletionBlock = () -> Void - private typealias AsyncLoadBlock = (@escaping AsyncLoadCompletionBlock) -> Void - private static var asyncLoadBlocks = [AsyncLoadBlock]() - // We use this queue to perform the media loads. // These loads are expensive, so we want to: // @@ -440,33 +411,6 @@ public class ConversationMediaView: UIView { // that can't be loaded, etc.). private static let loadQueue = DispatchQueue(label: "org.signal.asyncMediaLoadQueue") - private class func enqueue(asyncLoadBlock : @escaping AsyncLoadBlock) { - AssertIsOnMainThread() - - asyncLoadBlocks.append(asyncLoadBlock) - - processNextAsyncLoadBlock() - } - - // We want to load views _in the opposite order_ in which - // their loads were enqueued, to reflect current view state. - // I.e. the currently visible views were enqueued _after_ - // any views which are no longer visible. - private class func processNextAsyncLoadBlock() { - AssertIsOnMainThread() - - guard let block = asyncLoadBlocks.popLast() else { - // No more load blocks to process. - return - } - - block({ - DispatchQueue.main.async { - self.processNextAsyncLoadBlock() - } - }) - } - @objc public func loadMedia() { AssertIsOnMainThread() @@ -474,7 +418,7 @@ public class ConversationMediaView: UIView { switch loadState { case .unloaded: loadState = .loading - + guard let loadBlock = loadBlock else { return } @@ -489,7 +433,7 @@ public class ConversationMediaView: UIView { AssertIsOnMainThread() loadState = .unloaded - + guard let unloadBlock = unloadBlock else { return } From 5cb319a9c875591c8f35aca1633f1bd093c6bfd1 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Fri, 7 Dec 2018 16:00:06 -0500 Subject: [PATCH 03/10] Always load conversation media async. --- .../Cells/ConversationMediaView.swift | 85 +++++++++++++++++-- 1 file changed, 76 insertions(+), 9 deletions(-) diff --git a/Signal/src/ViewControllers/ConversationView/Cells/ConversationMediaView.swift b/Signal/src/ViewControllers/ConversationView/Cells/ConversationMediaView.swift index 09751834f..51603694b 100644 --- a/Signal/src/ViewControllers/ConversationView/Cells/ConversationMediaView.swift +++ b/Signal/src/ViewControllers/ConversationView/Cells/ConversationMediaView.swift @@ -383,23 +383,55 @@ public class ConversationMediaView: UIView { Logger.verbose("media cache miss") - ConversationMediaView.loadQueue.async { - guard let media = loadMediaBlock() else { - Logger.error("Failed to load media.") + // * loadCompletion is used to update the view's state to reflect + // the outcome of the load attempt. + // * loadQueueCompletion is used to kick off the next load. + let asyncLoadBlock: AsyncLoadBlock = { [weak self] (loadQueueCompletion: @escaping AsyncLoadCompletionBlock) in + AssertIsOnMainThread() - DispatchQueue.main.async { - loadCompletion(nil) - } + guard let strongSelf = self else { + loadQueueCompletion() + return + } + guard strongSelf.loadState == .loading else { + loadQueueCompletion() return } - DispatchQueue.main.async { - mediaCache.setObject(media, forKey: cacheKey as NSString) - loadCompletion(media) + ConversationMediaView.loadQueue.async { + guard let media = loadMediaBlock() else { + Logger.error("Failed to load media.") + + DispatchQueue.main.async { + loadCompletion(nil) + loadQueueCompletion() + } + return + } + + DispatchQueue.main.async { + mediaCache.setObject(media, forKey: cacheKey as NSString) + loadCompletion(media) + loadQueueCompletion() + } } } + + ConversationMediaView.enqueue(asyncLoadBlock: asyncLoadBlock) } + // We maintain a serial "queue" (as in the data structure, + // not a dispatch queue) of media loads. We don't just use + // a serial dispatch queue because we want to perform the + // loads _in the opposite of the order_ in which they are + // enqueued (see below). + // + // NOTE: These properties should only be accessed on the main thread. + private typealias AsyncLoadCompletionBlock = () -> Void + private typealias AsyncLoadBlock = (@escaping AsyncLoadCompletionBlock) -> Void + private static var asyncLoadBlocks = [AsyncLoadBlock]() + private static var currentAsyncLoadBlock: AsyncLoadBlock? + // We use this queue to perform the media loads. // These loads are expensive, so we want to: // @@ -411,6 +443,41 @@ public class ConversationMediaView: UIView { // that can't be loaded, etc.). private static let loadQueue = DispatchQueue(label: "org.signal.asyncMediaLoadQueue") + private class func enqueue(asyncLoadBlock : @escaping AsyncLoadBlock) { + AssertIsOnMainThread() + + asyncLoadBlocks.append(asyncLoadBlock) + + processNextAsyncLoadBlock() + } + + // We want to load views _in the opposite order_ in which + // their loads were enqueued, to reflect current view state. + // I.e. the currently visible views were enqueued _after_ + // any views which are no longer visible. + private class func processNextAsyncLoadBlock() { + AssertIsOnMainThread() + + guard currentAsyncLoadBlock == nil else { + // Only have one async load in flight at a time. + return + } + + guard let block = asyncLoadBlocks.popLast() else { + // No more load blocks to process. + return + } + currentAsyncLoadBlock = block + + block({ + DispatchQueue.main.async { + currentAsyncLoadBlock = nil + + self.processNextAsyncLoadBlock() + } + }) + } + @objc public func loadMedia() { AssertIsOnMainThread() From ddd6732f74e80c710ed3f73243cdd6fc8b6e5095 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Fri, 7 Dec 2018 16:00:15 -0500 Subject: [PATCH 04/10] Revert "Always load conversation media async." This reverts commit 297aa080163cb6eb324b40bae790768ff2fb1721. --- .../Cells/ConversationMediaView.swift | 85 ++----------------- 1 file changed, 9 insertions(+), 76 deletions(-) diff --git a/Signal/src/ViewControllers/ConversationView/Cells/ConversationMediaView.swift b/Signal/src/ViewControllers/ConversationView/Cells/ConversationMediaView.swift index 51603694b..09751834f 100644 --- a/Signal/src/ViewControllers/ConversationView/Cells/ConversationMediaView.swift +++ b/Signal/src/ViewControllers/ConversationView/Cells/ConversationMediaView.swift @@ -383,55 +383,23 @@ public class ConversationMediaView: UIView { Logger.verbose("media cache miss") - // * loadCompletion is used to update the view's state to reflect - // the outcome of the load attempt. - // * loadQueueCompletion is used to kick off the next load. - let asyncLoadBlock: AsyncLoadBlock = { [weak self] (loadQueueCompletion: @escaping AsyncLoadCompletionBlock) in - AssertIsOnMainThread() + ConversationMediaView.loadQueue.async { + guard let media = loadMediaBlock() else { + Logger.error("Failed to load media.") - guard let strongSelf = self else { - loadQueueCompletion() - return - } - guard strongSelf.loadState == .loading else { - loadQueueCompletion() + DispatchQueue.main.async { + loadCompletion(nil) + } return } - ConversationMediaView.loadQueue.async { - guard let media = loadMediaBlock() else { - Logger.error("Failed to load media.") - - DispatchQueue.main.async { - loadCompletion(nil) - loadQueueCompletion() - } - return - } - - DispatchQueue.main.async { - mediaCache.setObject(media, forKey: cacheKey as NSString) - loadCompletion(media) - loadQueueCompletion() - } + DispatchQueue.main.async { + mediaCache.setObject(media, forKey: cacheKey as NSString) + loadCompletion(media) } } - - ConversationMediaView.enqueue(asyncLoadBlock: asyncLoadBlock) } - // We maintain a serial "queue" (as in the data structure, - // not a dispatch queue) of media loads. We don't just use - // a serial dispatch queue because we want to perform the - // loads _in the opposite of the order_ in which they are - // enqueued (see below). - // - // NOTE: These properties should only be accessed on the main thread. - private typealias AsyncLoadCompletionBlock = () -> Void - private typealias AsyncLoadBlock = (@escaping AsyncLoadCompletionBlock) -> Void - private static var asyncLoadBlocks = [AsyncLoadBlock]() - private static var currentAsyncLoadBlock: AsyncLoadBlock? - // We use this queue to perform the media loads. // These loads are expensive, so we want to: // @@ -443,41 +411,6 @@ public class ConversationMediaView: UIView { // that can't be loaded, etc.). private static let loadQueue = DispatchQueue(label: "org.signal.asyncMediaLoadQueue") - private class func enqueue(asyncLoadBlock : @escaping AsyncLoadBlock) { - AssertIsOnMainThread() - - asyncLoadBlocks.append(asyncLoadBlock) - - processNextAsyncLoadBlock() - } - - // We want to load views _in the opposite order_ in which - // their loads were enqueued, to reflect current view state. - // I.e. the currently visible views were enqueued _after_ - // any views which are no longer visible. - private class func processNextAsyncLoadBlock() { - AssertIsOnMainThread() - - guard currentAsyncLoadBlock == nil else { - // Only have one async load in flight at a time. - return - } - - guard let block = asyncLoadBlocks.popLast() else { - // No more load blocks to process. - return - } - currentAsyncLoadBlock = block - - block({ - DispatchQueue.main.async { - currentAsyncLoadBlock = nil - - self.processNextAsyncLoadBlock() - } - }) - } - @objc public func loadMedia() { AssertIsOnMainThread() From 358d97bf5ba4c53b4cc06f8218734c9967c90a98 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Fri, 7 Dec 2018 16:24:46 -0500 Subject: [PATCH 05/10] Always load conversation media async. --- .../Cells/ConversationMediaView.swift | 69 +++++++++++++++++-- 1 file changed, 62 insertions(+), 7 deletions(-) diff --git a/Signal/src/ViewControllers/ConversationView/Cells/ConversationMediaView.swift b/Signal/src/ViewControllers/ConversationView/Cells/ConversationMediaView.swift index 09751834f..a94c299fb 100644 --- a/Signal/src/ViewControllers/ConversationView/Cells/ConversationMediaView.swift +++ b/Signal/src/ViewControllers/ConversationView/Cells/ConversationMediaView.swift @@ -29,12 +29,7 @@ public class ConversationMediaView: UIView { private var loadBlock : (() -> Void)? private var unloadBlock : (() -> Void)? - enum LoadState { - case unloaded - case loading - case loaded - case failed - } + // MARK: - LoadState // The loadState property allows us to: // @@ -44,7 +39,52 @@ public class ConversationMediaView: UIView { // * We skip media loads which are no longer // necessary by the time they reach the front // of the queue. - private var loadState: LoadState = .unloaded + + enum LoadState { + case unloaded + case loading + case loaded + case failed + } + + // Thread-safe access to load state. + // + // We use a "box" class so that we can capture a reference + // to this box (rather than self) and a) safely access + // if off the main thread b) not prevent deallocation of + // self. + private class ThreadSafeLoadState { + private var value: LoadState + + required init(_ value: LoadState) { + self.value = value + } + + func get() -> LoadState { + objc_sync_enter(self) + let valueCopy = value + objc_sync_exit(self) + return valueCopy + } + + func set(_ newValue: LoadState) { + objc_sync_enter(self) + value = newValue + objc_sync_exit(self) + } + } + private let threadSafeLoadState = ThreadSafeLoadState(.unloaded) + // Convenience accessors. + private var loadState: LoadState { + get { + return threadSafeLoadState.get() + } + set { + threadSafeLoadState.set(newValue) + } + } + + // MARK: - Initializers @objc public required init(mediaCache: NSCache, @@ -69,6 +109,14 @@ public class ConversationMediaView: UIView { notImplemented() } + deinit { + AssertIsOnMainThread() + + loadState = .unloaded + } + + // MARK: - + private func createContents() { AssertIsOnMainThread() @@ -383,7 +431,13 @@ public class ConversationMediaView: UIView { Logger.verbose("media cache miss") + let threadSafeLoadState = self.threadSafeLoadState ConversationMediaView.loadQueue.async { + guard threadSafeLoadState.get() == .loading else { + Logger.verbose("Skipping obsolete load.") + return + } + guard let media = loadMediaBlock() else { Logger.error("Failed to load media.") @@ -395,6 +449,7 @@ public class ConversationMediaView: UIView { DispatchQueue.main.async { mediaCache.setObject(media, forKey: cacheKey as NSString) + loadCompletion(media) } } From b9404938c4d5708196a867085b80e5a7743ffb0e Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Mon, 10 Dec 2018 10:12:21 -0500 Subject: [PATCH 06/10] Respond to CR. --- .../Cells/ConversationMediaView.swift | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/Signal/src/ViewControllers/ConversationView/Cells/ConversationMediaView.swift b/Signal/src/ViewControllers/ConversationView/Cells/ConversationMediaView.swift index a94c299fb..6c1ea27bf 100644 --- a/Signal/src/ViewControllers/ConversationView/Cells/ConversationMediaView.swift +++ b/Signal/src/ViewControllers/ConversationView/Cells/ConversationMediaView.swift @@ -212,6 +212,8 @@ public class ConversationMediaView: UIView { _ = addUploadProgressIfNecessary(animatedImageView) loadBlock = { [weak self] in + AssertIsOnMainThread() + if animatedImageView.image != nil { owsFailDebug("Unexpectedly already loaded.") return @@ -240,6 +242,8 @@ public class ConversationMediaView: UIView { cacheKey: cacheKey) } unloadBlock = { + AssertIsOnMainThread() + animatedImageView.image = nil } } @@ -262,6 +266,8 @@ public class ConversationMediaView: UIView { stillImageView.autoPinEdgesToSuperviewEdges() _ = addUploadProgressIfNecessary(stillImageView) loadBlock = { [weak self] in + AssertIsOnMainThread() + if stillImageView.image != nil { owsFailDebug("Unexpectedly already loaded.") return @@ -291,6 +297,8 @@ public class ConversationMediaView: UIView { cacheKey: cacheKey) } unloadBlock = { + AssertIsOnMainThread() + stillImageView.image = nil } } @@ -321,6 +329,8 @@ public class ConversationMediaView: UIView { } loadBlock = { [weak self] in + AssertIsOnMainThread() + if stillImageView.image != nil { owsFailDebug("Unexpectedly already loaded.") return @@ -350,6 +360,8 @@ public class ConversationMediaView: UIView { cacheKey: cacheKey) } unloadBlock = { + AssertIsOnMainThread() + stillImageView.image = nil } } @@ -464,7 +476,7 @@ public class ConversationMediaView: UIView { // views that are no longer visible, redundant loads // of media already being loaded, don't retry media // that can't be loaded, etc.). - private static let loadQueue = DispatchQueue(label: "org.signal.asyncMediaLoadQueue") + private static let loadQueue: DispatchQueue(label: "org.signal.asyncMediaLoadQueue", qos:.userInteractive) @objc public func loadMedia() { From 047afe21ae4ade8afef99995747f5159ad1a0518 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Mon, 10 Dec 2018 12:56:58 -0500 Subject: [PATCH 07/10] Fix typo. --- .../ConversationView/Cells/ConversationMediaView.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Signal/src/ViewControllers/ConversationView/Cells/ConversationMediaView.swift b/Signal/src/ViewControllers/ConversationView/Cells/ConversationMediaView.swift index 6c1ea27bf..3bfc696a7 100644 --- a/Signal/src/ViewControllers/ConversationView/Cells/ConversationMediaView.swift +++ b/Signal/src/ViewControllers/ConversationView/Cells/ConversationMediaView.swift @@ -476,7 +476,7 @@ public class ConversationMediaView: UIView { // views that are no longer visible, redundant loads // of media already being loaded, don't retry media // that can't be loaded, etc.). - private static let loadQueue: DispatchQueue(label: "org.signal.asyncMediaLoadQueue", qos:.userInteractive) + private static let loadQueue = DispatchQueue(label: "org.signal.asyncMediaLoadQueue", qos: .userInteractive) @objc public func loadMedia() { From 962c1acc9f1a0d5f590bc84b753a7e5745043438 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Mon, 10 Dec 2018 14:55:06 -0500 Subject: [PATCH 08/10] Fix "blinking" regression media views. --- .../ConversationView/Cells/ConversationMediaView.swift | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/Signal/src/ViewControllers/ConversationView/Cells/ConversationMediaView.swift b/Signal/src/ViewControllers/ConversationView/Cells/ConversationMediaView.swift index 3bfc696a7..b442fcfaa 100644 --- a/Signal/src/ViewControllers/ConversationView/Cells/ConversationMediaView.swift +++ b/Signal/src/ViewControllers/ConversationView/Cells/ConversationMediaView.swift @@ -435,9 +435,7 @@ public class ConversationMediaView: UIView { let mediaCache = self.mediaCache if let media = mediaCache.object(forKey: cacheKey as NSString) { Logger.verbose("media cache hit") - DispatchQueue.main.async { - loadCompletion(media) - } + loadCompletion(media) return } From 21ab3fbbcb4fcd640f8eb1902915bf0c54447adf Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Wed, 12 Dec 2018 10:06:21 -0500 Subject: [PATCH 09/10] Respond to CR. --- .../ConversationView/Cells/ConversationMediaView.swift | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/Signal/src/ViewControllers/ConversationView/Cells/ConversationMediaView.swift b/Signal/src/ViewControllers/ConversationView/Cells/ConversationMediaView.swift index b442fcfaa..deda1f9dd 100644 --- a/Signal/src/ViewControllers/ConversationView/Cells/ConversationMediaView.swift +++ b/Signal/src/ViewControllers/ConversationView/Cells/ConversationMediaView.swift @@ -214,11 +214,15 @@ public class ConversationMediaView: UIView { loadBlock = { [weak self] in AssertIsOnMainThread() + guard let strongSelf = self else { + return + } + if animatedImageView.image != nil { owsFailDebug("Unexpectedly already loaded.") return } - self?.tryToLoadMedia(loadMediaBlock: { () -> AnyObject? in + strongSelf.tryToLoadMedia(loadMediaBlock: { () -> AnyObject? in guard attachmentStream.isValidImage else { owsFailDebug("Ignoring invalid attachment.") return nil From 9db50bd9e020b34f9330a113571ac9fce1b9beab Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Wed, 12 Dec 2018 11:17:32 -0500 Subject: [PATCH 10/10] Reduce priority of media loads. --- .../ConversationView/Cells/ConversationMediaView.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Signal/src/ViewControllers/ConversationView/Cells/ConversationMediaView.swift b/Signal/src/ViewControllers/ConversationView/Cells/ConversationMediaView.swift index deda1f9dd..3f428b120 100644 --- a/Signal/src/ViewControllers/ConversationView/Cells/ConversationMediaView.swift +++ b/Signal/src/ViewControllers/ConversationView/Cells/ConversationMediaView.swift @@ -478,7 +478,7 @@ public class ConversationMediaView: UIView { // views that are no longer visible, redundant loads // of media already being loaded, don't retry media // that can't be loaded, etc.). - private static let loadQueue = DispatchQueue(label: "org.signal.asyncMediaLoadQueue", qos: .userInteractive) + private static let loadQueue = DispatchQueue(label: "org.signal.asyncMediaLoadQueue") @objc public func loadMedia() {