From b4f1929cf0a7f7450185f36b29be69d2a952bc6f Mon Sep 17 00:00:00 2001 From: Aditya Poudyal Date: Wed, 16 Sep 2026 16:07:49 -0700 Subject: [PATCH] feat(engine): lock GOP on every encoder path (#3973) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## What this does Video encoders normally choose their own keyframe positions. A keyframe is a frame that can be decoded on its own, and you can only cut a video cleanly at one. The engine already has a flag, `lockGopForChunkConcat`, that forces a keyframe every `gopSize` frames so chunk files can be joined with `ffmpeg -c copy`. Today that logic is copy-pasted in two places inside `buildEncoderArgs` and does not exist at all in the streaming encoder. This PR: 1. **Extracts the locked-GOP logic into three small shared helpers** in `chunkEncoder.ts`: `resolveLockedGopSize` (validation), `appendLockedGopArgs` (the `-g` / `-keyint_min` / `-sc_threshold` / `-force_key_frames` args), and `lockedGopCodecParams` (the x264/x265 param fragment). `buildEncoderArgs` now calls them instead of inlining the same block twice. 2. **Adds the same flag to the streaming encoder** (`StreamingEncoderOptions.lockGopForChunkConcat` + `gopSize`), which is the path most renders under four minutes actually use. It emits the identical args as the disk encoder, including `-bf 0` for h265 under a lock so B-frame reordering cannot reintroduce negative DTS at a boundary. ## What does not change - Any render that does not set the flag produces a byte-identical ffmpeg arg list. The flag defaults to `false` everywhere. - Nothing in the tree sets the flag yet except the distributed chunk worker, which already used the disk encoder. No user-facing behavior changes. - GPU encoders, VP9 and ProRes streaming encodes continue to ignore the flag, exactly as `EncoderOptions` documents. An earlier revision of this PR made GPU encoders honor it; that was dropped because the path is unreachable until HLS lands and was verified only on videotoolbox. ## Why Groundwork for `format: "hls"` in `@hyperframes/producer`. HLS packaging stream-copies the encoded video into fixed-length segments with `-f hls -c copy`, and the segmenter can only cut on keyframes the encoder already produced. Locking the GOP at encode time is what makes every segment land exactly on a 4-second boundary without re-encoding. The follow-up PR adds the format itself and should reject `--gpu` for `hls` in v1. ## Testing - `chunkEncoder.test.ts`: the three helpers tested directly; existing locked-GOP and GPU no-op tests unchanged. - `streamingEncoder.test.ts`: new `lockGopForChunkConcat` block covering default off, libx264, libx265, and invalid `gopSize`. - Engine typecheck, oxlint, oxfmt clean. Full engine suite matches main's baseline (the 4 `ffprobe.test.ts` HDR fixture failures are pre-existing on `main`). 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- .../engine/src/services/chunkEncoder.test.ts | 42 ++++++- packages/engine/src/services/chunkEncoder.ts | 109 ++++++++++-------- .../src/services/streamingEncoder.test.ts | 58 ++++++++++ .../engine/src/services/streamingEncoder.ts | 26 ++++- 4 files changed, 185 insertions(+), 50 deletions(-) diff --git a/packages/engine/src/services/chunkEncoder.test.ts b/packages/engine/src/services/chunkEncoder.test.ts index 695191721f..f21b977a53 100644 --- a/packages/engine/src/services/chunkEncoder.test.ts +++ b/packages/engine/src/services/chunkEncoder.test.ts @@ -3,7 +3,14 @@ import { mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "nod import { tmpdir } from "node:os"; import { dirname, join } from "node:path"; import { afterEach, describe, it, expect, vi } from "vitest"; -import { ENCODER_PRESETS, getEncoderPreset, buildEncoderArgs } from "./chunkEncoder.js"; +import { + ENCODER_PRESETS, + appendLockedGopArgs, + buildEncoderArgs, + getEncoderPreset, + lockedGopCodecParams, + resolveLockedGopSize, +} from "./chunkEncoder.js"; import { renderProvenanceArgs } from "../utils/renderProvenance.js"; const TINY_PNG = Buffer.from( @@ -1473,6 +1480,39 @@ describe("buildEncoderArgs lockGopForChunkConcat", () => { ).toThrow(/lockGopForChunkConcat=true requires a positive integer gopSize/); }); + it("resolveLockedGopSize floors, passes through null, and rejects bad sizes", () => { + expect(resolveLockedGopSize({})).toBeNull(); + expect(resolveLockedGopSize({ gopSize: 120 })).toBeNull(); + expect(resolveLockedGopSize({ lockGopForChunkConcat: true, gopSize: 120.7 })).toBe(120); + for (const bad of [undefined, 0, -10, NaN, Infinity]) { + expect(() => + resolveLockedGopSize({ lockGopForChunkConcat: true, gopSize: bad as number | undefined }), + ).toThrow(/lockGopForChunkConcat=true requires a positive integer gopSize/); + } + }); + + it("appendLockedGopArgs emits the closed-GOP quartet in order", () => { + const args: string[] = []; + appendLockedGopArgs(args, 120); + expect(args).toEqual([ + "-g", + "120", + "-keyint_min", + "120", + "-sc_threshold", + "0", + "-force_key_frames", + "expr:eq(mod(n,120),0)", + ]); + }); + + it("lockedGopCodecParams adds keyint only for h265", () => { + expect(lockedGopCodecParams("h264", 120)).toBe("scenecut=0:open-gop=0:repeat-headers=1"); + expect(lockedGopCodecParams("h265", 120)).toBe( + "keyint=120:min-keyint=120:scenecut=0:open-gop=0:repeat-headers=1", + ); + }); + it("true is a no-op on ProRes (intra-only — no GOP forcing needed)", () => { const args = buildEncoderArgs( { diff --git a/packages/engine/src/services/chunkEncoder.ts b/packages/engine/src/services/chunkEncoder.ts index aaa75d5376..512bffd06c 100644 --- a/packages/engine/src/services/chunkEncoder.ts +++ b/packages/engine/src/services/chunkEncoder.ts @@ -159,6 +159,60 @@ export function getEncoderPreset( // Re-export GPU utilities so existing consumers that import from chunkEncoder still work. export { detectGpuEncoder, type GpuEncoder } from "../utils/gpuEncoder.js"; +/** The `lockGopForChunkConcat` / `gopSize` pair, shared by every encoder entry point. */ +export interface LockedGopOptions { + lockGopForChunkConcat?: boolean; + gopSize?: number; +} + +/** + * Integer GOP length, or `null` when no lock was requested. Throws on a lock + * with an invalid size — a silent fallback ships open-GOP output that only + * surfaces later as a broken playback seam. + */ +export function resolveLockedGopSize(options: LockedGopOptions): number | null { + if (options.lockGopForChunkConcat !== true) return null; + if ( + typeof options.gopSize !== "number" || + !Number.isFinite(options.gopSize) || + options.gopSize <= 0 + ) { + throw new Error( + `[chunkEncoder] lockGopForChunkConcat=true requires a positive integer gopSize (received ${String(options.gopSize)})`, + ); + } + return Math.floor(options.gopSize); +} + +/** + * Closed-GOP / forced-keyframe args for libx264 / libx265, so an orchestrator + * can concat chunks with `-c copy` or cut the stream into segments with + * `-f hls -c copy`. Without them the encoder picks its own keyframes and a + * boundary may not land on an independently decodable IDR. + */ +export function appendLockedGopArgs(args: string[], gopSize: number): void { + args.push( + "-g", + String(gopSize), + "-keyint_min", + String(gopSize), + "-sc_threshold", + "0", + "-force_key_frames", + `expr:eq(mod(n,${gopSize}),0)`, + ); +} + +/** + * The `-x264-params` / `-x265-params` fragment that bakes the IDR cadence into + * the encoder itself — `-force_key_frames` alone still permits mini-GOPs with + * open-GOP references. `repeat-headers=1` keeps each boundary self-contained. + */ +export function lockedGopCodecParams(codec: "h264" | "h265", gopSize: number): string { + const shared = "scenecut=0:open-gop=0:repeat-headers=1"; + return codec === "h264" ? shared : `keyint=${gopSize}:min-keyint=${gopSize}:${shared}`; +} + export function buildEncoderArgs( options: EncoderOptions, inputArgs: string[], @@ -258,34 +312,10 @@ export function buildEncoderArgs( else args.push("-crf", String(quality)); // Closed-GOP / forced-keyframe args so an external orchestrator can - // ffmpeg-concat chunk files with `-c copy`. 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. - const lockGop = options.lockGopForChunkConcat === true; - let gop = 0; - if (lockGop) { - if ( - typeof options.gopSize !== "number" || - !Number.isFinite(options.gopSize) || - options.gopSize <= 0 - ) { - throw new Error( - `[chunkEncoder] lockGopForChunkConcat=true requires a positive integer gopSize (received ${String(options.gopSize)})`, - ); - } - gop = Math.floor(options.gopSize); - args.push( - "-g", - String(gop), - "-keyint_min", - String(gop), - "-sc_threshold", - "0", - "-force_key_frames", - `expr:eq(mod(n,${gop}),0)`, - ); - } + // ffmpeg-concat chunk files with `-c copy`. See `appendLockedGopArgs`. + const gop = resolveLockedGopSize(options); + const lockGop = gop !== null; + if (gop !== null) appendLockedGopArgs(args, gop); // Disable B-frames. Standard h264 with B-frames produces negative DTS // at the start of the stream (the first B-frame's decode order is @@ -322,11 +352,7 @@ export function buildEncoderArgs( codec === "h265" && options.hdr ? getHdrEncoderColorParams(options.hdr.transfer).x265ColorParams : "colorprim=bt709:transfer=bt709:colormatrix=bt709"; - let gopParams = ""; - if (lockGop) { - const shared = "scenecut=0:open-gop=0:repeat-headers=1"; - gopParams = codec === "h264" ? shared : `keyint=${gop}:min-keyint=${gop}:${shared}`; - } + const gopParams = gop !== null ? lockedGopCodecParams(codec, gop) : ""; const joinParams = (...parts: string[]): string => parts.filter((p) => p.length > 0).join(":"); if (preset === "ultrafast") { @@ -354,19 +380,10 @@ export function buildEncoderArgs( // displayable reference when alt-ref is on. The shared `vp9CpuUsed` // option pins speed/quality against libvpx-vp9 default drift across // versions for both chunked and streaming WebM encodes. - const lockGopVp9 = options.lockGopForChunkConcat === true; - if (lockGopVp9) { - if ( - typeof options.gopSize !== "number" || - !Number.isFinite(options.gopSize) || - options.gopSize <= 0 - ) { - throw new Error( - `[chunkEncoder] lockGopForChunkConcat=true requires a positive integer gopSize (received ${String(options.gopSize)})`, - ); - } - const gop = Math.floor(options.gopSize); - args.push("-g", String(gop), "-keyint_min", String(gop), "-auto-alt-ref", "0"); + const vp9Gop = resolveLockedGopSize(options); + const lockGopVp9 = vp9Gop !== null; + if (vp9Gop !== null) { + args.push("-g", String(vp9Gop), "-keyint_min", String(vp9Gop), "-auto-alt-ref", "0"); } if (pixelFormat === "yuva420p") { // Alpha + alt-ref is unsupported by libvpx-vp9. The closed-GOP diff --git a/packages/engine/src/services/streamingEncoder.test.ts b/packages/engine/src/services/streamingEncoder.test.ts index 2892918740..8cb2bc1766 100644 --- a/packages/engine/src/services/streamingEncoder.test.ts +++ b/packages/engine/src/services/streamingEncoder.test.ts @@ -363,6 +363,64 @@ describe("buildStreamingArgs", () => { expect(args[vfIdx + 1]).toBe("scale=in_range=pc:out_range=tv,format=nv12,hwupload"); }); }); + + // The HLS format locks the GOP so `-f hls -c copy` can cut segments on + // exact time boundaries. Every other format must keep the old arg list. + describe("lockGopForChunkConcat", () => { + it("omits closed-GOP args by default", () => { + const args = buildStreamingArgs(baseSdr, "/tmp/out.mp4"); + expect(args).not.toContain("-g"); + expect(args).not.toContain("-keyint_min"); + expect(args).not.toContain("-sc_threshold"); + expect(args).not.toContain("-force_key_frames"); + const paramIdx = args.indexOf("-x264-params"); + expect(args[paramIdx + 1]).not.toContain("open-gop=0"); + }); + + it("emits closed-GOP args and x264-params for libx264", () => { + const args = buildStreamingArgs( + { ...baseSdr, lockGopForChunkConcat: true, gopSize: 120 }, + "/tmp/out.mp4", + ); + expect(args[args.indexOf("-g") + 1]).toBe("120"); + expect(args[args.indexOf("-keyint_min") + 1]).toBe("120"); + expect(args[args.indexOf("-sc_threshold") + 1]).toBe("0"); + expect(args[args.indexOf("-force_key_frames") + 1]).toBe("expr:eq(mod(n,120),0)"); + const paramIdx = args.indexOf("-x264-params"); + expect(args[paramIdx + 1]).toContain("scenecut=0"); + expect(args[paramIdx + 1]).toContain("open-gop=0"); + expect(args[paramIdx + 1]).toContain("repeat-headers=1"); + expect(args[paramIdx + 1]).toContain("aq-strength=0.8"); + expect(args[args.indexOf("-bf") + 1]).toBe("0"); + }); + + it("emits keyint in x265-params and disables B-frames for libx265", () => { + const args = buildStreamingArgs( + { + ...baseSdr, + codec: "h265", + lockGopForChunkConcat: true, + gopSize: 90, + }, + "/tmp/out.mp4", + ); + expect(args[args.indexOf("-g") + 1]).toBe("90"); + expect(getX265ParamsValue(args)).toContain("keyint=90"); + expect(getX265ParamsValue(args)).toContain("min-keyint=90"); + expect(args[args.indexOf("-bf") + 1]).toBe("0"); + }); + + it("throws on a missing or invalid gopSize", () => { + for (const bad of [undefined, 0, -10, NaN, Infinity]) { + expect(() => + buildStreamingArgs( + { ...baseSdr, lockGopForChunkConcat: true, gopSize: bad as number | undefined }, + "/tmp/out.mp4", + ), + ).toThrow(/lockGopForChunkConcat=true requires a positive integer gopSize/); + } + }); + }); }); describe("createFrameReorderBuffer", () => { diff --git a/packages/engine/src/services/streamingEncoder.ts b/packages/engine/src/services/streamingEncoder.ts index 07de984469..56257d91c0 100644 --- a/packages/engine/src/services/streamingEncoder.ts +++ b/packages/engine/src/services/streamingEncoder.ts @@ -40,6 +40,8 @@ import { fpsToFfmpegArg, fpsToNumber, type Fps } from "@hyperframes/core"; import { appendVp9CpuUsedArg } from "./vp9Options.js"; import { appendRenderProvenanceArgs } from "../utils/renderProvenance.js"; +import { appendLockedGopArgs, lockedGopCodecParams, resolveLockedGopSize } from "./chunkEncoder.js"; + // Re-export EncoderOptions so callers can reference the type via this module. export type { EncoderOptions } from "./chunkEncoder.types.js"; @@ -154,6 +156,16 @@ export interface StreamingEncoderOptions { hdr?: { transfer: import("../utils/hdr.js").HdrTransfer }; /** When set, use rawvideo input instead of image2pipe. For HDR PQ-encoded frames. */ rawInputFormat?: "rgb48le"; + /** + * Force an IDR keyframe every `gopSize` frames. Set by the HLS format so + * `-f hls -c copy` can cut segments on exact time boundaries — the segmenter + * only splits at keyframes the encoder already emitted. Default `false` + * leaves the arg list byte-identical. libx264 / libx265 only (GPU encoders, + * VP9 and ProRes ignore it); see `EncoderOptions.lockGopForChunkConcat`. + */ + lockGopForChunkConcat?: boolean; + /** Required when `lockGopForChunkConcat` is `true`. Frames per GOP. */ + gopSize?: number; } export interface StreamingEncoderResult { @@ -320,11 +332,18 @@ export function buildStreamingArgs( if (bitrate) args.push("-b:v", bitrate); else args.push("-crf", String(quality)); + // Same closed-GOP lock as chunkEncoder.buildEncoderArgs, so the HLS + // packager can cut `-c copy` segments on exact keyframe boundaries. + const lockedGop = resolveLockedGopSize(options); + if (lockedGop !== null) appendLockedGopArgs(args, lockedGop); + // Mirrors chunkEncoder: disable B-frames for h264 so PTS == DTS, no // negative DTS at stream start. Without this, files freeze on the // first frame in VS Code preview, several browsers, and some HW // decoders. See chunkEncoder.buildEncoderArgs for the full reasoning. - if (codec === "h264") { + // h265 also gets `-bf 0` under a locked GOP: B-frame reordering across + // a segment boundary brings the same negative-DTS hazard back at every seam. + if (codec === "h264" || lockedGop !== null) { args.push("-bf", "0"); } @@ -338,10 +357,11 @@ export function buildStreamingArgs( options.rawInputFormat && options.hdr ? getHdrEncoderColorParams(options.hdr.transfer).x265ColorParams : "colorprim=bt709:transfer=bt709:colormatrix=bt709"; + const gopParams = lockedGop !== null ? `:${lockedGopCodecParams(codec, lockedGop)}` : ""; if (preset === "ultrafast") { - args.push(xParamsFlag, `aq-mode=3:${colorParams}`); + args.push(xParamsFlag, `aq-mode=3:${colorParams}${gopParams}`); } else { - args.push(xParamsFlag, `aq-mode=3:aq-strength=0.8:deblock=1,1:${colorParams}`); + args.push(xParamsFlag, `aq-mode=3:aq-strength=0.8:deblock=1,1:${colorParams}${gopParams}`); } // Apple devices require hvc1 tag for HEVC playback (default hev1 won't open in QuickTime) if (codec === "h265") {