Skip to content

Commit 70cd5ee

Browse files
UI modernization PR 8: extract CaptureEngine from CameraViewController (#129)
* Extract CaptureEngine from CameraViewController (UI modernization PR 8) Mechanical extraction, no behavior or threading changes. CaptureEngine (non-UI, NSObject, the AVCapturePhotoCaptureDelegate) now owns the capture session, device/lens/zoom discovery, capabilities gathering, flash/torch intent, quality/format/aspect configuration, and still capture + crop math. CameraViewController (~1,670 → ~960 lines) keeps the preview layer, overlays, permissions, lifecycle, and the recording/sample-buffer pipeline (queued for a later PR), reaching the session through engine-exposed properties. Engine-to-UI seams: onPicture (relayed to the actor system by the VC), onStatusChanged, and rotateOutputs(orientation:) — the VC still rotates the preview-layer connection, preserving the original method's exact shape. CropRectTests re-point to CaptureEngine.cropRect (assertions unchanged); adds 4 CaptureEngineTests for session-free logic. 376/376 tests pass (372 existing + 4 new), verified in two independent full runs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Add back-to-back camera-control round-trip tests Five more loopback happy paths now that the fake camera sits behind the CameraControlling seam: - toggle flash: the flipped mode value crosses back in the response - toggle camera: response carries fresh non-nil capabilities - set zoom: factor and zoom range echo through the FlatBuffers round trip - switch lens: telephoto request in, confirmation out - video recording: full 3-step stop protocol (StopRecordingVideo -> Ack -> Resp) across both state machines, camera back to .camera, monitor back to .monitor LoopbackFakeCamera completes a stopped recording by injecting StopRecordingVideoResp, the way the real pipeline's completion does. Also documents (Docs/UI_MODERNIZATION.md) a latent bug found while wiring the video test: the success StartRecordingVideoAck carrying recordingStartTime is dropped on the phone path (no state forwards it to the peer); only the error-ack path reaches the monitor. Left for its own fix PR with a failing test. 381/381 tests pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Fix dropped StartRecordingVideoAck on the phone path The recording pipeline sends the success StartRecordingVideoAck (carrying recordingStartTime for the monitor's timer sync) to the local session actor, but cameraShootingVideo had no case for it — the ack died in Theater's unhandled-message log and the monitor's recording timer never synced the actual start time. The watch path handles the ack explicitly, which masked the phone-path gap. Fix: forward the ack to the peer from cameraShootingVideo. Caught by the new loopback video-protocol test (asserted red before the fix, green after). 381/381 tests pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Harden onPicture delivery and fix a doc claim Capture the session ActorRef directly in the engine's onPicture closure instead of going through weak self, so an in-flight capture still reaches the actor if the VC deallocates before the photo delegate fires (master had this guarantee implicitly via the VC being the retained delegate). Also correct the PR 8 worklog line: CaptureEngine does import UIKit — orientation enums plus UIImage for still cropping. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 parent ebf633b commit 70cd5ee

9 files changed

Lines changed: 1216 additions & 833 deletions

File tree

Docs/UI_MODERNIZATION.md

Lines changed: 40 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -124,7 +124,7 @@ prior migration's own comment claimed were "removed"), the deprecated
124124
`UICmd.BecomeCamera` into the camera states — that's exactly what PR 7's
125125
`CaptureEngine` replaces.
126126

127-
### PR 7: CameraControlling protocol seam — **this PR**
127+
### PR 7: CameraControlling protocol seam — **shipped (#128)**
128128
The watch-hardening work had already built the seam (`WatchCameraControlling`,
129129
fake-backed); this PR generalizes it. Renamed `CameraControlling`, moved to its
130130
own file, extended with the nine members the phone camera states need
@@ -140,14 +140,17 @@ the loopback harness run the full two-device photo capture (become camera →
140140
become monitor → TakePic → OnPicture → Ack + Resp with bytes → both sides back
141141
to steady state) in CI for the first time.
142142

143-
### PR 8: Extract CaptureEngine from CameraViewController
144-
Move the ~1,100 engine lines (session setup, lens/zoom/capabilities, photo capture +
145-
crop math, AVAssetWriter recording, sample-buffer streaming) into a non-UI class with
146-
one owned serial queue — today session config runs on the actor thread, start/stop on
147-
`cameraConfigQueue`, and setup on main, unsynchronized. This is also where
148-
`AVCaptureVideoOrientation` (deprecated iOS 17) migrates to
149-
`AVCaptureDevice.RotationCoordinator`, and `isHighResolutionPhotoEnabled`
150-
(deprecated iOS 16) to `maxPhotoDimensions`.
143+
### PR 8: Extract CaptureEngine (configuration + stills) — **this PR**
144+
Mechanical extraction, no behavior/threading changes: `CaptureEngine` (non-UI,
145+
`NSObject`, the photo-capture delegate) now owns the capture session, device/lens/zoom
146+
discovery, capabilities, flash/torch intent, quality/format/aspect config, and still
147+
capture + crop. The VC keeps the preview layer, overlays, lifecycle, and the whole
148+
recording/sample-buffer pipeline (next PR), reaching the session via engine-exposed
149+
properties. Engine↔UI seams: `onPicture` (actor relay), `onStatusChanged`,
150+
`rotateOutputs(orientation:)` (VC still rotates the preview connection).
151+
Still queued for follow-ups: single owned serial queue (threading fix),
152+
`RotationCoordinator` (iOS 17) and `maxPhotoDimensions` (iOS 16) migrations,
153+
recording pipeline extraction.
151154

152155
### PR 9+: Camera SwiftUI chrome
153156
With the engine out, the VC is ~400 lines of real UI: SwiftUI chrome around a
@@ -252,3 +255,31 @@ With the engine out, the VC is ~400 lines of real UI: SwiftUI chrome around a
252255
bytes, both state machines back to steady state — all in-process, in CI.
253256
Until now that flow had only ever been verified with two phones on a desk.
254257
- 372/372 green.
258+
259+
### PR 8 — CaptureEngine extraction
260+
- Delegated the mechanical move to a fresh-context agent with an exact boundary
261+
spec and one objective gate: the full suite green. It came back 376/376
262+
(4 new engine tests) with an honest deviations list — the kind of report you
263+
want from a contractor.
264+
- CameraViewController: ~1,670 → ~960 lines. CaptureEngine: 866 lines of capture
265+
logic with no view code — its only UIKit dependencies are orientation enums and
266+
`UIImage` for still cropping.
267+
- The one redundancy knowingly introduced: after toggleCamera the preview
268+
rotation re-runs the (idempotent) output rotation. Faithful beats clever in a
269+
mechanical-move PR.
270+
- Orientation now lives in two places (VC for preview/UI, engine cache for output
271+
connections) — flagged as the thing the PR 9 RotationCoordinator migration
272+
should collapse.
273+
- Coverage follow-up on the same PR: 5 more back-to-back loopback round trips
274+
(flash mode value, toggle-camera capabilities payload, zoom factor + range echo,
275+
lens switch, and the full 3-step video stop protocol) — all green first run.
276+
381 total tests, up from 348 when this series started.
277+
- Bug found while wiring the video test, then fixed in the same PR (failing test
278+
first — red run captured): the success `StartRecordingVideoAck` carrying
279+
`recordingStartTime` for the monitor's timer sync was sent to the camera's
280+
local session actor but never forwarded to the peer — Theater logged "message
281+
not handled" and the monitor's recording timer never synced the real start
282+
time on the phone path. The watch path handled the ack explicitly, which is
283+
why nobody noticed. Fix: one forwarding case in `cameraShootingVideo`.
284+
The loopback harness caught in an afternoon what shipped unnoticed for years —
285+
because nobody ever watched both phones' timers at once.

RemoteCam/CameraControlling.swift

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -57,11 +57,11 @@ protocol CameraControlling: AnyObject {
5757

5858
extension CameraViewController: CameraControlling {
5959
var isTorchActive: Bool {
60-
videoDeviceInput?.device.isTorchActive ?? false
60+
engine.videoDeviceInput?.device.isTorchActive ?? false
6161
}
6262

6363
var currentFlashMode: AVCaptureDevice.FlashMode {
64-
cameraSettings.flashMode
64+
engine.cameraSettings.flashMode
6565
}
6666

6767
func updateTimerCountdown(value: Int) {

RemoteCam/CameraVideoStates.swift

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,12 @@ extension RemoteCamSession {
4646
}
4747
self.sendCommandOrGoToScanning(peer: [peer], msg: resp)
4848

49+
case let ack as RemoteCmd.StartRecordingVideoAck:
50+
// Sent by the recording pipeline once the writer produces its
51+
// first frames; forward to the monitor so its recording timer
52+
// syncs to the actual start time.
53+
self.sendCommandOrGoToScanning(peer: [peer], msg: ack, mode: .reliable)
54+
4955
case let stop as RemoteCmd.StopRecordingVideo:
5056
ctrl.stopRecordingVideo(stop.sendMediaToPeer)
5157
let ack = RemoteCmd.StopRecordingVideoAck()

0 commit comments

Comments
 (0)