Skip to content

Add safe-frame crop packs - #4933

Closed
mvanhorn wants to merge 6 commits into
heygen-com:mainfrom
mvanhorn:fix/safe-frame-crop-packs
Closed

mvanhorn wants to merge 6 commits into
heygen-com:mainfrom
mvanhorn:fix/safe-frame-crop-packs

Conversation

@mvanhorn

@mvanhorn mvanhorn commented Oct 3, 2026

Copy link
Copy Markdown
Contributor

demo

A landscape composition can name crop windows, such as 9:16 and 1:1. One encode writes the master plus a sibling file for each window. Studio can overlay those windows, and the player can clip to one. Copy marked data-safe-critical has to stay inside every requested window.

What changed

  • data-safe-frames on the composition root is a JSON array of named windows (id, ratio, and x / y / width / height as fractions of the frame). A composition with no data-safe-frames is unchanged.
  • hyperframes render --crop-pack all (or a comma-separated list of ids) captures the master as usual, then crops that encode into siblings such as launch.vertical.mp4 and launch.square.mp4. An unknown id fails before capture. GIF and png-sequence ignore the flag.
  • data-safe-critical marks an element that must stay inside every window (or only the named ones). hyperframes check reports safe_critical_overflow when a sampled box leaves its window. Static lint covers bad JSON, bad ids, bad rects, and a critical mark with no frames.
  • Studio has a preview toggle that draws labeled rectangles over the composition. Those overlays stay in Studio and are not part of the composition document.
  • <hyperframes-player safe-frame="vertical"> clips the view to that window. An unknown id leaves the full frame.

Tests

Parser, lint, crop-pack engine and producer, CLI plan and check, Studio overlay, and player tests passed on this branch.

Named data-safe-frames windows crop the encoded master via --crop-pack.
Studio overlays and the player safe-frame attribute preview those windows.

No Co-authored-by: trailers.

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the detailed write-up and demo. Reviewed at 22da764.

Where it stands: the branch is about 1,800 commits behind main and now conflicts in 27 files (the CLI render plan and execute path, check pipeline, project config, parsers and engine exports, lint rules, player, producer render request and orchestrator, Studio preview overlays, skills manifest and docs). The fork's CI has not run yet. Nothing on main does this yet, so the idea isn't superseded.

Before you spend time on a rebase, this needs a maintainer decision on the API. It adds two composition attributes, a CLI flag and config key, a new parsers subpath export, a player attribute, a Studio overlay and a new check rule. That is a lot of public surface to commit to at once. If the direction is accepted, splitting it would make it much easier to review. For example: (1) the data-safe-frames parser plus lint, (2) render --crop-pack, (3) the Studio overlay and the player attribute.

One correctness issue in the render path, worth fixing in any version:

  • --crop-pack together with --resolution crops the wrong region. resolveCropPackPlan (packages/cli/src/commands/render/plan.ts:545) takes the frame size from the authored data-width/data-height, and planCropPackMembers (packages/engine/src/utils/cropPack.ts:40) converts the window to pixels against that size. But the crop runs on the encoded master, which is at the supersampled size when --resolution is set (as the new 4K guide section says). Example: a 1920x1080 composition with a centered full-height 9:16 window rendered with --resolution 4k --crop-pack vertical gets crop=608:1080:656:0 on a 3840x2160 master. That is a quarter-size patch from the upper left of the intended window. The fix is to scale the rect to the master's probed size (you already call extractVideoMetadata in encodeCropPackMember), plus a test for this combination.

Minor: the siblings are re-encoded with encoder defaults (cropPack.ts:59), so they ignore the render's quality setting, and VP9's default bitrate is low. Matching the master's encode settings would keep the siblings from looking worse than the master.

Verdict: COMMENT
Reasoning: The branch conflicts heavily and the new public API needs a maintainer decision first; the crop/resolution bug above should be fixed in whatever version goes forward.

With --resolution the encoded master is supersampled past the authored
data-width/data-height, but the crop rect was planned in composition
pixels, so a 1920x1080 composition rendered at 4k cropped a quarter-size
patch from the upper left of the window. Plan the members against the
master's probed size (planCropPackMembersForMaster), falling back to the
composition size when the probe has no dimensions.

Siblings also re-encoded with encoder defaults, ignoring the render's
quality and leaving VP9 at its low default bitrate. Pass the master's
preset, CRF and bitrate through to each member: x264 preset with CRF or
bitrate for mp4, VP9 constant quality for webm, the ProRes profile for mov.
@mintlify

mintlify Bot commented Oct 25, 2026 •

Copy link
Copy Markdown

Preview approval required

This pull request comes from a fork, so Mintlify won't build a preview until someone with write access approves commit 7f2daf0.

Approve preview

@mvanhorn

Copy link
Copy Markdown
Contributor Author

Thanks, the --resolution case was a real bug. In 7e51fc6 the producer plans the crop pack against the master's probed size instead of the authored data-width/data-height (new planCropPackMembersForMaster in cropPack.ts, falling back to the composition size only when the probe has no dimensions). Your example now crops at x=1312 with the full 2160 height on the 3840x2160 master, and there's a test for that combination.

I took the encode note too: the stage now passes the master's preset, CRF and bitrate to each sibling, so mp4 uses the same x264 preset and CRF (or bitrate), webm uses VP9 constant quality instead of the default bitrate, and mov keeps the ProRes profile.

On that commit, oxfmt --check, oxlint on engine and producer, tsc --noEmit for engine, producer and cli, fallow audit against the branch base, and vitest for the engine utils and the crop pack stage all pass locally.

I pushed it on the existing base and didn't rebase yet, per your note about waiting for the API decision. If the direction is accepted, I'm happy to split it the way you outlined (parser plus lint, then render --crop-pack, then the Studio overlay and player attribute), each on current main.

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, the --resolution fix looks right. Planning against the master's probed size is the right place for it, and the 4K test pins the case.

Thanks also for passing the quality settings through. One follow-up on that: the sibling encode args are still a separate hand-written copy of the master's (videoEncodeArgs in cropPack.ts vs buildEncoderArgs in chunkEncoder.ts), and they differ in ways that show up in playback:

  • mp4 siblings keep B-frames, which the master turns off (-bf 0) because the negative DTS makes some players freeze on the first frame. They also lack the master's BT.709 color tags.
  • A transparent webm master produces an opaque sibling (yuv420p instead of yuva420p with the alpha flag).
  • An HDR mp4 master produces an 8-bit x264 sibling with no tone mapping, so colors will look off.

Building the sibling args from buildEncoderArgs with the master's options would keep the two from drifting. This isn't urgent while the API question is open.

Nothing else has changed on my side. This still needs a maintainer decision on the public API before a rebase is worth your time, and the split you outlined sounds good if the direction is accepted. CI hasn't run on this fork branch yet.

Verdict: COMMENT
Reasoning: The crop/resolution bug is fixed and tested; the API decision, rebase and a CI run are still pending, and the sibling encode settings still differ from the master's in a few visible ways.

The sibling args were a hand-written copy of buildEncoderArgs and drifted
from it: mp4 siblings kept B-frames and lacked BT.709 tags, a transparent
webm master produced an opaque sibling, and HDR masters had no path.

buildCropPackArgs now builds from buildEncoderArgs with the master's codec,
preset, quality, bitrate, pixel format, VP9 cpu-used and HDR options, then
drops the master's pc-to-tv range conversion (the master is already TV
range) and -avoid_negative_ts. Alpha webm masters are decoded with libvpx so
the alpha plane survives. HDR masters encode as 10-bit HEVC with the
master's HDR tags when HDR settings are passed, and still hard-error
without them.
@mvanhorn

Copy link
Copy Markdown
Contributor Author

Agreed, a second copy of the encoder args was always going to drift. In 574d32d the sibling args are built from buildEncoderArgs with the master's options: the orchestrator now passes the preset's codec, pixel format and HDR options plus the effective CRF or bitrate, fps and VP9 cpu-used, and cropPack.ts no longer has its own encoder table. From the built args it drops the -vf range conversion (the master is already TV range, so converting again would wash out the levels) and -avoid_negative_ts, then adds the crop filter, the stream maps and -map_metadata 0.

On your three cases:

  • mp4 siblings now get -bf 0, the BT.709 color tags and the same x264 params as the master.
  • Transparent webm: building from the master's yuva420p options was not enough on its own, because ffmpeg's native VP9 decoder drops the alpha plane when it reads the master. The master is now decoded with libvpx when the encode is alpha VP9. On a test master with 50% alpha, the sibling decodes back to alpha 127, where it was 255 before.
  • HDR: on 7e51fc6 an HDR master hard-errored in encodeCropPackMember rather than writing an 8-bit sibling (there was a test for that). With the master's HDR options passed through, the sibling is now 10-bit HEVC with the BT.2020/PQ tags, and it still hard-errors if HDR settings are missing.

I checked all three with real ffmpeg using the args buildCropPackArgs produces: h264 yuv420p bt709 with no B-frames and audio copied, vp9 with alpha, and hevc yuv420p10le smpte2084/bt2020, each 608x1080. The cropPack and chunkEncoder engine tests (101), the producer cropPackStage tests, tsc for engine and producer, oxlint and oxfmt pass. Understood on the API decision and the split. I'll hold the rebase until that's settled.

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this is the fix I was hoping for. Building the sibling args from buildEncoderArgs, with the same options the master's encoder gets, means the two can't drift apart again, and the alpha decode via libvpx is a good catch I hadn't spotted. Thanks also for the correction on HDR: you're right that the previous commit refused an HDR master rather than writing an 8-bit file, so my note there was wrong.

One small follow-up from the new lines. Dropping the master's -vf also drops its even-dimension pad, which is what keeps the master safe on odd sizes, and nothing on the crop side rounds the sibling size to even. For example, a 1:1 safe frame with height 0.123 on a 1080p master plans a 133x133 crop, and libx264 with yuv420p refuses it ("width not divisible by 2"). The old hand-written args had the same gap, so it isn't new, but rounding the planned output size down to even in outputSizeForCrop, with a test for an odd crop, would close it.

Otherwise nothing new from me. The public API still needs a maintainer decision before a rebase, and CI hasn't run on this fork branch yet.

Verdict: COMMENT
Reasoning: The encoder drift is fixed at its source; the API decision, rebase and a CI run are still pending, plus one small odd-size case on mp4 siblings.

The crop pass does not carry the master's even-dimension pad, so an
odd planned size (a 1:1 frame of height 0.123 on 1080p plans 133x133)
made libx264 with yuv420p refuse the sibling. outputSizeForCrop now
floors both sides to even, and the crop filter scales to that size.
@mvanhorn

Copy link
Copy Markdown
Contributor Author

Good catch on the odd sizes. Pushed 4717506: outputSizeForCrop now floors both output sides to even, so the crop filter scales an odd crop to an even size (your 1:1 example plans crop=133:133:0:0,scale=132:132). Added a parser test for the 133x133 case and an engine test that plans that frame on 1080p and checks the filter.

Locally the parsers safeFrames tests (15) and engine cropPack tests (14) pass, and the pre-commit lint, format and typecheck hooks pass. I'll hold the rebase and the split until there's a call on the API.

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, that's the fix I had in mind. Rounding in outputSizeForCrop covers every planner in one place, and the two tests pin the 133x133 case end to end.

Two small notes:

  1. When the crop is odd, the filter now scales the whole frame down by one pixel (crop=133:133:0:0,scale=132:132). That resample softens the sibling a little. Cropping one pixel less instead (cropping straight to 132x132 when the crop and output differ by less than 2 px) would keep pixels 1:1. Optional.

  2. Not from this commit, but I noticed it while tracing the callers: cropPackListing in packages/cli/src/commands/render.ts still works out sizes from the composition's data-width/data-height, while the producer now plans against the master's probed size. With --resolution 4k on a 1080p composition, the summary and the returned result report half the real width and height of each sibling. Building the listing from the producer's planned members would keep them in sync.

Nothing else new from me. The public API still needs a maintainer decision before the rebase, and CI hasn't run on this fork branch yet.

Verdict: COMMENT
Reasoning: The odd-size encode failure is fixed at its shared owner; the API decision, rebase and a CI run are still pending, plus one small size-reporting mismatch under --resolution.

…sting from the master

When even-rounding trims only one pixel, crop straight to the output size
and drop the scale filter so siblings keep the master's pixels 1:1.

The docker render path now builds its crop-pack listing with
planCropPackMembersForMaster, so `--resolution` renders report the
siblings' real size. A failed probe falls back to the composition size.
@mvanhorn

Copy link
Copy Markdown
Contributor Author

Both are done in 2d46566.

  1. When even-rounding trims only one pixel, the plan now crops straight to the output size (the 133x133 crop at 0,0 becomes crop=132:132:0:0), and the scale filter is dropped whenever the crop and output sizes match, so the sibling keeps the master's pixels 1:1. Crops that differ by 2 px or more still scale. The 133x133 test now pins crop=132:132:0:0, and a new test covers the case that still scales.

  2. The Docker path's cropPackListing now calls planCropPackMembersForMaster, the same planner the producer uses, so the printed summary, the telemetry and the returned result all take their sizes from the master's probed dimensions. If the probe fails, it falls back to the composition size instead of throwing after a successful render (test added).

Checked locally: engine cropPack tests (16), producer cropPackStage tests (4), CLI render tests (91), tsc for engine and cli, oxlint and oxfmt. I also ran the new filter through ffmpeg with libx264 yuv420p on a 1920x1080 source and got a 132x132 output.

I'll hold the rebase until the API decision. CI on this fork branch still needs a maintainer to approve the workflow run.

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, both are fixed nicely. Cropping straight to the even size keeps the siblings 1:1, and since the output never exceeds the crop, the trimmed rect can't leave the frame. It also drops the no-op scale=608:1080 from the common vertical case. Reusing planCropPackMembersForMaster for the Docker listing means the summary and the result both match what the producer wrote.

One small follow-up on the Docker path: it now runs ffprobe on the host, while Docker mode otherwise doesn't need FFmpeg installed there. On a machine without it, the probe fails, the listing falls back to the composition size, and a --resolution 4k render of a 1080p composition reports half-size siblings again, without any warning. When the probe fails and --resolution was set, leaving the sizes out (or using the requested resolution) would avoid reporting a guess. Not blocking.

The public API still needs a maintainer decision before the rebase, and CI hasn't run on this fork branch yet.

Verdict: COMMENT
Reasoning: Both earlier notes are fixed; one minor host-ffprobe fallback edge in the Docker listing, and the API decision, rebase and a CI run are still pending.

…e host can't probe

Docker mode doesn't need FFmpeg on the host, so the host probe of the master
can fail. With --resolution set, the listing now falls back to the requested
preset (matched to the composition's orientation for aspect-agnostic tiers)
instead of the composition size, so it doesn't report half-size siblings.
@mvanhorn

Copy link
Copy Markdown
Contributor Author

Done in 7f2daf0. When the host probe fails, the Docker listing now plans against the requested --resolution preset instead of the composition size, so a --resolution 4k render of a 1080p composition reports 3840x2160-based siblings without needing FFmpeg on the host. Aspect-agnostic tiers pick the preset that matches the composition's orientation, the same way the preflight does, and without --resolution it still uses the composition size. A successful probe still wins in every case. The fallback is a small exported helper, cropPackFallbackSize, with tests for all three cases.

Checked locally: CLI render.test.ts (78), tsc for cli, oxlint and oxfmt on the changed files.

I'll keep holding the rebase until the API decision.

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this closes the last note. Using the requested preset as the fallback is the right call: the preflight already guarantees that any render reaching the listing has a master of exactly the preset's size (matching aspect, whole-number scale), and cropPackFallbackSize resolves aspect-agnostic tiers with the same suggestMatchingPreset call as the preflight and the producer's compile stage, so the listing can't drift from what was written. A --resolution 4k render of a 1080p composition now lists 3840x2160-based siblings with or without FFmpeg on the host, and the no---resolution path is unchanged.

Nothing further from me on the code. The public API still needs a maintainer decision before the rebase, and CI hasn't run on this fork branch yet.

Verdict: COMMENT
Reasoning: The Docker fallback note is fixed correctly and matches the producer's sizing; no new issues, but the API decision, rebase and a CI run are still pending.

@jrusso1020

Copy link
Copy Markdown
Collaborator

Thanks for all the work here and the quick turnarounds. This adds more public API than we want to take in one go (new composition attributes, a CLI flag, a player attribute and a lint rule). Closing for now. A smaller PR, for example just the Studio safe-frame overlay, would be easy to review.

@jrusso1020 jrusso1020 closed this Oct 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants