Skip to content

feat(recording): cap the touch overlay frame rate at the caller's --fps - #3241

Open
thymikee wants to merge 4 commits into
mainfrom
fix/recording-overlay-fps
Open

thymikee wants to merge 4 commits into
mainfrom
fix/recording-overlay-fps

Conversation

@thymikee

@thymikee thymikee commented Oct 5, 2026 •

Copy link
Copy Markdown
Member

Summary

Follow-up to #3219. record start --fps <n> now also caps the touch-overlay export: the helper renders at most min(--fps, 30) frames a second (never above the source track's rate), so a lower --fps makes record stop faster on long recordings. Without --fps, behavior is unchanged (≤30).

fps is a durable recording fact, so Apple recovery and the Android manifest keep the cap across a daemon restart. HarmonyOS is out of scope. When the export itself runs out of its 70 s budget, the overlay warning names the budget and suggests a lower --fps. A compile that ate the budget keeps the plain message.

Validation

43e7b74fb: pnpm check:affected --run passed (4,168 tests). New Apple tests cover the simctl start snapshot and finalizeAppleRecordingFromCollected. Deleting either fps spread makes them fail.

Live: dedicated iPhone 17 simulator (iOS 27.0) with isolated --state-dir, Settings, touches shown, three scrolls plus one tap, then stop. Probed with AVFoundation (AVAssetReader frame count, minFrameDuration):

$ agent-device record start /tmp/ad-fps-live-fps10.mp4 --fps 10
$ agent-device press 200 400   # plus scroll down/up x3
$ agent-device record stop --json   → success, overlayWarning: none, telemetry: scroll+tap
frames=112 duration=11.20s effective_fps=10.00 minFrameDuration=60/600

$ agent-device record start /tmp/ad-fps-live-default.mp4
$ agent-device record stop --json   → success, overlayWarning: none
frames=342 duration=11.40s effective_fps=30.00 minFrameDuration=20/600

Without scrolling, the default export came out at 15 fps. That is simctl's own source rate on a static screen, which the helper never exceeds. Not run: Android emulator, physical iOS.

🤖 Generated with Claude Code

record start --fps now also caps the touch overlay's composited frame rate
(at most 30), carried as a recording fact so a recovered stop honours it.
An overlay export that runs out of its budget names that in overlayWarning.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@github-actions

github-actions Bot commented Oct 5, 2026 •

Copy link
Copy Markdown
PR Preview Action v1.8.1

QR code for preview link

🚀 View preview at
https://callstack.github.io/agent-device/pr-preview/pr-3241/

Built to branch gh-pages at 2026-10-06 05:53 UTC.
Preview will be ready when the GitHub Pages deployment is complete.

@github-actions

github-actions Bot commented Oct 5, 2026 •

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
Installed (including dependencies) 5.07 MB 5.07 MB +1.5 kB
Package (unpacked) 5.07 MB 5.07 MB +1.5 kB
Package (download) 1.52 MB 1.52 MB +444 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 28.6 ms 28.4 ms -0.2 ms
CLI --help 87.9 ms 85.7 ms -2.3 ms

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 21 files

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread packages/platform-apple/src/recording/completion.test.ts
Comment thread website/docs/docs/commands.md
Comment thread packages/capture-kit/src/recording/overlay.ts Outdated
Comment thread packages/contracts/src/screen-recording-runtime.ts Outdated
…an out of it

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@thymikee

thymikee commented Oct 6, 2026

Copy link
Copy Markdown
Member Author

At b08d6b9 the touch-overlay frame-rate cap has no live evidence yet, and the Apple finalize path has no test that protects it. There are no conflicts. The one cancelled check, Analyze (javascript-typescript), looks unrelated: the diff touches no CI config or dependency, and the other 20 checks pass. A re-run would confirm it.

The Swift helper that composites the export gains --max-fps and a new resolvedFrameDuration signature in recording-overlay.swift. Every TS test mocks runCmd, and the host compiles this file with swiftc only at record stop, so a compile or runtime error shows up only on a live run. The 600 s run cited in the body is from #3219, before --max-fps existed. Today nothing shows that --fps N lowers the exported frame rate, that no --fps still gives 30, or that the helper compiles. Please do one live run on the touch-overlay route: macOS host, iOS simulator (simctl) recording with --show touches, one tap, then record stop. Run it once with --fps 10 and once without --fps. Probe each exported MP4 and show the effective frame rate (frame count or AVAssetTrack minFrameDuration). It should be 10 fps or less with --fps 10, and unchanged (30 or less) without it. Both stops should succeed with no overlayWarning. Please paste the commands and output in the PR body.

The simulator route reads the live snapshot built at runtime.ts:489 and finalizes through finalizeAppleRecordingFromCollected at completion.ts:99. The Apple tests cover only completeAppleRecording. If someone deletes either spread, every added test stays green, and long simctl recordings that hit the budget lose the cap silently. Android has matching snapshot and recovery tests. Please add a finalizeAppleRecordingFromCollected case that asserts fps reaches finalize.complete and is absent when unset. Please also add an Apple runtime start and inspect case with fps: 15, like the Android live-snapshot.test.ts.

Is the fan-out needed? The wire is already small: one conditional --max-fps in overlay.ts and one Swift flag. The spread comes from persisting fps on the Android manifest and descriptor, which only matters so recovery after a daemon restart can still cap the overlay. Could the descriptor, manifest and snapshot be built from one RecordingFacts pick instead of about 10 hand-copied spreads? The recording-facts module would need to own a pick helper first. The overlay-budget error wording also looks like a separate small change riding in this PR.

Not blocking, so take or leave these: the PR body describes #3234 (the .ad --until change), lists 20 files that are not this PR's, and carries Closes #3197, which would auto-close an unrelated open issue on merge. Please rewrite it for the overlay cap and add the live evidence. Also, the new field is copied by hand at about 10 sites, including manifest.ts:116. Apple already enumerates the facts through RECORDING_FACTS_KEYS in apple/recovery.ts:54. A shared pick helper over those keys, or Android types that extend RecordingFacts, would fit as a follow-up.

The open inline threads still stand: recorded-fps test is fixed at this commit, so please resolve it. overlayTouches catches rejections does not apply, because overlayTouches returns overlayWarning on every overlay rejection, so your rebuttal is right. exportOverran wording is fixed at this commit, so please resolve it. never more than 30 is fixed at this commit, so please resolve it.

I did not run the tests or compile the Swift helper, so its syntax and behavior are unverified. My check covered code paths only, not a live simulator or Android run. I also did not check whether Android screenrecord honours --fps, since the cap affects only the overlay export. Before merge, please add the live simulator run, the two Apple fps tests and the corrected PR body without Closes #3197.

…ize path

Cover the simulator start snapshot and finalizeAppleRecordingFromCollected, the
route simctl recordings take, and name the touch-overlay cap in --fps help.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@thymikee

thymikee commented Oct 6, 2026

Copy link
Copy Markdown
Member Author

Addressed in 43e7b74fb:

  • Live run: done on a dedicated iOS 27 simulator with touches shown and a tap. With --fps 10 the export is 10.00 fps (112 frames / 11.2 s, minFrameDuration 1/10). Without --fps it is 30.00 fps (342 / 11.4 s). Both stops succeeded with no overlayWarning, and the helper compiled at stop. Commands and output are in the PR body. A first run with no scrolling came out at 15 fps by default: the helper never exceeds the source track's minFrameDuration, and simctl wrote 15 fps on a static screen. So I added scrolls so the cap actually binds.
  • Apple tests: runtime.test.ts now covers a simctl start with and without fps through inspect() and finish(). completion.test.ts covers finalizeAppleRecordingFromCollected with fps: 10 and with none. Deleting the spread at runtime.ts:489 or completion.ts:99 makes these tests fail. I checked both by deleting each one.
  • PR body: rewritten for this PR, and Closes #3197 is gone.
  • Budget wording: it stays in this PR because it now names --fps as the remedy, so it depends on this change.
  • Fan-out / pick helper: agreed that it belongs in a follow-up. A RecordingFacts pick in recording-facts.ts, used by the Android descriptor, manifest, and snapshot, would replace the hand-copied spreads. That refactor reaches beyond fps, so I've kept it out of this PR.
  • Inline threads: all four were already resolved.

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 3 files (changes from recent commits).

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment thread packages/command-registry/src/flag-definitions-action.ts Outdated
… fps ceiling

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant