From 9e8135ccec608a74fa3a1335a3879f80990f6708 Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Fri, 12 Jun 2026 16:30:08 +0200 Subject: [PATCH] =?UTF-8?q?refactor(apple):=20code-quality=20pass=20?= =?UTF-8?q?=E2=80=94=20audit=20fixes=20+=20centralized=20defaults=20keys?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A 6-agent adversarial audit of the client (11 confirmed of 39 findings, the rest filtered) drove these: - fix: SessionAudio ring buffer — guard a write larger than the ring (would push readIdx past writeIdx and corrupt the buffer; never happens, but guard not corrupt). - fix: CADisplayLink retain cycle (stage-2 presenter) — a weak-target DisplayLinkProxy so the view can deallocate (the link retains its target); stage-2 teardown added to both StreamView/StreamViewController deinits as a safety net. - fix: GamepadFeedback deinit { flag.stop() } — the drain thread holds the connection strongly and self weakly, so an abrupt teardown without stop() would leak it. - refactor: centralize the 12 UserDefaults/@AppStorage key literals (scattered across 8 files) into one DefaultsKey enum — a typo silently splits a setting's reader from its writer. - docs: RumbleRenderer @unchecked Sendable invariant; the HID digit-row table; the stage-2 layer compositing. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../Sources/PunktfunkClient/HostStore.swift | 3 ++- .../PunktfunkClient/SessionModel.swift | 6 ++--- .../PunktfunkClient/SettingsView.swift | 20 ++++++++-------- .../PunktfunkClient/SpeedTestSheet.swift | 8 +++---- .../Sources/PunktfunkKit/DefaultsKeys.swift | 23 +++++++++++++++++++ .../PunktfunkKit/GamepadFeedback.swift | 8 +++++++ .../Sources/PunktfunkKit/GamepadManager.swift | 4 ++-- .../Sources/PunktfunkKit/InputCapture.swift | 3 ++- .../Sources/PunktfunkKit/SessionAudio.swift | 4 ++++ .../Sources/PunktfunkKit/Stage2Pipeline.swift | 10 ++++++++ .../Sources/PunktfunkKit/StreamView.swift | 14 +++++++---- .../Sources/PunktfunkKit/StreamViewIOS.swift | 11 ++++++--- 12 files changed, 86 insertions(+), 28 deletions(-) create mode 100644 clients/apple/Sources/PunktfunkKit/DefaultsKeys.swift diff --git a/clients/apple/Sources/PunktfunkClient/HostStore.swift b/clients/apple/Sources/PunktfunkClient/HostStore.swift index a60319c..1ee9aa8 100644 --- a/clients/apple/Sources/PunktfunkClient/HostStore.swift +++ b/clients/apple/Sources/PunktfunkClient/HostStore.swift @@ -9,6 +9,7 @@ // --require-pairing only admit paired clients, so for them pairing is the only way in. import Foundation +import PunktfunkKit import SwiftUI struct StoredHost: Identifiable, Codable, Hashable { @@ -26,7 +27,7 @@ struct StoredHost: Identifiable, Codable, Hashable { @MainActor final class HostStore: ObservableObject { - private static let key = "punktfunk.hosts" + private static let key = DefaultsKey.hosts @Published var hosts: [StoredHost] { didSet { persist() } diff --git a/clients/apple/Sources/PunktfunkClient/SessionModel.swift b/clients/apple/Sources/PunktfunkClient/SessionModel.swift index a976e8e..9768859 100644 --- a/clients/apple/Sources/PunktfunkClient/SessionModel.swift +++ b/clients/apple/Sources/PunktfunkClient/SessionModel.swift @@ -207,9 +207,9 @@ final class SessionModel: ObservableObject { let defaults = UserDefaults.standard let audio = SessionAudio(connection: conn) audio.start( - speakerUID: defaults.string(forKey: "punktfunk.speakerUID") ?? "", - micUID: defaults.string(forKey: "punktfunk.micUID") ?? "", - micEnabled: defaults.object(forKey: "punktfunk.micEnabled") as? Bool ?? true) + speakerUID: defaults.string(forKey: DefaultsKey.speakerUID) ?? "", + micUID: defaults.string(forKey: DefaultsKey.micUID) ?? "", + micEnabled: defaults.object(forKey: DefaultsKey.micEnabled) as? Bool ?? true) self.audio = audio // Gamepads: forward GamepadManager's active controller as pad 0 and render the // host's feedback (rumble always; lightbar/player-LEDs/adaptive-triggers when the diff --git a/clients/apple/Sources/PunktfunkClient/SettingsView.swift b/clients/apple/Sources/PunktfunkClient/SettingsView.swift index 2343816..1da94c7 100644 --- a/clients/apple/Sources/PunktfunkClient/SettingsView.swift +++ b/clients/apple/Sources/PunktfunkClient/SettingsView.swift @@ -11,18 +11,18 @@ import SwiftUI @MainActor struct SettingsView: View { @Environment(\.dismiss) private var dismiss - @AppStorage("punktfunk.width") private var width = 1920 - @AppStorage("punktfunk.height") private var height = 1080 - @AppStorage("punktfunk.hz") private var hz = 60 - @AppStorage("punktfunk.compositor") private var compositor = 0 - @AppStorage("punktfunk.gamepadType") private var gamepadType = 0 - @AppStorage("punktfunk.bitrateKbps") private var bitrateKbps = 0 - @AppStorage("punktfunk.presenter") private var presenter = "stage1" - @AppStorage("punktfunk.micEnabled") private var micEnabled = true + @AppStorage(DefaultsKey.streamWidth) private var width = 1920 + @AppStorage(DefaultsKey.streamHeight) private var height = 1080 + @AppStorage(DefaultsKey.streamHz) private var hz = 60 + @AppStorage(DefaultsKey.compositor) private var compositor = 0 + @AppStorage(DefaultsKey.gamepadType) private var gamepadType = 0 + @AppStorage(DefaultsKey.bitrateKbps) private var bitrateKbps = 0 + @AppStorage(DefaultsKey.presenter) private var presenter = "stage1" + @AppStorage(DefaultsKey.micEnabled) private var micEnabled = true @ObservedObject private var gamepads = GamepadManager.shared #if os(macOS) - @AppStorage("punktfunk.speakerUID") private var speakerUID = "" - @AppStorage("punktfunk.micUID") private var micUID = "" + @AppStorage(DefaultsKey.speakerUID) private var speakerUID = "" + @AppStorage(DefaultsKey.micUID) private var micUID = "" @State private var outputDevices: [AudioDevice] = [] @State private var inputDevices: [AudioDevice] = [] #endif diff --git a/clients/apple/Sources/PunktfunkClient/SpeedTestSheet.swift b/clients/apple/Sources/PunktfunkClient/SpeedTestSheet.swift index 9d952ec..5ccd59a 100644 --- a/clients/apple/Sources/PunktfunkClient/SpeedTestSheet.swift +++ b/clients/apple/Sources/PunktfunkClient/SpeedTestSheet.swift @@ -32,10 +32,10 @@ struct SpeedTestSheet: View { @Environment(\.dismiss) private var dismiss let host: StoredHost - @AppStorage("punktfunk.width") private var width = 1920 - @AppStorage("punktfunk.height") private var height = 1080 - @AppStorage("punktfunk.hz") private var hz = 60 - @AppStorage("punktfunk.bitrateKbps") private var bitrateKbps = 0 + @AppStorage(DefaultsKey.streamWidth) private var width = 1920 + @AppStorage(DefaultsKey.streamHeight) private var height = 1080 + @AppStorage(DefaultsKey.streamHz) private var hz = 60 + @AppStorage(DefaultsKey.bitrateKbps) private var bitrateKbps = 0 private enum Phase: Equatable { case connecting diff --git a/clients/apple/Sources/PunktfunkKit/DefaultsKeys.swift b/clients/apple/Sources/PunktfunkKit/DefaultsKeys.swift new file mode 100644 index 0000000..1fdb039 --- /dev/null +++ b/clients/apple/Sources/PunktfunkKit/DefaultsKeys.swift @@ -0,0 +1,23 @@ +// One source of truth for the client's UserDefaults / @AppStorage keys. A magic-string key +// duplicated across a setting's writer (a Settings @AppStorage) and reader (e.g. a stream view +// reading UserDefaults) splits silently on a typo — the setting just stops taking effect. These +// live in PunktfunkKit because both the app and the kit's views read them. + +import Foundation + +/// Persisted-setting keys. The string VALUES are stable on disk — rename the symbol freely, but +/// never the string (it would orphan everyone's saved value). +public enum DefaultsKey { + public static let streamWidth = "punktfunk.width" + public static let streamHeight = "punktfunk.height" + public static let streamHz = "punktfunk.hz" + public static let compositor = "punktfunk.compositor" + public static let gamepadType = "punktfunk.gamepadType" + public static let gamepadID = "punktfunk.gamepadID" + public static let bitrateKbps = "punktfunk.bitrateKbps" + public static let micEnabled = "punktfunk.micEnabled" + public static let speakerUID = "punktfunk.speakerUID" + public static let micUID = "punktfunk.micUID" + public static let presenter = "punktfunk.presenter" + public static let hosts = "punktfunk.hosts" +} diff --git a/clients/apple/Sources/PunktfunkKit/GamepadFeedback.swift b/clients/apple/Sources/PunktfunkKit/GamepadFeedback.swift index b0a22a9..dae9e4f 100644 --- a/clients/apple/Sources/PunktfunkKit/GamepadFeedback.swift +++ b/clients/apple/Sources/PunktfunkKit/GamepadFeedback.swift @@ -43,6 +43,9 @@ private final class FeedbackStopFlag: @unchecked Sendable { /// amplitude and torn down on retarget; players run only while their motor is on, so an /// idle controller costs no radio traffic. Failures (pads without haptics, engine resets) /// downgrade to silence — rumble is best-effort by design. +/// +/// `@unchecked Sendable` is sound because every property (`controller`/`low`/`high`/`broken`) is +/// read and written only inside `queue` closures — the serial queue is the synchronization. private final class RumbleRenderer: @unchecked Sendable { private let queue = DispatchQueue(label: "io.unom.punktfunk.haptics", qos: .userInteractive) @@ -177,6 +180,11 @@ public final class GamepadFeedback { } } + /// Safety net: the drain thread captures `connection` strongly and only `self` weakly, so if + /// this is dropped without `stop()` (an abrupt teardown) the thread would poll forever and + /// leak the connection — signal it to exit. (`stop()` is the normal path and also joins it.) + deinit { flag.stop() } + /// Map the DualSense player-LED bit patterns (5 LEDs, hid-playstation's player /// conventions) onto GCControllerPlayerIndex. Unknown patterns fall back to the lit /// count, clamped to the four indices GC offers. diff --git a/clients/apple/Sources/PunktfunkKit/GamepadManager.swift b/clients/apple/Sources/PunktfunkKit/GamepadManager.swift index b099fc3..a7957a4 100644 --- a/clients/apple/Sources/PunktfunkKit/GamepadManager.swift +++ b/clients/apple/Sources/PunktfunkKit/GamepadManager.swift @@ -4,7 +4,7 @@ // follow `active` — exactly ONE physical controller is forwarded to the host, as pad 0. // // Selection: the user can pin a controller in Settings (persisted under -// "punktfunk.gamepadID"); with no pin — or the pinned one absent — the most recently +// DefaultsKey.gamepadID); with no pin — or the pinned one absent — the most recently // connected extended gamepad wins. GCController has no stable hardware serial, so the pin // is a fingerprint of vendorName|productCategory (+ a connect-order suffix for twins); // identical twin controllers may swap a pin across reconnects, which the Settings footer @@ -61,7 +61,7 @@ public final class GamepadManager: ObservableObject { } } - private static let preferredKey = "punktfunk.gamepadID" + private static let preferredKey = DefaultsKey.gamepadID /// Connect order (identity-keyed) — drives both twin de-dup suffixes and auto-pick. private var connectOrder: [ObjectIdentifier] = [] private var observers: [NSObjectProtocol] = [] diff --git a/clients/apple/Sources/PunktfunkKit/InputCapture.swift b/clients/apple/Sources/PunktfunkKit/InputCapture.swift index e484a58..2d50a41 100644 --- a/clients/apple/Sources/PunktfunkKit/InputCapture.swift +++ b/clients/apple/Sources/PunktfunkKit/InputCapture.swift @@ -557,7 +557,8 @@ public final class InputCapture { var m: [Int: UInt32] = [:] // a–z: HID 0x04..0x1D → VK 'A'..'Z'. for i in 0..<26 { m[0x04 + i] = UInt32(0x41 + i) } - // 1–9, 0: HID 0x1E..0x27 → VK '1'..'9','0'. + // 1–9: HID 0x1E..0x26 → VK '1'..'9'; then 0: HID 0x27 → VK '0' (set separately — + // the '0' key sits AFTER '9' in HID but its VK 0x30 sits BEFORE '1' (0x31)). for i in 0..<9 { m[0x1E + i] = UInt32(0x31 + i) } m[0x27] = 0x30 m[0x28] = 0x0D // return diff --git a/clients/apple/Sources/PunktfunkKit/SessionAudio.swift b/clients/apple/Sources/PunktfunkKit/SessionAudio.swift index 0634dcc..ad254d4 100644 --- a/clients/apple/Sources/PunktfunkKit/SessionAudio.swift +++ b/clients/apple/Sources/PunktfunkKit/SessionAudio.swift @@ -47,6 +47,10 @@ final class AudioRing: @unchecked Sendable { lock.lock() defer { lock.unlock() } let capacity = buf.count + // A single write larger than the whole ring would push readIdx PAST writeIdx below + // (inverting the valid range — corruption). It never happens (one decoded packet is far + // under capacity), but guard rather than corrupt. + guard count <= capacity else { return } if writeIdx + count - readIdx > capacity { readIdx = writeIdx + count - capacity // overflow: drop oldest } diff --git a/clients/apple/Sources/PunktfunkKit/Stage2Pipeline.swift b/clients/apple/Sources/PunktfunkKit/Stage2Pipeline.swift index 17a9476..6c8bc2f 100644 --- a/clients/apple/Sources/PunktfunkKit/Stage2Pipeline.swift +++ b/clients/apple/Sources/PunktfunkKit/Stage2Pipeline.swift @@ -12,6 +12,16 @@ import AVFoundation import Foundation import QuartzCore +/// Weak-target wrapper for CADisplayLink. The link retains its target, so targeting a view +/// directly makes a `view → link → view` cycle that only `invalidate()` breaks — if a teardown +/// is ever missed the view leaks and keeps ticking. This proxy holds the handler weakly, so the +/// view can deallocate and its `deinit` invalidate the link. +public final class DisplayLinkProxy: NSObject { + private let onTick: (CADisplayLink) -> Void + public init(_ onTick: @escaping (CADisplayLink) -> Void) { self.onTick = onTick } + @objc public func tick(_ link: CADisplayLink) { onTick(link) } +} + /// Newest-ready 1-slot ring: the decoder overwrites (drops the older undisplayed frame — lowest /// latency, no smoothing buffer), the display link takes-and-clears. Sendable; lock-guarded. private final class ReadyRing: @unchecked Sendable { diff --git a/clients/apple/Sources/PunktfunkKit/StreamView.swift b/clients/apple/Sources/PunktfunkKit/StreamView.swift index 27b5236..91bac85 100644 --- a/clients/apple/Sources/PunktfunkKit/StreamView.swift +++ b/clients/apple/Sources/PunktfunkKit/StreamView.swift @@ -434,7 +434,7 @@ public final class StreamLayerView: NSView { // Presenter choice — default stage-1 (the known-good AVSampleBufferDisplayLayer). Stage-2 // (`punktfunk.presenter == "stage2"`) takes explicit VTDecompressionSession decode + a // CAMetalLayer/display-link present; it falls back here if Metal can't be set up. - if UserDefaults.standard.string(forKey: "punktfunk.presenter") == "stage2", + if UserDefaults.standard.string(forKey: DefaultsKey.presenter) == "stage2", let meter = presentMeter, let pipeline = Stage2Pipeline(presentMeter: meter) { startStage2(pipeline, connection: connection, onFrame: onFrame, onSessionEnd: onSessionEnd) @@ -455,17 +455,22 @@ public final class StreamLayerView: NSView { onFrame: (@Sendable (AccessUnit) -> Void)?, onSessionEnd: (@Sendable () -> Void)? ) { let metal = pipeline.layer - displayLayer.addSublayer(metal) // contentsScale + frame set in layoutMetalLayer() + // The opaque metal layer composites OVER the AVSampleBufferDisplayLayer base, which sits + // idle (un-enqueued) in stage-2. contentsScale + frame are set in layoutMetalLayer(). + displayLayer.addSublayer(metal) metalLayer = metal stage2 = pipeline layoutMetalLayer() - let link = displayLink(target: self, selector: #selector(stage2Tick(_:))) + // Weak-proxy target so the link doesn't form a retain cycle with the view (see + // DisplayLinkProxy) — the link retains the proxy; the proxy holds the view weakly. + let proxy = DisplayLinkProxy { [weak self] link in self?.stage2Tick(link) } + let link = displayLink(target: proxy, selector: #selector(DisplayLinkProxy.tick(_:))) link.add(to: .main, forMode: .common) stage2Link = link pipeline.start(connection: connection, onFrame: onFrame, onSessionEnd: onSessionEnd) } - @objc private func stage2Tick(_ link: CADisplayLink) { + private func stage2Tick(_ link: CADisplayLink) { stage2?.renderTick( targetPresentNs: Stage2Pipeline.realtimeNs(forDisplayLinkTimestamp: link.targetTimestamp)) } @@ -523,6 +528,7 @@ public final class StreamLayerView: NSView { appObservers.forEach(NotificationCenter.default.removeObserver(_:)) windowObservers.forEach(NotificationCenter.default.removeObserver(_:)) pump?.stop() + teardownStage2() // invalidate the display link + stop the pipeline if stop() was missed } } #endif diff --git a/clients/apple/Sources/PunktfunkKit/StreamViewIOS.swift b/clients/apple/Sources/PunktfunkKit/StreamViewIOS.swift index caa4003..9268376 100644 --- a/clients/apple/Sources/PunktfunkKit/StreamViewIOS.swift +++ b/clients/apple/Sources/PunktfunkKit/StreamViewIOS.swift @@ -218,7 +218,7 @@ public final class StreamViewController: UIViewController { // Presenter choice — default stage-1 (the known-good AVSampleBufferDisplayLayer). Stage-2 // (`punktfunk.presenter == "stage2"`) takes VTDecompressionSession decode + a // CAMetalLayer/display-link present; falls back here if Metal can't be set up. - if UserDefaults.standard.string(forKey: "punktfunk.presenter") == "stage2", + if UserDefaults.standard.string(forKey: DefaultsKey.presenter) == "stage2", let meter = presentMeter, let pipeline = Stage2Pipeline(presentMeter: meter) { startStage2(pipeline, connection: connection, onFrame: onFrame, onSessionEnd: onSessionEnd) @@ -294,17 +294,21 @@ public final class StreamViewController: UIViewController { ) { let metal = pipeline.layer metal.contentsScale = streamView.contentScaleFactor + // Composites OVER the idle (un-enqueued in stage-2) AVSampleBufferDisplayLayer base. streamView.layer.addSublayer(metal) metalLayer = metal stage2 = pipeline layoutMetalLayer() - let link = CADisplayLink(target: self, selector: #selector(stage2Tick(_:))) + // Weak-proxy target so the link doesn't retain-cycle with the controller (see + // DisplayLinkProxy) — the link retains the proxy; the proxy holds self weakly. + let proxy = DisplayLinkProxy { [weak self] link in self?.stage2Tick(link) } + let link = CADisplayLink(target: proxy, selector: #selector(DisplayLinkProxy.tick(_:))) link.add(to: .main, forMode: .common) stage2Link = link pipeline.start(connection: connection, onFrame: onFrame, onSessionEnd: onSessionEnd) } - @objc private func stage2Tick(_ link: CADisplayLink) { + private func stage2Tick(_ link: CADisplayLink) { stage2?.renderTick( targetPresentNs: Stage2Pipeline.realtimeNs(forDisplayLinkTimestamp: link.targetTimestamp)) } @@ -394,6 +398,7 @@ public final class StreamViewController: UIViewController { deinit { observers.forEach(NotificationCenter.default.removeObserver(_:)) pump?.stop() + teardownStage2() // invalidate the display link + stop the pipeline if stop() was missed } }