Repository navigation
fix(cli): a re-encode of a media file over the cap is limited to fit it - #7376
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Verdict: REQUEST CHANGES · 2 blocker · 1 important · 0 nits · head 01af84cf
- blocker The build/typecheck fails at
offload.ts:409.preset.args.includes("libvpx-vp9")gives TS2345 ("libvpx-vp9"is not assignable tonever):argsis a const tuple union, soincludesnarrows tonever. This breaks the Typecheck, Build, CLI smoke (required) and Windows render jobs. Fix: comparepresetName === "alphaVideo", or widen with(preset.args as readonly string[]). - blocker The new ffmpeg test
offload.sizeLimit.test.ts"fits a noise clip that crf alone leaves over the cap" times out at 20 s on CI, all 4 tries (Flake gate). Fix: shrink the clip (shorter or smaller, with a cap to match) or give it an explicit timeout. - blocker (gates) Fallow audit:
limitForCRAP 31.6 > 30. Comment ratchet:offload.tscomment share went from 5.4% to 5.6% (+5 lines). - important
offload.ts:398: the limit applies only when the source itself is over the cap. A source under the cap in an efficient codec (for example 180 MiB HEVC or AV1, re-encoded to x264 crf 18) can still come out over 200 MiB and fail after the full encode, the same failure the PR targets. Consider a fallback for that case, or say in the body that it stays out of scope.
Scope
Read the diff and the ffmpegReencode/uploadPlanned call sites. The reuse cache is written only after a successful upload, so no over-cap output is cached under the unchanged key. The "fails without the fix" line is present.
…for the re-encode limit
Contributor
There was a problem hiding this comment.
Verdict: APPROVE — 3 resolved, 0 open, 0 new · 01af84cf → 19d1c7d9. Typecheck, flake gate (60 s timeout, 8 s clip), Fallow and comment ratchet all green; under-cap sources are now limited too. The trade-off that a very long video which used to fit can have its peaks clamped is stated in the body and accepted as a design call.
3 of 4 tasks
Merged
3 of 5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
hyperframes cloud renderre-encodes a media file the asset API caps, at crf 18. crf bounds quality, not size, so a source over the 200 MiB cap can come out of a long encode still over it, and the render then fails withMedia upload failed: ... is N MB after re-encoding at visually lossless quality, over the 200.0 MB a HeyGen asset can hold.The person waits through the whole encode and then gets the refusal.Why
The encode's size depends only on the quality target, so any re-encode can end over the cap and be refused after the whole encode. The cap is known before the encode starts, so the encode can be bounded by it.
How
Every video re-encode (x264, x265 and VP9) adds
-maxrateand-bufsizefor 85% of the cap over the clip's length (the trimmed length when only played segments are kept), less the preset's audio bitrate. crf stays the quality target, so the limit only bites where crf alone would exceed the budget; a source under the cap is covered too, because a file in an efficient codec can leave an x264 crf 18 encode larger than itself. On a very long video the one-second limit can also clamp a short peak that would have fit on average. VP9 (the alpha preset) takes the same rate as-b:v, because its-b:v 0constant-quality mode ignores-maxrate. Audio and images are unchanged.sizeLimitArgsincloud/offload.tsowns the arithmetic.The buffer is one second on purpose: an average-bitrate target (
-b:vat 95% of the cap with a 2 s buffer) overshot the cap on a 10 s noise clip (217 MiB against 200 MiB), so the limit is a one-second VBV cap on top of crf, not an average-bitrate target.Test plan
cloud/offload.sizeLimit.test.ts: the arithmetic (200 MiB over 100 s with 320 kbit/s audio gives 13940k) and no limit when audio fills the budget; a real ffmpeg run on an 8 s noise clip that crf 18 alone leaves over a 1 MiB cap and the limited encode fits;limitForlimits any video by its (or its cut's) length, leaves audio and images alone, and adds-b:vfor VP9. fails without the fix: withoutsizeLimitArgsthe file does not import, and with the limit removed from the args the second case is over the cap.All 16 files under
src/cloud/pass (180 tests), on Linux with ffmpeg.Real repro through the CLI source (bun, stub API, no key): a 10 s 1920x1080 noise mp4 of 513 MiB. On main the re-encode ran 131 s and failed with
302.4 MB after re-encoding ... over the 200.0 MB. On this branch the same project passes the size check (the run then stops at the stub API's upload, which has no upload URL), in 77 s.Unit tests added/updated
Manual testing performed
Documentation updated (not applicable)
Comments follow CONTRIBUTING.md "Comments"
Not exercised: the HDR (x265) and alpha (VP9) presets against a real over-cap source, a source whose audio is long and large, and Windows and macOS ffmpeg builds.