Repository navigation
Conversation
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
left a comment
There was a problem hiding this comment.
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-packtogether with--resolutioncrops the wrong region.resolveCropPackPlan(packages/cli/src/commands/render/plan.ts:545) takes the frame size from the authoreddata-width/data-height, andplanCropPackMembers(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--resolutionis 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 verticalgetscrop=608:1080:656:0on 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 callextractVideoMetadatainencodeCropPackMember), 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.
Preview approval requiredThis pull request comes from a fork, so Mintlify won't build a preview until someone with write access approves commit |
|
Thanks, the 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, 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 |
miguel-heygen
left a comment
There was a problem hiding this comment.
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 (
yuv420pinstead ofyuva420pwith 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.
|
Agreed, a second copy of the encoder args was always going to drift. In 574d32d the sibling args are built from On your three cases:
I checked all three with real ffmpeg using the args |
miguel-heygen
left a comment
There was a problem hiding this comment.
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.
|
Good catch on the odd sizes. Pushed 4717506: 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
left a comment
There was a problem hiding this comment.
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:
-
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. -
Not from this commit, but I noticed it while tracing the callers:
cropPackListinginpackages/cli/src/commands/render.tsstill works out sizes from the composition'sdata-width/data-height, while the producer now plans against the master's probed size. With--resolution 4kon 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.
|
Both are done in 2d46566.
Checked locally: engine 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
left a comment
There was a problem hiding this comment.
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.
|
Done in 7f2daf0. When the host probe fails, the Docker listing now plans against the requested Checked locally: CLI I'll keep holding the rebase until the API decision. |
miguel-heygen
left a comment
There was a problem hiding this comment.
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.
|
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. |
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-criticalhas to stay inside every requested window.What changed
data-safe-frameson the composition root is a JSON array of named windows (id,ratio, andx/y/width/heightas fractions of the frame). A composition with nodata-safe-framesis 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 aslaunch.vertical.mp4andlaunch.square.mp4. An unknown id fails before capture. GIF and png-sequence ignore the flag.data-safe-criticalmarks an element that must stay inside every window (or only the named ones).hyperframes checkreportssafe_critical_overflowwhen a sampled box leaves its window. Static lint covers bad JSON, bad ids, bad rects, and a critical mark with no frames.<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.