fix: Add handling for missing VideoFormat on Linux #169
Conversation
WalkthroughThe Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| VideoFormat::RGBA => user_data.tx.send(Frame::RGBx(RGBxFrame { | ||
| display_time: timestamp as u64, | ||
| width: frame_size.width as i32, | ||
| height: frame_size.height as i32, | ||
| data: frame_data, | ||
| })), |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider adding a dedicated Frame::RGBA variant for semantic clarity.
The fix correctly addresses the missing handling for VideoFormat::RGBA and follows the established pattern. However, mapping RGBA frames to Frame::RGBx might be semantically confusing since RGBA includes meaningful alpha channel data while RGBx typically has undefined alpha values.
Consider these alternatives:
- Add a dedicated
Frame::RGBAvariant in the frame module - If
Frame::RGBxis intended to handle both RGBx and RGBA formats, consider renaming it to be more generic
Verify that downstream frame processing code can handle RGBA data correctly when it receives Frame::RGBx frames that actually contain alpha channel information.
🏁 Script executed:
#!/bin/bash
# Description: Check if Frame enum has RGBA variant and how RGBx frames are processed downstream
# Check Frame enum definition
ast-grep --pattern 'enum Frame {
$$$
}'
# Check how Frame::RGBx is processed in the codebase
rg -A 10 "Frame::RGBx" --type rustLength of output: 2362
#!/bin/bash
set -e
echo "### linux mod imports ###"
sed -n '1,50p' src/capturer/engine/linux/mod.rs
echo
echo "### VideoFrame enum ###"
rg -A 20 "enum VideoFrame" --type rust
echo
echo "### Frame alias ###"
rg "type Frame" -A 5 --type rust
echo
echo "### RGBxFrame struct ###"
rg -A 10 "struct RGBxFrame" --type rust
echo
echo "### RGBAFrame struct ###"
rg -A 10 "struct RGBAFrame" --type rust
🏁 Script executed:
#!/bin/bash
set -e
echo "### Imports in src/capturer/engine/linux/mod.rs ###"
sed -n '1,50p' src/capturer/engine/linux/mod.rs || echo "File not found"
echo
echo "### Searching for 'enum Frame' definitions ###"
rg -n "enum Frame" --type rust || echo "enum Frame not found"
echo
echo "### Searching for 'type Frame' alias ###"
rg -n "type Frame" --type rust || echo "type Frame alias not found"
echo
echo "### Searching for 'struct RGBxFrame' ###"
rg -n "struct RGBxFrame" --type rust || echo "RGBxFrame struct not found"
echo
echo "### Searching for 'struct RGBAFrame' ###"
rg -n "struct RGBAFrame" --type rust || echo "RGBAFrame struct not found"Length of output: 2189
🏁 Script executed:
#!/bin/bash
set -e
echo "### Displaying Frame enum in src/frame/mod.rs ###"
sed -n '1,200p' src/frame/mod.rsLength of output: 275
🏁 Script executed:
#!/bin/bash
set -e
echo "### Displaying src/frame/video.rs ###"
sed -n '1,200p' src/frame/video.rs || echo "src/frame/video.rs not found"Length of output: 4357
Implement dedicated RGBA frame variant for clarity
The current change handles the missing VideoFormat::RGBA case, but mapping true RGBA frames into the RGBx variant conflates two distinct formats. RGBA carries a meaningful alpha channel, while RGBx implies a padded or ignored fourth byte. To avoid downstream confusion, we should:
- Define a new
RGBAFramestruct and correspondingVideoFrame::RGBA(RGBAFrame)variant insrc/frame/video.rs. - Update the match arm in
src/capturer/engine/linux/mod.rsto emitFrame::Video(VideoFrame::RGBA(...))instead ofFrame::RGBx. - If alpha really isn’t used downstream, explicitly strip it (e.g. via
remove_alpha_channel) and map toRGBorRGBx, making the drop deliberate.
Suggested diffs:
// src/frame/video.rs
use std::time::SystemTime;
+#[derive(Debug, Clone)]
+pub struct RGBAFrame {
+ pub display_time: SystemTime,
+ pub width: i32,
+ pub height: i32,
+ pub data: Vec<u8>, // Red, Green, Blue, Alpha
+}
#[derive(Debug, Clone)]
pub enum VideoFrame {
YUVFrame(YUVFrame),
RGB(RGBFrame),
RGBx(RGBxFrame),
XBGR(XBGRFrame),
BGRx(BGRxFrame),
BGR0(BGRFrame),
BGRA(BGRAFrame),
+ RGBA(RGBAFrame),
}// src/capturer/engine/linux/mod.rs
match video_info.format {
VideoFormat::RGBA => user_data.tx.send(
- Frame::RGBx(RGBxFrame {
+ Frame::Video(VideoFrame::RGBA(RGBAFrame {
display_time: timestamp as u64,
width: frame_size.width as i32,
height: frame_size.height as i32,
data: frame_data,
- }))
+ })),
),
_ => panic!("Unsupported frame format received"),
}If alpha is guaranteed unused, explicitly drop it:
let rgb_data = remove_alpha_channel(frame_data);
user_data.tx.send(Frame::Video(VideoFrame::RGB(RGBFrame {
display_time: timestamp as u64,
width: frame_size.width as i32,
height: frame_size.height as i32,
data: rgb_data,
})))?;This preserves clear semantics around alpha handling and keeps downstream frame consumers unambiguous.
🤖 Prompt for AI Agents
In src/capturer/engine/linux/mod.rs around lines 161 to 166, the code
incorrectly maps VideoFormat::RGBA frames to the RGBx variant, which conflates
distinct formats and can cause confusion about alpha channel usage. To fix this,
define a new RGBAFrame struct and add a corresponding VideoFrame::RGBA variant
in src/frame/video.rs. Then update the match arm in
src/capturer/engine/linux/mod.rs to send Frame::Video(VideoFrame::RGBA(...))
with the RGBAFrame data. If the alpha channel is not used downstream, explicitly
remove it before mapping to RGB or RGBx to make the alpha drop deliberate and
clear.
In the current code, VideoFormat::RGBA is included in the list of output formats for the buffer
scap/src/capturer/engine/linux/mod.rs
Lines 213 to 222 in 92cabc5
but the case where VideoFormat is VideoFormat::RGBA is not handled
scap/src/capturer/engine/linux/mod.rs
Lines 136 to 164 in 92cabc5
This PR will fix it.
Summary by CodeRabbit