Fix Gifski not finishing if the last frame could not be generated

Fixes #230
sindresorhus · Feb 9, 2021 · 290721d · 290721d
1 parent b7c8a4e
commit 290721d
Show file tree

Hide file tree

Showing 2 changed files with 56 additions and 14 deletions.
diff --git a/Gifski/Gifski.swift b/Gifski/Gifski.swift
@@ -192,6 +192,8 @@ final class Gifski {
 			var fps = (conversion.frameRate.map { Double($0) } ?? assetFrameRate).clamped(to: 0.1...Constants.allowedFrameRate.upperBound)
 			fps = min(fps, assetFrameRate)
 
+			print("FPS:", fps)
+
 			// `.zero` tolerance is much slower and fails a lot on macOS 11. (macOS 11.1)
 			if #available(macOS 11, *) {
 				let tolerance = CMTime(seconds: 0.5 / fps, preferredTimescale: .video)
@@ -212,6 +214,8 @@ final class Gifski {
 				return
 			}
 
+			print("Frame count:", frameCount)
+
 			self.progress.totalUnitCount = Int64(frameCount)
 
 			var frameForTimes = [CMTime]()
@@ -263,6 +267,15 @@ final class Gifski {
 					return
 				}
 
+				func finish() {
+					do {
+						try gifski.finish()
+						completionHandlerOnce(.success(self.gifData as Data))
+					} catch {
+						completionHandlerOnce(.failure(.writeFailed(error)))
+					}
+				}
+
 				guard !self.progress.isCancelled else {
 					completionHandlerOnce(.failure(.cancelled))
 					return
@@ -271,6 +284,13 @@ final class Gifski {
 				switch result {
 				case .success(let result):
 					self.progress.totalUnitCount = Int64(result.totalCount)
+
+					// This happens if the last frame in the video failed to be generated.
+					if result.isFinishedIgnoreImage {
+						finish()
+						return
+					}
+
 					let image = result.image
 
 					guard let bytePointer = image.bytePointer else {
@@ -295,12 +315,7 @@ final class Gifski {
 					}
 
 					if result.isFinished {
-						do {
-							try gifski.finish()
-							completionHandlerOnce(.success(self.gifData as Data))
-						} catch {
-							completionHandlerOnce(.failure(.writeFailed(error)))
-						}
+						finish()
 					}
 				case .failure where result.isCancelled:
 					completionHandlerOnce(.failure(.cancelled))

diff --git a/Gifski/Utilities.swift b/Gifski/Utilities.swift
@@ -330,6 +330,7 @@ extension AVAssetImageGenerator {
 		let completedCount: Int
 		let totalCount: Int
 		let isFinished: Bool
+		let isFinishedIgnoreImage: Bool
 	}
 
 	/// - Note: If you use `result.completedCount`, don't forget to update its usage in each `completionHandler` call as it can change if frames are skipped, for example, blank frames.
@@ -343,12 +344,6 @@ extension AVAssetImageGenerator {
 		var decodeFailureFrameCount = 0
 
 		generateCGImagesAsynchronously(forTimes: times) { requestedTime, image, actualTime, result, error in
-			let successFrameCount = totalCount - decodeFailureFrameCount
-			if successFrameCount <= 2 {
-				completionHandler(.failure(NSError.appError("\(decodeFailureFrameCount) of \(totalCount) frames failed to decode. This is a bug in macOS. We are looking into workarounds.")))
-				return
-			}
-
 			switch result {
 			case .succeeded:
 				completedCount += 1
@@ -361,25 +356,51 @@ extension AVAssetImageGenerator {
 							actualTime: actualTime,
 							completedCount: completedCount,
 							totalCount: totalCount,
-							isFinished: completedCount == totalCount
+							isFinished: completedCount == totalCount,
+							isFinishedIgnoreImage: false
 						)
 					)
 				)
 			case .failed:
 				// TODO: Ideally, we should trim blank frames when initially reading the video in `VideoValidator.swift`, but I don't know a way to detect blank frames. We should still keep this fix even if we find a way to trim as this handles blank frames in the middle of the video.
 				// TODO: Report the `xcrun` bug to Apple if it's still an issue in macOS 11.
 				if let error = error as? AVError {
+					// Ugly workaround for when the last frame is a failure.
+					func finishWithoutImageIfNeeded() {
+						guard completedCount == totalCount else {
+							return
+						}
+
+						completionHandler(
+							.success(
+								CompletionHandlerResult(
+									image: .empty,
+									requestedTime: requestedTime,
+									actualTime: actualTime,
+									completedCount: completedCount,
+									totalCount: totalCount,
+									isFinished: true,
+									isFinishedIgnoreImage: true
+								)
+							)
+						)
+					}
+
 					// We ignore blank frames. A video can sometimes contain blank frames at the start when you record an iOS simulator using `xcrun simctl io booted recordVideo simulator.mp4`.
 					if error.code == .noImageAtTime {
 						totalCount -= 1
+						print("No image at time. Completed: \(completedCount) Total: \(totalCount)")
+						finishWithoutImageIfNeeded()
 						break
 					}
 
-					// macOS 11 (still an issue in macOS 11.2) started throwing “decode failed” error for some frames in screen recordings. As a workaround, we ignore these.
+					// macOS 11 (still an issue in macOS 11.2) started throwing “decode failed” error for some frames in screen recordings. As a workaround, we ignore these as the GIF seems fine still.
 					if error.code == .decodeFailed {
 						decodeFailureFrameCount += 1
 						totalCount -= 1
+						print("Decode failure. Completed: \(completedCount) Total: \(totalCount)")
 						Crashlytics.recordNonFatalError(error: error, userInfo: ["requestedTime": requestedTime.seconds])
+						finishWithoutImageIfNeeded()
 						break
 					}
 				}
@@ -3657,3 +3678,9 @@ extension NSExtensionContext {
 		inputItemsTyped.compactMap(\.attachments).flatten()
 	}
 }
+
+
+extension CGImage {
+	static let empty = NSImage(size: CGSize(widthHeight: 1), flipped: false) { _ in true }
+		.cgImage(forProposedRect: nil, context: nil, hints: nil)!
+}