From 2b3900077054dcbcc218018c732c2534634497a5 Mon Sep 17 00:00:00 2001 From: EtienneLescot Date: Sun, 23 Aug 2026 21:18:29 +0200 Subject: [PATCH 1/3] fix(app): reach Save Diagnostics from the menu and tray, not nowhere electron/ipc/handlers.ts and preload.ts fully implemented saveDiagnostic, and "Save Diagnostics" was localized into all 13 languages, but nothing in the app ever called it -- no button, no menu item, no keyboard shortcut. Found this while working out how to answer a #460 reporter's own question about where to find the diagnostic log: there was no working answer. Extracted the file-writing logic into an exported exportDiagnosticFile, shared by the existing IPC handler and three new entry points in main.ts: the tray's context menu (idle state), the Windows/Linux Help menu, and the macOS app menu. The tray one matters most for capture bugs like #460 -- it's reachable without opening any window, which is exactly the state a HUD is usually in right after a recording fails to stop. Reused "Save Diagnostics"'s existing translations (copied from the otherwise orphaned settings.support.saveDiagnostics key into common.json's actions) rather than inventing new strings across 13 locales. Verified: tsc --noEmit clean, biome clean, full suite (2161 tests) passes, i18n:check passes. Did not launch the dev Electron app -- native menu/tray changes aren't observable through the browser preview tooling, and a second instance risks the single-instance lock other active worktrees hold. Co-Authored-By: Claude Sonnet 5 --- electron/ipc/handlers.ts | 111 ++++++++++++++++------------- electron/main.ts | 49 ++++++++++++- src/i18n/locales/ar/common.json | 3 +- src/i18n/locales/en/common.json | 3 +- src/i18n/locales/es/common.json | 3 +- src/i18n/locales/fr/common.json | 3 +- src/i18n/locales/it/common.json | 3 +- src/i18n/locales/ja-JP/common.json | 3 +- src/i18n/locales/ko-KR/common.json | 3 +- src/i18n/locales/pt-BR/common.json | 3 +- src/i18n/locales/ru/common.json | 3 +- src/i18n/locales/tr/common.json | 3 +- src/i18n/locales/vi/common.json | 3 +- src/i18n/locales/zh-CN/common.json | 3 +- src/i18n/locales/zh-TW/common.json | 3 +- 15 files changed, 136 insertions(+), 63 deletions(-) diff --git a/electron/ipc/handlers.ts b/electron/ipc/handlers.ts index 0f20dd56..958ef6d9 100644 --- a/electron/ipc/handlers.ts +++ b/electron/ipc/handlers.ts @@ -1664,6 +1664,66 @@ async function resolveMediaLinksForVideo(videoPath: string): Promise<{ return { resolvedVia: "none" }; } +/** + * Writes the diagnostic bundle a bug report needs: app/OS facts, the native + * helpers' raw stdout/stderr (which is where `[stop-timing]` and + * `encoder-selection` land — see nativeWindowsCaptureStop.ts), and the main + * process's own recent console output. Shared by the renderer's IPC call and + * the menu/tray "Save Diagnostics" entry point in main.ts, which has no + * renderer-side `projectState`/`logs` to offer and does not need to. + */ +export async function exportDiagnosticFile(payload: { + error: string; + stack?: string; + projectState: unknown; + logs: string[]; +}) { + const { filePath, canceled } = await dialog.showSaveDialog({ + title: "Save Diagnostic File", + defaultPath: `openscreen-diagnostic-${Date.now()}.json`, + filters: [{ name: "JSON", extensions: ["json"] }], + }); + + if (canceled || !filePath) return { success: false, canceled: true }; + + const HELPER_OUTPUT_MAX_BYTES = 64 * 1024; + const tail = (s: string, max: number) => (s.length <= max ? s : s.slice(s.length - max)); + + const diagnostic = { + timestamp: new Date().toISOString(), + appVersion: app.getVersion(), + platform: process.platform, + arch: process.arch, + // The same fact the About box leads with, and for the same reason: it is what + // explains why a copy does or does not offer an update check. This file is the + // artifact users actually attach, so it must not be the one that omits it. + channel: getInstallChannel(), + osRelease: os.release(), + osVersion: os.version(), + totalMemoryMB: Math.round(os.totalmem() / 1024 / 1024), + nodeVersion: process.versions.node, + electronVersion: process.versions.electron, + chromeVersion: process.versions.chrome, + error: payload.error, + stack: payload.stack, + projectState: payload.projectState, + recentLogs: payload.logs, + helperOutput: { + windows: tail(nativeWindowsCaptureOutput, HELPER_OUTPUT_MAX_BYTES), + mac: tail(nativeMacCaptureOutput, HELPER_OUTPUT_MAX_BYTES), + }, + mainProcessLogs: mainLogBuffer.snapshot(), + }; + + try { + await fs.writeFile(filePath, JSON.stringify(diagnostic, null, 2), "utf-8"); + return { success: true, path: filePath }; + } catch (error) { + console.error("Failed to write diagnostic file:", error); + return { success: false, error: String(error) }; + } +} + export function registerIpcHandlers( createEditorWindow: () => void, createSourceSelectorWindow: () => BrowserWindow, @@ -4092,55 +4152,8 @@ export function registerIpcHandlers( ipcMain.handle( "save-diagnostic", - async ( - _, - payload: { error: string; stack?: string; projectState: unknown; logs: string[] }, - ) => { - const { filePath, canceled } = await dialog.showSaveDialog({ - title: "Save Diagnostic File", - defaultPath: `openscreen-diagnostic-${Date.now()}.json`, - filters: [{ name: "JSON", extensions: ["json"] }], - }); - - if (canceled || !filePath) return { success: false, canceled: true }; - - const HELPER_OUTPUT_MAX_BYTES = 64 * 1024; - const tail = (s: string, max: number) => (s.length <= max ? s : s.slice(s.length - max)); - - const diagnostic = { - timestamp: new Date().toISOString(), - appVersion: app.getVersion(), - platform: process.platform, - arch: process.arch, - // The same fact the About box leads with, and for the same reason: it is what - // explains why a copy does or does not offer an update check. This file is the - // artifact users actually attach, so it must not be the one that omits it. - channel: getInstallChannel(), - osRelease: os.release(), - osVersion: os.version(), - totalMemoryMB: Math.round(os.totalmem() / 1024 / 1024), - nodeVersion: process.versions.node, - electronVersion: process.versions.electron, - chromeVersion: process.versions.chrome, - error: payload.error, - stack: payload.stack, - projectState: payload.projectState, - recentLogs: payload.logs, - helperOutput: { - windows: tail(nativeWindowsCaptureOutput, HELPER_OUTPUT_MAX_BYTES), - mac: tail(nativeMacCaptureOutput, HELPER_OUTPUT_MAX_BYTES), - }, - mainProcessLogs: mainLogBuffer.snapshot(), - }; - - try { - await fs.writeFile(filePath, JSON.stringify(diagnostic, null, 2), "utf-8"); - return { success: true, path: filePath }; - } catch (error) { - console.error("Failed to write diagnostic file:", error); - return { success: false, error: String(error) }; - } - }, + async (_, payload: { error: string; stack?: string; projectState: unknown; logs: string[] }) => + exportDiagnosticFile(payload), ); // One instance each, not one per call. DocumentService serialises saves of a diff --git a/electron/main.ts b/electron/main.ts index 85eb063b..6f253858 100644 --- a/electron/main.ts +++ b/electron/main.ts @@ -41,7 +41,11 @@ import { } from "./globalShortcut"; import { mainT, setMainLocale } from "./i18n"; import { getInstallChannel, offersUpdateCheck, platformOwnsUpdates } from "./install-channel"; -import { getSelectedDesktopSource, registerIpcHandlers } from "./ipc/handlers"; +import { + exportDiagnosticFile, + getSelectedDesktopSource, + registerIpcHandlers, +} from "./ipc/handlers"; import { installMainProcessErrorGuards } from "./main-process-errors"; import { registerSttIpc, shutdownStt } from "./stt"; import { checkLatestRelease } from "./update-checker"; @@ -211,6 +215,11 @@ function setupApplicationMenu() { role: "about", label: mainT("common", "actions.about") || "About OpenScreen", }, + { type: "separator" as const }, + { + label: mainT("common", "actions.saveDiagnostics") || "Save Diagnostics", + click: runSaveDiagnostics, + }, // Omitted entirely — here, in the Help menu and in the tray — where a package // manager owns the update. See `canOfferUpdateCheck`. ...(canOfferUpdateCheck() @@ -369,6 +378,11 @@ function setupApplicationMenu() { label: mainT("common", "actions.about") || "About OpenScreen", click: runAboutDialog, }, + { type: "separator" as const }, + { + label: mainT("common", "actions.saveDiagnostics") || "Save Diagnostics", + click: runSaveDiagnostics, + }, ], }); } @@ -519,6 +533,31 @@ function runUpdateCheck() { }); } +/** + * Menu and tray entry point for exporting a diagnostic bundle. The backend + * (`exportDiagnosticFile`) and its "Save Diagnostics" label already existed — + * nothing in the app ever called it (getopenscreen/openscreen#460). Reveals + * the written file on success, the same confirmation the export flow's "Show + * in folder" gives, so there is no need for a second dialog on top of the + * native Save dialog the user already went through. + * + * No renderer `projectState`/`logs` to attach from here, unlike the in-app + * crash path this shares a payload shape with — the diagnostic value for a + * capture bug is almost entirely `helperOutput`/`mainProcessLogs`, which + * `exportDiagnosticFile` reads straight from the main process regardless. + */ +function runSaveDiagnostics() { + exportDiagnosticFile({ error: "Manual diagnostic export", projectState: null, logs: [] }) + .then((result) => { + if (result.success && result.path) { + shell.showItemInFolder(result.path); + } + }) + .catch((error) => { + console.error("[diagnostics] save failed", error); + }); +} + /** Mirrors the flag that already drives the tray icon. An update must never interrupt a take — * and on Windows it physically cannot, because the capture helpers spawn from inside the * install directory and NSIS cannot overwrite a running .exe. */ @@ -730,6 +769,14 @@ function updateTrayMenu(recording: boolean = false) { label: mainT("common", "actions.about") || "About OpenScreen", click: runAboutDialog, }, + // Right next to About, and reachable without opening any window: this is the + // one place in the app most likely to still be usable right after a recording + // failed to stop, which is exactly when the [stop-timing]/encoder-selection + // lines this exports are worth the most (getopenscreen/openscreen#460). + { + label: mainT("common", "actions.saveDiagnostics") || "Save Diagnostics", + click: runSaveDiagnostics, + }, { type: "separator" as const }, { label: mainT("common", "actions.quit") || "Quit", diff --git a/src/i18n/locales/ar/common.json b/src/i18n/locales/ar/common.json index ef0d6736..b63cd026 100644 --- a/src/i18n/locales/ar/common.json +++ b/src/i18n/locales/ar/common.json @@ -40,7 +40,8 @@ "services": "خدمات", "hide": "إخفاء OpenScreen", "hideOthers": "إخفاء الآخرين", - "unhide": "إظهار الكل" + "unhide": "إظهار الكل", + "saveDiagnostics": "حفظ التشخيصات" }, "updates": { "available": "يتوفر OpenScreen {{latestVersion}}. الإصدار المثبت هو {{currentVersion}}.", diff --git a/src/i18n/locales/en/common.json b/src/i18n/locales/en/common.json index 4f303ee6..6eac9583 100644 --- a/src/i18n/locales/en/common.json +++ b/src/i18n/locales/en/common.json @@ -40,7 +40,8 @@ "services": "Services", "hide": "Hide OpenScreen", "hideOthers": "Hide Others", - "unhide": "Show All" + "unhide": "Show All", + "saveDiagnostics": "Save Diagnostics" }, "updates": { "available": "OpenScreen {{latestVersion}} is available. You are using {{currentVersion}}.", diff --git a/src/i18n/locales/es/common.json b/src/i18n/locales/es/common.json index 3c92583a..29a36426 100644 --- a/src/i18n/locales/es/common.json +++ b/src/i18n/locales/es/common.json @@ -40,7 +40,8 @@ "services": "Servicios", "hide": "Ocultar OpenScreen", "hideOthers": "Ocultar otros", - "unhide": "Mostrar todo" + "unhide": "Mostrar todo", + "saveDiagnostics": "Guardar diagnósticos" }, "updates": { "available": "OpenScreen {{latestVersion}} está disponible. Estás usando {{currentVersion}}.", diff --git a/src/i18n/locales/fr/common.json b/src/i18n/locales/fr/common.json index de72f313..c74ef66b 100644 --- a/src/i18n/locales/fr/common.json +++ b/src/i18n/locales/fr/common.json @@ -40,7 +40,8 @@ "services": "Services", "hide": "Masquer OpenScreen", "hideOthers": "Masquer les autres", - "unhide": "Tout afficher" + "unhide": "Tout afficher", + "saveDiagnostics": "Enregistrer les diagnostics" }, "updates": { "available": "OpenScreen {{latestVersion}} est disponible. Vous utilisez la version {{currentVersion}}.", diff --git a/src/i18n/locales/it/common.json b/src/i18n/locales/it/common.json index df18ace1..24f10fc6 100644 --- a/src/i18n/locales/it/common.json +++ b/src/i18n/locales/it/common.json @@ -40,7 +40,8 @@ "services": "Servizi", "hide": "Nascondi OpenScreen", "hideOthers": "Nascondi gli altri", - "unhide": "Mostra tutto" + "unhide": "Mostra tutto", + "saveDiagnostics": "Salva dati diagnostici" }, "updates": { "available": "OpenScreen {{latestVersion}} è disponibile. Stai usando la versione {{currentVersion}}.", diff --git a/src/i18n/locales/ja-JP/common.json b/src/i18n/locales/ja-JP/common.json index 88899975..0d3ca001 100644 --- a/src/i18n/locales/ja-JP/common.json +++ b/src/i18n/locales/ja-JP/common.json @@ -40,7 +40,8 @@ "services": "サービス", "hide": "OpenScreenを隠す", "hideOthers": "ほかを隠す", - "unhide": "すべて表示" + "unhide": "すべて表示", + "saveDiagnostics": "診断情報を保存" }, "updates": { "available": "OpenScreen {{latestVersion}} を利用できます。現在のバージョンは {{currentVersion}} です。", diff --git a/src/i18n/locales/ko-KR/common.json b/src/i18n/locales/ko-KR/common.json index 964fdd63..2ca4d25a 100644 --- a/src/i18n/locales/ko-KR/common.json +++ b/src/i18n/locales/ko-KR/common.json @@ -40,7 +40,8 @@ "services": "서비스", "hide": "OpenScreen 숨기기", "hideOthers": "다른 항목 숨기기", - "unhide": "모두 보기" + "unhide": "모두 보기", + "saveDiagnostics": "Save Diagnostics" }, "updates": { "available": "OpenScreen {{latestVersion}} 버전을 사용할 수 있습니다. 현재 버전은 {{currentVersion}}입니다.", diff --git a/src/i18n/locales/pt-BR/common.json b/src/i18n/locales/pt-BR/common.json index af25d126..d74e5b21 100644 --- a/src/i18n/locales/pt-BR/common.json +++ b/src/i18n/locales/pt-BR/common.json @@ -40,7 +40,8 @@ "services": "Serviços", "hide": "Ocultar OpenScreen", "hideOthers": "Ocultar Outros", - "unhide": "Mostrar Todos" + "unhide": "Mostrar Todos", + "saveDiagnostics": "Salvar Diagnósticos" }, "updates": { "available": "O OpenScreen {{latestVersion}} está disponível. Você está usando a versão {{currentVersion}}.", diff --git a/src/i18n/locales/ru/common.json b/src/i18n/locales/ru/common.json index d7ec15ae..141e8b98 100644 --- a/src/i18n/locales/ru/common.json +++ b/src/i18n/locales/ru/common.json @@ -40,7 +40,8 @@ "services": "Сервисы", "hide": "Скрыть OpenScreen", "hideOthers": "Скрыть остальные", - "unhide": "Показать все" + "unhide": "Показать все", + "saveDiagnostics": "Сохранить диагностику" }, "updates": { "available": "Доступен OpenScreen {{latestVersion}}. Установлена версия {{currentVersion}}.", diff --git a/src/i18n/locales/tr/common.json b/src/i18n/locales/tr/common.json index e36c2dc3..923079b1 100644 --- a/src/i18n/locales/tr/common.json +++ b/src/i18n/locales/tr/common.json @@ -40,7 +40,8 @@ "services": "Servisler", "hide": "OpenScreen’i Gizle", "hideOthers": "Diğerlerini Gizle", - "unhide": "Tümünü Göster" + "unhide": "Tümünü Göster", + "saveDiagnostics": "Teşhis Verilerini Kaydet" }, "updates": { "available": "OpenScreen {{latestVersion}} kullanılabilir. Mevcut sürümünüz {{currentVersion}}.", diff --git a/src/i18n/locales/vi/common.json b/src/i18n/locales/vi/common.json index 544cf3b8..089cf992 100644 --- a/src/i18n/locales/vi/common.json +++ b/src/i18n/locales/vi/common.json @@ -40,7 +40,8 @@ "services": "Dịch vụ", "hide": "Ẩn OpenScreen", "hideOthers": "Ẩn ứng dụng khác", - "unhide": "Hiển thị tất cả" + "unhide": "Hiển thị tất cả", + "saveDiagnostics": "Lưu thông tin chẩn đoán" }, "updates": { "available": "Đã có OpenScreen {{latestVersion}}. Bạn đang dùng phiên bản {{currentVersion}}.", diff --git a/src/i18n/locales/zh-CN/common.json b/src/i18n/locales/zh-CN/common.json index 30b018cc..0a7b2bd9 100644 --- a/src/i18n/locales/zh-CN/common.json +++ b/src/i18n/locales/zh-CN/common.json @@ -40,7 +40,8 @@ "services": "服务", "hide": "隐藏 OpenScreen", "hideOthers": "隐藏其他", - "unhide": "显示全部" + "unhide": "显示全部", + "saveDiagnostics": "保存诊断信息" }, "updates": { "available": "OpenScreen {{latestVersion}} 已发布。当前版本为 {{currentVersion}}。", diff --git a/src/i18n/locales/zh-TW/common.json b/src/i18n/locales/zh-TW/common.json index 07d66259..c54dc17e 100644 --- a/src/i18n/locales/zh-TW/common.json +++ b/src/i18n/locales/zh-TW/common.json @@ -40,7 +40,8 @@ "services": "服務", "hide": "隱藏 OpenScreen", "hideOthers": "隱藏其他", - "unhide": "全部顯示" + "unhide": "全部顯示", + "saveDiagnostics": "儲存診斷資料" }, "updates": { "available": "OpenScreen {{latestVersion}} 已推出。目前版本為 {{currentVersion}}。", From 401be4b01b620522a2adcc7651ef721550708191 Mon Sep 17 00:00:00 2001 From: EtienneLescot Date: Sun, 23 Aug 2026 23:17:23 +0200 Subject: [PATCH 2/3] fix(app): address CodeRabbit review on the Save Diagnostics PR Three findings, all confirmed against current code: - runSaveDiagnostics silently did nothing when exportDiagnosticFile resolved with success:false (a write failure after the user already picked a save location) -- it only handled the success and implicit-reject cases, so a real failure read as the menu action doing nothing. Now shows an error dialog with the underlying message as detail, cancellation still a no-op. - detectVideoEncoderRuntime's doc comment in mf_encoder.cpp still said the default path asks for no hardware-transform attribute at all, which was true when it was written but stopped being true once the default path started requesting MF_READWRITE_ENABLE_HARDWARE_TRANSFORMS. Updated to say what's actually true now: it's a request Media Foundation can still answer with software, which is why the runtime still has to be checked after the fact rather than assumed from the path. - ko-KR's actions.saveDiagnostics carried the English label because it was copied from settings.support.saveDiagnostics, which was itself never translated for Korean. Applied CodeRabbit's suggested translation. Verified: tsc --noEmit clean, biome clean, i18n:check passes, native helper rebuilds clean on MSVC, full suite (2161 tests) passes. Co-Authored-By: Claude Sonnet 5 --- electron/main.ts | 18 +++++++++++++++- .../native/wgc-capture/src/mf_encoder.cpp | 21 ++++++++++--------- src/i18n/locales/ko-KR/common.json | 2 +- 3 files changed, 29 insertions(+), 12 deletions(-) diff --git a/electron/main.ts b/electron/main.ts index 6f253858..a85629bf 100644 --- a/electron/main.ts +++ b/electron/main.ts @@ -549,7 +549,23 @@ function runUpdateCheck() { function runSaveDiagnostics() { exportDiagnosticFile({ error: "Manual diagnostic export", projectState: null, logs: [] }) .then((result) => { - if (result.success && result.path) { + if (result.canceled) return; + if (!result.success) { + // exportDiagnosticFile resolves rather than rejects on a write + // failure, so this is the branch that turns "user picked a save + // location and got silence" into a visible error instead of a + // menu action that looks like it did nothing. + showMessageBox({ + type: "error", + title: PRODUCT_NAME, + message: mainT("dialogs", "export.failed") || "Export Failed", + detail: result.error, + }).catch((error) => { + console.error("[diagnostics] failure dialog failed", error); + }); + return; + } + if (result.path) { shell.showItemInFolder(result.path); } }) diff --git a/electron/native/wgc-capture/src/mf_encoder.cpp b/electron/native/wgc-capture/src/mf_encoder.cpp index c1e5c2bb..4130b132 100644 --- a/electron/native/wgc-capture/src/mf_encoder.cpp +++ b/electron/native/wgc-capture/src/mf_encoder.cpp @@ -406,16 +406,17 @@ bool resolveStreamSinkIndex(IMFMediaSink* mediaSink, const GUID& majorType, DWOR // Did the video stream's encoder MFT actually land on hardware? // -// BeginWriting() succeeding says nothing about this: on the "default" path -// (see kVideoEncoderRuntime* in mf_encoder.h) no attribute asked for hardware -// transforms, so Media Foundation is free to hand the sink writer a software -// MFT even when a hardware one is registered and would have worked. The only -// way to know which one it actually picked is to ask the pipeline it built, -// after the fact -- IMFSinkWriterEx::GetTransformForStream walks the MFTs the -// sink writer inserted for a stream, and a hardware MFT instance is required -// to expose MFT_ENUM_HARDWARE_URL_Attribute on its own attribute store (not -// just on the IMFActivate MFTEnumEx returns), which is what distinguishes it -// from a software one at this point. +// BeginWriting() succeeding says nothing about this: even on the "default" +// path (see kVideoEncoderRuntime* in mf_encoder.h), which does now ask for +// MF_READWRITE_ENABLE_HARDWARE_TRANSFORMS, that is a request and not a +// guarantee -- Media Foundation is still free to hand the sink writer a +// software MFT when no hardware one is registered or the driver refuses it. +// The only way to know which one it actually picked is to ask the pipeline it +// built, after the fact -- IMFSinkWriterEx::GetTransformForStream walks the +// MFTs the sink writer inserted for a stream, and a hardware MFT instance is +// required to expose MFT_ENUM_HARDWARE_URL_Attribute on its own attribute +// store (not just on the IMFActivate MFTEnumEx returns), which is what +// distinguishes it from a software one at this point. // // Every failure path here returns "unknown" rather than guessing: this runs // after the sink writer is already committed to, so it must never be able to diff --git a/src/i18n/locales/ko-KR/common.json b/src/i18n/locales/ko-KR/common.json index 2ca4d25a..98d025d9 100644 --- a/src/i18n/locales/ko-KR/common.json +++ b/src/i18n/locales/ko-KR/common.json @@ -41,7 +41,7 @@ "hide": "OpenScreen 숨기기", "hideOthers": "다른 항목 숨기기", "unhide": "모두 보기", - "saveDiagnostics": "Save Diagnostics" + "saveDiagnostics": "진단 정보 저장" }, "updates": { "available": "OpenScreen {{latestVersion}} 버전을 사용할 수 있습니다. 현재 버전은 {{currentVersion}}입니다.", From e76fb96f547fdf3affaac12de28222c50f64a243 Mon Sep 17 00:00:00 2001 From: EtienneLescot Date: Mon, 24 Aug 2026 10:34:42 +0200 Subject: [PATCH 3/3] fix(recording): stop burning the step budget on a join that cannot succeed Pinned down the exact mechanism behind #460 on Intel HD 520, confirmed by a reporter's Save Diagnostics file on rc.4: the WGC frame callback (main.cpp's session.setFrameCallback) takes the shared frame-state `mutex` and calls session.context()->CopyResource() while still holding it. On this hardware that CopyResource hangs inside the driver. writeVideoFrames() needs the same mutex for its own per-iteration wait -- including to notice stopRequested -- so once the callback wedges, the writer thread can never even check whether a stop was requested. That is why the watchdog reported encode_stage=idle: not idle, blocked on a lock a stuck GPU call holds forever. quiesceCapture() already detects this and gives up after its own 5s drain, returning wgcDrained=false. Nothing downstream listened: video-writer-join called stopVideoWriter() unconditionally, joining a thread that structurally could never return, and paid the full step budget (8s default) before the watchdog force-exited the process anyway -- the same outcome the fix below reaches, just ~8s later. When wgcDrained is false, detach the thread and terminate immediately rather than falling through to a join that cannot succeed. Deliberately does not continue into encoder.finalize(): that resets the D3D device/context state a still-blocked writer thread might resume touching the moment the lock frees. No data is lost either way -- the fragmented sink writes moof+mdat incrementally, so whatever was on disk before the wedge is on disk regardless of which path gets there. Added a new fault-injection point (OPENSCREEN_WGC_TEST_STALL_FRAME_CALLBACK_MS) to reproduce the exact failure shape and verify the fix rather than trust it compiles. Along the way, found that stalling the *first* frame trips an unrelated 10s startup timeout before ever reaching this code path -- the stall has to land on a later frame, matching what the real diagnostic showed (recording-started succeeded before the hang). Also found that moving the cursor alone does not reliably force a WGC frame on this machine (likely hardware cursor compositing bypassing the desktop bitmap); a moving window does. Measured: 5075ms to exit with the fix, versus what would have been ~13000ms (5s drain + 8s step budget) without it. Confirmed via the process exiting right at video-writer-join, with no encoder-finalize/wgc-session-close in the steps afterward. The pre-existing --stall-readback (#252) regression test is unaffected -- that scenario stalls the writer's own readback, not the frame callback, so it never touches this branch. This does not fix the underlying driver hang, which needs the actual failing hardware to diagnose further. It gets the user a faster, honest failure instead of a long one; the recording is still lost when the driver wedges. Verified: tsc --noEmit clean, biome clean, native helper rebuilds clean on MSVC, full suite (2161 tests) passes, both stall regression tests pass. Co-Authored-By: Claude Sonnet 5 --- electron/native/wgc-capture/src/main.cpp | 69 +++++++++++++- scripts/test-windows-wgc-helper.mjs | 113 ++++++++++++++++++++++- 2 files changed, 178 insertions(+), 4 deletions(-) diff --git a/electron/native/wgc-capture/src/main.cpp b/electron/native/wgc-capture/src/main.cpp index c64d7970..a9e21d45 100644 --- a/electron/native/wgc-capture/src/main.cpp +++ b/electron/native/wgc-capture/src/main.cpp @@ -682,6 +682,16 @@ int main(int argc, char* argv[]) { // ordinary hardware, so the stop path can be regression-tested at all. const int testStallReadbackMs = std::max(0, readEnvInt("OPENSCREEN_WGC_TEST_STALL_READBACK_MS", 0)); + // Test-only: stall the WGC frame *callback* itself while it holds the + // same frame lock, rather than the writer's readback -- the shape + // getopenscreen/openscreen#460 actually reproduced on Intel HD 520 + // ("A WGC frame callback did not finish"). Distinct from + // testStallReadbackMs above because quiesceCapture()'s drain only ever + // sees the callback side: a stall placed in the writer instead leaves + // callbacksInFlight_ at zero and wgcDrained true, which cannot exercise + // the video-writer-join skip this stall exists to test. + const int testStallFrameCallbackMs = + std::max(0, readEnvInt("OPENSCREEN_WGC_TEST_STALL_FRAME_CALLBACK_MS", 0)); std::cout << "{\"event\":\"ready\",\"schemaVersion\":2}" << std::endl; @@ -931,6 +941,18 @@ int main(int argc, char* argv[]) { } } + // Gated on an already-arrived first frame: main() blocks up to 10s + // waiting for firstFrameWritten before it will even print + // recording-started, a startup budget this stall is meant to outlast + // (it needs to still be asleep when `stop` arrives, seconds later). + // Stalling the first frame trips that unrelated timeout instead of + // reaching the steady-state shutdown path this exists to test, and + // does not match the real report either -- getopenscreen/openscreen + // #460's diagnostic shows recording-started succeeding before the + // hang. + if (testStallFrameCallbackMs > 0 && firstFrameWritten.load()) { + std::this_thread::sleep_for(std::chrono::milliseconds(testStallFrameCallbackMs)); + } session.context()->CopyResource(latestFrameTexture.Get(), texture); latestFrameTimestampHns = timestampHns; if (!firstFrameWritten.exchange(true)) { @@ -1420,8 +1442,47 @@ int main(int argc, char* argv[]) { } logStopStep("audio-mixer"); beginStopStep("video-writer-join", stepBudgetMs); - stopVideoWriter(); - logStopStep("video-writer-join"); + if (wgcDrained) { + stopVideoWriter(); + logStopStep("video-writer-join"); + } else { + // wgc-quiesce already reported the frame callback stuck inside the + // driver (getopenscreen/openscreen#460 on Intel HD 520: a + // CopyResource that never returns), still holding the same + // frame-state `mutex` writeVideoFrames takes for its own + // per-iteration wait -- the one it also needs to notice + // stopRequested. Joining is not a step that can time out here, it is + // one that cannot ever succeed, and this is not the only step that + // assumed it would: encoder.finalize() below resets the very D3D + // device/context a still-blocked writer thread might resume touching + // the moment that lock frees, and quiesceCapture()/stop() already + // treat "leave everything alone and let process exit reclaim it" as + // the only safe response to exactly this state. So this ends the + // process here, on this thread, rather than pretending the rest of a + // clean shutdown is reachable -- which cost nothing extra before + // today: the same TerminateProcess happened anyway, just + // stepBudgetMs later, once this step's own watchdog gave up waiting + // on a join that could never return. detach() first, not because + // TerminateProcess needs it (it does not touch the C++ runtime, no + // std::thread destructor runs), but so nothing between here and the + // kill can trip over a still-joinable thread. + // + // The fragmented sink writes moof+mdat incrementally, roughly once a + // second, so this is not a new source of loss: whatever was already + // on disk before the callback wedged is on disk regardless of + // whether Finalize() ever runs, on this path or the slower one it + // replaces. + videoWriterThread.detach(); + std::cerr << "[stop-timing] step=video-writer-join elapsed_ms=" << stopElapsedMs() + << " phase=abandoned encode_stage=" << encoder.encodeStage() + << " audio_stage=" << encoder.audioStage() << " reason=frame-callback-stuck" + << std::endl; + std::cout << "{\"event\":\"stop-timeout\",\"schemaVersion\":2,\"step\":\"video-writer-join\"}" + << std::endl; + std::cout.flush(); + std::cerr.flush(); + TerminateProcess(GetCurrentProcess(), 3); + } if (usesDxgiInput) { std::cerr << "[frame-drops] gpu_bridge_contended=" << contendedFrames.load() << std::endl; } @@ -1430,7 +1491,9 @@ int main(int argc, char* argv[]) { // 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. + // the staging texture -- do not reorder these. Reaching this line at all + // means wgcDrained was true above: the branch that was not is a + // TerminateProcess call, not a fallthrough. beginStopStep("encoder-finalize", shutdownBudgetMs); const bool screenFinalized = encoder.finalize(); logStopStep("encoder-finalize"); diff --git a/scripts/test-windows-wgc-helper.mjs b/scripts/test-windows-wgc-helper.mjs index 1e513c72..ee40838e 100644 --- a/scripts/test-windows-wgc-helper.mjs +++ b/scripts/test-windows-wgc-helper.mjs @@ -45,6 +45,20 @@ 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 STALL_FRAME_CALLBACK_ENV = "OPENSCREEN_WGC_TEST_STALL_FRAME_CALLBACK_MS"; +/** + * Reproduces getopenscreen/openscreen#460 on ordinary hardware: stalls the WGC + * frame *callback* itself while it holds the frame lock, the shape that issue + * actually reproduced on Intel HD 520 ("A WGC frame callback did not finish"). + * Distinct from WITH_STALLED_READBACK above -- that stalls the writer's own + * readback, which quiesceCapture()'s drain cannot see (callbacksInFlight_ + * stays at zero), so it cannot exercise the video-writer-join skip this stall + * exists to test. + */ +const WITH_STALLED_FRAME_CALLBACK = + process.env.OPENSCREEN_WGC_TEST_STALL_FRAME_CALLBACK === "true" || + process.argv.includes("--stall-frame-callback"); +const STALL_FRAME_CALLBACK_MS = Number(process.env[STALL_FRAME_CALLBACK_ENV] ?? 60_000); const STOP_BUDGET_ENV = "OPENSCREEN_WGC_STOP_BUDGET_MS"; /** * The helper's global shutdown ceiling, pinned into its environment below so @@ -64,11 +78,15 @@ if (WITH_SOFTWARE_ENCODER && WITH_SOFTWARE_FALLBACK) { throw new Error("--software-encoder and --software-fallback are mutually exclusive"); } -function runHelper(config, { injectDefaultSinkWriterFailure = false, stallReadbackMs = 0 } = {}) { +function runHelper( + config, + { injectDefaultSinkWriterFailure = false, stallReadbackMs = 0, stallFrameCallbackMs = 0 } = {}, +) { return new Promise((resolve, reject) => { const env = { ...process.env }; delete env[INJECT_DEFAULT_SINK_WRITER_FAILURE_ENV]; delete env[STALL_READBACK_ENV]; + delete env[STALL_FRAME_CALLBACK_ENV]; env[STOP_BUDGET_ENV] = String(STOP_BUDGET_MS); if (injectDefaultSinkWriterFailure) { env[INJECT_DEFAULT_SINK_WRITER_FAILURE_ENV] = "1"; @@ -76,6 +94,9 @@ function runHelper(config, { injectDefaultSinkWriterFailure = false, stallReadba if (stallReadbackMs > 0) { env[STALL_READBACK_ENV] = String(stallReadbackMs); } + if (stallFrameCallbackMs > 0) { + env[STALL_FRAME_CALLBACK_ENV] = String(stallFrameCallbackMs); + } const child = spawn(HELPER_PATH, [JSON.stringify(config)], { env, stdio: ["pipe", "pipe", "pipe"], @@ -213,6 +234,48 @@ function startFixtureWindow() { }); } +/** + * Windows Graphics Capture delivers frames on compositor damage, not on a + * fixed clock -- on a genuinely idle desktop the frame pool can go a full + * test run without ever firing FrameArrived once. That is invisible to most + * of this harness, which just needs *a* frame eventually, but the + * stalled-frame-callback regression check needs one to land *inside* the + * DURATION_MS window specifically, so the stall this injects is actually the + * thing holding the frame lock when `stop` arrives. + * + * Moving the cursor alone does not reliably do this: most modern GPU/driver + * combinations composite the cursor on its own hardware overlay plane, so + * repositioning it never touches the desktop bitmap WGC captures (confirmed + * empirically here -- frames=0 with a cursor-only nudge running the whole + * test). A visible window changing position is not optional the way the + * cursor is; DWM has to redraw the area it moved across. Returns a stop + * function; always call it, paired failure or not, or the window and its + * PowerShell host outlive the test process. + */ +function startScreenActivity() { + const child = spawn( + "powershell", + [ + "-NoProfile", + "-Command", + "Add-Type -AssemblyName System.Windows.Forms; " + + "$f = New-Object System.Windows.Forms.Form; " + + "$f.StartPosition = 'Manual'; $f.Location = New-Object System.Drawing.Point(0,0); " + + "$f.Size = New-Object System.Drawing.Size(200,200); " + + "$f.TopMost = $true; $f.Show(); " + + "$x = 0; " + + "while ($true) { " + + "$f.Location = New-Object System.Drawing.Point($x, 0); " + + "$x = ($x + 20) % 200; " + + "[System.Windows.Forms.Application]::DoEvents(); " + + "Start-Sleep -Milliseconds 100; " + + "}", + ], + { stdio: ["ignore", "ignore", "ignore"], windowsHide: false }, + ); + return () => child.kill(); +} + function normalizeDeviceName(value) { return value .toLowerCase() @@ -413,16 +476,19 @@ const config = { }, }; +const stopScreenActivity = WITH_STALLED_FRAME_CALLBACK ? startScreenActivity() : null; let result; try { result = await runHelper(config, { injectDefaultSinkWriterFailure: WITH_SOFTWARE_FALLBACK, stallReadbackMs: WITH_STALLED_READBACK ? STALL_READBACK_MS : 0, + stallFrameCallbackMs: WITH_STALLED_FRAME_CALLBACK ? STALL_FRAME_CALLBACK_MS : 0, }); } finally { if (fixtureWindow) { fixtureWindow.child.kill(); } + stopScreenActivity?.(); } // The regression check for issue #252. With the frame lock deliberately wedged @@ -453,6 +519,51 @@ if (WITH_STALLED_READBACK) { process.exit(0); } +// The regression check for getopenscreen/openscreen#460: a frame callback +// wedged inside the driver, confirmed on real hardware via a Save Diagnostics +// report. Before the fix, video-writer-join burned its whole step budget +// joining a thread parked behind that same stuck callback -- this asserts +// both that the helper still exits promptly (not the ~13s that step's own +// budget alone would cost) and that it took the specific skip path rather +// than any other route to exiting. +if (WITH_STALLED_FRAME_CALLBACK) { + if (result.stopHung) { + throw new Error( + `Helper survived ${STOP_HANG_LIMIT_MS}ms past "stop" with a stalled frame callback. ` + + "Its shutdown watchdog did not fire (issue #460).", + ); + } + const steps = readStopTimingSteps(result.stderr); + if (!steps.includes("command-received")) { + throw new Error(`Helper never acknowledged "stop". Steps seen: ${steps.join(", ") || "none"}`); + } + if (!result.stderr.includes("reason=frame-callback-stuck")) { + throw new Error( + `Helper did not take the video-writer-join skip path. stderr:\n${result.stderr}`, + ); + } + // wgc-quiesce's own drain is a fixed 5000ms, so a healthy skip lands + // there plus the near-instant audio/microphone/webcam steps -- nowhere + // near the ~13s (5s drain + the 8s step budget) the join it replaces + // would have cost before this fix. + const STALLED_FRAME_CALLBACK_LATENCY_BUDGET_MS = 10_000; + if ( + result.stopLatencyMs !== null && + result.stopLatencyMs > STALLED_FRAME_CALLBACK_LATENCY_BUDGET_MS + ) { + throw new Error( + `Stop took ${result.stopLatencyMs}ms with a stalled frame callback, over the ` + + `${STALLED_FRAME_CALLBACK_LATENCY_BUDGET_MS}ms budget the video-writer-join skip should keep it under.`, + ); + } + console.log("WGC helper stalled-frame-callback stop check passed", { + stopLatencyMs: result.stopLatencyMs, + steps, + }); + fs.rmSync(outputPath, { force: true }); + process.exit(0); +} + assertStopWasClean(result); if (result.code !== 0) {