Repository navigation
feat: record-video captures real H.264 streams instead of screenshot polling - #52
Conversation
…polling Replace the screenshot-poll-and-re-encode recorder (capped ~8-10 fps) with native H.264 capture muxed straight into MP4 (passthrough, no re-encode): - iOS: FBSimulatorVideoStream eager H.264 at a constant --fps (1-60, default 30). The elementary stream carries no timestamps, so frames are laid out at exactly 1/fps -- the requested rate is honored and playback is smooth (uniform 16.7 ms spacing at 60 fps). - Android: adb screenrecord --output-format=h264 at the device's native variable rate, stitched across the API<34 180 s per-invocation limit. --fps is ignored there; --quality/--scale map to bitrate/size. New shared infra under Sources/iOSSimBackend/Util: AnnexBStreamParser (NAL splitting + access-unit assembly), H264PassthroughRecorder (AVAssetWriter passthrough; CFR for iOS, host-clock VFR for Android), H264MuxingPipeline; plus AdbStreamingProcess for incremental adb stdout. --fps range widened 1-30 -> 1-60 (default 10 -> 30). The legacy screenshot/screencap recorders are retained as automatic fallbacks. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: yuta.ooka <yuta.ooka@lycorp.co.jp>
… Sendable Replace every NSLock + `@unchecked Sendable` in the record-video path with `OSAllocatedUnfairLock<State>`-backed plain `Sendable` classes (CancellationFlag, OnceFlag, FirstErrorBox, H264StreamRecorder, H264PassthroughRecorder, H264MuxingPipeline, AdbStreamingProcess). Non-Sendable writer / subprocess state is confined to the lock, so no type needs an unchecked escape hatch and no withLockUnchecked is used. (Mutex would be cleaner still but requires macOS 15; the package targets macOS 14.) Also wait for the stream's first frame before committing to it: a cold FBSimulatorVideoStream that attaches but pushes nothing now falls back to screenshot capture instead of producing an empty recording. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: yuta.ooka <yuta.ooka@lycorp.co.jp>
98f2774 to
0b1f5c7
Compare
onevcat
left a comment
There was a problem hiding this comment.
Thanks a lot for this PR! The new H.264 passthrough pipeline is a big improvement, and the code and tests are in great shape.
I pulled the branch and tested it live on both platforms (iOS simulator + Android emulator, SDK 36): build, all 1123 unit tests, recording under motion, --fps / --scale handling, SIGTERM with a short-grace SIGKILL, flag validation, and --json output all check out. The recorded videos look correct and play at the right speed.
During live testing I found three issues — one real behavior gap (1) and two smaller ones (2, 3). Details are in the inline comments. Could you confirm and fix?
- Android:
--qualityis silently ignored at the default--scale 1.0—--bit-rateis only passed toscreenrecordwhen--scale < 1.0, which contradicts the PR description and README. - iOS: constant-frame-rate layout has no guard against stream under-delivery — if the stream drops frames, the video silently plays back faster than real time (non-blocking suggestion).
- iOS: mid-stream errors don't mention the partial file — the MP4 is already finalized and on disk, but the error message doesn't say so, unlike the Android branch (minor).
|
|
||
| let sdk = Self.detectSDK(adb: adb, serial: serial) | ||
| let size = scale < 1.0 ? Self.detectScaledSize(adb: adb, serial: serial, scale: scale) : nil | ||
| let bitrate = size.map { H264StreamRecorder.estimateBitrate(width: $0.width, height: $0.height, fps: 30, quality: quality) } |
There was a problem hiding this comment.
--quality does nothing on Android unless --scale < 1.0.
size is only detected when scale < 1.0, and bitrate is derived from size via size.map { ... } — so at the default scale, --bit-rate is never passed and --quality is silently ignored.
I verified this on a live emulator by capturing the device-side command line during recording (adb shell ps -ef | grep screenrecord):
--quality 20(default scale):screenrecord --output-format=h264 --time-limit 0 -— no--bit-rate--quality 20 --scale 0.5:screenrecord --output-format=h264 --time-limit 0 --bit-rate 3499200 --size 540x1200 -— works
This contradicts the PR description and README, which say --quality maps to bitrate on Android.
Suggested fix: always detect the display size via wm size (if detection fails, fall back to not passing --bit-rate), pass --bit-rate unconditionally, and keep passing --size only when scale < 1.0. detectScaledSize / parseWMSize can be reused as-is, so this should be a ~3-line change. (Alternatively, document the limitation and print a note like the --fps one — but making the code match the documented semantics seems better.)
There was a problem hiding this comment.
Fixed — display size is now detected unconditionally (detectSize), so bitrate is always computed regardless of --scale. Only the --size argument itself stays scale-gated.
Verified live (adb shell ps -ef | grep screenrecord) at default scale, both quality settings:
--quality 80(this CLI's actual default):--bit-rate 46061568(previously absent entirely — screenrecord fell back to its own fixed ~20 Mbps default regardless of--quality). This is the representative case; roughly a 2.3x bitrate change from before.--quality 20:--bit-rate 19740672— coincidentally close to screenrecord's own default, since quality≈20 happens to be near the formula's break-even point. Flagging so the numbers aren't read as "barely changed" in general.
Added bitrateWithoutSizeAtDefaultScale + scaledSize unit tests. (654d423)
| /// (variable-rate) frame is held for its true wall-clock duration. | ||
| /// Throws `.noFramesCaptured` — after deleting the empty output — when | ||
| /// no frame was ever appended. | ||
| public func finish(stopHostTime: TimeInterval?) async throws { |
There was a problem hiding this comment.
Non-blocking suggestion: guard CFR mode against stream under-delivery.
CFR mode trusts the frame count unconditionally: PTS is frameIndex / fps, so if FBSimulatorVideoStream delivers fewer frames than requested, the video is silently time-compressed and plays back faster than real time.
I hit this once during live testing: starting a 60 fps recording right after stopping a previous one produced only 77 frames over ~6 s of wall clock, i.e. a 1.28 s video that plays ~5x too fast. It didn't reproduce in controlled runs (60 fps at full size under motion tracked wall clock fine), so it looks like a cold-start / encoder-still-releasing edge case — but when it happens there is no hint to the caller, which is a silent semantic error for agent consumers.
A cheap guard: at finish time, compare framesAppended / frameRate against the actual wall-clock recording duration, and print a stderr warning when they diverge by more than ~20%. That keeps the smooth CFR playback while making the failure mode visible.
There was a problem hiding this comment.
Added a diagnostic guard (not a fix to the underlying under-delivery, since you flagged this as non-blocking): finish() now compares framesAppended / frameRate against the actual wall-clock recording duration and warns on stderr when they diverge by more than ~20% (threshold configurable). Factored into a pure isUnderDelivered helper with unit tests, including your exact repro numbers (77 frames / 60fps / ~6s). The recording itself is unchanged — this only makes the failure mode visible. (654d423)
There was a problem hiding this comment.
Follow-up: a later merge from upstream's idb migration (76639e4d → 1f6943f8) added a native in-process file recorder (FBSimulator.startRecording(toFile:configuration:)) that does exactly what our own hand-rolled H.264 stream → MP4 passthrough mux was doing for iOS — now as idb's own maintained code. iOS has been switched to call it directly, so the custom FBVideoStreamConfiguration/createStream integration is gone for iOS.
Consequence for this comment: since iOS no longer lays out its own PTS (idb's native encoder produces correct timing internally), the isUnderDelivered guard and the frameRate (CFR) mode in H264PassthroughRecorder became dead code — nothing calls them anymore (Android is the only remaining user of that class, and it's always variable-rate). I removed both, along with their unit tests, in ba2b75d.
I also re-tested the cold-start scenario you found (77 frames over ~6s) with several rapid back-to-back recordings against the new idb — it didn't reproduce, consistent with upstream's video-layer rewrite.
| await stopStreamBestEffort(videoStream) | ||
|
|
||
| if let error = streamError.first { | ||
| throw error |
There was a problem hiding this comment.
Minor: mid-stream errors should mention the partial file.
If the stream fails mid-recording, the MP4 has already been finalized successfully by this point and stays on disk — but the user only sees Failed to record video: ... with no hint that a playable partial recording exists. The Android branch handles the equivalent case with "...partial recording saved to ".
Suggest aligning the wording so the iOS error also points at the partial file.
There was a problem hiding this comment.
Fixed — mid-stream errors after the MP4 was already finalized now say "partial recording saved to <path>", matching the Android wording. Wording-only change; doesn't affect recording behavior. (654d423)
…ivery guard, partial-file error wording Fix three issues from @onevcat's review: 1. Android: --quality was silently ignored unless --scale < 1.0, because bitrate was only computed from a size that was only detected when scaled. Detect display size unconditionally (still only pass --size to screenrecord when scale < 1.0) so --bit-rate reaches screenrecord at the default scale too. Verified live: `--quality 20` at default scale now yields `--bit-rate 19740672` on the device's screenrecord command line (previously absent). 2. iOS: CFR mode now detects when the stream delivers fewer frames than the requested rate over the actual recording time (cold start / encoder still releasing) and warns on stderr, since the muxed file would otherwise silently play back faster than real time with no signal to the caller. 3. iOS: mid-stream stream errors that surface after the MP4 was already finalized now say "partial recording saved to <path>", matching the Android branch's wording, so a usable recording isn't discarded just because the command exited non-zero. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: yuta.ooka <yuta.ooka@lycorp.co.jp>
…264-stream Resolves the idb migration merged upstream (76639e4d -> 1f6943f8): static XCFrameworks, FBDeviceControl removed, FutureBridge/BridgeQueues deleted in favor of native async/await APIs (CompanionUtilities). Beyond the textual CHANGELOG.md conflict, this required porting IOSSimRecordVideoCommand.swift off the old FBVideoStream + manual FBDataConsumer + Annex-B passthrough-muxing integration (which no longer compiles) onto upstream's new FBSimulator.startRecording(toFile: configuration:) -- a native in-process file recorder that internally drives the same FBSimulatorVideoStream in eager H.264 mode and muxes to .mp4 via its own AVAssetWriter-backed file writer. This is the same passthrough-muxing architecture this command hand-rolled for the PR; it's now upstream's own maintained implementation, so the custom consumer/pipeline wiring for iOS is gone (Android's adb screenrecord path is untouched and still needs the Annex-B parser/muxer, since idb doesn't drive Android at all). Consequence: H264PassthroughRecorder's CFR (frameRate) mode and its isUnderDelivered under-delivery guard -- added during the earlier PR review to fix onevcat's finding lycorp-jp#2 -- are now dead code, since iOS no longer does its own PTS layout (idb's native encoder produces correct timing) and Android is VFR-only. Removed both, along with their unit tests; a PR follow-up comment will explain this to the reviewer. Verified: full unit suite green (849 tests; only the 5 pre-existing unrelated Init-skill failures), e2e RecordVideoTests 6/6 live on a booted simulator (including the short-grace SIGTERM finalize test), and a live Android emulator recording under motion (~60 fps, valid MP4). Repeated rapid back-to-back iOS recordings to probe the cold-start zero-frames issue found during the original PR review -- did not reproduce against the new idb, consistent with upstream's video-layer rewrite. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: yuta.ooka <yuta.ooka@lycorp.co.jp>
33fe688 to
ba2b75d
Compare
|
LGTM! |
`stream-video` had no native format on iOS: `mjpeg`/`raw`/`ffmpeg` were a screenshot-per-frame loop and `bgra` was raw pixels, so the only way to watch a simulator live was ~4 fps at ~1.9 MB/s. Meanwhile `record-video` had been driving native H.264 since lycorp-jp#52/lycorp-jp#70 — the capability was already there, just not wired to stdout. Both verbs now build one `FBVideoStreamConfiguration.h264Capture(...)` and differ only in their sink: record-video -> H264MuxingPipeline -> MP4 stream-video -> stdout That muxer's own docs already described it as "shared by the iOS (FBSimulatorVideoStream) and Android (adb screenrecord) capture paths"; iOS simply never connected to it, using idb's separate in-process file writer instead. `AnnexBPipelineConsumer` is the missing adapter from idb's byte-stream consumer protocol to the pipeline. Measured on a booted iPhone 17 Pro (iOS 27.0), 6 s captures: screenshot loop (mjpeg) 24 frames 4.1 fps 11.2 MB screenshot loop (PNG) 26 frames 4.5 fps 92.8 MB native h264 Annex B 144 frames 24.0 fps 1.33 MB The one real trade-off: frame timestamps now come from host arrival time instead of the encoder's sample clock, since Annex B carries no timing. Across three runs each under real touch input, inter-frame jitter measured 6.0-6.8 ms on the new path against 2.7-3.1 ms on the old — both far inside a 33 ms frame at 30 fps, and the `--fps` constant-rate and 100 ms-grace SIGTERM E2E tests both still pass. fmp4 transport was evaluated first and rejected: idb's FBFMP4FrameWriter emits live fragments with no `sidx`/`mfra`, which AVFoundation reads as zero frames, so a recording would have been unplayable in QuickTime and untranscodable to GIF. Annex B keeps the MP4 a regular non-fragmented file. Verified live: both `stream-video --format h264` surfaces plus the top-level route on an iOS UDID, `ffmpeg -f h264` remux, `bgra` non-regression, the GIF path, and the full iOS stream-video and record-video E2E suites. Refs lycorp-jp#132 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: onevcat <onevcat@gmail.com>
Summary
record-videowas capped around ~8-10 fps because it polled screenshots and re-encoded every frame. It now captures real video via each platform's native recording facility, muxed straight into MP4 (passthrough — no re-encode):FBSimulator.startRecording(toFile:configuration:)), which drivesFBSimulatorVideoStreamin eager H.264 mode at a constant--fps(1-60, default 30) and muxes to.mp4via its ownAVAssetWriter-backed file writer.--fpsmaps directly to the stream's cadence, so the requested rate is honored and playback is smooth.adb screenrecord --output-format=h264at the device's native variable frame rate (measured ~50-60 fps under motion, vs ~7-8 before) into a hand-rolled H.264 Annex-B → MP4 passthrough muxer (idb doesn't drive Android).--fpsis ignored there;--quality/--scalemap to bitrate/size. Recordings past the API < 34 per-invocation limit are stitched acrossscreenrecordrestarts.--fpsrange widened 1-30 → 1-60 (default 10 → 30). The legacy screenshot/screencap recorders are retained as automatic fallbacks if the native path is unavailable.Architecture note: this PR originally hand-rolled the same H.264-stream-to-MP4 passthrough muxing for iOS too. A later merge from upstream's idb migration (
76639e4d→1f6943f8) added a first-class native file recorder to idb itself using that same architecture, so iOS now calls straight through to it instead of re-implementing it. Android still needs the custom muxer since idb doesn't touch Android at all.Demo
Settings-screen scroll recorded with the new pipeline (no LINE app involved; alternating up/down scroll):
sim-use-ios-settings.mp4
sim-use-android-settings.mp4
New infrastructure
Android-only H.264 Annex-B → MP4 passthrough muxer under
Sources/iOSSimBackend/Util/(co-located there since onlySimUse, the executable target, can see bothAndroidBackendandiOSSimBackend):AnnexBStreamParser— chunk-wise NAL-unit splitting + access-unit assembly (exp-Golombfirst_mb_in_slice, emulation-prevention handling)H264PassthroughRecorder—AVAssetWriterpassthrough with host-clock variable-rate PTS; graceful zero-frame + segment-restart handlingH264MuxingPipeline— thread-safe parse→append glueAdbStreamingProcess(AndroidBackend) — long-running adb child with incremental binary stdoutTesting
make test, no device): parser (NAL splitting, chunk boundaries, exp-Golomb), muxer (AVCC framing, monotonic PTS, real fixture round-trip → playable MP4, zero-frame error), pipeline, Androidscreenrecord/wm sizeargument construction. 849 tests green (only 5 pre-existing unrelated failures).screenrecordh264 stream,--bit-ratenow reaches the device at default scale too,--sizescaling, SDK 36--time-limit 0).🤖 Generated with Claude Code