diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 1ea21c6c2f..2203ae78d6 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 }}" 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..f6ca4fbe64 100644 --- a/.harness/docs/git-workflow.md +++ b/.harness/docs/git-workflow.md @@ -56,7 +56,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/AGENTS.md b/AGENTS.md index bfa79b4984..5f270ef887 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -95,7 +95,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/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/native/wgc-capture/src/main.cpp b/electron/native/wgc-capture/src/main.cpp index f8036c24a0..63cbcbba7f 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::functionCreateTexture2D(&desc, nullptr, &latestFrameTexture))) { encodeFailed = true; - control.stopRequested = true; - control.cv.notify_all(); + control.requestStop(); return; } } @@ -711,8 +786,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,6 +798,9 @@ 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 @@ -737,8 +814,7 @@ int main(int argc, char* argv[]) { videoSample); if (!hasVideoSample) { encodeFailed = true; - control.stopRequested = true; - control.cv.notify_all(); + control.requestStop(); break; } lastEncodedVideoTimestampHns = frameTimestampHns; @@ -748,22 +824,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 +876,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 +974,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 +1012,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 +1190,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/wgc_session.cpp b/electron/native/wgc-capture/src/wgc_session.cpp index 89f0b55fe0..ccab06727e 100644 --- a/electron/native/wgc-capture/src/wgc_session.cpp +++ b/electron/native/wgc-capture/src/wgc_session.cpp @@ -5,7 +5,9 @@ #include #include +#include #include +#include namespace wf = winrt::Windows::Foundation; namespace wgcap = winrt::Windows::Graphics::Capture; @@ -273,23 +275,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; + } + 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 (session_) { - session_.Close(); - session_ = 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; } - if (framePool_) { - framePool_.Close(); - framePool_ = nullptr; + 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 +372,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/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/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 `