mirror of
https://github.com/heygen-com/hyperframes.git
synced 2026-09-03 04:38:33 +00:00
fix(producer): address PR review feedback on harness mode + plan() copy filter
Miguel (approved) and Vai (commented) both flagged the same PSNR-threshold doc/code mismatch; Vai additionally flagged a path-anchoring bug in the projectDir-copy filter and a dishonest type cast. Addressed all five findings: PSNR threshold doc/code mismatch (important): - Module docstring, `resolveMinPsnrForMode` JSDoc, and tests/README.md all claimed distributed-simulated tightens to ≥50 dB. The actual code uses `max(fixture.minPsnr, 10)` — 10 dB is a pathology floor, the per-test gate is the fixture's authored `minPsnr`. Updated all three doc sites to describe what the code does. The 50 dB target in §5.1 is a per- render distributed-vs-in-process contract; against the frozen baseline it's unreachable for either mode (shared encoder/JPEG jitter), so it can't be a per-fixture gate. `PLAN_PROJECT_DIR_COPY_SKIP` regex matched absolute paths (important): - `cpSync` calls the filter with the absolute source path, so a `projectDir` whose absolute path happens to contain a blocklisted segment (`/home/user/work/output/comp/`, `~/projects/dist/foo/`, etc.) caused the filter to return false for every descendant — empty compiled directory, broken render. Now matches relative-to-projectDir segments via `path.relative()` + `split(sep)`. Switched from a regex to a Set for clarity. Harness fixtures don't hit this because they live under `tests/<name>/src/`, but adapters call `plan()` with caller-supplied paths. Dishonest type cast in regression-harness.ts (important): - `as "mp4" | "mov" | "png-sequence"` claimed reachability for formats that `validateMetadata` doesn't accept (the schema is `"mp4" | "webm"`, and webm is rejected by `checkDistributedSupport`). Narrowed to hardcoded `format: "mp4"` with a comment naming the metadata-schema invariant that lets us do that. Renamed `chunkVideoInjectorFactory` (nit): - The variable was invoked once and never used again — "factory" implied repeated calls. Inlined as a plain `videoInjector: BeforeCaptureHook | null` ternary. Replaced tautology test (nit): - `expect(DISTRIBUTED_SIMULATED_MIN_PSNR_DB).toBe(10)` was a value-pin over an exported constant. The invariant the JSDoc actually asserts is "10 dB is below any real fixture's authored minPsnr"; if someone lands a permissive fixture (minPsnr: 5), the value-pin doesn't catch it. Replaced with a test that walks `tests/*/meta.json` and asserts every authored `minPsnr` is ≥ the floor. Validated in `docker:test --mode=distributed-simulated`: font-variant-numeric, many-cuts, gsap-letters-render-compat, style-1-prod, sub-composition-video — all PASSED. Unit tests: 15/15 pass (new fixture-scan test included). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -96,11 +96,16 @@ passing in the summary):
|
||||
- `format === "webm"` — `plan()` refuses webm.
|
||||
- `hdr === true` — distributed mode is SDR-only at v1.
|
||||
|
||||
For fixtures that *are* supported, the PSNR threshold tightens to **≥ 50
|
||||
dB** (the §5.1 determinism contract) or the fixture's own `minPsnr`,
|
||||
whichever is higher. A failure at this threshold means the distributed
|
||||
pipeline has drifted from in-process output — file an issue rather than
|
||||
adjusting the threshold.
|
||||
Both modes use the fixture's authored `minPsnr` as the per-test
|
||||
threshold — distributed must clear the same quality bar in-process
|
||||
clears against the same frozen baseline. (`DISTRIBUTED-RENDERING-PLAN.md`
|
||||
§5.1's 50 dB target is a per-render distributed-vs-in-process contract;
|
||||
against the frozen baseline file, neither mode reaches it consistently
|
||||
due to shared encoder/JPEG-capture jitter.) An absolute 10 dB pathology
|
||||
floor catches fully-black-output regressions when a fixture authors a
|
||||
permissive threshold. A distributed failure at the fixture's own
|
||||
threshold means the distributed pipeline has drifted — file an issue
|
||||
rather than relaxing the fixture.
|
||||
|
||||
`--update` is incompatible with `--mode=distributed-simulated`: the
|
||||
in-process renderer is the source of truth for baselines, and the
|
||||
@@ -124,10 +129,10 @@ bun run --cwd packages/producer docker:test font-variant-numeric -- --mode=distr
|
||||
bun run --cwd packages/producer docker:test many-cuts -- --mode=distributed-simulated
|
||||
```
|
||||
|
||||
Both modes must produce PSNR ≥ 50 dB against the existing baseline. If
|
||||
`--mode=distributed-simulated` fails on a baseline, the distributed
|
||||
primitive has a regression — stop, file an issue, do not paper over it
|
||||
by adjusting the threshold.
|
||||
Both modes must pass at each fixture's authored `minPsnr` against the
|
||||
existing baseline. If `--mode=distributed-simulated` fails where
|
||||
`--mode=in-process` passes, the distributed primitive has a regression —
|
||||
file an issue rather than relaxing the fixture's threshold.
|
||||
|
||||
## Distributed-only fixtures
|
||||
|
||||
|
||||
Reference in New Issue
Block a user