feat(export): GPU-accelerated H.264/HEVC export via NVIDIA RTX Rendering - #794
feat(export): GPU-accelerated H.264/HEVC export via NVIDIA RTX Rendering#794nmzpy wants to merge 11 commits into
Conversation
- HEVC custom bitrate max clamped to 70 Mbps; H.264 gets 50% headroom (105 Mbps). - Smaller w-14 Mbps input styled with the export menu's translucent palette. - 'Lightning (Beta)' pipeline toggle removed; 'NVIDIA CUDA compositor' relabeled to 'NVIDIA RTX Rendering'; 'Hardware' to 'GPU'. - H.264 hardware can now route to the native CUDA compositor, matching HEVC hardware. - Remove obsolete HEVC hint and en i18n hevcHint key.
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds H.264 and HEVC export settings, codec-aware routing, CUDA and overlay composition, transferable native frame transport, pending-media authorization, diagnostics, benchmarks, build tooling, and localized export controls. ChangesNative export and transport
Editor export configuration
Media handling and tooling
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
electron/electron-env.d.ts (1)
397-473: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winThe static-layout export IPC option types drift from the main-process contract.
NativeStaticLayoutExportOptionsinelectron/ipc/export/native-video.tsdeclarescursorAtlasOwned(Line 625) andtemporalBlur(Lines 609-613), and the export path branches on both. The two renderer-facing declarations of the same call do not expose the full set, so renderer code cannot express cursor ownership or a temporal blur plan without a cast, and the baked-sidecar default applies silently.
electron/electron-env.d.ts#L397-L473: addcursorAtlasOwned?: boolean;to thenativeStaticLayoutExportoption type.electron/preload.ts#L616-L637: add bothcursorAtlasOwned?: boolean;and thetemporalBlur?: { sampleCount: number; shutterFraction: number; weightCurvePower: number } | null;field to thenativeStaticLayoutExportoption type.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/electron-env.d.ts` around lines 397 - 473, Update the renderer-facing nativeStaticLayoutExport option types to match NativeStaticLayoutExportOptions: in electron/electron-env.d.ts lines 397-473, add optional cursorAtlasOwned?: boolean; in electron/preload.ts lines 616-637, add optional cursorAtlasOwned?: boolean and nullable temporalBlur with sampleCount, shutterFraction, and weightCurvePower fields.src/lib/exporter/nativeFrameCapture.ts (1)
63-95: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winVertical-flip responsibility moved out of the capture helper, but the caller still describes and requests a flip.
captureCanvasFrameForNativeExportnow returns top-down RGBA rows on both capture paths and ignores its third parameter, so the caller comment and thetrueargument are stale and misleading.
src/lib/exporter/nativeFrameCapture.ts#L63-L95: remove the unused_flipVerticalparameter from the signature, or document that it is retained only for backward compatibility, and update the callers andelectron/ipc/nativeVideoExport.test.tsaccordingly.src/lib/exporter/modernVideoExporter.ts#L4037-L4039: delete the "Flip rows vertically" comment and drop thetrueargument so the call readscaptureCanvasFrameForNativeExport(canvas, timestamp); state instead that renderer frames are canonical top-down RGBA.As per coding guidelines: "Treat renderer raw RGBA frames as canonical top-down frames and avoid duplicate vertical flips between renderer and FFmpeg integration."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/exporter/nativeFrameCapture.ts` around lines 63 - 95, The capture helper should no longer expose or request vertical flipping: remove the unused _flipVertical parameter from captureCanvasFrameForNativeExport, then update its callers and electron/ipc/nativeVideoExport.test.ts accordingly. In src/lib/exporter/modernVideoExporter.ts lines 4037-4039, remove the flip comment and true argument, call captureCanvasFrameForNativeExport(canvas, timestamp), and describe renderer frames as canonical top-down RGBA.Source: Coding guidelines
🟠 Major comments (21)
src/components/video-editor/ExportSettingsMenu.tsx-549-592 (1)
549-592: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winGate the "Selected" state on availability, and keep the switch reachable.
The
experimentalNvidiaCudaExportbranch precedes thenvidiaCudaExportAvailablecheck. If a persisted opt-in is true and the compositor is unavailable, the badge shows "Selected" and the hint claims "Exports will use the NVIDIA CUDA compositor on this device." The export then uses the raw fallback, so this text states a capability that the probe did not confirm. In the same state, Line 630 renders noSwitch, so the user cannot clear the stale opt-in. RequirenvidiaCudaExportAvailablefor the selected badge and hint, and render the switch in a disabled or off state when the compositor is unavailable.As per coding guidelines: "Never claim codec, rate-control, AQ, VBV, or performance capabilities unless confirmed by the probe or live encoder".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/video-editor/ExportSettingsMenu.tsx` around lines 549 - 592, Update the NVIDIA CUDA UI logic in ExportSettingsMenu so experimentalNvidiaCudaExport only produces the “Selected” badge and selected hint when nvidiaCudaExportAvailable is also true. When unavailable, ensure the associated Switch remains rendered and is disabled or shown off so users can clear the stale opt-in, without claiming CUDA compositor capability unless the probe confirms it.Source: Coding guidelines
src/components/video-editor/ExportSettingsMenu.tsx-111-126 (1)
111-126: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRe-clamp the custom bitrate when the codec maximum drops.
effectiveMaxMbpschanges withexportVideoCodec, but no code re-clampsexportBitrateMbpsafter a codec switch. If the user sets 100 Mbps with H.264 and then selects H.265, the committed value stays 100 while the new maximum is 70. TheSliderat Line 455 and theInputat Line 471 only bound new input; the stale value is still emitted to the export settings. Clamp the current value when the maximum changes.🐛 Proposed fix
useEffect(() => { setBitrateDraft(String(exportBitrateMbps)); }, [exportBitrateMbps]); const effectiveMaxMbps = exportVideoCodec === "hevc" ? 70 : 105; + + useEffect(() => { + if (exportBitrateMbps > effectiveMaxMbps) { + setBitrateDraft(String(effectiveMaxMbps)); + onExportBitrateMbpsChange?.(effectiveMaxMbps); + } + }, [effectiveMaxMbps, exportBitrateMbps, onExportBitrateMbpsChange]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/video-editor/ExportSettingsMenu.tsx` around lines 111 - 126, Update the bitrate state flow around effectiveMaxMbps and commitBitrateDraft to re-clamp exportBitrateMbps whenever exportVideoCodec changes and the maximum decreases. Emit the clamped value through onExportBitrateMbpsChange and synchronize bitrateDraft, while preserving the existing minimum, maximum, and default handling.src/components/video-editor/VideoEditor.tsx-2188-2193 (1)
2188-2193: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRestore MP4 preferences after current-project hydration.
Lines 2188-2193 apply the project values. The current-project startup path later reapplies user preferences for existing export controls, but it does not reapply these four values. The preference-saving effect then replaces the last-used codec, encoder preference, bitrate mode, and custom bitrate with the project values.
Reapply the four
initialEditorPreferencesvalues in the current-project preference restore block. Add a regression test for loading a project with different MP4 settings.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/video-editor/VideoEditor.tsx` around lines 2188 - 2193, The current-project preference restore flow must also restore the four MP4 preferences after project hydration. Update the existing restore block in VideoEditor to reapply initialEditorPreferences for exportVideoCodec, exportEncoderPreference, exportBitrateMode, and exportBitrateMbps, and add a regression test covering a project whose MP4 settings differ from those saved preferences.src/lib/exporter/exportBitrate.ts-154-168 (1)
154-168: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftApply codec-specific custom bitrate limits.
Line 165 allows 200 Mbps for both codecs. The PR requires a 70 Mbps maximum for HEVC and a 105 Mbps maximum for H.264.
resolveExportBitratedoes not receive the codec, so it cannot enforce either limit.Add
ExportVideoCodecto this resolver. Clamp custom Mbps by codec. Update the UI validation and boundary tests for both codecs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/exporter/exportBitrate.ts` around lines 154 - 168, Update resolveExportBitrate to accept an ExportVideoCodec and, for custom mode, clamp customMbps to the codec-specific maximum before converting it: 70 Mbps for HEVC and 105 Mbps for H.264. Propagate the new codec argument through callers, update UI validation to use the same limits, and revise boundary tests for both codecs.src/components/video-editor/VideoEditor.tsx-4918-4924 (1)
4918-4924: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep HEVC from selecting H.264 WebCodecs after static-layout fallback.
exportVideoCodecselects the native raw-frame path, butModernVideoExporterstill falls back from the CUDA/static-layout route intoinitializeEncoder()without a HEVC gate. Add a direct code-path reject for HEVC after static-layout failure, and ensure the muxed codec metadata respectsexportVideoCodec.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/video-editor/VideoEditor.tsx` around lines 4918 - 4924, Update the ModernVideoExporter static-layout fallback around initializeEncoder() to reject HEVC directly instead of allowing it to select the H.264 WebCodecs path after CUDA/static-layout failure. Also update the muxed codec metadata construction near exportVideoCodec so it reflects the selected exportVideoCodec rather than an incompatible fallback codec.Source: Coding guidelines
src/components/video-editor/projectPersistence.ts-245-247 (1)
245-247: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftEnforce the codec-specific bitrate limits.
The new generic 200 Mbps limit conflicts with the PR requirement: H.264 must not exceed 105 Mbps, and HEVC must not exceed 70 Mbps. Persisted projects can select unsupported values, and the UI advertises those values.
src/components/video-editor/projectPersistence.ts#L245-L247: Normalize Mbps with the selected codec limit.src/components/video-editor/projectPersistence.test.ts#L60-L77: Test the 105 Mbps H.264 limit and the 70 Mbps HEVC limit.src/lib/exporter/exportBitrate.test.ts#L167-L212: Test codec-aware custom bitrate resolution.src/lib/exporter/exportBitrate.test.ts#L230-L235: Replace the 200 Mbps expectation with codec-specific limits.src/i18n/locales/nl/settings.json#L266-L272: Show a codec-specific bitrate range.src/i18n/locales/pt-BR/settings.json#L266-L272: Show a codec-specific bitrate range.src/i18n/locales/ru/settings.json#L266-L272: Show a codec-specific bitrate range.src/i18n/locales/zh-CN/settings.json#L266-L272: Show a codec-specific bitrate range.src/i18n/locales/zh-TW/settings.json#L266-L272: Show a codec-specific bitrate range.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/video-editor/projectPersistence.ts` around lines 245 - 247, Update normalizeExportBitrateMbps in src/components/video-editor/projectPersistence.ts:245-247 to apply the selected codec’s maximum, 105 Mbps for H.264 and 70 Mbps for HEVC, rather than the generic 200 Mbps limit. Add coverage in src/components/video-editor/projectPersistence.test.ts:60-77 and src/lib/exporter/exportBitrate.test.ts:167-212, and replace the generic expectation in src/lib/exporter/exportBitrate.test.ts:230-235. Update the codec bitrate range text in src/i18n/locales/nl/settings.json:266-272, src/i18n/locales/pt-BR/settings.json:266-272, src/i18n/locales/ru/settings.json:266-272, src/i18n/locales/zh-CN/settings.json:266-272, and src/i18n/locales/zh-TW/settings.json:266-272.electron/native/nvidia-cuda-compositor/run-mp4-pipeline.mjs-1370-1379 (1)
1370-1379: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winTemporal blur flags are forwarded without a native support check.
The tiled overlay path at Lines 1434-1444 probes
--helpand fails with an actionable error when the native compositor does not advertise--tiled-overlay-manifest. The temporal blur path has no equivalent guard. If the staged native helper predates the temporal blur flags, the effect is dropped or the process fails with an opaque argument error instead of an explicit unsupported result.Requested sample counts of 1 or 2 are also dropped without any diagnostic.
Add the same
--helpcapability check for--temporal-blur-sample-count, and emit an explicit unsupported-temporal-motion-blur failure when the flag is absent.As per coding guidelines: "Temporal zoom motion blur must use the renderer-resolved sampling plan and bounded scratch storage; if CUDA is unavailable, return an explicit unsupported-temporal-motion-blur result rather than silently dropping the effect."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/native/nvidia-cuda-compositor/run-mp4-pipeline.mjs` around lines 1370 - 1379, The temporal blur argument handling must validate native support and report unsupported requests explicitly. Update the temporal blur flow around temporalBlurSampleCount to probe the staged compositor’s --help output for --temporal-blur-sample-count, fail with the established unsupported-temporal-motion-blur result when absent or CUDA is unavailable, and emit a diagnostic for requested sample counts of 1 or 2 instead of silently omitting the effect. Preserve the renderer-resolved sampling plan and bounded scratch-storage behavior.Source: Coding guidelines
electron/ipc/project/manager.ts-127-153 (1)
127-153: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winRemove the duplicated read-policy logic.
isAllowedLocalReadPathWithRoots(Lines 127-153) repeats the body ofisAllowedLocalReadPath(Lines 74-113) exactly, with only the root source differing. This is security-critical policy. Two copies can drift, and a future fix applied to one path will silently miss the other.
isAllowedLocalReadPathWithRootsis also declaredasyncbut performs only synchronous work (realpathSync), so theasynckeyword adds no value here.Extract one synchronous core and have both entry points call it.
♻️ Proposed refactor
+function isAllowedLocalReadPathForRoots(candidatePath: string, allowedPrefixes: string[]) { + const normalizedCandidatePath = normalizePath(candidatePath); + const foldedCandidatePath = foldPathComparisonKey(normalizedCandidatePath); + + let canonicalCandidatePath = normalizedCandidatePath; + try { + canonicalCandidatePath = normalizePath(realpathSync(normalizedCandidatePath)); + } catch { + // File may not exist yet; keep the lexical path. + } + + const lexicalAllowed = + allowedPrefixes.some((prefix) => isPathInsideDirectory(normalizedCandidatePath, prefix)) || + approvedLocalReadPaths.has(foldedCandidatePath); + if (!lexicalAllowed) { + return false; + } + if (canonicalCandidatePath === normalizedCandidatePath) { + return true; + } + return ( + allowedPrefixes.some((prefix) => isPathInsideDirectory(canonicalCandidatePath, prefix)) || + approvedLocalReadPaths.has(foldPathComparisonKey(canonicalCandidatePath)) + ); +} + -async function isAllowedLocalReadPathWithRoots(candidatePath: string, allowedPrefixes: string[]) { - const normalizedCandidatePath = normalizePath(candidatePath); - ... -} +function isAllowedLocalReadPathWithRoots(candidatePath: string, allowedPrefixes: string[]) { + return isAllowedLocalReadPathForRoots(candidatePath, allowedPrefixes); +}
isAllowedLocalReadPaththen becomesisAllowedLocalReadPathForRoots(candidatePath, getAllowedLocalReadRootsSync()).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/ipc/project/manager.ts` around lines 127 - 153, Extract the shared synchronous path-validation logic from isAllowedLocalReadPath and isAllowedLocalReadPathWithRoots into a single core helper that accepts candidatePath and allowedPrefixes. Update both entry points to delegate to that helper, with isAllowedLocalReadPath obtaining roots via getAllowedLocalReadRootsSync(), and remove async from isAllowedLocalReadPathWithRoots while preserving the existing canonical and approved-path checks.electron/ipc/project/manager.ts-61-72 (1)
61-72: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winLoad the recordings directory setting before the media server starts.
mediaServer.tsvalidates serve-time media paths withgetAllowedLocalReadRootsSync(), whileresolveLocalMediaUrlPath()can mint media URLs aftergetRecordingsDir()populatescustomRecordingsDir. If the settings file has not been read beforeensureMediaServer()starts, later media-server requests for files in a custom recordings directory can fail the sync prefix check even after getting a local media URL. Load the recording directory setting before starting the media server, or make the sync path function load the same value.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/ipc/project/manager.ts` around lines 61 - 72, Ensure the recordings directory setting is loaded before ensureMediaServer() starts so getAllowedLocalReadRootsSync() reflects the same custom directory used by resolveLocalMediaUrlPath(). Alternatively, update getAllowedLocalReadRootsSync() to synchronously obtain that setting while preserving the existing fallback and allowed-root entries.electron/native/nvidia-cuda-compositor/cursorTelemetry.mjs-181-183 (1)
181-183: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winNormalize and validate canonical JSON cursor samples.
The JSON branch returns unnormalized objects and silently removes invalid entries. A valid canonical sample with
cursorType: "pointer"therefore becomes cursor type0when Line 102 serializes it. A malformed JSON payload can also remove all cursor samples without an actionable error.
electron/native/nvidia-cuda-compositor/cursorTelemetry.mjs#L181-L183: Normalize JSON samples with the same bounds and defaults as TSV/CSV rows. Reject invalid entries with the sample index.electron/native/nvidia-cuda-compositor/cursorTelemetry.mjs#L97-L105: ResolvecursorTypewhencursorTypeIndexis absent.electron/native/nvidia-cuda-compositor/cursorTelemetry.test.mjs#L90-L102: Add cases for stringcursorType, out-of-range JSON values, and invalid JSON samples.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/native/nvidia-cuda-compositor/cursorTelemetry.mjs` around lines 181 - 183, Normalize canonical JSON samples in cursorTelemetry.mjs at lines 181-183 using the same bounds and defaults as TSV/CSV rows, and reject invalid entries with their sample index instead of silently filtering them. Update the serialization logic at lines 97-105 to resolve cursorType when cursorTypeIndex is absent. Add test coverage in cursorTelemetry.test.mjs lines 90-102 for string cursorType values, out-of-range JSON values, and invalid JSON samples.electron/mediaServer.ts-102-118 (1)
102-118: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRequire per-path approval before serving a file.
Line 118 authorizes every resolved path inside an allowed root. A caller can request an unapproved file under
userDataortemp, including a non-media file because this handler does not enforceisSupportedLocalMediaPath.Keep root containment as a condition for pending-path validation, but require the resolved path to match an approval recorded by
resolveLocalMediaUrlPath. Add an HTTP regression test for an existing unapproved file insideuserDatathat returns403.Also applies to: 141-164
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/mediaServer.ts` around lines 102 - 118, Update isAllowedMediaPath to require approvedLocalReadPaths membership for every served path, while retaining getAllowedLocalReadRootsSync and isPathInsideDirectory only to validate pending paths within allowed roots. Ensure resolveLocalMediaUrlPath records approvals for valid media paths, and add an HTTP regression test confirming an existing unapproved file under userData receives a 403 response.AGENTS.md-30-60 (1)
30-60: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRemove workstation-specific user paths.
This document exposes the
nmzusername and local installation paths. Replace these paths with environment-based examples such as%APPDATA%\Recordly-devand%LOCALAPPDATA%\Programs\Recordly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@AGENTS.md` around lines 30 - 60, Replace workstation-specific absolute paths and the username “nmz” in the AGENTS.md setup and uninstall instructions with environment-based Windows examples, including %APPDATA%\Recordly-dev, %APPDATA%\Recordly, and %LOCALAPPDATA%\Programs\Recordly; preserve the existing guidance and commands otherwise.src/components/launch/LaunchWindow.tsx-166-170 (1)
166-170: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not clear auto-start while source selection is pending.
SourcePopoverwaits onhandleSourceSelect()and only closes after that callback returns. In parallel, the popover closes when the window blurs, which setsopenIdtonulland clearspendingAutoStartRef. A background blur during selection can prevent the selected source from starting recording. Keep the pending request alive untilhandleSourceSelect()consumes it, closes the popover, or signals cancellation/explicit rejection.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/launch/LaunchWindow.tsx` around lines 166 - 170, The openId effect must not reset pendingAutoStartRef while handleSourceSelect is still processing a source selection. Update the effect and related SourcePopover/handleSourceSelect flow to preserve the pending request through blur-driven closure, clearing it only after consumption, popover closure, cancellation, or explicit rejection.electron/ipc/export/nativeStaticLayoutRoutePlan.ts-160-171 (1)
160-171: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftHEVC with Hardware preference selects a raw-video fallback instead of hard-failing. The route planner returns
fallbackRoute: "native-rawvideo"when CUDA is unavailable forvideoCodec: "hevc"andencoderPreference: "hardware". The encoder policy requires a hard failure with an actionable error andnoCpuFallback: truefor that combination, with no CPU, renderer, or Breeze fallback. The test encodes the same behavior.
electron/ipc/export/nativeStaticLayoutRoutePlan.ts#L160-L171: either confirm the consumer converts thehevc-hardware-route-unavailable:reason into a hard failure withnoCpuFallback: true, or return a mandatory-failure plan for this branch instead of a raw-video fallback.electron/ipc/export/nativeStaticLayoutRoutePlan.test.ts#L180-L206: update the expectation to assert the hard-failure result once the plan changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/ipc/export/nativeStaticLayoutRoutePlan.ts` around lines 160 - 171, Ensure the HEVC hardware-preference branch in createRawVideoFallbackPlan produces a mandatory hard-failure result with an actionable error and noCpuFallback: true when CUDA is unavailable, rather than a native-rawvideo fallback; alternatively, make the consumer explicitly convert hevc-hardware-route-unavailable reasons into that failure. Update electron/ipc/export/nativeStaticLayoutRoutePlan.test.ts lines 180-206 to assert the hard-failure result.Source: Coding guidelines
electron/ipc/nativeVideoExport.ts-627-663 (1)
627-663: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSort overlay layers by
orderin the precomposited builder.
buildNativeCudaOverlayStaticLayoutArgssorts layers byorder, thenid(lines 390-392). This builder iteratesconfig.overlayLayersin array order for both input registration and overlay chaining. If a caller supplies layers in an order that differs from theorderfield, the z-order of the composited output is wrong. Apply the same sort once and reuse the sorted array for both loops.🔧 Proposed fix to apply a deterministic layer order
- for (const layer of config.overlayLayers ?? []) { + const overlayLayers = [...(config.overlayLayers ?? [])].sort( + (left, right) => left.order - right.order || left.id.localeCompare(right.id), + ); + for (const layer of overlayLayers) { args.push( "-f", "rawvideo", @@ - for (const [index, layer] of (config.overlayLayers ?? []).entries()) { + for (const [index, layer] of overlayLayers.entries()) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/ipc/nativeVideoExport.ts` around lines 627 - 663, Update the precomposited overlay builder around the visible overlay input-registration and filter-chaining loops to create one sorted overlay-layer array using the existing order-then-id comparison from buildNativeCudaOverlayStaticLayoutArgs. Reuse that sorted array for both input registration and overlay chaining, preserving the matching input indices while ensuring compositing follows each layer’s order.Source: Coding guidelines
electron/ipc/export/native-video.ts-3888-3897 (1)
3888-3897: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winA temporal blur plan with
sampleCountbelow 3 is silently dropped.The guard only forwards the temporal blur arguments when
sampleCount >= 3. For a resolved plan withsampleCountof 1 or 2 the CUDA wrapper receives no temporal blur arguments, the export succeeds, and the effect disappears. The fallback guards at Line 4700 and Line 4920 hard-fail only when CUDA is unavailable or fails, so they do not cover this path.Either reject such a plan with an explicit unsupported-temporal-motion-blur error, or have the renderer never emit a plan below the minimum sample count and assert that invariant here.
As per coding guidelines: "Temporal zoom motion blur must use the renderer-resolved sampling plan and bounded scratch storage; if CUDA is unavailable, return an explicit unsupported-temporal-motion-blur result rather than silently dropping the effect."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/ipc/export/native-video.ts` around lines 3888 - 3897, The temporal blur argument construction in the export flow must not silently omit plans with sampleCount below 3. Enforce the minimum-sample invariant before the guard in the temporal blur handling, either by rejecting the plan with an explicit unsupported-temporal-motion-blur error or by asserting that the renderer-resolved plan is valid; ensure valid plans continue using their resolved sampling values and bounded scratch-storage behavior.Source: Coding guidelines
src/lib/exporter/nativeStaticLayoutOverlays.ts-361-368 (1)
361-368: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winBoth tiled-overlay validators accept overlapping tile payload ranges. Each validator keys uniqueness on the exact
byteOffset:byteLengthpair and never requiresbyteOffsetto be tile-aligned. Two records at offsets0and1both pass while sharing all but one byte, which breaks the documented "every payload region is written exactly once" contract and makes one tile decode wrong pixels.
src/lib/exporter/nativeStaticLayoutOverlays.ts#L361-L368: requirerecord.byteOffset % NATIVE_TILED_OVERLAY_TILE_BYTE_SIZE === 0invalidateTiledOverlayTileRecord, or track occupied intervals instead of exact keys.electron/native/nvidia-cuda-compositor/tiledOverlayManifest.mjs#L182-L189: apply the same alignment or interval check invalidateTileRecordso the wrapper keeps mirroring the TypeScript contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/exporter/nativeStaticLayoutOverlays.ts` around lines 361 - 368, Update validateTiledOverlayTileRecord in src/lib/exporter/nativeStaticLayoutOverlays.ts at lines 361-368 to reject records whose byteOffset is not aligned to NATIVE_TILED_OVERLAY_TILE_BYTE_SIZE, rather than relying only on exact range keys. Apply the same alignment validation in validateTileRecord in electron/native/nvidia-cuda-compositor/tiledOverlayManifest.mjs at lines 182-189 so both validators enforce the same tile-payload contract.electron/ipc/export/native-video.ts-149-154 (1)
149-154: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
completedFrameRequestIdsgrows without bound for the whole session.Every accepted frame adds a request id, and the set is cleared only in
attachNativeVideoExportFramePort. A long export writes one entry per frame, so a 30-minute 60 fps export retains about 108,000 entries for the session lifetime.The strict
sequence !== session.nextFrameSequencecheck at Line 372 already rejects replays and out-of-order frames, so the id set only needs to cover ids that are still in flight or recently settled. Bound it, for example by tracking the highest accepted request id plus the pending map, or by pruning ids below a watermark.♻️ Sketch: replace the unbounded set with a watermark
- if ( - session.completedFrameRequestIds.has(requestId) || - session.pendingFrameRequests.has(requestId) - ) { + if ( + requestId <= session.highestAcceptedFrameRequestId || + session.pendingFrameRequests.has(requestId) + ) {session.pendingFrameRequests.set(requestId, { sequence }); - session.completedFrameRequestIds.add(requestId); + session.highestAcceptedFrameRequestId = requestId; session.nextFrameSequence += 1;This assumes the renderer allocates request ids monotonically, which
nextNativeVideoExportWriteRequestId++inelectron/preload.tsdoes.Also applies to: 411-413
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/ipc/export/native-video.ts` around lines 149 - 154, Bound completed frame request tracking in the native video export flow instead of retaining every accepted id for the session. Update the state and acceptance logic around completedFrameRequestIds and the relevant request handler to use a watermark or equivalent pruning strategy, while preserving pending-request tracking and the existing strict session.nextFrameSequence replay/order validation.electron/native/nvidia-cuda-compositor/temporalAccumulate.test.mjs-186-229 (1)
186-229: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
stationaryPlaneEquivalenceindexescontentValueswith the frame width, so most content pixels read asundefined.
contentValuesis allocated withregionWidth * regionHeightentries by the callers (Lines 295, 342, 351), but Lines 201 and 222 index it withy * width + x. For every row above the first few, that index exceeds the array length and yieldsundefined.fusedAccumulatethen computes(weightFixed * undefined + 128) >> 8, which is0.All three paths use the same expression, so the equality assertions still pass. The tests therefore compare zeros instead of real content values, and the intended fixed-point equivalence coverage over the content region is lost. Index the content array with the region stride.
🐛 Proposed fix
for (let y = 0; y < height; y += 1) { for (let x = 0; x < width; x += 1) { const invariantValue = x < regionWidth && y < regionHeight - ? contentValues[y * width + x] + ? contentValues[y * regionWidth + x] : backgroundValue;for (let y = 0; y < regionHeight; y += 1) { for (let x = 0; x < regionWidth; x += 1) { const index = y * width + x; backgroundPrecompose[index] = fusedAccumulate( - new Array(weights.length).fill(contentValues[index]), + new Array(weights.length).fill(contentValues[y * regionWidth + x]), weights, ); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/native/nvidia-cuda-compositor/temporalAccumulate.test.mjs` around lines 186 - 229, Update both contentValues lookups in stationaryPlaneEquivalence to use regionWidth as the row stride instead of width, including the invariantValue calculation and backgroundPrecompose assignment. Keep frame indexing based on width so content pixels map correctly within the region.src/lib/exporter/modernVideoExporter.ts-3525-3566 (1)
3525-3566: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDiscard the native temp video file when the route is rejected.
Both rejection paths return
nullafternativeStaticLayoutExportreportedsuccess: truewith aresult.tempPath. Thefinallyblock at Lines 3654-3668 discards onlyoverlayTempPathand the background asset, so the produced video temp file stays on disk for the whole app session. For a long HEVC export this can be several gigabytes.Discard
result.tempPathbefore returningnullfrom the HEVC-route rejection and the effect-preservation rejection.🐛 Proposed fix
if (requestedVideoCodec === "hevc" && !acceptedHevcNativeRoute) { const routeSkipReason = "unsupported-native-hevc-route"; console.warn( "[VideoExporter] Rejecting HEVC native static-layout result from a non-CUDA route", { route: result.route, isStrictHevcHardware }, ); this.nativeStaticLayoutSkipReason = routeSkipReason; this.nativeStaticLayoutSkipReasons = [routeSkipReason]; + await window.electronAPI?.discardExportedTemp?.(result.tempPath).catch( + () => undefined, + ); restoreEncoderState(); return null; }Apply the same discard in the
shouldRejectNativeStaticLayoutResultForEffectPreservationbranch at Lines 3557-3565.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/exporter/modernVideoExporter.ts` around lines 3525 - 3566, Discard result.tempPath before returning null in both rejection branches: the unsupported native HEVC route check and the shouldRejectNativeStaticLayoutResultForEffectPreservation check. Reuse the existing temporary-file cleanup mechanism, then preserve the current skip-reason assignment, restoreEncoderState call, and null return.src/lib/exporter/modernVideoExporter.ts-1755-1781 (1)
1755-1781: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winGate rendering extension hooks on every native static-layout route.
unsupported-extension-hookis only added insidegetHevcNativeGpuFeatureSkipReasons(), which returns early on non-CUDA routes. These routes cannot render renderer extension hooks or cursor effects, so non-CUDA exports are silently missing those visuals. Add a matchingunsupported-extension-hookcheck ingetNativeStaticLayoutSkipReasons()for all native static-layout exports, and remove or attach the dangling cursor-rendering comment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/exporter/modernVideoExporter.ts` around lines 1755 - 1781, Update getNativeStaticLayoutSkipReasons() to add the same unsupported-extension-hook detection for cursor effects and all relevant extensionHookPhases on every native static-layout route, without the CUDA-only early return. Remove or relocate the dangling cursor-rendering comment so it accurately describes the shared logic, and avoid duplicating the check in getHevcNativeGpuFeatureSkipReasons() if the common method now owns it.Source: Coding guidelines
🟡 Minor comments (13)
src/components/video-editor/ExportSettingsMenu.tsx-468-480 (1)
468-480: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClamp the numeric input value before you emit it.
onChangeemits any finite parsed number, so typing500propagates 500 Mbps to the export settings. The clamp runs only incommitBitrateDrafton blur. Clamp inonChangeand keep the raw text inbitrateDraftfor display.🐛 Proposed fix
onChange={(event) => { setBitrateDraft(event.target.value); const parsed = Number(event.target.value); if (Number.isFinite(parsed)) { - onExportBitrateMbpsChange?.(parsed); + onExportBitrateMbpsChange?.( + Math.min( + effectiveMaxMbps, + Math.max(EXPORT_BITRATE_MIN_MBPS, parsed), + ), + ); } }}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/video-editor/ExportSettingsMenu.tsx` around lines 468 - 480, Update the bitrate input onChange handler to retain the raw event value in bitrateDraft but clamp finite parsed values to EXPORT_BITRATE_MIN_MBPS and effectiveMaxMbps before invoking onExportBitrateMbpsChange. Keep the existing behavior for non-numeric input and leave commitBitrateDraft unchanged.src/i18n/locales/nl/settings.json-246-265 (1)
246-265: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTranslate the CUDA and backend strings.
These locale files retain English text for backend labels, CUDA status, errors, and guidance.
src/i18n/locales/nl/settings.json#L246-L265: Add Dutch translations.src/i18n/locales/pt-BR/settings.json#L246-L265: Add Brazilian Portuguese translations.src/i18n/locales/ru/settings.json#L246-L265: Add Russian translations.src/i18n/locales/zh-CN/settings.json#L246-L265: Add Simplified Chinese translations.src/i18n/locales/zh-TW/settings.json#L246-L265: Add Traditional Chinese translations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/i18n/locales/nl/settings.json` around lines 246 - 265, Translate the remaining English backend and NVIDIA CUDA strings in the settings locale blocks for the backend object and the nvidiaCuda section, keeping the existing keys and message meanings unchanged. Update the anchor and sibling files listed here: src/i18n/locales/nl/settings.json#L246-L265, src/i18n/locales/pt-BR/settings.json#L246-L265, src/i18n/locales/ru/settings.json#L246-L265, src/i18n/locales/zh-CN/settings.json#L246-L265, and src/i18n/locales/zh-TW/settings.json#L246-L265. Use the same key structure already present in these locale files, and only replace the English text with the correct localized translations for the backend label, CUDA compositor labels, badges, toggle text, hints, and unavailable messages.src/i18n/locales/ko/settings.json-250-265 (1)
250-265: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTranslate the NVIDIA export strings into Korean.
These new strings remain in English in the Korean locale. Translate the labels, status badges, controls, hints, and failure messages.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/i18n/locales/ko/settings.json` around lines 250 - 265, Translate all newly added NVIDIA CUDA compositor strings in the Korean locale, including compositorTitle, backendLabel, backendSelected, every status badge, toggle labels, hints, and unavailable reason/messages. Preserve the existing JSON keys and interpolation placeholder {{reason}} while replacing the English values with natural Korean translations.src/i18n/locales/de/settings.json-276-276 (1)
276-276: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winLimit the strict-failure message to H.265 with Hardware.
This text says that H.265 exports and Hardware exports fail independently. State that only H.265 exports with the Hardware encoder fail when the CUDA compositor is unavailable.
As per coding guidelines, “For HEVC with Hardware preference” is the strict-policy condition.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/i18n/locales/de/settings.json` at line 276, Update the unavailableRequired translation value so it states that only HEVC/H.265 exports using the Hardware encoder fail when the CUDA compositor is unavailable. Preserve the guidance to install or update NVIDIA drivers or switch the encoder to Auto, and remove wording that implies all H.265 and all Hardware exports fail independently.Source: Coding guidelines
src/i18n/locales/de/settings.json-251-251 (1)
251-251: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winShow the codec-specific bitrate maximum.
The export contract caps H.264 at 105 Mbps and HEVC at 70 Mbps. These strings advertise 200 Mbps. Use a parameterized maximum from the validated codec limit, or state the correct codec-specific limits.
src/i18n/locales/de/settings.json#L251-L251: replace the fixed 200 Mbit/s maximum.src/i18n/locales/en/settings.json#L271-L271: replace the fixed 200 Mbps maximum.src/i18n/locales/es/settings.json#L251-L251: replace the fixed 200 Mbps maximum.src/i18n/locales/fr/settings.json#L251-L251: replace the fixed 200 Mbit/s maximum.src/i18n/locales/it/settings.json#L251-L251: replace the fixed 200 Mbps maximum.src/i18n/locales/ko/settings.json#L271-L271: replace the fixed 200 Mbps maximum.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/i18n/locales/de/settings.json` at line 251, Replace the fixed 200 Mbps bitrate maximum in src/i18n/locales/de/settings.json:251-251, src/i18n/locales/en/settings.json:271-271, src/i18n/locales/es/settings.json:251-251, src/i18n/locales/fr/settings.json:251-251, src/i18n/locales/it/settings.json:251-251, and src/i18n/locales/ko/settings.json:271-271 with codec-specific validated limits: H.264 up to 105 Mbps and HEVC up to 70 Mbps, using the existing parameterized limit mechanism if available while preserving each locale’s units and formatting.electron/ipc/state.ts-33-34 (1)
33-34: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe second regex does not match the
\\.\device prefix.In
/^\\.\\/the.is an unescaped wildcard, not a literal dot. The pattern therefore matches a leading backslash, any single character, and a backslash. It never matches the four-character device prefix\\.\, because that prefix has two backslashes before the dot. Two effects follow:
- A path such as
\\.\C:\Users\...keeps its prefix and does not fold to the same key asC:\Users\....- A path shaped like
\a\bis stripped tob.Escape the dot and match both leading backslashes.
🐛 Proposed fix
- const withoutExtendedPrefix = filePath.replace(/^\\\\\?\\/, "").replace(/^\\.\\/, ""); + const withoutExtendedPrefix = filePath + .replace(/^\\\\\?\\/, "") + .replace(/^\\\\\.\\/, ""); return withoutExtendedPrefix.toLowerCase();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/ipc/state.ts` around lines 33 - 34, Correct the second replacement in the path-normalization logic so it matches and removes the literal `\\.\` device prefix, including both leading backslashes, rather than treating the dot as a wildcard. Preserve the existing extended-prefix removal and lowercase normalization in the surrounding expression.scripts/benchmark-cuda4k.mjs-140-146 (1)
140-146: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winHandle a failed spawn before reading
result.stderr.If
spawnSynccannot startnode, it returnsstatus: null,stderr: null, and setsresult.error. Line 142 then enters the failure branch and Line 144 throwsTypeError: Cannot read properties of null (reading 'slice'). The real launch error is lost.🐛 Proposed fix
const result = spawnSync("node", args, { env, encoding: "utf8", maxBuffer: 512 * 1024 * 1024 }); const elapsedMs = performance.now() - startedAt; +if (result.error) { + console.error("Benchmark run could not start", result.error.message); + process.exit(1); +} if (result.status !== 0) { console.error("Benchmark run failed", result.status); - console.error(result.stderr.slice(-4000)); + console.error((result.stderr ?? "").slice(-4000)); process.exit(1); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/benchmark-cuda4k.mjs` around lines 140 - 146, Update the failure handling after spawnSync in the benchmark execution flow to handle result.error and null result.stderr before calling slice. Report the launch error when node cannot be started, while preserving the existing stderr output for processes that start and exit unsuccessfully.scripts/build-nvidia-cuda-compositor.mjs-148-155 (1)
148-155: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDirectory names sort lexicographically, not by version.
readdirSync(...).sort().reverse()ordersv12.6beforev9.0only by string comparison."v9.0" > "v12.6"in lexicographic order, sov9.0is preferred. The build then selects an older CUDA Toolkit when both are installed.Sort by parsed major and minor version numbers instead.
🐛 Proposed fix
+ const versionKey = (name) => { + const match = /^v?(\d+)\.(\d+)/.exec(name); + return match ? Number(match[1]) * 1000 + Number(match[2]) : -1; + }; if (existsSync(cudaInstallRoot)) { candidates.push( ...readdirSync(cudaInstallRoot) - .sort() - .reverse() + .sort((left, right) => versionKey(right) - versionKey(left)) .map((version) => path.join(cudaInstallRoot, version)), ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/build-nvidia-cuda-compositor.mjs` around lines 148 - 155, Update the candidate ordering in the cudaInstallRoot scan, using the version values from readdirSync(cudaInstallRoot) and sorting by parsed numeric major and minor components in descending order instead of applying lexicographic sort().reverse(). Preserve the existing path.join mapping so the newest CUDA Toolkit is preferred.electron/mediaServer.test.ts-110-115 (1)
110-115: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winClose the media server in test teardown.
vi.resetModules()does not close the HTTP server created byensureMediaServer(). Each HTTP test leaves a listening server handle after Line 111. Add an awaited test cleanup API that closes the server and resets its singleton state.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/mediaServer.test.ts` around lines 110 - 115, Update the afterEach teardown around ensureMediaServer to await the media server’s cleanup API before resetting modules and removing tempRoot. The cleanup must close the HTTP server and reset its singleton state so each test releases its listening handle.AGENTS.md-19-22 (1)
19-22: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winSet a language on both command fences.
Markdownlint MD040 reports both fences. Use
shfor the commands.
AGENTS.md#L19-L22: Change the opening fence to```sh.AGENTS.md#L44-L46: Change the opening fence to```sh.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@AGENTS.md` around lines 19 - 22, Set the language identifier to sh on both command code fences in AGENTS.md: update the fence at lines 19-22 and the fence at lines 44-46 from an unspecified language to ```sh, leaving the command contents unchanged.Source: Linters/SAST tools
src/lib/exporter/types.ts-237-239 (1)
237-239: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winApply the codec bitrate ceiling before custom bitrate usage.
resolveExportBitrate()convertscustomMbpsthroughcustomBitrateMbpsToBps(), which clamps only to the global 200 Mbps ceiling.ExportSettingsMenuapplies 70/105 Mbps, but persisted or programmatic custom values bypass that UI clamp. Add the codec-specific cap to the custom bitrate path/clampCustomBitrateMbps()instead of relying only on the settings UI.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/exporter/types.ts` around lines 237 - 239, Update resolveExportBitrate() and/or clampCustomBitrateMbps() so custom bitrate values are capped at the codec-specific 70/105 Mbps ceiling before customBitrateMbpsToBps() uses them, including persisted or programmatic values. Keep the existing global 200 Mbps clamp and ExportSettingsMenu behavior intact, and centralize the codec cap in the shared custom-bitrate path rather than relying on UI validation.electron/native/nvidia-cuda-compositor/tiledOverlayManifest.mjs-212-227 (1)
212-227: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winValidate
delta.changedTilesas an array before iterating.The reader parses the manifest from disk and validates every other field with a descriptive
failmessage. A delta withoutchangedTiles, or with a non-array value, reachesfor (const record of delta.changedTiles)and throws a bareTypeErrorinstead of a manifest error that names the layer and path.🛡️ Proposed guard
previousFrameIndex = delta.frameIndex; + if (!Array.isArray(delta.changedTiles)) { + fail( + `Tiled overlay layer ${id} has a delta without changed tile records: ${resolvedPath}`, + ); + } const seenDeltaTiles = new Set();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/native/nvidia-cuda-compositor/tiledOverlayManifest.mjs` around lines 212 - 227, Validate that each delta’s changedTiles field is an array before the for-of iteration in the delta-processing loop. If invalid or missing, call fail with a descriptive manifest error including the layer id and resolvedPath, then preserve the existing changed-tile validation for valid arrays.electron/preload.ts-508-546 (1)
508-546: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winBound frames on the transferable export path.
writeNativeVideoExportFramesViaChannelposts all frames inframeDataListbefore awaiting acknowledgements.frameDataListcomes from the exporter’s batch, so apply the main-processmaxQueuedWriteBytesbackpressure before posting frames, or enforce an explicit in-flight limit inelectron/preload.ts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/preload.ts` around lines 508 - 546, The writeNativeVideoExportFramesViaChannel flow currently posts every frame before awaiting acknowledgements, allowing unbounded queued data. Apply the existing maxQueuedWriteBytes backpressure or an explicit in-flight limit while iterating frameDataList, awaiting acknowledgements or otherwise limiting outstanding bytes before posting more frames, while preserving the existing fallback and result handling.Source: Coding guidelines
| function ensureNvEncHeaders() { | ||
| if (existsSync(path.join(NVENC_HEADERS_INCLUDE, "nvEncodeAPI.h"))) { | ||
| return; | ||
| } | ||
| console.log(`[build-nvidia-cuda-compositor] Cloning nv-codec-headers ${NVENC_HEADERS_TAG}...`); | ||
| execSync( | ||
| `git clone --depth 1 --branch ${NVENC_HEADERS_TAG} https://github.com/FFmpeg/nv-codec-headers.git "${nvEncHeadersRoot}"`, | ||
| { stdio: "inherit" }, | ||
| ); | ||
| if (!existsSync(path.join(NVENC_HEADERS_INCLUDE, "nvEncodeAPI.h"))) { | ||
| fallbackToBundledHelperOrExit( | ||
| `nv-codec-headers ${NVENC_HEADERS_TAG} could not be staged; a Blackwell-compatible nvEncodeAPI.h is required.`, | ||
| ); | ||
| } | ||
| // The legacy samples checkout ships nvEncodeAPI.h 8.1; the compiler picks the | ||
| // quoted include from the NvEncoder directory first, so the 13.0 header must | ||
| // replace it to build the encoder library against the current API. | ||
| const samplesHeader = path.join( | ||
| videoCodecSdkRoot, | ||
| "Samples", | ||
| "NvCodec", | ||
| "NvEncoder", | ||
| "nvEncodeAPI.h", | ||
| ); | ||
| const versionLine = readFileSync(path.join(NVENC_HEADERS_INCLUDE, "nvEncodeAPI.h"), "utf8") | ||
| .split(/\r?\n/) | ||
| .find((line) => line.includes("NVENCAPI_MAJOR_VERSION")); | ||
| if (existsSync(samplesHeader) && !/NVENCAPI_MAJOR_VERSION 13/.test(versionLine ?? "")) { | ||
| copyFileSync(path.join(NVENC_HEADERS_INCLUDE, "nvEncodeAPI.h"), samplesHeader); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
The legacy 8.1 samples header is never replaced.
Two defects prevent the SDK sample header replacement from running:
- Lines 166-168 return early when the staged
nv-codec-headerscheckout already containsnvEncodeAPI.h. On any repeat build, or when a user setsRECORDLY_NVENC_HEADERS_ROOTto an existing checkout, the replacement code at Lines 182-194 never executes. - Line 189 reads
versionLinefrom the staged header atNVENC_HEADERS_INCLUDE, not fromsamplesHeader. That staged header is the pinnedn13.0.19.1release, so it always reportsNVENCAPI_MAJOR_VERSION 13. The guard at Line 192 therefore evaluates!trueand skipscopyFileSyncin the expected case.
The combined effect is that Samples/NvCodec/NvEncoder/nvEncodeAPI.h keeps the legacy 8.1 header. The compiler prefers that quoted include, so the encoder library builds against API 8.1 and fails at runtime with NV_ENC_ERR_INVALID_PARAM, which is the exact failure the comment at Lines 42-45 describes.
Read the version from samplesHeader and run the replacement on every invocation.
🐛 Proposed fix
function ensureNvEncHeaders() {
- if (existsSync(path.join(NVENC_HEADERS_INCLUDE, "nvEncodeAPI.h"))) {
- return;
- }
- console.log(`[build-nvidia-cuda-compositor] Cloning nv-codec-headers ${NVENC_HEADERS_TAG}...`);
- execSync(
- `git clone --depth 1 --branch ${NVENC_HEADERS_TAG} https://github.com/FFmpeg/nv-codec-headers.git "${nvEncHeadersRoot}"`,
- { stdio: "inherit" },
- );
+ const stagedHeader = path.join(NVENC_HEADERS_INCLUDE, "nvEncodeAPI.h");
+ if (!existsSync(stagedHeader)) {
+ console.log(
+ `[build-nvidia-cuda-compositor] Cloning nv-codec-headers ${NVENC_HEADERS_TAG}...`,
+ );
+ execSync(
+ `git clone --depth 1 --branch ${NVENC_HEADERS_TAG} https://github.com/FFmpeg/nv-codec-headers.git "${nvEncHeadersRoot}"`,
+ { stdio: "inherit" },
+ );
+ }
if (!existsSync(path.join(NVENC_HEADERS_INCLUDE, "nvEncodeAPI.h"))) {
fallbackToBundledHelperOrExit(
`nv-codec-headers ${NVENC_HEADERS_TAG} could not be staged; a Blackwell-compatible nvEncodeAPI.h is required.`,
);
}
// The legacy samples checkout ships nvEncodeAPI.h 8.1; the compiler picks the
// quoted include from the NvEncoder directory first, so the 13.0 header must
// replace it to build the encoder library against the current API.
const samplesHeader = path.join(
videoCodecSdkRoot,
"Samples",
"NvCodec",
"NvEncoder",
"nvEncodeAPI.h",
);
- const versionLine = readFileSync(path.join(NVENC_HEADERS_INCLUDE, "nvEncodeAPI.h"), "utf8")
- .split(/\r?\n/)
- .find((line) => line.includes("NVENCAPI_MAJOR_VERSION"));
- if (existsSync(samplesHeader) && !/NVENCAPI_MAJOR_VERSION 13/.test(versionLine ?? "")) {
- copyFileSync(path.join(NVENC_HEADERS_INCLUDE, "nvEncodeAPI.h"), samplesHeader);
+ if (!existsSync(samplesHeader)) {
+ return;
}
+ const samplesVersionLine = readFileSync(samplesHeader, "utf8")
+ .split(/\r?\n/)
+ .find((line) => line.includes("NVENCAPI_MAJOR_VERSION"));
+ if (!/NVENCAPI_MAJOR_VERSION\s+13/.test(samplesVersionLine ?? "")) {
+ copyFileSync(stagedHeader, samplesHeader);
+ }
}As per coding guidelines: "Build the NVIDIA compositor against nv-codec-headers API 13.x pinned at n13.0.19.1, not the legacy 8.1 SDK sample header, and apply the API-13 sample compatibility patch."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function ensureNvEncHeaders() { | |
| if (existsSync(path.join(NVENC_HEADERS_INCLUDE, "nvEncodeAPI.h"))) { | |
| return; | |
| } | |
| console.log(`[build-nvidia-cuda-compositor] Cloning nv-codec-headers ${NVENC_HEADERS_TAG}...`); | |
| execSync( | |
| `git clone --depth 1 --branch ${NVENC_HEADERS_TAG} https://github.com/FFmpeg/nv-codec-headers.git "${nvEncHeadersRoot}"`, | |
| { stdio: "inherit" }, | |
| ); | |
| if (!existsSync(path.join(NVENC_HEADERS_INCLUDE, "nvEncodeAPI.h"))) { | |
| fallbackToBundledHelperOrExit( | |
| `nv-codec-headers ${NVENC_HEADERS_TAG} could not be staged; a Blackwell-compatible nvEncodeAPI.h is required.`, | |
| ); | |
| } | |
| // The legacy samples checkout ships nvEncodeAPI.h 8.1; the compiler picks the | |
| // quoted include from the NvEncoder directory first, so the 13.0 header must | |
| // replace it to build the encoder library against the current API. | |
| const samplesHeader = path.join( | |
| videoCodecSdkRoot, | |
| "Samples", | |
| "NvCodec", | |
| "NvEncoder", | |
| "nvEncodeAPI.h", | |
| ); | |
| const versionLine = readFileSync(path.join(NVENC_HEADERS_INCLUDE, "nvEncodeAPI.h"), "utf8") | |
| .split(/\r?\n/) | |
| .find((line) => line.includes("NVENCAPI_MAJOR_VERSION")); | |
| if (existsSync(samplesHeader) && !/NVENCAPI_MAJOR_VERSION 13/.test(versionLine ?? "")) { | |
| copyFileSync(path.join(NVENC_HEADERS_INCLUDE, "nvEncodeAPI.h"), samplesHeader); | |
| } | |
| } | |
| function ensureNvEncHeaders() { | |
| const stagedHeader = path.join(NVENC_HEADERS_INCLUDE, "nvEncodeAPI.h"); | |
| if (!existsSync(stagedHeader)) { | |
| console.log( | |
| `[build-nvidia-cuda-compositor] Cloning nv-codec-headers ${NVENC_HEADERS_TAG}...`, | |
| ); | |
| execSync( | |
| `git clone --depth 1 --branch ${NVENC_HEADERS_TAG} https://github.com/FFmpeg/nv-codec-headers.git "${nvEncHeadersRoot}"`, | |
| { stdio: "inherit" }, | |
| ); | |
| } | |
| if (!existsSync(path.join(NVENC_HEADERS_INCLUDE, "nvEncodeAPI.h"))) { | |
| fallbackToBundledHelperOrExit( | |
| `nv-codec-headers ${NVENC_HEADERS_TAG} could not be staged; a Blackwell-compatible nvEncodeAPI.h is required.`, | |
| ); | |
| } | |
| // The legacy samples checkout ships nvEncodeAPI.h 8.1; the compiler picks the | |
| // quoted include from the NvEncoder directory first, so the 13.0 header must | |
| // replace it to build the encoder library against the current API. | |
| const samplesHeader = path.join( | |
| videoCodecSdkRoot, | |
| "Samples", | |
| "NvCodec", | |
| "NvEncoder", | |
| "nvEncodeAPI.h", | |
| ); | |
| if (!existsSync(samplesHeader)) { | |
| return; | |
| } | |
| const samplesVersionLine = readFileSync(samplesHeader, "utf8") | |
| .split(/\r?\n/) | |
| .find((line) => line.includes("NVENCAPI_MAJOR_VERSION")); | |
| if (!/NVENCAPI_MAJOR_VERSION\s+13/.test(samplesVersionLine ?? "")) { | |
| copyFileSync(stagedHeader, samplesHeader); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/build-nvidia-cuda-compositor.mjs` around lines 165 - 195, Update
ensureNvEncHeaders so it does not return before the SDK sample-header
replacement logic runs; only skip cloning when the staged header already exists.
Read versionLine from samplesHeader, and replace that file whenever it exists
and is not API 13, preserving the pinned nv-codec-headers API 13.x source and
existing fallback behavior.
Source: Coding guidelines
- strict HEVC Hardware hard-fails (noCpuFallback) instead of rawvideo fallback in the route planner - enforce codec-specific custom bitrate caps (H.264 105 / HEVC 70 Mbps) in resolve/clamp/persistence - reject temporal blur plans below the CUDA minimum sample count instead of dropping the effect - bound completed frame request ids with a monotonic watermark - discard produced native temp video on HEVC route rejection - deterministic precomposited overlay z-order - clamp bitrate input on change; update tests and codec range strings
|
|
1 similar comment
|
|
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/components/video-editor/ExportSettingsMenu.tsx (1)
117-126: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winReuse the exported codec caps and re-clamp the draft when the codec changes.
Line 117 hardcodes 70 and 105.
src/lib/exporter/types.tsalready exportsEXPORT_BITRATE_HEVC_MAX_MBPS(70) andEXPORT_BITRATE_H264_MAX_MBPS(105). Import them so a cap change stays in one place.The effect at lines 113-115 only resyncs the draft when
exportBitrateMbpschanges. If the user selects 105 Mbps with H.264 and then switches to H.265,effectiveMaxMbpsbecomes 70 whilebitrateDraftandexportBitrateMbpsstay at 105. TheInputandSliderthen show a value above their ownmax. The export itself stays correct becauseclampCustomBitrateMbpsre-clamps by codec, so this is a display and persisted-preference inconsistency only.♻️ Proposed fix
- const effectiveMaxMbps = exportVideoCodec === "hevc" ? 70 : 105; + const effectiveMaxMbps = + exportVideoCodec === "hevc" ? EXPORT_BITRATE_HEVC_MAX_MBPS : EXPORT_BITRATE_H264_MAX_MBPS; + + useEffect(() => { + if (exportBitrateMbps > effectiveMaxMbps) { + setBitrateDraft(String(effectiveMaxMbps)); + onExportBitrateMbpsChange?.(effectiveMaxMbps); + } + }, [effectiveMaxMbps, exportBitrateMbps, onExportBitrateMbpsChange]);Add the constants to the existing import from the exporter types module.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/video-editor/ExportSettingsMenu.tsx` around lines 117 - 126, Update ExportSettingsMenu’s effectiveMaxMbps to use the exported EXPORT_BITRATE_HEVC_MAX_MBPS and EXPORT_BITRATE_H264_MAX_MBPS constants instead of hardcoded values. Extend the draft synchronization effect to re-clamp and persist the current bitrate when exportVideoCodec changes, keeping bitrateDraft and exportBitrateMbps within the selected codec’s cap.src/lib/exporter/exportBitrate.ts (1)
168-183: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep auto bitrate HEVC capped at 70 Mbps.
resolveExportBitrateonly passesoptions.codecon thecustombranch. Auto mode can still exceedEXPORT_BITRATE_HEVC_MAX_MBPSfor large/source presets, while customhevcinput is capped bygetCodecCustomBitrateCapMbps. Apply the same codec cap aftergetMp4ExportBitrateor before passing auto bitrate into the export config.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/exporter/exportBitrate.ts` around lines 168 - 183, Update resolveExportBitrate so the automatic bitrate returned by getMp4ExportBitrate is also capped using options.codec and EXPORT_BITRATE_HEVC_MAX_MBPS, while preserving the existing custom-mode cap via customBitrateMbpsToBps.electron/ipc/export/nativeStaticLayoutRoutePlan.ts (1)
156-167: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winPropagate
noCpuFallbackthrough static-layout failures.The selected CUDA route has
noCpuFallback: false, but it also affects post-route failures for HEVC Hardware: if helper, IPC, route mismatch, or post-export validation fails, the exporter currently logs/stops without the required hard-fail error andnoCpuFallback: true. Preserve the strict policy for hardware HEVC GPU routes so later failures use the same actionable hard-fail path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/ipc/export/nativeStaticLayoutRoutePlan.ts` around lines 156 - 167, Update the static-layout export flow around the selectedRoute result so HEVC Hardware CUDA routes propagate noCpuFallback through helper, IPC, route-mismatch, and post-export validation failures. Ensure each such failure uses the existing actionable hard-fail error path and reports noCpuFallback as true, while preserving normal behavior for non-strict routes.Source: Coding guidelines
🧹 Nitpick comments (5)
electron/native/nvidia-cuda-compositor/run-mp4-pipeline.mjs (1)
1376-1376: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueCache the
--helpprobe output.Lines 1376 and 1430 each spawn the native helper with
--help. An export that uses temporal blur and cursor-sprite overlays pays for two extra child processes. The tiled-overlay probe adds another. Resolve the capability text once and reuse it.♻️ Proposed refactor
+let cachedNativeHelp = null; +function getNativeHelp() { + if (cachedNativeHelp === null) { + cachedNativeHelp = run(nativeProbe, ["--help"]).stdout; + } + return cachedNativeHelp; +}Then replace each
run(nativeProbe, ["--help"]).stdoutcall withgetNativeHelp().Also applies to: 1430-1430
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/native/nvidia-cuda-compositor/run-mp4-pipeline.mjs` at line 1376, Cache the native helper capability text by introducing a shared getNativeHelp() accessor around the --help probe, reusing the same resolved output for every check. Update the probes at nativeHelp and the corresponding tiled-overlay/temporal-blur capability check to call getNativeHelp() instead of spawning run(nativeProbe, ["--help"]) directly.electron/ipc/register/export.ts (1)
344-356: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCompute
requestSettingsonce outside thetryblock.The catch handler rebuilds the same string with the same inputs. Hoist the call above
tryand reuse it in both log lines. This removes the duplicate and keeps the two log lines guaranteed identical.♻️ Proposed refactor
const encoderPreference = options.encoderPreference ?? "auto"; + const requestSettings = formatNativeExportRequestSettings({ + videoCodec, + encoderPreference, + inputMode, + encodingMode: options.encodingMode, + width: options.width, + height: options.height, + frameRate: options.frameRate, + }); let sessionId = "";Then delete the in-
trydeclaration at lines 344-352 and thefailedRequestSettingsdeclaration at lines 503-511, and userequestSettingsin the catch log.Also applies to: 503-514
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/ipc/register/export.ts` around lines 344 - 356, Hoist the formatNativeExportRequestSettings call above the try block and retain its result as requestSettings. Remove the duplicate in-try requestSettings and failedRequestSettings declarations, then reuse requestSettings in both the start and catch logs so they remain identical.electron/ipc/export/native-video.test.ts (1)
424-436: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the second assertion match the test name.
The test is named "invalidates on any current route mismatch even when identity matches". Lines 432-435 set
current.requestedCodec = "hevc"andcurrent.encoderPreference = "hardware", which are already the values returned bybaseCurrent(). The assertion therefore repeats the reuse case already covered at lines 350-352 and assertstrueinside a test about mismatches. Change it to a real mismatch onrequestedCodecorencoderPreference.💚 Proposed fix
const current = baseCurrent(); - current.requestedCodec = "hevc"; - current.encoderPreference = "hardware"; - expect(canReuseNativeStaticLayoutSourceProbe(baseEntry(), current)).toBe(true); + current.requestedCodec = "h264"; + expect(canReuseNativeStaticLayoutSourceProbe(baseEntry(), current)).toBe(false); + const preferenceMismatch = baseCurrent(); + preferenceMismatch.encoderPreference = "cpu"; + expect(canReuseNativeStaticLayoutSourceProbe(baseEntry(), preferenceMismatch)).toBe(false);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/ipc/export/native-video.test.ts` around lines 424 - 436, Update the second assertion in the test around canReuseNativeStaticLayoutSourceProbe so current.requestedCodec or current.encoderPreference differs from baseCurrent() while identity remains matching. Keep the assertion expecting false, ensuring the test name’s route-mismatch behavior is exercised rather than repeating the reuse case.src/lib/exporter/modernVideoExporter.ts (1)
2680-2688: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider gating the cursor-sprite contract on the Windows platform.
canUseNativeCursorAtlasOwnershiprequiresgetRuntimePlatform() === "win32"(Lines 2655-2659).canUseNativeCursorSpriteContractdoes not. The generalized NVIDIA CUDA compositor only runs on Windows, so on other platforms the sprite strip and positions sidecar are prepared and then rejected by the route guard at Line 4341. The result stays correct, but the work is wasted. Add the platform check for symmetry.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/exporter/modernVideoExporter.ts` around lines 2680 - 2688, Update canUseNativeCursorSpriteContract to require getRuntimePlatform() === "win32", matching canUseNativeCursorAtlasOwnership, while preserving the existing CPU and experimental export preference checks.src/lib/exporter/modernVideoExporter.overlayPreparation.test.ts (1)
242-247: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
mockResetremoves the default cursor-sprite implementations.The hoisted mocks define
startCursorSpriteCaptureas() => falseandcaptureCursorSpriteFrameas an unavailable result (Lines 45-49).mockReset()clears those implementations, so the mocks returnundefinedafter each reset. The comment states that the default unavailable fallback applies, which is no longer accurate. The tests still pass becauseundefinedis falsy at thestartCursorSpriteCapturecheck, but any future test that reachescaptureCursorSpriteFramewithout an explicitmockReturnValuefails with a TypeError oncapture.captured.Restore the defaults after the reset, or use
mockClear().♻️ Proposed fix
mocks.frameRendererStartCursorSpriteCapture.mockReset(); mocks.frameRendererCaptureCursorSpriteFrame.mockReset(); mocks.frameRendererFinishCursorSpriteCapture.mockReset(); mocks.frameRendererCancelCursorSpriteCapture.mockReset(); + mocks.frameRendererStartCursorSpriteCapture.mockReturnValue(false); + mocks.frameRendererCaptureCursorSpriteFrame.mockReturnValue({ + captured: false, + unavailableReason: "cursor sprite unavailable (mock)", + }); + mocks.frameRendererFinishCursorSpriteCapture.mockReturnValue(null);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/exporter/modernVideoExporter.overlayPreparation.test.ts` around lines 242 - 247, Update the cursor-sprite mock reset block for frameRendererStartCursorSpriteCapture and frameRendererCaptureCursorSpriteFrame so their hoisted unavailable fallback implementations are restored after resetting, or replace mockReset() with mockClear() where implementation preservation is intended. Keep the finish and cancel mocks’ existing reset behavior unless they also require default implementations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@electron/native/nvidia-cuda-compositor/run-mp4-pipeline.mjs`:
- Around line 1394-1401: Update the temporalBlurSampleCount branch in the CUDA
compositor pipeline to call fail() instead of console.warn() when the requested
count is below the minimum of 3. Match the sibling validation near the
resolved-plan check so the export aborts with the
unsupported-temporal-motion-blur diagnostic rather than continuing without the
effect.
- Around line 1406-1411: Update the overlay classification flow around
readOverlayManifest so each returned layer carries its manifest-derived kind,
order, positionsPath, and positions metadata before rgbaOverlayLayers and
cursorSpriteLayers are filtered or consumed. Preserve the existing layer data
while mapping the manifest fields, or derive kind from the manifest’s layer
type, ensuring requested overlays and cursor sprite arguments receive the
correct entries.
In `@src/lib/exporter/modernVideoExporter.ts`:
- Around line 5276-5291: Update the progress handling logic around the
preparing-signal suppression and reset lastPreparingTotalFrames whenever a
non-preparing progress phase is reported, so a later preparing signal with the
same totalFrames is delivered. Preserve the existing duplicate-suppression
behavior for consecutive preparing signals.
- Around line 4334-4368: Delete the produced native temporary video before
returning null in both rejection branches guarded by hasCursorSpriteLayer and
webcamNativeOwned, matching the cleanup performed by the earlier rejection
paths. Keep restoreEncoderState and the existing skip-reason assignments intact.
---
Outside diff comments:
In `@electron/ipc/export/nativeStaticLayoutRoutePlan.ts`:
- Around line 156-167: Update the static-layout export flow around the
selectedRoute result so HEVC Hardware CUDA routes propagate noCpuFallback
through helper, IPC, route-mismatch, and post-export validation failures. Ensure
each such failure uses the existing actionable hard-fail error path and reports
noCpuFallback as true, while preserving normal behavior for non-strict routes.
In `@src/components/video-editor/ExportSettingsMenu.tsx`:
- Around line 117-126: Update ExportSettingsMenu’s effectiveMaxMbps to use the
exported EXPORT_BITRATE_HEVC_MAX_MBPS and EXPORT_BITRATE_H264_MAX_MBPS constants
instead of hardcoded values. Extend the draft synchronization effect to re-clamp
and persist the current bitrate when exportVideoCodec changes, keeping
bitrateDraft and exportBitrateMbps within the selected codec’s cap.
In `@src/lib/exporter/exportBitrate.ts`:
- Around line 168-183: Update resolveExportBitrate so the automatic bitrate
returned by getMp4ExportBitrate is also capped using options.codec and
EXPORT_BITRATE_HEVC_MAX_MBPS, while preserving the existing custom-mode cap via
customBitrateMbpsToBps.
---
Nitpick comments:
In `@electron/ipc/export/native-video.test.ts`:
- Around line 424-436: Update the second assertion in the test around
canReuseNativeStaticLayoutSourceProbe so current.requestedCodec or
current.encoderPreference differs from baseCurrent() while identity remains
matching. Keep the assertion expecting false, ensuring the test name’s
route-mismatch behavior is exercised rather than repeating the reuse case.
In `@electron/ipc/register/export.ts`:
- Around line 344-356: Hoist the formatNativeExportRequestSettings call above
the try block and retain its result as requestSettings. Remove the duplicate
in-try requestSettings and failedRequestSettings declarations, then reuse
requestSettings in both the start and catch logs so they remain identical.
In `@electron/native/nvidia-cuda-compositor/run-mp4-pipeline.mjs`:
- Line 1376: Cache the native helper capability text by introducing a shared
getNativeHelp() accessor around the --help probe, reusing the same resolved
output for every check. Update the probes at nativeHelp and the corresponding
tiled-overlay/temporal-blur capability check to call getNativeHelp() instead of
spawning run(nativeProbe, ["--help"]) directly.
In `@src/lib/exporter/modernVideoExporter.overlayPreparation.test.ts`:
- Around line 242-247: Update the cursor-sprite mock reset block for
frameRendererStartCursorSpriteCapture and frameRendererCaptureCursorSpriteFrame
so their hoisted unavailable fallback implementations are restored after
resetting, or replace mockReset() with mockClear() where implementation
preservation is intended. Keep the finish and cancel mocks’ existing reset
behavior unless they also require default implementations.
In `@src/lib/exporter/modernVideoExporter.ts`:
- Around line 2680-2688: Update canUseNativeCursorSpriteContract to require
getRuntimePlatform() === "win32", matching canUseNativeCursorAtlasOwnership,
while preserving the existing CPU and experimental export preference checks.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ab9328c1-8e37-432d-8200-895beac6ee8f
📒 Files selected for processing (24)
electron/ipc/export/native-video.test.tselectron/ipc/export/native-video.tselectron/ipc/export/nativeStaticLayoutRoutePlan.test.tselectron/ipc/export/nativeStaticLayoutRoutePlan.tselectron/ipc/nativeVideoExport.test.tselectron/ipc/nativeVideoExport.tselectron/ipc/register/export.tselectron/native/nvidia-cuda-compositor/run-mp4-pipeline.mjssrc/components/video-editor/ExportSettingsMenu.tsxsrc/components/video-editor/editorPreferences.test.tssrc/components/video-editor/projectPersistence.test.tssrc/components/video-editor/projectPersistence.tssrc/i18n/locales/de/settings.jsonsrc/i18n/locales/en/settings.jsonsrc/i18n/locales/es/settings.jsonsrc/i18n/locales/fr/settings.jsonsrc/i18n/locales/it/settings.jsonsrc/i18n/locales/ko/settings.jsonsrc/lib/exporter/exportBitrate.test.tssrc/lib/exporter/exportBitrate.tssrc/lib/exporter/modernVideoExporter.nativeStaticLayout.test.tssrc/lib/exporter/modernVideoExporter.overlayPreparation.test.tssrc/lib/exporter/modernVideoExporter.tssrc/lib/exporter/types.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- src/i18n/locales/es/settings.json
- src/i18n/locales/it/settings.json
- src/i18n/locales/en/settings.json
- src/components/video-editor/projectPersistence.test.ts
- src/lib/exporter/exportBitrate.test.ts
- src/i18n/locales/de/settings.json
- src/i18n/locales/ko/settings.json
- src/i18n/locales/fr/settings.json
- electron/ipc/nativeVideoExport.test.ts
- src/components/video-editor/projectPersistence.ts
- src/lib/exporter/types.ts
- electron/ipc/nativeVideoExport.ts
| const overlayLayers = readOverlayManifest(overlayManifest, { | ||
| outputWidth, | ||
| outputHeight, | ||
| }); | ||
| const rgbaOverlayLayers = overlayLayers.filter((layer) => layer.kind === "rgba"); | ||
| const cursorSpriteLayers = overlayLayers.filter((layer) => layer.kind === "cursor-sprite"); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Confirm readOverlayManifest emits kind/order/positionsPath/positions for overlay layers.
fd overlayManifest.mjs --exec rg -n 'kind|order|positionsPath|positions|layers.push' {}
rg -n 'kind\s*[:=]|cursor-sprite' electron/native/nvidia-cuda-compositor --glob '*.mjs'Repository: webadderallorg/Recordly
Length of output: 1414
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "tracked overlayManifest.mjs files:"
git ls-files '*overlayManifest.mjs' || true
echo
echo "run-mp4-pipeline relevant sections:"
fd run-mp4-pipeline.mjs --exec sh -c 'echo "--- $1"; wc -l "$1"; sed -n "1385,1425p;1710,1745p" "$1"' sh {}
echo
echo "overlayManifest.mjs relevant sections:"
fd overlayManifest.mjs --exec sh -c 'echo "--- $1"; wc -l "$1"; sed -n "1,220p" "$1"' sh {}Repository: webadderallorg/Recordly
Length of output: 7328
Use manifest fields to classify each overlay layer.
readOverlayManifest() returns only id, path, dimensions, frame counts, and physical-path metadata; it does not attach kind. The filter(...layer.kind === ...), layer.positionsPath, and layer.positions reads therefore treat every manifest entry as missing/undefined, so the summary omits requested layers and cursor sprite args may miss the layer data. Populate kind, order, positionsPath, and positions from the manifest or derive kind from layer type before filtering.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@electron/native/nvidia-cuda-compositor/run-mp4-pipeline.mjs` around lines
1406 - 1411, Update the overlay classification flow around readOverlayManifest
so each returned layer carries its manifest-derived kind, order, positionsPath,
and positions metadata before rgbaOverlayLayers and cursorSpriteLayers are
filtered or consumed. Preserve the existing layer data while mapping the
manifest fields, or derive kind from the manifest’s layer type, ensuring
requested overlays and cursor sprite arguments receive the correct entries.
Source: Coding guidelines
- fail with unsupported-temporal-motion-blur below the CUDA minimum sample count instead of warning - attach manifest kind/order to overlay layers so classification and z-order survive - cache the native --help capability probe per export - discard produced temp video on cursor-sprite and webcam route rejection - reset the preparing progress watermark after non-preparing progress - reuse codec bitrate caps in the export settings and re-clamp on codec switch - cap HEVC auto bitrate at 70 Mbps - propagate noCpuFallback through HEVC Hardware post-route failures - hoist requestSettings computation; align native-video test assertion; restore cursor-sprite test defaults
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
electron/ipc/register/export.ts (2)
390-396: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winWait for prewarm teardown before starting FFmpeg.
Line 394 cancels the prewarm without waiting for its NVENC session to release. Line 396 can then start a competing hardware encoder session. This can cause a transient export failure.
Wait for bounded prewarm teardown before
spawn. If teardown fails for HEVC, return an actionable hard failure withnoCpuFallbackmetadata. Do not continue with a CPU or renderer fallback.As per coding guidelines, HEVC setup failures must hard-fail without CPU or renderer fallback.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/ipc/register/export.ts` around lines 390 - 396, Update the export flow around cancelInFlightCapabilityOnlyPrewarms and the subsequent ffmpegProcess spawn to await bounded prewarm teardown before starting FFmpeg. If teardown fails for HEVC, return an actionable hard failure carrying noCpuFallback metadata, and do not proceed to CPU or renderer fallback paths.Source: Coding guidelines
792-810: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winBlock new frame writes before finalization.
native-video-export-finishdoes not mark the session as finishing beforeawait session.writeSequence. A frame-port or cloned IPC request can append a write after that promise was captured. The handler can then close stdin before that accepted frame writes.Add a
finishingstate before the firstawait. Reject and settle subsequent frame requests. Then wait only for writes accepted before the state transition.As per coding guidelines, native-frame transport requires sequence ordering, acknowledgements, cancellation settlement, and backpressure.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/ipc/register/export.ts` around lines 792 - 810, Update the native-video-export finish handler around session.writeSequence to mark the session as finishing before the first await, then reject and settle any subsequent frame requests. Capture and await only the write sequence accepted before that transition, preserving ordered acknowledgements, cancellation settlement, and backpressure before closing stdin and finalizing via muxNativeVideoExportAudio.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@electron/native/nvidia-cuda-compositor/overlayManifest.mjs`:
- Around line 225-236: Sort the completed overlay layer collection by ascending
order in readOverlayManifest before the --overlay and --cursor-sprite
argument-building loops consume it. Use the normalized order assigned in the
layer objects, preserving manifest-defined values and the layers.length fallback
so mixed rgba/cursor-sprite manifests retain the cursor-sprite’s default z-order
above fixed-position layers.
---
Outside diff comments:
In `@electron/ipc/register/export.ts`:
- Around line 390-396: Update the export flow around
cancelInFlightCapabilityOnlyPrewarms and the subsequent ffmpegProcess spawn to
await bounded prewarm teardown before starting FFmpeg. If teardown fails for
HEVC, return an actionable hard failure carrying noCpuFallback metadata, and do
not proceed to CPU or renderer fallback paths.
- Around line 792-810: Update the native-video-export finish handler around
session.writeSequence to mark the session as finishing before the first await,
then reject and settle any subsequent frame requests. Capture and await only the
write sequence accepted before that transition, preserving ordered
acknowledgements, cancellation settlement, and backpressure before closing stdin
and finalizing via muxNativeVideoExportAudio.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ca19071e-99a5-4ed7-a0de-7c78326558d4
📒 Files selected for processing (15)
electron/ipc/export/native-video.test.tselectron/ipc/export/nativeStaticLayoutRoutePlan.test.tselectron/ipc/export/nativeStaticLayoutRoutePlan.tselectron/ipc/register/export.tselectron/native/nvidia-cuda-compositor/overlayManifest.mjselectron/native/nvidia-cuda-compositor/overlayManifest.test.mjselectron/native/nvidia-cuda-compositor/run-mp4-pipeline.mjssrc/components/video-editor/ExportSettingsMenu.tsxsrc/lib/exporter/exportBitrate.test.tssrc/lib/exporter/exportBitrate.tssrc/lib/exporter/index.tssrc/lib/exporter/modernVideoExporter.overlayPreparation.test.tssrc/lib/exporter/modernVideoExporter.progressDedup.test.tssrc/lib/exporter/modernVideoExporter.routeRejection.test.tssrc/lib/exporter/modernVideoExporter.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- src/lib/exporter/exportBitrate.test.ts
- src/lib/exporter/modernVideoExporter.overlayPreparation.test.ts
- electron/ipc/export/nativeStaticLayoutRoutePlan.ts
- src/components/video-editor/ExportSettingsMenu.tsx
- electron/ipc/export/native-video.test.ts
- electron/native/nvidia-cuda-compositor/run-mp4-pipeline.mjs
- sort overlay layers by ascending order before building native wrapper args so cursor sprite stays above rgba layers - await prewarm teardown before native spawn; HEVC Hardware teardown failure hard-fails with noCpuFallback - mark native video export sessions finishing before write sequence and reject later frame writes
1 participant
Hey team — I've been using Recordly daily for a while and wanted to get some of the bigger performance wins I kept hitting upstream. This PR is a whole-stack pass on the MP4 export path, with the main goal of making GPU export actually fast and giving us H.265 as an option.
What I tested on:
On that machine, full-canvas exports that were previously stuck at ~14 FPS on the CPU path now hit ~150 FPS through the new CUDA compositor. That's not a typo. The bottleneck shifted from "renderer pushing raw frames over IPC" to "the encoder doing real work."
What's in here:
hevc_nvenc) in addition to H.264.Why H.265: same visual quality at roughly half the file size. Modern platforms and hardware handle it fine, and it's ideal for anything going to social media where storage/bandwidth matters.
Known next steps I didn't want to lump in here:
This is a big diff, but it's been run through the full vitest suite and
tsc --noEmitclean. Happy to split or rebase however makes review easier.Summary by CodeRabbit
.m4asupport, pending media handling, and longer export extensions.