Rewrite with ScreenCaptureKit - #80
Conversation
| import AVFoundation | ||
| import ScreenCaptureKit | ||
|
|
||
| public final class Aperture { |
There was a problem hiding this comment.
Since this is simply a namespace, make it enum instead.
| public struct RecordingOptions { | ||
| public init( | ||
| destination: URL, | ||
| targetId: String? = nil, |
There was a problem hiding this comment.
| targetId: String? = nil, | |
| targetID: String? = nil, |
to match Swift style. Applies in other places too.
There was a problem hiding this comment.
Thanks! These are great. Took some courses on Swift before I wrote this to get a better understanding, but they don't really cover formatting
Is there something like eslint/prettier for Swift that can help me identify these? (and things like not adding : Bool if there's a default value etc)
Using XCode fwiw, but I can use VSCode too if there's something available there
There was a problem hiding this comment.
swiftlint with this config: https://github.com/sindresorhus/swiftlint-config
| case audioOnly | ||
| } | ||
|
|
||
| public enum ApertureError: Swift.Error { |
There was a problem hiding this comment.
| public enum ApertureError: Swift.Error { | |
| public enum Error: Swift.Error { |
| case noTargetProvided | ||
| case invalidFileExtension(String, Bool) | ||
| case noDisplaysConnected | ||
| case unknownError(Swift.Error) |
There was a problem hiding this comment.
| case unknownError(Swift.Error) | |
| case unknown(Swift.Error) |
And should be last.
| private var isRunning: Bool = false | ||
| /// Whether the recorder is paused | ||
| private var isPaused: Bool = false |
There was a problem hiding this comment.
I feel like these would be better as a single enum. Maybe isStreamRecording should also be included.
There was a problem hiding this comment.
How can I do this with an enum? Do you mean struct to hold each var? They can have independent values at times
There was a problem hiding this comment.
Can it be isRunning == true and isPaused == true at the same time?
| externalDeviceCaptureSession.stopRunning() | ||
| } | ||
|
|
||
| if (assetWriter?.status == .writing) { |
There was a problem hiding this comment.
| if (assetWriter?.status == .writing) { | |
| if assetWriter?.status == .writing { |
| audioDevice: audioDevice, | ||
| videoCodec: videoCodec | ||
| ) | ||
| extension Aperture.Recorder: SCStreamDelegate, SCStreamOutput, AVCaptureAudioDataOutputSampleBufferDelegate, AVCaptureVideoDataOutputSampleBufferDelegate { |
There was a problem hiding this comment.
If possible, one extension per conformance. Makes it easier to read.
| private func initOutput(target: Aperture.Target, options: Aperture.RecordingOptions, streamConfig: SCStreamConfiguration) async throws { | ||
| let assetWriter = try getAssetWriter(target: target, options: options) | ||
|
|
||
| var audioSettings: [String: Any] = [AVSampleRateKey : 48000, AVNumberOfChannelsKey : 2] |
There was a problem hiding this comment.
| var audioSettings: [String: Any] = [AVSampleRateKey : 48000, AVNumberOfChannelsKey : 2] | |
| var audioSettings: [String: Any] = [AVSampleRateKey: 48000, AVNumberOfChannelsKey: 2] |
| case unknownError(Swift.Error) | ||
| case noPermissions | ||
| } | ||
| public final class Recorder: NSObject { |
There was a problem hiding this comment.
Define this in an extension for improved readability.
|
|
||
| /// Internal helpers for when we are resuming, used to fix the buffer timing | ||
| private var isResuming: Bool = false | ||
| private var timeOffset: CMTime = .zero |
There was a problem hiding this comment.
| private var timeOffset: CMTime = .zero | |
| private var timeOffset = CMTime.zero |
|
|
||
| service = IOIteratorNext(iterator) | ||
| extension Aperture { | ||
| public static func hasPermissions() async -> Bool { |
There was a problem hiding this comment.
| public static func hasPermissions() async -> Bool { | |
| public static var hasPermissions: Bool { | |
| get async { |
| return "Unnamed screen" | ||
| extension CMSampleBuffer { | ||
| public func adjustTime(by offset: CMTime) -> CMSampleBuffer? { | ||
| guard CMSampleBufferGetFormatDescription(self) != nil else { return nil } |
There was a problem hiding this comment.
Use self.formatDescription. Same with the other things.
|
@sindresorhus addressed all except for this and configured/ran swiftlint 😌 |
|
While working on
will be pushing a commit with those two at some point tomorrow |
|
Ok, I think it's ready:
|
| if let mode = CGDisplayCopyDisplayMode(self.displayID) { | ||
| return mode.pixelWidth / mode.width | ||
| } | ||
| return 1 |
There was a problem hiding this comment.
| return 1 | |
| return 1 |
|
|
||
| public func fileOutputShouldProvideSampleAccurateRecordingStart(_ output: AVCaptureFileOutput) -> Bool { true } | ||
| if output is AVCaptureVideoDataOutput { | ||
| if assetWriter != nil, !isRunning { |
There was a problem hiding this comment.
| if assetWriter != nil, !isRunning { | |
| if | |
| assetWriter != nil, | |
| !isRunning | |
| { |
| let sampleBuffer = handleBuffer(buffer: sampleBuffer, isVideo: output is AVCaptureVideoDataOutput) | ||
|
|
||
| if output is AVCaptureAudioDataOutput { | ||
| if assetWriter != nil && !isRunning && target == .audioOnly { |
There was a problem hiding this comment.
| if assetWriter != nil && !isRunning && target == .audioOnly { | |
| if | |
| assetWriter != nil, | |
| !isRunning, | |
| target == .audioOnly | |
| { |
Same in many places. Prefer , over && in if/guard statements.
|
|
||
| extension Aperture.Recorder: AVCaptureAudioDataOutputSampleBufferDelegate, AVCaptureVideoDataOutputSampleBufferDelegate { | ||
| public func captureOutput(_ output: AVCaptureOutput, didOutput sampleBuffer: CMSampleBuffer, from connection: AVCaptureConnection) { | ||
| if isPaused { |
There was a problem hiding this comment.
| if isPaused { | |
| guard !isPaused else { |
| if isPaused { | ||
| return | ||
| } | ||
| guard sampleBuffer.isValid else { | ||
| return | ||
| } |
| get { activity != nil } | ||
| set { | ||
| if newValue { | ||
| activity = ProcessInfo.processInfo.beginActivity(options: .idleSystemSleepDisabled, reason: "Recording screen") |
There was a problem hiding this comment.
The activity handling could be simplified with something like this:
final class Activity {
private let activity: NSObjectProtocol
init(
_ options: ProcessInfo.ActivityOptions = [],
reason: String
) {
self.activity = ProcessInfo.processInfo.beginActivity(options: options, reason: reason)
}
deinit {
ProcessInfo.processInfo.endActivity(activity)
}
}There was a problem hiding this comment.
That's the old code, in the new version we just set it once on start, and then stop/remove it in the cleanup. Feels pretty simple, but I could move it to a separate class if you want
There was a problem hiding this comment.
The benefit of the class is that it's self-contained you can just nil the property in the recorder to end the activity.
There was a problem hiding this comment.
Sounds good. Updated.
I think all the comments are addressed, I'm going to start working on the readme updates
| } | ||
| if lastFrame.flags.contains(.valid) { | ||
| if timeOffset.value > 0 { | ||
| pts = CMTimeSubtract(pts, timeOffset) |
There was a problem hiding this comment.
CMTime support + and -
| } | ||
|
|
||
| var lastFrame = CMSampleBufferGetPresentationTimeStamp(resultBuffer) | ||
| let dur = CMSampleBufferGetDuration(resultBuffer) |
There was a problem hiding this comment.
Don't use abbreviations
| if isResuming { | ||
| isResuming = false | ||
|
|
||
| var pts = CMSampleBufferGetPresentationTimeStamp(buffer) |
| resultBuffer = resultBuffer.adjustTime(by: timeOffset) ?? resultBuffer | ||
| } | ||
|
|
||
| var lastFrame = CMSampleBufferGetPresentationTimeStamp(resultBuffer) |
There was a problem hiding this comment.
Use sampleBuffer instance methods, not the legacy global methods. Applies in many places.
|
You don't need to |
|
@sindresorhus Updated 👍 I think it's ok to keep during pause, not sure what would happen if it went to sleep, then resumed after Also updated the readme. We didn't have docs before, added some basic usage and options |
| <p align="center">Record the screen on macOS</p> | ||
| <img src="Media/aperture-logo.svg" width="64" height="64"> | ||
| <h3 align="center">Aperture</h3> | ||
| <p align="center">Record the screen on macOS</p> |
There was a problem hiding this comment.
Incorrect indentation.
| options: Aperture.RecordingOptions( | ||
| destination: URL(fileURLWithPath: "./screen-recording.mp4"), | ||
| targetID: screen.id, | ||
| ) |
There was a problem hiding this comment.
Incorrect indentation.
| Type: `Bool` | ||
|
|
||
| Default: `false` |
There was a problem hiding this comment.
| Type: `Bool` | |
| Default: `false` | |
| Type: `Bool`\ | |
| Default: `false` |
|
Instead of documenting the API in the readme, it would be better to use DocC and add good doc comments to the methods. You just need https://github.com/sindresorhus/Defaults/blob/main/.spi.yml and get it added to https://swiftpackageindex.com/add-a-package |
| @@ -7,32 +7,47 @@ func delay(seconds: TimeInterval, closure: @escaping () -> Void) { | |||
| } | |||
|
|
|||
| @@ -7,32 +7,47 @@ func delay(seconds: TimeInterval, closure: @escaping () -> Void) { | |||
| } | |||
|
|
|||
| let url = URL(fileURLWithPath: "../screen-recording.mp4") | |||
There was a problem hiding this comment.
| let url = URL(fileURLWithPath: "../screen-recording.mp4") | |
| let url = URL(filePath: "../screen-recording.mp4") |
| print("Finished recording:", url.path) | ||
| exit(0) | ||
| } catch let error as Aperture.Error { | ||
| print("Aperture Error: \(error.localizedDescription)") |
There was a problem hiding this comment.
It should exit here too.
| @@ -1,10 +1,10 @@ | |||
| // swift-tools-version:5.5 | |||
| // swift-tools-version:5.7 | |||
There was a problem hiding this comment.
Why such an old Xcode version?
There was a problem hiding this comment.
Didn't know that was an xcode version to be honest. I updated the minimum version to v13 and it said this has to be at least 5.7, so I changed it to fix
I'll update to 6.0.2
There was a problem hiding this comment.
Make it "5.11". 6 will trigger a lot of concurrency things we don't want to deal with yet.
| let finalError: Aperture.Error | ||
| if let error = error as? Aperture.Error { | ||
| finalError = error | ||
| } else { | ||
| finalError = Error.couldNotStartStream(error) | ||
| } |
There was a problem hiding this comment.
| let finalError: Aperture.Error | |
| if let error = error as? Aperture.Error { | |
| finalError = error | |
| } else { | |
| finalError = Error.couldNotStartStream(error) | |
| } | |
| let finalError = if let error = error as? Error { | |
| error | |
| } else { | |
| .couldNotStartStream(error) | |
| } |
| public let applicationName: String? | ||
| public let applicationBundleIdentifier: String? |
There was a problem hiding this comment.
| public let applicationName: String? | |
| public let applicationBundleIdentifier: String? | |
| public let appName: String? | |
| public let appBundleIdentifier: String? |
| /// The error handler for the recording session | ||
| var onError: ((Error) -> Void)? | ||
|
|
||
| func startRecording( |
There was a problem hiding this comment.
| func startRecording( | |
| func start( |
It's inconsistent with pause and the recorder part is already clear from the namespace. recorder.start()
|
|
||
| filter = screenFilter | ||
| case .window: | ||
| /// We need to call this before `SCContentFilter` below otherwise an error is thrown: https://forums.developer.apple.com/forums/thread/743615 |
There was a problem hiding this comment.
Only documentation comments should use ///. Applies in many places.
| throw Error.couldNotAddScreen | ||
| extension Aperture { | ||
| internal final class RecordingSession: NSObject { | ||
| /// The stream object for capturing anything on displays |
There was a problem hiding this comment.
| /// The stream object for capturing anything on displays | |
| // The stream object for capturing anything on displays |
| } else { | ||
| throw Error.couldNotAddScreen | ||
| extension Aperture { | ||
| internal final class RecordingSession: NSObject { |
There was a problem hiding this comment.
| internal final class RecordingSession: NSObject { | |
| final class RecordingSession: NSObject { |
internal is the default.
|
@sindresorhus re: docs, do I just need the I've added those for most properties, but that class is If it's more work than that ^, can we do it in a follow-up? Addressed the rest of the feedback |
Yes, although I prefer You can preview the docs in Xcode. Click "Product" and then "Build Documentation". |
|
|
||
| guard let screen = screens.first else { | ||
| // No screens | ||
| exit(1) |
There was a problem hiding this comment.
Tab indentation, to match the code.
| try await recorder.startRecording( | ||
| target: .screen, | ||
| options: Aperture.RecordingOptions( | ||
| destination: URL(fileURLWithPath: "./screen-recording.mp4"), |
There was a problem hiding this comment.
| destination: URL(fileURLWithPath: "./screen-recording.mp4"), | |
| destination: URL(filePath: "screen-recording.mp4"), |
| exit(1) | ||
| } | ||
|
|
||
| try await recorder.startRecording( |
There was a problem hiding this comment.
| try await recorder.startRecording( | |
| try await recorder.start( |
|
|
||
| try await Task.sleep(for: .seconds(5)) | ||
|
|
||
| try await recorder.stopRecording() |
There was a problem hiding this comment.
| try await recorder.stopRecording() | |
| try await recorder.stop() |
| ```swift | ||
| let screens = try await Aperture.Devices.screen() | ||
| ``` | ||
| <!-- Aperture.Devices.window(excludeDesktopWindows: false, onScreenWindowsOnly: false) --> |
|
@sindresorhus added in-line docs and added a link to the readme for swift package index (I kept the other readme sections as well) I'm not sure if I can submit it to package index until after it's merged to main, but also figured the text might need changes |
sindresorhus
left a comment
sindresorhus
left a comment
There was a problem hiding this comment.
Look good to me. Nice work! 👍
|
@sindresorhus Thanks for all the reviews! Is there anything special about releasing? Or just cut a release tag using the GH UI? Edit: Released https://github.com/wulkano/Aperture/releases/tag/v3.0.0 🎉 |
2 participants
Rewrites Aperture to use ScreenCaptureKit
Breaking changes:
highlightClicksis only available on macOS 15+Aperture.Recorder()and usestartRecording()andstopRecording()on itstartRecordingawaits until the file actually starts getting written before resolving, or throws any errors that happen during that timecropRectis top/left now (this is howScreenCaptureKitworks by default)This has as much feature parity as possible (other then the
highlightClicksnote above) but it adds some functionality as well:.movand.m4vfor videos.m4afor now, AAC or ALAC encoding)Potential features we'd want to add in a follow-up:
This PR still needs to update the readme, but wanted to get it reviewed first to fully land on the implementation before I go and update the readme
Closes #16
Closes #31
Closes #71
Closes #63
Closes #65
IssueHunt Summary
Referenced issues
This pull request has been submitted to: