feat: Implement resolution picker setting for Instant Mode #1258
Conversation
WalkthroughAdds an instant-mode max-resolution setting (default 1920), exposes it to UI and Tauri types, threads it into the Instant actor builder, computes a scaled even output_resolution, and propagates that resolution through capture pipelines, muxers, and the H264 encoder; also threads macOS shareable_content into actor creation. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant UI
participant Tauri as Tauri Backend
participant Builder as Instant ActorBuilder
participant Pipeline as Instant Pipeline
participant Capture as MakeCapturePipeline
participant Encoder as H264 Encoder
User->>UI: set "Instant mode max resolution"
UI->>Tauri: persist instantModeMaxResolution
User->>Tauri: start Instant recording
Tauri->>Tauri: read instant_mode_max_resolution (or default 1920)
Tauri->>Builder: with_max_output_size(value)
Builder->>Pipeline: create_pipeline(max_output_size=value)
Pipeline->>Pipeline: compute output_resolution (scale, preserve AR, even dims)
Pipeline->>Capture: make_instant_mode_pipeline(..., output_resolution)
Capture->>Encoder: H264EncoderBuilder::with_output_size(w,h)
Encoder->>Encoder: validate dims/chroma, setup converter/scaling if needed
Capture->>Pipeline: return OutputPipeline + video_info
Pipeline->>Builder: actor started with video_info
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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
🧹 Nitpick comments (3)
crates/recording/src/output_pipeline/win.rs (1)
54-59: Enforce even, positive dimensions before using output_size.H.264 commonly requires mod-2 dimensions; odd sizes can break encoders/muxers. Clamp to >=2 and round down to even before use.
- let output_size = config.output_size.unwrap_or(input_size); + let mut output_size = config.output_size.unwrap_or(input_size); + // enforce positive, even dimensions + if output_size.Width <= 1 || output_size.Height <= 1 { + return Err(anyhow!("Invalid output_size: {output_size:?}")); + } + output_size.Width &= !1; + output_size.Height &= !1;apps/desktop/src/routes/(window-chrome)/settings/general.tsx (2)
321-335: Tighten SelectSettingItem typing; avoid any for option values.Use the generic T for option values to preserve type-safety across unions (numbers and string literals).
- options: { text: string; value: any }[]; + options: { text: string; value: T }[];
206-214: Drop or gate debug logs.console.log in settings changes can be noisy in production. Gate behind a dev flag or remove.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/desktop/src-tauri/src/general_settings.rs(3 hunks)apps/desktop/src-tauri/src/recording.rs(2 hunks)apps/desktop/src/routes/(window-chrome)/settings/general.tsx(8 hunks)apps/desktop/src/utils/tauri.ts(2 hunks)crates/recording/src/capture_pipeline.rs(7 hunks)crates/recording/src/instant_recording.rs(10 hunks)crates/recording/src/lib.rs(1 hunks)crates/recording/src/output_pipeline/win.rs(4 hunks)crates/recording/src/studio_recording.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.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/recording/src/lib.rsapps/desktop/src-tauri/src/recording.rscrates/recording/src/instant_recording.rscrates/recording/src/studio_recording.rsapps/desktop/src-tauri/src/general_settings.rscrates/recording/src/output_pipeline/win.rscrates/recording/src/capture_pipeline.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/recording/src/lib.rscrates/recording/src/instant_recording.rscrates/recording/src/studio_recording.rscrates/recording/src/output_pipeline/win.rscrates/recording/src/capture_pipeline.rs
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use a 2-space indent for TypeScript code.
Use Biome for formatting and linting TypeScript/JavaScript files by runningpnpm format.Use strict TypeScript and avoid any; leverage shared types
Files:
apps/desktop/src/utils/tauri.tsapps/desktop/src/routes/(window-chrome)/settings/general.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use kebab-case for filenames for TypeScript/JavaScript modules (e.g.,user-menu.tsx).
Use PascalCase for React/Solid components.
Files:
apps/desktop/src/utils/tauri.tsapps/desktop/src/routes/(window-chrome)/settings/general.tsx
**/tauri.ts
📄 CodeRabbit inference engine (AGENTS.md)
Do not edit auto-generated files named
tauri.ts.NEVER EDIT auto-generated IPC bindings file: tauri.ts
Files:
apps/desktop/src/utils/tauri.ts
apps/desktop/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/desktop/src/**/*.{ts,tsx}: Desktop icons are auto-imported (unplugin-icons); do not import icons manually
Desktop IPC: Call generated tauri_specta commands/events; listen to generated events and use typed interfaces
Use @tanstack/solid-query for server state in the desktop app
Files:
apps/desktop/src/utils/tauri.tsapps/desktop/src/routes/(window-chrome)/settings/general.tsx
🧬 Code graph analysis (4)
apps/desktop/src-tauri/src/recording.rs (2)
apps/desktop/src/utils/tauri.ts (4)
GeneralSettingsStore(396-396)InstantModeResolution(413-413)PostDeletionBehaviour(434-434)PostStudioRecordingBehaviour(435-435)apps/desktop/src-tauri/src/general_settings.rs (1)
default(181-208)
crates/recording/src/instant_recording.rs (4)
crates/recording/src/capture_pipeline.rs (7)
output(61-61)output(65-65)output(68-69)make_instant_mode_pipeline(23-31)make_instant_mode_pipeline(50-73)make_instant_mode_pipeline(98-130)target_to_display_and_crop(139-229)crates/media-info/src/lib.rs (1)
from_raw_ffmpeg(211-219)crates/recording/src/sources/screen_capture/mod.rs (2)
display(67-73)init(285-342)crates/recording/src/output_pipeline/core.rs (3)
video_info(591-593)video_info(634-636)video_info(699-699)
apps/desktop/src-tauri/src/general_settings.rs (1)
apps/desktop/src/utils/tauri.ts (1)
InstantModeResolution(413-413)
apps/desktop/src/routes/(window-chrome)/settings/general.tsx (1)
apps/desktop/src/utils/tauri.ts (2)
GeneralSettingsStore(396-396)InstantModeResolution(413-413)
⏰ 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 (17)
apps/desktop/src/routes/(window-chrome)/settings/general.tsx (3)
65-80: Defaults wiring for instant resolution looks good.
94-104: Resolution options list is clear and type-safe.
441-458: UI binding for “Instant mode resolution” is correctly wired.Value, onChange, and options line up with InstantModeResolution.
crates/recording/src/studio_recording.rs (1)
401-407: Studio path initializes output_height to None — consistent with scope.apps/desktop/src-tauri/src/recording.rs (1)
49-51: Instant mode builder change verified — with_output_height is supported on all platforms.Confirmed: the
with_output_heightmethod at crates/recording/src/instant_recording.rs:300 has no platform-specific guards and is called unconditionally at lines 542-547. The fallback logic usingInstantModeResolution::default().target_height()is sound, and imports are correct. No issues found.crates/recording/src/lib.rs (1)
49-50: New output_height field successfully integrated across all initializations.Verification confirms both
RecordingBaseInputsstruct literal constructions have been updated:
instant_recording.rs:322initializes withself.output_heightstudio_recording.rs:406initializes withNoneNo other construction sites exist (other references are type annotations only). The struct compiles successfully with all fields initialized.
apps/desktop/src/utils/tauri.ts (1)
396-396: Auto-generated bindings confirmed; types are consistent and file has proper generation marker.Verification successful: apps/desktop/src/utils/tauri.ts is properly auto-generated by tauri-specta. The file header explicitly states "Do not edit this file manually." Both
instantModeResolution(line 396) andInstantModeResolution(line 413) are present and properly typed as a union of resolution strings. No manual edits detected.apps/desktop/src-tauri/src/general_settings.rs (2)
34-53: LGTM! Well-structured resolution enum with proper trait derivations.The
InstantModeResolutionenum is correctly defined with appropriate derives and thetarget_height()method provides clean mapping to pixel heights. The addition ofPartialEqandEqis useful for comparison operations.
144-145: Correct integration with settings store.The field is properly configured with
#[serde(default)]for backward compatibility with existing configurations, and the default initialization is consistent with the enum's default variant.Also applies to: 206-206
crates/recording/src/capture_pipeline.rs (3)
23-31: Trait signature correctly updated with scaled output parameter.The addition of
scaled_output: Option<(u32, u32)>to the trait method signature is appropriate and maintains the async trait pattern.
50-73: macOS implementation correctly extracts and propagates height.The implementation properly extracts the height component from
scaled_outputfor the AVFoundation muxer configuration. The optional chaining withmapensuresNoneis handled correctly.
98-130: Windows implementation correctly propagates scaled dimensions.The conversion to
SizeInt32with both width and height is appropriate for the Windows muxer. Theas i32casts are safe for all practical video resolutions (even theoretical 100K resolution would be well withini32::MAX).crates/recording/src/instant_recording.rs (5)
23-26: Good addition of video_info to Pipeline struct.Storing the final video info in the Pipeline struct allows proper propagation of the scaled dimensions throughout the recording actor lifecycle.
192-259: Scaling logic correctly maintains aspect ratio and ensures even dimensions.The implementation properly:
- Prevents upscaling when target height ≥ source height
- Maintains aspect ratio when calculating scaled width
- Ensures both dimensions are even (required for video encoding)
- Clamps dimensions to prevent exceeding source resolution
The check at line 216-217 is technically redundant since height is guaranteed to be ≥ 2 after line 212's
max(2)and the even adjustment, but it serves as defensive programming and doesn't harm clarity.
244-253: Correct construction of final video info with scaled dimensions.The implementation properly constructs the final
VideoInfousing scaled dimensions when available, while preserving the pixel format and frame rate from the base video info. The fallback tobase_video_infohandles the no-scaling case correctly.
267-331: Builder pattern correctly extended with output height support.The
ActorBuilderproperly integrates the newoutput_heightfield with:
- Appropriate initialization to
None- A fluent
with_output_height()method following existing patterns- Correct propagation to
RecordingBaseInputs
333-411: Proper integration of height-based scaling in actor spawning.The spawn function correctly:
- Propagates
inputs.output_heighttocreate_pipeline(line 378)- Extracts and uses the final scaled
video_infofrom the pipeline (lines 382, 391)- Ensures the
Actoris initialized with the correct output dimensions rather than the source dimensions
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
crates/enc-ffmpeg/src/video/h264.rs (3)
117-147: Clarify converter error mapping; don’t always blame pixel format.If scaler creation fails for reasons other than pixfmt (e.g., invalid/odd dims, memory), returning
PixFmtNotSupportedmisleads callers. After adding even-dimension enforcement, map remaining failures toFFmpeg(e)unless it’s genuinely a pixfmt issue.- Err(e) => { - if needs_pixel_conversion { - error!( - "Failed to create converter from {:?} to {:?}: {:?}", - input_config.pixel_format, output_format, e - ); - return Err(H264EncoderError::PixFmtNotSupported( - input_config.pixel_format, - )); - } - - return Err(H264EncoderError::FFmpeg(e)); - } + Err(e) => { + error!( + "Failed to create converter from {:?} to {:?}: {:?}", + input_config.pixel_format, output_format, e + ); + return Err(H264EncoderError::FFmpeg(e)); + }
118-123: Tie scaling quality to preset (fast path for Instant Mode).Use
FAST_BILINEARforUltrafastwhen scaling; keepBICUBICfor higher presets. This better matches latency goals.- let flags = if needs_scaling { - ffmpeg::software::scaling::flag::Flags::BICUBIC - } else { - ffmpeg::software::scaling::flag::Flags::FAST_BILINEAR - }; + let flags = match (needs_scaling, self.preset) { + (true, H264Preset::Ultrafast) => ffmpeg::software::scaling::flag::Flags::FAST_BILINEAR, + (true, _) => ffmpeg::software::scaling::flag::Flags::BICUBIC, + (false, _) => ffmpeg::software::scaling::flag::Flags::FAST_BILINEAR, + };
229-238: Preserve key frame metadata on conversion (aspect/color).PTS is preserved—good. Also consider copying SAR/aspect and color properties (range, primaries, matrix, trc) from
frametoconvertedto prevent washed colors or stretching after RGB→NV12 + scaling. API names vary; apply where available in your ffmpeg crate.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/enc-ffmpeg/src/video/h264.rs(6 hunks)crates/recording/src/output_pipeline/win.rs(5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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/recording/src/output_pipeline/win.rscrates/enc-ffmpeg/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/recording/src/output_pipeline/win.rscrates/enc-ffmpeg/src/video/h264.rs
🧬 Code graph analysis (2)
crates/recording/src/output_pipeline/win.rs (1)
crates/enc-ffmpeg/src/video/h264.rs (1)
builder(216-218)
crates/enc-ffmpeg/src/video/h264.rs (3)
crates/recording/src/sources/screen_capture/macos.rs (1)
pixel_format(32-34)crates/recording/src/sources/screen_capture/windows.rs (1)
pixel_format(48-50)crates/rendering/src/decoder/frame_converter.rs (1)
new(16-21)
⏰ 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 (7)
crates/recording/src/output_pipeline/win.rs (5)
35-35: LGTM: Optional output_size field enables resolution configuration.The optional field appropriately allows callers to specify a target output resolution while defaulting to input dimensions when None.
54-58: LGTM: Clean dimension initialization with sensible fallback.The code correctly derives
output_sizefrom the config or falls back toinput_size, ensuring valid dimensions are always available downstream.
82-83: LGTM: Native encoder correctly receives both input and output dimensions.Passing both
input_sizeandoutput_sizeenables the MediaFoundation encoder to apply internal scaling as intended.
92-93: LGTM: Muxer configuration correctly uses output dimensions.Using
output_sizefor the muxer configuration ensures the container metadata accurately reflects the encoded video dimensions.
106-118: I've confirmed thatH264EncoderBuilder::with_output_size()exists and is properly implemented. However, I need to verify the claim about line 58's guarantees before endorsing the suggested simplification.The web search didn't provide specifics about zero-value validation. Let me examine the actual code context around line 58:Now I understand the casting behavior better. Let me search for what line 58 actually does to validate the claim:Let me check the actual code context directly by examining line 58:Do not apply the suggested simplification; the defensive checks are necessary and appropriate.
The
with_output_size()method is confirmed to exist, but the suggested simplification introduces a critical safety issue. The builder validates its inputs and treats zero values as a signal to clear output dimensions, which suggests zero values are a real concern.The defensive checks in lines 106-115 serve an important purpose:
- They protect against zero or negative
output_sizevalues by providing fallback dimensions fromvideo_config- Casting between integer types doesn't perform checks, just reinterprets bits, so directly casting
output_size.Widthto u32 silently handles negative values unsafely- If
output_sizecontains zero values, the simplified code would pass zero towith_output_size(), which setsoutput_sizetoNonein the builder, losing the fallbackKeep the current implementation as-is. It correctly:
- Resolves the past critical issue by passing dimensions to the builder
- Defensively handles potentially invalid dimensions with fallbacks
- Ensures FFmpeg receives valid dimension values
crates/enc-ffmpeg/src/video/h264.rs (2)
185-193: LGTM: encoder I/O shape and converter wiring.Clean separation of output_format/size in
H264Encoder, and optional converter with PTS carry-over looks solid.Also applies to: 200-203
160-165: Time base conversion is correctly implemented; no issues found.The code properly handles the mismatch between encoder and stream time bases through a two-stage conversion:
Frame PTS stage (EncoderBase::update_pts): Converts input Duration to frame PTS using the encoder's time base (1/1,000,000). Calculation:
pts = (timestamp.as_secs_f64() * 1_000_000).round(), normalized by subtracting the first PTS.Packet rescaling stage (EncoderBase::process_packets): After encoding, packets are rescaled from encoder time base (1/1,000,000) to stream time base (1/90,000) via
packet.rescale_ts(). This is FFmpeg's built-in rescaling and handles the conversion without custom logic errors.The architecture follows FFmpeg best practices—no drift or rescale bugs are present.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/recording/src/output_pipeline/win.rs (1)
54-58: Consider bounds checking when casting dimensions to i32.While highly unlikely in practice, casting
video_config.width/height(likely u32) to i32 could theoretically overflow for values exceedingi32::MAX. Consider adding a debug assertion or saturating cast if defensive programming is desired.let input_size = SizeInt32 { - Width: video_config.width as i32, - Height: video_config.height as i32, + Width: video_config.width.min(i32::MAX as u32) as i32, + Height: video_config.height.min(i32::MAX as u32) as i32, };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/enc-ffmpeg/src/video/h264.rs(7 hunks)crates/recording/src/output_pipeline/win.rs(5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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/recording/src/output_pipeline/win.rscrates/enc-ffmpeg/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/recording/src/output_pipeline/win.rscrates/enc-ffmpeg/src/video/h264.rs
🧬 Code graph analysis (1)
crates/recording/src/output_pipeline/win.rs (1)
crates/enc-ffmpeg/src/video/h264.rs (1)
builder(232-234)
⏰ 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). (1)
- GitHub Check: Analyze (rust)
🔇 Additional comments (18)
crates/recording/src/output_pipeline/win.rs (5)
35-35: LGTM!The
output_sizefield is properly added to the configuration struct with appropriate type and visibility.
82-83: LGTM!The native encoder now correctly receives both input and output dimensions for scaled output.
92-93: LGTM!The muxer configuration now correctly uses output dimensions instead of input dimensions.
106-115: LGTM!The fallback dimension logic correctly handles cases where
output_sizedimensions might be zero or negative by falling back to input dimensions. This properly addresses the previous review concern about FFmpeg fallback ignoring scaling.
117-119: LGTM!The FFmpeg fallback encoder now correctly applies output scaling via
with_output_size, resolving the previous issue where user-selected resolution was ignored in the fallback path.crates/enc-ffmpeg/src/video/h264.rs (13)
19-19: LGTM!The
output_sizefield is properly defined and initialized in the builder pattern.
37-38: LGTM!The error variant is well-defined with a clear message describing the constraint.
63-70: LGTM!The validation correctly enforces non-zero even dimensions, preventing downstream encoding failures with NV12 format. This addresses the previous review feedback.
80-87: LGTM!The dimension extraction logic is correct. The zero-check (lines 84-87) is defensive since
with_output_sizealready validates non-zero dimensions, but this provides an additional safety guard.
89-95: LGTM!The safe format checking correctly handles missing format information by falling back to pixel conversion. This addresses the previous review concern about unwrap() panics.
97-109: LGTM!The pixel format selection logic correctly determines when conversion is needed and falls back to NV12.
111-121: LGTM!The NV12 dimension validation correctly enforces even dimensions required for 4:2:0 chroma subsampling. This addresses the previous review feedback and catches cases where input dimensions might not meet the constraint.
123-131: LGTM!The scaling detection logic is correct and includes helpful debug logging.
133-166: LGTM!The converter creation logic correctly handles both pixel format conversion and scaling with appropriate algorithm selection (BICUBIC for scaling). Error handling properly distinguishes between conversion failures and other errors.
176-178: LGTM!The encoder setup correctly uses output dimensions and format.
184-185: LGTM!Bitrate calculation correctly uses output dimensions rather than input dimensions.
215-218: LGTM!The struct fields are refactored to store only the necessary output configuration and converter, improving clarity.
245-254: LGTM!The frame conversion logic correctly applies scaling/pixel format conversion while preserving the presentation timestamp.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
crates/enc-ffmpeg/src/video/h264.rs (2)
133-139: Consider exposing scaler quality via preset or default to BILINEAR for Instant Mode.BICUBIC is higher quality but heavier. For “Instant Mode” latency, FAST_BILINEAR or BILINEAR may be preferable, or make it preset-driven.
245-254: Preserve frame properties and avoid per-frame alloc if possible.Carry over color props (range/space/primaries) from input to converted to prevent subtle shifts; optionally keep a reusable converted frame buffer to avoid alloc each frame.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/enc-ffmpeg/src/video/h264.rs(7 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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-ffmpeg/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-ffmpeg/src/video/h264.rs
🪛 GitHub Check: Clippy
crates/enc-ffmpeg/src/video/h264.rs
[warning] 64-64: manual implementation of .is_multiple_of()
warning: manual implementation of .is_multiple_of()
--> crates/enc-ffmpeg/src/video/h264.rs:64:41
|
64 | if width == 0 || height == 0 || width % 2 != 0 || height % 2 != 0 {
| ^^^^^^^^^^^^^^ help: replace with: !width.is_multiple_of(2)
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#manual_is_multiple_of
= note: #[warn(clippy::manual_is_multiple_of)] on by default
⏰ 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 (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (2)
crates/enc-ffmpeg/src/video/h264.rs (2)
89-96: Nice: removed panicky unwraps when probing encoder formats.Graceful fallback if formats() isn’t advertised avoids runtime panics.
63-70: The suggested fix is incorrect and would break compilation.The review comment recommends using
.is_multiple_of(2), but this method is behind the unstable feature gateunsigned_is_multiple_ofand is not available on stable Rust (as of Rust 1.90.0, October 2025). The codebase contains no such feature declarations—usingis_multiple_of()would cause a compilation error.The current code using
width % 2 != 0is the correct, stable-compatible approach and should remain unchanged. The clippy lintmanual_is_multiple_ofonly fires on nightly Rust and is not configured in the workspace lints anyway.The one detected call site at
crates/recording/src/output_pipeline/win.rs:118properly handles the Result return type via.and_then(), confirming the API change is correctly integrated.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
crates/enc-ffmpeg/src/video/h264.rs (3)
70-77: Apply Clippy'sis_multiple_ofsuggestion for cleaner even-check.The modulo-based even check can be replaced with the more readable
is_multiple_ofmethod.Apply this diff:
- if width == 0 || height == 0 || width % 2 != 0 || height % 2 != 0 { + if width == 0 || height == 0 || !width.is_multiple_of(2) || !height.is_multiple_of(2) { return Err(H264EncoderError::InvalidOutputDimensions { width, height }); }Note: For consistency, consider applying the same pattern at line 126 where even dimensions are also validated.
87-94: Simplify redundant dimension fallback logic.The check at lines 91-94 appears redundant: if
output_sizeis set viawith_output_size, it's already validated as non-zero; if unset, the fallback isinput_configdimensions. Overwriting zero dimensions with the sameinput_configdimensions accomplishes nothing, and the subsequent check at lines 96-101 catches any zero dimensions anyway.Consider simplifying to:
let (mut output_width, mut output_height) = self .output_size .unwrap_or((input_config.width, input_config.height)); - if output_width == 0 || output_height == 0 { - output_width = input_config.width; - output_height = input_config.height; - } - if output_width == 0 || output_height == 0 { return Err(H264EncoderError::InvalidOutputDimensions { width: output_width, height: output_height, }); }This keeps the error-check for invalid input configurations without the no-op reassignment.
125-132: Consider usingis_multiple_ofhere for consistency.While the modulo-based check is correct, using
is_multiple_ofwould be consistent with the suggested fix at line 71.Apply this diff for consistency:
if is_420(output_format) - && (output_width % 2 != 0 || output_height % 2 != 0) + && (!output_width.is_multiple_of(2) || !output_height.is_multiple_of(2)) { return Err(H264EncoderError::InvalidOutputDimensions { width: output_width, height: output_height, }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/enc-ffmpeg/src/video/h264.rs(7 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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-ffmpeg/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-ffmpeg/src/video/h264.rs
🧬 Code graph analysis (1)
crates/enc-ffmpeg/src/video/h264.rs (3)
crates/recording/src/sources/screen_capture/mod.rs (1)
pixel_format(230-230)crates/recording/src/sources/screen_capture/windows.rs (1)
pixel_format(48-50)crates/recording/src/sources/screen_capture/macos.rs (1)
pixel_format(32-34)
🪛 GitHub Check: Clippy
crates/enc-ffmpeg/src/video/h264.rs
[warning] 71-71: manual implementation of .is_multiple_of()
warning: manual implementation of .is_multiple_of()
--> crates/enc-ffmpeg/src/video/h264.rs:71:41
|
71 | if width == 0 || height == 0 || width % 2 != 0 || height % 2 != 0 {
| ^^^^^^^^^^^^^^ help: replace with: !width.is_multiple_of(2)
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#manual_is_multiple_of
= note: #[warn(clippy::manual_is_multiple_of)] on by default
⏰ 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 (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (4)
crates/enc-ffmpeg/src/video/h264.rs (4)
15-20: LGTM! Clean 4:2:0 format detection.The helper correctly identifies 4:2:0 chroma subsampling by checking the descriptor's chroma dimensions, with safe handling of missing descriptors.
103-109: LGTM! Safe encoder format capability check.The implementation correctly handles cases where format information is unavailable by falling back to NV12 conversion. The use of
.ok()to convert the Result to Option is appropriate here.
144-177: LGTM! Proper converter creation with appropriate scaling flags.The implementation correctly:
- Uses BICUBIC for scaling operations (better quality)
- Uses FAST_BILINEAR for pixel format conversion only (faster)
- Returns appropriate errors for conversion failures
256-265: LGTM! Frame conversion correctly preserves PTS.The implementation properly:
- Captures the original PTS before conversion
- Creates a correctly-sized output frame
- Runs the converter
- Preserves the PTS on the converted frame
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/enc-ffmpeg/src/video/h264.rs (2)
70-77: Consider using bitwise checks to avoid potential clippy warnings.Based on learnings, modulo-based checks can trigger clippy warnings. Consider using bitwise operations for even/odd checks.
Apply this diff:
- pub fn with_output_size(mut self, width: u32, height: u32) -> Result<Self, H264EncoderError> { - if width == 0 || height == 0 || width % 2 != 0 || height % 2 != 0 { - return Err(H264EncoderError::InvalidOutputDimensions { width, height }); - } + pub fn with_output_size(mut self, width: u32, height: u32) -> Result<Self, H264EncoderError> { + if width == 0 || height == 0 || (width & 1) != 0 || (height & 1) != 0 { + return Err(H264EncoderError::InvalidOutputDimensions { width, height }); + }Note: This method requires even dimensions for all formats upfront, which simplifies the API since most H264 encoders use 4:2:0 subsampling. Based on learnings
125-132: Use bitwise checks to avoid clippy warnings.Similar to the early validation, consider using bitwise operations instead of modulo.
Apply this diff:
if is_420(output_format) - && (output_width % 2 != 0 || output_height % 2 != 0) + && ((output_width & 1) != 0 || (output_height & 1) != 0) { return Err(H264EncoderError::InvalidOutputDimensions { width: output_width, height: output_height, }); }Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/enc-ffmpeg/src/video/h264.rs(8 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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-ffmpeg/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-ffmpeg/src/video/h264.rs
🧬 Code graph analysis (1)
crates/enc-ffmpeg/src/video/h264.rs (3)
crates/recording/src/sources/screen_capture/mod.rs (1)
pixel_format(230-230)crates/recording/src/sources/screen_capture/windows.rs (1)
pixel_format(48-50)crates/recording/src/sources/screen_capture/macos.rs (1)
pixel_format(32-34)
⏰ 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: Clippy
- 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 (6)
crates/enc-ffmpeg/src/video/h264.rs (6)
15-20: LGTM: Helper correctly identifies 4:2:0 formats.The
is_420helper properly detects chroma subsampling by checking descriptor log2 values, with safe fallback for unknown formats.
103-109: LGTM: Proper null-safe format checking.The format support check correctly addresses the past review feedback by safely handling cases where
video()orformats()return None/Err, falling back tofalseinstead of panicking.
134-177: LGTM: Well-structured converter creation with appropriate scaling flags.The logic correctly detects when conversion/scaling is needed, chooses appropriate quality flags (BICUBIC for scaling, FAST_BILINEAR for format conversion only), and provides clear error handling.
187-199: LGTM: Encoder properly configured with output dimensions.The encoder is correctly initialized with the target output dimensions and format, and the bitrate calculation appropriately uses the output resolution rather than input.
Also applies to: 212-219, 227-229
256-265: LGTM: Proper frame conversion with PTS preservation.The conversion flow correctly allocates a frame with output dimensions, runs the converter, and preserves the presentation timestamp.
334-341: LGTM: Bitrate calculation properly uses f64 to prevent overflow.The calculation correctly performs all arithmetic in f64 to avoid u32 overflow when computing width × height for large resolutions.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/enc-ffmpeg/src/video/h264.rs(8 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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-ffmpeg/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-ffmpeg/src/video/h264.rs
🧬 Code graph analysis (1)
crates/enc-ffmpeg/src/video/h264.rs (3)
crates/recording/src/sources/screen_capture/mod.rs (1)
pixel_format(230-230)crates/recording/src/sources/screen_capture/macos.rs (1)
pixel_format(32-34)crates/recording/src/sources/screen_capture/windows.rs (1)
pixel_format(48-50)
🪛 GitHub Check: Clippy
crates/enc-ffmpeg/src/video/h264.rs
[warning] 87-87: variable does not need to be mutable
warning: variable does not need to be mutable
--> crates/enc-ffmpeg/src/video/h264.rs:87:32
|
87 | let (mut output_width, mut output_height) = self
| ----^^^^^^^^^^^^^
| |
| help: remove this mut
[warning] 87-87: variable does not need to be mutable
warning: variable does not need to be mutable
--> crates/enc-ffmpeg/src/video/h264.rs:87:14
|
87 | let (mut output_width, mut output_height) = self
| ----^^^^^^^^^^^^
| |
| help: remove this mut
|
= note: #[warn(unused_mut)] on by default
⏰ 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: Analyze (rust)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
🔇 Additional comments (9)
crates/enc-ffmpeg/src/video/h264.rs (9)
15-20: Well-implemented 4:2:0 format detection helper.The helper correctly identifies 4:2:0 chroma-subsampled formats by checking the logarithmic chroma dimensions and safely handles missing descriptors.
26-26: LGTM: Optional output size field properly integrated.The
Option<(u32, u32)>type correctly models the optional output resolution configuration for the encoder.
70-77: LGTM: Early validation with appropriate constraints.The method validates dimensions up-front in the builder. Requiring even dimensions unconditionally (rather than only for 4:2:0 formats) is a conservative design choice that prevents edge cases with various codecs and scalers.
98-104: Excellent: Safe handling of codec format queries.The code properly uses
ok(),and_then, andmap_orto handle cases where the codec doesn't expose video info or supported formats, avoiding potential panics.
120-127: LGTM: Format-specific dimension validation.The validation correctly uses the
is_420helper to enforce even dimensions specifically for 4:2:0 chroma-subsampled formats, addressing previous review feedback about covering all 4:2:0 variants.
139-172: Well-structured converter setup with appropriate error handling.The logic correctly:
- Determines when conversion/scaling is needed
- Selects appropriate scaling flags (BICUBIC for quality when scaling, FAST_BILINEAR for speed when only converting pixel format)
- Distinguishes between pixel format conversion failures and other FFmpeg errors
182-194: LGTM: Encoder correctly configured with output dimensions.The encoder setup properly uses
output_width,output_height, andoutput_formatto ensure the encoder matches the actual output stream after scaling/conversion. Bitrate calculation correctly uses output dimensions rather than input dimensions.
251-260: Correct frame conversion with PTS preservation.The conversion logic properly:
- Allocates the output frame with the correct dimensions and format
- Preserves the PTS from the source frame to maintain proper timing
- Replaces the frame reference after conversion
329-336: Excellent: Overflow-safe bitrate calculation.The function now uses
f64arithmetic throughout, preventing the u32 overflow issue that could occur when multiplying large dimensions. This correctly addresses previous review feedback.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/enc-ffmpeg/src/video/h264.rs (1)
87-89: Remove unusedmutmodifiers to fix Clippy warnings.The variables
output_widthandoutput_heightare never mutated after initialization.Apply this diff:
- let (mut output_width, mut output_height) = self + let (output_width, output_height) = self .output_size .unwrap_or((input_config.width, input_config.height));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/enc-ffmpeg/src/video/h264.rs(8 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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-ffmpeg/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-ffmpeg/src/video/h264.rs
🧬 Code graph analysis (1)
crates/enc-ffmpeg/src/video/h264.rs (2)
crates/recording/src/sources/screen_capture/macos.rs (1)
pixel_format(32-34)crates/recording/src/sources/screen_capture/windows.rs (1)
pixel_format(48-50)
⏰ 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: Clippy
- 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 (6)
crates/enc-ffmpeg/src/video/h264.rs (6)
15-20: LGTM! Helper correctly identifies 4:2:0 chroma subsampling.The function safely checks the pixel format descriptor for 4:2:0 characteristics (both chroma dimensions subsampled by factor of 2). The fallback to
falsewhen descriptor is unavailable is appropriate.
98-118: LGTM! Safe format support detection and fallback logic.The encoder format support check correctly handles missing video capabilities or format lists by defaulting to
false, and the NV12 fallback with debug logging is appropriate.
127-170: LGTM! Converter setup correctly handles both scaling and pixel format conversion.The logic appropriately creates a converter when either dimension or format changes, selects quality-appropriate scaling flags (BICUBIC for resize, FAST_BILINEAR for format-only), and provides clear error messages distinguishing pixel format issues from general scaling failures.
180-212: LGTM! Encoder correctly configured with output dimensions.The encoder setup properly uses
output_width,output_height, andoutput_formatfor configuration, and bitrate calculation appropriately scales based on the output resolution rather than input. The struct initialization matches the new field set.
249-258: LGTM! Frame conversion correctly preserves timing and handles errors.The conversion logic properly allocates output frames with the correct format/dimensions, runs the converter, and crucially preserves the PTS to maintain frame timing through the pipeline.
327-334: LGTM! Bitrate calculation uses f64 to prevent overflow.The function correctly performs all arithmetic in
f64to avoid u32 overflow when computingwidth * height, addressing the critical issue from previous reviews.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/desktop/src-tauri/src/recording.rs (1)
541-547: Consider adding validation for max_output_size.The configuration correctly retrieves the instant mode max resolution setting with a sensible default of 1920. However, there's no validation to ensure the value is within reasonable bounds. Consider adding a check to ensure the value is at least a minimum threshold (e.g., 480) and not excessively large to prevent potential issues in the video encoding pipeline.
Example:
.with_max_output_size( general_settings .as_ref() .map(|settings| settings.instant_mode_max_resolution) - .unwrap_or_else(|| 1920), + .unwrap_or(1920) + .clamp(480, 7680), )apps/desktop/src/routes/(window-chrome)/settings/general.tsx (1)
64-64: Consider removing the unnecessary type alias.
ExtendedGeneralSettingsStoreis defined as a direct alias toGeneralSettingsStorewithout any extensions. Since it doesn't add additional fields or modify the type, you can useGeneralSettingsStoredirectly throughout the file to reduce indirection.-type ExtendedGeneralSettingsStore = GeneralSettingsStore; - -const createDefaultGeneralSettings = (): ExtendedGeneralSettingsStore => ({ +const createDefaultGeneralSettings = (): GeneralSettingsStore => ({crates/recording/src/instant_recording.rs (1)
207-237: Solid aspect ratio preservation logic with minor suggestions.The resolution scaling logic correctly:
- Preserves aspect ratio using floating-point calculations
- Ensures even dimensions (required for video codecs)
- Handles both landscape and portrait orientations
- Clamps to source dimensions
Consider these improvements:
- Add validation to ensure
max_output_sizeis reasonable (e.g., minimum 480, maximum 7680)- Add a doc comment explaining that
max_output_sizeapplies to the larger dimensionasync fn create_pipeline( output_path: PathBuf, screen_source: ScreenCaptureConfig<ScreenCaptureMethod>, mic_feed: Option<Arc<MicrophoneFeedLock>>, max_output_size: Option<u32>, ) -> anyhow::Result<Pipeline> { + // Validate max_output_size if provided + if let Some(size) = max_output_size { + anyhow::ensure!(size >= 480, "max_output_size must be at least 480"); + anyhow::ensure!(size <= 7680, "max_output_size must not exceed 7680"); + } + // ... rest of function
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/desktop/src-tauri/src/general_settings.rs(3 hunks)apps/desktop/src-tauri/src/recording.rs(2 hunks)apps/desktop/src/routes/(window-chrome)/settings/general.tsx(8 hunks)apps/desktop/src/utils/tauri.ts(1 hunks)crates/recording/src/capture_pipeline.rs(6 hunks)crates/recording/src/instant_recording.rs(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/desktop/src-tauri/src/general_settings.rs
- crates/recording/src/capture_pipeline.rs
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use a 2-space indent for TypeScript code.
Use Biome for formatting and linting TypeScript/JavaScript files by runningpnpm format.Use strict TypeScript and avoid any; leverage shared types
Files:
apps/desktop/src/utils/tauri.tsapps/desktop/src/routes/(window-chrome)/settings/general.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use kebab-case for filenames for TypeScript/JavaScript modules (e.g.,user-menu.tsx).
Use PascalCase for React/Solid components.
Files:
apps/desktop/src/utils/tauri.tsapps/desktop/src/routes/(window-chrome)/settings/general.tsx
**/tauri.ts
📄 CodeRabbit inference engine (AGENTS.md)
Do not edit auto-generated files named
tauri.ts.NEVER EDIT auto-generated IPC bindings file: tauri.ts
Files:
apps/desktop/src/utils/tauri.ts
apps/desktop/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/desktop/src/**/*.{ts,tsx}: Desktop icons are auto-imported (unplugin-icons); do not import icons manually
Desktop IPC: Call generated tauri_specta commands/events; listen to generated events and use typed interfaces
Use @tanstack/solid-query for server state in the desktop app
Files:
apps/desktop/src/utils/tauri.tsapps/desktop/src/routes/(window-chrome)/settings/general.tsx
**/*.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:
apps/desktop/src-tauri/src/recording.rscrates/recording/src/instant_recording.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/recording/src/instant_recording.rs
🧬 Code graph analysis (2)
apps/desktop/src/routes/(window-chrome)/settings/general.tsx (1)
apps/desktop/src/utils/tauri.ts (1)
GeneralSettingsStore(396-396)
crates/recording/src/instant_recording.rs (4)
crates/recording/src/capture_pipeline.rs (7)
output(58-58)output(62-62)output(65-66)make_instant_mode_pipeline(20-28)make_instant_mode_pipeline(47-70)make_instant_mode_pipeline(95-127)target_to_display_and_crop(136-226)crates/media-info/src/lib.rs (1)
from_raw_ffmpeg(211-219)crates/recording/src/sources/screen_capture/mod.rs (2)
display(67-73)init(285-342)crates/recording/src/output_pipeline/core.rs (3)
video_info(591-593)video_info(634-636)video_info(699-699)
⏰ 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 (7)
apps/desktop/src-tauri/src/recording.rs (1)
470-474: LGTM!The shareable content retrieval for macOS is well-structured with proper error handling and formatting.
apps/desktop/src/utils/tauri.ts (1)
396-396: Auto-generated file - no manual edits required.This file is auto-generated by tauri-specta (as noted in line 2). The addition of
instantModeMaxResolution?: numberto theGeneralSettingsStoretype is automatically derived from the Rust backend types and correctly reflects the new setting.As per coding guidelines
apps/desktop/src/routes/(window-chrome)/settings/general.tsx (2)
76-76: LGTM!The default value of 1920 (1080p width) is a sensible choice for instant mode recordings, balancing quality and file size.
91-100: LGTM!The resolution options and UI control are well-implemented. The values correctly represent standard resolutions (720p through 4K), and the control properly updates the settings store. The backend logic handles both landscape and portrait orientations appropriately by using these values as the maximum dimension for the longer side.
Also applies to: 434-445
crates/recording/src/instant_recording.rs (3)
23-26: LGTM!Adding
video_infoto thePipelinestruct is a clean design that stores the final output dimensions after scaling, making them easily accessible without recomputation.
267-303: LGTM!The
ActorBuildercorrectly extends to support the newmax_output_sizeparameter, following the builder pattern conventions with proper initialization and a fluent API method.
333-412: LGTM!The
max_output_sizeparameter is correctly threaded through the recording actor spawn process, and thevideo_infois properly extracted from the pipeline and passed to theActor. The rename fromcroptocrop_boundsalso improves code clarity.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
crates/recording/src/instant_recording.rs (1)
216-240: Consider extracting even dimension logic into a helper function.The pattern of ensuring even dimensions (required for video encoding) is repeated four times. Consider extracting to a helper:
fn ensure_even(mut value: u32) -> u32 { if value % 2 != 0 { value -= 1; } value }Then use:
let width = ensure_even(max_size.min(screen_info.width));This would improve readability and reduce duplication.
apps/desktop/src-tauri/src/general_settings.rs (1)
123-124: Consider adding validation for resolution bounds.The
u32type allows any value from 0 to ~4.3 billion, but reasonable video resolutions typically range from 640 (VGA) to 7680 (8K). While the UI constrains choices to predefined options, direct API calls could set invalid values.Consider adding a validation method or using a restricted type:
impl GeneralSettingsStore { pub fn validate_instant_mode_max_resolution(&self) -> bool { self.instant_mode_max_resolution >= 640 && self.instant_mode_max_resolution <= 7680 } }apps/desktop/src/routes/(window-chrome)/settings/general.tsx (1)
64-64: Type alias naming is misleading.The type
ExtendedGeneralSettingsStoreis just an alias toGeneralSettingsStoreand doesn't actually extend it. This naming could confuse maintainers who expect additional fields.Consider either:
- Removing the alias and using
GeneralSettingsStoredirectly- Renaming to something like
SettingsFormStateif it serves a different semantic purpose-type ExtendedGeneralSettingsStore = GeneralSettingsStore; +// Use GeneralSettingsStore directly throughout the file
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/desktop/src-tauri/src/general_settings.rs(3 hunks)apps/desktop/src/routes/(window-chrome)/settings/general.tsx(2 hunks)crates/recording/src/instant_recording.rs(11 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.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:
apps/desktop/src-tauri/src/general_settings.rscrates/recording/src/instant_recording.rs
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use a 2-space indent for TypeScript code.
Use Biome for formatting and linting TypeScript/JavaScript files by runningpnpm format.Use strict TypeScript and avoid any; leverage shared types
Files:
apps/desktop/src/routes/(window-chrome)/settings/general.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use kebab-case for filenames for TypeScript/JavaScript modules (e.g.,user-menu.tsx).
Use PascalCase for React/Solid components.
Files:
apps/desktop/src/routes/(window-chrome)/settings/general.tsx
apps/desktop/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/desktop/src/**/*.{ts,tsx}: Desktop icons are auto-imported (unplugin-icons); do not import icons manually
Desktop IPC: Call generated tauri_specta commands/events; listen to generated events and use typed interfaces
Use @tanstack/solid-query for server state in the desktop app
Files:
apps/desktop/src/routes/(window-chrome)/settings/general.tsx
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/recording/src/instant_recording.rs
🧬 Code graph analysis (2)
apps/desktop/src/routes/(window-chrome)/settings/general.tsx (2)
apps/desktop/src/utils/tauri.ts (9)
WindowExclusion(486-486)CaptureWindow(371-371)GeneralSettingsStore(396-396)AppTheme(341-341)commands(7-284)events(289-333)MainWindowRecordingStartBehaviour(418-418)PostStudioRecordingBehaviour(434-434)PostDeletionBehaviour(433-433)apps/desktop/src/store.ts (2)
generalSettingsStore(61-62)authStore(59-59)
crates/recording/src/instant_recording.rs (4)
crates/recording/src/capture_pipeline.rs (7)
output(58-58)output(62-62)output(65-66)make_instant_mode_pipeline(20-28)make_instant_mode_pipeline(47-70)make_instant_mode_pipeline(95-127)target_to_display_and_crop(136-226)crates/media-info/src/lib.rs (1)
from_raw_ffmpeg(211-219)crates/recording/src/sources/screen_capture/mod.rs (2)
display(67-73)init(285-342)crates/recording/src/output_pipeline/core.rs (3)
video_info(591-593)video_info(634-636)video_info(699-699)
⏰ 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: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Typecheck
- GitHub Check: Analyze (rust)
🔇 Additional comments (11)
crates/recording/src/instant_recording.rs (4)
23-26: LGTM: Pipeline now carries computed video info.This addition properly stores the final video information including the scaled output resolution, which is then used by the Actor.
256-264: LGTM: VideoInfo correctly reflects scaled output.The video_info is properly constructed using the computed output_resolution while preserving the pixel format and frame rate from the original screen capture.
273-309: LGTM: Builder pattern correctly extended.The
max_output_sizefield andwith_max_output_sizemethod follow the existing builder pattern style consistently.
339-418: LGTM: Parameter threading and video_info usage are correct.The changes properly:
- Thread
max_output_sizefrom the builder through tocreate_pipeline- Use the computed
video_infofrom the pipeline (which reflects scaled dimensions) instead of recomputing from screen_source- Rename
croptocrop_boundsfor clarityapps/desktop/src-tauri/src/general_settings.rs (2)
144-146: LGTM!The default value of 1920 (1080p width) is a sensible default for instant mode recordings, balancing quality and file size.
189-189: LGTM!Initialization is consistent with the serde default and follows the established pattern.
apps/desktop/src/routes/(window-chrome)/settings/general.tsx (5)
66-77: LGTM!Default settings are correctly defined with
instantModeMaxResolution: 1920matching the backend default.
91-99: LGTM!Resolution options are well-defined with appropriate values (horizontal resolution widths) and clear labels. The default of 1920 corresponds to the "1080p" option.
433-444: LGTM!The instant mode resolution setting is properly integrated with correct default value (1920), clear labeling, and appropriate placement in the Recording settings group.
202-210: LGTM!The
handleChangefunction correctly updates both local state and persistent storage, ensuring the instant mode resolution setting is properly saved.
356-356: Add missing icon import on line 356.
IconCapChevronDownis used but not imported. The icon is valid and used elsewhere in the codebase (e.g., inTargetDropdownButton.tsx), but the import statement is missing from this file.Add this import near the other icon imports (after line 38):
import IconLucidePlus from "~icons/lucide/plus"; import IconLucideX from "~icons/lucide/x"; +import IconCapChevronDown from "~icons/cap/chevron-down";Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/recording/src/instant_recording.rs (1)
471-535: Helper function defined but unused.This
clamp_sizehelper encapsulates the resolution scaling logic with proper tests, but it's currently unused—create_pipelinecontains a duplicate inline implementation. Refer to the earlier comment at lines 207-287 for the refactoring suggestion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/recording/src/capture_pipeline.rs(6 hunks)crates/recording/src/instant_recording.rs(12 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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/recording/src/capture_pipeline.rscrates/recording/src/instant_recording.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/recording/src/capture_pipeline.rscrates/recording/src/instant_recording.rs
🧬 Code graph analysis (2)
crates/recording/src/capture_pipeline.rs (2)
crates/recording/src/sources/screen_capture/mod.rs (1)
d3d_device(345-347)crates/scap-direct3d/src/lib.rs (3)
d3d_device(207-207)d3d_device(356-358)d3d_device(427-429)
crates/recording/src/instant_recording.rs (4)
crates/recording/src/capture_pipeline.rs (7)
output(58-58)output(62-62)output(65-66)make_instant_mode_pipeline(20-28)make_instant_mode_pipeline(47-70)make_instant_mode_pipeline(95-127)target_to_display_and_crop(136-226)crates/media-info/src/lib.rs (1)
from_raw_ffmpeg(211-219)crates/recording/src/sources/screen_capture/mod.rs (2)
display(67-73)init(285-342)crates/recording/src/output_pipeline/core.rs (3)
video_info(591-593)video_info(634-636)video_info(699-699)
⏰ 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 (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (9)
crates/recording/src/capture_pipeline.rs (4)
20-28: LGTM!The trait signature update correctly adds the
output_resolutionparameter for instant mode pipeline creation. The positioning and typing are appropriate.
95-127: LGTM with minor note on type conversion.The Windows instant mode implementation correctly uses both width and height from
output_resolution. Theu32toi32cast on lines 122-123 is safe for reasonable screen resolutions (up to ~2^31 pixels, far exceeding any practical display).
47-70: Aspect ratio handling is correct—no changes needed.The macOS implementation intentionally passes only the height to the muxer config. The encoder then automatically preserves aspect ratio by calculating the output width based on the original video dimensions. The encoder computes a downscale ratio from the provided height, then derives the output width as
original_width × downscale, ensuring aspect ratio preservation. This is a deliberate design choice, not an oversight.
75-92: The review comment is based on incorrect assumptions about the git history and the nature of the changes.The bitrate change is neither unrelated nor a mistake. Commit
f0415a3a("clamp resolution better") intentionally changed both studio and instant mode to usebitrate_multiplier: 0.15f32together. The studio mode bitrate was previously0.1f32(not0.08), and both values were unified to0.15f32as part of the resolution picker feature improvements.The
output_size: Nonein studio mode is the correct design—studio mode has no output resolution parameter, while instant mode receivesoutput_resolutionand passes it viaSome(...). This difference is intentional and consistent with the two recording modes' distinct behavior.Likely an incorrect or invalid review comment.
crates/recording/src/instant_recording.rs (5)
23-26: LGTM!Adding
video_infoto thePipelinestruct is a clean way to propagate video metadata through the pipeline lifecycle.
289-309: LGTM!The pipeline construction correctly passes
output_resolutionto the capture pipeline and derivesvideo_infofrom the scaled dimensions, ensuring metadata consistency.
317-381: LGTM!The builder pattern implementation for
max_output_sizeis clean and follows the existing pattern. The default ofNone(no scaling) is sensible.
383-462: LGTM!The actor spawning correctly threads
max_output_sizethrough the pipeline creation and uses the pipeline'svideo_inforather than deriving it from the screen source, ensuring the actor has accurate output dimensions.
537-641: Excellent test coverage!The test suite comprehensively validates the
clamp_sizefunction across various aspect ratios, scaling scenarios, and edge cases. The tests verify both dimension constraints and even-number enforcement, which is critical for video encoding.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/recording/src/instant_recording.rs (1)
401-465: Consider refining overlapping conditions at 9:16 boundary.The aspect ratio conditions have a minor overlap: when
inputhas exactly a 9:16 aspect ratio andinput.0 < input.1, both case 2 (line 418) and case 4 (line 448) match because case 2 uses>=and case 4 uses<=. The first match (case 2) wins, and both produce identical results at this boundary, so there's no correctness issue—just a code smell.Consider making the conditions mutually exclusive:
- else if input.0 <= input.1 && (input.0 as f64 / input.1 as f64) >= 9.0 / 16.0 { + else if input.0 <= input.1 && (input.0 as f64 / input.1 as f64) > 9.0 / 16.0 {Or handle the exact 9:16 case explicitly with a comment noting the design choice.
Additionally, the function name
clamp_sizemight be slightly misleading—the function performs adaptive aspect-ratio-preserving scaling rather than hard clamping (e.g., ultrawide outputs can exceedmax.0). Consider adding a doc comment clarifying the behavior:/// Scales `input` dimensions to fit within `max` bounds while preserving aspect ratio. /// For ultrawide/ultratall displays, one dimension may exceed the corresponding `max` /// value to maintain the aspect ratio. The `max` parameter assumes a 16:9 landscape /// reference frame.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/recording/src/instant_recording.rs(12 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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/recording/src/instant_recording.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/recording/src/instant_recording.rs
🧬 Code graph analysis (1)
crates/recording/src/instant_recording.rs (4)
crates/recording/src/capture_pipeline.rs (7)
output(58-58)output(62-62)output(65-66)make_instant_mode_pipeline(20-28)make_instant_mode_pipeline(47-70)make_instant_mode_pipeline(95-127)target_to_display_and_crop(136-226)crates/media-info/src/lib.rs (1)
from_raw_ffmpeg(211-219)crates/recording/src/sources/screen_capture/mod.rs (2)
display(67-73)init(285-342)crates/recording/src/output_pipeline/core.rs (3)
video_info(591-593)video_info(634-636)video_info(699-699)
⏰ 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: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (rust)
🔇 Additional comments (6)
crates/recording/src/instant_recording.rs (6)
23-26: LGTM: Clean separation of computed video info.Storing the computed
video_infoin thePipelinestruct eliminates the need to recompute it fromscreen_sourcelater, ensuring consistency throughout the actor lifecycle.
230-238: LGTM: Correct video_info construction.Properly constructs
video_infousing the scaledoutput_resolutiondimensions while preserving the originalpixel_formatandfpsfromscreen_info.
247-283: LGTM: Builder pattern correctly extended.The
max_output_sizefield andwith_max_output_sizemethod follow the established builder pattern conventions.
314-392: LGTM: Properly threads max_output_size through actor creation.The function correctly:
- Accepts
max_output_sizeparameter- Passes it to
create_pipeline(line 359)- Uses the computed
pipeline.video_info(line 363) instead of recomputing fromscreen_source- Stores
video_infoin the Actor (line 372)
467-571: LGTM: Comprehensive test coverage.Excellent test suite covering:
- Standard aspect ratios (16:9, 9:16, 4:3)
- Ultrawide and ultratall scenarios
- Boundary conditions (exact 16:9 and 9:16 ratios)
- Even dimension enforcement
- Edge cases (minimum sizes, inputs smaller than max)
The tests provide strong validation of the complex aspect ratio logic.
207-217: Duplication resolved and aspect ratio logic is sound.The
clamp_sizehelper function correctly handles multiple aspect ratios (landscape 16:9, portrait 9:16, ultrawide, and ultratall) while preserving input aspect ratios and enforcing even dimensions for video encoding. Comprehensive test coverage validates all scenarios including boundary cases and scaling. No aspect ratio issues identified in the implementation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/enc-ffmpeg/src/video/h264.rs (1)
98-104: Simplify usingis_some_andfor better idiom.The
map_or(false, ...)pattern can be replaced with the more idiomaticis_some_and.Apply this diff:
let encoder_supports_input_format = codec .video() .ok() .and_then(|codec_video| codec_video.formats()) - .map_or(false, |mut formats| { + .is_some_and(|mut formats| { formats.any(|f| f == input_config.pixel_format) });apps/desktop/src/routes/(window-chrome)/settings/general.tsx (1)
64-65: Consider removing or properly extending the type alias.The type alias
ExtendedGeneralSettingsStoreis a direct alias ofGeneralSettingsStoreand doesn't actually extend it. The name is misleading since it suggests additional type constraints. If you want to makeinstantModeMaxResolutionnon-optional in the local state, use:-type ExtendedGeneralSettingsStore = GeneralSettingsStore; +type ExtendedGeneralSettingsStore = GeneralSettingsStore & { + instantModeMaxResolution: number; +};Otherwise, remove the alias and use
GeneralSettingsStoredirectly throughout.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/desktop/src/routes/(window-chrome)/settings/general.tsx(8 hunks)crates/enc-ffmpeg/src/video/h264.rs(8 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.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-ffmpeg/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-ffmpeg/src/video/h264.rs
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use a 2-space indent for TypeScript code.
Use Biome for formatting and linting TypeScript/JavaScript files by runningpnpm format.Use strict TypeScript and avoid any; leverage shared types
Files:
apps/desktop/src/routes/(window-chrome)/settings/general.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use kebab-case for filenames for TypeScript/JavaScript modules (e.g.,user-menu.tsx).
Use PascalCase for React/Solid components.
Files:
apps/desktop/src/routes/(window-chrome)/settings/general.tsx
apps/desktop/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/desktop/src/**/*.{ts,tsx}: Desktop icons are auto-imported (unplugin-icons); do not import icons manually
Desktop IPC: Call generated tauri_specta commands/events; listen to generated events and use typed interfaces
Use @tanstack/solid-query for server state in the desktop app
Files:
apps/desktop/src/routes/(window-chrome)/settings/general.tsx
🧬 Code graph analysis (2)
crates/enc-ffmpeg/src/video/h264.rs (3)
crates/scap-direct3d/src/lib.rs (2)
pixel_format(415-417)pixel_format(524-526)crates/recording/src/sources/screen_capture/macos.rs (1)
pixel_format(32-34)crates/recording/src/sources/screen_capture/windows.rs (1)
pixel_format(48-50)
apps/desktop/src/routes/(window-chrome)/settings/general.tsx (1)
apps/desktop/src/utils/tauri.ts (1)
GeneralSettingsStore(396-396)
🪛 GitHub Check: Clippy
crates/enc-ffmpeg/src/video/h264.rs
[warning] 98-104: this map_or can be simplified
warning: this map_or can be simplified
--> crates/enc-ffmpeg/src/video/h264.rs:98:45
|
98 | let encoder_supports_input_format = codec
| _______________________________^
99 | | .video()
100 | | .ok()
101 | | .and_then(|codec_video| codec_video.formats())
102 | | .map_or(false, |mut formats| {
103 | | formats.any(|f| f == input_config.pixel_format)
104 | | });
| |^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_map_or
= note: #[warn(clippy::unnecessary_map_or)] on by default
help: use is_some_and instead
|
102 - .map_or(false, |mut formats| {
102 + .is_some_and(|mut formats| {
|
⏰ 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 (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (10)
crates/enc-ffmpeg/src/video/h264.rs (7)
15-20: LGTM! Clean helper for 4:2:0 detection.The implementation correctly identifies 4:2:0 chroma subsampling by checking both horizontal and vertical chroma decimation factors, and safely handles formats without descriptors.
70-77: LGTM! Appropriate validation scope.Correctly validates only non-zero dimensions here, deferring format-specific even-dimension checks to
build()where the output format is determined.
106-170: LGTM! Robust conversion and scaling logic.The implementation correctly:
- Determines pixel conversion needs and falls back to NV12 when necessary
- Validates even dimensions only for 4:2:0 formats using the
is_420helper- Detects scaling requirements and logs appropriately
- Selects proper scaling flags (BICUBIC for quality when scaling, FAST_BILINEAR for fast pixel-only conversion)
- Handles converter creation errors with appropriate error types
180-192: LGTM! Correct encoder configuration.Properly applies output dimensions and format to the encoder, and calculates bitrate based on the actual output resolution.
205-222: LGTM! Cleaner struct design.Replacing the
configfield with specificoutput_format,output_width, andoutput_heightfields improves clarity and reduces the struct's footprint to only what's needed for frame processing.
249-258: LGTM! Correct frame conversion flow.The conversion logic properly:
- Allocates a new frame with output dimensions and format
- Preserves the original PTS to maintain timing
- Updates the frame reference for encoding
327-334: LGTM! Safe bitrate calculation.Using
f64for intermediate calculations prevents overflow that could occur withu32arithmetic, especially for high resolutions.apps/desktop/src/routes/(window-chrome)/settings/general.tsx (3)
76-76: LGTM! Good default value for instant mode resolution.The default of 1920 (1080p width) is a sensible choice that balances quality and file size.
433-444: LGTM! Well-integrated setting component.The instant mode resolution setting follows the established pattern for select settings and integrates cleanly into the Recording section. The fallback value ensures the setting always has a valid default.
91-99: Backend correctly interprets width values and applies 16:9 aspect ratio.Verification confirms the backend handles
max_output_sizeas a width parameter (line 212 incrates/recording/src/instant_recording.rs). The height is computed from width using the 16:9 ratio at line 213:(max_output_size as f64 / 16.0 * 9.0) as u32. All frontend resolution options (1280, 1920, 2560, 3840) align with expected width values for their labeled heights (720p, 1080p, 1440p, 4K).
This pull request introduces support for configurable output resolution in Instant Mode recordings. Users can now select their preferred resolution for instant recordings in general settings, and the backend will scale the video output accordingly. The changes span both the frontend and backend to add this new setting, propagate it through the recording pipeline, and ensure correct video scaling.
Instant.Mode.resolution.mp4
Summary by CodeRabbit
New Features
Bug Fixes / Improvements