fix(render): preserve transparency in GIF output - #2327
Conversation
vanceingalls
left a comment
There was a problem hiding this comment.
Adversarial R1 pass (Via). No prior reviews on file.
Strengths.
- Root-cause fix at the correct layer:
renderFormat.ts:3-5extracts a singleoutputNeedsAlphapredicate and both callers (renderOrchestrator.ts:1470,distributed/plan.ts:793) route through it. Not the band-aid shape (noif (format==='gif' && hasAlpha) …special case). ✓ Miguel bar. - Correct scoping across the five render surfaces:
DistributedFormat(distributed/shared.ts:23) is"mp4" | "mov" | "png-sequence" | "webm"— gif is not distributed, so theplan.tsswap is refactor-only for the three alpha-capable distributed formats (behavior preserved,outputNeedsAlphareturns true for all three). aws-lambda / gcp-cloud-run / render-batch inherit viaDistributedFormat; cli-render and studioServer both callexecuteRenderJob, which the PR does update. No half-plumbed surface. - JSDoc block on
renderOrchestrator.ts:225-247updated to name gif as alpha output and correctly notes GIF's binary transparency (no partial alpha) — matches how the palette encoder actually works. - Empirical fixture cited in the PR body (transparent white-dot → PaletteAlpha with transparent corner) is the right kind of proof for a GIF-alpha claim; unit tests can't easily assert on ffmpeg output bytes in CI.
Important (non-blocker follow-ups).
- Reliance on ffmpeg-default flags in
gifEncodeArgs.ts:22-32/36-50. The transparency contract depends on two undocumented-in-the-args defaults:palettegen.reserve_transparent=1(reserves palette slot) andpaletteuse.alpha_threshold=128(binarizes alpha). Both are ffmpeg defaults today, but a version bump that flips either silently regresses to opaque output — the same failure mode this PR fixes. Recommend making them explicit:palettegen=stats_mode=diff:reserve_transparent=1andpaletteuse=dither=sierra2_4a:alpha_threshold=128. Documents intent + immunizes against ffmpeg default drift. - Opaque-GIF backward-compat gap. GIFs without transparent pixels now capture as RGBA PNG (bigger on disk, slower capture) and generate palettes with a reserved transparent slot (255 content colors instead of 256). No golden in the regression shards exercises GIF, so this ships uncovered. Consider a small regression test that opaque-GIF output stays byte-equivalent or perceptually within a delta to pre-PR output. Not a correctness blocker; the palette-slot loss is imperceptible in practice.
Nit.
encodeStage.test.ts:224-243locks the value (PNG frame pattern in ffmpeg args, both calls) but not the reachability through the orchestrator'sneedsAlpharesolution.renderFormat.test.tscovers the mapping in isolation, so the two together give a light reachability chain; a single higher-level test that drivesexecuteRenderJob({format:'gif'})and asserts the encode stage was invoked withneedsAlpha:truewould tighten the seam. Optional.
Merge conflict. mergeStateStatus: DIRTY / mergeable: CONFLICTING — author to-do before merge. Flagging so it isn't lost; not a review blocker.
Audited: all 6 files in diff, plus gifEncodeArgs.ts (ffmpeg-arg construction), distributed/shared.ts (DistributedFormat surface), distributed/planFormatBanlist.test.ts (gif exclusion from distributed), cli/render.ts + studioServer.ts (both route through executeRenderJob).
Trusting: the empirical fixture in the PR body for actual transparent-GIF output; regression shard list for coverage gaps.
Verdict: APPROVE
Reasoning: Right thesis, right layer, five-surface consistent (gif is in-process-only by DistributedFormat), CI green. Follow-ups on explicit ffmpeg flags + opaque-GIF backward compat are worth doing but not gating.
— Via
0a8a016 to
578777f
Compare
What
reserve_transparent=1andalpha_threshold=128.needsAlpha=falsecontinues to use JPEG frames without alpha-only palette filters.Why
Direct
--format gifrenders silently flattened transparent compositions when frames are captured as JPEG, because the palette encoder receives no alpha plane to preserve.GIF also needs page-side shader compositing. A blanket
needsAlphaexclusion disabled that path after enabling RGBA capture, while the layered compositor intentionally excludes GIF. That left shader GIFs on the DOM fallback and produced hard cuts instead of the authored WebGL blend.How
outputNeedsAlpha, shared by in-process and distributed planning.data-no-timelineto the static transparency fixture so the artifact regression does not wait for a timeline it intentionally does not register.Test plan
bun test packages/producer/src/services/render/renderFormat.test.ts packages/producer/src/services/render/stages/encodeStage.test.ts packages/producer/src/services/render/capturePlan.test.ts— 23 passed.bun run --filter @hyperframes/producer typecheckbun run --filter @hyperframes/producer buildbun run --filter @hyperframes/producer test:transparency— WebM, GIF, and PNG sequence alpha assertions passed; GIF shader control/transition frames scored 28.11/26.57 dB against the golden.bun run --cwd packages/producer tsx src/regression-harness.ts page-side-shader-compositor-render-compat --sequential— all 100 visual checkpoints passed, stream parity passed, and audio correlation was 1.000.git diff --check.