Skip to content

feat(recording): decode contact-sheet frames from an exported clip - #2744

Merged
thymikee merged 1 commit into
refactor/capture-kit-shared-capture-primitivesfrom
feat/recording-frame-decoder
Sep 22, 2026
Merged

thymikee merged 1 commit into
refactor/capture-kit-shared-capture-primitivesfrom
feat/recording-frame-decoder

Conversation

@thymikee

@thymikee thymikee commented Sep 21, 2026 •

Copy link
Copy Markdown
Member

Why

A contact sheet needs real decoded frames, and the only decoder that can read the clips this repo produces is Apple AVFoundation. This layer answers one question: can we hand a caller the frames at N requested times?

What

  • Swift helper recording-frames.swift compiled through the existing cached Swift seam, plus extractRecordingFrames() on the TypeScript side.
  • Frames land in a scratch directory whose lifecycle is the caller's: it is also the directory the pipeline cleans up on success and failure.
  • Typed reasons live in packages/capture-kit/src/recording/contact-sheet-report.ts, beside the extraction that fails with them: contact_sheet_unsupported_host, contact_sheet_frame_extraction_failed, contact_sheet_no_frames. Nothing outside this package reads them yet, so they are not cross-layer contracts and get no packages/contracts subpath.
  • The manifest is treated as untrusted: malformed, oversized, unreadable, or silent about skipped samples each land on their own reason, and a silent shortfall still fails.

Notes

Frames are decoded once at the sheet's cell width and never re-encoded; the pipeline re-lays them out without asking AVFoundation again.

@thymikee
thymikee added this pull request to stack #2748 September 21, 2026 21:00
@thymikee thymikee changed the title feat/recording frame decoder feat(recording): decode contact-sheet frames from an exported clip Sep 21, 2026
@github-actions

github-actions Bot commented Sep 21, 2026 •

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
Installed (including dependencies) 4.74 MB 4.75 MB +5.7 kB
Package (unpacked) 4.74 MB 4.75 MB +5.7 kB
Package (download) 1.42 MB 1.42 MB +1.2 kB

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 19.9 ms 18.9 ms -1.0 ms
CLI --help 55.3 ms 54.6 ms -0.7 ms

@thymikee

Copy link
Copy Markdown
Member Author

Reviewed a3412ea. I have one scope question before this layer is ready.

Does this layer need more than the Swift helper, extractRecordingFrames, and the extraction and no-frames reasons? The new @agent-device/contracts/recording-contact-sheet subpath ships six reasons, a result type and readVideoContainerKind that nothing here reads yet, which is what the fallow unused-export and unused-type findings catch. Could the subpath wait for the layer that consumes it, or start with only these two reasons plus the unsupported-host reason? What would have to change first?

Not blocking: the error pass-through at contact-sheet-frames.ts#L107 (and L252) is wider than the cancel case and could narrow to isRequestCanceledError; the two catch-and-wrap blocks at L104 and L251 are the 51-line clone fallow flags, and one toExtractionFailure helper would cover both; an invalid actualTime seems to be written as 0 ms in recording-frames.swift#L168 instead of marking the sample skipped; and extractRecordingFrames never calls assertContactSheetHostSupport, so a non-macOS host gets contact_sheet_frame_extraction_failed instead of the unsupported-host reason.

CI: Lint & Format fails on contact-sheet-frames.test.ts, which this PR adds, and Compatibility & Provenance fails on the unused exports and the clone above, so both are related. The Coverage eager-closure failure comes from the #2743 base. Smoke fails on a WebView step that this decoder is not wired to, so it looks unrelated. No conflicts.

@thymikee
thymikee force-pushed the feat/recording-frame-decoder branch from a3412ea to 766267a Compare September 22, 2026 06:44
@thymikee
thymikee removed this pull request from stack #2748 September 22, 2026 06:45
@thymikee
thymikee added this pull request to stack #2753 September 22, 2026 06:45
@thymikee
thymikee force-pushed the feat/recording-frame-decoder branch from 766267a to ccf6687 Compare September 22, 2026 06:55
@thymikee

Copy link
Copy Markdown
Member Author

Narrowed — the subpath now carries exactly three names: contact_sheet_unsupported_host, contact_sheet_frame_extraction_failed, contact_sheet_no_frames. Nothing else here reads anything more, so nothing more is declared.

Nothing has to change first for the subpath to exist; it lands with its first consumer, and every other name now arrives in the layer that raises it: the duration reason with the sample grid (#2745), the pixel-budget reason with the renderer (#2746), container/write/collision plus the result and cell types with the pipeline (#2752), and contact_sheet_threshold_invalid with the command that parses the flag (#2747). readVideoContainerKind moved the same way — the rename and its export now sit in #2752, where the pipeline sniffs the container before it spawns a decoder.

Non-blocking items taken:

  • throwExtractionFailure now rethrows a canceled request via isRequestCanceledError, so cancellation is no longer riding on the "already has a reason" branch; the pass-through for typed failures stays because a compile failure that names its own reason should not be relabelled.
  • The two catch-and-wrap blocks are one helper, which is the 51-line clone fallow flagged; Compatibility & Provenance is clean locally against this layer's base.
  • recording-frames.swift no longer reports 0 ms for an unusable CMTime. It refuses to name a presentation time it does not have: the sample is recorded as skipped before anything is written, so a frame cannot arrive labelled as the opening one.
  • extractRecordingFrames answers the host question itself, before compiling or spawning, so a caller that only asks for frames gets contact_sheet_unsupported_host rather than a spawn failure. It takes an optional hostPlatform for the same reason the pipeline does, and the tests state their host — this suite is in the unit lane, which also runs on Linux.

@thymikee

Copy link
Copy Markdown
Member Author

Reviewed ccf6687. The scope question from the first review is answered: the contracts subpath now declares only the three reasons this layer raises, each has a reader here, and the unused-export and clone findings are gone. The cancellation rethrow, the single extraction-failure helper and the actualTime fix look right.

Not blocking: the cancellation test (contact-sheet-frames.test.ts#L284) aborts before extract() runs, so it does not cover a canceled error from a rejected runCmd or compileSwiftSourceFile; a case that rejects with createRequestCanceledError() would pin it.

CI: Smoke Tests was still running. This module has no production importer yet, so Smoke does not reach it. No conflicts.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Sep 22, 2026
@thymikee
thymikee force-pushed the feat/recording-frame-decoder branch from ccf6687 to 442d367 Compare September 22, 2026 07:47
@thymikee
thymikee force-pushed the feat/recording-frame-decoder branch from 442d367 to e88dd5b Compare September 22, 2026 10:23
@thymikee

Copy link
Copy Markdown
Member Author

Reviewed e88dd5b. Since 442d367, this only moves the three refusal reasons from the contracts subpath into capture-kit/src/recording/contact-sheet-report.ts and drops the contracts export and its snapshot line. Nothing still imports @agent-device/contracts/recording-contact-sheet, and the constants are unchanged apart from the file comment.

CI is still running, with no failures yet. No conflicts.

@thymikee
thymikee merged commit 0f825dd into main Sep 22, 2026
18 checks passed
@thymikee
thymikee deleted the feat/recording-frame-decoder branch September 22, 2026 11:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-human Valid work that needs human implementation, judgment, or maintainer merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant