Repository navigation
feat(studio): support GIF export with loop count - #5593
Conversation
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Thanks, this is a clean, well-scoped change. It widens every spot the issue lists, and the tests cover the route, the list/serve/delete path, and cancel cleanup, including the race where cancel lands after completion. One behavior gap needs fixing, and the branch needs a rebase.
Should fix: GIF fps is not capped in Studio
The CLI caps GIF output at 30 fps (packages/cli/src/commands/render/plan.ts:281-285) and records that it did (gifFpsCapped). The producer does not cap it. Studio still offers 60 fps for GIF (RenderQueue.tsx FPS_OPTIONS), and neither routes/render.ts nor the adapters clamp it. So a Studio GIF at 60 fps is encoded with ~1.67 cs frame delays. Browsers stretch delays below 2 cs to about 10 cs, so the GIF plays far too slowly, including in Studio's own <img> preview. That also misses the issue's "same frame count as the CLI" check. The format tooltip already promises "up to 30 fps" (renderSettings.ts FORMAT_INFO.gif), but nothing enforces it.
Suggested fix: clamp fps to 30 for format === "gif" on the server, in routes/render.ts next to the fps parse, so API callers get the cap too. In the UI, hide or disable the 60 fps option when GIF is selected. A route test that sends { format: "gif", fps: 60 } and expects fps 30/1 in startRender would lock it in. Optionally, pass a gifFpsCapped flag through to Studio render telemetry, as the CLI does. This PR removes the comment in studioRenderTelemetry.ts that explained why that field was absent.
Rebase needed
The PR currently conflicts with main. The only conflict is packages/cli/src/server/studioServer.test.ts: your two GIF tests and a CPU-sampler test added on main were appended at the same spot. Keep both sides. studioRenderTelemetry.ts and studioServer.ts merge cleanly.
Checked and fine
gifLoopvalidation (helpers/encoderOptions.ts) rejects non-integers and values outside 0 to 65535 with a 400, and ignores the field for non-GIF formats. The tests cover both cases.- Both adapters forward
gifLooponly when set, and their extension regexes now include.giffor the sidecar and cancel cleanup. A completed GIF writesout.meta.jsonand doesn't clobber the output. - GPU encoding stays MP4-only (
startConfiguredRenderforcesgpu: falsefor GIF, and the route already gates onformat === "mp4"). - I verified the PR's claim that the list, serve, and delete paths recognize
.gif. They go throughRENDER_MIME/RENDER_EXTENSIONS, and the job-id strip regex was widened.
What I ran: read the full changed files, checked git merge-tree against current main, and compared against the CLI's GIF path. I did not run the tests locally. CI hasn't run on this fork PR yet.
— Rames
90d0ad7 to
ad9ce86
Compare
Thanks for the review! Addressed the GIF frame-rate issue in ad9ce86: the server now caps GIF requests at 30 fps, and Studio disables the 60 fps option, including handling previously saved settings. Added regression tests and verified Studio/CLI output parity for a 60 fps request. |
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Re-reviewed at ad9ce86. The frame-rate fix is what was asked for. The route clamps GIF requests above 30 fps to 30/1 right after the fps parse, so API callers get the cap too, and the table test covers 60, 59.94 and just-over-30 being clamped, 29.97/30/24 passing through, and the other formats untouched. In the UI, 60 fps is disabled for GIF and a saved 60 resolves to 30. I also checked that every render path on the merge result (route MIME and extension lists, the job-id strip, both adapters' sidecar and cancel cleanup) now handles .gif.
Before this can merge:
- A small rebase.
mainadded motion blur tests topackages/studio/vite.adapter.render.test.tsat the same spot as yours, so that file conflicts. Keep both sides. The source files merge cleanly. - Before/After captures in the description. The required "Studio and player captures" check reads the PR body, and since this changes Studio
.tsxfiles it needs## Beforeand## Aftersections, each with an image or video (for example the format picker with GIF and the loop count field, and a finished GIF in the render list). Without them that check fails once CI runs. CI hasn't run on this fork yet.
One smaller thing, from reading the code rather than running it: with the CLI's Studio server, renders are written to <project>/renders, and project history walks that folder. History hashes video and audio in place but copies other files under 64 MiB into its own store (packages/studio-server/src/history/uncopied.ts). .mp4/.webm/.mov are in that media list and .gif isn't, so each GIF export would also be copied into the project's history. It might be worth leaving render outputs out of history, or treating them as uncopied. It's fine as a follow-up if the maintainers prefer.
Verdict: COMMENT
Reasoning: The requested fps cap is correctly done and tested, but the branch needs a one-file rebase and the description needs Before/After captures for a required check; CI has not run on this head.
|
We want GIF export. Please rebase on main and add Before/After captures, then we'll review again. |
ad9ce86 to
1d74ae5
Compare
Preview approval requiredThis pull request comes from a fork, so Mintlify won't build a preview until someone with write access approves commit |
|
Thanks for the re-review! Rebased onto current main, keeping both the motion-blur tests and the GIF tests. The required Before and After captures are now in the PR description. I also ran the built app in Chrome, selected GIF with loop count 3, clicked Export, and downloaded this actual output:
Verified: 640 x 360, 3 seconds, 90 frames at 30 fps, loop count 3; all 90 frames contain distinct motion, the preview loads, and downloaded bytes match the saved render. Before/After captures and evidence. For the history concern, current main already excludes the Latest commit: 1d74ae5. Ready for another review when convenient; please approve any waiting fork workflows so CI can run. Thank you! |
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
CI has now finished at this head. The Studio and player captures check passes on the run that carries the edited description (an older red result from before the edit is still listed beside it), and the other checks are green, including the ones that were cancelled earlier.
Everything else is as in my last review: the rebase is clean, the Before/After captures are in the description, GIF renders stay out of project history, and the new tests fail when the code is broken. Whether GIF export is something to add is for the maintainers to decide.
Verdict: APPROVE
Reasoning: All earlier points are fixed and the tests catch regressions; checks are green at this head apart from one superseded captures run.
Merge 8 upstream commits (85dcc19..7d6c02f: heygen-com#6418, heygen-com#5273, heygen-com#6245, heygen-com#5593, heygen-com#5447, heygen-com#6415, heygen-com#6373, heygen-com#6375). Keep the fork's Windows-only ci.yml. For cancellableProcess.ts take upstream's heygen-com#5273, which supersedes the fork's own 744a620 port (same StringDecoder design, but the output limit counts raw bytes). Record the batch in docs/DECISIONS.md, mark heygen-com#6373/heygen-com#6375/heygen-com#6415/heygen-com#6418 as merged, and advance the commit watermark to 7d6c02f. Co-Authored-By: Claude Sonnet 5.5 <[email protected]>
What
Add GIF export to Studio, including configurable loop counts, image previews, downloads, deletion, and cancellation cleanup.
Why
GIF rendering already works through the CLI, but Studio only offers MP4, WebM, and MOV. This lets Studio users export GIFs for documentation, READMEs, and chat without switching to the terminal.
Related work
Fixes #5552.
No overlapping PRs were found during the initial issue review.
How
Added GIF to Studio’s format picker and render API.
Added optional gifLoop validation: integers from 0 to 65535, with 0 meaning infinite looping.
Forwarded GIF settings through both server adapters to the existing producer.
Updated render history, MIME types, file serving, deletion, and metadata paths to recognize .gif.
Used image previews for completed GIFs.
Ensured cancellation removes both the GIF output and its metadata sidecar, including cancellation racing with completion.
Extracted render UI components and shared test helpers to meet repository size, complexity, and duplication checks.
Test plan
How was this tested?
Before
Built from upstream main
a11280eef: Studio offers MP4, MOV, and WebM, with no GIF export option.After
Built from PR commit
1d74ae52b: GIF selected with loop count 3, a 30 fps limit, and the completed GIF in the render list.Actual GIF exported from Studio
Chrome automation clicked Export in Studio and downloaded this file. FFprobe reports 640 x 360, 3 seconds, 90 frames; the Netscape extension records loop count 3. All 90 decoded frames differ, the image preview loads, downloaded bytes match the saved file, and the metadata sidecar is separate. Machine-readable evidence.
Rebased onto current main with both upstream motion-blur tests and GIF tests retained. Added a regression test confirming
renders/*.gifis excluded from project history by current main, and documented Studio GIF export. Local verification: workspace build/typechecks, lint/format and all pre-commit hooks; 1,509 server tests, 53 Studio render tests, and 63 CLI tests passed. Mintlify validation and broken-link checks passed. This is local verification; GitHub CI still needs to complete.