Repository navigation
feat: deprecate the screenshot stream formats on iOS, keep them on Android - #138
Conversation
74a9287 to
8e7135c
Compare
Round 4 — no P0/P1 remains; review convergedThe reviewer accepted the timeline trade-off after seeing the measurement, and corrected my reasoning for it, which was worth more than the finding itself. Correction I took: my ffplay model was wrongI had written that ffplay's master clock is "driven by the last picture shown". For a video-only input it actually defaults to an external clock. The real mechanism is that ffplay holds the current picture for a duration derived from the next picture's PTS — so a wall-clock gap keeps the stale picture on screen for exactly that long, and sampling the transport clock does not change that schedule. Same conclusion, correct cause. This matters because that comment is what a future maintainer will read when they reconsider [P2] The direct surfaces still described the old contract — accepted, fixed
[P2] Retained iOS screenshot formats crash on pipe hangup — rejected, measuredMeasured all three: Simplification pass
Net 47 lines lighter. Kept deliberately, as advised: the write-ordering rationale, the adaptation-only continuity note, the Verificationunit 1406 pass · Android E2E 10/10 · iOS E2E stream + record both pass · live stream 78 pictures, Review summary across four roundsTwelve findings, all real. Fixed: PSI section syntax, PCR-only packet shape, concurrent write ordering, an uninterruptible blocked write, PTS/PCR equality, keep-alive clock restatement, a stale architecture comment, an imprecise causal explanation, direct-surface help and summary drift, and three documentation claims the code did not support. Two rejected with measurement. Two of the defects were introduced by earlier rounds' own fixes — which is the case for having run more than one round. One process note: I briefly damaged the local branch layout while moving this commit between layers (a cherry-pick conflicted and I reset the wrong branch). Recovered from the remote; ancestry re-verified — #133 → #136 → #138 — and all three layers confirmed free of conflict markers before this push. This commit spans all three layers, so it sits on the top one rather than being split three ways. |
Round 5 — clean confirmation, plus one thing the confirmation missedThe reviewer re-verified all four items and reported no P0/P1/P2 remaining, accepting the pipe-hangup rejection outright: with SIGPIPE at its default, the iOS While checking point 2 myself I found the confirmation had verified the behaviour and not the prose. Android's
The code four hundred lines below sets The Where the stack stands
Five rounds, thirteen findings, all real. Eleven fixed, two rejected with measurement — and two of the eleven were introduced by an earlier round's own fix, which is the argument for having kept going past round two. |
Final simplification passWent looking for code to cut and found comments that argued for the opposite of what ships — which is worse than verbose.
Also inlined a read-once local in
Verification on this HEAD
One thing stated precisely rather than glossed: I also ran an ad-hoc |
…droid `stream-video --format mjpeg` / `raw` / `ffmpeg` drive a screenshot loop that re-encodes every frame. On a simulator that is now strictly worse than `--format h264`, measured over the same 6 s on an iPhone 17 Pro: h264 144 frames 1.33 MB mjpeg 24 frames 11.2 MB raw/ffmpeg 26 frames 92.8 MB `h264` works on every booted simulator, so no state remains in which the loop is the better choice. The three formats now warn once on stderr and say so in `--help`; removal comes in a later release. Android keeps them and does not warn. `adb screenrecord` is unavailable on some devices, and there the `screencap` loop is the only way to stream at all — removing the formats would leave those devices able to record but not stream. The asymmetry is a real capability difference, and a unit test pins it so it does not get "tidied up" into consistency later. This reverses the plan in #134, which had assumed the formats could be retired globally once Android `stream-video` gained the automatic `screencap` fallback that `record-video` has. That fallback turns out not to be worth building: `record-video`'s is transparent because the output is a file either way, whereas a stream's container would change under a consumer that has already started decoding — MPEG-TS has no stream type for MJPEG, so there is no way to keep the wrapper stable. Failing with a clear message, which is what it does today, is the better behaviour. `bgra` is untouched: raw unencoded pixels are not something H.264 substitutes for. Refs #134 Signed-off-by: onevcat <onevcat@gmail.com>
The notice pointed at `ffplay -f mpegts -probesize 32 -fflags nobuffer -`, which does not open an Android stream and is the command the previous commit replaced everywhere else. Signed-off-by: onevcat <onevcat@gmail.com>
…eight Round 4 of review found no P0 or P1 and accepted the timeline trade-off. This lands the two documentation/UX findings, one correction to my own reasoning, and the simplification pass. **The comment recording the timeline decision was imprecise.** I had written that ffplay's master clock is "driven by the last picture"; for a video-only input it actually defaults to an external clock. The mechanism is that ffplay holds the current picture for a duration derived from the *next* picture's PTS, so a wall-clock gap keeps the stale picture on screen for exactly that long, and sampling the transport clock does not change that schedule. Same conclusion, correct cause — and this is the reasoning a future maintainer will use when reconsidering the cap, so it has to be right. Also dropped the claim that ffmpeg and browsers "schedule from PTS" the same way: the ffmpeg CLI is not a presentation scheduler, and MSE support for MPEG-TS is implementation-dependent. The comment now cites only the measured ffplay path. **The direct Android surface still advertised byte passthrough.** Its `--help` said "native screenrecord passthrough" and its summary printed bytes only, while the top-level verb printed frames and the changelog said h264 reports a frame count. Same stream, three descriptions. It now says MPEG-TS re-containering without re-encoding and reports frames, matching the other surfaces. The iOS command's abstract said "using screenshot capture", which stopped being true when h264 landed; it is now format-neutral. **Rejected: that the retained iOS screenshot formats crash on pipe hangup.** Measured instead — `mjpeg`, `raw` and `ffmpeg` all exit on SIGPIPE (signal 13) with no ObjC exception, which is the ordinary Unix outcome for `| head`. The `NSFileHandleOperationException` the finding describes only occurs when SIGPIPE is ignored, and only the Android sink does that, for itself. Nothing to fix. **Cuts.** `MPEGTSStreamWriter` loses `tableInterval` and `lastTablesTime`: the only caller emitted tables every 40 ms anyway, so the per-append re-emission was unreachable in production and the two entry points collapse into one `emitProgramTables()`. The top-level `ExecutionResult` loses `format`, which was populated and never read (`--json` is refused on this command and the summary picks on the counters alone). The iOS record command loses eight lines of architecture history that this stack had already invalidated. A test comment moved to the test it describes. Net 47 lines lighter, and `MPEGTSStreamWriter` is 62 lines of code where it was 76. Kept, as the reviewer advised: the write-ordering rationale, the adaptation-only continuity note, the `clockReference(at:)` primitive, and both concurrent-writer tests — one proves a race is permitted, the other that byte order holds. Verified: unit 1406 pass; Android E2E 10/10; iOS E2E stream + record both pass; live stream 78 pictures with program association intact and ffmpeg demuxing with no warnings. Signed-off-by: onevcat <onevcat@gmail.com>
The result type still documented h264 as a byte passthrough with no frame notion, which the re-containering path contradicts, and the sentence describing the engine had lost its head in an earlier edit. State the current contract in both, and drop the two before/after notes that only recorded that the behaviour used to differ. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: onevcat <onevcat@gmail.com>
Three comments argued for the opposite of what ships. The muxer's header said it emits no PCR-only packets, which is only true because nothing calls the primitive that builds one; `clockReference` argued for emitting them through an idle stretch, which is the design the stream writer measured and rejected. Point it at that evidence instead, so anyone reaching for it finds the measurement rather than an argument. `maxFrameGap` and `emitProgramTables` also both derived why an idle timeline has no clock to send and both cited the same ffmpeg evidence. The derivation belongs to the constant that causes it; the evidence belongs to the method that would otherwise be tempted to send one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: onevcat <onevcat@gmail.com>
… cheatsheet The README said `-analyzeduration 0` was required for the ffplay preview to open on Android. Measured on a moving emulator screen, the open takes ~1 s with the flag and ~1 s without it: zero selects ffmpeg's default analysis window rather than disabling it, and what bounds the open is `-probesize 32768`, because a still Android screen sends only program tables, which never count towards a media-time budget. The command drops the no-op flag everywhere it is printed (README, changelog, stderr hints, the deprecation notice), the explanation now credits `-probesize`, and it states the consequence that matters to a user: on a still Android screen the window appears only once the device moves. The skill's cheatsheet still described `--format h264` as Android-only behind `ffplay -f h264 -`, both of which this stack made wrong; it now shows the cross-platform MPEG-TS command and marks the screenshot formats deprecated on iOS. Two header comments that still said "passthrough" and "minus the muxer" for the Android leg describe the transport-stream writer instead. Signed-off-by: onevcat <onevcat@gmail.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…th iOS `StdoutStreamSink` set `O_NONBLOCK` on stdout so a full pipe would surface as EAGAIN. That flag lives on the open file description, not the descriptor, so it leaked: stdout on a terminal left the user's shell non-blocking after exit, and under `2>&1` the summary write to stderr hit EAGAIN inside `FileHandle` and aborted the process (SIGABRT, verified with a slow reader). Every Android stream format was affected, since they share the sink. The sink now leaves the flags alone. It waits for `POLLOUT` in 100 ms slices, checking cancellation between them, and writes at most `PIPE_BUF` bytes per call: a pipe reports writable only when that much room exists, and a write no larger than that completes without blocking once it does. A stalled consumer therefore still cannot pin the writer, which the existing test keeps proving against a blocking descriptor; a new test pins that the flags are untouched, and another that a slow reader receives every byte in order through the chunked writes. iOS had the same hang and no sink at all. `stream-video --format h264` and `bgra` copied the stream through idb's `FBFileWriter.syncWriter`, which blocks in `write(2)` on the encoder's callback thread; with a consumer that stopped reading, `stopStreaming()` waited behind it and Ctrl-C needed SIGKILL (sampled: the VideoToolbox callback parked in `write`, the main thread in `semaphore_wait`). The sink moves to `SimUseVideo`, and a small `FBDataConsumer` adapter feeds it, so both platforms share one interruptible path. A consumer that closes the pipe now ends the iOS stream in an orderly way instead of killing the process with SIGPIPE. An iOS E2E test pins the cancellation: a pipe nobody drains, SIGINT after it fills, exit within seconds. Signed-off-by: onevcat <onevcat@gmail.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
`stream-video` inherited the screenshot loop's default of 10 fps, so the documented preview command delivered a third of the rate the README and changelog quote for `h264`: measured 74 frames in 7.3 s at the default against 221 in 7.4 s with `--fps 30`. `h264` shares `record-video`'s encoder, whose default is 30, so `--fps` is now optional and resolves per format: 30 for h264, 10 for the screenshot formats, which cannot sustain more. The top-level verb forwards the option unresolved so the same rule applies there. Help text on both surfaces states the split, and the iOS help no longer claims only `bgra` reports no frame count (h264 does not either). Signed-off-by: onevcat <onevcat@gmail.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
a71955e to
5bd86d8
Compare
stream-video --format mjpeg/raw/ffmpegdrive a screenshot loopthat re-encodes every frame. On a simulator that is now strictly worse
than
--format h264, measured over the same 6 s on an iPhone 17 Pro:h264 144 frames 1.33 MB
mjpeg 24 frames 11.2 MB
raw/ffmpeg 26 frames 92.8 MB
h264works on every booted simulator, so no state remains in which theloop is the better choice. The three formats now warn once on stderr and
say so in
--help; removal comes in a later release.Android keeps them and does not warn.
adb screenrecordis unavailable onsome devices, and there the
screencaploop is the only way to stream atall — removing the formats would leave those devices able to record but
not stream. The asymmetry is a real capability difference, and a unit test
pins it so it does not get "tidied up" into consistency later.
This reverses the plan in #134, which had assumed the formats could be
retired globally once Android
stream-videogained the automaticscreencapfallback thatrecord-videohas. That fallback turns out notto be worth building:
record-video's is transparent because the outputis a file either way, whereas a stream's container would change under a
consumer that has already started decoding — MPEG-TS has no stream type
for MJPEG, so there is no way to keep the wrapper stable. Failing with a
clear message, which is what it does today, is the better behaviour.
bgrais untouched: raw unencoded pixels are not something H.264substitutes for.
Refs #134
Signed-off-by: onevcat onevcat@gmail.com
Stack created with GitHub Stacks CLI • Give Feedback 💬
Verification (top of stack)
make test— 1394 pass, including a new test pinning that the deprecation is iOS-onlyStreamVideoTests+RecordVideoTests— both suites passmake e2e-android— 10/10 suites, confirming Android still accepts the formats without warning--formatmjpegrawh264bgramjpegCI note: this repo triggers workflows on
pull_request: branches: [main], so a stacked PR only gets the DCO check. The above was run locally; CI will cover it once the lower layers merge.Stacked on #136, which is stacked on #133.
Refs #134 — the issue tracks removal in a later release, so it stays open.