feat(cli): expose png-sequence format#654
Conversation
jrusso1020
left a comment
There was a problem hiding this comment.
Verdict
Approve. Clean, surgical fix to a real CLI gap — RenderConfig.format already accepted "png-sequence" in packages/producer/src/services/renderOrchestrator.ts:252 and the producer fully implements it (verified at lines 2703–2731 and 3763–3793, plus the e2e check in packages/producer/src/transparency-test.ts). The CLI's VALID_FORMAT set was the only thing gating the flag, so this is correctly scoped to the CLI's accept-and-forward surface plus the matching docs.
Verified locally:
bun run --filter '@hyperframes/cli' test→ 259/259 passbun run --filter '@hyperframes/cli' typecheck→ clean
A few non-blocking observations below — none worth holding the PR for.
Key Concerns
(none blocking)
Test Coverage
Coverage is appropriately scoped. The two forwarding paths (renderLocal → producer.createRenderJob, and buildDockerRunArgs → container CLI) are covered by independent unit tests, which is exactly the regression model the existing dockerRunArgs.test.ts "regression tripwire for silent drops" comment calls out.
Producer behaviour for png-sequence is already covered end-to-end by packages/producer/src/transparency-test.ts:145–184, so an additional CLI-level e2e is not needed for this PR.
Gap worth noting (non-blocking): there's no test asserting the auto-output path for png-sequence is renders/<name>_<date>_<time> (i.e. that the empty FORMAT_EXT["png-sequence"] flows correctly when --output is omitted). Easy to add if desired, but the change is mechanical enough to skip.
Nits / Future
packages/cli/src/commands/render.ts:79—description: "Output path (default: renders/<name>.mp4)"is mp4-specific. With--format png-sequenceand no--outputit's actuallyrenders/<name>_<date>_<time>(a directory, no extension). Could broaden to"Output path (default: renders/<name>.<ext>)"or note the directory case. Pure docs string.packages/cli/src/commands/render.ts:937—readdirSync(outputPath, { withFileTypes: true })+entry.isFile()only sums top-level files. Fine forpng-sequencetoday (producer writes flat:frame_NNNNNN.png+ optionalaudio.aac), but if any future format ever writes subdirectories the deliverable size will be silently undercounted. Worth a one-line comment that this assumes a flat directory, or a recursive walk if you want to future-proof.packages/cli/src/server/studioServer.ts:228,237— the metadata regex/\.(mp4|webm|mov)$/would silently no-op ifpng-sequenceever flowed through Studio. Studio'suseRenderQueue.ts:65still typesformatas"mp4" | "webm" | "mov", so this isn't reachable today and is not introduced by this PR — flagging only because exposingpng-sequencein Studio is the natural follow-up and that regex is the trap waiting at the end of it.- The PR description correctly calls out that no end-to-end render was performed against the patched binary. Producer side is unchanged so this is acceptable, agreed.
The producer already supports `format: "png-sequence"` end-to-end (see RenderConfig in renderOrchestrator.ts), but the CLI's VALID_FORMAT validator rejects it before the flag reaches the producer. Surface it the same way `mov` and `webm` are surfaced. Behaviour: - `--format png-sequence` accepted alongside mp4/webm/mov. - Auto-output path uses no extension (FORMAT_EXT["png-sequence"] = "") since the producer treats outputPath as a directory of frame_NNNNNN.png. - `printRenderComplete` sums the contained file sizes when outputPath is a directory, instead of reporting the platform-dependent inode size. - DockerRenderOptions.format type extended; existing buildDockerRunArgs is unchanged because it forwards the string verbatim. Tests: - renderLocal forwards `format: "png-sequence"` to createRenderJob. - buildDockerRunArgs propagates `--format png-sequence` to the container. Docs: - Rendering guide: format flag table, format comparison table, new "PNG sequence (no encoding)" section, "How it works" extended. - CLI package docs: format flag table updated.
eb9f3d6 to
4e28658
Compare
|
Force-pushed to fix the failing Typecheck job. Sorry for dismissing your approval — happy to walk through what changed: Why CI was red Main moved on while this PR was open. Specifically, 2221647 ("refactor(cli): unify RenderOptions on browserGpuMode tri-state") renamed RenderOptions.browserGpu: boolean → browserGpuMode?: "auto" | "hardware" | "software". The png-sequence test I added on this branch still used the old field name, which is what the original Typecheck failure surfaced. Drive-by fix on main While digging in I noticed main itself currently fails bun run --filter '@hyperframes/cli' typecheck — confirmed by checking out a clean origin/main worktree and running it. The composition-flag PR (#631, 0e0a0e4) landed after the rename and brought in two new tests (forwards entryFile to createRenderJob… and omits entryFile from createRenderJob…) that still use browserGpu: false. Those two errors are inherited by every PR currently rebased on main. This rebase fixes all three call sites (the one I added + the two pre-existing) by switching to browserGpuMode: "software". So this PR now also unbreaks main's typecheck. Happy to split the upstream-test fix into its own commit/PR if you'd prefer it isolated — let me know. Verification
Please re-review when you have a moment |
jrusso1020
left a comment
There was a problem hiding this comment.
@TheodorKleynhans no worries we auto dismiss approvals on changes to avoid back actors from pushing in malicious code after an approval
What
Surface the
png-sequenceoutput format in the CLI'srendercommand, alongside the existingmp4,webm, andmovformats.The CLI already advertises this format internally —
RenderConfig.formatinpackages/producer/src/services/renderOrchestrator.ts:252accepts"png-sequence"and the producer fully implements it (capture → writeframe_NNNNNN.pngto a directory, optionalaudio.aacsidecar). The CLI'sVALID_FORMATset just hadn't been updated to forward the flag, so--format png-sequencefailed validation withGot "png-sequence". Must be mp4, webm, or mov.before reaching the producer.Why
PNG sequences are the standard intermediate for ingest into After Effects, Nuke, and Fusion (and for any pipeline that wants to post-process frames before encoding). The producer was built to support this and the docs/README reference it — but the CLI surface was the missing piece. This makes the feature reachable from the documented
npx hyperframes renderentry point without callers needing to drop down to the Producer API.No producer logic changes. The CLI is the only thing that gates this format today.
How
packages/cli/src/commands/render.tsVALID_FORMATnow includes"png-sequence".FORMAT_EXT["png-sequence"] = ""— the auto-output path becomesrenders/<name>_<date>_<time>(no extension), which is the directory the producer writes into. Matches the producer's contract thatoutputPathis treated as a directory for this format.ascast on the validated format both extend the union to include"png-sequence".RenderOptions.formatextended to match.printRenderCompletewas reportingstatSync(outputPath).sizefor the deliverable. For a directory that's a platform-dependent inode size (typically 0 or 4 KiB), which would be misleading for a sequence that's tens of MiB on disk. WhenoutputPathis a directory, sum the sizes of contained files instead. Single-file outputs are unaffected.--format png-sequence --output frames/.packages/cli/src/utils/dockerRunArgs.tsDockerRenderOptions.formatextended to"mp4" | "webm" | "mov" | "png-sequence". The arg builder forwards the string verbatim, so no logic change is needed — the type-only update is enough to compile.Docs
docs/guides/rendering.mdx: format flag table, format comparison table (added aPNG sequencerow), new### PNG sequence (no encoding)section, "How it works" extended to mention the no-encoder path.docs/packages/cli.mdx: format flag table updated.Test plan
packages/cli/src/commands/render.test.ts: new testforwards format: png-sequence through to createRenderJobexercises therenderLocal→producer.createRenderJobpath with the new format.packages/cli/src/utils/dockerRunArgs.test.ts: new testforwards --format png-sequence to the containerconfirms the docker arg builder propagates the flag (the existing "regression tripwire for silent drops" test in this file specifically calls out catching cases like the historical lost--hdrflag).bun run --filter '@hyperframes/cli' typecheck→ cleanbun run --filter '@hyperframes/cli' test→ 259/259 passbun run lint(oxlint) → 0 warnings, 0 errorsoxfmt --checkon the changed files → cleanlefthookpre-commit hooks (lint, format, typecheck) andcommit-msg(commitlint) all passedformat: "png-sequence"write path inrenderOrchestrator.ts) is unchanged from what's already onmain; this PR's behaviour change is purely the CLI's accept-and-forward of the flag, which is covered by the unit tests above. Happy to add an end-to-end render fixture if that's preferred for this kind of change.