fix(export): route Windows auto through native GPU planner#504
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds structured route planning and capability probes for native static-layout exports, integrates the plan into export execution and ModernVideoExporter, threads resolved source-audio fallback paths, hardens project prune error handling and tests, and replaces integer NV12 sampling with mapped-float cubic/bilinear resampling in the CUDA compositor. ChangesExport Routing and Native Static-Layout Integration
CUDA Compositor Pixel Quality Improvements
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
meiiie
changed the title
meiiie
changed the title
|
@coderabbitai review Ready for another review pass on the latest head Context for this pass:
Local validation on this head:
Please focus especially on native route selection/fallback safety, CUDA helper validation assumptions, and any risk from the bundled helper/manifest update. |
|
Focus areas: native route selection/fallback safety, CUDA helper validation assumptions, and risks from the bundled NVIDIA compositor helper/manifest update. The Windows Lightning ✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/lib/exporter/audioEncoder.test.ts (1)
60-61: ⚡ Quick winConsider validating companion entry gain for completeness.
The test now validates
mainBufferEntry.gain(line 59), but the companion entry gain field introduced in the type definition (line 18) is not tested here. For consistency and to ensure the new gain field works correctly for companion audio, consider adding an assertion.🧪 Suggested test enhancement
expect(prepared.companionEntries).toHaveLength(1); expect(prepared.companionEntries[0]?.buffer).toBe(micBuffer); +expect(prepared.companionEntries[0]?.gain).toBe(1); expect(decodeAudioFromUrl).toHaveBeenCalledWith("file:///tmp/recording.mp4");🤖 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/audioEncoder.test.ts` around lines 60 - 61, Add an assertion that validates the companion entry's gain — e.g., expect(prepared.companionEntries[0]?.gain).toBe(prepared.mainBufferEntry.gain) (or toBe(the expected mic gain value used earlier like micGain) depending on how the test defines expected gain) so the test covers the newly introduced companion gain field on prepared.companionEntries.
🤖 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/ipc/export/native-video.ts`:
- Line 3674: The change sets videoOutputValidated = true inside the GPU success
paths which prevents the later ffprobe-based check in
validateNativeVideoOutputFile() from running; revert that premature flip by
removing or deferring any assignment to the videoOutputValidated flag (the
variable named videoOutputValidated) inside the GPU success branches (the
CUDA/D3D11 success handlers) and only set videoOutputValidated = true after
validateNativeVideoOutputFile() returns success — keep the independent probe
call intact and ensure the GPU paths call validateNativeVideoOutputFile() (or
await its result) and set videoOutputValidated afterwards.
In `@electron/native/nvidia-cuda-compositor/src/main.cu`:
- Around line 1032-1043: The caller is passing x+1/y+1 into mapScaledCoordinate
which shifts the UV sample off the 2x2 luma-block center expected by
sampleNv12UvBilinear; remove the +1 offsets and pass the luma coordinates that
represent the block center (i.e., use x and y as the inputs to
mapScaledCoordinate) so suvX/suvY match sampleNv12UvBilinear's lumaX/lumaY
contract, and make the same change in the other occurrence (the block around
sampleNv12UvBilinear at ~1197-1202).
---
Nitpick comments:
In `@src/lib/exporter/audioEncoder.test.ts`:
- Around line 60-61: Add an assertion that validates the companion entry's gain
— e.g.,
expect(prepared.companionEntries[0]?.gain).toBe(prepared.mainBufferEntry.gain)
(or toBe(the expected mic gain value used earlier like micGain) depending on how
the test defines expected gain) so the test covers the newly introduced
companion gain field on prepared.companionEntries.
🪄 Autofix (Beta)
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: ee03bf86-9eca-48b4-a4d4-07cfa067e316
⛔ Files ignored due to path filters (1)
electron/native/bin/win32-x64/recordly-nvidia-cuda-compositor.exeis excluded by!**/*.exe
📒 Files selected for processing (11)
electron/ipc/export/native-video.test.tselectron/ipc/export/native-video.tselectron/ipc/recording/diagnostics.tselectron/ipc/recording/prune.tselectron/native/bin/win32-x64/helpers-manifest.jsonelectron/native/nvidia-cuda-compositor/src/main.cusrc/lib/exporter/audioEncoder.test.tssrc/lib/exporter/backendPolicy.test.tssrc/lib/exporter/backendPolicy.tssrc/lib/exporter/modernVideoExporter.fallback.test.tssrc/lib/exporter/modernVideoExporter.ts
|
@coderabbitai review Ready for a follow-up review pass on latest head Addressed the previous CodeRabbit feedback:
Validation on this head:
Please re-check the native validation flow, CUDA sampling fix, and helper/manifest update on this latest head. |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/ipc/export/native-video.test.ts`:
- Around line 707-726: The test relies on ambient
RECORDLY_WINDOWS_GPU_EXPORT_PREFER_HIGH_PERFORMANCE_ADAPTER which can make it
flaky; inside the test before calling
buildExperimentalWindowsGpuStaticLayoutArgs (and alongside the existing handling
for RECORDLY_WINDOWS_GPU_EXPORT_ADAPTER_INDEX), save and then explicitly set
process.env.RECORDLY_WINDOWS_GPU_EXPORT_PREFER_HIGH_PERFORMANCE_ADAPTER to a
known value (e.g., "1" or delete it) so the assertion is deterministic, then
restore the original value in the finally block; update the try/finally to
capture originalPrefer =
process.env.RECORDLY_WINDOWS_GPU_EXPORT_PREFER_HIGH_PERFORMANCE_ADAPTER, set the
env to the intended test value, run the assertions against
buildExperimentalWindowsGpuStaticLayoutArgs, and in finally restore
process.env.RECORDLY_WINDOWS_GPU_EXPORT_PREFER_HIGH_PERFORMANCE_ADAPTER to
originalPrefer (deleting if undefined).
In `@electron/ipc/export/native-video.ts`:
- Around line 2099-2102: The probe incorrectly uses packagedAutoCandidateEnabled
when deciding audio bypass eligibility; replace that with
packagedAutoCandidateActive so only when the packaged auto-candidate is actually
active we allow the validated-fallback behavior. Update the call to
getNvidiaCudaAudioExportSkipReason(audioMode, { allowValidatedFallbackCandidate:
... }) to pass packagedAutoCandidateActive (together with
isNvidiaCudaForceVideoOnlyEnabled() as before) instead of
packagedAutoCandidateEnabled so copy-source audio in a packaged build isn’t
wrongly marked eligible unless the auto-candidate is active.
🪄 Autofix (Beta)
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: 2d9ce027-3535-4e8a-85ff-e07f1ccbe565
⛔ Files ignored due to path filters (1)
electron/native/bin/win32-x64/recordly-nvidia-cuda-compositor.exeis excluded by!**/*.exe
📒 Files selected for processing (11)
electron/ipc/export/native-video.test.tselectron/ipc/export/native-video.tselectron/ipc/recording/diagnostics.tselectron/ipc/recording/prune.tselectron/native/bin/win32-x64/helpers-manifest.jsonelectron/native/nvidia-cuda-compositor/src/main.cusrc/lib/exporter/audioEncoder.test.tssrc/lib/exporter/backendPolicy.test.tssrc/lib/exporter/backendPolicy.tssrc/lib/exporter/modernVideoExporter.fallback.test.tssrc/lib/exporter/modernVideoExporter.ts
|
@coderabbitai review Follow-up fixes pushed in latest head
Validation:
Please re-check these two review points on |
|
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
electron/ipc/export/native-video.ts (1)
3644-3661:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRequire an audio stream before treating CUDA output as fully muxed.
When
shouldMuxAudioInlineis true, this path later skips the shared mux step and returns the CUDA file after only validating the video stream. The current checks never fail if the helper drops the audio stream entirely, so a silent MP4 can ship here without fallback.Suggested check
const cudaValidationIssues = validateNvidiaCudaExportSummary( cudaResult.summary, { durationSec: options.durationSec, targetFrames: Math.ceil(options.durationSec * options.frameRate), requiresTimelineSync: shouldMuxAudioInline, }, ); + if (shouldMuxAudioInline) { + const renderedMetadata = await probeNativeVideoMetadata( + ffmpegPath, + videoOnlyPath, + ); + if (!renderedMetadata.hasAudio) { + cudaValidationIssues.push("missing output audio stream"); + } + } if (cudaValidationIssues.length > 0) {Also applies to: 3718-3718, 4036-4042
🤖 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 3644 - 3661, When shouldMuxAudioInline is true we must ensure the CUDA export actually includes an audio stream before treating it as fully muxed: after calling runExperimentalNvidiaCudaStaticLayoutExport and before skipping the shared mux step, check cudaResult.summary (or call validateNvidiaCudaExportSummary with an added audio-required flag) to assert an audio stream is present and non-empty; if audio is missing, fail validation (throw or return an error) so the fallback/mux path runs. Update validateNvidiaCudaExportSummary (or add an explicit check immediately after runExperimentalNvidiaCudaStaticLayoutExport) to inspect cudaResult.summary.audioTracks or similar field and require at least one valid audio track when shouldMuxAudioInline / requiresTimelineSync is true.
🤖 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.
Outside diff comments:
In `@electron/ipc/export/native-video.ts`:
- Around line 3644-3661: When shouldMuxAudioInline is true we must ensure the
CUDA export actually includes an audio stream before treating it as fully muxed:
after calling runExperimentalNvidiaCudaStaticLayoutExport and before skipping
the shared mux step, check cudaResult.summary (or call
validateNvidiaCudaExportSummary with an added audio-required flag) to assert an
audio stream is present and non-empty; if audio is missing, fail validation
(throw or return an error) so the fallback/mux path runs. Update
validateNvidiaCudaExportSummary (or add an explicit check immediately after
runExperimentalNvidiaCudaStaticLayoutExport) to inspect
cudaResult.summary.audioTracks or similar field and require at least one valid
audio track when shouldMuxAudioInline / requiresTimelineSync is true.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 86cf00ec-3e4c-4526-b51d-32e54cbea040
📒 Files selected for processing (6)
electron/ipc/export/native-video.test.tselectron/ipc/export/native-video.tspackage.jsonscripts/dev-nvidia-cuda.mjssrc/lib/exporter/modernVideoExporter.nativeStaticLayout.test.tssrc/lib/exporter/modernVideoExporter.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/exporter/modernVideoExporter.ts
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 participant
Description
Routes Windows Lightning
autoexports through an explicit native planner so visually compatible projects try the native static layout path before Breeze/WebCodecs. The planner now makes CUDA/NVENC, Windows D3D11, and FFmpeg fallback decisions explicit, logs why each route was selected or rejected, and keeps CUDA behind the existing output validation gate.This update also refreshes the bundled NVIDIA CUDA compositor helper with higher-quality NV12 resampling for scaled content/webcam overlays. That addresses the visible blocky/noisy output we saw in CUDA exports without forcing CUDA for projects that still need WebGL/WebCodecs parity.
Motivation
After PR #410 landed, Windows
autoexports could still route into Breeze/WebCodecs before the native CUDA/D3D11 static compositor had a chance to run. That made NVIDIA-capable machines look like they had lost the native path. Separately, the CUDA compositor was fast but its nearest-neighbor style scaling made some screen recordings look visibly degraded. This PR makes routing deterministic and observable, then improves the native compositor quality where it is allowed to run.Changes Made
autoexports.nvidia-cuda-compositorwindows-d3d11-compositorffmpeg-static-layoutScope / Safety Notes
Beta Test Branch
beta/windows-nvidia-cuda-export-labpoints at the same commit for external tester pulls.Suggested tester run:
For dev-lab CUDA testing:
Diagnostics are written under:
%APPDATA%\Recordly\native-export-diagnostics%APPDATA%\Recordly-dev\native-export-diagnosticsValidation
npm test(70files,611tests)npx tsc --noEmit --pretty falsenpm run smoke:electron-main-cjsnpm run build:nvidia-cuda-compositornpx vite build --config vite.config.tsnpm run smoke:electron-main-cjscore.autocrlf=trueconverting files to CRLF while repo config expects LF.nvidia-cuda-compositor, output90frames /3.00s, native helper measured about274 FPS.nvidia-cuda-compositor; video/audio duration diff was about0.006s.Checklist
origin/main7bc06c9(v1.3.0-beta.2).Summary by CodeRabbit