perf(engine): segment-scope SDR→HDR preflight#445
perf(engine): segment-scope SDR→HDR preflight#445jrusso1020 wants to merge 1 commit intoperf/v2/phase-instrumentationfrom
Conversation
convertSdrToHdr now takes startTime + duration and limits the re-encode to the segment the composition actually uses. Mirrors the VFR→CFR fix: a 30-minute SDR source contributing a 2-second clip was transcoded in full pre-fix (a >100× waste); post-fix only the used segment is touched and entry.video.mediaStart is zeroed so downstream extraction seeks from 0. Phase 2 now captures the full VideoMetadata per resolvedVideos entry (previously only colorSpace) so segDuration can fall back to the source's natural duration when video.end is Infinity without an extra ffprobe. Validation: /tmp/hf-fixtures/hdr-sdr-mixed-scope hdrPreflightMs: >1000 → 150 (-87%) videoExtractMs: 1272 → 237 (-82%) tmpPeakBytes: 8.2MB → 4.5MB (-45%) Adds a regression test that synthesizes a 10s SDR + 2s HDR fixture inline and asserts the converted SDR file's duration matches the 2s used segment, not the 10s source.
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
miguel-heygen
left a comment
There was a problem hiding this comment.
Review: perf(engine): segment-scope SDR→HDR preflight
Correct approach — mirrors the established VFR segment-scope pattern. -ss before -i (input seeking), -t after -i, smart metadata reuse avoids extra ffprobe, shallow copy prevents caller mutation. The claimed perf numbers (87% drop in hdrPreflightMs) are consistent with scoping a 30-min transcode to 2 seconds.
P1 — Important (inherited, not a regression)
mediaStart beyond source duration → silent empty output. If entry.video.mediaStart > metadata.durationSeconds, -ss seeks past EOF and FFmpeg silently produces a 0-byte file. The subsequent entry.videoPath = convertedPath then points to a broken file. Same edge case exists in the VFR path. Since this code is being touched, worth adding:
if (startTime >= metadata.durationSeconds) { errors.push(...); continue; }P2 — Should fix
No validation that duration > 0 before passing to FFmpeg. If end === start, segDuration = 0, FFmpeg receives -t 0, and silently produces an empty output. One-line guard:
if (duration <= 0) {
throw new Error(`SDR→HDR conversion: duration must be positive (got ${duration})`);
}P3 — Suggestions
- Test lives inside the "on a VFR source" describe block but is about HDR preflight scoping. A separate
describeblock (or renaming to something broader like "extractAllVideoFrames preflight normalization") would improve readability. - Test uses
extractVideoMetadatawhile production code usesextractMediaMetadata. If the latter is canonical going forward, the test should match. - Benchmark reproducibility — the PR body references
/tmp/hf-fixtures/hdr-sdr-mixed-scopewhich isn't committed. Consider adding a script reference for anyone wanting to reproduce the numbers.
Verdict: Approve with minor suggestions
Core change is correct and well-tested. Push for the duration > 0 guard — it's a one-line fix that prevents a silent failure mode.

What
Scopes the SDR→HDR preflight re-encode to the segment the composition actually uses, mirroring the existing VFR→CFR segment-scope fix.
Why
convertSdrToHdrwas re-encoding entire source files, so a 30-minute SDR screen recording contributing a 2-second clip in a mixed HDR/SDR composition ate multi-second preflight time that produced frames no one would ever read. Validated on a mixed 30s-SDR + 2s-HDR fixture:hdrPreflightMsdrops 87% (1162→148ms),videoExtractMsdrops 82% (1272→231ms),tmpPeakBytesdrops 45% (8.2MB→4.5MB).Depends on #444 (phase-level instrumentation) for the measurement surface.
How
convertSdrToHdrgainsstartTimeanddurationparameters ahead of the upstreamtargetTransferarg added by feat(engine): wire options.hdr through chunkEncoder + dynamic SDR→HDR transfer #370. New signature:convertSdrToHdr(input, output, startTime, duration, targetTransfer, signal, config).-ss $start -t $durationis added to the ffmpeg args.VideoMetadataperresolvedVideosentry (previously justcolorSpace) so the caller can computesegDurationfromvideo.end - video.startwith a fallback tometadata.durationSeconds - video.mediaStartfor unbounded (Infinity) clips — without firing another ffprobe.entry.video.mediaStartis zeroed out via shallow-copy (doesn't mutate the caller'sVideoElement) so downstream extraction seeks from 0 instead of the original offset. Mirrors what the VFR→CFR path already does.Test plan
Validation on
/tmp/hf-fixtures/hdr-sdr-mixed-scope: