dedicated handle-needs-input error #1057
Conversation
WalkthroughAdds a new Changes
Sequence Diagram(s)sequenceDiagram
actor Caller
participant Enc as H264Encoder
participant MF as Media Foundation
Caller->>Enc: handle_needs_input(input)
Enc->>MF: ProcessTexture
alt error
Enc-->>Caller: Err(ProcessTexture)
else success
Enc->>MF: CreateSurfaceBuffer
alt error
Enc-->>Caller: Err(CreateSurfaceBuffer)
else success
Enc->>MF: CreateSample
alt error
Enc-->>Caller: Err(CreateSample)
else success
Enc->>MF: AddBuffer
alt error
Enc-->>Caller: Err(AddBuffer)
else success
Enc->>MF: SetSampleTime
alt error
Enc-->>Caller: Err(SetSampleTime)
else success
Enc->>MF: ProcessInput
alt error
Enc-->>Caller: Err(ProcessInput)
else success
Enc-->>Caller: Ok(())
end
end
end
end
end
end
note over Enc,Caller: Each failure maps to a distinct HandleNeedsInputError variant.
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/enc-mediafoundation/src/video/h264.rs (1)
417-419: Bitrate calculation underflows for fps < 30 (u32 subtraction)
(fps - 30)underflows whenfps < 30, skewing bitrate massively. Also risks intermediate overflow on large resolutions. Use saturating math and wider intermediates.-fn calculate_bitrate(width: u32, height: u32, fps: u32, multiplier: f32) -> u32 { - ((width * height * ((fps - 30) / 2 + 30)) as f32 * multiplier) as u32 -} +fn calculate_bitrate(width: u32, height: u32, fps: u32, multiplier: f32) -> u32 { + let fps_term = fps.saturating_sub(30) / 2 + 30; + let pixels = (width as u64) * (height as u64); + let base = pixels.saturating_mul(fps_term as u64); + let scaled = (base as f64 * multiplier as f64).round(); + scaled.min(u32::MAX as f64) as u32 +}
🧹 Nitpick comments (2)
crates/enc-mediafoundation/src/video/h264.rs (2)
81-95: Public error enum: consider future-proofing with non_exhaustiveGood granular variants. To allow adding variants without a breaking change, mark the enum as non-exhaustive.
-#[derive(Clone, Debug, thiserror::Error)] +#[non_exhaustive] +#[derive(Clone, Debug, thiserror::Error)] pub enum HandleNeedsInputError {
389-391: Avoid negative sample times (edge-case robustness)If a frame arrives with a timestamp earlier than the first,
SetSampleTimemay receive a negative value. Clamp at 0.- mf_sample - .SetSampleTime(timestamp.Duration - first_time.Duration) - .map_err(HandleNeedsInputError::SetSampleTime)?; + let sample_time = (timestamp.Duration - first_time.Duration).max(0); + mf_sample + .SetSampleTime(sample_time) + .map_err(HandleNeedsInputError::SetSampleTime)?;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/enc-mediafoundation/src/video/h264.rs(3 hunks)
🧰 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/scap-direct3d/src/lib.rs (1)
texture(454-456)
⏰ 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). (4)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (rust)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
🔇 Additional comments (2)
crates/enc-mediafoundation/src/video/h264.rs (2)
369-372: Per-step error mapping looks goodThe mapping of
process_textureerrors toHandleNeedsInputError::ProcessTextureis clean and consistent.
368-368: Signature change verified — call sites already handle HandleNeedsInputErrorCall sites updated: crates/recording/src/capture_pipeline.rs (lines ~333, ~566) use
.map_err(|e| format!("NeedsInput: {e}"))?; crates/enc-mediafoundation/examples/cli.rs (line ~157) checks.is_err()and breaks.
Summary by CodeRabbit