Skip to content

fix(producer): credit held video tails in coverage gate #2606

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
miguel-heygen merged 2 commits into from
Jul 17, 2026
Merged

fix(producer): credit held video tails in coverage gate #2606

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fb3e06f
fix(producer): credit held video tails in coverage gate
miguel-heygen Jul 17, 2026
7382ef7
test(producer): decouple makeExtracted's durationSeconds default from…
miguel-heygen Jul 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 41 additions & 0 deletions packages/producer/src/services/render/videoFrameCoverage.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,10 +128,51 @@ describe("computeVideoFrameCoverage", () => {
// landed in framePaths (mid-extraction crash, partial cache read, …).
const partial = makeExtracted("a", 5);
partial.totalFrames = 30;
partial.metadata.durationSeconds = 1;

@james-russo-rames-d-jusso james-russo-rames-d-jusso Jul 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 [nit] Test-fixture coupling — makeExtracted defaults durationSeconds = delivered/fps. This is why line 131 is now load-bearing on the pre-existing test: without it, makeExtracted("a", 5) at fps=30 sets durationSeconds = 5/30 = 0.167, the new code computes sourceFrames = ceil(0.167 * 30) = 5, giving expectedFrames = min(30, 5) = 5 and ratio = 5/5 = 1 — the test's own ratio ≈ 5/30 expectation would then fail.

Not a defect in this PR — the workaround is correct — but future tests that call makeExtracted to model "extraction failure" (delivered < expected) need to also override durationSeconds, or they'll silently report full coverage and mask the failure. Consider making makeExtracted take durationSeconds as an explicit parameter, or defaulting it to something that doesn't couple with delivered (e.g. videos[i].end - videos[i].start, or a fixed 10s).

— Rames D Jusso

@miguel-heygen miguel-heygen Jul 17, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed at the root instead of leaving it as a footgun: makeExtracted now takes durationSeconds as an explicit param (default Infinity, decoupled from delivered) rather than deriving it from delivered / fps. That also let me drop the two post-hoc .metadata.durationSeconds = ... mutations that existed only to work around the coupling. Pushed in 7382ef7.

const reports = computeVideoFrameCoverage(videos, [partial], 30);
expect(reports[0]!.capturedFrames).toBe(5);
expect(reports[0]!.ratio).toBeCloseTo(5 / 30, 5);
});

it("credits a non-looping held tail against the source portion only", () => {
const videos = [makeVideo({ id: "held", start: 0, end: 10 })];
const extracted = [makeExtracted("held", 90)];
extracted[0]!.metadata.durationSeconds = 3;
const reports = computeVideoFrameCoverage(videos, extracted, 30);
expect(reports[0]).toMatchObject({ expectedFrames: 90, capturedFrames: 90, ratio: 1 });
});

@james-russo-rames-d-jusso james-russo-rames-d-jusso Jul 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 [nit] No test exercises mediaStart > 0. The formula on line 152 (durationSeconds - video.mediaStart) is only exercised at mediaStart = 0 in all three new tests (the default from makeVideo). A sibling test with, say, a 5s source, mediaStart=1, 10s non-looping slot would confirm sourceFrames = ceil((5 - 1) * 30) = 120 (not 150) — that path is otherwise dark.

— Rames D Jusso


it("still requires the full authored slot for looping clips", () => {
const videos = [makeVideo({ id: "loop", start: 0, end: 10, loop: true })];
const extracted = [makeExtracted("loop", 90)];
extracted[0]!.metadata.durationSeconds = 3;
const reports = computeVideoFrameCoverage(videos, extracted, 30);
expect(reports[0]).toMatchObject({ expectedFrames: 300, capturedFrames: 90 });
expect(reports[0]!.ratio).toBeCloseTo(0.3, 5);
});

it("still fails when extraction is truncated before the held-tail source", () => {
const videos = [makeVideo({ id: "truncated", start: 0, end: 10 })];
const extracted = [makeExtracted("truncated", 60)];
extracted[0]!.metadata.durationSeconds = 3;
const reports = computeVideoFrameCoverage(videos, extracted, 30);
expect(reports[0]).toMatchObject({ expectedFrames: 90, capturedFrames: 60 });
expect(() => assertVideoFrameCoverage(reports, 0.95)).toThrow(VideoFrameCoverageError);
});

it("fails closed for invalid or exhausted source durations", () => {
const videos = [
makeVideo({ id: "zero", start: 0, end: 10 }),
makeVideo({ id: "trimmed-away", start: 0, end: 10, mediaStart: 4 }),
];
const extracted = [makeExtracted("zero", 0), makeExtracted("trimmed-away", 30)];
extracted[0]!.metadata.durationSeconds = 0;
extracted[1]!.metadata.durationSeconds = 3;
const reports = computeVideoFrameCoverage(videos, extracted, 30);
expect(reports[0]).toMatchObject({ expectedFrames: 300, capturedFrames: 0 });
expect(reports[1]).toMatchObject({ expectedFrames: 300, capturedFrames: 30 });
expect(() => assertVideoFrameCoverage(reports, 0.95)).toThrow(VideoFrameCoverageError);
});
});

describe("assertVideoFrameCoverage", () => {
Expand Down
13 changes: 12 additions & 1 deletion packages/producer/src/services/render/videoFrameCoverage.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,18 @@ export function computeVideoFrameCoverage(
const reports: VideoFrameCoverageReport[] = [];
for (const video of videos) {
const entry = byId.get(video.id);
const expectedFrames = expectedFramesForClip(video.start, video.end, fps);
const slotFrames = expectedFramesForClip(video.start, video.end, fps);
// Non-looping clips intentionally hold their final decoded frame when the
// authored slot outlasts the source (#2516). Coverage must therefore
// measure the source portion, while still requiring the full slot for
// looping clips and for missing extractions (where no hold is possible).
const sourceDuration = entry ? entry.metadata.durationSeconds - video.mediaStart : NaN;
const hasUsableSourceDuration = Number.isFinite(sourceDuration) && sourceDuration > 0;
const sourceFrames =
entry && !video.loop && hasUsableSourceDuration
? expectedFramesForClip(0, sourceDuration, fps)
: slotFrames;
const expectedFrames = entry && !video.loop ? Math.min(slotFrames, sourceFrames) : slotFrames;
// framePaths is a Map — `size` is the number of distinct captured frames
// delivered to the runtime injector, which is the load-bearing count
// (some extractors report a total that includes cache-hit-skipped frames
Expand Down
Loading