fix(recording): export touch overlay at capture size, never black (#2707) - #2767
Conversation
|
Size Report
Startup median (7 runs, lower is better):
|
3e404ec to
717f9f3
Compare
|
Reviewed at 717f9f3. The change is not proven on the route it touches: nobody has run a real record start, tap, record stop sequence at this head, the #2707 completion matrix in the PR body is still open, and the new e2e case in test/integration/recording-overlay.test.ts#L16 only runs under AGENT_DEVICE_RECORDING_E2E. So two things stay unproven: whether the touched export now matches the packages/capture-kit/src/recording/mp4-track-size.ts#L182 nearly duplicates childAtoms in packages/capture-kit/src/recording/mp4-atoms.ts — same MAX_SIBLINGS_PER_LEVEL, same 64-bit size==1 branch, same safe-integer and container-end checks, just walking a Buffer instead of an fd — and it disagrees with the original on size==0 (extend-to-end vs stop-here). This adds about 290 lines of test-only parser, fixtures, and a new package subpath to production capture-kit, to serve one gated integration test, when test/integration/support/recording-inspect.swift already computes the same transform-applied size via AVFoundation ( Not blocking: the #2707-labeled tests in src/platform-runtime-screen-recording-finalizer-host.test.ts#L84 exercise a catch-to-overlayWarning path already on main, not the new Swift checks, and the "unplayable" test's title says "keeping the raw recording" even though overlay.ts#L98 has already renamed the composite over the raw file, so these could use retitling or a check that's red on the old preset, but that's optional either way. All 21 CI checks are green, but none of them runs the AGENT_DEVICE_RECORDING_E2E device lane, so this green run says nothing about the changed export path. I did not reproduce the claim that the MediumQuality preset collapses the track to 220x480; that rests on an offline harness. The 32px size tolerance and the luma thresholds are not calibrated against real encoder output in any test, and a moov with no tkhd falls back to the stsd coded size without applying the transform, so a rot90 track in that case would come back transposed rather than undefined. Please attach the live device run showing the touched size equals the control with no overlayWarning and the 90s clip is not black, and replace the TS MP4 parser with the size recording-inspect.swift can already provide, before this is ready to merge. |
717f9f3 to
5a58942
Compare
Device evidence + review follow-up (
|
| clip | overlay | render | record stop |
overlay-blue px in tap crop |
|---|---|---|---|---|
short_ctrl |
--hide-touches |
1398x2034 |
success:true, overlayWarning:null |
0 (expected — no burn-in) |
short_touch |
default | 1398x2034 |
success:true, overlayWarning:null |
5114, centroid (140,140) = tap point |
Touches size == control, no overlayWarning, and the indicator actually drew (control 0 px). The touched file is also 709 KB vs the 118 KB raw control, i.e. a real full-res re-encode, not a copy-through.
~90s clip (taps + scroll, 20 gesture events): render 1206x2622 == its --hide-touches control, success:true, overlayWarning:null, mean luma 240.8–241.0 over 5 samples (min 240.82, not black), and every sampled scroll crop holds 3,194–4,972 overlay-blue px — the overlay drew into the full-size, non-black export. This is a real encoder's output passing the compositor's own checks, so the 32px tolerance and luma floors are exercised against genuine H.264 frames, not just the offline harness.
Not yet run — iPhone Duo inner panel. The Duo is held by a live session (duo2728, worktree t3code-1e588ea7); device status --stale finds nothing releasable, so I did not contend for it. The rot90 upright comparison it would cover is exercised by the AVFoundation transform-applied renderWidth/renderHeight in the gated case and by the offline rot90 harness; I'll re-run the Duo lane in the gated e2e as soon as that device is free. The non-rot90 completion row is now measured green on-device.
|
Reviewed at 5a58942. The parser follow-up is resolved: the TS MP4 parser, its test, fixtures, package export and allow-list entry are gone, and the e2e now reads One thing is still open. The export now always uses AVAssetExportPresetHighestQuality, and the new self-check compares the transform-applied Not blocking: the iPhone 17 short tap is reported as 1398x2034, but the 90s clip on the same device is 1206x2622 (its native size). Could you attach the two The Smoke Tests failure ( The ADR 0025, device-verification.md, and |
) The overlay burn-in re-encoded through AVAssetExportPresetMediumQuality, a fixed-canvas preset that caps the long edge at 480px, collapsing any touch recording to ~220x480 (480x220 landscape) and going all-black on long clips, always with exit 0. --hide-touches and an empty gesture list copy the raw capture through, so only the composited path was affected. Both quality tiers now export through the one geometry-preserving preset (HighestQuality); the hardware encoder makes the full-resolution re-encode cheap. The compositor now verifies its own output against the raw before the caller adopts it — resolved track size must match, and a uniformly black track whose raw had visible content is rejected — and throws on failure so the overlay is dropped, the raw kept, and the choice reported as overlayWarning rather than a broken file. Adds an mp4 transform-aware track-size reader and a device-lane assertion that a touched export equals its --hide-touches control.
5a58942 to
92b8fae
Compare
Geometry correction + Duo split (
|
| clip | overlay | open device |
record stop |
events | render | overlay-blue px |
|---|---|---|---|---|---|---|
ctrl_short |
--hide-touches |
iPhone 17 | success:true, overlayWarning:null |
1 | 1206x2622 |
0 |
touch_short |
default | iPhone 17 | success:true, overlayWarning:null |
1 | 1206x2622 |
5119, centroid (140,140)=tap |
Touches size == control == the ~90s clip (1206x2622), no overlayWarning, indicator drawn, control 0 px. So on iPhone 17 the non-rot90 row is consistent across the short pair and the 95.5s clip (20 events, mean luma ~241, drew 3–5k px/crop).
Duo inner panel → split into a tracked follow-up: #2774 (per your "splitting the Duo row would also work"). The Duo is held by a live session right now (cwd:0caa2f1b465d1a30:ios, pid 830, classification: live; device status --stale finds nothing releasable), so I did not contend for it. #2774 carries the exact completion conditions you listed: a short-tap record stop --json with no overlayWarning at upright 2006x2852 == the --hide-touches control (not the 2853x2007 coding grid), plus a ~90s taps+scroll clip with the same size match, no overlayWarning, non-zero luma and drawn overlay pixels, read via recording-inspect.swift's renderWidth/renderHeight.
On the Smoke Tests failure: agreed it's unrelated — it's on the wait text Agent Device Tester launch/snapshot route (wait_capture_stalled) and never runs record start/stop. Rebase onto current main (92b8faed3) with check:affected green.
|
Reviewed at 92b8fae: clean, and ready for human review. The overlay now exports at capture size and never falls back to black. This fixes the problem from the earlier review, and the patch is the same as 5a58942 after the rebase. The iPhone 17 result (touched export equal to the CI: Coverage ran all tests green but exited 1 on 3 unhandled rejections from Not blocking: No conflicts. |
Coverage was a pre-existing teardown flake (rerun → green)The red That file Re-ran the failed job as-is ( |
Summary
A
recordclip collapsed to ~220x480(and went all-black on long captures) whenever the touch overlay drew, while exiting 0. The burn-in re-encoded throughAVAssetExportPresetMediumQuality, a fixed-canvas preset that caps the long edge at 480px. The trigger was the overlay path, notrot90inner-panel tracks.overlayWarningonrecord stopinstead of returning a broken file at exit 0.--qualityno longer changes Apple output (it never affected the raw capture and no longer picks a resolution preset); CLI help and website docs corrected.device-verification.mdrot90 claims corrected.@agent-device/capture-kit/recording-mp4-track-sizereader backs the regression test.Closes #2707. 12 files.
Validation
Commit
3e404ec35.pnpm check:affected --runpassed all runnable checks (fallow complexity, full unit suite 11085, layering, package, bundle-owner, packaged-runner-swift, command-docs).An offline AVFoundation harness reproduced the collapse (
1206x2622→220x480, landscape1310x602→480x220) and confirms the fix preserves geometry and rejects black/truncated output. Unit tests cover the MP4 reader (v0/v1 display-vs-coded,rot90, coded fallback, truncated) and the finalizer drop-and-report path.test/integration/recording-overlay.test.tscompares a touched export to a--hide-touchescontrol, asserting equal track size and that the overlay drew.Device lane. Measured green on the exact issue device (iPhone 17
0E0FCBDC…, iOS 27.0, non-rot90,--udid-pinned): a touched tap export equals its--hide-touchescontrol size with nooverlayWarningand a drawn indicator, and a ~90s taps+scroll clip is non-black at full size with nooverlayWarning(see the evidence comments). The iPhone Duo inner (rot90) panel is split into follow-up #2774 — that device is held by a live session here, so I did not contend for it; #2774 carries its exact completion conditions.rot90upright handling is otherwise exercised by the AVFoundation transform-appliedrenderWidth/renderHeightcompare in the gated case.