refactor(apple): code-quality pass — audit fixes + centralized defaults keys
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) <noreply@anthropic.com>
This commit is contained in:
@@ -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"
|
||||
}
|
||||
@@ -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.
|
||||
|
||||
@@ -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] = []
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user