feat(wgc-capture): fall back to software H.264 encoder#73
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:
📝 WalkthroughWalkthroughAdds software-encoder preference plumbing from stored user settings into native Windows capture, where the encoder can retry with a software H.264 path and report the selected encoder. The launch UI now shows a dismissible fallback notice, and smoke tests/docs were updated. ChangesSoftware encoder preference and fallback flow
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
|
Marking this ready for review so maintainers and affected users can review/test the candidate implementation. The validation caveat from the PR body still applies: I validated the default WGC helper path and the forced software encoder path locally, but I do not have access to a real no-GPU / broken-driver / missing hardware-H.264-MFT environment. So this PR still uses The most useful next validation is testing the normal recording path in an affected environment and checking whether it falls back to the software H.264 path successfully. |
![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
🧹 Nitpick comments (1)
electron/native/wgc-capture/src/mf_encoder.cpp (1)
165-192: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueClarify the fallback comment The sink writer doesn’t use hardware transforms unless
MF_READWRITE_ENABLE_HARDWARE_TRANSFORMSis set toTRUE; this branch mainly matters becauseMFTRegisterLocalByCLSIDmakes the software encoder available in-process. A short code/README note would avoid implying the hardware flag is the primary fix.🤖 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/wgc-capture/src/mf_encoder.cpp` around lines 165 - 192, The comment around createSinkWriterFromUrl should clarify that the forceSoftwareEncoder path is primarily about registering the software H.264 encoder in-process via ensureSoftwareH264EncoderRegisteredForProcess, not about enabling hardware transforms. Update the nearby code comment or README note to explain that MF_READWRITE_ENABLE_HARDWARE_TRANSFORMS is only set to FALSE to avoid hardware transforms, while the key fallback behavior comes from MFTRegisterLocalByCLSID-backed registration.
🤖 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/wgc-capture/src/mf_encoder.cpp`:
- Around line 165-217: createSinkWriterFromUrl currently returns failures from
multiple stages, but configureSinkWriterAttempt always routes them into
logSinkWriterCreateFailure as if MFCreateSinkWriterFromURL failed. Update
createSinkWriterFromUrl and the call path in configureSinkWriterAttempt to
preserve which step failed (for example via a stage/status indicator or separate
return handling), and only invoke logSinkWriterCreateFailure for the actual
MFCreateSinkWriterFromURL call site. Keep the existing specific logs for
ensureSoftwareH264EncoderRegisteredForProcess, MFCreateAttributes, and
SetUINT32, and avoid running the H.264/AAC enumeration diagnostics unless the
sink writer creation itself failed.
---
Nitpick comments:
In `@electron/native/wgc-capture/src/mf_encoder.cpp`:
- Around line 165-192: The comment around createSinkWriterFromUrl should clarify
that the forceSoftwareEncoder path is primarily about registering the software
H.264 encoder in-process via ensureSoftwareH264EncoderRegisteredForProcess, not
about enabling hardware transforms. Update the nearby code comment or README
note to explain that MF_READWRITE_ENABLE_HARDWARE_TRANSFORMS is only set to
FALSE to avoid hardware transforms, while the key fallback behavior comes from
MFTRegisterLocalByCLSID-backed registration.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a7bcb17c-cab7-42da-a151-23f01192639b
📒 Files selected for processing (10)
electron/ipc/handlers.tselectron/native/README.mdelectron/native/wgc-capture/src/main.cppelectron/native/wgc-capture/src/mf_encoder.cppelectron/native/wgc-capture/src/mf_encoder.hscripts/test-windows-wgc-helper.mjssrc/hooks/useScreenRecorder.tssrc/lib/nativeWindowsRecording.tssrc/lib/userPreferences.test.tssrc/lib/userPreferences.ts
createSinkWriterFromUrl can fail before MFCreateSinkWriterFromURL is ever reached: local software H.264 MFT registration, MFCreateAttributes, or SetUINT32(MF_READWRITE_ENABLE_HARDWARE_TRANSFORMS), each of which already logs its own specific error. configureSinkWriterAttempt still routed every failure through logSinkWriterCreateFailure, which prints "MFCreateSinkWriterFromURL failed" and runs the H.264/AAC MFT-enumeration diagnostics, misattributing those early-stage failures on exactly the VM / headless / broken-driver systems the software fallback targets. Return the failing stage from createSinkWriterFromUrl via a SinkWriterCreateStage out-param and only run the sink-writer diagnostics when the MFCreateSinkWriterFromURL call itself failed; the earlier stages keep their own specific logs. Also clarify (code comment + README) that the software fallback works by registering the Microsoft software H.264 encoder in-process via MFTRegisterLocalByCLSID, and that clearing MF_READWRITE_ENABLE_HARDWARE_TRANSFORMS is only a secondary guard, not the primary fallback mechanism. Refs getopenscreen#18
There was a problem hiding this comment.
🧹 Nitpick comments (1)
electron/ipc/handlers.ts (1)
1631-1634: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the redundant nested
preferSoftwareEncodercopy
The native parser reads the top-levelpreferSoftwareEncoderkey, so theencoder.preferSoftwareEncoderfield isn’t used here and can drift from the value that actually matters. Keep a single source of truth unless the nested shape is meant for another consumer.🤖 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/handlers.ts` around lines 1631 - 1634, The recording config in the IPC handler is carrying a redundant nested preferSoftwareEncoder copy that the native parser does not use. Update the config built in the recording flow so only the top-level preferSoftwareEncoder value is kept as the source of truth, and remove the unused encoder.preferSoftwareEncoder field unless another consumer explicitly requires that nested shape.
🤖 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.
Nitpick comments:
In `@electron/ipc/handlers.ts`:
- Around line 1631-1634: The recording config in the IPC handler is carrying a
redundant nested preferSoftwareEncoder copy that the native parser does not use.
Update the config built in the recording flow so only the top-level
preferSoftwareEncoder value is kept as the source of truth, and remove the
unused encoder.preferSoftwareEncoder field unless another consumer explicitly
requires that nested shape.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f6407023-310a-4741-a130-dcc273853f82
📒 Files selected for processing (2)
electron/ipc/handlers.tssrc/hooks/useScreenRecorder.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/hooks/useScreenRecorder.ts
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- Drop dead-weight encoder.preferSoftwareEncoder from helper config; C++ parseConfig already reads it from the top level. - Move kVideoEncoderSelection* constexprs to mf_encoder.h so the inline default for videoEncoderSelection_ shares one source of truth. - Register CLSID_MSH264EncoderMFT with SYNCMFT|ASYNCMFT; the MFT supports both modes.
Parse the Windows helper encoder-selection event in the main process and
return the selected encoder from start-native-windows-recording. When the
helper reports "software-fallback" (the default GPU encoder failed and the
helper switched on its own), the recording HUD shows a small dismissible
notice with a "Don't show again" option persisted in user preferences.
An explicitly preferred software encoder ("software-preferred") and the
normal default path show no notice, and the encoder-selection event stays
available for diagnostics.
Refs getopenscreen#18
|
Thanks for the review @EtienneLescot — addressed in the latest push.
Validated locally: native helper build, default and |
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 (2)
src/components/launch/LaunchWindow.tsx (1)
672-743: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winStack the software-fallback notice below the system-locale prompt
src/components/launch/LaunchWindow.tsx:672-743Both banners render in the same
fixed top-8 left-1/2 z-30 ...slot, and the two states are independent, so they can appear together and cover each other. Offset the second notice or make them mutually exclusive; otherwise one becomes unreadable/unclickable.🤖 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 672 - 743, The software encoder fallback notice and the system locale prompt currently share the same fixed position and z-index in LaunchWindow, so they can overlap and block each other. Update the rendering around systemLocaleSuggestion and softwareEncoderFallbackNoticeVisible so the second banner is stacked below the first (for example by adjusting its top offset based on the prompt state) or make the two notices mutually exclusive. Use the existing banner blocks and dismiss handlers in LaunchWindow.tsx to keep the layout and click targets readable.electron/ipc/handlers.ts (1)
1643-1649: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTrim the env var before comparing.
envPreferSoftwareEncoderis lowercased but not trimmed, soOPENSCREEN_WGC_PREFER_SOFTWARE_ENCODER=true(trailing whitespace, easy to introduce via shell scripts/CI env files) silently fails to enable the override.🐛 Proposed fix
const envPreferSoftwareEncoder = ( - process.env.OPENSCREEN_WGC_PREFER_SOFTWARE_ENCODER ?? "" -).toLowerCase(); + process.env.OPENSCREEN_WGC_PREFER_SOFTWARE_ENCODER ?? "" +) + .trim() + .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/handlers.ts` around lines 1643 - 1649, Trim the OPENSCREEN_WGC_PREFER_SOFTWARE_ENCODER value before checking it in the preferSoftwareEncoder logic, since the current envPreferSoftwareEncoder normalization only lowercases the string and can miss values with surrounding whitespace. Update the parsing in the IPC handler that computes preferSoftwareEncoder so the env var is normalized consistently before comparing against "true" or "1".
🤖 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/handlers.ts`:
- Around line 1643-1649: Trim the OPENSCREEN_WGC_PREFER_SOFTWARE_ENCODER value
before checking it in the preferSoftwareEncoder logic, since the current
envPreferSoftwareEncoder normalization only lowercases the string and can miss
values with surrounding whitespace. Update the parsing in the IPC handler that
computes preferSoftwareEncoder so the env var is normalized consistently before
comparing against "true" or "1".
In `@src/components/launch/LaunchWindow.tsx`:
- Around line 672-743: The software encoder fallback notice and the system
locale prompt currently share the same fixed position and z-index in
LaunchWindow, so they can overlap and block each other. Update the rendering
around systemLocaleSuggestion and softwareEncoderFallbackNoticeVisible so the
second banner is stacked below the first (for example by adjusting its top
offset based on the prompt state) or make the two notices mutually exclusive.
Use the existing banner blocks and dismiss handlers in LaunchWindow.tsx to keep
the layout and click targets readable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ebfaccad-2285-4461-92ba-12ca6c92a64f
📒 Files selected for processing (23)
electron/ipc/handlers.tselectron/native/README.mdelectron/native/wgc-capture/src/mf_encoder.cppelectron/native/wgc-capture/src/mf_encoder.hsrc/components/launch/LaunchWindow.test.tsxsrc/components/launch/LaunchWindow.tsxsrc/hooks/useScreenRecorder.tssrc/i18n/locales/ar/launch.jsonsrc/i18n/locales/en/launch.jsonsrc/i18n/locales/es/launch.jsonsrc/i18n/locales/fr/launch.jsonsrc/i18n/locales/it/launch.jsonsrc/i18n/locales/ja-JP/launch.jsonsrc/i18n/locales/ko-KR/launch.jsonsrc/i18n/locales/pt-BR/launch.jsonsrc/i18n/locales/ru/launch.jsonsrc/i18n/locales/tr/launch.jsonsrc/i18n/locales/vi/launch.jsonsrc/i18n/locales/zh-CN/launch.jsonsrc/i18n/locales/zh-TW/launch.jsonsrc/lib/nativeWindowsRecording.tssrc/lib/userPreferences.test.tssrc/lib/userPreferences.ts
✅ Files skipped from review due to trivial changes (9)
- src/i18n/locales/tr/launch.json
- src/i18n/locales/pt-BR/launch.json
- src/i18n/locales/zh-CN/launch.json
- src/i18n/locales/zh-TW/launch.json
- src/i18n/locales/ru/launch.json
- src/i18n/locales/ar/launch.json
- src/i18n/locales/ja-JP/launch.json
- src/i18n/locales/vi/launch.json
- src/i18n/locales/it/launch.json
🚧 Files skipped from review as they are similar to previous changes (4)
- electron/native/wgc-capture/src/mf_encoder.h
- src/lib/userPreferences.ts
- electron/native/README.md
- electron/native/wgc-capture/src/mf_encoder.cpp
|
Nice — cherry-pick looks clean, and the Two minor follow-ups from the new files (non-blocking):
Approving. |
- Stack the software-encoder fallback notice and the system-locale prompt in one fixed top-center column so simultaneous banners no longer overlap and cover each other. - Trim OPENSCREEN_WGC_PREFER_SOFTWARE_ENCODER before comparing so trailing whitespace from shell scripts/CI env files still enables the override.
|
Thanks — addressed in the latest push. The HUD notices now stack cleanly, and the env override is trimmed before comparison. The notice remains gated to automatic software-fallback only. |
|
Thanks @My-Denia, this looks solid — all the review feedback has been addressed, and the fallback now shows a HUD notice instead of failing silently. One thing we still can't test: none of us actually have a machine without hardware H.264 encoding (no GPU / broken driver), which is the whole point of this PR. That's why it's still If you have that kind of hardware, could you try a normal recording (without forcing the software encoder) and let us know if it works? Ideally confirm the recording succeeds and the software-fallback notice shows up in the HUD. Even a quick note with your setup and the result would let us close #18. Thanks! 🙌 |
|
Added deterministic end-to-end validation for the automatic fallback path in With Validation results:
This is deterministic end-to-end fault-injection coverage at the original |
Problem
Windows MP4 recording can fail before
recording-startedwhen Media Foundation cannot build a working H.264 encoder path from the default hardware/GPU encoder set. This affects VMs, headless/no-GPU systems, and broken GPU-driver registrations.Refs #18
Review/validation note: This PR is ready for maintainer review and affected-user testing. Local validation covers the default WGC helper path and the forced software encoder path, but I do not have access to a real no-GPU / broken-driver / missing hardware-H.264-MFT environment. Therefore this PR uses
Refs #18rather thanFixes #18.Cause
The WGC helper currently relies on the default
MFCreateSinkWriterFromURLencoder selection. If that setup path fails, recording exits instead of retrying with the Windows software H.264 encoder.Change
preferSoftwareEncoderrequest/helper config flag and persist it in user preferences.OPENSCREEN_WGC_PREFER_SOFTWARE_ENCODER=trueas a support/config override.encoder-selectionhelper event so smoke tests can prove which path was used.npm run test:wgc-helper:win -- --software-encodercoverage for the software path.Validation
git diff --checknpx tsc --noEmitnpm test -- src/lib/userPreferences.test.tsnpm run build-vitenpx biome check electron/ipc/handlers.ts scripts/test-windows-wgc-helper.mjs src/hooks/useScreenRecorder.ts src/lib/nativeWindowsRecording.ts src/lib/userPreferences.test.ts src/lib/userPreferences.tsnpm run build:native:winnpm run test:wgc-helper:winnpm run test:wgc-helper:win -- --software-encoderRisk
No real no-GPU or broken-driver Windows machine was available in this validation pass; automatic fallback after default setup failure remains residual risk.
npm run lintstill reports the existing repository-wide Biome formatter/line-ending baseline. The patch-local Biome check above passed.Summary by CodeRabbit
preferSoftwareEncoderpreference (andOPENSCREEN_WGC_PREFER_SOFTWARE_ENCODER) to steer Windows capture toward the Microsoft software H.264 encoder.--software-encoder).