Skip to content

Handle windows rgb types properly #1026

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Brendonovich merged 3 commits into from
Sep 18, 2025
Merged

Handle windows rgb types properly #1026

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fc214d7
handle windows rgb types properly
Brendonovich Sep 18, 2025
c869b33
don't invert rgb24
Brendonovich Sep 18, 2025
65357f6
fix
Brendonovich Sep 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions crates/camera-directshow/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ windows = { workspace = true, features = [
"Win32_System_Com_StructuredStorage",
"Win32_System_Ole",
"Win32_System_Variant",
"Win32_System_Performance",
"Win32_Media_KernelStreaming",
] }

Expand Down
2 changes: 1 addition & 1 deletion crates/camera-ffmpeg/examples/cli.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ fn main() {

let _handle = selected_camera
.start_capturing(selected_format.0, |frame| {
let Ok(ff_frame) = frame.to_ffmpeg() else {
let Ok(ff_frame) = frame.as_ffmpeg() else {
eprintln!("Failed to convert frame to FFmpeg");
return;
};
Expand Down
2 changes: 1 addition & 1 deletion crates/camera-ffmpeg/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,5 +11,5 @@ pub use windows::*;
pub trait CapturedFrameExt {
/// Creates an ffmpeg video frame from the native frame.
/// Only size, format, and data are set.
fn to_ffmpeg(&self) -> Result<ffmpeg::frame::Video, ToFfmpegError>;
fn as_ffmpeg(&self) -> Result<ffmpeg::frame::Video, AsFFmpegError>;
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Sep 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

🧩 Analysis chain

Breaking rename: ensure all call sites migrated from to_ffmpeg to as_ffmpeg.

This is a public API change; sweep the workspace for stragglers.

Additionally, sanity‑check Windows RGB handling as part of this PR’s objective (RGB24 row width and Y inversion).

I can propose a fix if the checks above surface issues.

🏁 Script executed:

#!/bin/bash
rg -nP '\bto_ffmpeg\s*\(' -S -C2

Length of output: 402

🏁 Script executed:

#!/bin/bash
# Expect row_width = width * 3 for RGB24; flag suspicious width*4
rg -nP 'PixelFormat::RGB24[\s\S]*row_width\s*=\s*width\s*\*\s*4' -C3 crates/camera-ffmpeg/src/windows.rs
# Expect per-row indexing; flag constant starting offset without y
rg -nP 'PixelFormat::RGB24[\s\S]*src_row\s*=\s*&bytes\[\s*height\s*\*\s*row_width' -C3 crates/camera-ffmpeg/src/windows.rs

Length of output: 0

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== to_ffmpeg occurrences =="
rg -n --hidden -S '\bto_ffmpeg\b' -C2 || true
echo

echo "== as_ffmpeg occurrences =="
rg -n --hidden -S '\bas_ffmpeg\b' -C2 || true
echo

echo "== ToFfmpeg / AsFFmpegError / ToFfmpegError occurrences =="
rg -n --hidden -S 'ToFfmpeg|ToFfmpegError|AsFFmpegError|AsFfmpeg|AsFfmpegError' -C2 || true
echo

echo "== camera-ffmpeg src listing =="
ls -la crates/camera-ffmpeg/src || true
echo

echo "== Search for RGB24 / row_width / src_row patterns in crate =="
rg -n --hidden -S 'RGB24|row_width|src_row|height \* row_width|row_width\s*=' crates/camera-ffmpeg -C3 || true
echo

echo "== PixelFormat occurrences in crate =="
rg -n --hidden -S 'PixelFormat' crates/camera-ffmpeg -C3 || true
echo

if [ -f crates/camera-ffmpeg/src/windows.rs ]; then
  echo "== windows.rs matches =="
  rg -n -S 'RGB24|row_width|src_row|height \* row_width|row_width\s*=' crates/camera-ffmpeg/src/windows.rs -C3 || true
  echo
  echo "== windows.rs head (first 240 lines) =="
  sed -n '1,240p' crates/camera-ffmpeg/src/windows.rs || true
else
  echo "windows.rs not found at crates/camera-ffmpeg/src/windows.rs"
  echo "Searching for windows.rs anywhere in repo..."
  find . -type f -name 'windows.rs' -print || true
fi

Length of output: 41545

Rename migrated; docs update + critical Windows RGB24 bug

  • API rename: all code call-sites use as_ffmpeg; remaining reference is in docs — crates/camera-ffmpeg/README.md (lines 22–23) mentions CapturedFrameExt::to_ffmpeg() and ToFfmpegError — update to as_ffmpeg / AsFFmpegError.
  • Critical bug: crates/camera-ffmpeg/src/windows.rs — PixelFormat::RGB24 arm (≈lines 101–112) sets row_width = width * 4 and src_row = &bytes[height * row_width..] (constant offset). Change to 3 bytes/pixel and per-row indexing (e.g. row_width = width * 3 and src_row = &bytes[y * row_width..] or &bytes[(height - y - 1) * row_width..] to match ARGB/RGB32 inversion). Current code will copy incorrect byte counts and can cause pixel corruption / OOB.
  • Can provide a patch for the RGB24 branch.
🤖 Prompt for AI Agents 
crates/camera-ffmpeg/src/lib.rs (around line 14); docs:
crates/camera-ffmpeg/README.md (lines 22–23); windows impl:
crates/camera-ffmpeg/src/windows.rs (around lines 101–112): update the README to
rename the documented API from CapturedFrameExt::to_ffmpeg() and ToFfmpegError
to as_ffmpeg() and AsFFmpegError respectively; in the Windows RGB24 branch
change the per-pixel math to 3 bytes/pixel and compute per-row offsets (e.g.
row_width = width * 3 and use &bytes[y * row_width ..] or &bytes[(height - y -
1) * row_width ..] depending on whether the source scanlines are inverted) so
each src_row/slice uses the correct start for that row and you copy width*3
bytes per row, and add bounds checks to avoid OOB.

}
Comment on lines 11 to 15
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Sep 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Gate the trait on supported platforms to avoid missing-type compile errors on others.

AsFFmpegError is re-exported only on macOS/Windows. On other targets, this trait references an undefined type.

Apply:

- pub trait CapturedFrameExt {
+ #[cfg(any(target_os = "macos", target_os = "windows"))]
+ pub trait CapturedFrameExt {
     /// Creates an ffmpeg video frame from the native frame.
     /// Only size, format, and data are set.
     fn as_ffmpeg(&self) -> Result<ffmpeg::frame::Video, AsFFmpegError>;
 }

Optionally, also make the windows module gating consistent with macOS by using target_os = "windows".

📝 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.

Suggested change
pub trait CapturedFrameExt {
/// Creates an ffmpeg video frame from the native frame.
/// Only size, format, and data are set.
fn to_ffmpeg(&self) -> Result<ffmpeg::frame::Video, ToFfmpegError>;
fn as_ffmpeg(&self) -> Result<ffmpeg::frame::Video, AsFFmpegError>;
}
#[cfg(any(target_os = "macos", target_os = "windows"))]
pub trait CapturedFrameExt {
/// Creates an ffmpeg video frame from the native frame.
/// Only size, format, and data are set.
fn as_ffmpeg(&self) -> Result<ffmpeg::frame::Video, AsFFmpegError>;
}
🤖 Prompt for AI Agents 
In crates/camera-ffmpeg/src/lib.rs around lines 11 to 15, the CapturedFrameExt
trait references AsFFmpegError which is only re-exported on macOS/Windows
causing missing-type compile errors on other targets; add a cfg attribute to
gate the trait definition to supported platforms (e.g. #[cfg(any(target_os =
"macos", target_os = "windows"))]) so the trait is only compiled where
AsFFmpegError exists, and optionally make the windows module gating consistent
by using target_os = "windows" wherever the windows-specific items are declared.

6 changes: 3 additions & 3 deletions crates/camera-ffmpeg/src/macos.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,15 @@ use cidre::*;
use crate::CapturedFrameExt;

#[derive(thiserror::Error, Debug)]
pub enum ToFfmpegError {
pub enum AsFFmpegError {
#[error("Unsupported media subtype '{0}'")]
UnsupportedSubType(String),
#[error("{0}")]
Native(#[from] cidre::os::Error),
}

impl CapturedFrameExt for CapturedFrame {
fn to_ffmpeg(&self) -> Result<ffmpeg::frame::Video, ToFfmpegError> {
fn as_ffmpeg(&self) -> Result<ffmpeg::frame::Video, AsFFmpegError> {
let native = self.native().clone();

let width = native.image_buf().width();
Expand Down Expand Up @@ -112,7 +112,7 @@ impl CapturedFrameExt for CapturedFrame {
ff_frame
}
format => {
return Err(ToFfmpegError::UnsupportedSubType(format.to_string()));
return Err(AsFFmpegError::UnsupportedSubType(format.to_string()));
}
};

Expand Down
19 changes: 12 additions & 7 deletions crates/camera-ffmpeg/src/windows.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,18 @@ use ffmpeg::{format::Pixel, frame::Video as FFVideo};
use crate::CapturedFrameExt;

#[derive(thiserror::Error, Debug)]
pub enum ToFfmpegError {
pub enum AsFFmpegError {
#[error("FailedToGetBytes: {0}")]
FailedToGetBytes(windows_core::Error),
}

impl CapturedFrameExt for CapturedFrame {
fn to_ffmpeg(&self) -> Result<ffmpeg::frame::Video, ToFfmpegError> {
fn as_ffmpeg(&self) -> Result<ffmpeg::frame::Video, AsFFmpegError> {
let native = self.native();
let width = native.width;
let height = native.height;

let bytes = native.bytes().map_err(ToFfmpegError::FailedToGetBytes)?;
let bytes = native.bytes().map_err(AsFFmpegError::FailedToGetBytes)?;

Ok(match native.pixel_format {
PixelFormat::YUV420P => {
Expand Down Expand Up @@ -79,13 +79,18 @@ impl CapturedFrameExt for CapturedFrame {
ff_frame
}
PixelFormat::ARGB => {
let mut ff_frame = FFVideo::new(Pixel::ARGB, width as u32, height as u32);
let mut ff_frame = FFVideo::new(
// ik it's weird but that's how windows works
Pixel::BGRA,
width as u32,
height as u32,
);

let stride = ff_frame.stride(0);

for y in 0..height {
let row_width = width * 4;
let src_row = &bytes[y * row_width..];
let src_row = &bytes[(height - y - 1) * row_width..];
let dest_row = &mut ff_frame.data_mut(0)[y * stride..];

dest_row[0..row_width].copy_from_slice(&src_row[0..row_width]);
Expand All @@ -100,7 +105,7 @@ impl CapturedFrameExt for CapturedFrame {

for y in 0..height {
let row_width = width * 4;
let src_row = &bytes[y * row_width..];
let src_row = &bytes[height * row_width..];
let dest_row = &mut ff_frame.data_mut(0)[y * stride..];

dest_row[0..row_width].copy_from_slice(&src_row[0..row_width]);
Expand All @@ -115,7 +120,7 @@ impl CapturedFrameExt for CapturedFrame {

for y in 0..height {
let row_width = width * 4;
let src_row = &bytes[y * row_width..];
let src_row = &bytes[(height - y - 1) * row_width..];
let dest_row = &mut ff_frame.data_mut(0)[y * stride..];

dest_row[0..row_width].copy_from_slice(&src_row[0..row_width]);
Expand Down
1 change: 1 addition & 0 deletions crates/recording/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ cap-enc-avfoundation = { path = "../enc-avfoundation" }
cap-enc-mediafoundation = { path = "../enc-mediafoundation" }
cap-mediafoundation-ffmpeg = { path = "../mediafoundation-ffmpeg" }
cap-mediafoundation-utils = { path = "../mediafoundation-utils" }
cap-camera-windows = { path = "../camera-windows" }
windows = { workspace = true, features = [
"Win32_Foundation",
"Win32_Graphics_Gdi",
Expand Down
52 changes: 28 additions & 24 deletions crates/recording/examples/recording-cli.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,35 +25,39 @@ pub async fn main() {

info!("Recording to directory '{}'", dir.path().display());

// let camera_feed = CameraFeed::spawn(CameraFeed::default());
let camera_info = cap_camera::list_cameras()
.find(|c| c.display_name().contains("NVIDIA"))
.unwrap();

// camera_feed
// .ask(camera::SetInput {
// id: DeviceOrModelID::from_info(&cap_camera::list_cameras().next().unwrap()),
// })
// .await
// .unwrap()
// .await
// .unwrap();
let camera_feed = CameraFeed::spawn(CameraFeed::default());

let (error_tx, _) = flume::bounded(1);
let mic_feed = MicrophoneFeed::spawn(MicrophoneFeed::new(error_tx));

mic_feed
.ask(microphone::SetInput {
label:
// MicrophoneFeed::list()
// .into_iter()
// .find(|(k, _)| k.contains("Focusrite"))
MicrophoneFeed::default()
.map(|v| v.0)
.unwrap(),
camera_feed
.ask(feeds::camera::SetInput {
id: feeds::camera::DeviceOrModelID::from_info(&camera_info),
})
.await
.unwrap()
.await
.unwrap();

// let (error_tx, _) = flume::bounded(1);
// let mic_feed = MicrophoneFeed::spawn(MicrophoneFeed::new(error_tx));

// mic_feed
// .ask(microphone::SetInput {
// label:
// // MicrophoneFeed::list()
// // .into_iter()
// // .find(|(k, _)| k.contains("Focusrite"))
// MicrophoneFeed::default()
// .map(|v| v.0)
// .unwrap(),
// })
// .await
// .unwrap()
// .await
// .unwrap();

tokio::time::sleep(Duration::from_millis(10)).await;

let (handle, _ready_rx) = studio_recording::Actor::builder(
Expand All @@ -63,9 +67,9 @@ pub async fn main() {
},
)
.with_system_audio(true)
// .with_mic_feed(std::sync::Arc::new(
// mic_feed.ask(microphone::Lock).await.unwrap(),
// ))
.with_camera_feed(std::sync::Arc::new(
camera_feed.ask(feeds::camera::Lock).await.unwrap(),
))
.build()
.await
.unwrap();
Expand Down
17 changes: 15 additions & 2 deletions crates/recording/src/feeds/camera.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ use std::{
time::Duration,
};
use tokio::{runtime::Runtime, sync::oneshot, task::LocalSet};
use tracing::{debug, error, trace, warn};
use tracing::{debug, error, info, trace, warn};

const CAMERA_INIT_TIMEOUT: Duration = Duration::from_secs(4);

Expand Down Expand Up @@ -59,6 +59,12 @@ impl OpenState {
if let Some(connecting) = &self.connecting
&& id == connecting.id
{
if let Some(attached) = self.attached.take() {
let _ = attached.done_tx.send(());
}

trace!("Attaching new camera");

self.attached = Some(AttachedState {
id,
camera_info: data.camera_info,
Expand Down Expand Up @@ -256,14 +262,15 @@ async fn setup_camera(
});

let format = ideal_formats.swap_remove(0);

let frame_rate = format.frame_rate() as u32;

let (ready_tx, ready_rx) = oneshot::channel();
let mut ready_signal = Some(ready_tx);

let capture_handle = camera
.start_capturing(format.clone(), move |frame| {
let Ok(mut ff_frame) = frame.to_ffmpeg() else {
let Ok(mut ff_frame) = frame.as_ffmpeg() else {
return;
};

Expand Down Expand Up @@ -375,9 +382,15 @@ impl Message<SetInput> for CameraFeed {
}
};

trace!("Waiting for camera to be done");

let _ = done_rx.recv();

trace!("Stoppping capture of {:?}", &id);

let _ = handle.stop_capturing();

info!("Stopped capture of {:?}", &id);
})
});

Expand Down
2 changes: 1 addition & 1 deletion packages/ui-solid/src/auto-imports.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ declare global {
const IconCapSettings: typeof import('~icons/cap/settings.jsx')['default']
const IconCapShadow: typeof import('~icons/cap/shadow.jsx')['default']
const IconCapSquare: typeof import('~icons/cap/square.jsx')['default']
const IconCapStopCircle: typeof import("~icons/cap/stop-circle.jsx")["default"]
const IconCapStopCircle: typeof import('~icons/cap/stop-circle.jsx')['default']
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Sep 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

🧩 Analysis chain

Formatting-only change in a generated file; OK, but avoid hand-editing artifacts.

Swapping to single quotes matches the prevailing style in this file and is no-op semantically. Since this file is generated by unplugin-auto-import, prefer regenerating rather than hand edits to prevent drift and churn.

Run this to check for remaining double-quoted imports so you can fix at the generator/config level instead of manual edits:

🏁 Script executed:

#!/bin/bash
# List any lines still using double-quoted typeof import() entries
rg -nP 'typeof import\("~icons/.+?"\)\["default"\]' packages/ui-solid/src/auto-imports.d.ts || true

Length of output: 476

Regenerate auto-imports with single-quote imports

There are still double-quoted typeof import() entries in packages/ui-solid/src/auto-imports.d.ts — change the auto-import generator/config to emit single quotes and regenerate the file (avoid hand edits).

Occurrences:

  • packages/ui-solid/src/auto-imports.d.ts:24 — const IconCapEditor: typeof import("~icons/cap/editor.jsx")["default"]
  • packages/ui-solid/src/auto-imports.d.ts:59 — const IconCapUpload: typeof import("~icons/cap/upload.jsx")["default"]
  • packages/ui-solid/src/auto-imports.d.ts:69 — const IconLucideDatabase: typeof import("~icons/lucide/database.jsx")["default"]
  • packages/ui-solid/src/auto-imports.d.ts:71 — const IconLucideEye: typeof import("~icons/lucide/eye.jsx")["default"]
  • packages/ui-solid/src/auto-imports.d.ts:88 — const IconLucideVolumeX: typeof import("~icons/lucide/volume-x.jsx")["default"]
  • packages/ui-solid/src/auto-imports.d.ts:90 — const IconMdiLoading: typeof import("~icons/mdi/loading.jsx")["default"]
🤖 Prompt for AI Agents 
packages/ui-solid/src/auto-imports.d.ts around line 56 (and other occurrences
listed) — the auto-generated type imports use double quotes (e.g. typeof
import("~icons/…")["default"]); update the auto-import generator/config to emit
single-quoted module specifiers (typeof import('~icons/…')['default']) and then
regenerate the auto-imports.d.ts file (do not hand-edit the file); run the
generator/build step that produces auto-imports.d.ts and commit the regenerated
file so all occurrences are corrected.

const IconCapTrash: typeof import('~icons/cap/trash.jsx')['default']
const IconCapUndo: typeof import('~icons/cap/undo.jsx')['default']
const IconCapUpload: typeof import("~icons/cap/upload.jsx")["default"]
Expand Down
Loading