diff --git a/.github/scripts/discord-thread-validator.mjs b/.github/scripts/discord-thread-validator.mjs index 88ae9c7ea6..2ab906833e 100644 --- a/.github/scripts/discord-thread-validator.mjs +++ b/.github/scripts/discord-thread-validator.mjs @@ -8,14 +8,13 @@ export async function validateThreadChannel(threadId, prNumber, { botToken, foru return false; } const VALIDATION_TIMEOUT_MS = 5_000; + const controller = new AbortController(); + const timeout = setTimeout(() => controller.abort(), VALIDATION_TIMEOUT_MS); try { - const controller = new AbortController(); - const timeout = setTimeout(() => controller.abort(), VALIDATION_TIMEOUT_MS); const res = await fetch(`https://discord.com/api/v10/channels/${threadId}`, { headers: { Authorization: `Bot ${botToken}` }, signal: controller.signal, }); - clearTimeout(timeout); if (!res.ok) { warning(`Thread validation failed: channel ${threadId} returned ${res.status}`); return false; @@ -38,5 +37,12 @@ export async function validateThreadChannel(threadId, prNumber, { botToken, foru } catch (err) { warning(`Thread validation threw: ${err && err.message ? err.message : err}`); return false; + } finally { + // `finally`, not a line after the await: when `fetch` rejects — a real network + // error — the timer would otherwise stay armed and fire `controller.abort()` + // five seconds later, long after this returned. Under Vitest that lands after + // the worker has torn down, which is an intermittent post-run error rather + // than a failing test. Same shape as `callDiscord` in discord-bot-api.mjs. + clearTimeout(timeout); } } diff --git a/.github/scripts/discord-thread-validator.test.mjs b/.github/scripts/discord-thread-validator.test.mjs index f2c686b852..7e2718d1e5 100644 --- a/.github/scripts/discord-thread-validator.test.mjs +++ b/.github/scripts/discord-thread-validator.test.mjs @@ -108,4 +108,22 @@ describe("validateThreadChannel", () => { const result = await validateThreadChannel("500", number, { botToken }); expect(result).toBe(false); }); + + it("leaves no timer armed once it has returned, on either path", async () => { + vi.useFakeTimers(); + try { + vi.mocked(fetch).mockRejectedValue(new Error("network error")); + await validateThreadChannel("500", number, { botToken }); + // The failing path is the one that used to leak: `clearTimeout` sat after + // the `await`, so a rejected fetch skipped it and left the 5s abort timer + // running past the end of the test. + expect(vi.getTimerCount()).toBe(0); + + vi.mocked(fetch).mockResolvedValue({ ok: false, status: 404 }); + await validateThreadChannel("404", number, { botToken }); + expect(vi.getTimerCount()).toBe(0); + } finally { + vi.useRealTimers(); + } + }); }); diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 1ea21c6c2f..3c3f82937e 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -140,6 +140,29 @@ jobs: APPLE_APP_SPECIFIC_PASSWORD: ${{ secrets.APPLE_APP_SPECIFIC_PASSWORD }} run: | if [[ -n "$MAC_CERTIFICATE_P12" && -n "$MAC_CERTIFICATE_PASSWORD" && -n "$MAC_CSC_NAME" && -n "$APPLE_ID" && -n "$APPLE_TEAM_ID" && -n "$APPLE_APP_SPECIFIC_PASSWORD" ]]; then + # `CSC_NAME` must name the identity WITHOUT its certificate type. + # electron-builder picks the type itself and rejects a qualified name + # outright: + # + # ⨯ Please remove prefix "Developer ID Application:" from the + # specified name — appropriate certificate will be chosen + # automatically + # + # It does that at `Package .app bundle`, which sits after the ffmpeg + # build and the compositor addon — about twelve minutes in, and only + # on macOS. Since the same secret also feeds `codesign --sign` at + # `Sign DMG`, the mistake is easy to make: codesign accepts the full + # common name, so the qualified form looks right until electron-builder + # sees it. The short form satisfies both, because codesign matches on a + # substring of the common name. + case "$MAC_CSC_NAME" in + # Every pattern ends at the colon on purpose, so a company whose + # name merely starts with one of these words is not rejected. + "Developer ID Application:"*|"Developer ID Installer:"*|"Apple Development:"*|"Apple Distribution:"*|"3rd Party Mac Developer Application:"*|"3rd Party Mac Developer Installer:"*) + echo "::error::MAC_CSC_NAME carries a certificate-type prefix. Set it to the identity name alone, e.g. 'Jane Doe (AB12CD34EF)' rather than 'Developer ID Application: Jane Doe (AB12CD34EF)'. Read it from: security find-identity -v -p codesigning" + exit 1 + ;; + esac echo "enabled=true" >> "$GITHUB_OUTPUT" else echo "enabled=false" >> "$GITHUB_OUTPUT" @@ -260,9 +283,52 @@ jobs: exit 1 fi + # electron-builder used to do this itself. Its macPackager carried a + # `noIdentity && fallBackToAdhoc` branch that handed back `Identity("-")` + # whenever no certificate was found — mandatory on arm64, where an unsigned + # binary will not launch at all. 26.15.3 replaced that path with + # `findSigningIdentity`, which returns null instead, and `sign()` leaves on + # `return false`. Nothing signs the bundle, and what ships is the bare + # linker signature on the Electron binary: `Identifier=Electron`, + # `Sealed Resources=none`. + # + # That is not cosmetic. macOS keys TCC grants to an app's code signature, + # so a bundle signed as "Electron" cannot hold one. v1.9.0-rc.1 asked for + # Accessibility, the user granted it, `AXIsProcessTrusted()` still returned + # false, and the editable-cursor preflight in useScreenRecorder re-opened + # the same dialog on every press of record — recording was impossible. + # + # Signed with the same runtime and entitlements electron-builder applies, + # so a locally signed build and a certificate-signed one differ only in the + # identity. Both arches on purpose: 26.8.1 only fell back on arm64, which + # left Intel DMGs unsigned for their whole existence. + - name: Ad-hoc sign the .app + if: steps.signing.outputs.enabled != 'true' + run: | + codesign --force --deep --sign - \ + --options runtime \ + --entitlements macos.entitlements \ + "${{ steps.find_app.outputs.app_bundle }}" + + # UNCONDITIONAL. Gated on `enabled == 'true'`, this step never ran for the + # RC builds — the only ones that could be unsigned — so the regression + # above shipped with every macOS check in this job green. - name: Verify .app code signature - if: steps.signing.outputs.enabled == 'true' - run: codesign --verify --deep --strict "${{ steps.find_app.outputs.app_bundle }}" + run: | + APP="${{ steps.find_app.outputs.app_bundle }}" + codesign --verify --deep --strict "$APP" + + # The identifier, not just the structure: `--verify` passes on the bare + # linker signature too, so it alone would not have caught this. What + # distinguishes a bundle macOS can attach permissions to is that its + # signing identifier matches the bundle id. + EXPECTED="$(/usr/libexec/PlistBuddy -c 'Print :CFBundleIdentifier' "$APP/Contents/Info.plist")" + ACTUAL="$(codesign -dv --verbose=2 "$APP" 2>&1 | sed -n 's/^Identifier=//p')" + echo "signature identifier=${ACTUAL} expected=${EXPECTED}" + if [[ "$ACTUAL" != "$EXPECTED" ]]; then + echo "::error::The .app is signed as '${ACTUAL}', not '${EXPECTED}' — macOS cannot attach Accessibility or Screen Recording permissions to a bundle whose signature does not carry its own identifier" + exit 1 + fi - name: Create DMG id: dmg @@ -301,8 +367,30 @@ jobs: rm -rf "$STAGING" echo "dmg_path=$DMG_OUTPUT" >> "$GITHUB_OUTPUT" + # The four steps below used to carry `&& !contains(github.ref_name, '-')`, + # which skipped them for every pre-release, `-rc.N` tags included. Two + # costs, and the second is the one that mattered. + # + # Testers paid the first: a DMG signed with Developer ID but not notarized + # is still refused by Gatekeeper — `spctl` answers `rejected, source= + # Unnotarized Developer ID` — so every RC tester had to know about + # `xattr -rd com.apple.quarantine` before they could open the thing they + # were being asked to test. + # + # The release paid the second. With the skip in place, notarization never + # ran until the stable tag, so the first exercise of the credentials, the + # certificate chain and Apple's acceptance of every nested Mach-O landed on + # the highest-stakes build there is. That is not theoretical: the run that + # first enabled signing here died in `Package .app bundle` on a malformed + # `MAC_CSC_NAME`, and it was only visible because a full build was run + # deliberately. Notarizing each RC turns every candidate into a rehearsal. + # + # The trade is a few minutes per macOS job and a dependency on Apple's + # notary service being reachable — `--wait` is capped at 15 minutes below. + # If that ever becomes flaky enough to block RCs, the fix is + # `continue-on-error` on pre-releases, not going back to skipping them. - name: Sign DMG - if: steps.signing.outputs.enabled == 'true' && !contains(github.ref_name, '-') + if: steps.signing.outputs.enabled == 'true' run: | codesign --force \ --sign "${{ secrets.MAC_CSC_NAME }}" \ @@ -310,7 +398,7 @@ jobs: "${{ steps.dmg.outputs.dmg_path }}" - name: Notarize DMG - if: steps.signing.outputs.enabled == 'true' && !contains(github.ref_name, '-') + if: steps.signing.outputs.enabled == 'true' run: | xcrun notarytool submit "${{ steps.dmg.outputs.dmg_path }}" \ --apple-id "${{ secrets.APPLE_ID }}" \ @@ -320,11 +408,11 @@ jobs: timeout-minutes: 15 - name: Staple notarization ticket - if: steps.signing.outputs.enabled == 'true' && !contains(github.ref_name, '-') + if: steps.signing.outputs.enabled == 'true' run: xcrun stapler staple "${{ steps.dmg.outputs.dmg_path }}" - name: Validate stapled DMG - if: steps.signing.outputs.enabled == 'true' && !contains(github.ref_name, '-') + if: steps.signing.outputs.enabled == 'true' run: | xcrun stapler validate "${{ steps.dmg.outputs.dmg_path }}" spctl -a -vv -t install "${{ steps.dmg.outputs.dmg_path }}" @@ -386,6 +474,10 @@ jobs: steps: - name: Checkout code uses: actions/checkout@v4 + with: + # Full history + tags: the RC notes below are built from `git log` over the + # range since the previous RC tag, and resolving that tag needs the tags. + fetch-depth: 0 - name: Resolve release tag id: release @@ -433,6 +525,21 @@ jobs: else NOTES_START_TAG="v$((PX - 1)).0.0" fi + # For an RC, compare against the PREVIOUS RC of the same line, not the previous + # stable. Deriving the start tag from STABLE_VERSION alone made every RC of a + # line span the same range, so each re-cut just repeated the last RC's notes + # plus its own handful, and testers could not see what the re-cut changed. + # Walk down from the current rc number so a skipped or failed RC doesn't break it. + if [[ "$IS_PRERELEASE" == "true" ]]; then + RC_NUMBER="${VERSION##*.}" + for (( n = RC_NUMBER - 1; n >= 1; n-- )); do + CANDIDATE="v${STABLE_VERSION}-rc.${n}" + if git rev-parse -q --verify "refs/tags/${CANDIDATE}" >/dev/null; then + NOTES_START_TAG="$CANDIDATE" + break + fi + done + fi echo "Computed notes_start_tag=${NOTES_START_TAG} for tag=${TAG}" echo "tag=$TAG" >> "$GITHUB_OUTPUT" @@ -482,17 +589,38 @@ jobs: if gh release view "$TAG" >/dev/null 2>&1; then gh release upload "$TAG" "${FILES[@]}" --clobber else - # --notes-start-tag controls which previous tag GitHub compares against - # when auto-generating the release notes. Default behaviour (most recent - # prior release by date) doesn't work for this fork because the v1.4.0 - # release in the fork was re-published after v1.5.0, which makes GitHub - # pick v1.4.0 as the "previous" for any v1.5.x release. + if [[ -n "$PRERELEASE_FLAG" ]]; then + # RC notes come from `git log`, not --generate-notes. GitHub's generator + # lists only the PRs it manages to associate, and on this repo it silently + # drops real ones — #254 and #261 were merged into the release branch and + # never appeared in v1.9.0-rc.2's body — so an RC could omit the very fix + # the re-cut was for. The commit range is the actual diff and can't lie. + # Stable releases keep --generate-notes below: they're the public-facing + # ones and want the PR links and the New Contributors section. + { + echo "## Changes since ${NOTES_START_TAG}" + echo + git log --no-merges --reverse --pretty='- %s' \ + --invert-grep --grep='^chore(release): bump to' \ + "${NOTES_START_TAG}..${TAG}" + echo + echo "**Full Changelog**: ${GITHUB_SERVER_URL}/${GITHUB_REPOSITORY}/compare/${NOTES_START_TAG}...${TAG}" + } > "${RUNNER_TEMP}/rc-notes.md" + cat "${RUNNER_TEMP}/rc-notes.md" + NOTES_ARGS=(--notes-file "${RUNNER_TEMP}/rc-notes.md") + else + # --notes-start-tag controls which previous tag GitHub compares against + # when auto-generating the release notes. Default behaviour (most recent + # prior release by date) doesn't work for this fork because the v1.4.0 + # release in the fork was re-published after v1.5.0, which makes GitHub + # pick v1.4.0 as the "previous" for any v1.5.x release. + NOTES_ARGS=(--generate-notes --notes-start-tag "$NOTES_START_TAG") + fi # shellcheck disable=SC2086 gh release create "$TAG" "${FILES[@]}" \ --target "$GITHUB_SHA" \ --title "$TAG" \ - --generate-notes \ - --notes-start-tag "$NOTES_START_TAG" \ + "${NOTES_ARGS[@]}" \ $PRERELEASE_FLAG fi diff --git a/.github/workflows/diagnostic-artifact.yml b/.github/workflows/diagnostic-artifact.yml index bdf9bde9d5..4c408a68aa 100644 --- a/.github/workflows/diagnostic-artifact.yml +++ b/.github/workflows/diagnostic-artifact.yml @@ -2,9 +2,12 @@ name: Diagnostic artifact on: push: - branches: [main] + branches: [main, "release/**"] + # Release branches too: a recording fix targeting a release is exactly when a + # reviewer needs the compiled helper, and filtering on main alone meant + # retargeting a PR silently removed the artifact its own test steps ask for. pull_request: - branches: [main] + branches: [main, "release/**"] workflow_dispatch: permissions: diff --git a/.harness/docs/git-workflow.md b/.harness/docs/git-workflow.md index 68f2486452..99fc1ff0d9 100644 --- a/.harness/docs/git-workflow.md +++ b/.harness/docs/git-workflow.md @@ -21,20 +21,22 @@ Conventions for the Mavis reins when working in this repo. ## CI (`.github/workflows/ci.yml`) -CI runs on every PR to `main` and every push to `main`: +CI runs on every PR to `main`, `feat/ai-edition` and `release/**`, and on every push to those: - `npm run lint` (Biome) -- `npx tsc --noEmit` (TypeScript) +- `npx tsc --noEmit` (TypeScript, app code) +- `npx tsc -p tsconfig.test.json --noEmit` (TypeScript, test files — a separate gate at zero) - `npm run test` (Vitest unit) -- `npm run test:browser` (Vitest + Playwright headless) +- `npm run docs:check` - `npx vite build` (renderer build smoke) +- `cargo test` / `cargo check` for the compositor on macOS, Windows and Linux -All five must be green before merge. Native helper code is NOT covered by CI — manual smoke test is required for `electron/*-helper/` changes; note it in the PR description. +All must be green before merge. Native helper code is NOT covered by CI — manual smoke test is required for `electron/*-helper/` changes; note it in the PR description. ## Pull request flow 1. Branch from `main`. 2. Implement + add tests in the same package. -3. Run locally: `npm run lint && npx tsc --noEmit && npm run test`. For browser/e2e-touching changes, also run the relevant suite. +3. While implementing, run only the affected tests (`npx vitest --run ` or `--changed`); `npx tsc --noEmit` and `npm run lint` are the cheap inner-loop checks. Run the full `npm run test` **once**, here, before pushing. 4. Push and open the PR via `gh pr create`. Use `.github/pull_request_template.md`. 5. Wait for the Mavis reviewer (`openscreen-reviewer`) PASS or address the requested changes. 6. Merge once CI is green and review is PASS. PR titles must follow Conventional Commits (enforced by the `semantic-pr` job in `ci.yml`) — this keeps the auto-generated release notes clean. @@ -56,7 +58,7 @@ The workflow: 1. Computes the next SemVer from `package.json` + `bump`, builds `vX.Y.Z-rc.N`. 2. Migrates every issue/PR in the rolling `Next Release` milestone into a fresh `vX.Y.Z` milestone. Each migrated item gets a hidden marker comment so re-running is idempotent. 3. Commits `package.json` → `X.Y.Z-rc.N` on a fresh branch `release/vX.Y.Z-rc.N`. **The branch is NOT merged into `main`** — it stays frozen so the RC build only contains what was on `main` at the moment of cut. -4. Pushes the tag `vX.Y.Z-rc.N` at the release branch tip. This triggers `build.yml`, which publishes a **GitHub pre-release** (badged as such, does not become "Latest"). macOS notarization is skipped on RC tags. +4. Pushes the tag `vX.Y.Z-rc.N` at the release branch tip. This triggers `build.yml`, which publishes a **GitHub pre-release** (badged as such, does not become "Latest"). RC tags are signed and notarized like stable ones, so testers do not have to clear the quarantine attribute by hand. 5. Posts in `#rc-testing` on Discord with the download link. Tier 3 (homebrew/winget/nix/aur) does **not** run on pre-releases — they're already gated on `!prerelease`. diff --git a/.harness/reins/openscreen-dev/agent.md b/.harness/reins/openscreen-dev/agent.md index 4bd9ce150e..a44c491a9d 100644 --- a/.harness/reins/openscreen-dev/agent.md +++ b/.harness/reins/openscreen-dev/agent.md @@ -26,6 +26,8 @@ You are the generalist implementer for the OpenScreen project — a free, open-s - `npx tsc --noEmit` passes. - `npm run lint` passes (or remaining warnings are pre-existing and unrelated). -- `npm run test` passes for any unit tests you added or affected. +- The tests you added or affected pass — run those files, `npx vitest --run `, not the + whole suite. `npm run test` is minutes; run it once at the end if at all, and let CI be the + full-suite gate. Never `npm run test:watch` (it never terminates). - The change is documented in the PR description (what + why + how to test). - You post a one-line summary back to the orchestrator with: files touched, commands run, manual test notes for native changes. diff --git a/.harness/reins/openscreen-tester/agent.md b/.harness/reins/openscreen-tester/agent.md index bd4658ce4a..aadbf7bbb7 100644 --- a/.harness/reins/openscreen-tester/agent.md +++ b/.harness/reins/openscreen-tester/agent.md @@ -9,7 +9,7 @@ You are the test specialist for the OpenScreen project — a free, open-source s ## Scope -- **Own**: Vitest unit tests (`*.test.ts` / `*.test.tsx`, jsdom), Vitest browser tests (`vitest.browser.config.ts`, Playwright headless), Playwright e2e (`tests/e2e/`). +- **Own**: Vitest unit tests (`*.test.ts` / `*.test.tsx`), Playwright e2e (`tests/e2e/`). - **Don't own**: writing production code (hand off to `openscreen-dev`). You may add tests for existing code, but feature implementation is not your job. Final PR quality gate is `openscreen-reviewer`. ## How you work @@ -17,16 +17,23 @@ You are the test specialist for the OpenScreen project — a free, open-source s - Read `AGENTS.md` at the repo root for commands and conventions. - Read `technical-documentation/testing/writing-tests.md` for the project's test style guide. - Match the style of neighboring `*.test.` files in the same package — don't invent new patterns. -- Unit tests: `npm run test` (Vitest, jsdom). Browser tests: `npm run test:browser` (needs `npm run test:browser:install` once). E2E: `npm run test:e2e` (Playwright). +- Iterate with `npx vitest --run ` on the files you are writing. `npm run test` is the + whole suite (minutes) — run it once at the end, not between edits. E2E: `npm run test:e2e`. +- The Vitest environment is `node` by default. A test that needs a DOM opts in with + `// @vitest-environment jsdom` on line 1 — add it only when the test actually renders. - E2E specs in `tests/e2e/windows-native-checklist.spec.ts` are Windows-only — gate with `test.skip` for other platforms rather than deleting. - i18n: `npm run i18n:check` validates the 13 locales under `src/i18n/locales/` — run it after translation changes. -- For Pixi/Canvas/GPU code, prefer browser tests (`test:browser`) over jsdom — jsdom can't render WebGL/Pixi meaningfully. +- jsdom can't render WebGL/Pixi meaningfully and there is no browser-test tier anymore. Real + codec/GPU behavior belongs to the Rust suites in `crates/` or to the manual checklist — + don't write a jsdom test that pretends to cover it. +- Anything gated on `process.platform` must pin the platform in the test. CI is Linux-only, + so an unpinned Linux-only path is green in CI and red on every Windows and macOS machine. - Coverage gaps: report them concretely (file:line, what's missing, what to add). Don't write the test for someone else's feature unprompted — flag it. ## Stop when -- `npm run test` passes. -- For browser-tested changes: `npm run test:browser` passes. +- The files you touched pass under `npx vitest --run `, and one final `npm run test` is + green (that single full run is the gate — not one per edit). - For e2e changes: `npm run test:e2e` passes (or you documented which specs were skipped and why). - `npm run i18n:check` passes if any locale file was touched. - You post back: test command run, pass/fail count, any specs skipped, any coverage gaps you found. diff --git a/AGENTS.md b/AGENTS.md index bfa79b4984..2d8e2d0491 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -8,8 +8,7 @@ OpenScreen is a free, open-source screen recorder and video editor (Electron + R - Start dev: `npm run dev` (Vite dev server; Electron window opens via `vite-plugin-electron`) - Build: `npm run build` (TypeScript check + Vite build + electron-builder) - Typecheck: `npx tsc --noEmit` — app code only. CI also runs `npx tsc -p tsconfig.test.json --noEmit` in a separate job ("Typecheck (tests)"), so **run both**: test files are invisible to the root config, and a type error in a `*.test.ts` fails CI while the root check stays green. -- Test (unit): `npm run test` (Vitest, jsdom env) -- Test (browser): `npm run test:browser` (Vitest + Playwright, requires `npm run test:browser:install` first) +- Test (unit): `npx vitest --run ` while you work, `npm run test` once at the end — see [Testing instructions](#testing-instructions) - Test (e2e): `npm run test:e2e` (Playwright) - Lint: `npm run lint` (Biome 2.4) - Format: `npm run format` (Biome, tabs, double quotes, 100-col) @@ -37,11 +36,37 @@ OpenScreen is a free, open-source screen recorder and video editor (Electron + R ## Testing instructions -- Unit tests live next to source as `*.test.ts` / `*.test.tsx` (Vitest, jsdom). -- Browser tests use `vitest.browser.config.ts` (Playwright headless) — only run when DOM/Pixi rendering matters. +### When to run what + +The full unit suite is ~1670 tests over 140 files and takes over a minute. Running it after +every edit is the main way an agent turns a 5-minute task into a 30-minute one, so don't: + +- **While you work** — run only what you touched: `npx vitest --run src/lib/foo.test.ts`, + or `npx vitest --run src/lib/ai-edition` for a directory. `npm run test:changed` picks + the affected files off the working tree, `npx vitest --run --changed main` off the + branch diff. A single file is 1–10s against ~80s for everything. +- **Typecheck and lint freely** — `npx tsc --noEmit` and `npm run lint` are seconds, not + minutes. They are the right inner-loop check, not the test suite. +- **Once, at the end** — `npm run test` before you commit or open the PR. One full run per + task, not per edit. If the change is narrow and CI will run anyway, the targeted run plus + CI is enough; say so rather than burning the wall-clock twice. +- **Never** `npm run test:watch` — it does not terminate, and it will hang the session. + +### Layout and conventions + +- Unit tests live next to source as `*.test.ts` / `*.test.tsx` (Vitest). Config is + `vitest.config.ts`; it covers `src/`, `electron/` and `.github/`. +- **The default environment is `node`.** A test that needs a DOM opts in with + `// @vitest-environment jsdom` on line 1 — that is also the fix for `document is not + defined`. Don't add it to a test that doesn't need it: jsdom setup dominates this + suite's runtime (see the comment in `vitest.config.ts`). +- Anything platform-conditional (`process.platform`) must pin the platform in the test. + CI is Linux-only, so a Linux-only code path left unpinned is green in CI and red on + every Windows and macOS machine — `electron/recording/webm-seek-index.test.ts` is the + worked example. - E2E tests are in `tests/e2e/` (Playwright). Some specs are platform-specific (e.g. `windows-native-checklist.spec.ts`). - Add a test for every new behavior in the same package as the code under test. -- All tests must pass before opening a PR. CI runs `npm run test` and `npm run test:browser` on every PR. +- All tests must pass before opening a PR. CI runs `npm run test` on every PR. ## Desktop E2E testing with computer-use @@ -95,7 +120,7 @@ Unit/browser tests can't exercise real capture (native screen recording, a physi Two `workflow_dispatch` workflows cut a release with a pre-release candidate (RC) first, then promote to stable. Trunk-based, no extra branch. Full operational guide in `.harness/docs/git-workflow.md` § Release flow. -- **Cut RC**: Actions → "Cut a release candidate" → Run workflow. Inputs: `bump` (patch|minor|major), `rc_number` (default 1), optional `target_version` override. Snaps issues out of the rolling `Next Release` milestone into a versioned `vX.Y.Z` milestone, bumps `package.json`, pushes the `vX.Y.Z-rc.N` tag, which triggers the existing `build.yml` to publish a GitHub pre-release. Notarization is skipped on RCs. Notifies `#rc-testing` on Discord. +- **Cut RC**: Actions → "Cut a release candidate" → Run workflow. Inputs: `bump` (patch|minor|major), `rc_number` (default 1), optional `target_version` override. Snaps issues out of the rolling `Next Release` milestone into a versioned `vX.Y.Z` milestone, bumps `package.json`, pushes the `vX.Y.Z-rc.N` tag, which triggers the existing `build.yml` to publish a GitHub pre-release. RCs are notarized like stable releases, which also rehearses the credentials before the promotion build depends on them. Notifies `#rc-testing` on Discord. - **Promote RC**: Actions → "Promote RC to stable release" → Run workflow. Input: `rc_tag` (e.g. `v1.5.0-rc.2`), optional `release_notes_extra`. Closes the `vX.Y.Z` milestone, strips `-rc.N` from `package.json`, pushes `vX.Y.Z` tag, which triggers `build.yml` to publish a stable release (full notarization, Tier 3 homebrew/winget/nix/aur fires). Notifies `#announcements` on Discord. - **Manual fallback**: `git tag vX.Y.Z-rc.N && git push origin vX.Y.Z-rc.N` does the same as Cut RC (minus the milestone migration and Discord announce) — useful for emergency cuts. diff --git a/crates/compositor/src/compositor_linux.rs b/crates/compositor/src/compositor_linux.rs index adefba877f..fc32ecaff8 100644 --- a/crates/compositor/src/compositor_linux.rs +++ b/crates/compositor/src/compositor_linux.rs @@ -726,6 +726,12 @@ impl Compositor { *self.live_params.borrow_mut() = p; } + /// Cf. `compositor_windows::set_has_webcam` — le seul champ de `LiveParams` qui dépend du + /// clip courant, rebranché par `walk_composited_timeline` sans écraser le reste. + pub fn set_has_webcam(&self, v: bool) { + self.live_params.borrow_mut().has_webcam = v; + } + pub fn set_scene(&self, s: Option) { *self.scene.borrow_mut() = s; } diff --git a/crates/compositor/src/compositor_macos.rs b/crates/compositor/src/compositor_macos.rs index cd553fb94d..4ea09a42c2 100644 --- a/crates/compositor/src/compositor_macos.rs +++ b/crates/compositor/src/compositor_macos.rs @@ -577,6 +577,12 @@ impl Compositor { *self.live_params.borrow_mut() = p; } + /// Cf. `compositor_windows::set_has_webcam` — le seul champ de `LiveParams` qui dépend du + /// clip courant, rebranché par `walk_composited_timeline` sans écraser le reste. + pub fn set_has_webcam(&self, v: bool) { + self.live_params.borrow_mut().has_webcam = v; + } + pub fn set_scene(&self, s: Option) { *self.scene.borrow_mut() = s; } diff --git a/crates/compositor/src/compositor_windows.rs b/crates/compositor/src/compositor_windows.rs index 09381e4543..b4f19b9bed 100644 --- a/crates/compositor/src/compositor_windows.rs +++ b/crates/compositor/src/compositor_windows.rs @@ -596,6 +596,14 @@ impl Compositor { *self.live_params.borrow_mut() = p; } + /// Rebranche le seul champ qui dépend du CLIP et non des réglages (cf. `LiveParams::has_webcam`). + /// L'export pose ses `LiveParams` une fois pour toute la timeline, mais chaque clip a sa propre + /// réponse à « y a-t-il une caméra ? » : d'où un setter ciblé plutôt qu'un `set_live_params` + /// par clip, qui écraserait les réglages posés par l'appelant. + pub fn set_has_webcam(&self, v: bool) { + self.live_params.borrow_mut().has_webcam = v; + } + /// Installe (ou retire) la scène de l'app. Présente → `compose_frame` prend ses placements /// depuis le layout preset au lieu du planning fixture. pub fn set_scene(&self, s: Option) { diff --git a/crates/compositor/src/frame_geometry.rs b/crates/compositor/src/frame_geometry.rs index 0addb485dc..738a14f8e9 100644 --- a/crates/compositor/src/frame_geometry.rs +++ b/crates/compositor/src/frame_geometry.rs @@ -601,12 +601,37 @@ pub struct LiveParams { /// False when the "webcam" decoder is actually just the screen video again (the TS side /// falls `webcamPath` back to the screen asset's own path when a clip has no real camera, /// purely so the decoder pipeline has something valid to open) — drawing the PiP box in - /// that case duplicates the screen video into its own corner. Live-only: derived in - /// `live.rs` by comparing the active clip's screen/webcam paths; defaults `true` (draw) - /// so fixture/bench renders and any caller that never sets it keep their old behavior. + /// that case duplicates the screen video into its own corner. Derived per clip from the + /// screen/webcam paths via `webcam_is_real`: in `live.rs` for the preview, in + /// `timeline_walk.rs` for every export. Defaults `true` (draw) so fixture/bench renders + /// and any caller that never sets it keep their old behavior. pub has_webcam: bool, } +fn same_source_path(a: &str, b: &str) -> bool { + a.eq_ignore_ascii_case(b) +} + +/// True when this clip really has a camera to draw. +/// +/// TWO ways the app says "no camera", and both must be caught here, because the +/// webcam decoder is opened either way — the live path falls back to the SCREEN +/// file when the webcam path won't open, and `ExportDialog` sends the screen path +/// outright, so the decoder always yields frames. Whether those frames are the +/// camera or a second copy of the screen is decided HERE and nowhere else. +/// +/// - the empty string, which is what `sceneDescription.ts` and +/// `NativeCompositorOverlay` send for an asset with no `cameraTrack`; +/// - the screen's own path, which `ExportDialog.tsx` sends and which older +/// scenes still use. +/// +/// Missing the empty-string case is what put the screen recording inside the PiP +/// box: `"" != "/…/recording.mp4"`, so the box was drawn, and the decoder behind +/// it was the screen fallback. +pub fn webcam_is_real(webcam_path: &str, screen_path: &str) -> bool { + !webcam_path.trim().is_empty() && !same_source_path(webcam_path, screen_path) +} + impl Default for LiveParams { fn default() -> Self { Self { diff --git a/crates/compositor/src/live.rs b/crates/compositor/src/live.rs index 9c1c8f382d..015d01d782 100644 --- a/crates/compositor/src/live.rs +++ b/crates/compositor/src/live.rs @@ -28,6 +28,7 @@ use crate::scene::Scene; use crate::config::{self, Cfg}; use crate::cursor::CursorTrack; use crate::d3d::Gpu; +use crate::frame_geometry::webcam_is_real; use crate::pipeline::Decoder; use crate::timeline_walk::{frame_step, FrameStep, NextFrameTime}; use anyhow::Result; @@ -590,26 +591,6 @@ fn same_source_path(a: &str, b: &str) -> bool { a.eq_ignore_ascii_case(b) } -/// True when the active clip really has a camera to draw. -/// -/// TWO ways the app says "no camera", and both must be caught here, because the -/// webcam decoder is opened either way — `open_and_seek_clip` falls back to the -/// SCREEN file when the webcam path won't open, so `wdec` always yields frames. -/// Whether those frames are the camera or a second copy of the screen is decided -/// HERE and nowhere else. -/// -/// - the empty string, which is what `sceneDescription.ts` and -/// `NativeCompositorOverlay` send for an asset with no `cameraTrack`; -/// - the screen's own path, the older convention kept working for scenes that -/// still use it. -/// -/// Missing the empty-string case is what put the screen recording inside the PiP -/// box: `"" != "/…/recording.mp4"`, so the box was drawn, and the decoder behind -/// it was the screen fallback. -fn webcam_is_real(webcam_path: &str, screen_path: &str) -> bool { - !webcam_path.trim().is_empty() && !same_source_path(webcam_path, screen_path) -} - fn scene_clip_matches( clip: &crate::scene::SceneClip, screen_path: &str, diff --git a/crates/compositor/src/timeline_walk.rs b/crates/compositor/src/timeline_walk.rs index 0e0efdad6a..adde1d52b3 100644 --- a/crates/compositor/src/timeline_walk.rs +++ b/crates/compositor/src/timeline_walk.rs @@ -17,6 +17,7 @@ use crate::compositor::Compositor; use crate::config::Cfg; use crate::cursor::CursorTrack; use crate::d3d::Gpu; +use crate::frame_geometry::webcam_is_real; use crate::pipeline::{ClipSource, Decoder}; use crate::regions::{speed_segments_for_window, SpeedSegment}; use crate::scene::Scene; @@ -164,6 +165,13 @@ pub(crate) unsafe fn walk_composited_timeline( let mut frames: u64 = 0; for (clip_index, clip) in clips.iter().enumerate() { + // Le preset de layout est GLOBAL (un seul panneau pour toute la timeline) mais la + // caméra est PAR CLIP : un projet mélange sans problème un enregistrement avec webcam + // et un import qui n'en a pas. Le preset ne doit donc s'appliquer qu'aux clips qui ont + // vraiment une caméra — sinon la boîte PiP est dessinée avec, derrière, le décodeur de + // repli, c'est-à-dire l'écran lui-même recopié dans son propre coin (issue #248). + // La preview vive fait exactement ça dans `live.rs` ; c'est ici l'équivalent export. + comp.set_has_webcam(webcam_is_real(&clip.webcam, &clip.screen)); if !screen_decs.contains_key(&clip.screen) { screen_decs.insert(clip.screen.clone(), Decoder::open(&clip.screen, gpu)?); } diff --git a/crates/poc-d3d/src/bench.rs b/crates/poc-d3d/src/bench.rs index cb45254bd1..aa2de7d602 100644 --- a/crates/poc-d3d/src/bench.rs +++ b/crates/poc-d3d/src/bench.rs @@ -48,6 +48,7 @@ pub fn run() -> Result<()> { // poc-d3d.exe --cfg C0..C8 --fixture --repeat 3 --out out/ // --cfg GIF → bench natif GIF (slice 1) +// --webcam → force le chemin caméra (défaut `/webcam.mp4`) fn run_bench(args: &[String]) -> Result<()> { let get = |k: &str, d: &str| -> String { arg(args, k, d) }; let fixture = get("--fixture", "fixture"); @@ -56,7 +57,11 @@ fn run_bench(args: &[String]) -> Result<()> { let cfg_arg = get("--cfg", "C0..C8"); let screen = format!("{fixture}/screen.mp4"); - let webcam = format!("{fixture}/webcam.mp4"); + // Override explicite parce que le cas « pas de caméra » n'est PAS un fichier + // différent : l'app renvoie le chemin de l'écran lui-même (`ExportDialog`) ou la + // chaîne vide (`sceneDescription`). Le reproduire demande donc de piloter le chemin, + // pas le contenu — `--webcam ` rejoue exactement l'issue #248. + let webcam = get("--webcam", &format!("{fixture}/webcam.mp4")); std::fs::create_dir_all(&out).ok(); // sélection des cfg diff --git a/electron-builder.json5 b/electron-builder.json5 index 0a75205453..13c1cb2c58 100644 --- a/electron-builder.json5 +++ b/electron-builder.json5 @@ -210,7 +210,26 @@ "displayName": "OpenScreen", "backgroundColor": "transparent", "capabilities": ["runFullTrust", "microphone", "webcam"], - "languages": ["en-US", "fr-FR"], + // Mirrors SUPPORTED_LOCALES in src/i18n/config.ts. This list is what the Store + // shows as "supported languages" on the product page and what lets the listing + // surface in each language's Store search — declaring only en-US/fr-FR advertised + // 2 of the 13 languages the app actually ships. Bare tags (ar, es, ...) match every + // region of that language, which is what the renderer's locale resolution does too. + "languages": [ + "en-US", + "fr-FR", + "ar", + "es", + "it", + "ja-JP", + "ko-KR", + "pt-BR", + "ru", + "tr", + "vi", + "zh-CN", + "zh-TW" + ], "showNameOnTiles": true } } diff --git a/electron/ai-edition/agent-tools.test.ts b/electron/ai-edition/agent-tools.test.ts index 08b98b13e4..4fe706b87c 100644 --- a/electron/ai-edition/agent-tools.test.ts +++ b/electron/ai-edition/agent-tools.test.ts @@ -158,7 +158,9 @@ describe("the mutating-tool table", () => { "addCameraFullscreen", "addSpeed", "addTrim", + "addTrims", "addZoom", + "addZooms", "moveClip", "removeClip", "removeModifier", @@ -205,6 +207,36 @@ describe("executeAgentTool", () => { expect(payload.segments[1].kind).toBe("silence"); }); + it("getTranscript returns a long transcript whole, word for word", () => { + // The regression test for a `.slice(0, 800)` that used to sit here. On the + // production path a segment is one WORD, so the cap cut a half-hour + // recording at roughly its fifth minute and reported nothing — the model + // trimmed the silences it could see and called the job done. 4000 words is + // about half an hour of speech. + const base = fixtureDocument(); + const segments = Array.from({ length: 4000 }, (_, i) => ({ + id: `seg_${i}`, + kind: "speech" as const, + startSec: i * 0.45, + endSec: i * 0.45 + 0.4, + text: `mot${i}`, + wordIds: [], + })); + const doc = { + ...base, + transcript: null, + transcripts: [{ ...base.transcripts[0], segments }], + }; + + const result = executeAgentTool(doc, "getTranscript", "{}"); + expect(result.ok).toBe(true); + const payload = JSON.parse(result.resultJson); + expect(payload.segments).toHaveLength(4000); + // The last word matters more than the count: a cap keeps the head and + // drops the tail, so the tail is what proves it is gone. + expect(payload.segments.at(-1).text).toBe("mot3999"); + }); + it("getTranscript fails cleanly when no transcript exists", () => { const doc = { ...fixtureDocument(), transcripts: [], transcript: null }; const result = executeAgentTool(doc, "getTranscript", "{}"); @@ -227,6 +259,189 @@ describe("executeAgentTool", () => { expect(result.summary).toMatch(/added trim 0:20\.0 – 0:22\.0/); }); + it("addTrims lands exactly what the same calls one at a time would", () => { + // The property the batch tools exist to have: they save round trips and + // change nothing else. If this ever diverges, the batch has grown a second + // implementation of the rules and the two will drift. + const ranges = [ + { startSec: 1, endSec: 2, reason: "silence" }, + { startSec: 40, endSec: 41, reason: "silence" }, + { startSec: 5, endSec: 4, reason: "silence" }, // reversed on purpose + ]; + + let oneAtATime = fixtureDocument(); + for (const range of ranges) { + const step = executeAgentTool(oneAtATime, "addTrim", JSON.stringify(range)); + expect(step.ok).toBe(true); + oneAtATime = step.document as AxcutDocument; + } + + const batch = executeAgentTool(fixtureDocument(), "addTrims", JSON.stringify({ ranges })); + expect(batch.ok).toBe(true); + + const shape = (doc: AxcutDocument) => + doc.timeline.trimRanges.map((t) => ({ + startSec: t.startSec, + endSec: t.endSec, + reason: t.reason, + origin: t.origin, + clipId: t.clipId, + })); + expect(shape(batch.document as AxcutDocument)).toEqual(shape(oneAtATime)); + }); + + it("addTrims applies the good ranges and refuses the bad one by itself", () => { + // `replaceTimeline`, the repo's other array-taking tool, refuses in one + // block. That is right for rebuilding a timeline and ruinous here: one bad + // bound must not cost the other nine, and the model must be able to see + // WHICH one without re-reading the document. + const result = executeAgentTool( + fixtureDocument(), + "addTrims", + JSON.stringify({ + ranges: [ + { startSec: 1, endSec: 2 }, + { startSec: 25, endSec: 35 }, // spans both clips of asset_1 — ambiguous + { startSec: 40, endSec: 41 }, + ], + }), + ); + + expect(result.ok).toBe(true); + const payload = JSON.parse(result.resultJson); + expect(payload.requested).toBe(3); + expect(payload.appliedCount).toBe(2); + expect(payload.refusedCount).toBe(1); + expect(payload.refused).toHaveLength(1); + expect(payload.refused[0].index).toBe(1); + // The refusal keeps the unitary wording, which names the clips and the fix. + expect(payload.refused[0].error).toMatch(/clipId/); + expect(payload.applied.map((a: { index: number }) => a.index)).toEqual([0, 2]); + // The fixture starts with one trim; two more landed. + expect(result.document?.timeline.trimRanges).toHaveLength(3); + expect(result.summary).toMatch(/added 2 trims, 1 refused/); + }); + + it("addTrims refuses a MALFORMED range by itself, not the whole call", () => { + // The batch schema advertises the element shape without enforcing it, so a + // bad entry reaches the unitary executor and is refused at its index. If it + // were enforced at the container, one typo would cost every other cut — + // which is precisely what `applyBatch` says it exists to prevent. + const result = executeAgentTool( + fixtureDocument(), + "addTrims", + JSON.stringify({ + ranges: [{ startSec: 1, endSec: 2 }, { startSec: "oops" }, { startSec: 40, endSec: 41 }], + }), + ); + + expect(result.ok).toBe(true); + const payload = JSON.parse(result.resultJson); + expect(payload.appliedCount).toBe(2); + expect(payload.refused).toEqual([{ index: 1, error: expect.stringMatching(/endSec/) }]); + expect(result.document?.timeline.trimRanges).toHaveLength(3); + }); + + it("addZooms refuses a MALFORMED region by itself, not the whole call", () => { + const result = executeAgentTool( + fixtureDocument(), + "addZooms", + JSON.stringify({ + regions: [ + { startSec: 1, endSec: 3, depth: 9 }, // depth is an ordinal 1–6 + { startSec: 10, endSec: 12 }, + ], + }), + ); + + expect(result.ok).toBe(true); + const payload = JSON.parse(result.resultJson); + expect(payload.appliedCount).toBe(1); + expect(payload.refused[0].index).toBe(0); + expect(result.document?.zoomRanges).toHaveLength(1); + }); + + it("addTrims still refuses a batch that is not a non-empty list", () => { + for (const args of ['{"ranges":[]}', '{"ranges":"1-2"}', "{}"]) { + const result = executeAgentTool(fixtureDocument(), "addTrims", args); + expect(result.ok).toBe(false); + expect(result.document).toBeUndefined(); + } + }); + + it("addTrims reports a whole-batch refusal as a failure, not an empty success", () => { + const result = executeAgentTool( + fixtureDocument(), + "addTrims", + JSON.stringify({ + ranges: [ + { startSec: 1, endSec: 2, assetId: "asset_missing" }, + { startSec: 3, endSec: 4, assetId: "asset_missing" }, + ], + }), + ); + expect(result.ok).toBe(false); + expect(result.document).toBeUndefined(); + const error = JSON.parse(result.resultJson).error; + expect(error).toMatch(/\[0\]/); + expect(error).toMatch(/\[1\]/); + expect(error).toMatch(/Nothing was modified/); + }); + + it("addZooms lands the reachable regions and names the one covering no clip", () => { + const result = executeAgentTool( + fixtureDocument(), + "addZooms", + JSON.stringify({ + regions: [ + { startSec: 1, endSec: 3, depth: 2 }, + { startSec: 400, endSec: 402 }, // past the end of the timeline + { startSec: 10, endSec: 12, depth: 4 }, + ], + }), + ); + + expect(result.ok).toBe(true); + const payload = JSON.parse(result.resultJson); + expect(payload.appliedCount).toBe(2); + expect(payload.refused[0].index).toBe(1); + // Each applied entry still carries what the unitary tool reports, so the + // model can quote the rendered scale instead of the depth ordinal. + expect(payload.applied[0].renderedScale).toBe(ZOOM_DEPTH_SCALES[2]); + expect(payload.applied[1].renderedScale).toBe(ZOOM_DEPTH_SCALES[4]); + expect(result.document?.zoomRanges).toHaveLength(2); + }); + + it("addZooms leaves overlapping regions overlapping, exactly as one-at-a-time does", () => { + // A deliberate non-decision, pinned so it stays deliberate. + // + // `timelineMap.ts` forbids two zooms of different identities from + // overlapping, but only the `set*` path clamps (via `replacePillSpan`) — + // no `add*` does, in the agent OR in the UI. So two overlapping addZoom + // calls already produce an overlapping document today. Deconflicting + // inside the batch would make `addZooms` mean something its unitary + // sibling does not, and the model would get different results depending on + // how it chose to group its calls. The batch saves round trips; it does + // not quietly hold different rules. The bench still flags the overlap + // (`editorial.ts` zoomIssues), which is where that argument belongs. + const regions = [ + { startSec: 1, endSec: 6 }, + { startSec: 4, endSec: 9 }, + ]; + + let oneAtATime = fixtureDocument(); + for (const region of regions) { + oneAtATime = executeAgentTool(oneAtATime, "addZoom", JSON.stringify(region)) + .document as AxcutDocument; + } + const batch = executeAgentTool(fixtureDocument(), "addZooms", JSON.stringify({ regions })); + + const spans = (doc: AxcutDocument) => + doc.zoomRanges.map((z) => ({ startMs: z.startMs, endMs: z.endMs, depth: z.depth })); + expect(spans(batch.document as AxcutDocument)).toEqual(spans(oneAtATime)); + expect(batch.document?.zoomRanges).toHaveLength(2); + }); + it("addTrim rejects unknown assets", () => { const result = executeAgentTool( fixtureDocument(), diff --git a/electron/ai-edition/agent-tools.ts b/electron/ai-edition/agent-tools.ts index 4118daed5a..67bf72a9be 100644 --- a/electron/ai-edition/agent-tools.ts +++ b/electron/ai-edition/agent-tools.ts @@ -348,6 +348,32 @@ export const addTrimArgs = z.object({ reason: z.string().default(""), }); +/** + * ponytail: the element schema is `addTrimArgs` itself, not a copy of it. + * + * A batch is N unitary calls and nothing else — same validation, same clip + * resolution, same refusal wording — so the two can never drift into meaning + * different things. A separate element schema would be one more place to forget + * `clipId` the next time the unitary one grows a field. + * + * The `union(…, unknown)` is what makes "each item stands or falls alone" true for + * MALFORMED items too, not just unplaceable ones. A bare `z.array(addTrimArgs)` + * rejects the whole call the moment one entry is bad — and it rejects it in + * LangChain, before `applyBatch` runs — so nine good cuts would be thrown away + * with the tenth and `refused[index]` could never name it. Advertising the + * union keeps the element shape in the JSON schema the model reads (it shows up + * as `anyOf: [addTrim, {}]`) while letting a bad entry through to the unitary + * executor, which refuses it by itself with the wording it always uses. + * + * No cap on the array. A half-hour recording has hundreds of silences, and the + * point of this tool is precisely that it should not have to guess how many are + * too many. Picking a number here would repeat the mistake `getTranscript` made + * with its 800. + */ +export const addTrimsArgs = z.object({ + ranges: z.array(z.union([addTrimArgs, z.unknown()])).min(1), +}); + export const setTrimArgs = z.object({ trimRangeId: z.string().min(1), startSec: secondsSchema, @@ -410,6 +436,12 @@ export const addZoomArgs = z.object({ focus: focusSchema.default({ cx: 0.5, cy: 0.5 }), }); +/** Same contract as `addTrimsArgs`: the element schema IS the unitary one, and + * it is advertised rather than enforced so a bad region is refused by itself. */ +export const addZoomsArgs = z.object({ + regions: z.array(z.union([addZoomArgs, z.unknown()])).min(1), +}); + export const setZoomArgs = z.object({ zoomId: z.string().min(1), startSec: secondsSchema.optional(), @@ -486,6 +518,8 @@ export const removeClipArgs = z.object({ */ export const MUTATING_TOOL_NAMES: ReadonlySet = new Set([ "addTrim", + "addTrims", + "addZooms", "setTrim", "setClipRange", "moveClip", @@ -654,6 +688,79 @@ function failure(message: string): AgentToolExecution { return { ok: false, resultJson: JSON.stringify({ error: message }) }; } +/** + * Runs `unitName` once per item, folding the document forward. + * + * ponytail: the batch tools exist to save ROUND TRIPS, not to mean something new. + * Replaying the unitary executor is what guarantees that — anchoring, clip + * resolution, clamping, the wording of every refusal, all identical by + * construction rather than by a second implementation staying in step. A batch + * of N is exactly N unitary calls minus N-1 round trips, and `agent-tools.test` + * asserts that against a document built the long way. + * + * ponytail: PARTIAL application, deliberately. `replaceTimeline` is the repo's + * other array-taking tool and it refuses in one block — "Refused … Nothing was + * modified" — which is right for a tool that rebuilds the whole timeline and + * ruinous for one that adds ten independent cuts: a single bad bound would throw + * away nine good ones and the model would have to guess which. So each item + * stands or falls alone, and the result says which did what. `ok:false` is kept + * for the case where NOTHING landed, because that is the only one where the + * document did not move. + */ +function applyBatch( + document: AxcutDocument, + unitName: "addTrim" | "addZoom", + items: unknown[], + options: AgentToolOptions | undefined, + noun: string, +): AgentToolExecution { + let current = document; + const applied: Array> = []; + const refused: Array<{ index: number; error: string }> = []; + + items.forEach((item, index) => { + const execution = executeAgentTool(current, unitName, JSON.stringify(item), options); + let payload: Record = {}; + try { + payload = JSON.parse(execution.resultJson) as Record; + } catch { + payload = { error: execution.resultJson }; + } + if (execution.ok && execution.document) { + current = execution.document; + applied.push({ index, ...payload }); + } else { + refused.push({ index, error: String(payload.error ?? "refused") }); + } + }); + + // Nothing landed: the document is untouched, so say so the way every other + // refusal does rather than reporting a success with an empty list. + if (applied.length === 0) { + return failure( + `No ${noun} was added. ` + + refused.map((r) => `[${r.index}] ${r.error}`).join(" | ") + + " Nothing was modified.", + ); + } + + const refusedSuffix = refused.length ? `, ${refused.length} refused` : ""; + return { + ok: true, + document: current, + // The counts come first on purpose: the model must be able to see that one + // of ten was refused WITHOUT re-reading the document, and know which one. + resultJson: JSON.stringify({ + requested: items.length, + appliedCount: applied.length, + refusedCount: refused.length, + applied, + ...(refused.length ? { refused } : {}), + }), + summary: `added ${applied.length} ${noun}${applied.length === 1 ? "" : "s"}${refusedSuffix}`, + }; +} + /** The clips as the model would have to name them, for an error about an id it * got wrong — a bare "Unknown clip: demo" leaves it guessing twice. */ function clipRoster(document: AxcutDocument): string { @@ -854,9 +961,21 @@ export function executeAgentTool( if (!transcript) { return failure(`No transcript for asset ${assetId ?? "(none)"}.`); } - // ponytail: segments only — words would blow the context for long - // recordings and the segment text already carries the content. - const segments = transcript.segments.slice(0, 800).map((s) => ({ + // ponytail: no cap. There used to be a `.slice(0, 800)` here, guarded by + // "words would blow the context" — written believing a segment was a + // phrase. On the production path a segment IS one word + // (src/lib/captioning/transcribe.ts: whisper's word timings are mapped + // one-to-one), so the cap cut the transcript at the 800th WORD — around + // five minutes of speech — and said nothing about it. The model read a + // fifth of a half-hour recording, cut the silences it could see, and + // reported the job done, because nothing in the payload told it otherwise. + // + // A whole 30-minute transcript is ~285k characters, ~70k tokens: large, + // and well inside every model this app talks to. If a recording ever does + // get near a window, the honest fix is to know the window — the app has no + // per-model context budget today — not to guess a number here and drop the + // rest in silence. + const segments = transcript.segments.map((s) => ({ id: s.id, kind: s.kind, startSec: s.startSec, @@ -932,6 +1051,12 @@ export function executeAgentTool( }; } + case "addTrims": { + const parsed = addTrimsArgs.safeParse(args); + if (!parsed.success) return failure(parsed.error.message); + return applyBatch(document, "addTrim", parsed.data.ranges, options, "trim"); + } + case "setTrim": { const parsed = setTrimArgs.safeParse(args); if (!parsed.success) return failure(parsed.error.message); @@ -1167,6 +1292,12 @@ export function executeAgentTool( }; } + case "addZooms": { + const parsed = addZoomsArgs.safeParse(args); + if (!parsed.success) return failure(parsed.error.message); + return applyBatch(document, "addZoom", parsed.data.regions, options, "zoom"); + } + case "setZoom": { const parsed = setZoomArgs.safeParse(args); if (!parsed.success) return failure(parsed.error.message); diff --git a/electron/ai-edition/chat-compaction.test.ts b/electron/ai-edition/chat-compaction.test.ts index 5c9c17bd3d..838458dda4 100644 --- a/electron/ai-edition/chat-compaction.test.ts +++ b/electron/ai-edition/chat-compaction.test.ts @@ -1,12 +1,12 @@ -import { beforeEach, describe, expect, it } from "vitest"; +import { describe, expect, it } from "vitest"; import type { AiEditionChatMessage } from "../../src/native/contracts"; import { applyCompaction, budgetSnapshot, buildCompactionPrompt, compactionReducesHistory, + compactionSplitIndex, estimateHistoryTokens, - shouldCompact, } from "./chat-compaction"; function msg( @@ -51,18 +51,9 @@ describe("budgetSnapshot", () => { }); }); -describe("shouldCompact", () => { - beforeEach(() => undefined); - +describe("compactionSplitIndex", () => { it("returns null for very short histories", () => { - expect(shouldCompact([msg("user", "hi")])).toBeNull(); - }); - - it("returns null when under the threshold", () => { - const small = Array.from({ length: 6 }, (_, i) => - msg(i % 2 ? "assistant" : "user", `short-${i}`), - ); - expect(shouldCompact(small, 1_000_000)).toBeNull(); + expect(compactionSplitIndex([msg("user", "hi")])).toBeNull(); }); it("compacts on a user-message boundary near the midpoint", () => { @@ -70,11 +61,21 @@ describe("shouldCompact", () => { for (let i = 0; i < 10; i += 1) { msgs.push(msg(i % 2 ? "assistant" : "user", `turn-${i}-${"x".repeat(800)}`)); } - const out = shouldCompact(msgs, 50); + const out = compactionSplitIndex(msgs); expect(out).not.toBeNull(); - expect(out?.compact).toBe(true); // boundary must be a user message - expect(msgs[out!.splitIndex]?.role).toBe("user"); + expect(msgs[out as number]?.role).toBe("user"); + }); + + it("does not consult any token budget — a tiny history still splits", () => { + // The regression this pins: compaction used to refuse below 70% of a + // guessed 80k-token budget, which gated the manual button too, so + // pressing Compact on a short conversation did nothing at all and said + // nothing about why. The app cannot know a model's context window, so + // there is no threshold left to be wrong about. + const tiny = Array.from({ length: 6 }, (_, i) => msg(i % 2 ? "assistant" : "user", "hi")); + expect(estimateHistoryTokens(tiny)).toBeLessThan(100); + expect(compactionSplitIndex(tiny)).not.toBeNull(); }); }); diff --git a/electron/ai-edition/chat-compaction.ts b/electron/ai-edition/chat-compaction.ts index 1b11b5e32a..5be23d87be 100644 --- a/electron/ai-edition/chat-compaction.ts +++ b/electron/ai-edition/chat-compaction.ts @@ -1,11 +1,17 @@ -// Context-budget heuristic + message-history compaction. Mirrors axcut's -// "compact-on-overflow" approach in spirit (sliding window with summary), -// but kept simple: char-based token estimate + a manual Compact button. +// Message-history compaction: fold the older half of a conversation into one +// "Earlier context" summary. The summary is an LLM call (no tools, plain text) +// using the active provider. // -// Chat-service calls `shouldCompact` before each new turn; when the heuristic -// trips, `compactHistory` summarizes the older half of the conversation and -// returns a new history list to feed the model. The summary itself is an -// LLM call (no tools, plain text → JSON summary) using the active provider. +// ponytail: compaction is MANUAL ONLY, and there is no overflow heuristic. +// There used to be one — compact automatically once the history passed 70% of +// `DEFAULT_BUDGET_TOKENS = 80_000`. That 80k was invented: the app has no +// per-model context window, so the number could not be right for anything. It +// was far too small for Gemini's 1M window (throwing away context at 5% fill, +// and paying a blocking summarizer call to do it) and would be too large for +// something small. It is the same mistake `getTranscript` made with its 800 +// segments, and it gets the same answer: a guessed limit is deleted, not +// retuned. Until the app can ask a provider for the real window, the only +// honest trigger is a person deciding they want it, which is the button. import type { AiEditionChatMessage } from "../../src/native/contracts"; @@ -22,8 +28,10 @@ export interface CompactionBudget { } /** - * Default budget. Real providers run 100k+ contexts, but we leave headroom - * for tool-call payload + system prompt. Adjust per provider if needed. + * The denominator of the context pill in the chat panel, and nothing else — + * no code branches on it any more. It is still a made-up number, so it must + * never regain a decision: read it as "the conversation is about this big", + * not as "you are this close to a limit". */ export const DEFAULT_BUDGET_TOKENS = 80_000; @@ -52,25 +60,22 @@ export function budgetSnapshot( } /** - * Decide whether the chat history should be compacted before the next turn. - * Returns the boundary index where compaction should cut (older half). + * Where a compaction should cut, or `null` when there is nothing to fold. + * + * The only refusal left is "fewer than 4 messages": that is not a guess about + * anyone's context window, it is that summarizing one exchange into a summary + * cannot make it shorter. Everything else is the caller's decision. */ -export function shouldCompact( - messages: AiEditionChatMessage[], - budgetTokens: number = DEFAULT_BUDGET_TOKENS, - thresholdRatio = 0.7, -): { compact: boolean; splitIndex: number } | null { +export function compactionSplitIndex(messages: AiEditionChatMessage[]): number | null { if (messages.length < 4) return null; - const budget = budgetSnapshot(messages, budgetTokens); - if (budget.ratio < thresholdRatio) return null; // Split roughly in half. Snap to a user-message boundary so the model // doesn't see a half-turn after compaction. const split = Math.floor(messages.length / 2); for (let i = split; i < messages.length; i += 1) { - if (messages[i]?.role === "user") return { compact: true, splitIndex: i }; + if (messages[i]?.role === "user") return i; } - return { compact: true, splitIndex: split }; + return split; } /** diff --git a/electron/ai-edition/chat-service.compaction.test.ts b/electron/ai-edition/chat-service.compaction.test.ts index ee9427844c..2489398c5a 100644 --- a/electron/ai-edition/chat-service.compaction.test.ts +++ b/electron/ai-edition/chat-service.compaction.test.ts @@ -49,8 +49,9 @@ function stubSummarizer(reply: string) { return invoke; } -// Long enough that four of them clear the 70%-of-80k-tokens trip point, short -// enough that three of them do not. +// Deliberately huge: these used to be sized against the 70%-of-80k trip point, +// and they stay huge for the opposite reason — a history this big is the case +// that USED to compact itself, so it is the one that proves nothing does now. const LONG = "x".repeat(60_000); beforeEach(() => { @@ -63,13 +64,36 @@ beforeEach(() => { }); }); -describe("auto-compaction", () => { - it("leaves the transcript whole and compacts only what the model is given", async () => { +describe("compaction", () => { + it("NEVER runs on its own, however big the history gets", async () => { + // The headline rule. A turn used to measure the history against a + // guessed 80k-token budget and, past 70% of it, block on a whole extra + // summarizer call before the user's request was even sent. The app has + // no way to ask a provider how big its context window is, so that number + // could not be right for anything — it threw away context at 5% fill on + // a 1M-token Gemini. Six turns here estimate at ~90k tokens, comfortably + // past the old trip point. + const summarizer = stubSummarizer("EARLIER CONTEXT"); + const session = createSession("proj_no_auto"); + for (let i = 0; i < 6; i += 1) { + await runChat("proj_no_auto", session.id, `${LONG}#${i}`, stubConfig()); + } + + expect(getSessionContextUsage("proj_no_auto", session.id)?.usedTokens).toBeGreaterThan(56_000); + expect(summarizer).not.toHaveBeenCalled(); + // And nothing was folded away behind the user's back. + const history = histories.at(-1) ?? []; + expect(history.some((m) => m.content === `${LONG}#0`)).toBe(true); + expect(history.at(-1)?.content).toBe(`${LONG}#5`); + }); + + it("compacts on the button, and leaves the transcript whole", async () => { stubSummarizer("EARLIER CONTEXT"); const session = createSession("proj_compact_transcript"); for (let i = 0; i < 4; i += 1) { await runChat("proj_compact_transcript", session.id, `${LONG}#${i}`, stubConfig()); } + await compactSessionNow("proj_compact_transcript", session.id, stubConfig()); // Four user turns, four replies, nothing deleted: this array is what the // renderer shows, and the user never asked for half of it to go away. @@ -78,26 +102,46 @@ describe("auto-compaction", () => { expect(transcript[0]?.content).toBe(`${LONG}#0`); expect(transcript.filter((m) => m.role === "user")).toHaveLength(4); - // The fourth turn is the one that tripped the budget: the model got the - // summary in place of the older half, not the whole conversation. + // The next turn gets the summary in place of the older half. + await runChat("proj_compact_transcript", session.id, "and then?", stubConfig()); const history = histories.at(-1) ?? []; expect(history[0]?.content).toBe("EARLIER CONTEXT"); - expect(history).toHaveLength(4); expect(history.some((m) => m.content === `${LONG}#0`)).toBe(false); - expect(history.at(-1)?.content).toBe(`${LONG}#3`); + expect(history.at(-1)?.content).toBe("and then?"); - // The context pill measures the payload, so compaction actually shows up: - // the whole transcript estimates at ~60k tokens, the payload at half. + // The context pill measures the payload, so compaction shows up there. const usage = getSessionContextUsage("proj_compact_transcript", session.id); expect(usage?.usedTokens).toBeLessThan(40_000); }); + it("compacts an ORDINARY conversation — the button is not gated by a budget", async () => { + // The same guessed budget gated the manual path: `compactSessionNow` + // went through the same heuristic, so below 70% of 80k the button did + // nothing at all, silently. This session is ~3k tokens — a perfectly + // normal chat, roughly 5% of the old trip point, and exactly the size at + // which the button used to be a no-op. Pressing it is the decision now; + // there is no number left to overrule it. + const summarizer = stubSummarizer("EARLIER CONTEXT"); + const session = createSession("proj_compact_short"); + const paragraph = "a".repeat(2_000); + for (let i = 0; i < 3; i += 1) { + await runChat("proj_compact_short", session.id, `${paragraph}#${i}`, stubConfig()); + } + const used = getSessionContextUsage("proj_compact_short", session.id)?.usedTokens ?? 0; + expect(used).toBeLessThan(56_000 / 10); + + const manual = await compactSessionNow("proj_compact_short", session.id, stubConfig()); + expect(summarizer).toHaveBeenCalledTimes(1); + expect(manual?.summary).toBe("EARLIER CONTEXT"); + }); + it("keeps the summary in the payload when the tail is longer than the window", async () => { stubSummarizer("EARLIER CONTEXT"); const session = createSession("proj_compact_window"); for (let i = 0; i < 30; i += 1) { await runChat("proj_compact_window", session.id, `turn ${i}`, stubConfig()); } + await compactSessionNow("proj_compact_window", session.id, stubConfig()); const huge = LONG.repeat(4); await runChat("proj_compact_window", session.id, huge, stubConfig()); @@ -109,47 +153,48 @@ describe("auto-compaction", () => { expect(history.at(-1)?.content).toBe(huge); }); - it("stops retrying after a summary that does not shrink the payload", async () => { + it("refuses a summary that does not shrink the payload, and keeps the session", async () => { const oversized = stubSummarizer("z".repeat(400_000)); const session = createSession("proj_compact_blocked"); for (let i = 0; i < 5; i += 1) { await runChat("proj_compact_blocked", session.id, `${LONG}#${i}`, stubConfig()); } - // Two more turns tripped the heuristic after the failure; neither paid - // for another summarizer call. + // Adopting a summary longer than what it replaces would grow the payload. + // The session is left exactly as it was. (There is no "stop retrying" + // flag any more: nothing retries on its own, so the only next attempt is + // another press, which is the user asking again knowingly.) + expect(await compactSessionNow("proj_compact_blocked", session.id, stubConfig())).toBeNull(); expect(oversized).toHaveBeenCalledTimes(1); - const history = histories.at(-1) ?? []; - expect(history.some((m) => m.content === "EARLIER CONTEXT")).toBe(false); expect(selectSession("proj_compact_blocked", session.id)?.messages).toHaveLength(10); - // The Compact button is an explicit request, so it tries again — and a - // success unblocks the automatic path. + await runChat("proj_compact_blocked", session.id, "and then?", stubConfig()); + expect(histories.at(-1)?.some((m) => m.content === "EARLIER CONTEXT")).toBe(false); + + // A second press with a usable summary lands. const usable = stubSummarizer("EARLIER CONTEXT"); const manual = await compactSessionNow("proj_compact_blocked", session.id, stubConfig()); expect(usable).toHaveBeenCalledTimes(1); expect(manual?.summary).toBe("EARLIER CONTEXT"); - expect(manual?.session.messages).toHaveLength(10); + expect(manual?.session.messages).toHaveLength(12); - await runChat("proj_compact_blocked", session.id, "and then?", stubConfig()); + await runChat("proj_compact_blocked", session.id, "and after that?", stubConfig()); expect(histories.at(-1)?.[0]?.content).toBe("EARLIER CONTEXT"); }); - // The regression this guards is `planCompaction` measuring the wrong list. - // `splitIndex` comes back from `shouldCompact` as an index INTO WHAT IT WAS - // GIVEN, and it is then applied to the payload. Measure the transcript - // instead — which never shrinks, so it keeps tripping — and the index runs - // off the end of the much shorter payload, so `payload.slice(0, splitIndex)` - // swallows the whole thing, current user turn included. The model is then - // asked to answer a question it was never shown. - // - // Three turns is not enough to see it: the collapse needs a payload that has - // already been compacted at least once, so the two lists have diverged. + // `splitIndex` comes back as an index INTO WHAT WAS MEASURED, and it is then + // applied to the payload. Measure the transcript instead — which compaction + // never shrinks — and the index runs off the end of the much shorter + // payload, so `payload.slice(0, splitIndex)` swallows the whole thing, the + // current user turn included, and the model is asked to answer a question it + // was never shown. Repeated compactions are what make the two lists diverge, + // so the button is pressed between every turn here. it("never summarizes away the turn the user just sent", async () => { stubSummarizer("EARLIER CONTEXT"); const session = createSession("proj_compact_current_turn"); for (let i = 0; i < 10; i += 1) { await runChat("proj_compact_current_turn", session.id, `${LONG}#${i}`, stubConfig()); + await compactSessionNow("proj_compact_current_turn", session.id, stubConfig()); } // Every turn, not just the last: the collapse is intermittent, so a diff --git a/electron/ai-edition/chat-service.ts b/electron/ai-edition/chat-service.ts index eec2003643..89c1d78e20 100644 --- a/electron/ai-edition/chat-service.ts +++ b/electron/ai-edition/chat-service.ts @@ -27,8 +27,8 @@ import { buildCompactionPrompt, COMPACTION_SYSTEM_PROMPT, compactionReducesHistory, + compactionSplitIndex, DEFAULT_BUDGET_TOKENS, - shouldCompact, } from "./chat-compaction"; import type { CursorTelemetryReader } from "./deep-agent/service"; import type { DocumentService } from "./document-service"; @@ -133,10 +133,6 @@ export interface ChatSession { /** Main-process bookkeeping: the compaction boundary, not part of the * transcript. See `modelMessages`. */ compaction?: SessionCompaction; - /** Set when a summarize call came back no smaller than what it replaced. - * Auto-compaction then stops trying — otherwise every following turn pays - * for the same useless summarizer call. */ - compactionBlocked?: boolean; } /** The message list compaction hands the model: summary first, then everything @@ -368,22 +364,14 @@ export async function runChat( const editsAllowed = config.allowAgentEdits !== false; - // P3.7 — context compaction: when what we send the model grows past the - // heuristic budget, summarize the older half into a single "Earlier - // context" assistant message. The current user turn stays uncompacted, so - // the model still sees the request verbatim. - const plan = session.compactionBlocked ? null : planCompaction(session); - if (plan) { - await tryCompactSession({ - session, - plan, - apiKey: apiKey ?? "", - provider: config.provider, - model: config.model, - baseUrl: config.baseUrl, - reasoningEffort: config.reasoningEffort, - }); - } + // ponytail: NO automatic compaction here. A turn used to first check the + // history against a guessed 80k-token budget and, past 70% of it, block on + // a whole extra summarizer call before the user's request was even sent. + // The app cannot ask a provider how big its context window is, so that + // budget was a number someone picked — wrong by an order of magnitude for a + // 1M-token Gemini, and silently discarding context the model could have + // held. Compaction is now only ever what the user asked for by pressing the + // button. See chat-compaction.ts. const history = modelHistory(session).map((m) => ({ role: m.role as "user" | "assistant" | "system", @@ -608,9 +596,9 @@ export async function compactSessionNow( const credential = llmConfig.getCredential(def.id, def.envKeys); const apiKey = credential?.value ?? ""; - // The button is an explicit request, so it ignores `compactionBlocked` — - // the user knows they are spending a summarizer call, and a success clears - // the flag for the automatic path too. + // Pressing the button IS the decision — nothing here second-guesses it + // against a budget. It only declines when there is genuinely nothing to + // fold (fewer than 4 messages), which is the one refusal left. const plan = planCompaction(session); if (!plan) return null; @@ -729,25 +717,21 @@ interface CompactionPlan { } /** - * Decide whether the next turn should compact, measuring the payload rather - * than the transcript. Measuring the transcript would re-trip on every turn - * for the rest of the session, since compaction no longer shrinks it. + * Where a compaction would cut, measured against the payload rather than the + * transcript — compaction never rewrites the transcript, so the split index + * has to be an index into the list it will actually be applied to. * * A second compaction folds the previous summary into the new one: it sits at * `payload[0]`, so it is part of the prefix being summarized. */ function planCompaction(session: ChatSession): CompactionPlan | null { const payload = modelMessages(session); - const decision = shouldCompact(payload); - if (!decision?.compact || decision.splitIndex <= 0) return null; + const splitIndex = compactionSplitIndex(payload); + if (splitIndex === null || splitIndex <= 0) return null; // Payload index → transcript index. With a summary in front, payload[i] // is transcript message `coveredCount + i - 1`. const offset = session.compaction ? session.compaction.coveredCount - 1 : 0; - return { - payload, - splitIndex: decision.splitIndex, - coveredCount: decision.splitIndex + offset, - }; + return { payload, splitIndex, coveredCount: splitIndex + offset }; } /** @@ -804,15 +788,14 @@ async function tryCompactSession(opts: { const summaryMessage = compacted[0]; if (!summaryMessage) return null; if (!compactionReducesHistory(plan.payload, compacted)) { - // ponytail: the model handed back a summary at least as long as the - // messages it replaced. Adopting it would grow the payload, and - // retrying next turn just buys the same answer again — so stop asking - // until the user compacts by hand. - session.compactionBlocked = true; + // The model handed back a summary at least as long as the messages it + // replaced: adopting it would grow the payload. Refuse it and leave the + // session alone. (There is no "stop trying" flag any more — nothing + // retries on its own, so the only next attempt is another button press, + // which is the user asking again knowingly.) return null; } session.compaction = { summary: summaryMessage, coveredCount: plan.coveredCount }; - session.compactionBlocked = false; return { summaryMessageId: summaryMessage.id, summary, diff --git a/electron/ai-edition/deep-agent/service.test.ts b/electron/ai-edition/deep-agent/service.test.ts index ec7770ac73..43e963c227 100644 --- a/electron/ai-edition/deep-agent/service.test.ts +++ b/electron/ai-edition/deep-agent/service.test.ts @@ -42,11 +42,13 @@ const OPENSCREEN_TOOLS = [ "getTranscript", "getCursorTrack", "addTrim", + "addTrims", "setTrim", "setClipRange", "moveClip", "replaceTimeline", "addZoom", + "addZooms", "setZoom", "addSpeed", "setSpeed", @@ -84,11 +86,13 @@ const ARGS: Record = { getTranscript: {}, getCursorTrack: {}, addTrim: { startSec: 1, endSec: 2 }, + addTrims: { ranges: [{ startSec: 1, endSec: 2 }] }, setTrim: { trimRangeId: "trim_1", startSec: 1, endSec: 2 }, setClipRange: { clipId: "clip_1", sourceStartSec: 0, sourceEndSec: 10 }, moveClip: { clipId: "clip_1", beforeClipId: null }, replaceTimeline: { intervals: [{ startSec: 0, endSec: 10 }] }, addZoom: { startSec: 1, endSec: 2 }, + addZooms: { regions: [{ startSec: 1, endSec: 2 }] }, setZoom: { zoomId: "zoom_nope" }, addSpeed: { startSec: 1, endSec: 2 }, setSpeed: { speedId: "speed_nope" }, @@ -171,7 +175,7 @@ function recordingSink(): { sink: OpenScreenAgentSink; events: SinkEvent[] } { } /** `buildTools` returns a tuple with a DISTINCT type per tool, one per zod - * schema, so `tools.find(...)` is a 19-way union — and `.invoke` is generic, a + * schema, so `tools.find(...)` is a 21-way union — and `.invoke` is generic, a * shape TypeScript will not call through a union (TS2349). Widening to the * interface every one of them implements is what the model is handed anyway: * `createAgent` takes them as `ClientTool`, i.e. exactly this. Nothing the @@ -187,7 +191,7 @@ function toolsFor(document: AxcutDocument) { } describe("the tool surface handed to the model", () => { - it("is exactly OpenScreen's 19 tools", () => { + it("is exactly OpenScreen's 21 tools", () => { const { tools } = toolsFor(fixtureDocument()); expect(tools.map((t) => t.name)).toEqual(OPENSCREEN_TOOLS); }); @@ -288,6 +292,23 @@ describe("the sink announces each call exactly once, with the real verdict", () expect(events[1]).toMatchObject({ kind: "toolEnd", name: "getTranscript", ok: false }); }); + it("lets a malformed batch entry reach the executor instead of throwing at the schema", async () => { + // LangChain parses the tool's schema BEFORE calling us, so a batch schema + // that enforced its element shape would reject the whole call here — the + // per-item `refused[index]` that `addTrims` promises the model could never + // happen on the product path, only in a direct-executor test. + const { tools, holder } = toolsFor(fixtureDocument()); + const tool = tools.find((t) => t.name === "addTrims"); + if (!tool) throw new Error("addTrims is not built"); + + const result = JSON.parse( + String(await tool.invoke({ ranges: [{ startSec: 1, endSec: 2 }, { startSec: "oops" }] })), + ); + expect(result.appliedCount).toBe(1); + expect(result.refused[0].index).toBe(1); + expect(holder.current.timeline.trimRanges).toHaveLength(2); + }); + it("advances the holder on a write, and leaves it alone on a refusal", async () => { const { tools, holder } = toolsFor(fixtureDocument()); const before = holder.current; @@ -398,7 +419,7 @@ describe("the prompt when the user has turned project edits off", () => { }); describe("the tools when the user has turned project edits off", () => { - it("still builds all 18 — the model has to be able to NAME the edit", () => { + it("still builds all 21 — the model has to be able to NAME the edit", () => { const { sink } = recordingSink(); const tools: BuiltTool[] = buildTools({ current: fixtureDocument() }, sink, false); expect(tools.map((t) => t.name)).toEqual(OPENSCREEN_TOOLS); diff --git a/electron/ai-edition/deep-agent/service.ts b/electron/ai-edition/deep-agent/service.ts index bc10c415c2..e025827e1d 100644 --- a/electron/ai-edition/deep-agent/service.ts +++ b/electron/ai-edition/deep-agent/service.ts @@ -30,7 +30,9 @@ import { addCameraFullscreenArgs, addSpeedArgs, addTrimArgs, + addTrimsArgs, addZoomArgs, + addZoomsArgs, type CursorTelemetryLoad, executeAgentTool, getCursorTrackArgs, @@ -110,7 +112,7 @@ const BASE_SYSTEM_PROMPT = [ // happened to list and silently misses every paraphrase — and every language // other than English. Say what the tool does; let the model do the matching. "How the tools map to intent — pick the most specific one, and prefer the smallest edit that satisfies the request:", - "- Silences, pauses and dead stretches are removed as trims INSIDE the placed clip: one addTrim per range. The placed clip stays the canonical cut; it is not rebuilt to drop them.", + "- Silences, pauses and dead stretches are removed as trims INSIDE the placed clip. Send them together with addTrims once you know the ranges; addTrim is for a single cut or a correction. The placed clip stays the canonical cut; it is not rebuilt to drop them.", "- Changing where a clip starts or ends within its source is setClipRange — the clip's in/out, distinct from a trim.", `- addZoom takes a virtual-timeline span (depth is an ordinal 1–6 selecting from a fixed table — ${ZOOM_DEPTH_LEGEND} — never a multiplier; focus in 0–1 frame fractions). addSpeed changes pacing over a span. addAnnotation puts text on screen. addCameraFullscreen enlarges the webcam, and only does something where assets[].hasCameraTrack is true.`, "- moveClip changes the order of placed clips, one call per clip that moves, preserving ids, source ranges, trims and anchored effects. replaceTimeline rebuilds the timeline from kept intervals and sorts them, so it cannot reorder anything.", @@ -142,7 +144,9 @@ export const TOOL_DESCRIPTIONS: Record = { getCursorTrack: "Read the recorded pointer track for an asset: where the cursor was over time, downsampled to a readable rate. Each point carries atSec (the asset's own source clock), virtualSec (the same instant on the edited timeline — the coordinate addZoom takes, null when no clip carries it), cx/cy as 0–1 fractions of the frame, and `shape`, an index into the pointer bitmaps the recording used (equal values are the same pointer; a change means the pointer changed, e.g. arrow to text caret). Points that are not plain moves carry `kind`; points a trim cuts out of playback carry `trimmed`. These are real samples, not a summary — reading what the pointer was doing is yours. Omit assetId for the primary asset. It answers `available:false` in two DIFFERENT ways you must not confuse: reason 'no-sidecar' means this asset was checked and genuinely has no telemetry, while reason 'unavailable' means it could not be read from here.", addTrim: - "Add a trim range: a cut of a span inside a clip (this source-time span will not be played or exported) that does NOT split the clip. Times are in seconds of the asset's source time. This is the preferred (and for 'remove silences' requests, the only) way to handle silences; it preserves the user's placed clips and only adds a cut. Call this once per silent range. A cut belongs to ONE clip: `clipId` is inferred when a single clip covers the range, but when several clips draw on the same asset over it the call FAILS and lists them — pass the `clipId` you mean (ids come from getCurrentDocument).", + "Add ONE trim range: a cut of a span inside a clip (this source-time span will not be played or exported) that does NOT split the clip. Times are in seconds of the asset's source time. This is the preferred (and for 'remove silences' requests, the only) way to handle silences; it preserves the user's placed clips and only adds a cut. When you have several cuts to make, use addTrims and send them together — this one is for a single cut or a later correction. A cut belongs to ONE clip: `clipId` is inferred when a single clip covers the range, but when several clips draw on the same asset over it the call FAILS and lists them — pass the `clipId` you mean (ids come from getCurrentDocument).", + addTrims: + "Add MANY trim ranges in one call: `ranges` is a list, each entry taking exactly the fields addTrim takes. Use this whenever you have more than one cut to make — 'remove the silences' on a half-hour recording is hundreds of cuts, and sending them one at a time costs one round trip each. Each range stands or falls ALONE: one that cannot be placed is refused by itself and listed in `refused` with its index and the reason, while every other range is still applied. Nothing is rolled back, so a single bad bound never costs you the rest. The result leads with requested / appliedCount / refusedCount so you can see a partial outcome without re-reading the document — report what was refused rather than claiming the whole list landed.", setTrim: "Move or resize an existing trim range by id. Times are source-time seconds. The cut follows to whichever clip the new range lands in, when that clip is unambiguous.", setClipRange: @@ -152,6 +156,7 @@ export const TOOL_DESCRIPTIONS: Record = { replaceTimeline: "Replace the whole timeline with the given kept intervals of the primary asset's source time. Everything outside the intervals becomes a trim. The intervals are SORTED, so this can never reorder clips — use moveClip for that. DO NOT use this for 'cut silences' or 'remove pauses' — the user has likely placed clips on the timeline that you'd be discarding. Use this ONLY when the user explicitly asks you to rebuild the timeline from scratch (e.g. 'start over with the kept intervals from the transcript'). It is refused when it would merge away, shorten or drop an existing clip; the refusal names them and the tool to use instead.", addZoom: `Add a zoom-in over a span of the edited timeline (virtual seconds). depth is an ORDINAL 1–6, not a factor: it selects a magnification from a fixed table (${ZOOM_DEPTH_LEGEND}), so the default depth 3 renders at 1.80×. The result reports renderedScale — quote that, never the depth, when telling the user how strong the zoom is. focus is the zoom centre in 0–1 fractions of the frame (default centre). Use for 'zoom in on …' and the smart-zoom pass.`, + addZooms: `Add MANY zooms in one call: \`regions\` is a list, each entry taking exactly the fields addZoom takes (same depth table, ${ZOOM_DEPTH_LEGEND}). Use this for the smart-zoom pass, where you have decided every zoom before emitting the first one — sending them one at a time costs one round trip each. Each region stands or falls ALONE: one that covers no clip is refused by itself and listed in \`refused\` with its index and the reason, while the others are still applied. The result leads with requested / appliedCount / refusedCount, and each applied entry carries its renderedScale — quote that, never the depth.`, setZoom: `Move, resize, or restyle an existing zoom by id (virtual-timeline seconds). Only the fields you pass are changed. depth selects from the same table (${ZOOM_DEPTH_LEGEND}); if the zoom carries a customScale (getCurrentDocument shows it as depthIsOverridden), that custom value is what renders, and passing depth clears it so the depth takes effect — the result says so. The result reports the resulting renderedScale.`, addSpeed: "Add a speed-change region over a span of the edited timeline (virtual seconds). speed > 1 fast-forwards, < 1 slows down (default 1.5×). Use to speed through slow stretches without cutting them.", @@ -298,11 +303,13 @@ export function buildTools( build("getTranscript", getTranscriptArgs), build("getCursorTrack", getCursorTrackArgs), build("addTrim", addTrimArgs), + build("addTrims", addTrimsArgs), build("setTrim", setTrimArgs), build("setClipRange", setClipRangeArgs), build("moveClip", moveClipArgs), build("replaceTimeline", replaceTimelineArgs), build("addZoom", addZoomArgs), + build("addZooms", addZoomsArgs), build("setZoom", setZoomArgs), build("addSpeed", addSpeedArgs), build("setSpeed", setSpeedArgs), diff --git a/electron/ai-edition/document-service.test.ts b/electron/ai-edition/document-service.test.ts index 995044f753..30f5fd718e 100644 --- a/electron/ai-edition/document-service.test.ts +++ b/electron/ai-edition/document-service.test.ts @@ -387,7 +387,10 @@ describe("DocumentService", () => { expect(onDisk.annotations).toHaveLength(120); }); - it("survives many interleaved saves of one project", async () => { + // 20 real temp-file+rename round trips, serialized through the save queue. + // That is genuinely more than 5s of disk when the rest of the suite is + // running in parallel, so it gets its own timeout rather than flaking. + it("survives many interleaved saves of one project", { timeout: 20_000 }, async () => { const doc = await service.createProject("Storm"); // Sizes deliberately alternate long/short: equal-length writes overwrite // each other cleanly and would prove nothing. diff --git a/electron/electron-env.d.ts b/electron/electron-env.d.ts index e5381999bf..9e1fb7d0d0 100644 --- a/electron/electron-env.d.ts +++ b/electron/electron-env.d.ts @@ -178,6 +178,8 @@ interface Window { recordingId: number; webcam: import("../src/lib/recordingSession").RecordedVideoAssetInput; cursorCaptureMode?: import("../src/lib/recordingSession").CursorCaptureMode; + durationMs?: number; + webcamOffsetMs?: number; }) => Promise<{ success: boolean; path?: string; @@ -233,6 +235,7 @@ interface Window { recordingId: number; webcam: import("../src/lib/recordingSession").RecordedVideoAssetInput; cursorCaptureMode?: import("../src/lib/recordingSession").CursorCaptureMode; + durationMs?: number; webcamOffsetMs?: number; }) => Promise<{ success: boolean; diff --git a/electron/ipc/handlers.ts b/electron/ipc/handlers.ts index 3800671e5b..7d63b934b1 100644 --- a/electron/ipc/handlers.ts +++ b/electron/ipc/handlers.ts @@ -71,6 +71,10 @@ import { createCursorRecordingSession } from "../native-bridge/cursor/recording/ import { requestMacCursorAccessibilityAccess } from "../native-bridge/cursor/recording/macNativeCursorRecordingSession"; import { findPipeWireCursorHelperPath } from "../native-bridge/cursor/recording/pipeWireCursorRecordingSession"; import type { CursorRecordingSession } from "../native-bridge/cursor/recording/session"; +import { + terminateNativeWindowsCapture, + waitForNativeWindowsCaptureStop, +} from "../recording/nativeWindowsCaptureStop"; import { patchWebmDurationOnDisk } from "../recording/webm-duration"; import { reindexRecordingOnDisk } from "../recording/webm-seek-index"; import { registerNativeBridgeHandlers } from "./nativeBridge"; @@ -439,6 +443,12 @@ type AttachNativeMacWebcamRecordingInput = { recordingId?: number; webcam?: RecordedVideoAssetInput; cursorCaptureMode?: CursorCaptureMode; + /** + * Webcam clip duration (ms), head start included. A streamed webcam file carries + * no Duration header and the renderer no longer holds the blob to patch, so the + * main process repairs the container on disk with this value. + */ + durationMs?: number; /** See {@link ProjectMedia.webcamOffsetMs}. */ webcamOffsetMs?: number; }; @@ -527,7 +537,74 @@ let nativeWindowsCursorRecordingStartMs = 0; let nativeWindowsPauseStartedAtMs: number | null = null; let nativeWindowsPauseRanges: Array<{ startMs: number; endMs: number }> = []; let nativeWindowsIsPaused = false; -const NATIVE_WINDOWS_CAPTURE_STOP_TIMEOUT_MS = 60_000; +/** Cuts a surviving helper's output loose so it cannot pollute the next recording. */ +let nativeWindowsCaptureDrainCleanup: (() => void) | null = null; + +function detachNativeWindowsCaptureOutputDrain() { + nativeWindowsCaptureDrainCleanup?.(); + nativeWindowsCaptureDrainCleanup = null; +} + +function resetNativeWindowsCaptureState() { + nativeWindowsCaptureDrainCleanup = null; + nativeWindowsCaptureProcess = null; + nativeWindowsCaptureTargetPath = null; + nativeWindowsCaptureWebcamTargetPath = null; + nativeWindowsCaptureRecordingId = null; + nativeWindowsCursorOffsetMs = 0; + nativeWindowsCursorCaptureMode = "editable-overlay"; + nativeWindowsCursorRecordingStartMs = 0; + nativeWindowsPauseStartedAtMs = null; + nativeWindowsPauseRanges = []; + nativeWindowsIsPaused = false; +} + +/** + * An MP4 the helper never indexed is a few bytes of header at most. Anything + * larger might be a real recording, and deleting one of those to tidy up after + * a failed stop is a far worse outcome than leaving a stray file behind. + */ +const NATIVE_WINDOWS_SALVAGEABLE_OUTPUT_BYTES = 64 * 1024; + +/** + * Best-effort removal of the files a failed or discarded native Windows capture + * left behind. Each removal is isolated: a helper that outlived its kill still + * holds the MP4 open on Windows, and an EBUSY there must not mask why we were + * cleaning up in the first place. + */ +async function removeNativeWindowsCaptureOutputs( + screenVideoPath: string | null, + webcamVideoPath: string | null, + options: { onlyIfUnusable?: boolean } = {}, +) { + const targets = [ + screenVideoPath, + webcamVideoPath, + screenVideoPath ? `${screenVideoPath}.cursor.json` : null, + ]; + + for (const target of targets) { + if (!target || !isPathWithinDir(target, RECORDINGS_DIR)) { + continue; + } + try { + if (options.onlyIfUnusable && target !== `${screenVideoPath}.cursor.json`) { + const stats = await fs.stat(target).catch(() => null); + if (stats && stats.size >= NATIVE_WINDOWS_SALVAGEABLE_OUTPUT_BYTES) { + console.warn( + "[native-wgc] keeping a capture output that may still be playable:", + target, + stats.size, + ); + continue; + } + } + await fs.rm(target, { force: true }); + } catch (error) { + console.warn("[native-wgc] could not remove leftover capture output:", target, error); + } + } +} let nativeMacCaptureProcess: ChildProcessWithoutNullStreams | null = null; let nativeMacCaptureOutput = ""; let nativeMacCaptureTargetPath: string | null = null; @@ -1132,8 +1209,10 @@ function waitForNativeWindowsCaptureStart(proc: ChildProcessWithoutNullStreams) reject(new Error("Timed out waiting for native Windows capture to start")); }, 12000); - const onOutput = (chunk: Buffer) => { - nativeWindowsCaptureOutput += chunk.toString(); + // Observes only. `attachNativeWindowsCaptureOutputDrain` is the single + // writer of `nativeWindowsCaptureOutput` and is registered first, so the + // chunk that triggers this call is already in the buffer. + const onOutput = () => { if (nativeWindowsCaptureOutput.includes("Recording started")) { cleanup(); resolve(); @@ -1167,59 +1246,70 @@ function waitForNativeWindowsCaptureStart(proc: ChildProcessWithoutNullStreams) }); } -function waitForNativeWindowsCaptureStop(proc: ChildProcessWithoutNullStreams) { - return new Promise((resolve, reject) => { - const timer = setTimeout(() => { - cleanup(); - if (!proc.killed) { - proc.kill(); - } - reject( - new Error( - `Timed out waiting for native Windows capture to stop. Output path: ${ - nativeWindowsCaptureTargetPath ?? "unknown" - }. Output: ${nativeWindowsCaptureOutput.trim()}`, - ), - ); - }, NATIVE_WINDOWS_CAPTURE_STOP_TIMEOUT_MS); - const onOutput = (chunk: Buffer) => { - nativeWindowsCaptureOutput += chunk.toString(); - }; - const onClose = (code: number | null) => { - cleanup(); - const match = nativeWindowsCaptureOutput.match(/Recording stopped\. Output path: (.+)/); - if (match?.[1]) { - resolve(match[1].trim()); - return; - } - if (code === 0 && nativeWindowsCaptureTargetPath) { - resolve(nativeWindowsCaptureTargetPath); - return; - } - reject( - new Error( - nativeWindowsCaptureOutput.trim() || - `Native Windows capture exited with code=${code ?? "unknown"}`, - ), - ); - }; - const onError = (error: Error) => { - cleanup(); - reject(error); - }; - const cleanup = () => { - clearTimeout(timer); - proc.stdout.off("data", onOutput); - proc.stderr.off("data", onOutput); - proc.off("close", onClose); - proc.off("error", onError); - }; +/** + * Keeps reading the helper for as long as it lives. + * + * `waitForNativeWindowsCaptureStart` drops every listener the moment it sees + * "Recording started", so until this existed the whole recording ran unobserved: + * helper warnings and `[stop-timing]` diagnostics were discarded, which is why + * issue #252 had no helper-side evidence from a real app run and had to be + * reproduced by driving the .exe by hand. macOS has had this since it shipped + * (`attachNativeMacCaptureOutputDrain`); Windows never did. + */ +function attachNativeWindowsCaptureOutputDrain(proc: ChildProcessWithoutNullStreams) { + const drain = (chunk: Buffer) => { + nativeWindowsCaptureOutput += chunk.toString(); + }; + const cleanup = () => { + proc.stdout.off("data", drain); + proc.stderr.off("data", drain); + }; - proc.stdout.on("data", onOutput); - proc.stderr.on("data", onOutput); - proc.once("close", onClose); - proc.once("error", onError); - }); + proc.stdout.on("data", drain); + proc.stderr.on("data", drain); + proc.once("close", cleanup); + // An 'error' event with no listener throws, and in the main process that is + // an uncaught exception rather than a rejected promise. Both streams need a + // sink for the whole life of the helper: stdin raises EPIPE when the helper + // died before we wrote to it, and `kill()` on a wedged process re-emits its + // failure on the ChildProcess itself. + // All four emitters, not just stdin: `cleanup` only drops 'data', so an + // abandoned-but-still-alive helper leaves these pipes open with no consumer, + // and an ECONNRESET when the OS finally reaps it would take down the main + // process. + proc.stdin.on("error", (error) => { + console.warn("[native-wgc] helper stdin error:", error); + }); + proc.stdout.on("error", (error) => { + console.warn("[native-wgc] helper stdout error:", error); + }); + proc.stderr.on("error", (error) => { + console.warn("[native-wgc] helper stderr error:", error); + }); + proc.on("error", (error) => { + console.warn("[native-wgc] helper process error:", error); + }); + + // Returned so an abandoned helper can be cut loose. A process that survived + // both kill attempts keeps writing, and `nativeWindowsCaptureOutput` is + // shared with whatever recording starts next. + return cleanup; +} + +/** + * Sends `stop` and closes the command channel behind it. + * + * The helper treats stdin EOF as a stop too, so ending the stream is a free + * second signal if the write itself is lost. + */ +function sendNativeWindowsStopCommand(proc: ChildProcessWithoutNullStreams) { + if (!proc.stdin.writable) { + return false; + } + + proc.stdin.write("stop\n"); + proc.stdin.end(); + return true; } function readNativeWindowsWebcamFormat(output: string) { @@ -2323,6 +2413,8 @@ export function registerIpcHandlers( windowsHide: true, }); nativeWindowsCaptureProcess = proc; + nativeWindowsCaptureDrainCleanup = attachNativeWindowsCaptureOutputDrain(proc); + console.info("[native-wgc] helper spawned", { pid: proc.pid }); await waitForNativeWindowsCaptureStart(proc); const captureStartedAtMs = Date.now(); @@ -2354,16 +2446,8 @@ export function registerIpcHandlers( } catch (error) { console.error("Failed to start native Windows recording:", error); nativeWindowsCaptureProcess?.kill(); - nativeWindowsCaptureProcess = null; - nativeWindowsCaptureTargetPath = null; - nativeWindowsCaptureWebcamTargetPath = null; - nativeWindowsCaptureRecordingId = null; - nativeWindowsCursorOffsetMs = 0; - nativeWindowsCursorCaptureMode = "editable-overlay"; - nativeWindowsCursorRecordingStartMs = 0; - nativeWindowsPauseStartedAtMs = null; - nativeWindowsPauseRanges = []; - nativeWindowsIsPaused = false; + detachNativeWindowsCaptureOutputDrain(); + resetNativeWindowsCaptureState(); await stopCursorRecording(); return { success: false, error: String(error) }; } @@ -2621,12 +2705,84 @@ export function registerIpcHandlers( return { success: false, error: "Native Windows capture is not running." }; } + // Discarding does not need a finalized file, so it must not wait for one. + // Cancel and Restart both route here, and making them sit through the + // full stop handshake meant a wedged helper could not be escaped from at + // all -- the user waited out the timeout only to be told the recording + // failed, then waited it out again to cancel. Linux has always done this; + // Windows never did. + if (discard) { + try { + completeNativeWindowsCursorPauseRange(); + await stopCursorRecording(); + pendingCursorRecordingData = null; + const exited = await terminateNativeWindowsCapture(proc); + if (!exited) { + detachNativeWindowsCaptureOutputDrain(); + } + await removeNativeWindowsCaptureOutputs(preferredPath, preferredWebcamPath); + return { success: true, discarded: true }; + } finally { + // Unconditional. Killing a wedged helper can itself throw, and + // leaving the handle set would make every later recording fail + // with "already running" against a process nobody can stop. + resetNativeWindowsCaptureState(); + if (onRecordingStateChange) { + onRecordingStateChange(false, (selectedSource || { name: "Screen" }).name); + } + } + } + try { completeNativeWindowsCursorPauseRange(); - const stoppedPathPromise = waitForNativeWindowsCaptureStop(proc); - proc.stdin.write("stop\n"); - const stoppedPath = await stoppedPathPromise; - const screenVideoPath = stoppedPath || preferredPath; + const stopPromise = waitForNativeWindowsCaptureStop({ + proc, + targetPath: preferredPath, + readOutput: () => nativeWindowsCaptureOutput, + }); + if (!sendNativeWindowsStopCommand(proc)) { + console.warn("[native-wgc] stop command channel was already closed"); + } + const stopResult = await stopPromise; + if (!stopResult.ok) { + console.error("[native-wgc] stop failed", { + reason: stopResult.reason, + exited: stopResult.exited, + pid: proc.pid, + output: stopResult.message, + }); + if (!stopResult.exited) { + detachNativeWindowsCaptureOutputDrain(); + } + await stopCursorRecording(); + // Same as the discard path. `startCursorRecording` clears this on + // the next recording anyway, so this is not what keeps the samples + // from being written next to someone else's video -- it just stops + // a lost take's telemetry from sitting in memory until then. + pendingCursorRecordingData = null; + // The helper never announced a finalized file, so what is on disk + // is almost certainly an unindexed stub, and leaving those behind + // just accumulates unplayable recordings the user cannot explain. + // Almost: size-gate it, because throwing away a recording to tidy + // up after a failed stop is the worse mistake of the two. + await removeNativeWindowsCaptureOutputs(preferredPath, preferredWebcamPath, { + onlyIfUnusable: true, + }); + // The helper log goes to console/diagnostics above, not into this + // string: it ends up in a toast, and pasting an entire capture log + // into the HUD tells the user nothing they can act on. + return { + success: false, + reason: stopResult.reason, + error: + stopResult.reason === "stop-timeout" + ? "Timed out waiting for native Windows capture to stop. The recording could not be saved." + : stopResult.message.split(/\r?\n/).filter(Boolean).at(-1) || + "Native Windows capture failed.", + }; + } + + const screenVideoPath = stopResult.screenVideoPath || preferredPath; if (!screenVideoPath) { throw new Error("Native Windows capture did not return an output path."); } @@ -2636,15 +2792,6 @@ export function registerIpcHandlers( } else { pendingCursorRecordingData = null; } - if (discard) { - pendingCursorRecordingData = null; - await Promise.all([ - fs.rm(screenVideoPath, { force: true }), - preferredWebcamPath ? fs.rm(preferredWebcamPath, { force: true }) : Promise.resolve(), - fs.rm(`${screenVideoPath}.cursor.json`, { force: true }), - ]); - return { success: true, discarded: true }; - } if (cursorCaptureMode === "editable-overlay") { compactPendingCursorTelemetryPauseRanges(nativeWindowsPauseRanges); @@ -2684,16 +2831,7 @@ export function registerIpcHandlers( await stopCursorRecording(); return { success: false, error: String(error) }; } finally { - nativeWindowsCaptureProcess = null; - nativeWindowsCaptureTargetPath = null; - nativeWindowsCaptureWebcamTargetPath = null; - nativeWindowsCaptureRecordingId = null; - nativeWindowsCursorOffsetMs = 0; - nativeWindowsCursorCaptureMode = "editable-overlay"; - nativeWindowsCursorRecordingStartMs = 0; - nativeWindowsPauseStartedAtMs = null; - nativeWindowsPauseRanges = []; - nativeWindowsIsPaused = false; + resetNativeWindowsCaptureState(); const source = selectedSource || { name: "Screen" }; if (onRecordingStateChange) { onRecordingStateChange(false, source.name); @@ -2788,6 +2926,13 @@ export function registerIpcHandlers( } }); + // On-disk write streams for in-progress recordings, keyed by output file name. + // Chunks append as they arrive so the renderer never buffers the full video (#616). + // Declared here because both the webcam attach below and store-recorded-session + // finalize through the same registry. + const recordingStreams = new RecordingStreamRegistry(); + registerRecordingStreamHandlers(ipcMain, recordingStreams, resolveRecordingOutputPath); + /** * Writes a browser-recorded webcam clip next to a natively-recorded screen * video and rewrites the session manifest to include both. @@ -2817,7 +2962,7 @@ export function registerIpcHandlers( await fs.access(screenVideoPath, fsConstants.R_OK); - if (!payload.webcam?.fileName || !payload.webcam.videoData) { + if (!payload.webcam?.fileName) { return { success: false, error: `Native ${platformLabel} webcam attachment is missing video data.`, @@ -2825,7 +2970,31 @@ export function registerIpcHandlers( } const webcamVideoPath = resolveRecordingOutputPath(payload.webcam.fileName); - await fs.writeFile(webcamVideoPath, Buffer.from(payload.webcam.videoData)); + // A streamed webcam arrives with an empty buffer: its bytes are already on + // disk, so close the stream and keep the file rather than writing it here. + // Nothing multi-gigabyte crosses IPC or gets flattened into one Buffer (#253). + const webcamStreamed = await finalizeRecordingFile( + recordingStreams, + payload.webcam.fileName, + webcamVideoPath, + payload.webcam.videoData, + ); + // Mirrors finalizeRecordingFile's own condition, so this fires exactly when + // it wrote nothing and the session would point at a file that isn't there. + if ( + !webcamStreamed && + !(payload.webcam.videoData && payload.webcam.videoData.byteLength > 0) + ) { + return { + success: false, + error: `Native ${platformLabel} webcam attachment is missing video data.`, + }; + } + // Streamed files lack the WebM Duration header, which the editor needs to + // scale its timeline. Best-effort: a failed repair leaves the clip intact. + if (webcamStreamed && isValidDurationMs(payload.durationMs)) { + await repairRecordingContainer(webcamVideoPath, payload.durationMs); + } const createdAt = typeof payload.recordingId === "number" && Number.isFinite(payload.recordingId) @@ -2892,11 +3061,6 @@ export function registerIpcHandlers( }, ); - // On-disk write streams for in-progress recordings, keyed by output file name. - // Chunks append as they arrive so the renderer never buffers the full video (#616). - const recordingStreams = new RecordingStreamRegistry(); - registerRecordingStreamHandlers(ipcMain, recordingStreams, resolveRecordingOutputPath); - ipcMain.handle("store-recorded-session", async (_, payload: StoreRecordedSessionInput) => { try { return await storeRecordedSessionFiles(payload); diff --git a/electron/media/mediaLinksRegistry.test.ts b/electron/media/mediaLinksRegistry.test.ts index 8d224094b4..bf50b11cfa 100644 --- a/electron/media/mediaLinksRegistry.test.ts +++ b/electron/media/mediaLinksRegistry.test.ts @@ -205,4 +205,97 @@ describe("mediaLinksRegistry", () => { } }); }); + + // `findMediaLinksByFingerprint` is a READ that writes: when the path has + // drifted it refreshes `lastKnownPath` in the background. Not awaiting that + // write is the right call — a lookup should not pay for it — but it means + // nothing is watching the promise, and a rejected promise nobody watches is a + // process-level `unhandledRejection`. Under vitest that fails the run from + // OUTSIDE every test, which is how this was found: 1628 passing tests, a red + // job, and a stack pointing at a temp dir a finished suite had removed. + // + // Both cases below assert the same thing in two ways a real machine produces + // it. Neither asserts that the refresh succeeds — that is precisely what is + // allowed to fail. + describe("the background path refresh", () => { + /** Same bytes at a second path, so a lookup matches by fingerprint and + * then tries to write the new path back. */ + async function registerThenMove(): Promise<{ original: string; moved: string }> { + const original = path.join(tempDir, "moved.webm"); + await writeFileOfSize(original, 900, "m"); + await registerMediaLinks(tempDir, original, { webcamVideoPath: `${original}-cam.webm` }); + const moved = path.join(tempDir, "moved-elsewhere.webm"); + await fs.copyFile(original, moved); + return { original, moved }; + } + + async function withoutUnhandledRejections(fn: () => Promise): Promise { + const rejections: unknown[] = []; + const onRejection = (reason: unknown) => rejections.push(reason); + process.on("unhandledRejection", onRejection); + try { + await fn(); + // Node decides a rejection is unhandled a tick after the microtask + // queue drains, so the assertion needs a real timer, not a flush. + await new Promise((resolve) => setTimeout(resolve, 50)); + } finally { + process.off("unhandledRejection", onRejection); + } + return rejections; + } + + // The failure is INJECTED, not arranged with `chmod 0o555`. A read-only + // directory does not stop a file being created inside it on Windows, so the + // refresh wrote fine there and this case failed on every Windows dev machine + // while staying green in Linux CI. `skipIf(getuid() === 0)` could not see it + // either: `process.getuid` is undefined on Windows, so the guard read as + // "not root" and ran the test anyway. Failing the write itself asserts the + // same thing on every platform, root or not. + it("logs a refresh it cannot write, and still answers the lookup", async () => { + const { original, moved } = await registerThenMove(); + const warned = vi.spyOn(console, "warn").mockImplementation(() => { + // swallowed: the test asserts on it, the suite output does not need it + }); + // Registry readable, the tmp-file write refused: only the write can fail. + const write = vi + .spyOn(fs, "writeFile") + .mockRejectedValue(Object.assign(new Error("permission denied"), { code: "EACCES" })); + try { + const rejections = await withoutUnhandledRejections(async () => { + const resolved = await findMediaLinksByFingerprint(tempDir, moved); + // A refresh that failed is not a lookup that failed. + expect(resolved?.webcamVideoPath).toBe(`${original}-cam.webm`); + }); + expect(rejections).toEqual([]); + expect(warned).toHaveBeenCalled(); + // The refresh was really attempted: without this the case would pass + // just as well with the whole write path deleted. + expect(write).toHaveBeenCalled(); + } finally { + write.mockRestore(); + warned.mockRestore(); + } + }); + + it("survives the directory disappearing while the refresh is queued", async () => { + // The CI shape: a suite's `afterEach` removes its temp dir while a write + // is still in the queue. Whoever wins the race is fine — what must not + // happen is a rejection escaping into the process. + const { moved } = await registerThenMove(); + const warned = vi.spyOn(console, "warn").mockImplementation(() => { + // may or may not fire: the write is allowed to win the race + }); + try { + const rejections = await withoutUnhandledRejections(async () => { + const lookup = findMediaLinksByFingerprint(tempDir, moved); + await fs.rm(tempDir, { recursive: true, force: true }); + await lookup; + }); + expect(rejections).toEqual([]); + } finally { + warned.mockRestore(); + await fs.mkdir(tempDir, { recursive: true }); + } + }); + }); }); diff --git a/electron/media/mediaLinksRegistry.ts b/electron/media/mediaLinksRegistry.ts index 5fafb9a027..c2507d138d 100644 --- a/electron/media/mediaLinksRegistry.ts +++ b/electron/media/mediaLinksRegistry.ts @@ -162,13 +162,21 @@ const writeQueues = new Map>(); function withWriteLock(baseDir: string, fn: () => Promise): Promise { const queue = writeQueues.get(baseDir) ?? Promise.resolve(); const result = queue.then(fn, fn); - writeQueues.set( - baseDir, - result.then( - () => undefined, - () => undefined, - ), + // The tail swallows the outcome so the NEXT writer runs either way; `result` + // keeps the real one for the caller. + const tail = result.then( + () => undefined, + () => undefined, ); + writeQueues.set(baseDir, tail); + // Drop the key once the chain has drained — but only if nothing queued behind + // us in the meantime, or we would strand a tail a later caller is already + // chained on. The map is keyed by an arbitrary directory path, so without + // this it grows for the life of the process; production has one key, a test + // run has one per temp dir. `tail` never rejects, so this cannot leak either. + void tail.then(() => { + if (writeQueues.get(baseDir) === tail) writeQueues.delete(baseDir); + }); return result; } @@ -300,13 +308,24 @@ export async function findMediaLinksByFingerprint( // Path drifted from what's on record — refresh it so the next lookup can // take a cheaper path if one becomes available again. + // + // ponytail: deliberately not awaited — a lookup must not pay for a write it + // does not need — but `void` alone is not fire-and-forget, it is + // fire-and-crash. Nothing was watching this promise, so any failure became an + // unhandled rejection: in the main process that is a process-level event, and + // under vitest it fails the whole run from outside every test (the CI symptom + // was `mkdir ENOENT` when a suite's temp dir was removed while this write was + // still queued, reported after 1628 passing tests). A refresh that cannot + // happen is not worth interrupting anyone over — but it is worth a line. if (match.lastKnownPath !== videoPath) { void updateRegistry(baseDir, (file) => ({ version: 1, entries: file.entries.map((e) => fingerprintsMatch(e.fingerprint, fingerprint) ? { ...e, lastKnownPath: videoPath } : e, ), - })); + })).catch((error) => { + console.warn("[media-links] could not refresh the recorded path:", error); + }); } return { diff --git a/electron/native-bridge/cursor/recording/windowsNativeRecordingSession.ts b/electron/native-bridge/cursor/recording/windowsNativeRecordingSession.ts index c7a057fea3..d2d2f0cc86 100644 --- a/electron/native-bridge/cursor/recording/windowsNativeRecordingSession.ts +++ b/electron/native-bridge/cursor/recording/windowsNativeRecordingSession.ts @@ -186,7 +186,10 @@ export class WindowsNativeRecordingSession implements CursorRecordingSession { } if (payload.asset?.id && !this.assets.has(payload.asset.id)) { - const assetDisplay = screen.getDisplayNearestPoint({ x: payload.x, y: payload.y }); + // payload.x/y are physical screen pixels; `screen` works in DIPs. + const assetDisplay = screen.getDisplayNearestPoint( + screen.screenToDipPoint({ x: payload.x, y: payload.y }), + ); this.assets.set(payload.asset.id, { id: payload.asset.id, platform: "win32", diff --git a/electron/native/wgc-capture/src/cursor-sampler.cpp b/electron/native/wgc-capture/src/cursor-sampler.cpp index 21558c79a4..fc68ebf283 100644 --- a/electron/native/wgc-capture/src/cursor-sampler.cpp +++ b/electron/native/wgc-capture/src/cursor-sampler.cpp @@ -410,6 +410,14 @@ static void runSamplingLoop(int intervalMs, HWND targetWindow, const CLSID& pngC // main // ───────────────────────────────────────────────────────────────────────────── int main(int argc, char* argv[]) { + // Without this the process is DPI-unaware and Win32 virtualises every + // coordinate it hands back — GetCursorInfo().ptScreenPos and GetWindowRect + // come out divided by the primary display's scale factor — while the WGC + // capture and the consumer both work in physical pixels. On any scaled + // display the cursor then lands short of its real position, by more the + // further it is from the origin (getopenscreen/openscreen#272). + SetProcessDpiAwarenessContext(DPI_AWARENESS_CONTEXT_PER_MONITOR_AWARE_V2); + if (argc < 2) { std::cerr << "Usage: cursor-sampler [windowHandle]" << std::endl; return 1; diff --git a/electron/native/wgc-capture/src/main.cpp b/electron/native/wgc-capture/src/main.cpp index f8036c24a0..b7878b2670 100644 --- a/electron/native/wgc-capture/src/main.cpp +++ b/electron/native/wgc-capture/src/main.cpp @@ -58,6 +58,23 @@ struct CaptureControl { std::atomic paused = false; std::mutex mutex; std::condition_variable cv; + // Stop is signalled on its own mutex/CV pair, deliberately not on `mutex` + // (the frame-state lock in main) and not on this struct's `mutex` either. + // + // The frame lock is held across GPU work that cannot be interrupted: the + // WGC frame callback's CopyResource, and the video writer's staging-texture + // Map/readback. Waiting for a stop behind it made shutdown depend on the + // capture pipeline still being healthy -- and a `condition_variable` has to + // re-acquire its mutex before `wait` can return, so one wedged driver call + // left the main thread parked forever without emitting a single + // [stop-timing] line (issue #252). Nothing on this pair touches either + // frame lock, so a stop is always observed no matter what the GPU is doing. + // + // Threads that already hold the frame lock do call requestStop(), so the + // lock order is frame mutex -> stopMutex. Nothing ever takes them the other + // way round. + std::mutex stopMutex; + std::condition_variable stopCv; std::chrono::steady_clock::time_point pauseStartedAt; std::chrono::steady_clock::duration totalPausedDuration{}; // Shared T0 for every stream's timeline (screen video, audio, webcam). @@ -86,8 +103,48 @@ struct CaptureControl { } paused = nextPaused; } + + // The single way to ask for a stop. Every caller goes through here so that + // a future one cannot forget half of the handshake. + void requestStop() { + { + std::scoped_lock lock(stopMutex); + stopRequested = true; + } + // Publishing the flag under `stopMutex` before notifying is what makes + // waitForStop() immune to a wakeup landing between its predicate check + // and its enqueue on the CV. + stopCv.notify_all(); + // The frame pipeline parks on `cv`; wake it too so the video writer + // notices on this pass instead of after its next 100 ms timeout. + cv.notify_all(); + } + + void waitForStop() { + std::unique_lock lock(stopMutex); + // Bounded even though requestStop() publishes under `stopMutex`. This + // is the one wait in the helper that must never be able to hang, and + // re-reading an atomic every 200 ms costs nothing to guarantee it. + while (!stopRequested.load()) { + stopCv.wait_for(lock, std::chrono::milliseconds(200)); + } + } }; +int readEnvInt(const char* name, int fallback) { + char raw[32]{}; + const DWORD length = GetEnvironmentVariableA(name, raw, static_cast(sizeof(raw))); + if (length == 0 || length >= sizeof(raw)) { + return fallback; + } + + try { + return std::stoi(raw); + } catch (...) { + return fallback; + } +} + std::wstring utf8ToWide(const std::string& value) { if (value.empty()) { return {}; @@ -361,9 +418,19 @@ bool parseConfig(const std::string& json, CaptureConfig& config) { void readCaptureCommands(CaptureControl& control, const std::function& onPauseChanged) { std::string line; while (std::getline(std::cin, line)) { + // The comparisons below are exact, so a stray carriage return would + // drop the command in total silence -- the one command this helper + // must never fail to act on. + while (!line.empty() && (line.back() == '\r' || line.back() == '\n')) { + line.pop_back(); + } if (line == "stop" || line == "q" || line == "quit") { - control.stopRequested = true; - control.cv.notify_all(); + // Acknowledged before anything else runs. Issue #252 was reported + // with no way to tell "the helper never saw the stop" apart from + // "the helper saw it and then wedged"; this line settles that in + // every future report. + std::cerr << "[stop-timing] step=command-received elapsed_ms=0" << std::endl; + control.requestStop(); return; } if (line == "pause") { @@ -381,8 +448,10 @@ void readCaptureCommands(CaptureControl& control, const std::function= 1280 * 720 ? 8'000'000 : 4'000'000; if (!webcamEncoder.initialize( @@ -599,8 +680,7 @@ int main(int argc, char* argv[]) { desc.MiscFlags = 0; if (FAILED(session.device()->CreateTexture2D(&desc, nullptr, &latestFrameTexture))) { encodeFailed = true; - control.stopRequested = true; - control.cv.notify_all(); + control.requestStop(); return; } } @@ -711,8 +791,7 @@ int main(int argc, char* argv[]) { hasWebcamSample = webcamEncoder.captureBgraSample(webcamFrame, webcamTimestampHns, webcamSample); if (!hasWebcamSample) { encodeFailed = true; - control.stopRequested = true; - control.cv.notify_all(); + control.requestStop(); break; } lastWebcamTimestampHns = webcamTimestampHns; @@ -724,21 +803,25 @@ int main(int argc, char* argv[]) { } } } + if (testStallReadbackMs > 0) { + std::this_thread::sleep_for(std::chrono::milliseconds(testStallReadbackMs)); + } if (latestFrameTexture) { - // captureVideoSample performs the GPU readback - // (CopyResource/Map) from latestFrameTexture, which must - // stay serialized (via `mutex`) against the WGC - // frame-arrival callback above, which writes new data - // into the same texture on another thread. - hasVideoSample = encoder.captureVideoSample( - latestFrameTexture.Get(), - frameTimestampHns, - !writeSeparateWebcam && webcamFrame.data ? &webcamFrame : nullptr, - videoSample); + if (encoderOptions.useDxgiInput) { + hasVideoSample = encoder.captureDxgiSample( + latestFrameTexture.Get(), + frameTimestampHns, + videoSample); + } else { + hasVideoSample = encoder.captureVideoSample( + latestFrameTexture.Get(), + frameTimestampHns, + !writeSeparateWebcam && webcamFrame.data ? &webcamFrame : nullptr, + videoSample); + } if (!hasVideoSample) { encodeFailed = true; - control.stopRequested = true; - control.cv.notify_all(); + control.requestStop(); break; } lastEncodedVideoTimestampHns = frameTimestampHns; @@ -748,22 +831,22 @@ int main(int argc, char* argv[]) { // Submit the captured samples to their sink writers OUTSIDE // `mutex`. IMFSinkWriter::WriteSample runs the H.264 encode // synchronously and can be slow (especially the software encoder - // fallback used when preferSoftwareEncoder is set). Holding - // `mutex` across it would block the main thread's stop-wait - // (which locks the same mutex to check control.stopRequested) - // for as long as this thread keeps re-acquiring the lock faster - // than the main thread can, hanging the helper indefinitely - // after a stop request (issue #115). + // fallback used when preferSoftwareEncoder is set), and every + // millisecond it holds `mutex` is a millisecond the WGC frame + // callback spends queued behind it dropping frames (issue #115). + // + // This no longer has anything to do with noticing a stop -- that + // moved off `mutex` entirely (see CaptureControl::stopMutex) after + // issue #252 showed the readback below can wedge inside the lock + // regardless of how briefly WriteSample is held. if (hasWebcamSample && !webcamEncoder.submitVideoSample(webcamSample.Get())) { encodeFailed = true; - control.stopRequested = true; - control.cv.notify_all(); + control.requestStop(); break; } if (hasVideoSample && !encoder.submitVideoSample(videoSample.Get())) { encodeFailed = true; - control.stopRequested = true; - control.cv.notify_all(); + control.requestStop(); break; } @@ -800,8 +883,7 @@ int main(int argc, char* argv[]) { [&](const BYTE* data, DWORD byteCount, int64_t timestampHns, int64_t durationHns) { if (!encoder.writeAudio(data, byteCount, timestampHns, durationHns)) { encodeFailed = true; - control.stopRequested = true; - control.cv.notify_all(); + control.requestStop(); return false; } return true; @@ -899,27 +981,33 @@ int main(int argc, char* argv[]) { } }); + // The lock covers the wait and the decision, and nothing else. Every + // teardown call below runs outside it, because session.stop() waits for any + // in-flight WGC callback to finish -- and those callbacks block on this very + // mutex. Tearing down while holding it deadlocks the two against each other, + // on the one path the shutdown watchdog does not cover. + bool firstFrameArrived = false; { std::unique_lock lock(mutex); const bool started = control.cv.wait_for(lock, std::chrono::seconds(10), [&] { return firstFrameWritten.load() || control.stopRequested.load(); }); - if (!started || !firstFrameWritten) { - control.stopRequested = true; - control.cv.notify_all(); - if (stdinThread.joinable()) { - stdinThread.detach(); - } - microphoneCapture.stop(); - loopbackCapture.stop(); - webcamCapture.stop(); - if (audioMixer) { - audioMixer->stop(); - } - session.stop(); - std::cerr << "ERROR: Timed out waiting for first WGC frame" << std::endl; - return 1; + firstFrameArrived = started && firstFrameWritten.load(); + } + if (!firstFrameArrived) { + control.requestStop(); + if (stdinThread.joinable()) { + stdinThread.detach(); } + microphoneCapture.stop(); + loopbackCapture.stop(); + webcamCapture.stop(); + if (audioMixer) { + audioMixer->stop(); + } + session.stop(); + std::cerr << "ERROR: Timed out waiting for first WGC frame" << std::endl; + return 1; } if (audioMixer) { @@ -931,44 +1019,176 @@ int main(int argc, char* argv[]) { std::cout << "{\"event\":\"recording-started\",\"schemaVersion\":2}" << std::endl; std::cout << "Recording started" << std::endl; - { - std::unique_lock lock(mutex); - control.cv.wait(lock, [&] { - return control.stopRequested.load(); - }); - } + control.waitForStop(); const auto stopStart = std::chrono::steady_clock::now(); - auto logStopStep = [&](const char* step) { - const auto ms = std::chrono::duration_cast( + auto stopElapsedMs = [&] { + return std::chrono::duration_cast( std::chrono::steady_clock::now() - stopStart).count(); - std::cerr << "[stop-timing] step=" << step << " elapsed_ms=" << ms << std::endl; }; + // Which step we are inside right now, as opposed to which ones finished. + // Issue #252 was reported with an empty [stop-timing] log precisely because + // the old instrumentation only spoke after a step returned, which is the + // one thing a hung step never does. + std::atomic currentStopStep{"stop-wait"}; + std::atomic shutdownComplete = false; + + // A ceiling on the whole shutdown, and a tighter one per step. + // + // The ceiling exists because the app is waiting on the other end of the + // pipe: NATIVE_WINDOWS_CAPTURE_STOP_TIMEOUT_MS in + // electron/recording/nativeWindowsCaptureStop.ts must stay comfortably + // above this, so the helper always ends itself rather than being killed + // mid-finalize by a parent that ran out of patience. Change one and change + // the other. + // + // The per-step budget is tighter because most steps fail differently: + // stopping threads and closing WGC either completes in milliseconds or is + // wedged inside a driver, and there is no slow-but-working case worth + // waiting for -- waiting is exactly what cost issue #252 a minute of the + // user's time. Finalizing is the opposite. IMFSinkWriter::Finalize drains + // the encoder and writes the MP4 index, which on a long recording through + // the software encoder legitimately takes seconds (issue #34 raised the + // app-side timeout for precisely this), so it gets whatever is left of the + // ceiling rather than a step budget of its own. + const int shutdownBudgetMs = std::max(2000, readEnvInt("OPENSCREEN_WGC_STOP_BUDGET_MS", 50000)); + const int stepBudgetMs = + std::min(shutdownBudgetMs, std::max(1000, readEnvInt("OPENSCREEN_WGC_STEP_BUDGET_MS", 8000))); + std::atomic currentStepDeadlineMs{stepBudgetMs}; + + auto beginStopStep = [&](const char* step, int budgetMs) { + currentStopStep = step; + // Clamped to the ceiling: no sequence of individually-patient steps can + // add up to a shutdown the app has already given up on. + currentStepDeadlineMs = + std::min(stopElapsedMs() + budgetMs, shutdownBudgetMs); + std::cerr << "[stop-timing] step=" << step << " elapsed_ms=" << stopElapsedMs() + << " phase=begin" << std::endl; + }; + // `step= elapsed_ms=` has to stay the leading shape of every line: + // scripts/diagnostic-tool/diagnostic.mjs matches on it, so a trailing + // `phase=` is additive but a leading one would hide the line from the tool. + auto logStopStep = [&](const char* step) { + std::cerr << "[stop-timing] step=" << step << " elapsed_ms=" << stopElapsedMs() << std::endl; + }; + + // None of the steps below can be interrupted: a wedged GPU readback, a + // camera that stops delivering samples, or a WinRT Close() that never + // returns would each leave the helper alive forever, which the app sees as a + // freeze ending in a lost recording (issue #252). Give each step a deadline + // and end the process if one blows through it, naming the step so the next + // bug report starts where this one had to guess. Joinable rather than + // detached: it references main's locals, and its poll interval makes the + // join at the end cost at most one tick. + std::thread shutdownWatchdog([&] { + while (!shutdownComplete.load()) { + // Re-read the flag as part of the same decision as the deadline. + // Checking them separately let a shutdown that completed during the + // sleep still be killed. + if (stopElapsedMs() >= currentStepDeadlineMs.load() && !shutdownComplete.load()) { + const char* step = currentStopStep.load(); + std::cerr << "[stop-timing] step=" << step << " elapsed_ms=" << stopElapsedMs() + << " phase=abandoned" << std::endl; + std::cout << "{\"event\":\"stop-timeout\",\"schemaVersion\":2,\"step\":\"" << step + << "\"}" << std::endl; + std::cout.flush(); + std::cerr.flush(); + // TerminateProcess rather than exit(): exit() runs static + // destructors on this thread, and ~MFEncoder finalizes the sink + // writer behind the very lock a wedged encoder would be holding. + // This thread exists to end the process, not to queue behind the + // hang it is reporting. + TerminateProcess(GetCurrentProcess(), 3); + } + std::this_thread::sleep_for(std::chrono::milliseconds(50)); + } + }); + // Quiesce the frame producer first. Until WGC is closed, callbacks keep + // arriving and keep taking the frame lock, racing the writer's last pass on + // the shared D3D context at exactly the moment we can least afford a stall. + beginStopStep("wgc-quiesce", stepBudgetMs); + // The drain outcome decides the shape of the whole rest of the shutdown: + // a callback that never came back makes wgc-session-close skip the device + // release, so a report that does not say which happened cannot be read. + const bool wgcDrained = session.quiesceCapture(); + std::cerr << "[stop-timing] step=wgc-quiesce elapsed_ms=" << stopElapsedMs() + << " drained=" << (wgcDrained ? "true" : "false") << std::endl; + beginStopStep("microphone", stepBudgetMs); microphoneCapture.stop(); logStopStep("microphone"); + beginStopStep("loopback", stepBudgetMs); loopbackCapture.stop(); logStopStep("loopback"); + beginStopStep("webcam", stepBudgetMs); webcamCapture.stop(); logStopStep("webcam"); + beginStopStep("audio-mixer", stepBudgetMs); if (audioMixer) { audioMixer->stop(); } logStopStep("audio-mixer"); + beginStopStep("video-writer-join", stepBudgetMs); stopVideoWriter(); logStopStep("video-writer-join"); - session.stop(); - logStopStep("wgc-session-close"); - { - std::scoped_lock lock(mutex); - encoder.finalize(); - logStopStep("encoder-finalize"); + // No frame lock here, and the ordering above is what makes that safe rather + // than incidental: stopVideoWriter() joined the only thread that calls into + // the encoder's GPU readback, and audioMixer->stop() joined the only other + // thread that writes to it. MFEncoder's own writerMutex_ deliberately does + // NOT cover copyFrameToBuffer, so finalizing before those joins would race + // the staging texture -- do not reorder these. + beginStopStep("encoder-finalize", shutdownBudgetMs); + const bool screenFinalized = encoder.finalize(); + logStopStep("encoder-finalize"); + if (!screenFinalized) { + std::cerr << "ERROR: Failed to finalize the recording" << std::endl; + } + + // Report success the moment the screen file is durable, not at the end of + // the process's life. Finalize is what writes the MP4 index; everything + // after it is housekeeping that cannot improve that file but can still + // wedge on a bad driver. Announcing here means a watchdog kill during + // teardown costs the user nothing -- the app reads this line and keeps the + // recording. + // + // Gated on the SCREEN finalize alone, and printed before the webcam's. + // The app treats this line as proof the screen file is playable, so a + // failed screen Finalize must not reach it. The webcam is a second, + // optional file and must not be able to veto the first: letting it decide + // meant one bad camera clip discarded a complete capture, and because both + // finalizes share the same ceiling, a slow screen finalize could leave the + // webcam step no budget at all and get the process killed before this line + // ever ran. A webcam that fails below is an error on stderr and a non-zero + // exit -- not a lost recording. + if (!encodeFailed && screenFinalized) { + std::cout << "{\"event\":\"recording-stopped\",\"schemaVersion\":2,\"screenPath\":\"" + << jsonEscape(config.outputPath) << "\""; if (writeSeparateWebcam) { - webcamEncoder.finalize(); - logStopStep("webcam-encoder-finalize"); + std::cout << ",\"webcamPath\":\"" << jsonEscape(config.webcamOutputPath) << "\""; + } + std::cout << "}" << std::endl; + std::cout << "Recording stopped. Output path: " << config.outputPath << std::endl; + } + + bool webcamFinalized = true; + if (writeSeparateWebcam) { + beginStopStep("webcam-encoder-finalize", shutdownBudgetMs); + webcamFinalized = webcamEncoder.finalize(); + logStopStep("webcam-encoder-finalize"); + if (!webcamFinalized) { + std::cerr << "ERROR: Failed to finalize the webcam recording" << std::endl; } } + // Releasing the device goes last: by now no thread can still be holding the + // D3D context. + beginStopStep("wgc-session-close", stepBudgetMs); + session.stop(); + logStopStep("wgc-session-close"); + + shutdownComplete = true; + shutdownWatchdog.join(); + if (stdinThread.joinable()) { stdinThread.detach(); } @@ -977,13 +1197,9 @@ int main(int argc, char* argv[]) { std::cerr << "ERROR: Failed to encode WGC frame" << std::endl; return 1; } - - std::cout << "{\"event\":\"recording-stopped\",\"schemaVersion\":2,\"screenPath\":\"" - << jsonEscape(config.outputPath) << "\""; - if (writeSeparateWebcam) { - std::cout << ",\"webcamPath\":\"" << jsonEscape(config.webcamOutputPath) << "\""; + if (!screenFinalized || !webcamFinalized) { + return 1; } - std::cout << "}" << std::endl; - std::cout << "Recording stopped. Output path: " << config.outputPath << std::endl; + return 0; } diff --git a/electron/native/wgc-capture/src/mf_encoder.cpp b/electron/native/wgc-capture/src/mf_encoder.cpp index 60f82e9f55..383f441663 100644 --- a/electron/native/wgc-capture/src/mf_encoder.cpp +++ b/electron/native/wgc-capture/src/mf_encoder.cpp @@ -2,6 +2,8 @@ #include "audio_sample_utils.h" +#include +#include #include #include #include @@ -134,6 +136,7 @@ enum class SinkWriterCreateStage { SoftwareEncoderRegistration, CreateAttributes, DisableHardwareTransforms, + ConfigureDxgiManager, CreateSinkWriter, }; @@ -179,6 +182,7 @@ HRESULT ensureSoftwareH264EncoderRegisteredForProcess() { HRESULT createSinkWriterFromUrl( const std::wstring& outputPath, bool forceSoftwareEncoder, + IMFDXGIDeviceManager* dxgiDeviceManager, bool injectDefaultSinkWriterFailureOnce, bool& injectedDefaultSinkWriterFailure, Microsoft::WRL::ComPtr& sinkWriter, @@ -218,6 +222,27 @@ HRESULT createSinkWriterFromUrl( failedStage = SinkWriterCreateStage::DisableHardwareTransforms; return hr; } + } else if (dxgiDeviceManager != nullptr) { + HRESULT hr = MFCreateAttributes(&attributes, 3); + if (FAILED(hr)) { + std::cerr << "ERROR: MFCreateAttributes(DXGI sink writer) failed (hr=0x" + << std::hex << hr << std::dec << ")" << std::endl; + failedStage = SinkWriterCreateStage::CreateAttributes; + return hr; + } + hr = attributes->SetUINT32(MF_READWRITE_ENABLE_HARDWARE_TRANSFORMS, TRUE); + if (FAILED(hr)) { + failedStage = SinkWriterCreateStage::ConfigureDxgiManager; + return hr; + } + hr = attributes->SetUnknown(MF_SINK_WRITER_D3D_MANAGER, dxgiDeviceManager); + if (FAILED(hr)) { + std::cerr << "ERROR: Set MF_SINK_WRITER_D3D_MANAGER failed (hr=0x" + << std::hex << hr << std::dec << ")" << std::endl; + failedStage = SinkWriterCreateStage::ConfigureDxgiManager; + return hr; + } + attributes->SetUINT32(MF_LOW_LATENCY, TRUE); } failedStage = SinkWriterCreateStage::CreateSinkWriter; @@ -347,12 +372,34 @@ bool MFEncoder::initialize( fps_ = std::max(1, fps); device_ = device; context_ = context; + captureDevice_ = device; + captureContext_ = context; + useDxgiInput_ = options.useDxgiInput; videoEncoderSelection_ = kVideoEncoderSelectionDefault; if (!succeeded(MFStartup(MF_VERSION), "MFStartup")) { return false; } + if (useDxgiInput_) { + if (!initializeDxgiEncodingDevice()) { + return false; + } + if (!succeeded( + MFCreateDXGIDeviceManager(&dxgiResetToken_, &dxgiDeviceManager_), + "MFCreateDXGIDeviceManager")) { + return false; + } + if (!succeeded( + dxgiDeviceManager_->ResetDevice(device_.Get(), dxgiResetToken_), + "IMFDXGIDeviceManager::ResetDevice")) { + return false; + } + if (!initializeVideoProcessor()) { + return false; + } + } + Microsoft::WRL::ComPtr outputType; if (!succeeded(MFCreateMediaType(&outputType), "MFCreateMediaType(output)")) { return false; @@ -370,13 +417,49 @@ bool MFEncoder::initialize( return false; } inputType->SetGUID(MF_MT_MAJOR_TYPE, MFMediaType_Video); - inputType->SetGUID(MF_MT_SUBTYPE, MFVideoFormat_RGB32); + inputType->SetGUID( + MF_MT_SUBTYPE, + useDxgiInput_ ? MFVideoFormat_NV12 : MFVideoFormat_RGB32); inputType->SetUINT32(MF_MT_INTERLACE_MODE, MFVideoInterlace_Progressive); - inputType->SetUINT32(MF_MT_DEFAULT_STRIDE, static_cast(width_ * 4)); + if (!useDxgiInput_) { + inputType->SetUINT32(MF_MT_DEFAULT_STRIDE, static_cast(width_ * 4)); + } setFrameSize(inputType.Get(), static_cast(width_), static_cast(height_)); setFrameRate(inputType.Get(), static_cast(fps_)); setPixelAspectRatio(inputType.Get()); + if (useDxgiInput_) { + if (!succeeded( + MFCreateVideoSampleAllocatorEx( + __uuidof(IMFVideoSampleAllocatorEx), + reinterpret_cast(videoSampleAllocator_.GetAddressOf())), + "MFCreateVideoSampleAllocatorEx")) { + return false; + } + if (!succeeded( + videoSampleAllocator_->SetDirectXManager(dxgiDeviceManager_.Get()), + "IMFVideoSampleAllocator::SetDirectXManager")) { + return false; + } + Microsoft::WRL::ComPtr allocatorAttributes; + if (!succeeded(MFCreateAttributes(&allocatorAttributes, 2), "MFCreateAttributes(allocator)")) { + return false; + } + allocatorAttributes->SetUINT32(MF_SA_D3D11_USAGE, D3D11_USAGE_DEFAULT); + allocatorAttributes->SetUINT32( + MF_SA_D3D11_BINDFLAGS, + D3D11_BIND_RENDER_TARGET | D3D11_BIND_SHADER_RESOURCE); + if (!succeeded( + videoSampleAllocator_->InitializeSampleAllocatorEx( + 4, + 30, + allocatorAttributes.Get(), + inputType.Get()), + "IMFVideoSampleAllocatorEx::InitializeSampleAllocatorEx")) { + return false; + } + } + bool injectedDefaultSinkWriterFailure = false; auto resetSinkWriterAttempt = [&]() { @@ -397,6 +480,7 @@ bool MFEncoder::initialize( const HRESULT sinkWriterHr = createSinkWriterFromUrl( outputPath, forceSoftwareEncoder, + forceSoftwareEncoder ? nullptr : dxgiDeviceManager_.Get(), options.injectDefaultSinkWriterFailureOnce, injectedDefaultSinkWriterFailure, sinkWriter_, @@ -439,6 +523,11 @@ bool MFEncoder::initialize( }; if (options.preferSoftwareEncoder) { + if (useDxgiInput_) { + std::cerr << "ERROR: DXGI input requires a hardware Media Foundation encoder" + << std::endl; + return false; + } return configureSinkWriterAttempt( true, kVideoEncoderSelectionSoftwarePreferred, @@ -449,6 +538,11 @@ bool MFEncoder::initialize( return true; } + if (useDxgiInput_) { + std::cerr << "ERROR: Hardware DXGI H.264 encoder setup failed" << std::endl; + return false; + } + std::cerr << "WARNING: Default Media Foundation H.264 encoder setup failed; " << "retrying with the Microsoft software H.264 encoder." @@ -603,6 +697,302 @@ bool MFEncoder::copyBgraFrameToBuffer(const BgraFrameView& frame, BYTE* destinat return true; } +bool MFEncoder::initializeDxgiEncodingDevice() { + Microsoft::WRL::ComPtr captureDxgiDevice; + if (!succeeded(captureDevice_.As(&captureDxgiDevice), "Query capture IDXGIDevice")) { + return false; + } + Microsoft::WRL::ComPtr adapter; + if (!succeeded(captureDxgiDevice->GetAdapter(&adapter), "Get capture DXGI adapter")) { + return false; + } + + const UINT flags = D3D11_CREATE_DEVICE_BGRA_SUPPORT | D3D11_CREATE_DEVICE_VIDEO_SUPPORT; + D3D_FEATURE_LEVEL featureLevels[] = { + D3D_FEATURE_LEVEL_11_1, + D3D_FEATURE_LEVEL_11_0, + D3D_FEATURE_LEVEL_10_1, + D3D_FEATURE_LEVEL_10_0, + }; + D3D_FEATURE_LEVEL featureLevel{}; + if (!succeeded( + D3D11CreateDevice( + adapter.Get(), + D3D_DRIVER_TYPE_UNKNOWN, + nullptr, + flags, + featureLevels, + ARRAYSIZE(featureLevels), + D3D11_SDK_VERSION, + &device_, + &featureLevel, + &context_), + "D3D11CreateDevice(encoder)")) { + return false; + } + + Microsoft::WRL::ComPtr multithread; + if (!succeeded(context_.As(&multithread), "Query encoder ID3D10Multithread")) { + return false; + } + multithread->SetMultithreadProtected(TRUE); + return true; +} + +bool MFEncoder::initializeVideoProcessor() { + if (!succeeded(device_.As(&videoDevice_), "Query ID3D11VideoDevice")) { + return false; + } + if (!succeeded(context_.As(&videoContext_), "Query ID3D11VideoContext")) { + return false; + } + + D3D11_VIDEO_PROCESSOR_CONTENT_DESC contentDesc{}; + contentDesc.InputFrameFormat = D3D11_VIDEO_FRAME_FORMAT_PROGRESSIVE; + contentDesc.InputFrameRate = {static_cast(fps_), 1}; + contentDesc.InputWidth = static_cast(width_); + contentDesc.InputHeight = static_cast(height_); + contentDesc.OutputFrameRate = {static_cast(fps_), 1}; + contentDesc.OutputWidth = static_cast(width_); + contentDesc.OutputHeight = static_cast(height_); + contentDesc.Usage = D3D11_VIDEO_USAGE_PLAYBACK_NORMAL; + + if (!succeeded( + videoDevice_->CreateVideoProcessorEnumerator( + &contentDesc, + &videoProcessorEnumerator_), + "CreateVideoProcessorEnumerator")) { + return false; + } + + UINT nv12Support = 0; + if (!succeeded( + videoProcessorEnumerator_->CheckVideoProcessorFormat( + DXGI_FORMAT_NV12, + &nv12Support), + "CheckVideoProcessorFormat(NV12)")) { + return false; + } + if ((nv12Support & D3D11_VIDEO_PROCESSOR_FORMAT_SUPPORT_OUTPUT) == 0) { + std::cerr << "ERROR: D3D11 video processor does not support NV12 output" << std::endl; + return false; + } + + return succeeded( + videoDevice_->CreateVideoProcessor( + videoProcessorEnumerator_.Get(), + 0, + &videoProcessor_), + "CreateVideoProcessor"); +} + +bool MFEncoder::convertBgraTextureToNv12( + ID3D11Texture2D* texture, + ID3D11Texture2D* outputTexture) { + + if (!captureBridgeTexture_) { + D3D11_TEXTURE2D_DESC bridgeDesc{}; + texture->GetDesc(&bridgeDesc); + bridgeDesc.BindFlags = D3D11_BIND_SHADER_RESOURCE | D3D11_BIND_RENDER_TARGET; + bridgeDesc.CPUAccessFlags = 0; + bridgeDesc.Usage = D3D11_USAGE_DEFAULT; + bridgeDesc.MiscFlags = D3D11_RESOURCE_MISC_SHARED_KEYEDMUTEX; + if (!succeeded( + captureDevice_->CreateTexture2D(&bridgeDesc, nullptr, &captureBridgeTexture_), + "CreateTexture2D(capture bridge)")) { + return false; + } + if (!succeeded(captureBridgeTexture_.As(&captureBridgeMutex_), "Query capture bridge mutex")) { + return false; + } + + Microsoft::WRL::ComPtr bridgeResource; + if (!succeeded(captureBridgeTexture_.As(&bridgeResource), "Query capture bridge resource")) { + return false; + } + HANDLE sharedHandle = nullptr; + if (!succeeded(bridgeResource->GetSharedHandle(&sharedHandle), "Get capture bridge handle")) { + return false; + } + if (!succeeded( + device_->OpenSharedResource( + sharedHandle, + __uuidof(ID3D11Texture2D), + reinterpret_cast(encoderBridgeTexture_.GetAddressOf())), + "Open encoder bridge texture")) { + return false; + } + if (!succeeded(encoderBridgeTexture_.As(&encoderBridgeMutex_), "Query encoder bridge mutex")) { + return false; + } + } + + if (!succeeded(captureBridgeMutex_->AcquireSync(0, 5000), "Acquire capture bridge")) { + return false; + } + captureContext_->CopyResource(captureBridgeTexture_.Get(), texture); + if (!succeeded(captureBridgeMutex_->ReleaseSync(1), "Release capture bridge")) { + return false; + } + if (!succeeded(encoderBridgeMutex_->AcquireSync(1, 5000), "Acquire encoder bridge")) { + return false; + } + const auto releaseEncoderBridge = [&]() { + return succeeded(encoderBridgeMutex_->ReleaseSync(0), "Release encoder bridge"); + }; + + D3D11_VIDEO_PROCESSOR_INPUT_VIEW_DESC inputViewDesc{}; + inputViewDesc.FourCC = 0; + inputViewDesc.ViewDimension = D3D11_VPIV_DIMENSION_TEXTURE2D; + inputViewDesc.Texture2D.MipSlice = 0; + inputViewDesc.Texture2D.ArraySlice = 0; + + Microsoft::WRL::ComPtr inputView; + if (!succeeded( + videoDevice_->CreateVideoProcessorInputView( + encoderBridgeTexture_.Get(), + videoProcessorEnumerator_.Get(), + &inputViewDesc, + &inputView), + "CreateVideoProcessorInputView")) { + releaseEncoderBridge(); + return false; + } + + D3D11_VIDEO_PROCESSOR_OUTPUT_VIEW_DESC outputViewDesc{}; + outputViewDesc.ViewDimension = D3D11_VPOV_DIMENSION_TEXTURE2D; + outputViewDesc.Texture2D.MipSlice = 0; + + Microsoft::WRL::ComPtr outputView; + if (!succeeded( + videoDevice_->CreateVideoProcessorOutputView( + outputTexture, + videoProcessorEnumerator_.Get(), + &outputViewDesc, + &outputView), + "CreateVideoProcessorOutputView")) { + releaseEncoderBridge(); + return false; + } + + const RECT sourceRect{0, 0, width_, height_}; + const RECT destinationRect{0, 0, width_, height_}; + videoContext_->VideoProcessorSetOutputTargetRect( + videoProcessor_.Get(), + TRUE, + &destinationRect); + videoContext_->VideoProcessorSetStreamFrameFormat( + videoProcessor_.Get(), + 0, + D3D11_VIDEO_FRAME_FORMAT_PROGRESSIVE); + videoContext_->VideoProcessorSetStreamSourceRect( + videoProcessor_.Get(), + 0, + TRUE, + &sourceRect); + videoContext_->VideoProcessorSetStreamDestRect( + videoProcessor_.Get(), + 0, + TRUE, + &destinationRect); + + D3D11_VIDEO_PROCESSOR_STREAM stream{}; + stream.Enable = TRUE; + stream.OutputIndex = 0; + stream.InputFrameOrField = 0; + stream.PastFrames = 0; + stream.FutureFrames = 0; + stream.pInputSurface = inputView.Get(); + + const bool converted = succeeded( + videoContext_->VideoProcessorBlt( + videoProcessor_.Get(), + outputView.Get(), + 0, + 1, + &stream), + "VideoProcessorBlt"); + const bool released = releaseEncoderBridge(); + return converted && released; +} + +bool MFEncoder::captureDxgiSample( + ID3D11Texture2D* texture, + int64_t timestampHns, + Microsoft::WRL::ComPtr& outSample) { + outSample.Reset(); + if (!texture) { + return false; + } + + D3D11_TEXTURE2D_DESC desc{}; + texture->GetDesc(&desc); + if (desc.Width != static_cast(width_) || + desc.Height != static_cast(height_) || + desc.Format != DXGI_FORMAT_B8G8R8A8_UNORM) { + std::cerr << "ERROR: Unexpected WGC DXGI texture format or dimensions" << std::endl; + return false; + } + + const int64_t sampleDuration = 10'000'000LL / fps_; + int64_t sampleTime = 0; + { + std::scoped_lock writerLock(writerMutex_); + if (!sinkWriter_ || finalized_) { + return false; + } + if (firstTimestampHns_ < 0) { + firstTimestampHns_ = timestampHns; + } + sampleTime = timestampHns - firstTimestampHns_; + if (sampleTime <= lastTimestampHns_) { + sampleTime = lastTimestampHns_ + sampleDuration; + } + lastTimestampHns_ = sampleTime; + } + + Microsoft::WRL::ComPtr sample; + if (!succeeded(videoSampleAllocator_->AllocateSample(&sample), "Allocate DXGI video sample")) { + return false; + } + + Microsoft::WRL::ComPtr buffer; + if (!succeeded(sample->GetBufferByIndex(0, &buffer), "Get DXGI video buffer")) { + return false; + } + + Microsoft::WRL::ComPtr dxgiBuffer; + if (!succeeded(buffer.As(&dxgiBuffer), "Query IMFDXGIBuffer")) { + return false; + } + Microsoft::WRL::ComPtr nv12Texture; + if (!succeeded( + dxgiBuffer->GetResource( + __uuidof(ID3D11Texture2D), + reinterpret_cast(nv12Texture.GetAddressOf())), + "IMFDXGIBuffer::GetResource")) { + return false; + } + if (!convertBgraTextureToNv12(texture, nv12Texture.Get())) { + return false; + } + + DWORD maximumLength = 0; + if (!succeeded(buffer->GetMaxLength(&maximumLength), "IMFMediaBuffer::GetMaxLength(DXGI)")) { + return false; + } + if (!succeeded( + buffer->SetCurrentLength(maximumLength), + "IMFMediaBuffer::SetCurrentLength(DXGI)")) { + return false; + } + + sample->SetSampleTime(sampleTime); + sample->SetSampleDuration(sampleDuration); + outSample = sample; + return true; +} + bool MFEncoder::captureVideoSample( ID3D11Texture2D* texture, int64_t timestampHns, diff --git a/electron/native/wgc-capture/src/mf_encoder.h b/electron/native/wgc-capture/src/mf_encoder.h index e5fbd74c88..53995f90fe 100644 --- a/electron/native/wgc-capture/src/mf_encoder.h +++ b/electron/native/wgc-capture/src/mf_encoder.h @@ -29,6 +29,7 @@ struct AudioInputFormat { struct MFEncoderOptions { bool preferSoftwareEncoder = false; bool injectDefaultSinkWriterFailureOnce = false; + bool useDxgiInput = false; }; constexpr const char* kVideoEncoderSelectionDefault = "default"; @@ -67,6 +68,10 @@ class MFEncoder { int64_t timestampHns, const BgraFrameView* webcamFrame, Microsoft::WRL::ComPtr& outSample); + bool captureDxgiSample( + ID3D11Texture2D* texture, + int64_t timestampHns, + Microsoft::WRL::ComPtr& outSample); bool captureBgraSample( const BgraFrameView& frame, int64_t timestampHns, @@ -77,6 +82,11 @@ class MFEncoder { const char* videoEncoderSelection() const; private: + bool initializeDxgiEncodingDevice(); + bool initializeVideoProcessor(); + bool convertBgraTextureToNv12( + ID3D11Texture2D* texture, + ID3D11Texture2D* outputTexture); bool ensureStagingTexture(ID3D11Texture2D* texture); bool copyFrameToBuffer( ID3D11Texture2D* texture, @@ -89,7 +99,20 @@ class MFEncoder { Microsoft::WRL::ComPtr sinkWriter_; Microsoft::WRL::ComPtr device_; Microsoft::WRL::ComPtr context_; + Microsoft::WRL::ComPtr captureDevice_; + Microsoft::WRL::ComPtr captureContext_; + Microsoft::WRL::ComPtr captureBridgeTexture_; + Microsoft::WRL::ComPtr captureBridgeMutex_; + Microsoft::WRL::ComPtr encoderBridgeTexture_; + Microsoft::WRL::ComPtr encoderBridgeMutex_; Microsoft::WRL::ComPtr stagingTexture_; + Microsoft::WRL::ComPtr dxgiDeviceManager_; + Microsoft::WRL::ComPtr videoSampleAllocator_; + Microsoft::WRL::ComPtr videoDevice_; + Microsoft::WRL::ComPtr videoContext_; + Microsoft::WRL::ComPtr videoProcessorEnumerator_; + Microsoft::WRL::ComPtr videoProcessor_; + UINT dxgiResetToken_ = 0; std::mutex writerMutex_; DWORD videoStreamIndex_ = 0; DWORD audioStreamIndex_ = 0; @@ -100,5 +123,6 @@ class MFEncoder { int64_t firstTimestampHns_ = -1; int64_t lastTimestampHns_ = -1; bool finalized_ = false; + bool useDxgiInput_ = false; const char* videoEncoderSelection_ = kVideoEncoderSelectionDefault; }; diff --git a/electron/native/wgc-capture/src/wgc_session.cpp b/electron/native/wgc-capture/src/wgc_session.cpp index 89f0b55fe0..76649a9902 100644 --- a/electron/native/wgc-capture/src/wgc_session.cpp +++ b/electron/native/wgc-capture/src/wgc_session.cpp @@ -1,11 +1,14 @@ #include "wgc_session.h" #include +#include #include #include #include +#include #include +#include namespace wf = winrt::Windows::Foundation; namespace wgcap = winrt::Windows::Graphics::Capture; @@ -61,7 +64,7 @@ WgcSession::~WgcSession() { } bool WgcSession::createD3DDevice() { - UINT flags = D3D11_CREATE_DEVICE_BGRA_SUPPORT; + UINT flags = D3D11_CREATE_DEVICE_BGRA_SUPPORT | D3D11_CREATE_DEVICE_VIDEO_SUPPORT; #if defined(_DEBUG) flags |= D3D11_CREATE_DEVICE_DEBUG; #endif @@ -107,6 +110,12 @@ bool WgcSession::createD3DDevice() { return false; } + Microsoft::WRL::ComPtr multithread; + if (!succeeded(d3dContext_.As(&multithread), "Query ID3D10Multithread")) { + return false; + } + multithread->SetMultithreadProtected(TRUE); + Microsoft::WRL::ComPtr dxgiDevice; if (!succeeded(d3dDevice_.As(&dxgiDevice), "Query IDXGIDevice")) { return false; @@ -273,23 +282,81 @@ bool WgcSession::start() { return true; } -void WgcSession::stop() { - if (framePool_) { - framePool_.FrameArrived(frameArrivedToken_); +bool WgcSession::quiesceCapture(int drainTimeoutMs) { + if (quiesced_) { + return callbacksInFlight_.load() == 0; } - if (session_) { - session_.Close(); - session_ = nullptr; + quiesced_ = true; + + try { + if (framePool_) { + framePool_.FrameArrived(frameArrivedToken_); + } + } catch (...) { + // Revoking a handler the runtime has already torn down is not a reason + // to abandon the rest of the shutdown. } - if (framePool_) { - framePool_.Close(); - framePool_ = nullptr; + { + // Drop the callback under the same lock onFrameArrived copies it under, + // so any handler that has not read it yet becomes a no-op... + std::scoped_lock lock(callbackMutex_); + frameCallback_ = nullptr; + } + // ...then wait out the handlers that already read it. Without this, stop() + // could Reset() the D3D context while a callback was still issuing + // CopyResource on it. + // + // Bounded, because a callback wedged inside the display driver never + // finishes and this runs on paths that have no watchdog above them (the + // first-frame timeout in main.cpp). Giving up is reported rather than + // papered over: the caller keeps the device alive instead, which leaks it + // until the process exits and is the lesser of the two failures. + const auto drainDeadline = + std::chrono::steady_clock::now() + std::chrono::milliseconds(drainTimeoutMs); + while (callbacksInFlight_.load() > 0) { + if (std::chrono::steady_clock::now() >= drainDeadline) { + std::cerr << "WARNING: A WGC frame callback did not finish; leaving the device alive" + << std::endl; + return false; + } + std::this_thread::sleep_for(std::chrono::milliseconds(1)); + } + + // Close() is a C++/WinRT projection and throws hresult_error on failure. + // Letting that escape would take the process down through std::terminate + // mid-shutdown, discarding a recording that is already finalized by the time + // this runs. There is nothing to do about a capture session that refuses to + // close except stop caring about it. + try { + if (session_) { + session_.Close(); + } + if (framePool_) { + framePool_.Close(); + } + } catch (winrt::hresult_error const& error) { + std::cerr << "WARNING: Failed to close the WGC session (hr=0x" << std::hex + << static_cast(error.code()) << std::dec << ")" << std::endl; + } catch (...) { + std::cerr << "WARNING: Failed to close the WGC session" << std::endl; + } + session_ = nullptr; + framePool_ = nullptr; + started_ = false; + return true; +} + +void WgcSession::stop() { + if (!quiesceCapture()) { + // A callback is still inside the driver holding this context. Releasing + // it now would pull the device out from under a live CopyResource, so + // leak it and let process exit reclaim it. + return; } item_ = nullptr; winrtDevice_ = nullptr; d3dContext_.Reset(); d3dDevice_.Reset(); - started_ = false; } void WgcSession::onFrameArrived( @@ -312,10 +379,30 @@ void WgcSession::onFrameArrived( { std::scoped_lock lock(callbackMutex_); callback = frameCallback_; + if (callback) { + // Counted under the same lock quiesceCapture() clears the callback + // under, so once it has cleared it no new callback can start and + // the counter it then drains cannot go back up. + callbacksInFlight_ += 1; + } } if (callback) { + // Scoped rather than a bare decrement after the call, for two reasons: + // a callback that left by exception would otherwise strand + // quiesceCapture()'s drain forever, and the guard has to outlive + // frame.Close() -- dropping the count first would let quiesce return and + // close the frame pool while this handler is still closing a frame that + // pool owns. + struct InFlightGuard { + std::atomic& counter; + ~InFlightGuard() { + counter -= 1; + } + } guard{callbacksInFlight_}; callback(texture.Get(), timeSpanToHns(frame.SystemRelativeTime())); + frame.Close(); + return; } frame.Close(); } diff --git a/electron/native/wgc-capture/src/wgc_session.h b/electron/native/wgc-capture/src/wgc_session.h index 43de21a87a..33aba29b41 100644 --- a/electron/native/wgc-capture/src/wgc_session.h +++ b/electron/native/wgc-capture/src/wgc_session.h @@ -9,6 +9,7 @@ #include #include +#include #include #include @@ -26,6 +27,14 @@ class WgcSession { bool initialize(HWND window, int fps, bool captureCursor); void setFrameCallback(FrameCallback callback); bool start(); + // Stops frame delivery and waits out any callback already running, without + // touching the D3D device. Split out of stop() so a caller can quiesce the + // producer early in a shutdown and only release the device once nothing can + // still be using it. Idempotent; stop() calls it. + // + // Returns false if a callback was still running when `drainTimeoutMs` + // expired -- releasing the device after that is unsafe, so stop() skips it. + bool quiesceCapture(int drainTimeoutMs = 5000); void stop(); int captureWidth() const; @@ -51,6 +60,8 @@ class WgcSession { winrt::event_token frameArrivedToken_{}; FrameCallback frameCallback_; std::mutex callbackMutex_; + std::atomic callbacksInFlight_ = 0; + bool quiesced_ = false; int width_ = 0; int height_ = 0; int fps_ = 60; diff --git a/electron/preload.ts b/electron/preload.ts index 04b2427aec..8e018ed8e1 100644 --- a/electron/preload.ts +++ b/electron/preload.ts @@ -209,6 +209,7 @@ contextBridge.exposeInMainWorld("electronAPI", { recordingId: number; webcam: { fileName: string; videoData: ArrayBuffer }; cursorCaptureMode?: import("../src/lib/recordingSession").CursorCaptureMode; + durationMs?: number; webcamOffsetMs?: number; }) => { return ipcRenderer.invoke("attach-native-linux-webcam-recording", payload); @@ -242,6 +243,7 @@ contextBridge.exposeInMainWorld("electronAPI", { recordingId: number; webcam: { fileName: string; videoData: ArrayBuffer }; cursorCaptureMode?: import("../src/lib/recordingSession").CursorCaptureMode; + durationMs?: number; webcamOffsetMs?: number; }) => { return ipcRenderer.invoke("attach-native-mac-webcam-recording", payload); diff --git a/electron/recording/nativeWindowsCaptureStop.test.ts b/electron/recording/nativeWindowsCaptureStop.test.ts new file mode 100644 index 0000000000..7ba34317c5 --- /dev/null +++ b/electron/recording/nativeWindowsCaptureStop.test.ts @@ -0,0 +1,363 @@ +import type { ChildProcessWithoutNullStreams } from "node:child_process"; +import { EventEmitter } from "node:events"; +import { PassThrough, Writable } from "node:stream"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { + readStoppedPath, + terminateNativeWindowsCapture, + waitForNativeWindowsCaptureStop, +} from "./nativeWindowsCaptureStop"; + +/** + * Stands in for wgc-capture.exe. `exitCode`/`signalCode` are real properties on + * `ChildProcess` and the code under test reads them to decide whether waiting + * for 'close' can still pay off, so the fake has to model them honestly. + */ +class FakeHelper extends EventEmitter { + stdout = new PassThrough(); + stderr = new PassThrough(); + stdin: Writable; + exitCode: number | null = null; + signalCode: string | null = null; + pid: number | undefined = 4242; + killCalls = 0; + /** When false, kill() is recorded but the process refuses to die. */ + diesOnKill = true; + + constructor() { + super(); + this.stdin = new Writable({ + write(_chunk, _encoding, callback) { + callback(); + }, + }); + } + + kill() { + this.killCalls += 1; + if (this.diesOnKill) { + this.exit(1); + } + return true; + } + + exit(code: number) { + this.exitCode = code; + this.emit("close", code); + } +} + +function asProc(helper: FakeHelper) { + return helper as unknown as ChildProcessWithoutNullStreams; +} + +let helper: FakeHelper; + +beforeEach(() => { + vi.useFakeTimers(); + helper = new FakeHelper(); +}); + +afterEach(() => { + vi.useRealTimers(); + vi.restoreAllMocks(); +}); + +describe("readStoppedPath", () => { + it("reads the finalized path out of the helper log", () => { + expect(readStoppedPath("Recording stopped. Output path: C:\\rec\\a.mp4\n")).toBe( + "C:\\rec\\a.mp4", + ); + }); + + it("is null when the helper never reported a finalized file", () => { + expect(readStoppedPath("Recording started\n[stop-timing] step=microphone elapsed_ms=0\n")).toBe( + null, + ); + }); +}); + +describe("waitForNativeWindowsCaptureStop", () => { + it("resolves with the path the helper reported", async () => { + let output = "Recording started\n"; + const pending = waitForNativeWindowsCaptureStop({ + proc: asProc(helper), + targetPath: "C:\\rec\\a.mp4", + readOutput: () => output, + }); + + output += "Recording stopped. Output path: C:\\rec\\a.mp4\n"; + helper.exit(0); + + await expect(pending).resolves.toEqual({ ok: true, screenVideoPath: "C:\\rec\\a.mp4" }); + }); + + it("falls back to the requested path when the helper exits 0 quietly", async () => { + const pending = waitForNativeWindowsCaptureStop({ + proc: asProc(helper), + targetPath: "C:\\rec\\a.mp4", + readOutput: () => "", + }); + + helper.exit(0); + + await expect(pending).resolves.toEqual({ ok: true, screenVideoPath: "C:\\rec\\a.mp4" }); + }); + + /** + * The helper can be gone before the stop IPC even runs -- it force-exits on + * its own shutdown watchdog, and a lost D3D device kills it outright. Node + * never re-emits 'close' for a process that already exited, so waiting for + * one burned the entire stop timeout and reported it as a hang (issue #252). + */ + it("settles immediately when the helper has already exited", async () => { + helper.exitCode = 0; + + const result = await waitForNativeWindowsCaptureStop({ + proc: asProc(helper), + targetPath: "C:\\rec\\a.mp4", + readOutput: () => "Recording stopped. Output path: C:\\rec\\a.mp4\n", + }); + + expect(result).toEqual({ ok: true, screenVideoPath: "C:\\rec\\a.mp4" }); + // No timers were needed: nothing was ever scheduled to wait on. + expect(vi.getTimerCount()).toBe(0); + }); + + /** + * The helper announces a finalized recording before it releases the GPU + * device, so its own watchdog killing it during teardown must still count as + * a success -- the MP4 on disk is complete, and the caller deletes files it + * is told are failures. + */ + it("keeps the recording when the helper was killed after finalizing", async () => { + const pending = waitForNativeWindowsCaptureStop({ + proc: asProc(helper), + targetPath: "C:\\rec\\a.mp4", + readOutput: () => + "[stop-timing] step=encoder-finalize elapsed_ms=400\n" + + "Recording stopped. Output path: C:\\rec\\a.mp4\n" + + "[stop-timing] step=wgc-session-close elapsed_ms=8001 phase=abandoned\n" + + '{"event":"stop-timeout","schemaVersion":2,"step":"wgc-session-close"}\n', + }); + + helper.exit(3); + + await expect(pending).resolves.toEqual({ ok: true, screenVideoPath: "C:\\rec\\a.mp4" }); + }); + + /** + * The webcam is a second, optional file, and the helper announces the screen + * recording before finalizing it precisely so a bad camera clip cannot veto a + * complete capture. The exit code is non-zero and the reason is on stderr; + * the screen MP4 is still finished and must still be kept. + */ + it("keeps the screen recording when only the webcam failed to finalize", async () => { + const pending = waitForNativeWindowsCaptureStop({ + proc: asProc(helper), + targetPath: "C:\\rec\\a.mp4", + readOutput: () => + "Recording stopped. Output path: C:\\rec\\a.mp4\n" + + "[stop-timing] step=webcam-encoder-finalize elapsed_ms=900\n" + + "ERROR: Failed to finalize the webcam recording\n", + }); + + helper.exit(1); + + await expect(pending).resolves.toEqual({ ok: true, screenVideoPath: "C:\\rec\\a.mp4" }); + }); + + it("classifies the helper's own shutdown watchdog as a stop timeout", async () => { + const pending = waitForNativeWindowsCaptureStop({ + proc: asProc(helper), + targetPath: "C:\\rec\\a.mp4", + readOutput: () => + "[stop-timing] step=video-writer-join elapsed_ms=8001 phase=abandoned\n" + + '{"event":"stop-timeout","schemaVersion":2,"step":"video-writer-join"}\n', + }); + + helper.exit(3); + + await expect(pending).resolves.toEqual({ + ok: false, + reason: "stop-timeout", + message: "The recorder stalled while shutting down (video-writer-join).", + exited: true, + }); + }); + + it("reports a helper failure with its output rather than a timeout", async () => { + const pending = waitForNativeWindowsCaptureStop({ + proc: asProc(helper), + targetPath: "C:\\rec\\a.mp4", + readOutput: () => "ERROR: Failed to encode WGC frame\n", + }); + + helper.exit(1); + + await expect(pending).resolves.toEqual({ + ok: false, + reason: "helper-failed", + message: "ERROR: Failed to encode WGC frame", + exited: true, + }); + }); + + /** Every run ends with diagnostics, so "the last line" is never the cause. */ + it("skips diagnostic noise when picking the user-facing failure message", async () => { + const pending = waitForNativeWindowsCaptureStop({ + proc: asProc(helper), + targetPath: "C:\\rec\\a.mp4", + readOutput: () => + "ERROR: Failed to initialize Media Foundation encoder\n" + + "[stop-timing] step=microphone elapsed_ms=2\n" + + '{"event":"warning","code":"webcam-unavailable"}\n', + }); + + helper.exit(1); + + await expect(pending).resolves.toMatchObject({ + reason: "helper-failed", + message: "ERROR: Failed to initialize Media Foundation encoder", + }); + }); + + it("still settles when killing the wedged helper throws", async () => { + helper.diesOnKill = false; + const forceKill = vi.fn(async () => { + throw new Error("EPERM"); + }); + + const pending = waitForNativeWindowsCaptureStop({ + proc: asProc(helper), + targetPath: "C:\\rec\\a.mp4", + readOutput: () => "", + timeoutMs: 20_000, + killGraceMs: 2_000, + forceKill, + }); + + await vi.advanceTimersByTimeAsync(20_000); + await vi.advanceTimersByTimeAsync(4_000); + + await expect(pending).resolves.toMatchObject({ + ok: false, + reason: "stop-timeout", + exited: false, + }); + }); + + it("kills the helper and reports a timeout when it never finalizes", async () => { + const pending = waitForNativeWindowsCaptureStop({ + proc: asProc(helper), + targetPath: "C:\\rec\\a.mp4", + readOutput: () => "[stop-timing] step=video-writer-join phase=begin elapsed_ms=0\n", + timeoutMs: 20_000, + }); + + await vi.advanceTimersByTimeAsync(20_000); + + await expect(pending).resolves.toEqual({ + ok: false, + reason: "stop-timeout", + // A short sentence, not the log: this ends up in a toast. + message: "The recorder did not shut down in time.", + exited: true, + }); + expect(helper.killCalls).toBe(1); + }); + + /** The timeout path is the likeliest place for an already-finalized file. */ + it("keeps a recording the helper finalized before the parent gave up", async () => { + const pending = waitForNativeWindowsCaptureStop({ + proc: asProc(helper), + targetPath: "C:\\rec\\a.mp4", + readOutput: () => + "Recording stopped. Output path: C:\\rec\\a.mp4\n" + + "[stop-timing] step=wgc-session-close elapsed_ms=1 phase=begin\n", + timeoutMs: 20_000, + }); + + await vi.advanceTimersByTimeAsync(20_000); + + await expect(pending).resolves.toEqual({ ok: true, screenVideoPath: "C:\\rec\\a.mp4" }); + }); + + it("reports the exit code rather than a progress line when nothing failed loudly", async () => { + const pending = waitForNativeWindowsCaptureStop({ + proc: asProc(helper), + targetPath: "C:\\rec\\a.mp4", + readOutput: () => 'Recording started\n{"event":"ready","schemaVersion":2}\n', + }); + + helper.exit(9); + + await expect(pending).resolves.toMatchObject({ + reason: "helper-failed", + message: "Native Windows capture exited with code=9", + }); + }); + + it("escalates to a forced tree kill when the helper survives kill()", async () => { + helper.diesOnKill = false; + const forceKill = vi.fn(async () => { + helper.exit(1); + }); + + const pending = waitForNativeWindowsCaptureStop({ + proc: asProc(helper), + targetPath: "C:\\rec\\a.mp4", + readOutput: () => "", + timeoutMs: 20_000, + killGraceMs: 2_000, + forceKill, + }); + + await vi.advanceTimersByTimeAsync(20_000); + await vi.advanceTimersByTimeAsync(2_000); + + const result = await pending; + expect(forceKill).toHaveBeenCalledWith(4242); + expect(result).toMatchObject({ ok: false, reason: "stop-timeout", exited: true }); + }); + + it("reports the helper as surviving when even the forced kill fails", async () => { + helper.diesOnKill = false; + // taskkill returns, but the helper is wedged below user mode and survives. + const forceKill = vi.fn(async () => undefined); + + const pending = waitForNativeWindowsCaptureStop({ + proc: asProc(helper), + targetPath: "C:\\rec\\a.mp4", + readOutput: () => "", + timeoutMs: 20_000, + killGraceMs: 2_000, + forceKill, + }); + + await vi.advanceTimersByTimeAsync(20_000); + await vi.advanceTimersByTimeAsync(4_000); + + await expect(pending).resolves.toMatchObject({ + ok: false, + reason: "stop-timeout", + exited: false, + }); + }); +}); + +describe("terminateNativeWindowsCapture", () => { + it("is a no-op for a helper that already exited", async () => { + helper.exitCode = 0; + + await expect(terminateNativeWindowsCapture(asProc(helper))).resolves.toBe(true); + expect(helper.killCalls).toBe(0); + }); + + it("does not wait out the grace period when kill() works", async () => { + const pending = terminateNativeWindowsCapture(asProc(helper), { graceMs: 2_000 }); + + await expect(pending).resolves.toBe(true); + expect(helper.killCalls).toBe(1); + }); +}); diff --git a/electron/recording/nativeWindowsCaptureStop.ts b/electron/recording/nativeWindowsCaptureStop.ts new file mode 100644 index 0000000000..95da2c19ac --- /dev/null +++ b/electron/recording/nativeWindowsCaptureStop.ts @@ -0,0 +1,275 @@ +import { type ChildProcessWithoutNullStreams, execFile } from "node:child_process"; + +/** + * Stopping a native Windows (WGC) recording, as a unit that can be tested. + * + * This lives outside `electron/ipc/handlers.ts` for one reason: that module + * calls `app.getPath()` while it is being imported, so nothing in it can be + * loaded from a test. The stop path shipped broken twice (issues #115, #252) + * with no test able to see it, so it moved here. + */ + +/** + * The outer bound on a stop, and deliberately not the lever. + * + * This was raised from 15s to 60s for issue #34 so `IMFSinkWriter::Finalize` + * had room to drain on slow encoders, and it stays at 60s for the same reason: + * a parent that gave up first would kill a working save. + * + * It must stay above the helper's own shutdown ceiling + * (`OPENSCREEN_WGC_STOP_BUDGET_MS`, 50s — see the stop sequence in + * `electron/native/wgc-capture/src/main.cpp`), which is what guarantees the + * helper always ends itself rather than being killed mid-finalize from here. + * Raise one and raise the other. + * + * What changed for issue #252 is that reaching this timeout is no longer how a + * wedged recorder is caught: the helper bounds every shutdown step itself and + * force-exits within seconds, so 'close' arrives long before this fires. + * Getting here means the helper is stuck somewhere even `TerminateProcess` + * could not reach. + */ +export const NATIVE_WINDOWS_CAPTURE_STOP_TIMEOUT_MS = 60_000; + +/** How long a killed helper gets to actually die before we escalate. */ +const NATIVE_WINDOWS_CAPTURE_KILL_GRACE_MS = 2_000; + +const RECORDING_STOPPED_PATTERN = /Recording stopped\. Output path: (.+)/; +const STOP_TIMEOUT_EVENT_PATTERN = /"event":"stop-timeout"[^\n]*"step":"([^"]+)"/; + +export type NativeWindowsCaptureStopReason = "stop-timeout" | "helper-failed"; + +export type NativeWindowsCaptureStopResult = + | { ok: true; screenVideoPath: string } + | { + ok: false; + reason: NativeWindowsCaptureStopReason; + message: string; + /** False when a wedged helper survived even the forced kill. */ + exited: boolean; + }; + +export function readStoppedPath(output: string) { + return output.match(RECORDING_STOPPED_PATTERN)?.[1]?.trim() || null; +} + +/** The step the helper's shutdown watchdog gave up on, if it fired. */ +export function readAbandonedStep(output: string) { + return output.match(STOP_TIMEOUT_EVENT_PATTERN)?.[1] ?? null; +} + +/** + * The most useful line of a failed helper run, for a toast. + * + * The log ends with `[stop-timing]` and JSON protocol lines on every run, so + * "the last line" is reliably a diagnostic rather than a cause. Prefer what the + * helper actually complained about. + */ +export function readHelperFailureMessage(output: string, code: number | null) { + const complaints = output + .split(/\r?\n/) + .map((line) => line.trim()) + .filter((line) => line.startsWith("ERROR:") || line.startsWith("WARNING:")); + + // Only lines that describe a failure. The rest of a helper log is progress + // ("Recording started") and diagnostics, and reporting the last of those as + // the error reads like a success message on a red toast. + return complaints.at(-1) ?? `Native Windows capture exited with code=${code ?? "unknown"}`; +} + +function hasExited(proc: ChildProcessWithoutNullStreams) { + return proc.exitCode !== null || proc.signalCode !== null; +} + +/** + * `taskkill /T /F` on the helper. `ChildProcess.kill()` maps to + * `TerminateProcess` on Windows, which is already forceful but cannot touch a + * thread that is stuck below user mode -- the exact state a wedged display + * driver leaves the helper in. Escalating gives us a second chance, and an + * orphan that survives both is worth reporting rather than pretending away. + */ +function forceKillProcessTree(pid: number) { + return new Promise((resolve) => { + // Bounded: taskkill walks the process tree and opens handles, both of + // which can block on exactly the wedged process it is being asked to + // kill. Nothing else can settle the stop promise by this point, so a + // taskkill that never returns would recreate the unbounded wait this + // whole path exists to end. + execFile( + "taskkill", + ["/PID", String(pid), "/T", "/F"], + { timeout: NATIVE_WINDOWS_CAPTURE_KILL_GRACE_MS, windowsHide: true }, + () => resolve(), + ); + }); +} + +function waitForExit(proc: ChildProcessWithoutNullStreams, timeoutMs: number) { + if (hasExited(proc)) { + return Promise.resolve(true); + } + + return new Promise((resolve) => { + const settle = (exited: boolean) => { + clearTimeout(timer); + proc.off("close", onClose); + resolve(exited); + }; + const onClose = () => settle(true); + const timer = setTimeout(() => settle(false), timeoutMs); + proc.once("close", onClose); + }); +} + +/** + * Kills the helper and confirms it actually died, escalating once. Resolves to + * whether the process is gone. + */ +export async function terminateNativeWindowsCapture( + proc: ChildProcessWithoutNullStreams, + options: { + graceMs?: number; + forceKill?: (pid: number) => Promise; + } = {}, +) { + if (hasExited(proc)) { + return true; + } + + const graceMs = options.graceMs ?? NATIVE_WINDOWS_CAPTURE_KILL_GRACE_MS; + const forceKill = options.forceKill ?? forceKillProcessTree; + + proc.kill(); + if (await waitForExit(proc, graceMs)) { + return true; + } + + if (typeof proc.pid === "number") { + await forceKill(proc.pid); + return waitForExit(proc, graceMs); + } + + return false; +} + +/** + * Waits for the helper to report a finalized recording. + * + * Resolves rather than rejects on failure: the caller needs to tell a stop + * timeout apart from a helper error to pick the right message, and an `Error` + * carrying the whole accumulated helper log is not something to put in front of + * a user. + */ +export function waitForNativeWindowsCaptureStop(options: { + proc: ChildProcessWithoutNullStreams; + /** Path we asked the helper to write, used when it exits 0 without saying so. */ + targetPath: string | null; + /** The accumulated helper output; read lazily so late chunks are included. */ + readOutput: () => string; + timeoutMs?: number; + killGraceMs?: number; + forceKill?: (pid: number) => Promise; +}): Promise { + const { proc, targetPath, readOutput } = options; + const timeoutMs = options.timeoutMs ?? NATIVE_WINDOWS_CAPTURE_STOP_TIMEOUT_MS; + + const settleFromOutput = (code: number | null): NativeWindowsCaptureStopResult => { + const output = readOutput(); + // The helper announces this as soon as the MP4 index is written, before + // it releases the GPU device. So a helper that was killed during teardown + // still reports a recording that is complete and playable -- taking its + // word for that is what keeps the file (issue #252). + const stoppedPath = readStoppedPath(output); + if (stoppedPath) { + return { ok: true, screenVideoPath: stoppedPath }; + } + if (code === 0 && targetPath) { + return { ok: true, screenVideoPath: targetPath }; + } + // The helper's own shutdown watchdog gave up. That is a stop timeout, not + // a generic failure, and it knows which step stalled. + const abandonedStep = readAbandonedStep(output); + if (abandonedStep) { + return { + ok: false, + reason: "stop-timeout", + message: `The recorder stalled while shutting down (${abandonedStep}).`, + exited: true, + }; + } + return { + ok: false, + reason: "helper-failed", + message: readHelperFailureMessage(output, code), + exited: true, + }; + }; + + // The helper may already be gone -- it force-exits on its own shutdown + // watchdog, and a DXGI device loss can kill it outright mid-recording. Node + // does not re-emit 'close' for a process that has already exited, so + // registering a listener first would burn the whole timeout waiting for an + // event that can never arrive. + if (hasExited(proc)) { + return Promise.resolve(settleFromOutput(proc.exitCode)); + } + + return new Promise((resolve) => { + const onClose = (code: number | null) => { + cleanup(); + resolve(settleFromOutput(code)); + }; + const onError = (error: Error) => { + cleanup(); + resolve({ + ok: false, + reason: "helper-failed", + message: error.message, + exited: hasExited(proc), + }); + }; + const cleanup = () => { + clearTimeout(timer); + proc.off("close", onClose); + proc.off("error", onError); + }; + + const timer = setTimeout(() => { + cleanup(); + void (async () => { + let exited = false; + try { + exited = await terminateNativeWindowsCapture(proc, { + graceMs: options.killGraceMs, + forceKill: options.forceKill, + }); + } catch (error) { + // Killing a wedged, possibly protected process can itself + // fail. `cleanup()` has already dropped this promise's only + // other path to settling, so swallowing the rejection here + // would hang the stop handler forever -- the very failure + // this timeout exists to end. + console.warn("[native-wgc] could not terminate the wedged helper:", error); + } + // Check for a finalized recording before calling this a loss. The + // helper announces the file as soon as its index is written and + // only then does its GPU teardown, so the run most likely to end + // up here is also the one most likely to have already produced a + // perfectly playable MP4. + const stoppedPath = readStoppedPath(readOutput()); + if (stoppedPath) { + resolve({ ok: true, screenVideoPath: stoppedPath }); + return; + } + resolve({ + ok: false, + reason: "stop-timeout", + message: "The recorder did not shut down in time.", + exited, + }); + })(); + }, timeoutMs); + + proc.once("close", onClose); + proc.once("error", onError); + }); +} diff --git a/electron/recording/webm-seek-index.test.ts b/electron/recording/webm-seek-index.test.ts index fd654a3a6e..58fa423aca 100644 --- a/electron/recording/webm-seek-index.test.ts +++ b/electron/recording/webm-seek-index.test.ts @@ -4,6 +4,9 @@ import path from "node:path"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { reindexRecordingOnDisk } from "./webm-seek-index"; +/** The platforms whose native helpers already write an indexed file. */ +const NON_LINUX = ["darwin", "win32"] as const; + /** * The property under test is not "does libavformat work" — the Rust side owns * that, and `crates/compositor/tests/remux_seek_index.rs` proves it. It is the @@ -14,13 +17,26 @@ import { reindexRecordingOnDisk } from "./webm-seek-index"; describe("recording re-index", () => { let dir: string; const ORIGINAL = "original recording bytes"; - + const REAL_PLATFORM = process.platform; + + const setPlatform = (value: NodeJS.Platform) => + Object.defineProperty(process, "platform", { value, configurable: true }); + + /** + * Pin the platform, because the wrapper is Linux-gated and returns + * `unsupported-platform` before it touches anything else. Left to the real + * platform, every case below stops at that guard and asserts nothing on a + * macOS or Windows checkout — where this suite read as six red tests that + * were neither the contributor's fault nor a real regression. + */ beforeEach(async () => { + setPlatform("linux"); dir = await mkdtemp(path.join(tmpdir(), "openscreen-reindex-")); vi.spyOn(console, "warn").mockImplementation(() => undefined); }); afterEach(async () => { + setPlatform(REAL_PLATFORM); await rm(dir, { recursive: true, force: true }); vi.restoreAllMocks(); }); @@ -116,18 +132,17 @@ describe("recording re-index", () => { expect(await readFile(filePath, "utf8")).toBe("remuxed bytes"); }); - it("does nothing on platforms whose capture already writes indexed files", async () => { + // Both platforms the guard is there for, so the Linux pin above can never + // quietly become the only thing this suite ever exercises. + it.each(NON_LINUX)("does nothing on %s, which captures an index already", async (platform) => { const filePath = await makeRecording(); const service = fakeRemux("remuxed bytes"); - const platform = process.platform; - Object.defineProperty(process, "platform", { value: "win32", configurable: true }); - try { - const result = await reindexRecordingOnDisk(filePath, service); - expect(result).toEqual({ reindexed: false, reason: "unsupported-platform" }); - expect(service.remuxSeekable).not.toHaveBeenCalled(); - expect(await readFile(filePath, "utf8")).toBe(ORIGINAL); - } finally { - Object.defineProperty(process, "platform", { value: platform, configurable: true }); - } + setPlatform(platform); + + const result = await reindexRecordingOnDisk(filePath, service); + + expect(result).toEqual({ reindexed: false, reason: "unsupported-platform" }); + expect(service.remuxSeekable).not.toHaveBeenCalled(); + expect(await readFile(filePath, "utf8")).toBe(ORIGINAL); }); }); diff --git a/electron/windows.ts b/electron/windows.ts index 687a3a175b..0b19d5b988 100644 --- a/electron/windows.ts +++ b/electron/windows.ts @@ -264,7 +264,22 @@ export function createHudOverlayWindow(): BrowserWindow { backgroundThrottling: false, }, }); - win.setIgnoreMouseEvents(true, { forward: true }); + + // Deliberately NOT born click-through: the renderer asks for it on mount, over + // "hud-overlay-ignore-mouse-events" (`show: false` holds this window back until + // ready-to-show, so the two are ~85 ms apart — measured, not assumed). What that + // leaves open is an invisible rectangle that can swallow one desktop click in + // those 85 ms, right after the user launched the app — against what doing it here + // cost them: the whole app (issue #266). On Windows the `forward` option is a global + // WH_MOUSE_LL hook, and that hook is the only way out of the state, because + // Chromium sends no pointermove to a window it has made input-transparent — so + // the renderer can never ask to leave it on its own. Electron latches + // the install behind `forwarding_mouse_messages_` and retries only after a + // setIgnoreMouseEvents(false) — the very call a dead hook prevents. One refused + // or revoked hook (Windows drops any whose callback overruns the 300 ms + // LowLevelHooksTimeout — on this thread, still busy booting the app) and the HUD + // is painted, inert, forever. Asking later moves the install onto an IPC message, + // i.e. onto a main thread that is provably pumping. // Keep the recording controls out of the recording (see applyContentProtection). applyContentProtection(win, "HUD"); diff --git a/package.json b/package.json index fcff53ac65..42d2f41eb2 100644 --- a/package.json +++ b/package.json @@ -1,7 +1,7 @@ { "name": "openscreen", "private": true, - "version": "1.8.0", + "version": "1.9.0", "type": "module", "packageManager": "npm@10.9.4", "engines": { @@ -46,6 +46,7 @@ "build:whisper-binaries": "bash scripts/build-whisper-stt.sh", "test:whisper-stt": "node scripts/test-whisper-stt.mjs", "test": "vitest --run", + "test:changed": "vitest --run --changed", "wb": "vitest --run --config vitest.workbench.config.ts", "wb:l0": "vitest --run --config vitest.workbench.config.ts workbench/l0", "wb:watch": "vitest --config vitest.workbench.config.ts workbench/l0", diff --git a/scripts/diagnostic-tool/diagnostic.mjs b/scripts/diagnostic-tool/diagnostic.mjs index 3b08d798cc..f19020ae51 100644 --- a/scripts/diagnostic-tool/diagnostic.mjs +++ b/scripts/diagnostic-tool/diagnostic.mjs @@ -149,8 +149,12 @@ function buildConfig(opts) { function parseStopTiming(stderrText) { const lines = []; for (const line of stderrText.split(/\r?\n/)) { - const m = line.match(/\[stop-timing\]\s+step=(\S+)\s+elapsed_ms=(\d+)/); - if (m) lines.push({ step: m[1], elapsedMs: Number(m[2]) }); + // `phase` is the point of the whole log: `begin` is the step being + // entered, `abandoned` names the step the shutdown watchdog gave up on. + // Dropping it left the report unable to say which step hung -- the one + // question a #252 bug report has to answer. + const m = line.match(/\[stop-timing\]\s+step=(\S+)\s+elapsed_ms=(\d+)(?:\s+phase=(\S+))?/); + if (m) lines.push({ step: m[1], elapsedMs: Number(m[2]), phase: m[3] ?? "end" }); } return lines; } @@ -303,7 +307,14 @@ async function main() { console.log(`[diag] stop elapsed: ${report.stopElapsedMs}ms`); console.log(`[diag] stop timing steps:`); for (const entry of report.stopTiming) { - console.log(`[diag] ${entry.step.padEnd(28)} ${entry.elapsedMs}ms`); + // Only the outcome of each step, so the summary reads as one line per + // step rather than an entry-and-exit pair, and an abandoned step is + // impossible to miss. + if (entry.phase === "begin") { + continue; + } + const suffix = entry.phase === "end" ? "" : ` <-- ${entry.phase.toUpperCase()}`; + console.log(`[diag] ${entry.step.padEnd(28)} ${entry.elapsedMs}ms${suffix}`); } console.log(`[diag] report: ${outputPath}`); } diff --git a/scripts/test-windows-wgc-helper.mjs b/scripts/test-windows-wgc-helper.mjs index c6c69441e7..849bbd6e9e 100644 --- a/scripts/test-windows-wgc-helper.mjs +++ b/scripts/test-windows-wgc-helper.mjs @@ -35,18 +35,47 @@ const WITH_SOFTWARE_FALLBACK = const INJECT_DEFAULT_SINK_WRITER_FAILURE_ENV = "OPENSCREEN_WGC_TEST_INJECT_DEFAULT_SINK_WRITER_FAILURE_ONCE"; const INJECTION_MARKER = "TEST-ONLY: Injected default MFCreateSinkWriterFromURL failure"; +const STALL_READBACK_ENV = "OPENSCREEN_WGC_TEST_STALL_READBACK_MS"; +/** + * Reproduces issue #252 on ordinary hardware: holds the frame lock across a + * stall the way a wedged GPU readback does. Before the fix the helper hung + * forever with no `[stop-timing]` output at all; it must now always exit. + */ +const WITH_STALLED_READBACK = + process.env.OPENSCREEN_WGC_TEST_STALL_READBACK === "true" || + process.argv.includes("--stall-readback"); +const STALL_READBACK_MS = Number(process.env[STALL_READBACK_ENV] ?? 60_000); +const STOP_BUDGET_ENV = "OPENSCREEN_WGC_STOP_BUDGET_MS"; +/** + * The helper's global shutdown ceiling, pinned into its environment below so + * the harness and the helper cannot drift apart. It matters because the + * encoder-finalize step is the one allowed to spend the whole ceiling — issue + * #34 exists because a long software-encoder finalize legitimately takes + * seconds — so a limit below it would kill a helper that was still working and + * report it as the #252 hang. + */ +const STOP_BUDGET_MS = Number(process.env[STOP_BUDGET_ENV] ?? 50_000); +/** Past the helper's own ceiling it never ended itself, which IS issue #252. */ +const STOP_HANG_LIMIT_MS = STOP_BUDGET_MS + 15_000; +/** A healthy stop is well under a second. */ +const STOP_LATENCY_BUDGET_MS = 15_000; if (WITH_SOFTWARE_ENCODER && WITH_SOFTWARE_FALLBACK) { throw new Error("--software-encoder and --software-fallback are mutually exclusive"); } -function runHelper(config, { injectDefaultSinkWriterFailure = false } = {}) { +function runHelper(config, { injectDefaultSinkWriterFailure = false, stallReadbackMs = 0 } = {}) { return new Promise((resolve, reject) => { const env = { ...process.env }; delete env[INJECT_DEFAULT_SINK_WRITER_FAILURE_ENV]; + delete env[STALL_READBACK_ENV]; + env[STOP_BUDGET_ENV] = String(STOP_BUDGET_MS); if (injectDefaultSinkWriterFailure) { env[INJECT_DEFAULT_SINK_WRITER_FAILURE_ENV] = "1"; } + if (stallReadbackMs > 0) { + env[STALL_READBACK_ENV] = String(stallReadbackMs); + } const child = spawn(HELPER_PATH, [JSON.stringify(config)], { env, stdio: ["pipe", "pipe", "pipe"], @@ -56,12 +85,23 @@ function runHelper(config, { injectDefaultSinkWriterFailure = false } = {}) { let stdout = ""; let stderr = ""; let stopTimer = null; + let stopSentAt = null; + let stopHung = false; + let hangTimer = null; const scheduleStop = () => { if (stopTimer) { return; } stopTimer = setTimeout(() => { + stopSentAt = Date.now(); child.stdin.write("stop\n"); + // The whole point of issues #115 and #252 was a helper that never + // came back from `stop`. Without a bound here the harness inherits + // the hang instead of reporting it. + hangTimer = setTimeout(() => { + stopHung = true; + child.kill(); + }, STOP_HANG_LIMIT_MS); }, DURATION_MS); }; const fallbackTimer = setTimeout(scheduleStop, 15_000); @@ -81,11 +121,57 @@ function runHelper(config, { injectDefaultSinkWriterFailure = false } = {}) { if (stopTimer) { clearTimeout(stopTimer); } - resolve({ code, stdout, stderr }); + if (hangTimer) { + clearTimeout(hangTimer); + } + resolve({ + code, + stdout, + stderr, + stopHung, + stopLatencyMs: stopSentAt === null ? null : Date.now() - stopSentAt, + }); }); }); } +/** + * Every `[stop-timing]` step the helper *finished*, in order. + * + * `phase=begin` is the same step announced on entry, so counting both listed + * every step twice. `phase=abandoned` is kept: that step did end, just badly. + */ +function readStopTimingSteps(stderr) { + return [...stderr.matchAll(/\[stop-timing\]\s+step=(\S+)\s+elapsed_ms=\d+(?:\s+phase=(\S+))?/g)] + .filter((match) => match[2] !== "begin") + .map((match) => match[1]); +} + +function assertStopWasClean(result) { + if (result.stopHung) { + throw new Error( + `Helper did not exit within ${STOP_HANG_LIMIT_MS}ms of "stop" (issue #252). ` + + `stop-timing steps seen: ${readStopTimingSteps(result.stderr).join(", ") || "none"}`, + ); + } + const steps = readStopTimingSteps(result.stderr); + if (!steps.includes("command-received")) { + throw new Error( + 'Helper never acknowledged the stop command ("[stop-timing] step=command-received").', + ); + } + if (steps.includes("wgc-session-close") === false) { + throw new Error( + `Helper stopped without completing its shutdown sequence. Steps: ${steps.join(", ")}`, + ); + } + if (result.stopLatencyMs !== null && result.stopLatencyMs > STOP_LATENCY_BUDGET_MS) { + throw new Error( + `Stop took ${result.stopLatencyMs}ms, over the ${STOP_LATENCY_BUDGET_MS}ms budget.`, + ); + } +} + function startFixtureWindow() { return new Promise((resolve, reject) => { const child = spawn("mspaint.exe", [], { @@ -294,12 +380,44 @@ let result; try { result = await runHelper(config, { injectDefaultSinkWriterFailure: WITH_SOFTWARE_FALLBACK, + stallReadbackMs: WITH_STALLED_READBACK ? STALL_READBACK_MS : 0, }); } finally { if (fixtureWindow) { fixtureWindow.child.kill(); } } + +// The regression check for issue #252. With the frame lock deliberately wedged +// there is no usable recording to assert on -- what matters is only that the +// helper still noticed the stop and still died, naming the step it died in. +if (WITH_STALLED_READBACK) { + if (result.stopHung) { + throw new Error( + `Helper survived ${STOP_HANG_LIMIT_MS}ms past "stop" with a stalled readback. ` + + "Its shutdown watchdog did not fire (issue #252).", + ); + } + const steps = readStopTimingSteps(result.stderr); + if (!steps.includes("command-received")) { + throw new Error(`Helper never acknowledged "stop". Steps seen: ${steps.join(", ") || "none"}`); + } + if (!/phase=abandoned/.test(result.stderr)) { + throw new Error( + `Helper exited without reporting an abandoned shutdown step. stderr:\n${result.stderr}`, + ); + } + console.log("WGC helper stalled-readback stop check passed", { + stopLatencyMs: result.stopLatencyMs, + steps, + abandoned: result.stderr.match(/step=(\S+)\s+elapsed_ms=\d+\s+phase=abandoned/)?.[1] ?? null, + }); + fs.rmSync(outputPath, { force: true }); + process.exit(0); +} + +assertStopWasClean(result); + if (result.code !== 0) { if ( WITH_WEBCAM && @@ -451,6 +569,8 @@ console.log( JSON.stringify( { success: true, + stopLatencyMs: result.stopLatencyMs, + stopTimingSteps: readStopTimingSteps(result.stderr), outputPath, webcamOutputPath, bytes: fs.statSync(outputPath).size, diff --git a/src/components/ai-edition/CaptionsPane.gating.test.tsx b/src/components/ai-edition/CaptionsPane.gating.test.tsx index b1e87c92bb..cae76b59a4 100644 --- a/src/components/ai-edition/CaptionsPane.gating.test.tsx +++ b/src/components/ai-edition/CaptionsPane.gating.test.tsx @@ -1,3 +1,4 @@ +// @vitest-environment jsdom // Captions are a view of the transcript, so the pane's "Transcribe video" // button is a retry, not a first step — the background pass has already tried. // On a media with no audio track that retry can only fail again, so the button diff --git a/src/components/ai-edition/ChatWelcome.test.tsx b/src/components/ai-edition/ChatWelcome.test.tsx index 28ccb4248d..8d835285ef 100644 --- a/src/components/ai-edition/ChatWelcome.test.tsx +++ b/src/components/ai-edition/ChatWelcome.test.tsx @@ -1,3 +1,4 @@ +// @vitest-environment jsdom // ChatWelcome guards the "no provider connected" empty state: the copy reaches // the DOM, the CTA fires, and a non-English locale is really translated rather // than falling back to English. localeParity.test.ts covers key presence for diff --git a/src/components/ai-edition/ColorField.test.tsx b/src/components/ai-edition/ColorField.test.tsx index 84e6f771e3..636167367b 100644 --- a/src/components/ai-edition/ColorField.test.tsx +++ b/src/components/ai-edition/ColorField.test.tsx @@ -1,3 +1,4 @@ +// @vitest-environment jsdom import "@testing-library/jest-dom"; import { fireEvent, render, screen } from "@testing-library/react"; import { beforeAll, describe, expect, it, vi } from "vitest"; diff --git a/src/components/ai-edition/EditorEmptyState.test.tsx b/src/components/ai-edition/EditorEmptyState.test.tsx index e6ae692461..acb513e4b5 100644 --- a/src/components/ai-edition/EditorEmptyState.test.tsx +++ b/src/components/ai-edition/EditorEmptyState.test.tsx @@ -1,3 +1,4 @@ +// @vitest-environment jsdom import "@testing-library/jest-dom"; import { act, cleanup, fireEvent, render, screen, waitFor } from "@testing-library/react"; import type { ReactElement } from "react"; diff --git a/src/components/ai-edition/ExportDialog.tsx b/src/components/ai-edition/ExportDialog.tsx index 97ae481c0b..9017569891 100644 --- a/src/components/ai-edition/ExportDialog.tsx +++ b/src/components/ai-edition/ExportDialog.tsx @@ -316,7 +316,12 @@ export function ExportDialog({ open, onClose, document }: ExportDialogProps) { action: { label: t("exportDialog.showInFolder"), onClick: () => { - void window.electronAPI?.revealInFolder?.(pickedPath); + // `revealInFolder` is a bare ipcRenderer.invoke, so it rejects when + // the main handler throws. The export already succeeded — failing to + // open the folder is not worth a second toast, but it is worth a line. + void window.electronAPI?.revealInFolder?.(pickedPath).catch((err) => { + console.warn("[export] failed to reveal the file in its folder:", err); + }); }, }, }); diff --git a/src/components/ai-edition/Modals.tsx b/src/components/ai-edition/Modals.tsx index ee0a89e431..739ca3f546 100644 --- a/src/components/ai-edition/Modals.tsx +++ b/src/components/ai-edition/Modals.tsx @@ -1512,7 +1512,13 @@ export function SourceTranscriptModal({ const v = videoRef.current; if (!v) return; if (v.paused) { - void v.play(); + // Same catch as VirtualPreview's: `play()` rejects on the autoplay policy + // or when a new load interrupts it, and `isPlaying` is driven by the + // element's own play/pause events — so a rejection leaves nothing to + // reconcile, it just must not escape as an unhandled rejection. + void v.play().catch(() => { + // swallow: rejection just means playback never started + }); } else { v.pause(); } @@ -1528,7 +1534,11 @@ export function SourceTranscriptModal({ const requestFullscreen = () => { const v = videoRef.current; if (!v) return; - void v.requestFullscreen?.(); + // Rejects when the gesture isn't accepted or the element can't go fullscreen. + // Nothing to reconcile — the document stays as it was. + void v.requestFullscreen?.().catch(() => { + // swallow: rejection just means we stayed windowed + }); }; return ( diff --git a/src/components/ai-edition/NewEditorShell.tsx b/src/components/ai-edition/NewEditorShell.tsx index 56a220d0bd..208942e27a 100644 --- a/src/components/ai-edition/NewEditorShell.tsx +++ b/src/components/ai-edition/NewEditorShell.tsx @@ -465,7 +465,13 @@ export function NewEditorShell() { const togglePlay = useCallback(() => { if (!videoElement) return; if (videoElement.paused) { - void videoElement.play(); + // Same catch as VirtualPreview's: `play()` rejects on the autoplay policy + // or when a new load interrupts it, and the store's `playing` flag is + // driven by the element's own play/pause listeners above — so a rejection + // leaves nothing to reconcile, it just must not escape unhandled. + void videoElement.play().catch(() => { + // swallow: rejection just means playback never started + }); } else { videoElement.pause(); } @@ -677,7 +683,9 @@ export function NewEditorShell() { } } if (action === "record") { - void window.electronAPI?.startNewRecording?.(); + void window.electronAPI?.startNewRecording?.().catch((err) => { + console.warn("[editor] failed to start a new recording:", err); + }); } resolve(choice); })(); @@ -688,7 +696,9 @@ export function NewEditorShell() { const handleNewRecording = useCallback(async () => { const choice = await promptUnsaved("record"); if (choice !== "cancel") { - void window.electronAPI?.startNewRecording?.(); + void window.electronAPI?.startNewRecording?.().catch((err) => { + console.warn("[editor] failed to start a new recording:", err); + }); } }, [promptUnsaved]); diff --git a/src/components/ai-edition/NewProjectModal.test.tsx b/src/components/ai-edition/NewProjectModal.test.tsx index 614479dd6e..03469939d0 100644 --- a/src/components/ai-edition/NewProjectModal.test.tsx +++ b/src/components/ai-edition/NewProjectModal.test.tsx @@ -1,3 +1,4 @@ +// @vitest-environment jsdom import "@testing-library/jest-dom"; import { cleanup, fireEvent, render, screen } from "@testing-library/react"; import type { ReactElement } from "react"; diff --git a/src/components/ai-edition/Preview.test.tsx b/src/components/ai-edition/Preview.test.tsx index d3498dfebe..294ad4c30a 100644 --- a/src/components/ai-edition/Preview.test.tsx +++ b/src/components/ai-edition/Preview.test.tsx @@ -1,3 +1,4 @@ +// @vitest-environment jsdom import "@testing-library/jest-dom"; import { act, cleanup, fireEvent, render, screen } from "@testing-library/react"; import { afterEach, describe, expect, it, vi } from "vitest"; diff --git a/src/components/ai-edition/PreviewCanvas.tsx b/src/components/ai-edition/PreviewCanvas.tsx index 4e430d5e82..86879fd128 100644 --- a/src/components/ai-edition/PreviewCanvas.tsx +++ b/src/components/ai-edition/PreviewCanvas.tsx @@ -214,8 +214,21 @@ export function PreviewCanvas(props: PreviewCanvasProps) { ); const cropRegion: CropRegion = activeClip?.cropRegion ?? DEFAULT_CROP_REGION; + // P4 — the layout preset is global (one panel for the whole timeline) but the camera + // is per clip, so the layout has to be resolved against the clip under the playhead. + const activeCameraTrack = useMemo( + () => resolveActiveCameraTrack(assets, props.clips, props.currentTimeSec), + [assets, props.clips, props.currentTimeSec], + ); + const activeClipHasCamera = Boolean(activeCameraTrack?.visible && activeCameraTrack.sourcePath); + const layout = useMemo(() => { - const preset = settings.webcamLayoutPreset as WebcamLayoutPreset; + // A clip with no camera lays out as "no-webcam", whatever the panel says. Hiding + // only the webcam slot is not enough: the block presets size the SCREEN off the + // block, so the screen stayed squeezed into its half with nothing beside it. + const preset = ( + activeClipHasCamera ? settings.webcamLayoutPreset : "no-webcam" + ) as WebcamLayoutPreset; const mask = settings.webcamMaskShape as WebcamMaskShape; // ponytail: padding shrinks the available content area for ALL layouts // (PiP/dual/stack) so the screen doesn't fill the canvas edge-to-edge. @@ -240,7 +253,7 @@ export function PreviewCanvas(props: PreviewCanvasProps) { canvasSize: frameSize, maxContentSize, screenSize: croppedScreenSize, - webcamSize: settings.webcamLayoutPreset === "no-webcam" ? null : WEBCAM_SOURCE_SIZE, + webcamSize: preset === "no-webcam" ? null : WEBCAM_SOURCE_SIZE, layoutPreset: preset, webcamSizePreset: settings.webcamSizePreset, // ponytail: PiP webcam is grabbable. Pass through the user's @@ -254,6 +267,7 @@ export function PreviewCanvas(props: PreviewCanvasProps) { frameSize, screenNativeSize, cropRegion, + activeClipHasCamera, settings.webcamLayoutPreset, settings.webcamMaskShape, settings.webcamSizePreset, @@ -293,17 +307,9 @@ export function PreviewCanvas(props: PreviewCanvasProps) { () => buildWebcamStyle(effectiveLayout, settings, frameSize), [effectiveLayout, settings, frameSize], ); - // P4 — the layout math above only knows the user's chosen preset - // (PiP/dual/stack), not whether the clip under the playhead actually has a - // camera. Without this, an empty (but styled — shadow, background) webcam - // slot stays visible for clips with no camera attached. - const activeCameraTrack = useMemo( - () => resolveActiveCameraTrack(assets, props.clips, props.currentTimeSec), - [assets, props.clips, props.currentTimeSec], - ); - const showWebcamSlot = Boolean( - layout?.webcamRect && activeCameraTrack?.visible && activeCameraTrack.sourcePath, - ); + // `layout` already resolves to "no-webcam" (hence `webcamRect: null`) for a + // camera-less clip, so this is belt-and-braces rather than the only guard. + const showWebcamSlot = Boolean(layout?.webcamRect && activeClipHasCamera); const [isPlaying, setIsPlaying] = useState(false); const handleVideoElement = useMemo(() => props.onVideoElement, [props.onVideoElement]); // L'élément `