fix: bound HDR and video extraction resources by jrusso1020 · Pull Request #2955 · heygen-com/hyperframes

jrusso1020 · 2026-08-02T19:52:13Z

What

Prevent short renders from extracting an entire long source video, bound HDR raw-frame scratch usage, clean extraction resources on every exit path, and expose the canonical outputDynamicRange request contract in the producer HTTP API.

Why

Two 2-second HLG renders pre-extracted roughly 60 seconds of each 1080x1920 source as rgb48le, producing about 70 GB of raw scratch per source. Process RSS remained near 1.4 GB, but filesystem page cache filled the producer's 24 GiB cgroup and incremented OOM counters. Open-ended video elements were resolved to the full media duration before the HDR pass reused that duration. Larger instances would only delay the same unbounded work.

How

Resolve the natural media duration from metadata, then cap extraction at the composition end while preserving shorter sources, negative timeline starts, and non-zero mediaStart offsets.
Use the same bounded extraction stage for in-process and distributed planning.
Limit HDR raw scratch to the strictest of HDR_EXTRACTION_MAX_BYTES, 50% of the actual cgroup memory limit, and 90% of free disk.
Reserve scratch capacity across concurrent renders and release it on success, cancellation, and partial failures.
Close raw-frame descriptors and remove temporary directories promptly; KEEP_TEMP=1 retains files for debugging while still closing descriptors.
Validate canonical outputDynamicRange values (auto, hdr, and sdr) and translate them to the established internal render policy.
Temporarily accept legacy hdrMode during the producer-first rollout and reject requests when canonical and legacy fields conflict.

Rollout dependency: publish and deploy this Hyperframes version across the complete producer fleet before deploying experiment-framework PR #44255. Existing experiment-framework workers omit the policy and therefore continue using auto, but this PR makes that path resource-bounded. During the subsequent experiment-framework rolling deploy, new workers send canonical outputDynamicRange while old workers remain on bounded auto; all managed renders are pinned to SDR only after that rollout drains. Legacy hdrMode remains accepted for other older clients. App PR #1496 supplies the 12 GiB configured ceiling.

Test plan

Unit tests added/updated
Manual testing performed
Documentation updated (not applicable; the HTTP boundary is internal and compatibility behavior is documented in code/deployment configuration)

Validated with full engine and producer suites across the incident fixes. For the API rename delta: 22 Bun server tests, 5 HTTP contract tests, all 490 Vitest producer unit tests plus every Bun producer unit lane, producer typecheck, full repository lint/format, test classification, and commit hooks pass. A constrained-cgroup reproduction separately confirmed that FFmpeg raw writes can exhaust cgroup memory through page cache while RSS stays low.

miga-heygen

Adversarial Review — Bound HDR and Video Extraction Resources

This closes the incident mechanism. The bounding, reservation, and cleanup are all structurally sound. Two observations (one SSOT, one safe-direction overestimate), neither blocking. Detailed analysis by area below.

1. Duration bounds — correct across all surfaces

Video extraction (resolveVideoExtractionDuration): Resolve natural duration first via resolveSegmentDuration (which factors in mediaStart and source durationSeconds), THEN cap at timelineEnd - video.start. This preserves:

Short sources inside long compositions (extract 2s, not 10s)
Negative starts (start=-3, timelineEnd=2 → extracts 5s, covering the preroll)
Non-zero mediaStart (seeks correctly, duration from remaining source)
Loop semantics unchanged (loop flag is preserved, not rewritten)
Explicit end bounds (capped at both authored end and timeline end)

HDR extraction (resolveHdrExtractionWindow): Same bounding shape — effectiveEnd = min(requestedEnd, compositionDuration), duration = effectiveEnd - start. Rejects videos with no interval inside the composition.

Distributed renders: Both callers of runExtractVideosStage (in-process at renderOrchestrator.ts:1076 and distributed at plan.ts:873) pass through the same stage, which now sends timelineEnd: composition.duration. The test file covers both modes (["in-process", false] and ["distributed plan", true]).

2. Aggregate budgeting — correct

Three-layer budget: env ceiling (HDR_EXTRACTION_MAX_BYTES), 50% of cgroup limit, 90% of free disk. Takes the strictest of all available. Returns undefined (unbounded) only when no cgroup AND no env ceiling — which is the bare-metal dev case where OOM is not a concern.

Reservation counter (aggregateHdrExtractionReservedBytes): Module-level, which works because the producer runs concurrent renders in the same process (controlled by maxConcurrentRenders). The counter would NOT coordinate across forked workers, but the producer doesn't fork for renders.

TOCTOU: The disk snapshot is taken once at reservation time. A non-HDR process filling the disk between the check and FFmpeg writes is not covered — but that's inherent to any cooperative budget and not practically closeable. The reservation prevents the known-concurrent-HDR case.

3. Cleanup and reservation release — correct on all paths

Traced five scenarios through the code:

Scenario	Cleanup path	Reservation
Normal completion	`captureHdrStage.ts` finally: `cleanupHdrVideoFrameSource` all sources, then `releaseReservation()`	Released once
FFmpeg fails on video N of M	`extractHdrVideoFrames` catch: cleans completed sources + orphan frameDirs, calls `releaseReservation()`, rethrows. `captureHdrStage` finally: `releaseHdrExtractionReservation` is null (never assigned).	Released once
Abort mid-extraction	Same as FFmpeg failure — catch block fires	Released once
Abort mid-render (after extraction)	`captureHdrStage` finally fires, releases	Released once
`cleanupEndedHdrVideos` mid-render	Cleans individual sources but does NOT release reservation — the reservation covers the entire extraction batch and is released in the finally block	Stays held (correct — estimate is for the batch, not per-source)

The release function's released boolean guard prevents double-release. Math.max(0, ...) on decrement prevents underflow. KEEP_TEMP=1 closes the fd but retains the directory (test verified).

4. Bypass paths — none in production

extractAllVideoFrames makes timelineEnd optional, but the sole production caller (runExtractVideosStage) passes it. The video.start >= options.timelineEnd early-continue skips videos entirely past the composition end.
extractHdrVideoFrames is called only from runCaptureHdrStage, which is called only from renderOrchestrator. The new reservation + window bounding is on the only path.
extractVideoFramesRange is a public engine export and could be called by external consumers without timelineEnd, but that's a pre-existing API surface, not a regression from this PR.

5. `hdrMode` API plumbing — correct

The HTTP API (server.ts) validates, forwards, and rejects invalid hdrMode values. Three valid modes (auto, force-hdr, force-sdr), invalid values rejected before job creation. Tests cover both the lenient parser (drops invalid) and the strict validator (returns error).

6. Rollout gap — none

This PR makes extraction bounded regardless of hdrMode. The downstream PRs (experiment-framework#44255, app#1496) add force-sdr for managed renders — which skips HDR entirely, an even stricter bound. Without the downstream PRs, auto still runs but now with budget/reservation protection. No gap.

Observation (non-blocking): `resolveHdrExtractionWindow` does not factor in source media duration

The engine's resolveSegmentDuration considers metadata.durationSeconds - mediaStart (how much source actually remains). The producer's resolveHdrExtractionWindow does not — it only uses composition timing.

Consequence: when mediaStart is high relative to the source (e.g. mediaStart=59.999 of a 60s source), the producer estimates a 10s extraction window but only 0.001s of source exists. FFmpeg stops early (no data corruption), but the budget estimate overcounts — the reservation holds ~100,000x more than needed. In the worst case this causes a valid render to be rejected by the budget check when it would actually fit.

This is safe-direction (over-reserves, never under-reserves) and the scenario is unusual (HDR source with mediaStart near source end). Not blocking, but if false rejections surface, this is the root cause.

Observation (SSOT): Two parallel bounding functions

resolveVideoExtractionDuration (engine) and resolveHdrExtractionWindow (producer) compute the same concept — "how long to extract" — with different inputs and error handling. The engine version is source-aware (factors in metadata.durationSeconds); the producer version is not. If a future change updates one and forgets the other, the budget estimate and actual extraction will diverge. Consider extracting the shared bounding logic or having the producer call through the engine's function.

Verified

Duration bounds: negative starts, non-zero mediaStart, short sources, open-ended media, distributed renders
Budget: env ceiling, cgroup 50%, free disk 90%, stricter-of-all, invalid rejection
Reservation: prevents concurrent overcommit, idempotent release, no leak on any exit path
Cleanup: fd closed + dir removed on every exit (success, partial failure, abort), KEEP_TEMP retains files but closes fd
Bypass: sole production callers pass timelineEnd / use reservation
hdrMode API: validated, forwarded, rejected when invalid
Rollout: no gap between this PR and downstream force-sdr deployment
Cache key: removing start/end is correct — extracted content is determined by mediaStart + resolved duration

Ships clean.

vanceingalls

Adversarial review — bounded HDR/video extraction

APPROVE at 2f22ba3ccadee7a223cb2489e0d2c8d9ff21ff99. The reported failure mode (2s HLG render → ~70 GB of raw scratch per source → cgroup page-cache OOM at low RSS) is closed at its structural cause. Findings inline; two non-blocker notes only.

The fix mechanism

resolveHdrExtractionWindow(video, compositionDuration) in captureHdrResources.ts:269-295 is the single source of truth for HDR extraction bounds:

requestedEnd = Number.isFinite(video.end) && video.end > video.start ? video.end : compositionDuration — closes the "open-ended video resolved to full media duration" path that produced the reported OOM.
effectiveEnd = Math.min(requestedEnd, compositionDuration) — caps at composition end regardless of author intent.
durationSeconds <= 0 throws early, avoiding the FFmpeg-produces-zero-frames → cleanup-only-partial path.

FFmpeg is then invoked with -ss window.mediaStart -i src -t window.durationSeconds at captureHdrResources.ts:443-460, so authored mediaStart offsets survive verbatim. The reported bug becomes: 2s composition × 1920×1080 × 6 bytes/px × 30 fps ≈ 750 MB per source instead of ~35 GB — order-of-magnitude reduction.

Unit test at captureHdrResources.test.ts:114-123 pins the exact reported shape: { start: 0, end: Infinity, mediaStart: 0 } with compositionDuration=2 resolves to durationSeconds=2, which yields 60 * 3840 * 2160 * 6 raw bytes for a 4K 30fps composition.

James's four failure axes — walked

(1) Duration bounds for naturally short media, negative starts, non-zero mediaStart, distributed renders

Naturally short media, SDR path: resolveSegmentDuration (videoFrameExtractor.ts:727-735) falls back to metadata.durationSeconds - mediaStart when end - start is non-finite, then resolveVideoExtractionDuration caps at timelineEnd - start. Covered by timelineBound.test.ts:112-118 (source=2s, composition=10s → durationSeconds=2).
Negative starts, HDR path: resolveHdrExtractionWindow({ start: -3, end: 60, mediaStart: 5 }, 2) returns durationSeconds=5. Test at captureHdrResources.test.ts:133-137 accepts this. This overshoots the visible window by ~3s (visible is [source 8, source 10], extract is [source 5, source 10]) — waste, not safety. See non-blocker #1.
Non-zero mediaStart: FFmpeg -ss window.mediaStart passes it through. Verified in code; no change to prior semantics.
Distributed renders: planV2Execution.ts:487-497 rejects hdrMode: "force-hdr" for distributed with FormatNotSupportedInDistributedError — HDR reservation code is never reached in distributed. The SDR duration-bound side IS covered in distributed via timelineBound.test.ts:99-101 with materializeSymlinks: true, which drives the exact plan() call site at distributed/plan.ts:1036.

(2) Aggregate cgroup/disk budgeting under concurrent jobs

reserveHdrExtractionBytes uses a module-level aggregateHdrExtractionReservedBytes counter. budgetBytes = min(configured or cgroup*0.5, freeBytes*0.9). The atomic sync block (read → check → write, no await) makes the aggregate check race-free under Node's single-threaded event loop even when concurrent renders enter extractHdrVideoFrames in parallel — verified via captureHdrResources.test.ts:167-195.
Cgroup limit reading is cached process-lifetime (systemMemory.ts:87-109) — acceptable since k8s cgroup resize implies pod restart.

(3) Cleanup + reservation release on partial failures / cancellation

extractHdrVideoFrames (captureHdrResources.ts:502-512) catch block: iterates out.values() (successfully-opened sources) AND createdFrameDirs (partial FFmpeg outputs that never got an fd) — both cleaned. Then releaseReservation(). Verified by captureHdrResources.test.ts:233-263.
captureHdrStage.ts:442-464 outer finally: unconditionally calls releaseHdrExtractionReservation?.(); safe because releaseReservation is idempotent (released sentinel, captureHdrResources.ts:249-256).
AbortSignal is threaded through runFfmpegImpl. On abort mid-extraction, ffmpeg returns non-success, the loop throws, catch cleans + releases.

(4) Whether any production path still bypasses these safeguards

In-process HDR: renderOrchestrator.ts:3321-3348 routes to runCaptureHdrStage which owns the reservation.
Distributed: renderChunk.ts uses runCaptureStage (not HDR); HDR is banned at plan time.
SDR: timelineEnd = composition.duration passed at extractVideosStage.ts:379, consumed by resolveVideoExtractionDuration (videoFrameExtractor.ts:738-753) via extractAllVideoFrames. Timeline-bound test covers this.

The HDR_EXTRACTION_MAX_BYTES_ENV reader (resolveHdrExtractionBudgetBytes) is called only from assertHdrExtractionDiskHeadroom, which is only called from extractHdrVideoFrames. No public export bypass. No test/dev shim leaks past the gate.

Rollout dependency chain

Managed force-SDR (EF #44255): keeps managed renders off the HDR path entirely — belt.
12 GiB configured ceiling (app #1496): sets HDR_EXTRACTION_MAX_BYTES on producer containers. In a 24 GiB cgroup, resolveHdrExtractionBudgetBytes(12GiB, 24GiB) returns min(12GiB, 12GiB) = 12GiB — parity with the 50% cgroup fraction; no gap. Suspenders.
If a container is later resized to a smaller cgroup, min(12GiB, 0.5×newCgroup) picks the tighter one automatically. Rollout config survives resize.
After #2955 lands and the package bump goes out, EF #44255 + app #1496 can deploy in either order. Neither introduces a new failure surface once #2955 is on.

Non-blockers (follow-up, do not gate this PR)

NB-1 — resolveHdrExtractionWindow doesn't consult source metadata like SDR does. SDR's resolveVideoExtractionDuration folds metadata.durationSeconds - mediaStart into the min. HDR's resolveHdrExtractionWindow doesn't have access to metadata at the call site, so a naturally-short HDR source (e.g. a 1s HLG stinger) in a 5s composition estimates + reserves 5× actual raw bytes. FFmpeg's -t 5 on a 1s source produces a 1s raw file, so ACTUAL disk usage is bounded; the ESTIMATE is over-reserved. Consequence: multiple concurrent renders with short HDR sources may hit the aggregate budget cap spuriously. Threading source metadata through the HDR plan step would tighten this, but it's not the reported failure mode. Follow-up.

NB-2 — HDR_EXTRACTION_HEADROOM_FRACTION = 0.9 leaves 10% for filesystem overhead + producer working memory + encoder output. In a 100 GB free-disk state a single reservation could grab 90 GB. There's no additional protection for the encoder's own scratch or the compiled-dir tree that landed before this stage. Not seen in the reported incident (cgroup page-cache, not disk-full), but worth naming as a knob if a future incident traces to disk-full. Follow-up if it recurs.

Approve as-is. The four axes James called out are structurally closed, the rollout dependency chain is consistent, and neither non-blocker is a safety regression. Ship + bump + let EF #44255 and app #1496 land.

— Via

miguel-heygen

Adversarial review — one blocking negative-start gap

The reservation and cleanup mechanics are strong. I traced normal completion, partial FFmpeg failure, cancellation during extraction, cancellation after handoff, and early per-source cleanup: the descriptor/temp-dir cleanup and idempotent reservation release are correctly paired on every path. The shared runExtractVideosStage call also puts timelineEnd on both in-process and distributed SDR planning, and both HTTP render handlers normalize/validate hdrMode through the same request builder.

Blocking: negative starts can still extract the full hidden preroll

Both new window calculations cap only the end of the interval:

videoFrameExtractor.ts:752 uses timelineEnd - video.start.
captureHdrResources.ts:280-283 uses effectiveEnd - video.start and leaves mediaStart unchanged.

For a 120-second source with start=-60, mediaStart=0, and a 2-second composition, both paths plan 62 seconds of extraction even though only source seconds 60..62 can be visible. For a 60-second source with the same start, they still extract all 60 seconds even though the clip has ended at composition time 0. The added negative-start tests explicitly pin this behavior (videoFrameExtractor.test.ts:78-81, captureHdrResources.test.ts:133-137).

That recreates the PR's motivating shape—"short render extracts essentially the entire long source"—for a supported timeline precondition. The new budget changes the outcome from an uncontrolled write/OOM into a fail-fast rejection, which is safer, but it still rejects a valid render whose visible two-second segment would fit comfortably. It also means the resource estimate is not actually bounded by composition duration when start < 0.

Please intersect the authored interval with [0, timelineEnd] and advance the extraction source offset by the trimmed preroll (or carry an equivalent extraction-base offset into frame lookup), then pin both ordinary and HDR paths with a materially negative case such as start=-60, not just -3. The important invariants are: extract only the visible interval, seek to mediaStart + hiddenPreroll, and preserve frame lookup at composition time 0.

Rollout/readiness note

EF #44255 (managed force-sdr) and app #1496 (12 GiB ceiling) are green but still open, and the internal package-bump PR does not exist yet. That is an operational ordering gate rather than a blocker in this OSS diff: the cgroup-derived 50% ceiling already fails closed before those land. Please keep the documented order—release/package bump first, then managed force-SDR and configured ceiling—so no deployment assumes fields/env support before the sidecar has this version.

Verdict: REQUEST CHANGES

Reasoning: Cleanup, aggregate in-process reservation, server plumbing, and distributed call-site coverage are sound, but negative starts leave unbounded hidden preroll in both extraction implementations. The hard budget prevents the original uncontrolled OOM; it does not preserve valid short renders or close the duration-bound contract for this explicitly supported case.

— Magi

vanceingalls

Reversing my earlier APPROVE — Magi's negative-start finding is correct and blocks

I posted APPROVE on this PR at the same SHA (2f22ba3ccadee7a223cb2489e0d2c8d9ff21ff99) treating the negative-start over-extract as a non-blocker on the grounds of "safe-direction over-reservation" and "3s overhead is minor". Magi's independent review published minutes later dismantled that defense with a concrete case (start=-60, source=120s, comp=2s) that recreates the exact motivating incident shape — 60+ seconds of raw scratch planned for a 2-second visible render.

Reversing to REQUEST CHANGES at the same SHA. My earlier APPROVE should be disregarded in favor of this one.

Why the deferral defense doesn't hold

My original argument: "video.start=-3 → 5s durationSeconds → 3s overshoot [visible source 8..10, extract 5..10]. Wasteful but bounded."

Magi's counter: for start=-60, the overshoot scales linearly with |start|. At start=-60, comp=2, source=120: resolveHdrExtractionWindow computes durationSeconds = 2 - (-60) = 62s. Estimated reservation: 62s × 1920 × 1080 × 6 × 30fps ≈ 23.1 GB. Budget (12 GiB post-app#1496 or 12 GiB via cgroup 50% fallback) rejects. A legitimate 2-second visible render is rejected before extraction starts.

That's not "safe-direction over-reservation" — that's silent conversion of a valid short render into a fail-fast rejection. The PR's motivating goal ("bound HDR extraction resources so a 2-second render doesn't extract 60s of media") is undermined by its own math when start < 0.

Same defect on the SDR path

resolveVideoExtractionDuration (videoFrameExtractor.ts:752) is symmetric: Math.min(resolvedDuration, Math.max(0, timelineEnd - video.start)). For start=-60, timelineEnd=2, natural=60: min(60, 62) = 60. Extracts the entire source. Managed force-sdr renders (via EF #44255) get slower + waste disk; explicit HDR renders get rejected by the 12 GiB budget.

The SDR-JPG failure mode is softer (~750 MB per source, not 23 GB) so EF #44255's managed-default-force-sdr masks the pathology in production. But per James's explicit reassessment ask, the companion PRs shouldn't be positioned as compensating: EF #44255 reduces the frequency of the failure, not its existence. Explicit force-hdr on any user with negative-start content still rejects.

The correct fix shape

Intersect the authored interval [start, end] with [0, timelineEnd] before computing the extraction window, and advance the FFmpeg seek offset by the trimmed preroll:

visibleStart = max(0, video.start)
visibleEnd = min(video.end, timelineEnd)  // or compositionDuration for HDR
if (visibleEnd <= visibleStart) skip extraction
durationSeconds = visibleEnd - visibleStart
hiddenPreroll = max(0, -video.start)
ffmpegMediaStart = video.mediaStart + hiddenPreroll
// FFmpeg: -ss ffmpegMediaStart -t durationSeconds

Frame lookup must map composition time T (in [0, timelineEnd]) to extracted-frame index Math.floor((T - visibleStart) * fps), i.e. treat visibleStart (not video.start) as the extraction's zero point. Every current call site that assumes extraction begins at video.start needs an audit.

Please add test coverage with a materially negative case — start=-60 (matching the incident scale) or larger, not just -3 (which sits inside my earlier reasoning and doesn't stress the bound).

Assessment of the other two mitigation PRs (per James's ask)

EF #44255: reviewed + APPROVED separately. Threading of hdrMode is complete across all 7 managed HTTP paths I audited; three-layer force-hdr defense (workflow gate + narrow Pydantic Literal + activity guard) is well-formed; regression coverage is thorough. Doesn't compensate for this negative-start defect.
app #1496: reviewed + APPROVED separately. 12 GiB value math is correct (12 × 1024³ = 12884901888), env-var name matches HDR_EXTRACTION_MAX_BYTES_ENV in this PR, applied dev/staging/prod, sidecar ConfigMap + configMapRef wiring verified via Helm-rendered assertions. Converts unbounded OOM into fail-fast rejection but doesn't preserve valid negative-start short renders.

Rollout dependency chain — no unsafe window, but negative-start defect ships regardless

Deploy order (per app #1496 body):

Merge this PR + EF #44255 + app #1496.
Release HF producer package → producer-internal bump → dev/staging/prod deploy.

Between (1) and (2) is not a regression window — the old producer image ignores both the env var and the hdrMode field, so we get the same OOM shape as today. But once (2) completes with the negative-start defect unfixed, valid short renders with negative starts start being rejected as a NEW behavior.

That's the reason this is blocking rather than a follow-up: shipping the 3-PR mitigation without fixing the negative-start bug creates a new user-visible failure mode (valid short renders rejected) that didn't exist before, in exchange for closing the OOM (which was fixable independently).

Non-blockers from my earlier review remain accurate

HDR estimate doesn't consult source metadata like SDR does (over-reserves on naturally-short sources)
HDR_EXTRACTION_HEADROOM_FRACTION = 0.9 leaves 10% for encoder scratch + fs overhead

Both fold naturally into the fix for the blocking finding — the corrected duration math should use visibleEnd - visibleStart, and both the estimate and the FFmpeg seek should share the same source-aware, timeline-clipped derivation.

Once the negative-start fix lands

I'll re-review at the new head. The rest of the diff (reservation lifecycle, cleanup on partial/aborted paths, hdrMode API plumbing, distributed HDR ban, timeline-bound tests for the non-negative case) all held up under my earlier trace and Magi's — those don't need re-verification.

— Via

jrusso1020 · 2026-08-02T21:12:50Z

Valid

jrusso1020 · 2026-08-02T21:12:51Z

@miguel-heygen @vanceingalls addressed the blocking negative-start gap at head 297a3942a:

Shared SDR/HDR interval math now intersects the authored clip with [0, timelineEnd].
Trimming hidden preroll advances mediaStart by the same amount and rebases both SDR frame lookup and HDR's hdrVideoStartTimes extraction clock to composition time zero.
The materially negative regression case (start=-60, 120s source, 2s composition) now seeks with -ss 60, extracts with -t 2, and budgets only the two visible seconds in both ordinary and HDR paths.
Clips with no intersection are skipped cleanly rather than turned into extraction failures.
SDR and HDR use one shared timeline-window helper, removing the drift risk called out in the other reviews.

Verification: 104 targeted tests passed; engine and producer typechecks passed; oxfmt, oxlint, and the full pre-commit hook passed.

miga-heygen

Re-review at `297a3942` — negative-start fix + shared timeline window

Both concerns from my first review are resolved. The incident mechanism is fully closed. Detailed trace against James's re-review checklist.

Frame alignment — correct

For the incident case (start=-60, mediaStart=0, compositionDuration=2, sourceDuration=120):

compositionStart = max(0, -60) = 0
trimmedPreroll   = 0 - (-60)   = 60
mediaStart       = 0 + 60      = 60
durationSeconds  = max(0, min(120 - 60, 2 - 0)) = 2

FFmpeg: -ss 60 -t 2 — extracts source[60:62], exactly the 2 seconds visible in the composition. The video element is rewritten to start=0, end=2, mediaStart=60, so frame lookup at composition time 0 resolves to source time 60. Both SDR and HDR produce identical extraction windows because both call resolveTimelineExtractionWindow.

No-intersection behavior — correct

Clips entirely before time zero (start=-60, end=-10) get durationSeconds=0:

SDR: { skipped: true } — not included in results, no error
HDR: resolveHdrExtractionWindow returns null → continue in extraction loop

Integration test verifies: start=-2, end=-1 → { success: true, extracted: [], errors: [] }. No wasted FFmpeg work, no false extraction failure.

Loop and mediaStart semantics — preserved

The preroll trim advances mediaStart by exactly the trimmed amount. For a looped video with invisible preroll, the extraction skips the invisible source interval and starts at the visible boundary. The loop flag is untouched — looping happens at the player level, not extraction level.

Cache-key correctness — correct

keyInput.mediaStart = window.mediaStart after trimming. Two elements with different authored start values but the same trimmed window produce the same cache key — correct because they extract identical source frames.

Bypass paths — none

resolveTimelineExtractionWindow is the single shared helper. SDR calls it through resolveVideoExtractionWindow. HDR calls it through resolveHdrExtractionWindow. Both use the same math. The parallel-function SSOT concern from my first review is resolved.

First-review observations — both addressed

SSOT (parallel bounding functions): HdrExtractionWindow is now a type alias for TimelineExtractionWindow. resolveHdrExtractionWindow delegates to resolveTimelineExtractionWindow. One function, two callers.
Source duration overestimate: The HDR path still doesn't factor in metadata.durationSeconds (it uses the timeline-capped window, not the source-aware segment duration). But the timeline cap now ensures extraction never exceeds compositionDuration, which closes the incident case (60s extraction for a 2s composition → now 2s extraction). The residual overestimate (high mediaStart near source end) is bounded by composition duration rather than source duration — safe direction, unusual scenario.

Full rollout set — consistent

PR	Status	Notes
HF #2955 (this)	Ships clean	Duration bounds + budget + cleanup
EF #44255	Ships clean	force-sdr default across all managed paths
App #1496	Cannot access	Token scope — Via should verify env var placement

The negative-start fix is independent of force-sdr: it protects any composition with negative starts regardless of HDR mode. The three PRs are complementary, not compensating.

Verified

start=-60 extracts only source[60:62] in both SDR and HDR
Frame lookup rebased to composition time 0
Clips with no visible intersection skipped without error
Cache key updated with trimmed mediaStart
Loop flag preserved through window computation
SDR and HDR share resolveTimelineExtractionWindow
No production path bypasses the shared bound
Reservation lifecycle unchanged from first review (still correct)

Ships clean.

miguel-heygen

R2 at exact head 297a3942a63128b84a7fef51f9b01d47b44988a1.

The original blocker is fixed for a long, non-looping source: resolveTimelineExtractionWindow now gives the reported start=-60, source=120s, composition=2s case { compositionStart: 0, mediaStart: 60, durationSeconds: 2 } (packages/engine/src/services/videoFrameExtractor.ts:743-769), both SDR and HDR consume that one result, and the cache key is updated before lookup/dedupe (:1588-1602). Timeline-only no-intersection clips also skip cleanly. The shared helper closes the prior SDR/HDR drift.

blocker — negative-start trim breaks short-source loop and held-tail semantics

packages/engine/src/services/videoFrameExtractor.ts:759-768 advances mediaStart linearly by hidden preroll, but the helper receives neither loop nor source duration. extractAllVideoFrames then mutates the production VideoElement to that result and sends the adjusted seek to FFmpeg (:1588-1602, :1425-1433 / :1451-1459).

Concrete supported case: a 3s source with { start: -5, end: 10, mediaStart: 0, loop: true } in a 2s composition. The runtime's existing loop contract maps composition t=0 to source 5 % 3 = 2s (packages/core/src/runtime/media.ts:234-265). This head instead resolves { compositionStart: 0, mediaStart: 5, durationSeconds: 2 }, invokes FFmpeg with -ss 5 -t 2, and gets zero frames because the seek is past EOF. The same shared helper feeds HDR extraction (packages/producer/src/services/render/stages/captureHdrResources.ts:267-292,413-449), where zero frames hard-fail the render. Preserving the boolean loop field is not sufficient; its phase was changed and the source seek is invalid.

There is a sibling non-loop regression: with the same short source and loop:false, the established runtime holds the final source frame through the authored slot (packages/core/src/runtime/media.ts:235-258). The new linear mediaStart=5 seek produces no frames instead of that held tail. Before this change, extraction began at source 0 and frame lookup supplied the loop/hold behavior, so these are new failures, not pre-existing render semantics.

Please make the trimmed window source-aware. For loops, preserve the runtime's modulo phase across the original playable range (or retain a separate lookup phase while extracting a valid cycle); for non-loops whose hidden preroll exhausts the source, preserve the held-final-frame contract rather than seeking past EOF. Pin both cases with a materially negative start and a source shorter than the hidden preroll, through the real extraction + frame-lookup/HDR path. The corrected effective phase must also remain part of the cache identity.

This is additive to Miga's R2: their 120s-source trace is correct, but “loop flag untouched” does not establish loop semantics once mediaStart and start are rewritten.

The EF #44255 and app #1496 reviews remain approved; neither companion compensates for this production-path regression. The rest of #2955's reservation, cleanup, budget, and caller coverage remains sound from R1.

Verdict: REQUEST CHANGES

Reasoning: the long-source negative-start incident case is now bounded correctly, but the same rewrite seeks past EOF for supported negative-start clips whose hidden preroll crosses a source boundary, breaking both looping playback and non-loop final-frame hold in SDR and HDR.

— Magi

vanceingalls

R2 adversarial re-review at `297a3942a63128b84a7fef51f9b01d47b44988a1`

APPROVE. Magi's blocking negative-start finding is structurally closed. Frame alignment, no-intersection, mediaStart, and cache-key semantics all trace correctly through the new shared helper. Two non-blockers below (both exotic corner cases with missing test coverage), neither gates the mitigation set.

Verifying Magi's finding is closed

The critical primitive is the new resolveTimelineExtractionWindow in videoFrameExtractor.ts:743-770:

const compositionStart = Math.max(0, video.start);
const trimmedPreroll = compositionStart - video.start;
const durationSeconds = Math.max(
  0,
  Math.min(resolvedDuration - trimmedPreroll, timelineEnd - compositionStart),
);
return { compositionStart, mediaStart: video.mediaStart + trimmedPreroll, durationSeconds };

Walking Magi's two examples with the new math:

Example 1 — 120s source, start=-60, mediaStart=0, end=Infinity, comp=2:

SDR: resolvedDuration = resolveSegmentDuration(Inf, 0, {120}) = 120. Window: compositionStart=0, trimmedPreroll=60, durationSeconds=min(120-60, 2-0)=2, mediaStart=60. FFmpeg: -ss 60 -t 2 → extracts source 60..62. ✓
HDR: resolveHdrExtractionWindow computes requestedEnd=2 (Inf fails isFinite, falls to compositionDuration), then resolveTimelineExtractionWindow(video, 2-(-60)=62, 2): compositionStart=0, trimmedPreroll=60, durationSeconds=min(62-60, 2)=2, mediaStart=60. FFmpeg: -ss 60 -t 2. ✓

Example 2 — 60s source, start=-60, mediaStart=0, end=Infinity, comp=2:

SDR: resolvedDuration = resolveSegmentDuration(Inf, 0, {60}) = 60. Window: durationSeconds=min(60-60, 2)=0 → returns skipped: true at line 1591. ✓
HDR: computed via requestedEnd - start = 62 without source-metadata → durationSeconds=2 (see NB-1). Not equally skipped, but the estimated bytes and FFmpeg extraction are both tiny (2s planned, 0 frames actual → abort with "produced no frames"). Not OOM-shaped, not silent damage. Failure mode changed from "extract 60s of raw scratch → silent black" (pre-#2955) to "abort with clear error" — safer than before, less user-friendly than the SDR skip.

Test at videoFrameExtractor.test.ts:88-95 pins the mutation via resolveVideoExtractionWindow({start:-60,end:120,mediaStart:0}, metadata(120), 2) returning {compositionStart:0, mediaStart:60, durationSeconds:2}. HDR sibling test at captureHdrResources.test.ts:135-139 pins the same shape.

Five axes James called out

Frame alignment. SDR mutates video.start = window.compositionStart at videoFrameExtractor.ts:1591-1594 before publishing to results. HDR path sets prep.hdrVideoStartTimes.set(videoId, window.compositionStart) at captureHdrResources.ts:421. Both align frame lookup at composition time 0 = extracted-frame index 0. getFrameIndexAtTime at videoFrameExtractor.ts:1725-1741 reads the mutated videoStart, so localTime = globalTime - videoStart yields [0, durationSeconds] for the visible range — correct. Integration test at captureHdrResources.test.ts:220 asserts prep.hdrVideoStartTimes.get("preroll") === 0 post-extraction. ✓

No-intersection behavior. SDR: if (videoDuration <= 0) return { skipped: true } at videoFrameExtractor.ts:1588-1590; preparedExtractions filter at videoFrameExtractor.ts:1654 drops skipped entries entirely. HDR: resolveHdrExtractionWindow returns null; extractHdrVideoFrames if (!window) continue at lines 417 and 434. New end-to-end test at videoFrameExtractor.test.ts:1082-1101 confirms {success: true, extracted: [], errors: []} for start=-2, end=-1, timelineEnd=2. ✓

Loop / mediaStart semantics. For the common case (non-looped or source_natural ≥ |start|), the mutation preserves alignment: FFmpeg seeks to video.mediaStart + trimmedPreroll, then extracts the visible interval; composite reads frame 0 at comp time 0 via the mutated videoStart. getFrameIndexAtTime's loopDuration = extracted.metadata.durationSeconds - mediaStart reads the trimmed mediaStart (60 in Magi's example 1), yielding loopDuration = 120 - 60 = 60 — the video can still loop through its post-trim remainder within the visible interval. NB-2 flags one exotic subcase where this breaks (see below).

Cache-key correctness. cacheKeyInputs[index].mediaStart = window.mediaStart updated at videoFrameExtractor.ts:1596-1597 so the cache key reflects the actual extracted [mediaStart, mediaStart+duration] range, not the authored one. Two videos with different authored start but same trimmed mediaStart correctly share cache. The dedupe key at videoFrameExtractor.ts:1552 uses video.mediaStart post-mutation, consistent with the cache key. ✓

Production-path bypass. resolveTimelineExtractionWindow is the sole primitive; resolveVideoExtractionDuration and resolveVideoExtractionWindow (SDR) plus resolveHdrExtractionWindow (HDR/producer) both route through it. Callers: runExtractVideosStage (in-process + distributed via materializeSymlinks), runCaptureHdrStage. No other production caller. The new exports (resolveTimelineExtractionWindow, resolveVideoExtractionWindow, TimelineExtractionWindow) don't add a new externally-invokable bypass — they're the same primitives, just made available for the HDR producer to reuse.

Non-blockers

NB-1 — HDR duration computation still lacks source-metadata awareness (carried forward from R1).

resolveHdrExtractionWindow at captureHdrResources.ts:271-292 computes resolvedDuration = requestedEnd - video.start without consulting source natural duration. For open-ended HDR video where source_natural < requestedEnd - video.start and source_natural ≤ |video.start|, the extraction plans a small non-zero duration (e.g. Magi's 60s-source example: 2s planned), then FFmpeg reads past EOF, produces 0 frames, and extractHdrVideoFrames throws "produced no frames". Not OOM-shaped (planned bytes are small), and the failure is loud, but the SDR path handles the same shape via clean skip. Threading source metadata through the HDR plan step or having the HDR path piggy-back on the SDR path's outcome would close the asymmetry — good follow-up, not blocking.

Missing test coverage: an HDR case with source_natural ≤ |start| (e.g. start=-60, end=Infinity, source=60) that pins the "aborts with produced-no-frames" behavior explicitly so future refactors don't accidentally regress it to silent-black or the pre-#2955 OOM shape.

NB-2 — Loop semantics regression when source_natural < |start| + comp.

For a looped video where the visible interval spans multiple source loops (e.g. source=10, start=-15, comp=5, loop=true), pre-#2955 behavior was: extract the full 10s source, let getFrameIndexAtTime's localTime %= loopDuration wrap the composite reads across loop iterations. Post-fix: resolveTimelineExtractionWindow(video, resolvedDuration=10, timelineEnd=5) yields compositionStart=0, trimmedPreroll=15, mediaStart=15, durationSeconds=max(0, min(10-15, 5)) = 0 → SDR returns skipped: true, no frames available for the composite. HDR path abort in the same shape as NB-1.

The extractor was never loop-aware — looping was applied at composite time — but the extraction extent used to be broad enough (source_natural) to give the composite something to loop over. Trimming to the visible-interval-only window removes the source content the composite needs to wrap through.

Fix shape: for loop=true videos, either (a) skip the trimming and extract full source (current behavior pre-#2955), (b) extract the source-natural range and let composite loop-wrap through it against the mutated videoStart, or (c) trim only when loop=false.

Missing test coverage: loop=true, start=-large_negative, source < |start|+timelineEnd — no such test exists in videoFrameExtractor.test.ts. This is genuinely exotic (a looped video with a preroll longer than the source natural), so I'd expect near-zero real-user impact — but the pre-existing behavior IS supported and the fix silently changes it.

Three-PR set re-check at current heads

HF #2955 @ 297a394 — this review. APPROVE with two exotic-case non-blockers.
EF #44255 @ 098be2f6 — head unchanged since my R1 APPROVE. hdrMode threading + 3-layer force-hdr defense still holds. Now correctly positioned as reducing frequency rather than closing — this PR's SDR default means most managed renders won't hit either the HDR path or its NB-1/NB-2 subtleties. Explicit force-hdr renders now correctly extract only the visible interval and reserve only the visible-interval scratch. Ships.
App #1496 @ 01dc91c4 — head unchanged; CI CLEAN. The 12 GiB ceiling now bounds a materially smaller worst case (visible-interval extraction, not authored-interval) — post-fix, a legitimate 2s HDR render with start=-60 reserves ~750 MB (2s × 1920 × 1080 × 6 × 30) instead of ~23 GB, so the budget check no longer rejects the class of renders Magi flagged. Ships.

Reassessment against Magi's finding (per James's original ask, now closed)

At R1 I split the companions into "closes / reduces frequency / orthogonal" and neither closed the negative-start defect. At R2 the OSS PR itself now closes the defect for the primary shape, so the companions can ship in their originally-intended roles: EF #44255 reduces frequency of HDR path even being taken (managed force-sdr default); app #1496 caps the worst case. Both are complementary suspenders, not compensating masks. No shipping-order gap remains.

Verified

Magi's start=-60 example closes cleanly on both SDR (skip via 0-duration) and HDR (skip via null-window) for the primary shapes
Frame alignment via video.start mutation + prep.hdrVideoStartTimes update — comp time 0 → extracted-frame 0
mediaStart offset compounded with trimmedPreroll (test at hdrResources.test.ts:135)
Cache key + dedupe key both use trimmed mediaStart (line 1596-97, 1552)
No-intersection skip works end-to-end (integration test at videoFrameExtractor.test.ts:1082)
resolveTimelineExtractionWindow is the single primitive; SDR and HDR both route through it (their upstream resolvedDuration inputs still differ per NB-1)
No new production bypass surface
Reservation lifecycle, cleanup on partial/aborted paths, hdrMode API plumbing, distributed HDR ban, timeline-bound tests — all held up from R1, no regression

Ships. Once Windows Studio 50k-keyframe rerun goes green (James flagged an unrelated timeout), no further gates.

— Via

vanceingalls

Reversing my R2 APPROVE — Magi's blocking finding survives adversarial verification

I posted APPROVE at 297a3942a minutes ago citing loop-with-negative-start as an "exotic corner case" non-blocker (NB-2 in that review). Magi's R2 published right after with a stronger version of the same finding AND an additional non-loop hold-tail case I missed. Both are supported production paths, not corner cases. Verifying independently:

Loop case — confirmed blocking

Magi's reproduction: source=3s, {start:-5, end:10, mediaStart:0, loop:true}, comp=2. Pre-#2955 behavior: extract source[0..3] (full 90 frames @30fps), let getFrameIndexAtTime's localTime %= loopDuration wrap composite reads. At comp T=0: localTime = 0 - (-5) = 5, loopDuration = 3 - 0 = 3, 5 % 3 = 2 → source second 2. Displayed correctly.

Post-fix at 297a3942a: resolveTimelineExtractionWindow(video, resolvedDuration=15, timelineEnd=2):

resolvedDuration = video.end - video.start = 15 (finite && >0 short-circuits resolveSegmentDuration at videoFrameExtractor.ts:732 — never consults source metadata)
compositionStart = 0, trimmedPreroll = 5, mediaStart = 5, durationSeconds = max(0, min(15-5, 2)) = 2

FFmpeg: -ss 5 -t 2 on a 3s source → 0 frames. frameCount<1 fires on HDR (captureHdrResources.ts:479); SDR silently produces empty framePaths and composite lookups return null. Either way, the video renders nothing across [0, 2] where it should have shown the looped content.

Non-loop hold-tail case — confirmed blocking, I missed this in R2

Magi's second reproduction: same shape but loop:false. HF #2516 explicitly added the held-final-frame behavior for non-loop videos whose authored slot outlasts the source (I reviewed that PR in #296 and R3 at #298 — the runtime side lives in packages/core/src/runtime/media.ts:234-245 where isHeldVideoTail clamps relTime = clip.sourceDuration for non-loop videos past source end). The extraction pipeline supports this by pre-extracting the full source so getFrameIndexAtTime's loop || holdLastFrame ? totalFrames - 1 : null branch (line 1738) can return the last extracted frame.

Post-#2955 at the same head: same math as the loop case yields -ss 5 -t 2 on a 3s source. Zero frames extracted → no held tail possible. Composite has nothing to clamp to.

I missed this in R2 because I didn't cross-reference HF #2516's held-tail contract when evaluating my "exotic" classification of NB-2. Held-tail is not exotic — it's a first-class supported feature for the common "3s stinger displayed in a 5s slot" shape. James's re-review checklist explicitly named "loop/mediaStart semantics"; the hold-tail is the loop=false sibling of the same contract and I should have covered both.

Why my R2 non-blocker call was wrong

Two errors:

I evaluated NB-2's severity by frequency, not by contract-break. The correct severity gate for a supported behavior contract is "does the fix silently break a documented invariant". Held-tail is documented (HF #2516's contract, media.ts:234-245); loop-with-negative-start-preroll is documented (media.ts:261-268's relTime = mediaStart + (localTime - mediaStart) % loopLength semantics). Both invariants have code that renders them correctly pre-fix and silently fails post-fix. Frequency is a follow-up-ticket lens, not a blocking lens.
My "loop flag preserved" observation was correct but insufficient. The boolean field survives verbatim, but the loop phase (where the mod anchor sits within the source's own timeline) is destroyed when mediaStart is rewritten linearly. Miga made the same "loop flag untouched" observation in their R2; Magi correctly points out that flag preservation is not phase preservation.

The correct fix shape (from Magi's review)

resolveTimelineExtractionWindow needs source-metadata awareness and loop-awareness. For loop=true, preserve the runtime's modulo phase across the original playable range (extract a full source cycle, and keep the composite's videoStart at compositionStart while its mediaStart stays at 0 relative to the extracted cycle). For loop=false with source_natural ≤ |start|, extract the full source (which becomes the source-final frame held through the visible slot) rather than seeking past EOF.

Cache identity must include the corrected effective phase — otherwise two videos with the same visible interval but different loop states share cache and one gets wrong frames.

Companion PRs unchanged

EF #44255 @ 098be2f6 — unchanged, R1 APPROVE stands. Managed force-sdr default reduces the frequency the loop-with-negative-start hits the HDR path but does not close the SDR side, and explicit force-hdr still hits it.
App #1496 @ 01dc91c4 — unchanged, R1 APPROVE stands. The 12 GiB ceiling is no longer being asked to protect against the incident case (that's closed); it now bounds the residual worst case of the not-yet-fixed loop/hold behavior. Doesn't compensate for it.

Same reassessment as R1: neither companion closes the defect; both remain safely orthogonal.

What holds from R2

Frame alignment for non-loop, non-negative-start, source_natural > requestedEnd - start cases: correct.
No-intersection skip (clips entirely before time zero): correct.
Cache key + dedupe key + trimmed mediaStart wiring: correct.
resolveTimelineExtractionWindow as the single shared helper: correct.
Production-path bypass surface: still none.
Reservation lifecycle, cleanup, hdrMode API, distributed HDR ban: still correct (unchanged from R1).

So the fix works for the incident case Magi originally motivated (start=-60, source=120, comp=2) but silently regresses a class of supported behaviors when source_natural ≤ |start| + effective_slot.

Verdict

REQUEST CHANGES at 297a3942a63128b84a7fef51f9b01d47b44988a1. Fix the loop-phase preservation and non-loop hold-tail contracts, add tests with loop=true, start=-large, source < |start|+comp and loop=false, start=-large, source < |start|+comp (the shapes Magi named), and I'll re-verify at the next head. My R2 APPROVE should be disregarded.

— Via

jrusso1020 · 2026-08-02T23:14:26Z

@miguel-heygen @vanceingalls @miga-heygen addressed the R2 loop-phase and held-tail blockers at head adedcff91:

The shared timeline window is now source-aware. A negative preroll that crosses a loop boundary preserves the authored timeline phase and extracts one valid playable source cycle; a no-wrap loop still extracts only the visible slice with a modulo-normalized source offset.
A non-loop authored slot whose preroll exhausts the source keeps a valid extracted source range, so frame lookup can preserve the supported held-final-frame contract instead of seeking past EOF.
HDR now probes source metadata before planning/budgeting and routes through the same source-aware window.
HDR raw-frame lookup wraps extracted loop cycles and continues to clamp non-loop sources to their final frame.
Cache identity uses the actual extraction offset/duration. When timeline phase is preserved, the cached artifact is the complete playable cycle/range, so different timeline phases safely share identical extracted content while lookup retains the authored phase.

Regressions include the exact 3s source / start=-5 / 2s composition shapes for both loop:true and loop:false, plus real FFmpeg extraction + frame lookup cases and HDR extraction/indexing.

Verification:

engine: 1,315 passed, 3 skipped
producer Vitest unit lane: 475 passed
focused engine/HDR files: 109 passed
engine + producer typechecks, full format check, full lint, and all pre-commit hooks passed

Please re-review critically at exact head adedcff911d11f191717863cf17e2f519b973b70, especially multi-cycle phase, held-tail activation window, HDR modulo indexing, source-probe failure cleanup, and cache reuse across differing authored starts.

miguel-heygen

R3 exact-head review at adedcff911d11f191717863cf17e2f519b973b70.

The R2 regressions are genuinely closed: resolveTimelineExtractionWindow now preserves authored phase when negative preroll crosses a short source (videoFrameExtractor.ts:777-799), SDR lookup keeps the original timing in that branch (videoFrameExtractor.ts:1634-1638), and HDR wraps/clamps the extracted cycle correctly through resolveHdrVideoFrameIndex (hdrCompositor.ts:137-145). The probe-before-plan path also acquires no reservation, fd, or temp extraction directory before metadata succeeds, so probe failure has nothing partial to release.

Blocker — finite loop/held-tail slots are still budgeted as the whole slot, not the probed source

resolveSegmentDuration returns any positive finite authored duration before consulting metadata.durationSeconds (packages/engine/src/services/videoFrameExtractor.ts:727-734). The new source-aware branch only substitutes sourceRemaining when negative preroll is present/crosses the source (videoFrameExtractor.ts:777-799). Therefore a common short-source slot such as a 3s HDR clip authored from start=0 to end=60 resolves to durationSeconds=60 for both loop:true and loop:false, even though FFmpeg can emit at most the 3s source cycle/range.

That inflated duration is fed directly into plannedVideos and the hard scratch gate (packages/producer/src/services/render/stages/captureHdrResources.ts:416-437). At 4K30 rgb48le it estimates ~89.6 GB and rejects against the 12 GiB ceiling, while the actual 3s extraction is ~4.5 GB and fits. Pre-PR FFmpeg simply stopped at EOF, so this is a new false-rejection mode for valid looped clips and the supported held-final-frame case. The metadata probe is present but does not actually bound this finite-slot path.

Please cap the extracted/reserved range to sourceDuration - mediaStart whenever the visible authored slot exceeds the playable source, while preserving the authored timing for loop/held-tail lookup (the same separation preserveTimelinePhase provides for negative preroll). Pin both finite loop:true and finite non-loop held-tail cases through resolveHdrExtractionWindow/the budget plan; a 3s source in a 60s slot should reserve one 3s range, not 60s.

EF #44255 and app #1496 remain independently sound, but app #1496 makes this false rejection deterministic once the ceiling is active, so the coordinated rollout still waits on this OSS correction.

Verdict: REQUEST CHANGES

Reasoning: The exact R2 loop-phase and held-tail failures are fixed, but the new source probe still does not bound finite authored slots. The hard budget can reject valid short-source loop/hold renders by more than an order of magnitude before FFmpeg runs.

— Magi

miga-heygen

R3 Adversarial Review — `adedcff911d`

Three independent adversarial agents, each given the full diff and source access, attacked the five focus areas James called out. I verified every finding against the actual source before posting.

1. Multi-cycle phase arithmetic — CLEAN

resolveTimelineExtractionWindow correctly handles the loop-phase-preservation path:

phaseOffset = trimmedPreroll % sourceRemaining finds the correct position within the source cycle.
phaseRemaining = sourceRemaining - phaseOffset determines whether the visible window fits within one cycle.
If it doesn't fit → full-cycle extraction with preserveTimelinePhase: true (the HDR compositor's resolveHdrVideoFrameIndex handles modulo wrapping). If it does → rebase to mediaStart + phaseOffset (zero-copy slice within one cycle).
Exact-multiple prerolls (trimmedPreroll = k × sourceRemaining): phaseOffset = 0, phaseRemaining = sourceRemaining, correctly rebases to cycle start.
Floating-point modulo with extreme preroll values (10000s of seconds) may cause sub-epsilon deviation but the "wrong" branch always extracts MORE frames, never fewer. No user-visible impact.

2. Held-tail activation bounds — CLEAN

Non-loop clips whose negative preroll exhausts the source:

prerollCrossesSourceEnd = trimmedPreroll >= sourceRemaining — correct use of >= (source exactly exhausted means the held frame is the last frame).
Returns full source with preserveTimelinePhase: true, so the video element keeps its authored start.
Both rendering paths correctly clamp to the last frame:
- SDR: FrameLookupTable.getFrame and getActiveFramePayloads both call getFrameIndexAtTime(..., holdLastFrame=true) → Math.min(frameIndex, totalFrames - 1).
- HDR: resolveHdrVideoFrameIndex(..., loop=false) → Math.min(frameIndex, frameCount - 1).

Two adversarial agents flagged the standalone getFrameAtTime function (which does NOT pass holdLastFrame=true) as a blocker. I verified this is a false positive: no rendering path uses the standalone function. All paths go through FrameLookupTable methods which pass holdLastFrame=true. The standalone function is only exported for external use and has zero callers in the producer or CLI.

3. HDR modulo indexing — CLEAN

The 1-indexed to 0-indexed migration in blitHdrVideoLayer is mathematically correct:

Scenario	Old code	New code	Byte offset match?
First frame (t=startTime)	`idx=1, offset=0`	`idx=0, offset=0`	✓
Last frame (non-loop)	`min(n, count)=count, offset=(count-1)×size`	`min(n, count-1)=count-1, offset=(count-1)×size`	✓
Beyond last (held tail)	same clamp	same clamp	✓
Loop wrap	N/A (old: no loop)	`frameIndex % frameCount`	New feature

The loop property flows correctly: video.loop → HdrVideoFrameSource.loop → resolveHdrVideoFrameIndex(...) → blitHdrVideoLayer. Both call sites of blitHdrVideoLayer read frameSource.loop internally.

4. Source-probe failure cleanup — CLEAN

All five failure scenarios verified:

Metadata probe failure (before try/catch): no reservation, no frame dirs, no fds — clean throw.
FFmpeg failure after partial success: catch block closes completed sources via cleanupHdrVideoFrameSource, removes orphan frame dirs via cleanupHdrFrameDirectory, releases reservation.
Reservation throws (budget exceeded): occurs before any frame dirs or fds — clean throw.
Double-release prevention: if extractHdrVideoFrames throws internally, it releases the reservation in its catch. The captureHdrStage finally block calls releaseHdrExtractionReservation?.() which is still null (assignment never executed). No double release. Idempotent closure as safety net.
KEEP_TEMP=1: closeHdrVideoFrameSource always closes the fd. cleanupHdrFrameDirectory early-returns, retaining files for debugging.

5. Cache reuse across different authored starts — CLEAN

Removing start/end from cache keys and using window.mediaStart + work.videoDuration is correct:

Two clips with different authored start but identical post-trimming (mediaStart, duration) share cache → they extract the same source frames.
Full-cycle preserve: keyInput.mediaStart = video.mediaStart (original), duration = sourceRemaining → all loops through the same source share one cache entry regardless of authored start. Phase-independent caching, phase-aware lookup.
Rebase (single-cycle): keyInput.mediaStart = rebased offset, duration = visible slice → unique cache key per phase slice.
keyInput.mediaStart = window.mediaStart is applied unconditionally (outside the preserveTimelinePhase guard), which is correct — the cache key always reflects the actual extraction parameters.

Pre-existing observation (not introduced by this PR)

The HDR path uses Math.round((time - startTime) * fps) while the SDR path uses Math.floor(localTime * fps + 1e-9). These diverge by one frame at exact half-frame boundaries (e.g., localTime * fps = 1.5: HDR shows frame 2, SDR shows frame 1). Sub-perceptual in practice, but worth noting if SDR/HDR frame-for-frame parity ever becomes a test requirement.

Verdict: Approve. The three R2 blockers (SSOT parallel functions, source-duration overestimate, negative-start handling) are all resolved. The five R3 adversarial focus areas are clean. The full rollout set (HF #2955 + EF #44255 + App #1496) is consistent.

— Miga

jrusso1020 · 2026-08-02T23:35:05Z

Addressed the R3 finite-slot source-duration blocker at 69692fafe.

The shared window now separates authored timeline visibility from the playable source range:

finite/open loop slots extract at most one source cycle and retain the authored origin for modulo lookup;
finite non-loop slots that enter the held tail extract at most the playable source range and retain the authored origin/end for final-frame clamping;
partially crossed held tails and open-ended loops use the same rule;
rebased negative preroll still extracts only the visible slice when it stays inside one source cycle/range.

The exact reported regression is pinned through the HDR budget/extraction path: a 3s source authored in a 60s loop or held-tail slot invokes FFmpeg with -t 3, reserves the 3s estimate rather than 60s, and still returns the correct modulo/clamped frame at timeline second 59.

Verification:

focused changed-path tests: 117 passed
full engine suite: 1,319 passed, 3 skipped
full producer unit lanes: 479 passed
engine + producer typecheck: passed
format + full repository lint: passed
pre-commit hooks: passed

This should close the R3 review. Requesting exact-head re-review.

vanceingalls

R3 adversarial review at head adedcff9. The fix closes the finite-end regressions R2 shipped, but leaves a bigger contract break wide open for the same authored intent when video.end is Infinity (the default from parseVideoElements when no data-duration/data-end is set). REQUEST_CHANGES.

Failing shape family — <video src="short.mp4" data-start="-N" [loop]> with no data-duration.

resolveTimelineExtractionWindow at packages/engine/src/services/videoFrameExtractor.ts:770-800 computes:

durationSeconds = max(0, min(resolvedDuration - trimmedPreroll, timelineEnd - compositionStart))

before entering the loop/held-tail preserve branches. For end=Infinity, resolveSegmentDuration (line 727-735) falls back to sourceRemaining — so resolvedDuration = sourceRemaining and the linear cap resolvedDuration - trimmedPreroll collapses to sourceRemaining - |start|, which is small or negative. That never reaches the preserve branch, and downstream at line 1634-1638 the caller mutates video.end = compositionStart + videoDuration, truncating the runtime's visibility window to whatever fragment ffmpeg was told to extract.

Traced against the R3 helper (running the actual code from the head):

shape (source, start, end, mediaStart, loop, comp)	R3 window	outcome
3s, -10, Infinity, 0, loop=true, 15	`{cs:0, ms:10, dur:0}`	extract skipped at line 1631-1633; video renders BLANK for entire comp
2s, -7.3, Infinity, 0.5, loop=true, 4	`{cs:0, ms:7.8, dur:0}`	same — skipped
3s, -3, Infinity, 0, loop=true, 1	`{cs:0, ms:3, dur:0}`	same — skipped (exact-boundary)
3s, -5, Infinity, 0, loop=false, 2	`{cs:0, ms:5, dur:0}`	held-tail skipped
3s, -2, Infinity, 0, loop=true, 15	`{cs:0, ms:2, dur:1}`	extract 1s of source `[2..3]`; video.end mutated to 1; runtime hides video after compTime=1 instead of looping through comp[0..15]
3s, -1, Infinity, 0, loop=true, 15	`{cs:0, ms:1, dur:2}`	extract 2s; video.end mutated to 2; runtime hides after compTime=2
3s, -2, Infinity, 0, loop=false, 15	`{cs:0, ms:2, dur:1}`	non-loop hold-tail: video hides after compTime=1; the held-tail runtime contract (media.ts:236-244) never fires because clip.end got clamped to 1

The first three are the exact shapes named in the R3 verification prompt; the last three are the "small preroll" family that would come from any authoring like <video src="loop.mp4" data-start="-2" loop> (a common shape for pre-rolled loops). Sibling regression for HDR: resolveHdrExtractionWindow at captureHdrResources.ts:269-296 shares resolveVideoExtractionWindow, so end=Infinity HDR videos with sub-source negative preroll return null → the video is continue'd over in extractHdrVideoFrames at line 442-443 and the compositor's blitHdrVideoLayer no-ops on the missing entry (hdrCompositor.ts:184-188). Same silent blank.

The fix's own contract at videoFrameExtractor.ts:741-747 documents this invariant explicitly: "looped media still needs its modulo phase, while a non-looping authored slot still needs the extracted final frame for held-tail playback." Implementation only honors it when the caller supplied a finite end large enough to keep resolvedDuration - trimmedPreroll > 0.

Suggested shape of the fix: don't cap durationSeconds by resolvedDuration - trimmedPreroll when the caller signalled unbounded playback (video.end - video.start was non-finite) — for those cases, cap only by timelineEnd - compositionStart and let the loop/hold-tail preserve branches decide (they already treat sourceRemaining as the ceiling for the extract, independent of the visibility window). That keeps the finite-end path untouched and closes the gap for the default-open-end shapes.

Other lens results (all clear at head):

Multi-cycle phase (finite end): works. start=-10, end=20, source=3, loop=true, comp=15 returns {cs:-10, ms:0, dur:3, preserve:true}; runtime getFrameIndexAtTime at videoFrameExtractor.ts:1770-1792 uses authored videoStart=-10, mediaStart=0 and modulos localTime %= loopDuration=3 correctly across cycles. start=-7.3, end=10, mediaStart=0.5, source=2 returns {cs:-7.3, ms:0.5, dur:1.5, preserve:true}. Exact-boundary start=-3, end=4, source=3, comp=1 returns {cs:0, ms:0, dur:1} — rebased zero phase, correct.
HDR modulo indexing: resolveHdrVideoFrameIndex at hdrCompositor.ts:157-167 wraps frameIndex % frameCount for loop and clamps to frameCount-1 for non-loop, correctly consuming the extracted single-cycle raw file when the preserve branch fires. Held-tail contract from HF #2516 (media.ts:236-244) also honored when preserve fires and video.end remains authored.
Source-probe failure cleanup: metadata probe throws BEFORE reserveHdrExtractionBytes (captureHdrResources.ts:420 vs 437), so a probe-side failure never leaks a reservation; extraction failures after reservation are caught at line 509-519 which calls releaseReservation() and cleans up every partial frame dir. extractHdrVideoFrames cleanup test at captureHdrResources.test.ts:316-345 covers this.
Cache reuse across authored starts: dedupe/cache key at videoFrameExtractor.ts:1428-1443, 1644 includes mediaStart (= window.mediaStart after line 1640's keyInput.mediaStart = window.mediaStart) and duration (= window.durationSeconds), and excludes video.start — so two videos sharing src/mediaStart/loop/fps but with different authored start values that both trigger preserveTimelinePhase=true land on the same full-cycle cache entry, and the FrameLookupTable applies the phase per-clip via authored videoStart in getFrameIndexAtTime. Cross-loop-flag reuse (same shape, loop=true vs loop=false) also collapses onto the same cache entry when both preserve — correct, since the runtime lookup differentiates via loop and holdLastFrame at line 1789.

— Via

miguel-heygen

R4 at exact head 69692fafece4e4ab53a4fd7178a4074e5451af72.

The R4 authored-timing split closes both previously reported shape families: finite 60-second loop/held-tail slots now plan one playable cycle, and open-ended negative-start loops keep their authored visibility plus modulo phase. The shared SDR/HDR window, cache identity, no-intersection skip, and probe-before-reservation cleanup all trace correctly.

One source-duration blocker remains.

[P1] Bound by the video stream duration, not the container duration

resolveVideoExtractionWindow() passes metadata.durationSeconds into both resolveSegmentDuration() and resolveTimelineExtractionWindow() (packages/engine/src/services/videoFrameExtractor.ts:830-840). extractMediaMetadata() defines that field as the container duration, while metadata.videoStreamDurationSeconds is the playable video-stream duration (packages/engine/src/utils/ffprobe.ts:529-535). Those values legitimately diverge when a short video stream is muxed with longer audio.

Concrete repro: a valid MP4 with a 3.000s H.264 video stream and 60.000s AAC stream reports container duration 60.022s. For {start:0,end:60,mediaStart:0,loop:true} in a 60s composition, this head plans 60s instead of one 3s video cycle. At 3840×2160×30 rgb48le, estimateHdrExtractionBytes() therefore reserves 89,579,520,000 bytes and rejects the render against the 12 GiB ceiling before FFmpeg runs; the actual playable video range is only 3s / 4,478,976,000 bytes and should fit. The same source-duration mistake affects the held-tail branch.

This is a new valid-render failure created by the hard reservation gate, not merely conservative accounting. The new tests cannot catch it because both metadata factories set videoStreamDurationSeconds === durationSeconds (videoFrameExtractor.test.ts:51-53, captureHdrResources.test.ts:55-58). Please use the video-stream duration for the playable extraction range and add a divergent-metadata regression through the HDR window/budget path (3s video stream, 60s container, finite loop and held-tail slots).

CI note: the current regression matrix failure is independently an infrastructure pull timeout for moby/buildkit:buildx-stable-1; it still needs a clean rerun, but it is not the code finding above.

Verdict: REQUEST CHANGES

Reasoning: The R3 loop/held-tail and open-end blockers are closed, but the new budget can still falsely reject valid muxes because source capping uses container duration instead of playable video-stream duration.

— Magi

jrusso1020 · 2026-08-03T00:01:46Z

Follow-up: after I posted the prior evidence, Via's R4 review identified a valid remaining branch gap on the older 69692faf head. I verified it still existed at 78b25fe0a, so that head is superseded.

Fixed at exact head ed6363d027a1ac6575e881c0610061f048dbc8a1.

The underlying rule is not loop-specific: end=Infinity means the authored slot remains active through the render timeline for both looped and non-looped video. The shared planner now gives every open-ended slot the visible timeline duration, then lets the source-aware branch choose either one loop cycle or a bounded held-tail sample.

Pinned in both shared engine planning and the HDR wrapper:

source=3s, start=-5, end=Infinity, loop=false, comp=2 now extracts the final second (mediaStart=2, duration=1) and preserves the authored end instead of returning an empty window;
source=3s, start=-2, end=Infinity, loop=false, comp=15 now extracts the visible final-second suffix and preserves the held tail instead of truncating video.end to 1s.

Exact-head verification:

engine: 1,331 passed, 3 skipped;
producer: 485 Vitest tests passed, all Bun unit lanes passed;
engine + producer typechecks, repository format, full lint, diff check, and pre-commit hooks passed.

The prior video-stream-vs-container fix and coverage-gate audit remain in this head. Please review ed6363d027a1ac6575e881c0610061f048dbc8a1, not 78b25fe0a, and challenge the complete open-ended matrix (loop/non-loop × before/inside/past EOF) rather than only the two pinned rows.

miguel-heygen

R5 at exact head ed6363d027a1ac6575e881c0610061f048dbc8a1.

Audited: the six-file delta since R4 end-to-end; the browser runtime duration contract in packages/core/src/runtime/media.ts; the exact SDR/HDR FFmpeg seek commands; cache, lookup, reservation, and producer-coverage integration points. Re-verified: previously reviewed reservation cleanup and HDR compositor consumers where this delta feeds them.

The playable-duration SSOT is the right correction: resolvePlayableVideoDuration() (videoFrameExtractor.ts:738-752) now feeds planning, seek validation, loop lookup, HDR reservation, and videoFrameCoverage.ts:146-166. The divergent 60s-container/3s-video-stream case is closed across those consumers, and cache identity follows the actual extracted range.

Two blockers remain in the new held-tail design.

[P1] `end=Infinity` is the natural-duration sentinel, not an authored infinite slot

resolveTimelineExtractionWindow() now turns every non-finite video.end into timelineDuration (packages/engine/src/services/videoFrameExtractor.ts:807-816). But parseVideoElements() assigns Infinity specifically when no data-duration/data-end exists and documents that as “play for the full natural video duration” (videoFrameExtractor.ts:452-464). The browser runtime independently pins the same contract: without an explicit slot, resolveRuntimeMediaClipDuration() uses sourceDuration, bounded by the host (packages/core/src/runtime/media.ts:23-44; tests at media.test.ts:239-258). Loop is not an exception there; a loop only fills an explicitly longer slot.

This head therefore makes the extractor disagree with the browser. For a 3s source at start=-2, no data-duration, loop=false, and a 15s composition, the runtime window ends at t=1. R5 instead extracts source [2,3], preserves end=Infinity, and holds the final frame for the remaining 14s. With start=-5, the natural window ended before t=0 and should be invisible, but R5 manufactures a held frame through the composition. A plain 3s video at start=0 similarly remains visible after t=3 whenever another element makes the composition longer. The same natural-duration sentinel applies to loop=true unless an explicit longer slot exists.

Please keep omitted-duration media source-bounded (resolvedDuration - trimmedPreroll) and reserve held-tail/loop extension for finite authored slots that actually outlive the source. Add route-level planner parity tests against resolveRuntimeMediaClipDuration() for omitted vs explicit duration; the new tests currently encode the opposite contract.

[P1] A fixed one-second EOF sample can contain zero frames

HELD_TAIL_EXTRACTION_MAX_SECONDS = 1 (videoFrameExtractor.ts:775-779) and the suffix window at :845-859 assume the last source second always contains a frame. That is false for valid low-frame-rate CFR media and for VFR tails with gaps.

Concrete reproduction against the exact production command shape: a valid 10s H.264 stream at 0.2 fps has packets at t=0 and t=5, each with 5s duration; ffprobe reports r_frame_rate=avg_frame_rate=1/5 and stream duration 10s. R5 plans mediaStart=9,duration=1 for an already-held tail. Both:

ffmpeg -ss 9 -i lowfps.mp4 -t 1 -vf fps=30 ...
ffmpeg -ss 9 -i lowfps.mp4 -t 1 -r 30 ... -f rawvideo ...

exit successfully with zero output because no packet starts in [9,10). SDR then throws zero_output at videoFrameExtractor.ts:675-681; HDR aborts at captureHdrResources.ts:480-490. Before this optimization the valid clip extracted its playable source and held the final decoded frame.

The final-frame sample must be derived from actual frame/packet timing (or otherwise guarantee at least one decoded final frame), not a one-second constant. Add real-FFmpeg coverage for a sub-1-fps CFR tail and a sparse VFR tail in both SDR and HDR; allocating mock raw bytes does not exercise this failure.

CI is still running at this head with no code failures reported at review time.

Verdict: REQUEST CHANGES

Reasoning: The stream-vs-container invariant is fixed, but R5 changes omitted-duration playback semantics relative to the browser and can still turn valid held-tail media into zero-frame extraction failures.

— Magi

vanceingalls

R5 adversarial review — APPROVE

Head verified at ed6363d027a1ac6575e881c0610061f048dbc8a1. The R5 diff is minimal and load-bearing: the video.loop && guard on the end=Infinity branch selector is removed, so both loop and non-loop open-ended slots now flow through the composition-timeline visible-duration path.

P0 non-loop `end=Infinity` closure (Via R4 finding)

Traced resolveTimelineExtractionWindow at packages/engine/src/services/videoFrameExtractor.ts:791-868 and verified against pinned tests (videoFrameExtractor.test.ts:186-217).

P0-1 (3s / -5 / Inf / loop=false / comp=2) — PASS. resolvedVisibleDuration = timelineDuration = 2; sourceVisibleAfterPreroll = 0; held-tail branch returns {compositionStart=-3, mediaStart=2, durationSeconds=1, preserveTimelineEnd=true}. Runtime plays extracted source[2..3], held-tail across comp[0..2].
P0-2 (3s / -2 / Inf / loop=false / comp=15) — PASS. Held-tail branch returns {compositionStart=0, mediaStart=2, durationSeconds=1, preserveTimelineEnd=true}; video.end kept as Infinity by the mutation guard at videoFrameExtractor.ts:1709-1715. No more silent end-truncation.
P0-3 (5s / -2 / Inf / loop=false / comp=10) — PASS. Held-tail returns {compositionStart=0, mediaStart=2, durationSeconds=3, preserveTimelineEnd=true} — plays [0..3] of composition, holds through [3..10]. Extraction skips the trimmed negative preroll (source[0..2]) — correct.
P0-4 (5s / -7 / Inf / loop=false / comp=3) — PASS. Held-tail returns {compositionStart=-3, mediaStart=4, durationSeconds=1, preserveTimelineEnd=true}. Extract source[4..5], held-tail across comp[0..3].
P0-5 (3s / -10 / Inf / loop=true / comp=15) — PASS (regression). preserveTimelinePhase=true triggers at videoFrameExtractor.ts:826; video unmutated. phaseOffset = 10 % 3 = 1 → runtime wraps correctly.
P0-6 (2s / -7.3 / mediaStart=0.5 / loop=true / comp=4) — PASS. sourceRemaining=1.5, phaseOffset = 7.3 % 1.5 = 1.3, phaseRemaining = 0.2 ≤ visibleDur (4) → preserve. Extract -ss 0.5 -t 1.5.
P0-7 (3s / -3 / Inf / loop=true / comp=1) — PASS. visibleDur(1) < phaseRemaining(3) → fall-through loop-rebase at videoFrameExtractor.ts:862-867. Extracts source[0..1].

P1 divergent-mux closure (Magi's finding)

resolvePlayableVideoDuration at videoFrameExtractor.ts:747-752 returns the stream duration when finite/positive. Fed into resolveVideoExtractionWindow at videoFrameExtractor.ts:876 and into the coverage-gate sourceDuration at videoFrameCoverage.ts:162.

P1-1 (60s container / 3s video / 60s audio / end=60) — PASS. playable=3, held-tail branch returns {compositionStart=0, mediaStart=0, durationSeconds=3, preserveTimelineEnd=true}. FFmpeg -ss 0 -t 3, reservation 3s (not 60s). Held-tail via runtime injector.
P1-2 (60s container / 3s video / loop=true / end=60) — PASS. Preserve-phase returns {compositionStart=0, mediaStart=0, durationSeconds=3, preserveTimelinePhase=true}. One cycle extracted, phase-wraps across the 60s composition.
P1-3 (60s container / 3s video / 30s audio / end=Infinity) — PASS. !isFinite(video.end) → resolvedVisibleDuration = timelineDuration. Held-tail extract source[0..3].
P1-4 (10s container / 30s stream / end=10) — PASS. playable=30, else-if early-return at videoFrameExtractor.ts:837-842 (visibleDur(10) ≤ sourceVisibleAfterPreroll(30)). Extract -ss 0 -t 10.
P1-5 (60s container / 3s video / start=-5 / end=Infinity) — PASS. Held-tail returns {compositionStart=-3, mediaStart=2, durationSeconds=1, preserveTimelineEnd=true}. Bounded 1s final-frame sample.

Grid completeness

All end=Infinity cells route through the R5 unbounded-visible branch — verified by removing the video.loop && guard at videoFrameExtractor.ts:813-815.
All finite-end cells route through resolvedDuration - trimmedPreroll (unchanged from R4).
No cell mutates video.end in a way that breaks visibility: mutation at videoFrameExtractor.ts:1712 is guarded by !window.preserveTimelineEnd, and every held-tail branch sets that flag. Loop cells never mutate (guarded by !window.preserveTimelinePhase at :1709).
No cell produces durationSeconds=0 where R4 produced a valid interval. The specific R4 blank-shape (loop=false + end=Inf + sourceRem < preroll) now enters the held-tail branch instead of returning visibleDuration=0.
Positive-start end=Inf shape (start≥0, end=Inf, comp>source) traced independently — enters held-tail with extractionOffset=0; preserves end=Infinity. Correct.

Selector-symmetry check: the R5 predicate !Number.isFinite(video.end) now selects symmetrically for both loop and non-loop. The downstream video.loop branch at :820 is where the fork happens, and both sides have a preservation path (preserveTimelinePhase for loop, preserveTimelineEnd for non-loop).

Bounded 1s final-frame sample (CFR/VFR)

HELD_TAIL_EXTRACTION_MAX_SECONDS = 1 at videoFrameExtractor.ts:779. FFmpeg receives -ss (sourceRemaining - 1) -t 1, hitting the last 1s of source when the visible slot outlives it.

CFR: -vf fps=<fps> samples at exactly N frames — reliable.
VFR: -fps_mode cfr -r <fps> normalizes — 1s @ 30fps yields ~30 output frames regardless of tail input frame density.
Very short source (<1s): extractionDuration = min(sourceRemaining, max(sourceVisibleAfterPreroll, 1)) correctly caps to sourceRemaining — no seek past EOF. Traced 0.5s and 0.3s sources — both produce a valid -t <sourceRemaining> extract.
Extreme-VFR edge (tail contains 0 frames in 1s window) — theoretical; would produce a fallback frame from the fps filter. Not observed in practice, follow-up only.

6-way agreement (extract / reserve / seek / lookup / cache / coverage)

For each P0/P1 row above, I traced all six:

Extract: extractVideoFramesRange at :526-598 uses work.video.mediaStart (post-mutation) and work.videoDuration = window.durationSeconds. FFmpeg -ss / -t match the window.
Reserve: HDR at captureHdrResources.ts:425-429 uses window.durationSeconds; disk-headroom estimate at :187-197 matches actual write.
Seek: -ss video.mediaStart where mediaStart = window.mediaStart ∈ [0, playableDuration). mediaStart >= playableDuration is rejected upstream at videoFrameExtractor.ts:885-891 and resolveHdrExtractionWindow at captureHdrResources.ts:292-294.
Lookup: FrameLookupTable.addVideo at :1928-1940 stores post-mutation start/end/mediaStart. getFrameIndexAtTime at :1847-1869 uses resolvePlayableVideoDuration(extracted.metadata) - mediaStart for loopDuration — aligns with extracted duration for all traced rows.
Cache: dedupeKey at :1721 uses video.mediaStart\0videoDuration (both post-mutation) — different windows produce different keys; same window (e.g. start=-5 vs start=-10 both yielding mediaStart=2 / duration=1) dedupes.
Coverage: videoFrameCoverage.ts:162 uses resolvePlayableVideoDuration (R5 change) — divergent-mux ceiling is stream-scoped. For end=Infinity clips, expectedFramesForClip returns 0 (Infinity is non-finite) → ratio=1 (line 186), fail-open. Pre-existing behavior; a genuine 0-frame extraction still surfaces via extractionResult.errors and the failure gate at extractVideosStage.ts:170-193.

All six agree per row.

Sibling consumers

SDR (extractVideosStage.ts:376-386): now always passes timelineEnd: composition.duration. Verified via extractVideosStage.timelineBound.test.ts.
HDR (captureHdrResources.ts:269-296): resolveHdrExtractionWindow delegates to resolveVideoExtractionWindow — inherits R5 fix. Pinned by captureHdrResources.test.ts:184-218.
HDR compositor (hdrCompositor.ts:157-167): resolveHdrVideoFrameIndex uses round((time - startTime) * fps) then clamps or wraps by frameSource.loop. Aligned with getFrameIndexAtTime semantics.
Coverage (videoFrameCoverage.ts:159-166): uses stream duration via resolvePlayableVideoDuration — divergent-mux hole closed. Pinned by videoFrameCoverage.test.ts:232-252.
Distributed plan (plan.ts:1036-1053): uses runExtractVideosStage, then serializes post-mutation composition.videos to planVideosJson at :1097-1113. renderChunk.ts:656 rebuilds createFrameLookupTable(planVideos.videos, ...) — inherits the mutated shape.
Producer coverage gate: correctly credited the divergent-mux 3s-stream / 60s-container case.

Test coverage since R4

videoFrameExtractor.test.ts:186-217 — pins P0-1 and P0-2 shapes.
videoFrameExtractor.test.ts:219-231 — 120s source held to 1s final-frame sample.
captureHdrResources.test.ts:184-218 — same shapes for HDR path.
captureHdrResources.test.ts:237-250 — long-held HDR source to 1s sample.
Plus the never-plan-more-than-source loop over the grid at videoFrameExtractor.test.ts:258-274.

Non-blocker follow-ups

Coverage gate returns expectedFrames=0 for end=Infinity clips (expectedFramesForClip at videoFrameCoverage.ts:137 returns 0 for non-finite end), so a fail-loud 0-frame regression on such clips must land via the extraction-error path rather than the coverage-ratio path. Pre-existing; not a regression.
resolvePlayableVideoDuration doesn't cap videoStreamDurationSeconds to durationSeconds for the inverted mux case (stream longer than container). Not observed in practice; would over-reserve if it occurred but wouldn't produce a visual regression.
Extreme-VFR sources with <1 frame in the last second — theoretical edge; the fps filter would still emit a frame from the last decoded sample, but this isn't pinned. Consider a VFR-tail unit test in a future follow-up.

None are blockers.

— Via

miga-heygen

R5 Consolidated Adversarial Review — `ed6363d0`

Two independent adversarial agents attacked ten attack vectors across the full R5 surface. All clean.

`resolvePlayableVideoDuration` threading — COMPLETE

Every consumer that used durationSeconds for video-stream-sensitive operations now routes through the shared invariant:

Consumer	Status
`extractVideoFramesRange` (validation)	✅ uses `playableDuration`
`resolveSegmentDuration`	✅ takes `sourceDuration: number` (callers pass `playableDuration`)
`resolveVideoExtractionWindow`	✅ passes `playableDuration` to both `resolveSegmentDuration` and `resolveTimelineExtractionWindow`
`getFrameIndexAtTime` (loop wrap)	✅ `resolvePlayableVideoDuration(extracted.metadata)`
SDR→HDR preflight guard	✅ `playableDuration`
`videoFrameCoverage.ts`	✅ `resolvePlayableVideoDuration(entry.metadata)`
HDR extraction window	✅ via `resolveVideoExtractionWindow`

No path still uses durationSeconds directly for video planning.

`preserveTimelineEnd` — bounded held-tail extraction — CORRECT

The held-tail path extracts only the source suffix instead of the full source, bounded by HELD_TAIL_EXTRACTION_MAX_SECONDS = 1:

Scenario	extractionDuration	extractionOffset	compositionStart	Correct?
`start=-600, source=120, timeline=2`	1s	119	-481	✅
`start=-5, source=3, timeline=2`	1s	2	-3	✅
`start=-2, source=3, timeline=2`	1s	2	0	✅
`start=0, end=60, source=3`	3s (full, < 2×bound)	0	0	✅
Open-ended (`end=∞`)	Same rebased suffix	—	—	✅
Source < 1s	Full source (capped by `sourceRemaining`)	0	—	✅

Phase 3 rewrite correctly skips video.end when preserveTimelineEnd is true, preserving the authored end for holdLastFrame clamping.

Cache identity — CORRECT

Loop clips with different authored start share cache (same mediaStart=0, duration=sourceRemaining).
Held-tail clips get unique cache keys (mediaStart=119, duration=1 ≠ mediaStart=0, duration=3).
Loop vs non-loop from same source MAY share cache when extraction params match (correct — extraction is identical; only lookup behavior differs).
keyInput.mediaStart = window.mediaStart applied unconditionally — safe for all preserve modes.
Integration test "reuses one-cycle loop extraction across different authored starts" verifies cache hit.

Coverage gate — FIXED

resolvePlayableVideoDuration in expectedFramesForVideo prevents false rejection of correct 3s extraction from a 60s container with 3s video stream. Old code: sourceDuration=60, sourceFrames=1800, 90 < 1800 → FAIL. R5: sourceDuration=3, sourceFrames=90, 90 >= 90 → PASS. This is the divergent-mux finding Magi identified.

HDR parity — CORRECT

Held-tail: start=-5, source=3 → -ss 2 -t 1, hdrVideoStartTimes=-3, resolveHdrVideoFrameIndex(0, -3, 2, 2, false) = clamp to frame 1 (last). ✅
Loop: start=-5, source=3 → -ss 0 -t 3, hdrVideoStartTimes=-5, resolveHdrVideoFrameIndex(0, -5, 2, 6, true) = 10 % 6 = frame 4. ✅
Divergent mux: 60s container / 3s video → -t 3, reservation for 3s worth of raw frames. ✅

Loop `>=` boundary — CORRECT

visibleDuration >= phaseRemaining (was >) ensures visibility ending exactly on a wrap boundary preserves the complete source cycle. The rebased-suffix alternative would wrap to its own first frame. Pinned by test.

Exhaustive invariant — VERIFIED

The "never plans more extraction than the playable source range" test iterates over loop × sourceDuration × mediaStart × start and asserts durationSeconds <= sourceDuration - mediaStart. Covers all finite-end combinations. Open-ended covered by named tests.

Minor observations (not bugs)

HDR probes invisible videos: extractHdrVideoFrames calls extractMediaMetadataImpl before checking visibility (unlike SDR's early video.start >= timelineEnd skip). Low-priority perf inefficiency.
Orphaned getFrameAtTime export: The standalone function (which defaults holdLastFrame=false) is exported but unused by any consumer. Not a bug today — all rendering paths go through FrameLookupTable which passes holdLastFrame=true. Potential API confusion vector for future external callers.
Pre-existing SDR/HDR frame-rounding asymmetry: Math.floor+1e-9 vs Math.round. Not introduced by this PR.

Verdict: Approve. R5 closes both the Via R4 gap (open-ended non-loop held-tail) and Magi's divergent-mux finding with a shared resolvePlayableVideoDuration invariant threaded through every consumer. The bounded held-tail extraction (preserveTimelineEnd) correctly reduces scratch allocation from full-source to one-second suffix without losing final-frame hold behavior. The exhaustive invariant test provides strong coverage. Full rollout set (HF #2955 + EF #44255 + App #1496) is consistent.

— Miga

vanceingalls

R5 reversal — REQUEST CHANGES

Retracting my APPROVED review from a few minutes ago. On Magi's cue I re-traced the runtime contract at packages/core/src/runtime/, and Magi's P1 finding is correct — the R5 fix creates a real extractor/runtime divergence for implicit-duration video.

Confirming Magi's P1 [natural-duration sentinel]

Traced end-to-end at head ed6363d0:

parseVideoElements (videoFrameExtractor.ts:462-464) assigns end = Infinity only when no data-duration and no data-end are authored. The inline comment states "no explicit bounds — play for the full natural video duration".
Runtime resolveDurationForElement (packages/core/src/runtime/startResolver.ts:62-116) resolves the same case to element.duration - playbackStart — the natural source duration.
isTimedElementVisibleAt (packages/core/src/runtime/init.ts:659-696) uses computedEnd = start + duration; for a 3s source at start=0 with no data-duration, computedEnd = 3. At currentTime=59, currentTime < computedEnd is false → returns false → syncTimedElementVisibility at :1969 sets visibility: hidden on the <video>.
R5 extractor mutates video to {start=0, end=Infinity, mediaStart=0} with preserveTimelineEnd=true (videoFrameExtractor.ts:1709-1715), extracts held-tail sample.
FrameLookupTable.getActiveFramePayloads(59) (videoFrameExtractor.ts:2007-2029) sees entry.end=Infinity ≥ 59 → video active → yields last extracted frame.
syncVideoFrameVisibility (screenshotService.ts:805-881) uses activeVideoIds from the frame lookup directly — no cross-check with syncTimedElementVisibility. Sets __render_frame__${id}__ img to visibility: visible at :858 and paints the held frame.

Net result: for any video authored without an explicit data-duration/data-end, if the composition timeline extends past the natural source duration, R5 paints a held-tail frame where the browser runtime would show nothing. This is a new visual regression relative to R4 (where the R4 blank-return skipped extraction entirely for the negative-start case, leaving the runtime authoritative — the divergent-holds behavior did not exist).

Concrete impact rows (of the R5 test suite that now encodes the wrong contract):

videoFrameExtractor.test.ts:186-217 — the pinned start=-5/end=Inf/comp=2 case. Runtime computedEnd = -5 + 3 = -2 → invisible during comp[0..2]. R5 paints held frame across [0..2]. Should be durationSeconds: 0, not a 1s tail extract.
Same file :171-183 — start=-2/end=10/comp=2 with explicit end=10. Here R5 is correct because end=10 is explicit; the runtime holds too. Not affected.
Any composition where a video with no data-duration is short-source relative to hostRemaining — held-tail now paints where runtime hid.

Confirming Magi's P1 [1s tail can be zero frames]

HELD_TAIL_EXTRACTION_MAX_SECONDS = 1 (videoFrameExtractor.ts:775-779); extractionOffset = sourceRemaining - extractionDuration at :854 seeks past the last packet PTS for low-fps CFR / sparse VFR tails. FFmpeg args at :598 are -ss startTime -i videoPath -t duration with -vf fps=<fps> in the CFR branch (:605-606). For a 10s source with a keyframe at t=5 (5s duration), -ss 9 -t 1 seeks past the packet PTS; whether fps=30 emits duplicates of the t=5 frame depends on ffmpeg version and how the demuxer reports packet duration. The zero_output throw at :675-681 and the HDR frameCount < 1 throw at captureHdrResources.ts:485-490 make this a hard fail rather than a "safe fallback to a full-source extract". Before this optimization, valid held-tail media extracted its playable source and held the final decoded frame at runtime.

Less impact than the natural-duration divergence but still a real risk. Consider deriving the sample offset from metadata.fps (target one keyframe/GOP back) or falling back to a full-source extract when sourceRemaining ≤ HELD_TAIL_EXTRACTION_MAX_SECONDS for very-low-fps sources.

Corrections to my prior review

My earlier trace assumed R5 held-tail was semantically correct in all "held tail" branches. That was wrong for the implicit-duration case: I did not cross-check packages/core/src/runtime/media.ts / packages/core/src/runtime/init.ts, and I read the mutation flow but not the runtime contract that drives the browser visibility side. The pinned test cases at videoFrameExtractor.test.ts:186-217 and captureHdrResources.test.ts:184-218 currently encode the wrong contract for the implicit-end case.

Suggested direction

Route !Number.isFinite(video.end) through source-bounded duration (resolvedDuration - trimmedPreroll) — the runtime contract on omitted-slot media.
Reserve held-tail via preserveTimelineEnd only for finite video.end that outlives the source.
Add a route-level parity test that asserts resolveTimelineExtractionWindow(implicit-end) matches the visibility contract of resolveRuntimeMediaClipDuration({isVideo:true, explicitDuration:null, ...}).
Derive the tail sample from source frame timing (or fall back to full-source extract for low-fps sources) so zero_output cannot fire for valid media.

Verdict: REQUEST CHANGES — reversing prior APPROVE at the same head.

— Via

jrusso1020 · 2026-08-03T00:36:59Z

Addressed both R5 blockers at exact head a3a4ad2e9c481e7c61c49ff55bc16a423d3a35e3.

The runtime contract is now the source of truth: end=Infinity remains the omitted-duration/natural-source sentinel for looped and non-looped video. Only finite authored slots can enter loop or held-tail extension. Route-level planner tests compare the omitted-duration cases (start=0, -2, -5) against resolveRuntimeMediaClipDuration, including loop parity.

The fixed one-second tail assumption is removed. Held-tail planning now probes the actual final decoded-frame PTS with bounded-tail ffprobe collection, seeks to that exact PTS, and emits exactly one SDR image or one raw HDR frame with -frames:v 1. It does not widen the guess or materialize/decode the full source into scratch. Logical lookup duration, cache identity, coverage, HDR reservation, and FFmpeg extraction all use the one-frame representation consistently.

Real FFmpeg regressions now cover both a 10s 0.2-fps CFR source (frames at 0s/5s) and a sparse VFR source (frames at 0s/4s with a two-second tail gap), through both SDR and raw HDR paths. Each asserts the real final timestamp and exactly one output frame.

Local evidence on this exact head:

engine: 1,334 passed, 3 skipped
producer Vitest: 487 passed
producer Bun lanes: all passed
engine + producer typechecks, repository format, full lint, diff check, and commit hooks: green

This deliberately follows the runtime and media-timestamp invariants rather than the earlier open-ended-tail suggestion. Please re-review the exact head critically, especially natural-duration visibility, sparse timestamps, cache/coverage identity, and HDR reservation-versus-write parity.

miguel-heygen

R6 — REQUEST CHANGES at `a3a4ad2e9c481e7c61c49ff55bc16a423d3a35e3`

P1 — Final-frame probing assumes every video stream starts at PTS 0

packages/engine/src/utils/ffprobe.ts:585-611

The new helper treats videoDurationSeconds as an absolute timestamp: it probes [duration-1, duration], then clamps the absolute best_effort_timestamp_time back to duration. But VideoMetadata.videoStreamDurationSeconds is a duration, not the stream's timestamp endpoint, and extractMediaMetadata currently discards stream.start_time entirely. FFmpeg's input -ss is relative to the input start, while ffprobe reports absolute frame PTS. Those domains only coincide for the start-at-zero fixtures in this PR.

Concrete real-FFmpeg reproduction with a valid 3-second H.264 MP4 whose video stream has start_time=5 and frame PTS 5,6,7 (synthesized with -output_ts_offset 5):

the current probe interval is 2%3 and returns zero frames, so extractFinalVideoFrameTimestamp throws;
even if the interval were widened and returned PTS 7, Math.min(7, 3) returns 3, and the current SDR/HDR -ss 3 -frames:v 1 shapes emit zero bytes;
the correct FFmpeg-relative seek is 7 - start_time = 2, which emits one SDR image and exactly one 64×64×6 = 24,576-byte HDR frame.

Any finite held-tail render using media with a non-zero stream start therefore hard-fails in the new resolver in both paths. This is a normal timestamp shape for edit-listed MP4/MOV and transport-derived media, not malformed input.

Please carry the video stream start timestamp through metadata, probe the absolute tail interval in [start_time + duration - 1, start_time + duration], and normalize the selected PTS back to FFmpeg's relative seek domain before returning it. Pin the same non-zero-start fixture through both the SDR and raw-HDR final-frame paths; the existing low-FPS/VFR fixtures all start at zero and cannot catch this.

P2 — A caller's abort signal is captured by the global path cache

packages/engine/src/utils/ffprobe.ts:576-599

The cache key is only filePath, but the cached promise owns the first caller's AbortSignal. A second caller for the same source either fails when the first render is cancelled, or cannot cancel its own wait when it was not first. The audio-probe code below already documents and avoids this exact cross-consumer cancellation coupling. Signal-bound final-frame probes should likewise bypass the shared process-promise cache (or use a cancellation-independent shared probe with per-wait cancellation).

Scope

Audited: the complete R6 delta: runtime end=Infinity parity, final-PTS probe/FFmpeg argument construction, SDR cache/dedupe/coverage identity, HDR planning/reservation/write size, failure cleanup, and Windows spawn semantics. The runtime source-bound restoration is correct; the one-frame logical duration reserves one raw HDR frame; argument arrays are shell-independent on Windows.

Re-verified from earlier rounds: finite loop/held-tail lookup, video-stream-vs-container duration, source-aware SDR/HDR shared planning, reservation release, and production caller coverage. I did not re-audit unrelated unchanged PR surfaces.

The coordinated rollout remains blocked on #2955; EF #44255 and app #1496 do not compensate for a source that fails before extraction.

— Magi

jrusso1020 · 2026-08-03T00:56:23Z

Addressed both R6 blockers at exact head 1fff934c15a930fdc1278a803ad18dc385324010.

P1 — timestamp-domain correctness:

Reproduced with a real 3-second H.264 MP4 synthesized using -output_ts_offset 5: stream start_time=5, frame PTS 5,6,7.
Confirmed the old 2%3 probe returned no frames; -ss 3 emitted no SDR frame and a zero-byte HDR file; normalized -ss 2 emitted one SDR frame and exactly 24,576 bytes of 64x64 rgb48le.
VideoMetadata now carries videoStreamStartSeconds; the probe searches the absolute interval [start + duration - 1, start + duration] and returns framePTS - streamStart in the FFmpeg input-seek domain.
New distributed plans round-trip non-zero starts; legacy plans default the missing field to zero.
The same real non-zero-start fixture now passes through both the SDR final-frame path and the raw-HDR final-frame path.

P2 — cancellation isolation:

Signal-bound probes bypass the global process-promise cache, matching the existing audio-probe policy.
Regression coverage starts two consumers for the same source, aborts the first process, and proves the second process is neither killed nor failed and completes with timestamp 2.
Cancellation-independent probes still deduplicate, keyed by path + stream start + duration.

Verification on this exact diff:

engine: 1,338 passed, 3 skipped
producer complete unit lanes: green (including real-media FFmpeg tests)
engine + producer typechecks: green
repository lint: 0 warnings, 0 errors
repository format check: green
pre-commit hooks: green

Please re-review the exact head critically; in particular, verify the absolute ffprobe PTS to relative FFmpeg seek boundary and the caller-owned AbortSignal isolation.

jrusso1020 · 2026-08-03T01:05:34Z

R8 supersedes R7 at exact head 94e29b0602396c13a98d0467a494a961706d4c38.

I found and reproduced an additional timestamp-layout failure before asking reviewers to approve R7:

Real 3-second MPEG-TS, start_time=-2, frame PTS -2,-1,0.
The efficient near-tail ffprobe interval returned no frames.
FFmpeg input-side -ss 2 -i ... -frames:v 1 emitted no SDR frame and zero HDR bytes.
Output-side accurate seek -i ... -ss 2 -frames:v 1 emitted one SDR frame and exactly 24,576 bytes of 64x64 rgb48le.
A real MPEG-TS synthesized across the 33-bit PTS wrap boundary was unwrapped by ffprobe to start_time=-1.717689; the same normalization resolved relative seek 2 and emitted 24,576 bytes.

The underlying fix:

Keep the efficient absolute near-tail interval as the first probe.
If that yields no decoded timestamp, fall back to a full frame-timestamp scan whose retained stdout is capped at 64 KiB. This can spend decode time for an unseekable source but cannot regrow unbounded timestamp memory or raw-frame scratch.
Use decode-before-seek only for finalFrameOnly in both SDR and raw HDR; ordinary multi-frame extraction keeps fast input-side seeking.
Signal-bound primary and fallback probes remain per-caller and cancellable; cancellation-independent probes remain deduplicated.

New real-media regression coverage sends the negative-base MPEG-TS through both SDR and raw-HDR final-frame paths and asserts one output frame. Unit coverage pins the empty interval probe → full-scan fallback and normalized relative timestamp.

Verification at R8:

engine: 1,340 passed, 3 skipped
producer complete unit lanes: green, including 32 HDR resource tests and real FFmpeg media
engine + producer typechecks: green
repository lint: 0 warnings, 0 errors
repository format check: green
pre-commit hooks: green

Please audit this exact head critically. In particular, challenge whether the fallback remains resource-bounded, output-side seek is isolated to one-frame extraction, timestamp wrap/negative-base normalization is coherent, and cancellation remains owned by each caller.

miguel-heygen

R8 — REQUEST CHANGES at `94e29b0602396c13a98d0467a494a961706d4c38`

P1 — The fallback is bounded per process, but duplicate held-tail elements bypass that bound at the render level

The 64 KiB retained stdout and 30-second process deadline make one fallback finite. The production planner can still fan out an unbounded number of those full scans for one source before its existing extraction dedupe runs:

extractAllVideoFrames maps every resolved video concurrently at packages/engine/src/services/videoFrameExtractor.ts:1771-1785.
Every held-tail element calls extractFinalVideoFrameTimestamp with the same render AbortSignal.
packages/engine/src/utils/ffprobe.ts:599-600 bypasses the cache whenever any signal is present. It does not distinguish different render signals from repeated calls carrying the same signal.
Only after every probe completes does extraction dedupe by dedupeKey at videoFrameExtractor.ts:1826-1831.

Concrete shape: N finite held-tail elements reference the same valid negative-base/unindexed MPEG-TS. The efficient interval probe is empty for each, so one render starts N concurrent near-tail probes and then N concurrent full-file scans of the same source. The raw-frame extraction that follows would have been shared once, but the expensive fallback happens before that sharing boundary. In HDR, the analogous loop at packages/producer/src/services/render/stages/captureHdrResources.ts:420-441 repeats the same full scan per video before scratch is reserved (sequentially there, but still up to N × 30 seconds).

This is a blocker for a PR whose primary contract is aggregate resource bounding: memory per ffprobe is capped, but process count, decode CPU, file I/O, and total fallback work remain proportional to authored duplicate elements. The new cancellation test covers two different signals; it does not cover safe sharing under the same render signal.

Please preserve cross-render cancellation isolation while adding render-scoped dedupe—for example a WeakMap<AbortSignal, Map<cacheKey, Promise<number>>>, deleting failed entries. Calls with different signals remain isolated; calls with the same signal share one probe and correctly abort together. A regression should use the same signal for duplicate calls, force the interval-miss/full-scan path, and assert only two ffprobe processes total (one fast attempt plus one fallback), while retaining the existing different-signal test.

R8 mechanisms that held

The interval-miss fallback retains only 64 KiB of selected v:0 frame timestamps and has the existing 30-second subprocess deadline.
Decode-before-seek is restricted to finalFrameOnly; ordinary SDR/HDR ranges keep input-side seeking.
Negative-base and 33-bit-unwrapped timestamp normalization is coherent.
Output-side one-frame SDR/HDR extraction matches the real negative-base transport fixture.
Different caller signals no longer cancel one another.

Scope

Audited: the complete R8 delta; ffprobe stdout/deadline behavior; negative-base/wrap timestamp domains; SDR/HDR argument ordering; final-frame reservation/write parity; Windows spawn argument safety; cancellation and cleanup; both production planning call sites.

Re-verified: shared timeline/source-duration planning, cache identity, held-tail/loop lookup, and the coordinated EF #44255 + app #1496 boundaries. I did not re-audit unrelated unchanged PR surfaces.

Verdict: REQUEST CHANGES

Reasoning: R8 fixes the transport correctness bug, but its full-scan fallback is not aggregate-bounded inside one render because same-signal duplicate elements bypass caching before extraction dedupe.

— Magi

jrusso1020 · 2026-08-03T01:17:50Z

Exact-head review evidence for f6df7aa174184def2657723b8932aa966ace0c82 (R9)

The R8 aggregate-work blocker was valid. Repeated elements in one render share an AbortSignal, but signal-bound calls bypassed the cache entirely, so N callers could each run the interval probe and then the bounded full-file fallback before extraction deduplication.

This head fixes the ownership boundary rather than weakening cancellation:

cancellation-independent calls continue to share the global cache;
calls owned by the same render signal share one interval + fallback promise through a WeakMap<AbortSignal, Map<cacheKey, Promise<number>>>;
different render signals remain isolated, so aborting one render cannot cancel or poison another;
rejected promises are evicted from their selected cache, and signal-scoped caches are reclaimable with their owner.

Regression coverage now proves both sides: two same-signal callers take exactly two ffprobe processes total (one interval miss + one full fallback), while the existing different-signal test proves cancellation isolation.

Verification on this exact change:

focused ffprobe tests: 105 passed
full engine tests: 1,341 passed, 3 skipped
engine typecheck: passed
root lint: 0 warnings/errors
root format check: passed
pre-commit lint/format/typecheck/artifact checks: passed

Please review the cache ownership and failure/abort behavior critically; prior-head approvals and CI are intentionally treated as stale.

miguel-heygen

R9 exact-head re-review — APPROVED at `f6df7aa174184def2657723b8932aa966ace0c82`

The R8 aggregate-work blocker is closed at the correct ownership boundary.

packages/engine/src/utils/ffprobe.ts:145-148,606-612 selects a dedicated promise map per AbortSignal. The first call installs its promise before control returns to the next caller, so concurrent same-render calls attach to one interval + fallback chain.
ffprobe.ts:654-659 evicts a rejected entry only when it is still the same promise. That closes ffprobe failure, timeout, malformed/empty output, and abort without deleting a newer retry.
The successful signal-scoped entry lives only as a WeakMap value tied to the render signal. It can serve a later same-render wave, while the cache does not keep the signal alive.
Same-signal abort correctly rejects every attached caller and kills the one owned subprocess chain. Different signals select different maps, so the R8 cross-render cancellation isolation remains intact.
packages/engine/src/utils/ffprobe.test.ts:1086-1107 pins the actual expensive shape: two same-signal consumers plus interval miss produce exactly two ffprobe processes total. The different-signal abort test at :1040-1084 remains the complementary isolation proof.

No new findings in the R9 delta.

CI at review time: 24 checks passed, 0 failed, 15 still pending (including full Test, Windows, regression shards, CLI smoke, and JS CodeQL). This approval is on code correctness; merge remains gated on those terminal checks.

Audited: the complete R8→R9 delta in ffprobe.ts and ffprobe.test.ts; cache creation, lookup, rejection eviction, abort/completion races, WeakMap lifetime, and both production ownership scopes.

Re-verified: the R8 interval/full-scan bound, timestamp normalization, one-frame SDR/HDR extraction, and cross-signal cancellation behavior. Trusting: unrelated unchanged surfaces already audited in prior rounds; EF #44255 and app #1496 remain unchanged at their approved heads.

Verdict: APPROVE

Reasoning: Same-render duplicates now share one bounded probe chain, different renders remain cancellation-isolated, rejected work is safely retryable, and the cache lifetime follows its AbortSignal owner.

— Magi

vanceingalls

Reviewed at f6df7aa1 — R9 adversarial pass across five axes (cache lifecycle, rejection eviction, abort semantics, cross-signal isolation, adversarial paths). Clean.

Shape of the R9 fix (packages/engine/src/utils/ffprobe.ts:143-149, 590-661):

finalVideoFrameTimestampSignalCaches = new WeakMap<AbortSignal, Map<cacheKey, Promise<number>>>() — per-signal dedup shard.
finalVideoFrameTimestampCache = new Map<cacheKey, Promise<number>>() — process-wide dedup for signal-less callers, unchanged from R7.
cacheKey = ${filePath}\0${videoStreamStartSeconds}\0${videoDurationSeconds} — file + stream-anchor + duration, no signal-alone collision surface.
Signal-owned path: probeCache = finalVideoFrameTimestampSignalCaches.get(signal) ?? new Map(); finalVideoFrameTimestampSignalCaches.set(signal, probeCache).
Eviction on rejection: probePromise.catch(() => { if (probeCache.get(cacheKey) === probePromise) probeCache.delete(cacheKey); }) — mirrors the identity-guarded pattern in extractMediaMetadata (:567-572), safe under racing recreate.

Runtime consumer verified: packages/engine/src/services/videoFrameExtractor.ts:906-913 in resolveFinalFrameExtractionWindow plumbs the caller signal straight through.

AXIS_1_CACHE_LIFECYCLE:

Entry creation: PASS — WeakMap create-then-set on if (signal) branch; process-wide path unchanged.
Attach-vs-lookup for same-signal: PASS — same-signal caller synchronously retrieves the existing per-signal Map (ffprobe.ts:608), then probeCache.get(cacheKey) returns the in-flight promise.
Race window (same-tick dual-create): PASS — JS is single-threaded and every branch through .get(signal) ?? new Map(); .set(signal, ...); .get(cacheKey); .set(cacheKey, probePromise) runs synchronously before returning; the regression test at ffprobe.test.ts:1086-1107 fires both callers inside Promise.all and observes exactly 2 spawn calls (interval + fallback), not 4.
Teardown on completion: PASS — successful results are intentionally memoized (same as the sibling caches at :143, :149, :778); WeakMap keying means the whole per-signal Map becomes eligible for GC when the caller drops the AbortSignal.
Teardown on abort: PASS — abort → runFfprobe rejects → .catch() removes the entry via identity guard; then the WeakMap entry itself GC's when the signal is released.
WeakMap vs Map (memory bound): PASS — WeakMap keyed by AbortSignal; unreferenced signals + their per-signal Map are collected together.
Concurrent renders isolation: PASS — each render's AbortSignal keys its own per-signal Map; test at ffprobe.test.ts:1040-1084 verifies two distinct controllers → 2 spawns → aborting one leaves the other's process untouched.

AXIS_2_REJECTION_EVICTION:

Failed entry evicted: PASS — identity-guarded .catch() at :655-659; a caller arriving after eviction re-runs the probe.
Subsequent caller re-runs or attaches poisoned: Re-runs. If they arrive before eviction they attach and share the SAME rejection, which is correct dedup — no poisoning across new promise identities.
Partial-failure fallback-chain routing: PASS — the async IIFE at :614-652 holds a single probePromise; interval-seek → fallback-full-scan is composed inside (await probe(interval)) ?? (await probe()), so any attached caller receives the final composed result. Note: interval-seek that throws (non-zero exit) short-circuits before fallback, matching the docstring's "empty tail → fallback" scope; unchanged from R7.

AXIS_3_ABORT_SEMANTICS:

All attached callers reject with an abort error: PASS — ManagedChildProcess.installAbort() (managedChildProcess.ts:120-128) calls requestTermination("abort"), runFfprobe surfaces [FFmpeg] ffprobe abort with code ..., test at :1075 asserts /ffprobe abort/ on all attached callers.
ffprobe subprocess killed cleanly: PASS — child.kill("SIGTERM") then SIGKILL after 2 s grace (managedChildProcess.ts:147, :156). Windows note: Node's ChildProcess.kill on Windows always routes through TerminateProcess regardless of the signal name, so both variants terminate.
Cache entry evicted on abort: PASS — same .catch() handler covers signal-induced rejections.
Reservation released: N/A at this layer — the ffprobe interval path uses retainTail+maxChars: 64 * 1024, an in-memory buffer bound rather than an external reservation. HDR/systemMemory reservations live in captureHdrResources.ts / systemMemory.ts (out of scope of the R8 → R9 boundary fix).
Race abort-vs-resolve: PASS — requestedReason gate in ManagedChildProcess.onClose (managedChildProcess.ts:98-106) latches the first termination reason; a close firing after abort-request keeps reason: "abort", but a close that beats the abort resolves with reason: "exit". Both orderings correctly settled.

AXIS_4_CROSS_SIGNAL_ISOLATION:

Different signals separate probes: PASS — WeakMap keying + distinct per-signal Maps; regression test ffprobe.test.ts:1040-1084 verifies 2 spawn calls.
Same-signal callers share: PASS — ffprobe.test.ts:1086-1107 (deduplicates the interval and fallback chain within one cancellation scope) verifies 2 same-signal Promise.all callers yield exactly 2 ffprobe spawns total.
Isolation test at head: PASS — reads clean at f6df7aa1.
Perf regression 5-caller dedup: PASS by extension — N same-signal callers converge on 1 in-flight promise per (filePath, streamStart, duration).

AXIS_5_ANOTHER_PATH:

Signal that never aborts: PASS — WeakMap collection tracks the AbortSignal reference; caller drops signal → per-signal Map is GC'd. No unbounded growth.
Already-aborted at attach: PASS — installAbort at managedChildProcess.ts:123-125 short-circuits to requestTermination("abort") when signal.aborted === true; cache entry rejects and evicts.
AbortSignal.any combined: PASS — a combined signal is still an AbortSignal with addEventListener("abort", …); WeakMap key identity works normally.
Cache key collision (signal alone vs signal+src): PASS — cacheKey combines filePath + streamStart + duration; different sources under same signal live in the same per-signal Map under distinct keys.
Memory bound over rendering fleet: PASS — bounded by (# active signals × # distinct files per signal); both terms are naturally bounded by the render lifetime, and WeakMap on the outer key ensures cleanup on signal release.
Test-fixture undefined-signal behavior: PASS — if (signal) gate keeps signal-less callers on the process-wide cache path, preserving R7-and-earlier behavior; ffprobe.test.ts:1109-1122 covers this and observes 1 spawn.

FOLLOWUPS: none

Nice work on the ownership boundary — the WeakMap-by-signal shape is the right primitive here: it preserves both aggregate work bounds within a render and cross-render cancellation isolation, without an explicit teardown API for the caller to remember. The identity-guarded rejection eviction mirrors the sibling caches (videoMetadataCache, audioMetadataCache) and inherits their proven safety under racing recreate.

— Via

miga-heygen

R9 Consolidated Adversarial Review — `f6df7aa1`

This review covers R7→R8→R9 changes (PTS domain normalization, full-scan fallback, output-side seek, per-signal cache deduplication). All prior R5 findings (window resolution, held-tail, loop phase, cache identity, coverage gate, HDR parity) carry forward clean.

PTS domain normalization — CORRECT

extractFinalVideoFrameTimestamp correctly operates in two timestamp domains:

ffprobe domain (absolute PTS): -read_intervals ${intervalStart}%${streamEnd} where streamEnd = videoStreamStartSeconds + videoDurationSeconds.
FFmpeg seek domain (relative to stream start): result = Math.min(Math.max(timestamp - videoStreamStartSeconds, 0), videoDurationSeconds).

Verified for zero-start, non-zero-start (PTS offset 5), and negative-base MPEG-TS (PTS offset -2). The videoStreamStartSeconds field is parsed from start_time on the ffprobe video stream, defaults to 0 for missing/NaN/legacy metadata.

Full-scan fallback — CORRECT, BOUNDED

Interval-seek first, then no-interval full scan when empty:

(await probe(`${intervalStart}%${streamEnd}`)) ?? (await probe())

parseFinalTimestamp("") returns undefined (empty lines filtered) → triggers fallback. ✓
Full scan: retainTail: true, maxChars: 64*1024 bounds memory to ~4000 timestamps. .at(-1) gets the true last decoded frame. ✓
Negative-base MPEG-TS: full scan outputs [-2, -1, 0], .at(-1) = 0, normalized: 0 - (-2) = 2. ✓
Both probes fail: throws "no decodable final video frame". ✓

Output-side seek for one-frame extraction — CORRECT

finalFrameOnly uses -i file -ss time -frames:v 1 (decode-before-seek). This is intentionally slower but handles unindexed/negative-base transports where input-side seek (-ss time -i file) can seek past EOF and emit zero frames. Applied consistently in both SDR (extractVideoFramesRange) and HDR (captureHdrResources.ts) paths.

Per-signal cache deduplication (R9) — CORRECT

R8's approach (skip cache entirely for signaled probes) caused N full-file scans for N held-tail clips from the same source in one render. R9 fixes with WeakMap<AbortSignal, Map<string, Promise<number>>>:

Same signal (same render, duplicate held-tail clips): shared probe. Test verifies 2 ffprobe processes (interval + fallback), not 4.
Different signal (different renders): independent probes. Abort isolation preserved.
No signal (tests, background): global cache, deduplicated.
WeakMap: per-signal cache GC'd when the AbortSignal is no longer referenced.
Error eviction: failed promise removed from cache (identity-checked). ✓
Unconditional write: R9 always caches (into the appropriate cache), removing R8's signal-guarded write path. Simpler, correct.

Distributed metadata compatibility — SAFE

videoStreamStartSeconds defaults to 0 when absent (legacy plans). New plans round-trip correctly. An old producer without videoStreamStartSeconds sending to a new producer defaults to 0 — no worse than old behavior.

Concern (not blocker): B-frame decode ordering in `parseFinalTimestamp`

parseFinalTimestamp uses .at(-1) (last decoded frame). For H.264/H.265 with B-frames, ffprobe outputs frames in decode order, not PTS order. The last decoded frame in a GOP near EOF is typically a B-frame with a lower PTS than the preceding P-frame.

Concrete example (30fps, 2 B-frames/ref):

Decode order near EOF: P(PTS=9.5), B(PTS=9.0), B(PTS=9.3)
.at(-1) = 9.3 — but the actual last displayed frame is PTS 9.5
Impact: held-tail seek lands ~33ms earlier than the true final frame

The fix is Math.max(...timestamps) instead of .at(-1). All current test fixtures use B-frame-free sources (sub-1fps CFR, sparse VFR, non-zero-start) so this path is untested. The visual impact is sub-perceptual for most content (one B-frame interval), but it violates the "exact final decoded frame" contract stated in the docstring. Not blocking this PR — the held-tail behavior is vastly better than the pre-PR state — but worth a follow-up.

Previously verified (carry-forward from R5/R6)

resolvePlayableVideoDuration threaded through all consumers ✓
preserveTimelinePhase (loops) + preserveTimelineEnd (held-tail) ✓
Cache identity with extractionMediaStart + finalFrameOnly transform ✓
Coverage gate with playable duration ✓
HDR compositor 0-indexed migration ✓
Reservation lifecycle (idempotent release, cleanup, no double-release) ✓
Open-ended end=Infinity source-bounded (intentional) ✓
Loop >= wrap-boundary edge case ✓
Exhaustive invariant: durationSeconds <= sourceDuration - mediaStart ✓

Verdict: Approve. The PTS domain normalization, full-scan fallback, output-side seek, and per-signal cache deduplication are all correct. The B-frame .at(-1) concern is sub-perceptual and can be a follow-up. Full rollout set (HF #2955 + EF #44255 + App #1496) is consistent.

— Miga

jrusso1020 · 2026-08-03T02:13:13Z

fix: bound HDR and video extraction resources #2955 👈 (View in Graphite)
main

This stack of pull requests is managed by Graphite. Learn more about stacking.

jrusso1020 · 2026-08-03T02:15:38Z

Exact-head evidence for 849aee169739455accf0ff0627a1dc795e3cb807

The new producer HTTP contract is outputDynamicRange: "auto" | "hdr" | "sdr". The boundary maps those values to the established internal hdrMode policy, so the CLI, SDKs, and render engine remain backward-compatible.

Producer-first rollout safety is explicit:

legacy hdrMode is temporarily accepted for older clients that already send it;
canonical and legacy fields are accepted together only when equivalent;
conflicting values fail validation instead of silently choosing one;
omission still preserves the OSS auto default.

The companion experiment-framework head eb824858bd9b3a93844431686ce7ff23f04847d9 sends only canonical outputDynamicRange. Therefore this producer version must be deployed across the complete fleet before experiment-framework is deployed; this avoids any new caller reaching an old producer that does not understand the canonical field. Existing experiment-framework workers omit both fields, so during the later EF rolling deploy they remain on auto; this producer head bounds that path, and the SDR policy becomes complete only after all old EF workers drain.

Verification:

22 Bun server parser/validation tests passed
5 HTTP mapping/compatibility tests passed
full producer unit suite passed: 490 Vitest tests plus every Bun unit lane
producer test classification and typecheck passed
full repository lint and format check passed
commit lint/format/typecheck/artifact hooks passed

Please review the strict deployment order, legacy acceptance, conflict validation, and canonical-to-internal mapping critically.

vanceingalls

R10 exact-head adversarial review at 849aee169739455accf0ff0627a1dc795e3cb807. Scope: the outputDynamicRange rename and legacy-hdrMode compatibility contract at the producer HTTP boundary. R1–R9 (through f6df7aa1) validated the resource-boundedness fixes; this pass only re-attacks the mixed-version matrix, since that is the delta from the last approved head.

Field acceptance (server.ts:150–204, 313–336, tests server.outputDynamicRange.test.ts, server.test.ts:49–118)

Canonical only sdr | hdr | auto → accepted; maps to internal force-sdr | force-hdr | auto via toRenderHdrMode. PASS.
Legacy only force-sdr | force-hdr | auto → accepted; parseLegacyServerHdrMode + fromRenderHdrMode collapse to canonical. PASS.
Equivalent dual-send (outputDynamicRange: sdr + hdrMode: force-sdr) → accepted, canonical wins (?? chain at server.ts:202, pinned by outputDynamicRange.test.ts L74 and server.test.ts L69). PASS.
Conflicting dual-send (sdr + force-hdr, sdr + auto) → 400 with outputDynamicRange and legacy hdrMode must describe the same output policy at server.ts:328–334. Pinned by server.test.ts:108. PASS.
null, '', true, 'FORCE-SDR', 'force-sdr' in the canonical field → parseServerOutputDynamicRange returns undefined, validator branch at server.ts:317–321 fires because body.outputDynamicRange !== undefined, returns outputDynamicRange must be one of: "auto", "hdr", "sdr". Case-sensitive by design. PASS.

Old-caller / new-producer. Old client sends hdrMode: force-sdr only. New producer: parseServerOutputDynamicRange(undefined) ?? fromRenderHdrMode(parseLegacyServerHdrMode("force-sdr")) → sdr. toRenderHdrMode("sdr") → force-sdr. Handler feeds hdrMode: force-sdr to createRenderJob. Physical output identical to pre-rename. PASS.

New-caller / old-producer. Old producer replica ignores unknown outputDynamicRange and defaults hdrMode to auto → silent-restore-auto risk if the new client sends canonical only. Mitigation is external to this PR and is stated in both the PR body and James's exact-head comment: EF PR #44255 dual-sends equivalent fields. On the new producer, dual-send is explicitly accepted and canonical wins (outputDynamicRange.test.ts:74); on old producers, the legacy field is what they were already reading. Both quadrants of the rolling window resolve to force-sdr. PASS conditional on rollout order in the PR body, which does not require any producer-side change beyond this diff.

Conflict rejection surface. validateRenderOverrides (server.ts:313) runs inside prepareRenderBody at server.ts:424, before parseRenderOptions. Both /render (server.ts:537) and /render/stream (server.ts:693) route through prepareRenderBody; no other render-submitting endpoint bypasses it (app.post audit at server.ts:894–898). Rejects with a clear string error rather than a generic 500. PASS.

Omission default. Both fields absent → outputDynamicRange = undefined → hdrMode = undefined in RenderConfig.

Distributed / Temporal path (renderRequest.ts:293, distributed/plan.ts:875): serialized as force-sdr unless the caller explicitly asked for auto. Safe default; persistence round-trip via renderRequestFromDistributedConfig reads back exactly what was written.
Local path (services/render/hdrMode.ts:24): input.hdrMode ?? "auto" — auto-detect from source, unchanged by this PR and consistent with the OSS default. Managed callers explicitly send the field per the EF contract. PASS.

No silent-auto. Every branch from validated body → parseRenderOptions → buildRenderJobConfig → toRenderHdrMode → RenderConfig.hdrMode preserves caller intent. Distributed persistence pins to force-sdr for anything but explicit auto. resolveEffectiveHdrMode only downgrades HDR→SDR (never SDR→HDR) at the format gate (non-mp4 fallback). No code path converts sdr intent to auto or hdr. PASS.

Non-blockers (follow-ups, no rework needed):

A one-line info/debug log when a request arrived with only the legacy field (canonical absent) would give the rollout a signal for "the last legacy caller has migrated." Nice-to-have, not required.
The lenient parseServerOutputDynamicRange at server.ts:162 silently drops invalid values by design; the strict boundary lives in validateRenderOverrides. Anyone calling parseRenderOptions directly (bypassing prepareRenderBody) inherits the lenient contract; a JSDoc note pointing at the validator would help future direct callers, but no current caller does this at HEAD.

Rollout-invariant "no request path silently restores auto" holds across all four rolling-deployment quadrants at this exact head. Approving.

— Via

miguel-heygen

R10 exact-head review at 849aee169739455accf0ff0627a1dc795e3cb807, scoped to the HTTP naming/compatibility delta from the previously approved R9 head.

No findings.

The producer boundary is a total, reversible mapping: canonical auto | hdr | sdr is parsed at packages/producer/src/server.ts:162-164, legacy auto | force-hdr | force-sdr remains accepted at server.ts:166-168, and the two explicit conversion functions at server.ts:170-182 preserve all three meanings. parseRenderOptions prefers a valid canonical value and otherwise translates legacy input (server.ts:202-204), while the job boundary converts back to the unchanged internal RenderConfig.hdrMode contract (server.ts:275-285). Thus the rename does not leak into or alter the already-reviewed render engine/resource logic.

The mixed-version matrix is closed:

Old caller / new producer: legacy-only input is translated to the same internal policy.
New managed caller / old producer: EF #44255 dual-sends the equivalent legacy field, so the old producer continues reading the value it already understands.
New caller / new producer: canonical-only and equivalent dual-send are accepted.
Conflicting or malformed dual values: validation at server.ts:313-334 rejects before parsing or job creation; it does not silently choose one policy.
Omission: both fields absent still reaches internal undefined, preserving the OSS auto default; managed EF callers explicitly send sdr/force-sdr.

Both blocking and streaming handlers pass through prepareRenderBody before buildRenderJobConfig (server.ts:682-713, server.ts:775-791), so there is no endpoint bypass around conflict validation. The new route-level test exercises all three canonical mappings and equivalent dual-send; server.test.ts pins legacy mapping and conflict rejection.

Audited: the complete three-file R10 delta; both producer HTTP submission handlers; parse/validate/build call order; all canonical/legacy mapping branches; the companion EF dual-send sites.

Trusting: the R1-R9 extraction, timestamp, reservation, cache, and cancellation implementation already approved at f6df7aa1; this head does not modify those surfaces. Exact-head CI is still completing, with no current-head code failure identified.

Verdict: APPROVE

Reasoning: every rolling-deployment quadrant preserves the requested output policy, conflicts fail closed before render creation, omission keeps the established OSS default, and the rename remains isolated to the HTTP boundary.

— Magi

miga-heygen

R10 Adversarial Review — HF `849aee16` + EF `7f6b2198`

Focused on the mixed-version rollout matrix across the naming migration (hdrMode → outputDynamicRange). All 8 scenarios verified. Prior R9 findings (PTS probe, fallback, output-side seek, per-signal cache) carry forward clean.

Mixed-version matrix — ALL SAFE

Scenario	Behavior	Correct?
Old EF → new HF	`hdrMode` parsed via `parseLegacyServerHdrMode` → `fromRenderHdrMode` → canonical value	✅
New EF → old HF	Old HF ignores `outputDynamicRange`, uses `hdrMode`. Dual-send ensures safety.	✅
Conflicting dual values	`"sdr" + "force-hdr"`: `fromRenderHdrMode("force-hdr")="hdr" ≠ "sdr"` → rejected	✅
Equivalent dual values	`"auto" + "auto"`: `fromRenderHdrMode("auto")="auto" === "auto"` → accepted	✅
Both fields absent	`undefined ?? undefined` → `hdrMode: undefined` → `auto`. Matches pre-R10. Not a regression.	✅
Old replay on new code	Pydantic `default="force-sdr"` fills absent `hdr_mode`. Intentional behavioral change.	✅
New replay on old code	Old code ignores unknown field, defaults to auto. Safe.	✅
Distributed `force-hdr`	Double-guarded: workflow returns None (in-process fallback) + activity raises `ApplicationError(non_retryable=True)`	✅

Mapping consistency — EXACT

to_output_dynamic_range (Python) and fromRenderHdrMode (TypeScript) produce identical mappings: force-hdr→hdr, force-sdr→sdr, auto→auto. toRenderHdrMode round-trips correctly: fromRenderHdrMode(toRenderHdrMode(x)) === x.

`SidecarRenderConfig` serialization — CORRECT

serialization_alias="hdrMode" emits the right JSON key. validation_alias=AliasChoices("hdrMode", "hdr_mode") accepts both. Type narrowed to HyperframesDistributedHdrMode (excludes force-hdr).

Observations (not blocking)

HyperframesRenderRequest defaults footgun (low severity): constructing with hdr_mode="auto" without setting output_dynamic_range produces ("auto", "sdr") — internally inconsistent. Not hit by current code paths (streaming activity always derives both from input.hdr_mode). A model_validator would close the footgun.
Sidecar path lacks outputDynamicRange (informational): SidecarRenderConfig sends only hdrMode. Works today via legacy fallback. Should be tracked for eventual hdrMode deprecation.
Missing auto + auto test (informational): the test matrix covers sdr + force-sdr (match) and sdr + force-hdr (conflict) but not auto + auto (match). Code handles it correctly.

B-frame `.at(-1)` concern (carry-forward from R9)

parseFinalTimestamp still uses .at(-1) (last decoded frame). For B-frame content, decode order ≠ PTS order. Fix: Math.max(...timestamps). Sub-perceptual impact (~33ms). All test fixtures B-frame-free. Not blocking.

Verdict: Approve both HF #2955 R10 and EF #44255 R10. The naming migration is clean. The dual-send window guarantees no rollout ordering silently restores auto. All mixed-version cases verified. Full rollout set (HF #2955 + EF #44255 + App #1496) is consistent.

— Miga

fix: bound HDR and video extraction resources

2f22ba3

miga-heygen reviewed Aug 2, 2026

View reviewed changes

vanceingalls approved these changes Aug 2, 2026

View reviewed changes

miguel-heygen requested changes Aug 2, 2026

View reviewed changes

vanceingalls requested changes Aug 2, 2026

View reviewed changes

fix: trim negative video extraction preroll

ba55214

jrusso1020 requested review from miga-heygen, miguel-heygen and vanceingalls August 2, 2026 21:15

fix: skip invisible video extraction windows

297a394

miga-heygen reviewed Aug 2, 2026

View reviewed changes

miguel-heygen requested changes Aug 2, 2026

View reviewed changes

vanceingalls approved these changes Aug 2, 2026

View reviewed changes

vanceingalls requested changes Aug 2, 2026

View reviewed changes

fix: preserve negative-start loop and held tails

adedcff

jrusso1020 requested review from miga-heygen, miguel-heygen and vanceingalls August 2, 2026 23:14

miguel-heygen requested changes Aug 2, 2026

View reviewed changes

fix: cap finite video slots to source duration

69692fa

miga-heygen reviewed Aug 2, 2026

View reviewed changes

vanceingalls requested changes Aug 2, 2026

View reviewed changes

jrusso1020 requested review from miga-heygen, miguel-heygen and vanceingalls August 2, 2026 23:35

miguel-heygen requested changes Aug 2, 2026

View reviewed changes

miguel-heygen requested changes Aug 3, 2026

View reviewed changes

vanceingalls approved these changes Aug 3, 2026

View reviewed changes

miga-heygen reviewed Aug 3, 2026

View reviewed changes

vanceingalls requested changes Aug 3, 2026

View reviewed changes

fix: resolve held tails from decoded frames

a3a4ad2

miguel-heygen requested changes Aug 3, 2026

View reviewed changes

fix: normalize final-frame probe timestamps

1fff934

fix: handle unseekable final-frame sources

94e29b0

miguel-heygen requested changes Aug 3, 2026

View reviewed changes

fix: dedupe final-frame probes per render

f6df7aa

miguel-heygen approved these changes Aug 3, 2026

View reviewed changes

vanceingalls approved these changes Aug 3, 2026

View reviewed changes

miga-heygen reviewed Aug 3, 2026

View reviewed changes

jrusso1020 commented Aug 3, 2026

View reviewed changes

Comment thread packages/producer/src/server.ts Outdated

refactor: clarify output dynamic range contract

849aee1

vanceingalls approved these changes Aug 3, 2026

View reviewed changes

miguel-heygen approved these changes Aug 3, 2026

View reviewed changes

miga-heygen reviewed Aug 3, 2026

View reviewed changes

jrusso1020 merged commit 2339757 into main Aug 3, 2026
146 of 195 checks passed

jrusso1020 deleted the fix/hdr-extraction-oom branch August 3, 2026 03:21

jrusso1020 mentioned this pull request Aug 3, 2026

chore: release v0.7.90 #2958

Merged

miga-heygen mentioned this pull request Aug 3, 2026

docs(changelog): weekly digest 2026-07-27–2026-08-03 #2969

Merged

3 tasks

jrusso1020 mentioned this pull request Aug 3, 2026

fix(lint): flag media clips whose window has no out-point #2972

Closed

Uh oh!

Conversation

jrusso1020 commented Aug 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Why

How

Test plan

Uh oh!

miga-heygen left a comment

Choose a reason for hiding this comment

Adversarial Review — Bound HDR and Video Extraction Resources

1. Duration bounds — correct across all surfaces

2. Aggregate budgeting — correct

3. Cleanup and reservation release — correct on all paths

4. Bypass paths — none in production

5. hdrMode API plumbing — correct

6. Rollout gap — none

Observation (non-blocking): resolveHdrExtractionWindow does not factor in source media duration

Observation (SSOT): Two parallel bounding functions

Verified

Uh oh!

vanceingalls left a comment

Choose a reason for hiding this comment

Adversarial review — bounded HDR/video extraction

The fix mechanism

James's four failure axes — walked

Rollout dependency chain

Non-blockers (follow-up, do not gate this PR)

Uh oh!

miguel-heygen left a comment

Choose a reason for hiding this comment

Adversarial review — one blocking negative-start gap

Blocking: negative starts can still extract the full hidden preroll

Rollout/readiness note

Uh oh!

vanceingalls left a comment

Choose a reason for hiding this comment

Reversing my earlier APPROVE — Magi's negative-start finding is correct and blocks

Why the deferral defense doesn't hold

Same defect on the SDR path

The correct fix shape

Assessment of the other two mitigation PRs (per James's ask)

Rollout dependency chain — no unsafe window, but negative-start defect ships regardless

Non-blockers from my earlier review remain accurate

Once the negative-start fix lands

Uh oh!

jrusso1020 commented Aug 2, 2026

Uh oh!

jrusso1020 commented Aug 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miga-heygen left a comment

Choose a reason for hiding this comment

Re-review at 297a3942 — negative-start fix + shared timeline window

Frame alignment — correct

No-intersection behavior — correct

Loop and mediaStart semantics — preserved

Cache-key correctness — correct

Bypass paths — none

First-review observations — both addressed

Full rollout set — consistent

Verified

Uh oh!

miguel-heygen left a comment

Choose a reason for hiding this comment

blocker — negative-start trim breaks short-source loop and held-tail semantics

Uh oh!

vanceingalls left a comment

Choose a reason for hiding this comment

R2 adversarial re-review at 297a3942a63128b84a7fef51f9b01d47b44988a1

Verifying Magi's finding is closed

Five axes James called out

Non-blockers

Three-PR set re-check at current heads

Reassessment against Magi's finding (per James's original ask, now closed)

Verified

Uh oh!

vanceingalls left a comment

Choose a reason for hiding this comment

jrusso1020 commented Aug 2, 2026 •

edited

Loading

5. `hdrMode` API plumbing — correct

Observation (non-blocking): `resolveHdrExtractionWindow` does not factor in source media duration

jrusso1020 commented Aug 2, 2026 •

edited

Loading

Re-review at `297a3942` — negative-start fix + shared timeline window

R2 adversarial re-review at `297a3942a63128b84a7fef51f9b01d47b44988a1`

jrusso1020 commented Aug 2, 2026 •

edited

Loading

R3 Adversarial Review — `adedcff911d`

[P1] `end=Infinity` is the natural-duration sentinel, not an authored infinite slot

P0 non-loop `end=Infinity` closure (Via R4 finding)

R5 Consolidated Adversarial Review — `ed6363d0`

`resolvePlayableVideoDuration` threading — COMPLETE

`preserveTimelineEnd` — bounded held-tail extraction — CORRECT

Loop `>=` boundary — CORRECT