Repository navigation
feat: bracket GIF exports with START/END marker cards - #100
Conversation
A forever-looping GIF has no visible boundary — viewers can't tell where the flow starts and ends (team feedback on real repro clips). record-video --format gif now opens with a START card and closes with an END card (~1 s each), rendered at the encoded frame size with CoreText on a dark background. On by default on all three surfaces; opt out with --no-gif-markers. The flag resolves through ResolvedRecordingOptions/RecordingOutputPlan like the other GIF options, so the surfaces cannot drift. A failed card render degrades to a marker-less GIF instead of failing the transcode — the footage matters more than the chrome. Card dimensions come from the track's naturalSize, which matches the decoded frames for sim-use's own recordings (identity transform). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: wonseob <wonseob@linecorp.com>
onevcat
left a comment
There was a problem hiding this comment.
Nice feature — the boundary problem on looping GIFs is real, and the implementation is clean (options resolved through ResolvedRecordingOptions, render failure degrading to a marker-less GIF instead of failing the transcode). I'd like to keep it, but flipped to opt-in: default marker-less, with a --gif-markers flag to enable.
Why the default should be marker-less
- A recording tool's default contract is a faithful capture of the screen. Injecting two synthetic frames means the first frame is now a "START" card — anything downstream that grabs the first frame as a thumbnail, does frame-by-frame analysis, or visual-diffs the output gets polluted by chrome. sim-use's primary consumer is agents observing the screen, not humans watching demos.
- It's a silent behavior change to existing output. The most telling evidence is in this diff itself: two existing tests had to add
markers: falseto keep passing. User scripts and the GIF-output E2E assertions hit the same thing. - The motivating use case — a human watching a repro clip — is a presentation concern for a subset of usage; it shouldn't define the default output.
Proposed changes
- On all three surfaces, drop
inversion: .prefixedNoand default tofalse:Keeping the@Flag(help: "Bracket a GIF with START/END marker cards (~1 s each) so the looping clip has a visible boundary. GIF only.") public var gifMarkers: Bool = false
gif-prefix in the name is right — it scopes the flag to GIF (mp4 has a scrubber, no loop-boundary problem) and leaves room to evolve into a valued option (e.g. an overlay style) later. - Remove the
= truedefault parameters from the library layer (ResolvedRecordingOptions.init,RecordingOutputPlan.init,GIFTranscoder.transcode/transcodeRecording). Right now the CLI layer and the library layer each carry a copy of the default, which is a drift hazard. The default should live only inResolvedRecordingOptions(its doc comment already claims "the default lives here and nowhere else"); make the downstream parameters required. - Tests: the
markers: falseretrofits on the two pre-existing tests can be reverted (that's the default again), the marker tests passmarkers: trueexplicitly, and theResolvedRecordingOptionsassertion flips to#expect(!gif.gifMarkers). - README / CHANGELOG / cheatsheet wording: "disable with
--no-gif-markers" → "opt in with--gif-markers". Also, per the repo's development rules, option changes should be reflected inskills/sim-use/SKILL.mdas well — worth a check that record-video's option list there picks this up.
Review follow-up (lycorp-jp#100): a recording should be a faithful capture of the screen by default, and downstream consumers may assume the first frame is already the device screen. The markers are now opt-in (--gif-markers) on all three surfaces; the boolean leaves room to grow into a mode argument (overlay/color/image) later if wanted. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: wonseob <wonseob@linecorp.com>
|
Thank you for the thoughtful feedback — both points make a lot of sense! 414ca1b reverses the flag as suggested: markers are now opt-in via |
onevtail
left a comment
There was a problem hiding this comment.
The core marker implementation looks sound on the exact head (414ca1b1556d78320a8799bd5e152a6f72516aa7), including the default-disabled behavior and START/content/END frame ordering. A few items still need to be addressed:
- Please remove the repeated
gifMarkers/markersdefault arguments from the library and orchestration layers (ResolvedRecordingOptions,RecordingOutputPlan,GIFTranscoder, andAndroidRecordVideoCommand.record). Internal callers should pass the value explicitly so the default policy has a single authoritative source. - Please update the evidence-GIF entry in
skills/sim-use/SKILL.mdto document or use--gif-markers. Updating the reference cheatsheet does not cover this primary skill entry. - Please add focused coverage showing that
--gif-markersparses and forwards astruethrough the top-level, iOS, and Android command surfaces. - The visible
Unit tests (macOS)check was cancelled after an hour and remains failed. Local tests pass, so this may be a runner or output-filtering issue, but the check should be rerun or investigated until the exact head has a successful result.
The main implementation does not appear to require a redesign; these are contained follow-up changes.
onevtail - an assistant to @onevcat
|
Never mind with the comments above. I can follow up for the left things! |
|
Taking over the remaining review items as promised — continued in #101, which carries your two commits forward unchanged (authorship preserved). Thanks again for the feature and the quick turnaround on the opt-in flip! The CI timeout investigation also moves there: the raw-log artifact never uploaded on timeout cancellations, which #101 fixes so a reproducing hang finally names the wedged test. |
…box sessions The suite's round-trip tests each drive a real VideoToolbox H.264 encode session via makeSyntheticMP4. Two such sessions ran concurrently through weeks of green CI; the third one added with the marker feature deadlocked the encoder on GitHub's virtualized macOS runners in 4 of 4 runs (#100/#101) and wedged the whole swift-test process until the job timeout, while never reproducing on real hardware. Forensics: the partial test.log from #101 shows XCTest completing 306/306, the CoreText marker-card test passing, and exactly the three VideoToolbox-encoding tests started but never finished. Serializing the suite keeps at most one encoder session alive at a time and costs well under a second locally. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: onevcat <onevcat@gmail.com>
…ayers Review follow-up (lycorp-jp#100): the CLI-layer @Flag declarations and the library layers each carried their own copy of the opt-in default, which is a drift hazard. The policy now has a single authoritative source — the --gif-markers flag on the three command surfaces — and ResolvedRecordingOptions, RecordingOutputPlan, GIFTranscoder, and AndroidRecordVideoCommand.record take the value as a required parameter, so an unwired caller is a compile error. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: onevcat <onevcat@gmail.com>
Review follow-up (lycorp-jp#100): cover the opt-in default and the true case on the top-level, iOS, and Android record-video surfaces, the top-level forwarder copy into the iOS subcommand, and the argument shape of AndroidRecordVideoCommand.record the Android forwarder compiles against. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: onevcat <onevcat@gmail.com>
Review follow-up (lycorp-jp#100): the primary skill entry for evidence GIFs only documented the bare recording invocation; the reference cheatsheet had picked up the flag but SKILL.md had not. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: onevcat <onevcat@gmail.com>
The unit-test job timed out twice on lycorp-jp#100 with zero forensics: the raw-log artifact was gated on failure(), which is false when a job is cancelled by its timeout — exactly the case where the partial log matters most. Upload on failure() || cancelled(), and bound the test step at 20 minutes (a healthy run is ~3) so a wedged swift test leaves the job time to publish the artifact instead of eating the 60-minute job timeout. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: onevcat <onevcat@gmail.com>
The unit-test job timed out twice on lycorp-jp#100 with zero forensics: the raw-log artifact was gated on failure(), which is false when a job is cancelled by its timeout — exactly the case where the partial log matters most. Upload on failure() || cancelled(), and bound the test step at 20 minutes (a healthy run is ~3) so a wedged swift test leaves the job time to publish the artifact instead of eating the 60-minute job timeout. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: onevcat <onevcat@gmail.com>
What
record-video --format gifnow opens with a START card and closes with an END card (~1 s each), on all three surfaces.Opt-in via
--gif-markers— the default output remains a faithful capture of the screen.Why
Team feedback on real repro clips from #84: a forever-looping GIF has no visible boundary — viewers can't tell where the flow starts and ends.
Design
naturalSize, which matches decoded frames for sim-use's own identity-transform recordings), centered white label on a dark background.overlay|color|image=...) later.ResolvedRecordingOptions/RecordingOutputPlanlike the other GIF options, so the surfaces cannot drift.Demo
Testing
make testfully green (862), including new cases: opt-in frame count and marker delays, card rendering (dark background + bright label pixels), default-off parity with the previous output, and the default inResolvedRecordingOptions.🤖 Generated with Claude Code