fall back to software H264 encoder when hardware ProcessInput fails #1062
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds a flag-aware MFT enumeration API, new H.264 encoder constructors supporting scaled output and explicit software-only paths, and updates the recording capture pipeline to use these constructors with runtime software-fallback and pending-frame handling on encoder ProcessInput failures. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Caller
participant H264 as H264Encoder
participant MFT as EncoderDevice
participant MF as Media Foundation
App->>H264: new_with_scaled_output(format, in_res, out_res, fps, bitrate_mult)
H264->>MFT: enumerate_with_flags(major_type, subtype, HARDWARE|TRANSCODE_ONLY|SORTANDFILTER)
MFT->>MF: MFTEnumEx(flags, input/output types)
MF-->>MFT: MFT list
MFT-->>H264: EncoderDevice[]
H264->>MF: Configure transform (enable_hardware_transforms = true)
MF-->>H264: Ready
H264-->>App: Encoder instance
Note over H264,MFT: Software path uses TRANSCODE_ONLY and disables hardware transforms
sequenceDiagram
autonumber
participant Pipe as CapturePipeline
participant Enc as H264Encoder (HW)
participant SW as H264Encoder (SW)
participant Src as FrameSource
loop capture
Src->>Pipe: frame + timestamp
alt using HW encoder
Pipe->>Enc: ProcessInput(frame)
alt success
Enc-->>Pipe: ok
else E_FAIL
Pipe->>Pipe: store pending_frame
Pipe->>SW: new_with_scaled_output_software(...) or new_software(...)
Pipe->>SW: ProcessInput(pending_frame)
SW-->>Pipe: ok
end
else using SW encoder
Pipe->>SW: ProcessInput(frame)
SW-->>Pipe: ok
end
end
Note over Pipe: Frame timing propagated across pending/soft paths
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@coderabbitai review |
✅ Actions performedReview 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 (5)
crates/recording/src/capture_pipeline.rs (4)
249-256: Ensure fps consistency between encoder and muxer.You derive frame_rate (Line 249) but the muxer uses screen_config.fps() (Line 277). If these diverge, timestamps/bitrate signaling can drift.
Consider aligning the muxer fps to frame_rate:
- fps: screen_config.fps(), + fps: frame_rate,Also applies to: 272-281
320-396: Solid E_FAIL fallback with pending-frame replay; consider tidying the old transform before swap.The fallback path is correct and preserves the stalled frame. Minor robustness ideas:
- Attempt a best-effort finish()/flush on the failed transform before replacing it to release resources early (ignore errors).
- Log the selected encoder’s friendly name after swap to confirm software selection at runtime.
625-696: Instant-mode fallback mirrors studio-mode well; same notes apply.Good reuse of the E_FAIL fallback logic and pending-frame handling. As above, consider a best‑effort flush/finish on the failed MFT and log the chosen software encoder identity.
713-717: Avoid unwrap on shared output lock.A poisoned mutex will panic the task. Propagate a descriptive error instead.
- let mut output = output.lock().unwrap(); + let mut output = output + .lock() + .map_err(|e| format!("OutputLock: {e}"))?;crates/enc-mediafoundation/src/video/h264.rs (1)
300-318: Nit: parameter name typo (“bitrate_multipler”).Consider renaming to bitrate_multiplier for clarity and consistency, and deprecate the old public signatures if any.
Also applies to: 320-338, 357-372
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/enc-mediafoundation/src/mft.rs(1 hunks)crates/enc-mediafoundation/src/video/h264.rs(6 hunks)crates/recording/src/capture_pipeline.rs(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (3)
crates/enc-mediafoundation/src/mft.rs (1)
24-29: Good defaulting via delegate.Deferring enumerate(...) to enumerate_with_flags(...) with sensible defaults keeps the public API stable while enabling the new flag-driven path.
crates/recording/src/capture_pipeline.rs (1)
499-513: Confirm intentional fps override to 30.You hard-code frame_rate = 30 for instant mode regardless of capture config. If intentional (e.g., to cap CPU), OK; otherwise consider using source.0.config().fps() for parity with studio mode.
crates/enc-mediafoundation/src/video/h264.rs (1)
118-126: First() selection may still pick hardware in “software” mode.Because enumerate_with_flags() can still return hardware-first lists without explicit filtering, first().cloned() isn’t guaranteed to be software. See my comment in mft.rs for addressing enumeration bias.
| pub fn enumerate_with_flags( | ||
| major_type: GUID, | ||
| subtype: GUID, | ||
| flags: MFT_ENUM_FLAG, | ||
| ) -> Result<Vec<EncoderDevice>> { | ||
| let output_info = MFT_REGISTER_TYPE_INFO { | ||
| guidMajorType: major_type, | ||
| guidSubtype: subtype, | ||
| }; | ||
| let encoders = enumerate_mfts( | ||
| &MFT_CATEGORY_VIDEO_ENCODER, | ||
| MFT_ENUM_FLAG_HARDWARE | MFT_ENUM_FLAG_TRANSCODE_ONLY | MFT_ENUM_FLAG_SORTANDFILTER, | ||
| None, | ||
| Some(&output_info), | ||
| )?; | ||
| let flags = if flags.is_empty() { | ||
| MFT_ENUM_FLAG_SORTANDFILTER | ||
| } else { | ||
| flags | MFT_ENUM_FLAG_SORTANDFILTER | ||
| }; | ||
| let encoders = | ||
| enumerate_mfts(&MFT_CATEGORY_VIDEO_ENCODER, flags, None, Some(&output_info))?; | ||
| let mut encoder_devices = Vec::new(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Analysis chain
Software-only intent can be defeated by unconditional SORTANDFILTER; don’t bias toward hardware when caller asks for software.
MFT_ENUM_FLAG_SORTANDFILTER tends to surface hardware first. If the caller omits MFT_ENUM_FLAG_HARDWARE (e.g., your software-only path), unconditionally OR-ing SORTANDFILTER here can still prefer hardware, and your subsequent first() pick may select a hardware MFT, undermining the fallback.
- Do not force SORTANDFILTER when a caller explicitly supplies flags.
- Optionally, when HARDWARE is not set, prefer software results (e.g., by reordering/filtering) to make software selection deterministic.
Apply this to avoid hardware bias when callers pass explicit flags:
- let flags = if flags.is_empty() {
- MFT_ENUM_FLAG_SORTANDFILTER
- } else {
- flags | MFT_ENUM_FLAG_SORTANDFILTER
- };
+ let flags = if flags.is_empty() {
+ MFT_ENUM_FLAG_SORTANDFILTER
+ } else {
+ flags
+ };Please verify that new_with_scaled_output_software() actually picks the Microsoft software H.264 MFT on systems where hardware encoders are present (print/display_name is sufficient). If it still resolves to a hardware MFT, we should add a software-first filter in this function.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching repo for relevant symbols..."
rg -n "MFT_ENUM_FLAG_SORTANDFILTER|MFT_ENUM_FLAG_HARDWARE|new_with_scaled_output_software|enumerate_with_flags|enumerate_mfts|MFT_CATEGORY_VIDEO_ENCODER" -S || true
echo
echo "Displaying crates/enc-mediafoundation/src/mft.rs (if present):"
sed -n '1,240p' crates/enc-mediafoundation/src/mft.rs || true
echo
echo "Searching for function new_with_scaled_output_software definition/usages..."
rg -n "new_with_scaled_output_software" -S || true
echo
echo "Searching for calls to enumerate_with_flags..."
rg -n "enumerate_with_flags" -S || true
echo
echo "Listing crate tree for enc-mediafoundation:"
if [ -d crates/enc-mediafoundation ]; then
find crates/enc-mediafoundation -maxdepth 2 -type f -print
fiLength of output: 6856
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Showing relevant portions of crates/enc-mediafoundation/src/video/h264.rs (around usages and function):"
sed -n '1,260p' crates/enc-mediafoundation/src/video/h264.rs || true
echo
echo "Showing lines 260-640 (to capture full function if longer):"
sed -n '260,640p' crates/enc-mediafoundation/src/video/h264.rs || true
echo
echo "Showing caller in crates/recording/src/capture_pipeline.rs (around line 650-700):"
sed -n '640,720p' crates/recording/src/capture_pipeline.rs || trueLength of output: 21294
Remove forced SORTANDFILTER for explicit flags
• Change enumerate_with_flags to not OR SORTANDFILTER when flags is non-empty:
- let flags = if flags.is_empty() {
- MFT_ENUM_FLAG_SORTANDFILTER
- } else {
- flags | MFT_ENUM_FLAG_SORTANDFILTER
- };
+ let flags = if flags.is_empty() {
+ MFT_ENUM_FLAG_SORTANDFILTER
+ } else {
+ flags
+ };• Optional: for software-only paths (no HARDWARE), filter or reorder results to pick the Microsoft software MFT first.
• Verify new_with_scaled_output_software() selects the software encoder on systems with hardware MFTs.
📝 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.
| pub fn enumerate_with_flags( | |
| major_type: GUID, | |
| subtype: GUID, | |
| flags: MFT_ENUM_FLAG, | |
| ) -> Result<Vec<EncoderDevice>> { | |
| let output_info = MFT_REGISTER_TYPE_INFO { | |
| guidMajorType: major_type, | |
| guidSubtype: subtype, | |
| }; | |
| let encoders = enumerate_mfts( | |
| &MFT_CATEGORY_VIDEO_ENCODER, | |
| MFT_ENUM_FLAG_HARDWARE | MFT_ENUM_FLAG_TRANSCODE_ONLY | MFT_ENUM_FLAG_SORTANDFILTER, | |
| None, | |
| Some(&output_info), | |
| )?; | |
| let flags = if flags.is_empty() { | |
| MFT_ENUM_FLAG_SORTANDFILTER | |
| } else { | |
| flags | MFT_ENUM_FLAG_SORTANDFILTER | |
| }; | |
| let encoders = | |
| enumerate_mfts(&MFT_CATEGORY_VIDEO_ENCODER, flags, None, Some(&output_info))?; | |
| let mut encoder_devices = Vec::new(); | |
| pub fn enumerate_with_flags( | |
| major_type: GUID, | |
| subtype: GUID, | |
| flags: MFT_ENUM_FLAG, | |
| ) -> Result<Vec<EncoderDevice>> { | |
| let output_info = MFT_REGISTER_TYPE_INFO { | |
| guidMajorType: major_type, | |
| guidSubtype: subtype, | |
| }; | |
| let flags = if flags.is_empty() { | |
| MFT_ENUM_FLAG_SORTANDFILTER | |
| } else { | |
| flags | |
| }; | |
| let encoders = | |
| enumerate_mfts(&MFT_CATEGORY_VIDEO_ENCODER, flags, None, Some(&output_info))?; | |
| let mut encoder_devices = Vec::new(); |
🤖 Prompt for AI Agents
In crates/enc-mediafoundation/src/mft.rs around lines 31 to 47, the function
currently forces MFT_ENUM_FLAG_SORTANDFILTER into flags even when the caller
supplied explicit flags; remove that unconditional OR so that when flags is
non-empty you use the caller's flags unchanged (i.e. if flags.is_empty() set
flags = MFT_ENUM_FLAG_SORTANDFILTER else keep flags as provided). Additionally
(optional) if you need software-only behavior, filter or reorder the returned
encoders to prefer the Microsoft software MFT (e.g. detect absence of HARDWARE
flag and move the Microsoft encoder to the front), and verify that
new_with_scaled_output_software() picks the software encoder when hardware MFTs
are present.
| .SetUINT32( | ||
| &MF_READWRITE_ENABLE_HARDWARE_TRANSFORMS, | ||
| enable_hardware_transforms as u32, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
Verify MF_READWRITE_ENABLE_HARDWARE_TRANSFORMS applies to MFT attributes.
This attribute is documented for SourceReader/SinkWriter. It may be ignored on IMFTransform attributes. If so, disabling hardware here won’t have effect; you must ensure software selection at enumeration time.
🌐 Web query:
Does MF_READWRITE_ENABLE_HARDWARE_TRANSFORMS apply to IMFTransform attributes, or only to Source Reader/Sink Writer (IMFSourceReader/IMFSinkWriter)?
💡 Result:
Short answer: It applies to the Source Reader and Sink Writer (IMFSourceReader/IMFSinkWriter) only — not as a general IMFTransform attribute. The Microsoft docs show MF_READWRITE_ENABLE_HARDWARE_TRANSFORMS is used when creating source readers/sink writers; hardware-MFT-specific attributes are exposed on the MFT's IMFAttributes (see the "Hardware MFTs" topic). [1][2]
Sources:
[1] MF_READWRITE_ENABLE_HARDWARE_TRANSFORMS (Microsoft Docs).
[2] Hardware MFTs — Hardware MFT attributes (Microsoft Docs).
MF_READWRITE_ENABLE_HARDWARE_TRANSFORMS is for IMFSourceReader/IMFSinkWriter only — setting it on MFT attributes is ignored
Microsoft docs show this flag is used when creating SourceReader/SinkWriter, not as a general IMFTransform attribute; the SetUINT32 here (crates/enc-mediafoundation/src/video/h264.rs:168–171) will likely be ignored — select software MFTs at enumeration/activation or set the flag when creating the SourceReader/SinkWriter instead.
🤖 Prompt for AI Agents
crates/enc-mediafoundation/src/video/h264.rs around lines 168–171: the code sets
MF_READWRITE_ENABLE_HARDWARE_TRANSFORMS on an MFT attribute bag, but that flag
is respected only by IMFSourceReader/IMFSinkWriter and is ignored on
IMFTransform attributes; remove the SetUINT32 call here and instead either pass
MF_READWRITE_ENABLE_HARDWARE_TRANSFORMS when creating the
SourceReader/SinkWriter or explicitly choose software MFTs during
enumeration/activation (filter/skip hardware MFTs) so the hardware/software
selection actually takes effect.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/enc-mediafoundation/src/mft.rs(1 hunks)crates/enc-mediafoundation/src/video/h264.rs(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/enc-mediafoundation/src/mft.rs
🧰 Additional context used
📓 Path-based instructions (4)
crates/**/src/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
For desktop IPC, use tauri_specta derive/macros on Rust types/events; do not hand-roll bindings
Files:
crates/enc-mediafoundation/src/video/h264.rs
**/*.{ts,tsx,js,jsx,rs}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not add inline, block, or docstring comments in any language; code must be self-explanatory
Files:
crates/enc-mediafoundation/src/video/h264.rs
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Format Rust code usingrustfmtand ensure all Rust code passes workspace-level clippy lints.
Rust modules should be named with snake_case, and crate directories should be in kebab-case.
Files:
crates/enc-mediafoundation/src/video/h264.rs
crates/*/src/**/*
📄 CodeRabbit inference engine (AGENTS.md)
Rust crates should place tests within the
src/and/or a siblingtests/directory for each crate insidecrates/*.
Files:
crates/enc-mediafoundation/src/video/h264.rs
🧬 Code graph analysis (1)
crates/enc-mediafoundation/src/video/h264.rs (1)
crates/enc-mediafoundation/src/mft.rs (1)
enumerate_with_flags(31-63)
🔇 Additional comments (3)
crates/enc-mediafoundation/src/video/h264.rs (3)
18-19: LGTM: Imports for MFT enumeration flags are appropriate.
357-372: LGTM: Software-only convenience constructor is useful and clear.
168-171: MF_READWRITE_ENABLE_HARDWARE_TRANSFORMS is ignored by MFTs—remove the SetUINT32 here.This flag applies to SourceReader/SinkWriter, not IMFTransform attributes; setting it here has no effect.
Apply this diff:
- attributes - .SetUINT32( - &MF_READWRITE_ENABLE_HARDWARE_TRANSFORMS, - enable_hardware_transforms as u32, - ) - .map_err(NewVideoEncoderError::EventGenerator)?;
| fn new_with_scaled_output_with_flags( | ||
| d3d_device: &ID3D11Device, | ||
| format: DXGI_FORMAT, | ||
| input_resolution: SizeInt32, | ||
| output_resolution: SizeInt32, | ||
| frame_rate: u32, | ||
| bitrate_multipler: f32, | ||
| flags: MFT_ENUM_FLAG, | ||
| enable_hardware_transforms: bool, | ||
| ) -> Result<Self, NewVideoEncoderError> { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Drop the now-ineffective enable_hardware_transforms parameter and adjust call sites.
Selection is controlled at enumeration time via flags; the parameter is unused after removing the SetUINT32.
Apply these diffs:
Signature:
- flags: MFT_ENUM_FLAG,
- enable_hardware_transforms: bool,
+ flags: MFT_ENUM_FLAG,Call site (hardware path):
- MFT_ENUM_FLAG_HARDWARE | MFT_ENUM_FLAG_TRANSCODE_ONLY,
- true,
+ MFT_ENUM_FLAG_HARDWARE | MFT_ENUM_FLAG_TRANSCODE_ONLY,Call site (software path):
- MFT_ENUM_FLAG_TRANSCODE_ONLY,
- false,
+ MFT_ENUM_FLAG_TRANSCODE_ONLY,Also applies to: 300-318, 320-338
| let transform = | ||
| EncoderDevice::enumerate_with_flags(MFMediaType_Video, MFVideoFormat_H264, flags) | ||
| .map_err(|_| NewVideoEncoderError::NoVideoEncoderDevice)? | ||
| .first() | ||
| .cloned() | ||
| .ok_or(NewVideoEncoderError::NoVideoEncoderDevice)? | ||
| .create_transform() | ||
| .map_err(NewVideoEncoderError::EncoderTransform)?; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t assume the first enumerated MFT will activate—try all candidates.
If the first device fails to activate, you currently error out instead of trying others. Iterate until one succeeds to improve reliability.
Apply this diff:
- let transform =
- EncoderDevice::enumerate_with_flags(MFMediaType_Video, MFVideoFormat_H264, flags)
- .map_err(|_| NewVideoEncoderError::NoVideoEncoderDevice)?
- .first()
- .cloned()
- .ok_or(NewVideoEncoderError::NoVideoEncoderDevice)?
- .create_transform()
- .map_err(NewVideoEncoderError::EncoderTransform)?;
+ let transform = {
+ let devices =
+ EncoderDevice::enumerate_with_flags(MFMediaType_Video, MFVideoFormat_H264, flags)
+ .map_err(|_| NewVideoEncoderError::NoVideoEncoderDevice)?;
+ devices
+ .into_iter()
+ .find_map(|d| d.create_transform().ok())
+ .ok_or(NewVideoEncoderError::NoVideoEncoderDevice)?
+ };📝 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.
| let transform = | |
| EncoderDevice::enumerate_with_flags(MFMediaType_Video, MFVideoFormat_H264, flags) | |
| .map_err(|_| NewVideoEncoderError::NoVideoEncoderDevice)? | |
| .first() | |
| .cloned() | |
| .ok_or(NewVideoEncoderError::NoVideoEncoderDevice)? | |
| .create_transform() | |
| .map_err(NewVideoEncoderError::EncoderTransform)?; | |
| let transform = { | |
| let devices = | |
| EncoderDevice::enumerate_with_flags(MFMediaType_Video, MFVideoFormat_H264, flags) | |
| .map_err(|_| NewVideoEncoderError::NoVideoEncoderDevice)?; | |
| devices | |
| .into_iter() | |
| .find_map(|d| d.create_transform().ok()) | |
| .ok_or(NewVideoEncoderError::NoVideoEncoderDevice)? | |
| }; |
🤖 Prompt for AI Agents
In crates/enc-mediafoundation/src/video/h264.rs around lines 118 to 126, the
code currently takes the first enumerated encoder device and errors if it fails
to activate; instead iterate over all candidates returned by
EncoderDevice::enumerate_with_flags, attempt to activate/create_transform for
each one, and return the first successful transform; if none succeed, convert
the last (or aggregated) error into NewVideoEncoderError::EncoderTransform or
NoVideoEncoderDevice as appropriate. Concretely: call enumerate_with_flags(...)?
to get the list, if empty return NoVideoEncoderDevice; then for each candidate
call cloned.create_transform().map_err(NewVideoEncoderError::EncoderTransform)
and on the first Ok return it, otherwise continue; after the loop return
NoVideoEncoderDevice or the last EncoderTransform error.
Windows capture now watches for the ProcessInput E_FAIL from hardware H.264, immediately swaps to a software-only encoder, and replays the stalled frame. That automatic fallback relies on a tiny helper tweak that lets us enumerate software MFTs on demand, so recording stays alive even on fragile GPUs.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chore