Add desktop/server MP4 export pipeline, hardening, and render tests (v1.3.0)
Desktop MP4 export renders frames from a hidden BrowserWindow via capturePage, pipes PNGs to a bundled ffmpeg (libx264/yuv420p/+faststart), and writes the file through an atomic temp-file swap. It shares scene validation (shared/renderValidation.cjs) and the render.html renderer with the standalone Puppeteer server, which is hardened in parallel. Render worker & main (electron/): - abortable render loop with an inactivity watchdog and cancel plumbing - atomic temp-file + rename so a cancel/failure never truncates a pre-existing target (#7); stdin EPIPE / early-exit / backpressure-hang handled (#8) - HiDPI frames normalized to canvas size (#10); odd dimensions padded to even (#13, also server); backgroundThrottling disabled (#12) - reject canvases larger than the display (#11); single-render guard; isolated render session that blocks network requests (#5) Tests: - Electron harnesses (scripts/verify-*.cjs) for capture, export, cancel, timeout, assets, background, capture-fidelity, resolution, odd-dims, aggregated by verify-desktop-render.cjs - server smoke + e2e and shared validation unit tests - headless hook (src/devtools/headless.ts) to run the in-app suite under browser/electron hosts; pixel-diff util + capture fixtures (test/) - dev_docs plans: integration, fixes (#7-#13), and the e2e/capture strategy Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
thandClaude Opus 4.8 committed
1 parent
46f9f32aef
commit
518d43855b
41 files changed
+3907
-86
No files matched your search
@@ -0,0 +1,356 @@
|
||||
# Electron Rendering — Fix Plan (Second-Review Findings #7–#13)
|
||||
|
||||
Date: 2026-06-06
|
||||
|
||||
This document is a follow-on to `electron-rendering-integration-plan_2.md`.
|
||||
That plan tracked findings **#1–#6** (all resolved). A second review of the
|
||||
uncommitted desktop-render changes surfaced a new set of issues, numbered
|
||||
**#7–#13** here to avoid collision. Two of them (#7, #8) are user-facing data
|
||||
loss / crash bugs and should be fixed before any GUI/packaging sign-off.
|
||||
|
||||
> **Status (2026-06-06): all of #7–#13 are now IMPLEMENTED and verified.**
|
||||
> #10 and #11 took the interim-mitigation path (per-frame resize back to canvas
|
||||
> size + fail-fast on canvases larger than the display), not the offscreen
|
||||
> rewrite; #13 pads odd dimensions up to even on both render paths. All six
|
||||
> `test:render-desktop` checks pass, as do the server smoke + e2e tests; #7, #8,
|
||||
> and #13 were additionally verified with throwaway harnesses (overwrite safety;
|
||||
> early-ffmpeg-exit surfaces an error without crash or hang; a 101×101 canvas
|
||||
> encodes to a clean 102×102 MP4). See the per-finding "Status" notes below.
|
||||
|
||||
> Note: plan_2's "What is already correct" section claims *"Partial output file
|
||||
> is removed on failure/cancel"* as a safety property. That is precisely the
|
||||
> mechanism behind finding **#7** — the worker writes ffmpeg output straight to
|
||||
> the user's chosen path, so "removing the partial file" deletes a pre-existing
|
||||
> file the user chose to overwrite. The note should be corrected when #7 lands.
|
||||
|
||||
## Summary table
|
||||
|
||||
| # | Sev | File | One-line |
|
||||
| --- | --- | --- | --- |
|
||||
| 7 | CRITICAL | `electron/renderWorker.cts` | Direct-to-target ffmpeg write + `fs.rm` destroys a pre-existing file on cancel/ffmpeg failure (data loss). |
|
||||
| 8 | HIGH | `electron/renderWorker.cts` | No `error` listener on `ffmpeg.stdin`; an early ffmpeg exit makes the next `write()` throw an uncaught EPIPE that crashes the main process. |
|
||||
| 9 | HIGH (trivial) | `package.json` | `"packageManager": "traversal@1.0.0"` is a bogus value; breaks corepack-driven tooling. |
|
||||
| 10 | MEDIUM | `electron/renderWorker.cts` | `capturePage` returns frames at the display scale factor → HiDPI output is N× the requested resolution. |
|
||||
| 11 | MEDIUM | `electron/renderWorker.cts` | A non-offscreen render window can't be captured larger than the display → canvases > screen are truncated. |
|
||||
| 12 | MEDIUM (perf) | `electron/renderWorker.cts` + `server/render.html` | Hidden-window rAF is throttled, so `__animatrSeek` hits its 1 s fallback every frame → renders crawl at ~1 fps. |
|
||||
| 13 | LOW (optional) | `electron/renderWorker.cts` + `server/server.js` | Odd canvas width/height fails `yuv420p`/libx264 (shared latent bug). |
|
||||
|
||||
Recommended sequencing: **#9 (1 line)** → **#7, #8 (correctness)** →
|
||||
**#12 (one line, big perf win)** → decide the **#10/#11 strategy (offscreen vs.
|
||||
mitigations)** → **#13 (optional hardening)**.
|
||||
|
||||
---
|
||||
|
||||
## #7 — CRITICAL: desktop export destroys an existing file on cancel/failure
|
||||
|
||||
> **Status: IMPLEMENTED.** Worker renders to a sibling
|
||||
> `.<name>.<jobId>.partial` file and promotes it with `rename` (rm-then-rename
|
||||
> fallback on Windows) only after `ffmpegDone` resolves; the failure/cancel path
|
||||
> only ever removes the temp file. One extra change was required: ffmpeg infers
|
||||
> its muxer from the output extension, and the temp file has none, so the spawn
|
||||
> now passes an explicit `-f mp4` (this also hardens a final path the user typed
|
||||
> without a `.mp4` extension). Verified: cancel mid-render leaves a pre-existing
|
||||
> sentinel file byte-identical with no leftover `.partial`.
|
||||
|
||||
### Problem
|
||||
`renderMp4Job` spawns ffmpeg writing **directly** to the user-chosen path:
|
||||
|
||||
```ts
|
||||
const ffmpeg = spawn(ffmpegPath, [ '-y', ..., input.outputPath ])
|
||||
```
|
||||
|
||||
and on any error/cancel does:
|
||||
|
||||
```ts
|
||||
await fs.rm(input.outputPath, { force: true }).catch(() => {})
|
||||
```
|
||||
|
||||
`ffmpeg -y <outputPath>` opens and **truncates** the target the instant it
|
||||
starts. If the user chose to overwrite an existing file (the native save dialog
|
||||
explicitly offers this), then any of:
|
||||
- user clicks Cancel mid-render,
|
||||
- the inactivity watchdog fires,
|
||||
- ffmpeg exits non-zero,
|
||||
|
||||
leaves the original file already truncated, and the catch block then deletes it.
|
||||
**Net result: the user's pre-existing file is gone and there is no replacement.**
|
||||
The standalone server never has this problem because it renders into a private
|
||||
`mkdtemp` workDir and only exposes the finished file.
|
||||
|
||||
### Fix (render to a sibling temp file, promote on success)
|
||||
Write to a temp file in the **same directory** as the target (same filesystem ⇒
|
||||
`rename` is atomic and can't hit `EXDEV`), and only touch the real path once the
|
||||
render has fully succeeded:
|
||||
|
||||
```ts
|
||||
const finalPath = input.outputPath
|
||||
const tmpPath = path.join(
|
||||
path.dirname(finalPath),
|
||||
`.${path.basename(finalPath)}.${input.jobId}.partial`,
|
||||
)
|
||||
// spawn ffmpeg with tmpPath instead of finalPath
|
||||
|
||||
// success path, after `await ffmpegDone`:
|
||||
await fs.rm(finalPath, { force: true }) // Windows rename won't replace an existing dest
|
||||
await fs.rename(tmpPath, finalPath)
|
||||
return { path: finalPath }
|
||||
|
||||
// catch path:
|
||||
await fs.rm(tmpPath, { force: true }).catch(() => {}) // NEVER finalPath
|
||||
```
|
||||
|
||||
Notes:
|
||||
- The `rm(finalPath)` before `rename` only runs after a **successful** render
|
||||
(tmp file is complete), so the old file is replaced, never lost.
|
||||
- Keep `'-movflags', '+faststart'` — ffmpeg still finalizes into `tmpPath`.
|
||||
- Optionally sweep stale `.*.partial` files in the target dir on worker start
|
||||
(best-effort), to cover hard crashes.
|
||||
|
||||
### Verification
|
||||
- New `scripts/verify-overwrite.cjs`: pre-create a sentinel file at the target
|
||||
path, start an export to it, cancel mid-render, assert the **sentinel bytes
|
||||
are unchanged** and no `.partial` file remains.
|
||||
- Re-run `scripts/verify-cancel.cjs` (already passing) — must stay green.
|
||||
|
||||
---
|
||||
|
||||
## #8 — HIGH: an early ffmpeg exit crashes/hangs the worker
|
||||
|
||||
> **Status: IMPLEMENTED.** The investigation found the EPIPE was only one of
|
||||
> three failure modes when ffmpeg exits mid-render; all three are now fixed:
|
||||
> 1. **EPIPE crash** — added `ffmpeg.stdin.on('error', …)` so a write to the
|
||||
> dead pipe can't become an uncaughtException.
|
||||
> 2. **Unhandled-rejection crash** — `ffmpegDone` rejects mid-loop, long before
|
||||
> the post-loop `await` attaches a handler; under Node's default
|
||||
> `--unhandled-rejections=throw` that terminated the process. Now a no-op
|
||||
> handler is attached at creation (`void ffmpegDone.catch(() => {})`); real
|
||||
> handling still flows through `await ffmpegDone`. (This is why the original
|
||||
> cancel test passed but the early-exit path died — the cancel path attaches
|
||||
> its handler within Node's grace window, the early-exit path does not.)
|
||||
> 3. **Backpressure hang** — when ffmpeg is dead, `stdin.write` returns false and
|
||||
> the old `await stdin.once('drain')` waited forever (no drain ever comes, and
|
||||
> it wasn't abortable). The wait now races `ffmpegDone` and `raceAbort(signal)`.
|
||||
>
|
||||
> Also added `if (settled) break` at the top of the frame loop so a dead ffmpeg
|
||||
> stops being fed. Verified: a stub ffmpeg that exits non-zero mid-stream makes
|
||||
> `renderMp4Job` reject with the ffmpeg error, and the process survives and exits
|
||||
> 0 (no `UNCAUGHT_EXCEPTION` / `UNHANDLED_REJECTION`, no hang).
|
||||
|
||||
### Problem
|
||||
The only error listeners are on the `ffmpeg` ChildProcess (`spawn`/`error`/
|
||||
`exit`) and `ffmpeg.stderr`. There is **no listener on `ffmpeg.stdin`**. If
|
||||
ffmpeg exits early (malformed frame, disk full, codec/resource error), the next
|
||||
loop iteration runs:
|
||||
|
||||
```ts
|
||||
if (!ffmpeg.stdin.write(png)) { ... }
|
||||
```
|
||||
|
||||
Writing to a pipe whose read end is closed emits an **asynchronous** `'error'`
|
||||
(EPIPE) on the stdin stream. Because it is async, the surrounding `try/catch`
|
||||
does not catch it; with no `'error'` listener it becomes an
|
||||
`uncaughtException` and takes down the **entire Electron app** (not just the
|
||||
export). The standalone server has the same write pattern, but there it only
|
||||
crashes an isolated, restartable server process — in the desktop app it kills
|
||||
the user's editor.
|
||||
|
||||
### Fix
|
||||
1. Swallow the stdin error (the real failure is already surfaced via the
|
||||
`exit`/`ffmpegDone` path):
|
||||
|
||||
```ts
|
||||
ffmpeg.stdin.on('error', () => { /* EPIPE after early exit; surfaced via ffmpegDone */ })
|
||||
```
|
||||
|
||||
2. Stop feeding a dead ffmpeg so the *real* error surfaces promptly. `settled`
|
||||
is already set in the `exit` handler — check it at the top of the loop:
|
||||
|
||||
```ts
|
||||
for (let i = 0; i < input.options.totalFrames; i += 1) {
|
||||
if (signal.aborted) throw abortReason(signal)
|
||||
if (settled) break // ffmpeg already exited; quit feeding it
|
||||
...
|
||||
ffmpeg.stdin.write(png)
|
||||
}
|
||||
ffmpeg.stdin.end()
|
||||
await ffmpegDone // rejects with "ffmpeg exited with code N: <stderr>"
|
||||
```
|
||||
|
||||
### Verification
|
||||
- New `scripts/verify-ffmpeg-early-exit.cjs`: induce an early ffmpeg exit (e.g.
|
||||
point `FFMPEG_PATH` at a stub that reads a few bytes then `exit(1)`, or kill
|
||||
the ffmpeg child mid-stream) and assert `renderMp4Job` **rejects** with a
|
||||
surfaced ffmpeg error and the **process does not crash** (the test harness
|
||||
itself surviving is the assertion).
|
||||
|
||||
---
|
||||
|
||||
## #9 — HIGH (trivial): bogus `packageManager` field
|
||||
|
||||
> **Status: IMPLEMENTED.** Line removed from `package.json`; valid JSON, no
|
||||
> references remain.
|
||||
|
||||
### Problem
|
||||
`package.json:6`:
|
||||
|
||||
```json
|
||||
"packageManager": "traversal@1.0.0",
|
||||
```
|
||||
|
||||
`traversal` is not a package manager. It is referenced nowhere and is absent
|
||||
from `package-lock.json` — almost certainly an accidental insertion. In any
|
||||
corepack-enabled environment (Node ships corepack; CI or contributors may have
|
||||
run `corepack enable`), invoking npm/yarn/pnpm reads this field and errors
|
||||
(`Unsupported package manager 'traversal'`), blocking installs and builds.
|
||||
|
||||
### Fix
|
||||
Delete the line. (If a manager really must be pinned, use a real value like
|
||||
`"npm@10.x"`.)
|
||||
|
||||
### Verification
|
||||
`npm install` / `npm ci` and `corepack npm -v` succeed in a clean checkout.
|
||||
|
||||
---
|
||||
|
||||
## #10 / #11 / #12 — Capture fidelity & performance
|
||||
|
||||
These three share a root cause: the render window is a **hidden, non-offscreen**
|
||||
`BrowserWindow`, and `capturePage` is coupled to the physical display
|
||||
(scale factor and size) and to Chromium's background throttling. The strategic
|
||||
fix is offscreen rendering (already floated in plan_2 finding #1); pragmatic
|
||||
per-issue mitigations are listed if offscreen is deferred.
|
||||
|
||||
### #10 — MEDIUM: HiDPI doubles the output resolution
|
||||
|
||||
> **Status: IMPLEMENTED (interim mitigation).** The frame loop now calls
|
||||
> `frame.getSize()` and, when the captured `NativeImage` doesn't match the
|
||||
> requested canvas, `frame.resize({width, height})` back to the canvas dims
|
||||
> before `toPNG()`. No-op on scale-factor-1 displays. The offscreen "deep fix"
|
||||
> below was not taken. (Can't be reproduced on this scale-1 box; should be
|
||||
> sanity-checked once on a HiDPI machine.)
|
||||
|
||||
`capturePage(captureRect)` takes a rect in DIPs but returns a `NativeImage` at
|
||||
the **display's device scale factor**. On a 2× display a 1920×1080 canvas yields
|
||||
a 3840×2160 PNG, so the MP4 is double the requested resolution and ~4× the size
|
||||
— diverging from the server, which pins `deviceScaleFactor: 1`. Likely
|
||||
unnoticed so far because the verify harness runs at scale factor 1.
|
||||
|
||||
Interim mitigation (normalize every frame to the requested size):
|
||||
|
||||
```ts
|
||||
const frame = await raceAbort(renderWindow.webContents.capturePage(captureRect), signal)
|
||||
const { width, height } = frame.getSize()
|
||||
const png = (width === captureRect.width && height === captureRect.height
|
||||
? frame
|
||||
: frame.resize({ width: captureRect.width, height: captureRect.height })
|
||||
).toPNG()
|
||||
```
|
||||
|
||||
(Do **not** use `app.commandLine.appendSwitch('force-device-scale-factor','1')`
|
||||
— it is global and would degrade the editor UI on HiDPI.)
|
||||
|
||||
### #11 — MEDIUM: canvases larger than the display are truncated
|
||||
|
||||
> **Status: IMPLEMENTED (interim mitigation).** `main.cts` now compares the
|
||||
> normalized canvas against `screen.getPrimaryDisplay().size` before starting and
|
||||
> throws a clear error ("Canvas WxH is larger than this display …") rather than
|
||||
> silently truncating. This is desktop-only: the web/server path keeps using a
|
||||
> headless viewport and still accepts up to 4096². The offscreen "deep fix" below
|
||||
> (which would let the desktop path render oversize canvases too) was not taken.
|
||||
|
||||
A non-offscreen window can't be composited/captured beyond the display bounds,
|
||||
so a canvas larger than the screen (validation allows up to 4096²) produces a
|
||||
clipped frame. The server avoids this with a headless Chrome viewport.
|
||||
|
||||
Interim mitigation: in `main.cts`, before starting, compare the (normalized)
|
||||
canvas against `screen.getPrimaryDisplay().workAreaSize` and **fail fast with a
|
||||
clear error** rather than silently truncating; or document a desktop max-canvas
|
||||
limit. (The real fix is offscreen.)
|
||||
|
||||
### #12 — MEDIUM (perf): hidden-window rAF throttling ⇒ ~1 fps
|
||||
|
||||
> **Status: IMPLEMENTED.** `backgroundThrottling: false` added to the render
|
||||
> window's `webPreferences`. (#10 and #11 remain open; the offscreen-rendering
|
||||
> "deep fix" below was not taken.)
|
||||
|
||||
`__animatrSeek` now resolves after a double `requestAnimationFrame`, with a
|
||||
`setTimeout(finish, 1000)` anti-hang fallback. But the render window is
|
||||
`show: false` with **no `backgroundThrottling: false`**, so Chromium throttles/
|
||||
pauses rAF for the hidden document — the rAFs don't fire promptly and the **1 s
|
||||
fallback becomes the per-frame path**. A 10 s @ 30 fps render then takes ~5 min.
|
||||
(The verify harness checks correctness, not throughput, so this slipped through.)
|
||||
|
||||
Fix (one line, large win):
|
||||
|
||||
```ts
|
||||
webPreferences: {
|
||||
...
|
||||
backgroundThrottling: false, // keep rAF firing while the window is hidden
|
||||
}
|
||||
```
|
||||
|
||||
### Deep fix (recommended) — offscreen rendering, subsumes #10/#11/#12
|
||||
Switch the render window to `webPreferences: { offscreen: true }`:
|
||||
- renders to an in-memory bitmap **independent of the physical display** (fixes
|
||||
#11 oversize) at a controllable scale (fixes #10 HiDPI),
|
||||
- isn't background-throttled and pairs with `webContents.setFrameRate(...)`
|
||||
(fixes #12),
|
||||
- capture via either the `paint` event / `beginFrameSubscription` or
|
||||
`capturePage` (still pair with the existing rAF settle to avoid the
|
||||
off-by-one from plan_2 #2).
|
||||
|
||||
Cost/risk: changes the capture mechanism, so the **entire** `test:render-desktop`
|
||||
suite (all 6 checks) must be re-run, plus new HiDPI + large-canvas checks, plus
|
||||
a GPU-vs-software-rendering sanity pass on each platform. If offscreen lands,
|
||||
#10/#11/#12 close together and the interim mitigations above are unnecessary.
|
||||
|
||||
### Verification (either path)
|
||||
- New `scripts/verify-resolution.cjs`: render and `ffprobe` the output; assert
|
||||
width/height **exactly equal** the canvas dims. (Won't reproduce #10 on a
|
||||
scale-factor-1 CI box; note that, and run it once on a HiDPI machine.)
|
||||
- New `scripts/verify-large-canvas.cjs`: render a canvas larger than the test
|
||||
display; assert full-size, non-truncated frames.
|
||||
- New throughput assertion (extend `verify-export.cjs`): time N frames and fail
|
||||
if per-frame wall-clock is near the 1 s fallback — guards the #12 regression.
|
||||
|
||||
---
|
||||
|
||||
## #13 — LOW (optional): odd canvas dimensions fail `yuv420p`
|
||||
|
||||
> **Status: IMPLEMENTED (pad to even).** Both ffmpeg spawns (desktop worker and
|
||||
> standalone server) now carry `-vf pad=ceil(iw/2)*2:ceil(ih/2)*2`, so an odd
|
||||
> canvas encodes (e.g. 101×101 → 102×102) instead of crashing with "height not
|
||||
> divisible by 2". Validation was left permissive (no reject). Verified by a
|
||||
> throwaway harness rendering a 101×101 scene to a clean 102×102 H.264 MP4, and
|
||||
> the server e2e test still passes.
|
||||
|
||||
### Problem
|
||||
libx264 with `-pix_fmt yuv420p` requires **even** width/height. Validation
|
||||
allows any integer 1..4096, so an odd-dimension canvas (e.g. 101×101) makes
|
||||
ffmpeg exit with a confusing "height not divisible by 2" error. This is shared
|
||||
with the standalone server (pre-existing), but the new desktop path is a fresh
|
||||
surface for it. On HiDPI it's accidentally masked because the 2× capture is
|
||||
always even — fixing #10 (normalize to canvas size) re-exposes it.
|
||||
|
||||
### Fix (pick one, apply to both desktop worker and server)
|
||||
- Pad to even in the ffmpeg args (least surprising, adds ≤1 px):
|
||||
`'-vf', 'pad=ceil(iw/2)*2:ceil(ih/2)*2'`, **or**
|
||||
- Reject odd dimensions in `shared/renderValidation.cjs` with a clear message.
|
||||
|
||||
### Verification
|
||||
Add an odd-dimension case (e.g. 101×101) to `shared/renderValidation.test.cjs`
|
||||
and/or a desktop check asserting a clean result instead of an ffmpeg crash.
|
||||
|
||||
---
|
||||
|
||||
## Out of scope / already correct (do not redo)
|
||||
- Validation parity, the abort/watchdog plumbing, the cancel-before-jobId race,
|
||||
ENOENT friendly message, packaging wiring, and the `__animatrSeek` paint
|
||||
settle (plan_2 #2) are all sound and unchanged by this plan.
|
||||
- Image assets are imported as `data:` URLs, so the render window's
|
||||
`*://*/*` network block (plan_2 #5) does not drop them — no action needed.
|
||||
|
||||
## Suggested doc bookkeeping
|
||||
When these land, update `electron-rendering-integration-plan_2.md`:
|
||||
- correct the "Partial output file is removed on failure/cancel" note (#7),
|
||||
- and `electron-rendering-manual-checks.md` with the new verify scripts.
|
||||
Reference in new issue
Block a user