feat(engine): add lockGopForChunkConcat option to buildEncoderArgs#766
Conversation
64d6c02 to
ee866de
Compare
vanceingalls
left a comment
There was a problem hiding this comment.
Strengths:
- Default-off gating preserves byte-identical in-process PSNR baselines (
chunkEncoder.ts:173) — the right shape for a Phase-2 land that has to coexist with production renders. - Test coverage hits all four GPU paths + vp9 + prores no-ops (
chunkEncoder.test.ts:548-680), HDR + closed-GOP coexistence, and thegopSizevalidation throw on missing/invalid input. joinParamshelper atchunkEncoder.ts:223correctly handles emptygopParamsso the unlocked path stays byte-identical to the pre-PR-x264-params/-x265-paramsstring. The pin on this is good defensive engineering.
Findings:
important (follow-up): chunkEncoder.ts:439 encodeFramesChunkedConcat already does in-process ffmpeg -f concat -c copy of per-chunk encodes but does NOT pass lockGopForChunkConcat: true. By this PR's own justification ("Without these, libx264 / libx265 emit open-GOP frames with mid-chunk scenecut keyframes; the first frame of each chunk isn't an independently-decodable IDR and concat-copy playback freezes at chunk seams on some decoders" — chunkEncoder.ts:170-172), this is the exact contract the in-process chunk-concat path needs. It is not a regression introduced by this PR — the in-process path has always been open-GOP, and gating the new flag default-off was an explicit goal here. But since encodeStage.ts:150 calls this path in production today, the silent-freeze risk the PR describes already applies to in-process chunk-concat output. Follow-up ticket: either (a) opt encodeFramesChunkedConcat into lockGopForChunkConcat once a PSNR-baselines re-pin can be staged, or (b) document why the in-process path is fine without it (different decoder reach? smaller chunk counts?). Not blocking this PR.
nit: chunkEncoder.ts:188 gop = Math.floor(options.gopSize) after validating Number.isFinite(options.gopSize) && options.gopSize > 0 — if the caller passes gopSize: 1.7, this silently floors to 1. Probably fine in practice (gopSize is always set to an integer chunkSize), but a Number.isInteger check in the validator would be a tighter contract.
nit: chunkEncoder.types.ts:24-27 jsdoc says "Default false" but gopSize is in the same paragraph as a separately-numbered field — read once it's fine, but breaking lockGopForChunkConcat and gopSize into two adjacent @property-style sections would scan faster.
CI: The Format / Render on windows-latest / Tests on windows-latest failures shown on this PR are all pre-existing on main (the v0.6.2 release commit on main shows the same three failures: gh api repos/heygen-com/hyperframes/commits/main/check-runs confirms). The Format failure is packages/core/package.json + packages/shader-transitions/package.json formatting drift unrelated to this PR's diff (engine only). The Windows producer build is a pre-existing esbuild module-resolution issue at D:\a\hyperframes\hyperframes\packages\producer\build.mjs. None gate this PR on its merits — flagging here so the ❌ in James's Slack post doesn't mislead the rest of the stack.
Verdict: APPROVE
Reasoning: Default-off gate keeps in-process byte-identical; tests cover GPU/SW × codec × HDR × preset matrix; the one Rule-2 contract gap (in-process encodeFramesChunkedConcat doesn't opt in) is pre-existing and a follow-up, not a regression.
— Vai
Part of Phase 2 of the distributed rendering plan (determinism hardening). See DISTRIBUTED-RENDERING-PLAN.md §7.1 and §17.2 (gating table). Adds two optional fields to EncoderOptions: lockGopForChunkConcat?: boolean // default false gopSize?: number // required when lockGopForChunkConcat=true When the flag is true on the SW libx264 / libx265 paths, buildEncoderArgs emits closed-GOP / forced-keyframe args so the resulting chunk file can be losslessly concatenated (`ffmpeg -f concat -c copy`) with sibling chunks: -g <gopSize> -keyint_min <gopSize> -sc_threshold 0 -force_key_frames "expr:eq(mod(n,<gopSize>),0)" -x264-params "...:scenecut=0:open-gop=0:repeat-headers=1" -x265-params "keyint=<gopSize>:min-keyint=<gopSize>:scenecut=0:open-gop=0:repeat-headers=1" -bf 0 (added for h265 too when locked) GPU encoders, vp9, and prores ignore the flag (their concat-copy story is separate — see plan §7.2 / §8). In-process behavior is unchanged: the default (false) path emits no new args. New unit tests pin both branches in packages/engine/src/services/ chunkEncoder.test.ts. This is part of a stack of 10 PRs; this is PR 1 of 10. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ee866de to
2d6372a
Compare

What
Adds a
lockGopForChunkConcat: boolean(defaultfalse) option plus a requiredgopSize: numberinput tobuildEncoderArgsinpackages/engine/src/services/chunkEncoder.ts. Whentrue, the SW libx264 / libx265 paths emit closed-GOP / forced-keyframe args so the resulting chunk file can be losslessly concatenated (ffmpeg -f concat -c copy) with sibling chunks.The new args, per DISTRIBUTED-RENDERING-PLAN.md §7.1:
GPU encoders, vp9, and prores ignore the flag.
Why
Part of Phase 2 of the distributed rendering plan (determinism hardening). Distributed chunk workers each produce one mp4 chunk; the assemble step
ffmpeg-concats them with-c copy. That requires each chunk to start on an independently-decodable IDR keyframe and the encoder's internal GOP structure to land exactly on chunk boundaries — open-GOP and scenecut-driven keyframes break concat-copy at chunk seams.The gate keeps the in-process renderer's PSNR baselines byte-identical (in-process callers never pass
lockGopForChunkConcat=true).How
lockGopForChunkConcat?: booleanandgopSize?: numberthroughEncoderOptions.buildEncoderArgs, conditionally pushed the-g/-keyint_min/-sc_threshold/-force_key_framesflags and joined thescenecut=0:open-gop=0:repeat-headers=1controls onto the existing-x264-params/-x265-paramsstring (so anti-banding + HDR mastering tuning is preserved).-bf 0for h265 too when locked (closed-GOP concat-copy doesn't tolerate B-frames; the h264 branch already emits this unconditionally).gopSize > 0at runtime when the flag is on.The 90000 video_track_timescale and
-bf 0for h264 were already pinned by the existing function — no duplication.Test plan
chunkEncoder.test.tsgains 10 cases covering the default (no new args),lockGopForChunkConcat=truewith libx264 / libx265, ultrafast preset, GPU no-op, vp9 no-op, prores no-op, missing/invalidgopSizethrows, and HDR + closed-GOP coexistence.bun run --cwd packages/engine test— all 59 chunkEncoder tests + 591 engine tests pass.bun run --cwd packages/engine typecheckclean.bunx oxlint+bunx oxfmt --checkclean.This is part of a stack of 10 PRs; this is PR 1 of 10. Companion PRs: #767–#775.
🤖 Generated with Claude Code